From 2636de902af877decc3b019ef7b8c4b25891de2d Mon Sep 17 00:00:00 2001 From: Kyle Carberry Date: Tue, 21 Mar 2023 20:03:21 +0000 Subject: [PATCH 1/5] chore: increase parallelism of TestWorkspaceQuota This does a lot of build operations, so having multiple provisioner daemons is great. We were actually approaching the ceiling here for test duration! --- enterprise/coderd/workspacequota_test.go | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/enterprise/coderd/workspacequota_test.go b/enterprise/coderd/workspacequota_test.go index ed4b5448fe0a4..c1695c814b74c 100644 --- a/enterprise/coderd/workspacequota_test.go +++ b/enterprise/coderd/workspacequota_test.go @@ -2,9 +2,11 @@ package coderd_test import ( "context" + "sync" "testing" "github.com/google/uuid" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/coder/coder/coderd/coderdtest" @@ -37,12 +39,12 @@ func TestWorkspaceQuota(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) defer cancel() max := 1 - client := coderdenttest.New(t, &coderdenttest.Options{ + client, _, api := coderdenttest.NewWithAPI(t, &coderdenttest.Options{ UserWorkspaceQuota: max, - Options: &coderdtest.Options{ - IncludeProvisionerDaemon: true, - }, }) + coderdtest.NewProvisionerDaemon(t, api.AGPL) + coderdtest.NewProvisionerDaemon(t, api.AGPL) + coderdtest.NewProvisionerDaemon(t, api.AGPL) user := coderdtest.CreateFirstUser(t, client) coderdenttest.AddLicense(t, client, coderdenttest.LicenseOptions{ @@ -104,12 +106,18 @@ func TestWorkspaceQuota(t *testing.T) { template := coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID) // Spin up three workspaces fine + var wg sync.WaitGroup for i := 0; i < 3; i++ { - workspace := coderdtest.CreateWorkspace(t, client, user.OrganizationID, template.ID) - build := coderdtest.AwaitWorkspaceBuildJob(t, client, workspace.LatestBuild.ID) - verifyQuota(ctx, t, client, i+1, 3) - require.Equal(t, codersdk.WorkspaceStatusRunning, build.Status) + wg.Add(1) + go func() { + defer wg.Done() + workspace := coderdtest.CreateWorkspace(t, client, user.OrganizationID, template.ID) + build := coderdtest.AwaitWorkspaceBuildJob(t, client, workspace.LatestBuild.ID) + assert.Equal(t, codersdk.WorkspaceStatusRunning, build.Status) + }() } + wg.Wait() + verifyQuota(ctx, t, client, 3, 3) // Next one must fail workspace := coderdtest.CreateWorkspace(t, client, user.OrganizationID, template.ID) From a8e256ac499ffdab30f92697dbf6eef0a43b2840 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 21 Mar 2023 16:30:39 -0500 Subject: [PATCH 2/5] Seralizable transactions should retry automatically --- coderd/database/db.go | 36 +++++++++++++++++++++++++++++++++++- coderd/database/db_test.go | 31 +++++++++++++++++++++++++++++++ coderd/database/errors.go | 8 ++++++++ 3 files changed, 74 insertions(+), 1 deletion(-) diff --git a/coderd/database/db.go b/coderd/database/db.go index f8de976e92f72..72b9ab3e7f174 100644 --- a/coderd/database/db.go +++ b/coderd/database/db.go @@ -67,8 +67,42 @@ func (q *sqlQuerier) Ping(ctx context.Context) (time.Duration, error) { return time.Since(start), err } -// InTx performs database operations inside a transaction. func (q *sqlQuerier) InTx(function func(Store) error, txOpts *sql.TxOptions) error { + _, inTx := q.db.(*sqlx.Tx) + isolation := sql.LevelDefault + if txOpts != nil { + isolation = txOpts.Isolation + } + + // If we are not already in a transaction, and we are running in serializable + // mode, we need to run the transaction in a retry loop. The caller should be + // prepared to allow retries if using serializable mode. + // If we are in a transaction already, the parent InTx call will handle the retry. + // We do not want to duplicate those retries. + if !inTx && isolation == sql.LevelSerializable { + // This is an arbitrarily chosen number. + const retryAmount = 3 + var err error + attempts := 0 + for attempts = 0; attempts < retryAmount; attempts++ { + err = q.runTx(function, txOpts) + if err == nil { + // Transaction succeeded. + return nil + } + if err != nil && !IsSerializedError(err) { + // We should only retry if the error is a serialization error. + return err + } + } + // Transaction kept failing in serializable mode. + return xerrors.Errorf("transaction failed after %d attempts: %w", attempts, err) + } + return q.runTx(function, txOpts) +} + +// InTx performs database operations inside a transaction. +func (q *sqlQuerier) runTx(function func(Store) error, txOpts *sql.TxOptions) error { if _, ok := q.db.(*sqlx.Tx); ok { // If the current inner "db" is already a transaction, we just reuse it. // We do not need to handle commit/rollback as the outer tx will handle diff --git a/coderd/database/db_test.go b/coderd/database/db_test.go index b92138994d99c..c922da979f506 100644 --- a/coderd/database/db_test.go +++ b/coderd/database/db_test.go @@ -8,6 +8,7 @@ import ( "testing" "github.com/google/uuid" + "github.com/lib/pq" "github.com/stretchr/testify/require" "github.com/coder/coder/coderd/database" @@ -15,6 +16,36 @@ import ( "github.com/coder/coder/coderd/database/postgres" ) +func TestSerializedRetry(t *testing.T) { + t.Parallel() + if testing.Short() { + t.SkipNow() + } + + sqlDB := testSQLDB(t) + db := database.New(sqlDB) + + called := 0 + txOpts := &sql.TxOptions{Isolation: sql.LevelSerializable} + err := db.InTx(func(tx database.Store) error { + // Test nested error + return tx.InTx(func(tx database.Store) error { + // The easiest way to mock a serialization failure is to + // return a serialization failure error. + called++ + return &pq.Error{ + Code: "40001", + Message: "serialization_failure", + } + }, txOpts) + }, txOpts) + require.Error(t, err, "should fail") + // The double "execute transaction: execute transaction" is from the nested transactions. + // Just want to make sure we don't try 9 times. + require.Equal(t, err.Error(), "transaction failed after 3 attempts: execute transaction: execute transaction: pq: serialization_failure", "error message") + require.Equal(t, called, 3, "should retry 3 times") +} + func TestNestedInTx(t *testing.T) { t.Parallel() if testing.Short() { diff --git a/coderd/database/errors.go b/coderd/database/errors.go index 9a7c04cd5f528..8de29d7d9092f 100644 --- a/coderd/database/errors.go +++ b/coderd/database/errors.go @@ -6,6 +6,14 @@ import ( "github.com/lib/pq" ) +func IsSerializedError(err error) bool { + var pqErr *pq.Error + if errors.As(err, &pqErr) { + return pqErr.Code.Name() == "serialization_failure" + } + return false +} + // IsUniqueViolation checks if the error is due to a unique violation. // If one or more specific unique constraints are given as arguments, // the error must be caused by one of them. If no constraints are given, From 012e2cb38ebc846882f64aca478c67ad39eaaac4 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 21 Mar 2023 16:45:33 -0500 Subject: [PATCH 3/5] Generator was not handling "nil" slices correctly --- coderd/database/dbgen/take.go | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/coderd/database/dbgen/take.go b/coderd/database/dbgen/take.go index caa4a1b4036db..25512e64e5031 100644 --- a/coderd/database/dbgen/take.go +++ b/coderd/database/dbgen/take.go @@ -1,10 +1,10 @@ package dbgen -import "net" +import ( + "net" +) func takeFirstIP(values ...net.IPNet) net.IPNet { - takeFirstSlice([]string{}) - return takeFirstF(values, func(v net.IPNet) bool { return len(v.IP) != 0 && len(v.Mask) != 0 }) @@ -14,19 +14,22 @@ func takeFirstIP(values ...net.IPNet) net.IPNet { // []any is not a comparable type. func takeFirstSlice[T any](values ...[]T) []T { return takeFirstF(values, func(v []T) bool { - return len(v) != 0 + return v != nil && len(v) != 0 }) } // takeFirstF takes the first value that returns true func takeFirstF[Value any](values []Value, take func(v Value) bool) Value { - var empty Value for _, v := range values { if take(v) { return v } } - // If all empty, return empty + // If all empty, return the last element + if len(values) > 0 { + return values[len(values)-1] + } + var empty Value return empty } From 8769db02fcc1e7f8c07e8e244ea19a6f6e65aba4 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 21 Mar 2023 17:24:13 -0500 Subject: [PATCH 4/5] Linting --- coderd/database/dbgen/take.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/coderd/database/dbgen/take.go b/coderd/database/dbgen/take.go index 25512e64e5031..64bc8ae1e3cdb 100644 --- a/coderd/database/dbgen/take.go +++ b/coderd/database/dbgen/take.go @@ -14,7 +14,7 @@ func takeFirstIP(values ...net.IPNet) net.IPNet { // []any is not a comparable type. func takeFirstSlice[T any](values ...[]T) []T { return takeFirstF(values, func(v []T) bool { - return v != nil && len(v) != 0 + return len(v) != 0 }) } From 443a53f707888f01873760a0952224de7bd711e9 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 21 Mar 2023 17:33:23 -0500 Subject: [PATCH 5/5] Revert dbgen change --- coderd/database/dbgen/take.go | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/coderd/database/dbgen/take.go b/coderd/database/dbgen/take.go index 64bc8ae1e3cdb..caa4a1b4036db 100644 --- a/coderd/database/dbgen/take.go +++ b/coderd/database/dbgen/take.go @@ -1,10 +1,10 @@ package dbgen -import ( - "net" -) +import "net" func takeFirstIP(values ...net.IPNet) net.IPNet { + takeFirstSlice([]string{}) + return takeFirstF(values, func(v net.IPNet) bool { return len(v.IP) != 0 && len(v.Mask) != 0 }) @@ -20,16 +20,13 @@ func takeFirstSlice[T any](values ...[]T) []T { // takeFirstF takes the first value that returns true func takeFirstF[Value any](values []Value, take func(v Value) bool) Value { + var empty Value for _, v := range values { if take(v) { return v } } - // If all empty, return the last element - if len(values) > 0 { - return values[len(values)-1] - } - var empty Value + // If all empty, return empty return empty }