From f7b9f1ce69457fd2a110d529b731411e8fc8dc3c Mon Sep 17 00:00:00 2001 From: Mariell Hoversholm Date: Wed, 26 Mar 2025 14:44:44 +0100 Subject: [PATCH] Unified Storage: Return an already exists error (#102857) * Unified Storage: Return an already exists error When inserting a resource that already exists (i.e. race condition), we can safely catch UNIQUE constraint violations and transform them into a `k8s.io/apimachinery/pkg/api/errors` error that stands the test of `errors.IsAlreadyExists`. * feat: clarify existing conflict error * chore: make update-workspace * feat: make new package for backend error * fix: assign dependency owner * feat: use dialect for checking error type * chore: go generate * revert: to 5af369166d6 --- go.mod | 2 +- pkg/storage/unified/backend/errors.go | 21 ++++++++++++++++ pkg/storage/unified/backend/errors_test.go | 14 +++++++++++ pkg/storage/unified/resource/server.go | 1 + pkg/storage/unified/sql/backend.go | 29 ++++++++++++++++++++++ pkg/storage/unified/sql/backend_test.go | 25 +++++++++++++++++++ 6 files changed, 91 insertions(+), 1 deletion(-) create mode 100644 pkg/storage/unified/backend/errors.go create mode 100644 pkg/storage/unified/backend/errors_test.go diff --git a/go.mod b/go.mod index c9db9cce631..703d8d440ee 100644 --- a/go.mod +++ b/go.mod @@ -396,7 +396,7 @@ require ( github.com/invopop/jsonschema v0.13.0 // indirect github.com/jackc/pgpassfile v1.0.0 // indirect github.com/jackc/pgservicefile v0.0.0-20240606120523-5a60cdf6a761 // indirect - github.com/jackc/pgx/v5 v5.7.2 // indirect + github.com/jackc/pgx/v5 v5.7.2 // @grafana/grafana-search-and-storage github.com/jackc/puddle/v2 v2.2.2 // indirect github.com/jbenet/go-context v0.0.0-20150711004518-d14ea06fba99 // indirect github.com/jcmturner/aescts/v2 v2.0.0 // indirect diff --git a/pkg/storage/unified/backend/errors.go b/pkg/storage/unified/backend/errors.go new file mode 100644 index 00000000000..49a5a232b6a --- /dev/null +++ b/pkg/storage/unified/backend/errors.go @@ -0,0 +1,21 @@ +package backend + +import ( + "net/http" + + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +// Errors that may be returned by the storage backend, to be converted by the resource layer. +// These are defined here to let all storage backends use the same error types without depending on one another. +var ( + ErrResourceAlreadyExists error = &apierrors.StatusError{ + ErrStatus: metav1.Status{ + Status: metav1.StatusFailure, + Reason: metav1.StatusReasonAlreadyExists, + Message: "the resource already exists", + Code: http.StatusConflict, + }, + } +) diff --git a/pkg/storage/unified/backend/errors_test.go b/pkg/storage/unified/backend/errors_test.go new file mode 100644 index 00000000000..644b7342c96 --- /dev/null +++ b/pkg/storage/unified/backend/errors_test.go @@ -0,0 +1,14 @@ +package backend + +import ( + "testing" + + "github.com/stretchr/testify/require" + apierrors "k8s.io/apimachinery/pkg/api/errors" +) + +func TestErrResourceAlreadyExistsIsRecognisable(t *testing.T) { + t.Parallel() + + require.True(t, apierrors.IsAlreadyExists(ErrResourceAlreadyExists), "ErrResourceAlreadyExists should be recognised as an AlreadyExists error") +} diff --git a/pkg/storage/unified/resource/server.go b/pkg/storage/unified/resource/server.go index 0509c938382..aa1b2e044a6 100644 --- a/pkg/storage/unified/resource/server.go +++ b/pkg/storage/unified/resource/server.go @@ -480,6 +480,7 @@ func (s *server) Create(ctx context.Context, req *CreateRequest) (*CreateRespons if found != nil && len(found.Value) > 0 { rsp.Error = &ErrorResult{ Code: http.StatusConflict, + Reason: string(metav1.StatusReasonAlreadyExists), Message: "key already exists", // TODO?? soft delete replace? } return rsp, nil diff --git a/pkg/storage/unified/sql/backend.go b/pkg/storage/unified/sql/backend.go index 674f86a1a03..8e2e0b644bf 100644 --- a/pkg/storage/unified/sql/backend.go +++ b/pkg/storage/unified/sql/backend.go @@ -9,7 +9,11 @@ import ( "sync" "time" + "github.com/go-sql-driver/mysql" "github.com/google/uuid" + unifiedbackend "github.com/grafana/grafana/pkg/storage/unified/backend" + "github.com/jackc/pgx/v5/pgconn" + "github.com/mattn/go-sqlite3" "github.com/prometheus/client_golang/prometheus" "go.opentelemetry.io/otel/trace" "go.opentelemetry.io/otel/trace/noop" @@ -337,6 +341,9 @@ func (b *backend) create(ctx context.Context, event resource.WriteEvent) (int64, Folder: folder, GUID: guid, }); err != nil { + if isRowAlreadyExistsError(err) { + return guid, unifiedbackend.ErrResourceAlreadyExists + } return guid, fmt.Errorf("insert into resource: %w", err) } @@ -377,6 +384,28 @@ func (b *backend) create(ctx context.Context, event resource.WriteEvent) (int64, return rv, nil } +// isRowAlreadyExistsError checks if the error is the result of the row inserted already existing. +func isRowAlreadyExistsError(err error) bool { + var sqlite sqlite3.Error + if errors.As(err, &sqlite) { + return sqlite.ExtendedCode == sqlite3.ErrConstraintUnique + } + + var pg *pgconn.PgError + if errors.As(err, &pg) { + // https://www.postgresql.org/docs/current/errcodes-appendix.html + return pg.Code == "23505" // unique_violation + } + + var mysqlerr *mysql.MySQLError + if errors.As(err, &mysqlerr) { + // https://dev.mysql.com/doc/mysql-errors/8.0/en/server-error-reference.html + return mysqlerr.Number == 1062 // ER_DUP_ENTRY + } + + return false +} + func (b *backend) update(ctx context.Context, event resource.WriteEvent) (int64, error) { ctx, span := b.tracer.Start(ctx, tracePrefix+"Update") defer span.End() diff --git a/pkg/storage/unified/sql/backend_test.go b/pkg/storage/unified/sql/backend_test.go index f275b2b78e5..d4fbc781479 100644 --- a/pkg/storage/unified/sql/backend_test.go +++ b/pkg/storage/unified/sql/backend_test.go @@ -8,10 +8,12 @@ import ( "testing" sqlmock "github.com/DATA-DOG/go-sqlmock" + "github.com/mattn/go-sqlite3" "github.com/stretchr/testify/require" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "github.com/grafana/grafana/pkg/apimachinery/utils" + unifiedbackend "github.com/grafana/grafana/pkg/storage/unified/backend" "github.com/grafana/grafana/pkg/storage/unified/resource" "github.com/grafana/grafana/pkg/storage/unified/sql/db/dbimpl" "github.com/grafana/grafana/pkg/storage/unified/sql/test" @@ -229,6 +231,29 @@ func TestBackend_create(t *testing.T) { require.Equal(t, int64(200), v) }) + t.Run("resource already exists", func(t *testing.T) { + t.Parallel() + b, ctx := setupBackendTest(t) + b.SQLMock.ExpectBegin() + expectSuccessfulResourceVersionExec(t, b.TestDBProvider, + func() { b.ExecWithResult("insert resource", 0, 1) }, + func() { b.ExecWithResult("insert resource_history", 0, 1) }, + ) + b.SQLMock.ExpectCommit() + b.SQLMock.ExpectBegin() + b.SQLMock.ExpectExec("insert resource").WillReturnError(sqlite3.Error{Code: sqlite3.ErrConstraint, ExtendedCode: sqlite3.ErrConstraintUnique}) + b.SQLMock.ExpectRollback() + + // First we insert the resource successfully. This is what the happy path test does as well. + v, err := b.create(ctx, event) + require.NoError(t, err) + require.Equal(t, int64(200), v) + + // Then we try to insert the same resource again. This should fail. + _, err = b.create(ctx, event) + require.ErrorIs(t, err, unifiedbackend.ErrResourceAlreadyExists) + }) + t.Run("error inserting into resource", func(t *testing.T) { t.Parallel() b, ctx := setupBackendTest(t)