From 412091d18cdd8bcb02893dd8b91859b3d58f994f Mon Sep 17 00:00:00 2001 From: Dirkjan Bussink Date: Tue, 29 Aug 2023 07:55:09 +0200 Subject: [PATCH 1/3] Fix setup order to avoid races This ensures that we always set up the sandbox instances first with the correct vschema etc. so that anything that starts goroutines afterwards like `newTestResolver` won't race with reading the vschema while it still might be written. Signed-off-by: Dirkjan Bussink --- go/vt/vtgate/executor_framework_test.go | 34 ++++++++++++++----------- go/vt/vtgate/schema/tracker.go | 4 +++ 2 files changed, 23 insertions(+), 15 deletions(-) diff --git a/go/vt/vtgate/executor_framework_test.go b/go/vt/vtgate/executor_framework_test.go index 4917ce2864a..a1e1e91d53b 100644 --- a/go/vt/vtgate/executor_framework_test.go +++ b/go/vt/vtgate/executor_framework_test.go @@ -132,8 +132,16 @@ func createExecutorEnv(t testing.TB) (executor *Executor, sbc1, sbc2, sbclookup ctx, cancel = context.WithCancel(context.Background()) cell := "aa" hc := discovery.NewFakeHealthCheck(make(chan *discovery.TabletHealth)) + s := createSandbox(KsTestSharded) s.VSchema = executorVSchema + sb := createSandbox(KsTestUnsharded) + sb.VSchema = unshardedVSchema + // Use the 'X' in the name to ensure it's not alphabetically first. + // Otherwise, it would become the default keyspace for the dual table. + bad := createSandbox("TestXBadSharding") + bad.VSchema = badVSchema + serv := newSandboxForCells(ctx, []string{cell}) serv.topoServer.CreateKeyspace(ctx, KsTestSharded, &topodatapb.Keyspace{SidecarDbName: sidecar.DefaultName}) // Force a new cache to use for lookups of the sidecar database identifier @@ -152,6 +160,7 @@ func createExecutorEnv(t testing.TB) (executor *Executor, sbc1, sbc2, sbclookup if !created { log.Fatal("Failed to [re]create a sidecar database identifier cache!") } + resolver := newTestResolver(ctx, hc, serv, cell) sbc1 = hc.AddTestTablet(cell, "-20", 1, "TestExecutor", "-20", topodatapb.TabletType_PRIMARY, true, 1, nil) sbc2 = hc.AddTestTablet(cell, "40-60", 1, "TestExecutor", "40-60", topodatapb.TabletType_PRIMARY, true, 1, nil) @@ -164,16 +173,9 @@ func createExecutorEnv(t testing.TB) (executor *Executor, sbc1, sbc2, sbclookup _ = hc.AddTestTablet(cell, "e0-", 1, "TestExecutor", "e0-", topodatapb.TabletType_PRIMARY, true, 1, nil) // Below is needed so that SendAnyWherePlan doesn't fail - sb := createSandbox(KsTestUnsharded) sbclookup = hc.AddTestTablet(cell, "0", 1, KsTestUnsharded, "0", topodatapb.TabletType_PRIMARY, true, 1, nil) _ = hc.AddTestTablet(cell, "2", 3, KsTestUnsharded, "0", topodatapb.TabletType_REPLICA, true, 1, nil) - // Ues the 'X' in the name to ensure it's not alphabetically first. - // Otherwise, it would become the default keyspace for the dual table. - bad := createSandbox("TestXBadSharding") - bad.VSchema = badVSchema - - sb.VSchema = unshardedVSchema queryLogger := streamlog.New[*logstats.LogStats]("VTGate", queryLogBufferSize) plans := cache.NewDefaultCacheImpl(cache.DefaultConfig) executor = NewExecutor(ctx, serv, cell, resolver, false, false, testBufferSize, plans, nil, false, querypb.ExecuteOptions_Gen4, queryLogger) @@ -194,16 +196,17 @@ func createCustomExecutor(t testing.TB, vschema string) (executor *Executor, sbc ctx, cancel = context.WithCancel(context.Background()) cell := "aa" hc := discovery.NewFakeHealthCheck(nil) + s := createSandbox(KsTestSharded) s.VSchema = vschema + sb := createSandbox(KsTestUnsharded) + sb.VSchema = unshardedVSchema + serv := newSandboxForCells(ctx, []string{cell}) resolver := newTestResolver(ctx, hc, serv, cell) - sbc1 = hc.AddTestTablet(cell, "-20", 1, "TestExecutor", "-20", topodatapb.TabletType_PRIMARY, true, 1, nil) - sbc2 = hc.AddTestTablet(cell, "40-60", 1, "TestExecutor", "40-60", topodatapb.TabletType_PRIMARY, true, 1, nil) - - sb := createSandbox(KsTestUnsharded) + sbc1 = hc.AddTestTablet(cell, "-20", 1, KsTestSharded, "-20", topodatapb.TabletType_PRIMARY, true, 1, nil) + sbc2 = hc.AddTestTablet(cell, "40-60", 1, KsTestSharded, "40-60", topodatapb.TabletType_PRIMARY, true, 1, nil) sbclookup = hc.AddTestTablet(cell, "0", 1, KsTestUnsharded, "0", topodatapb.TabletType_PRIMARY, true, 1, nil) - sb.VSchema = unshardedVSchema queryLogger := streamlog.New[*logstats.LogStats]("VTGate", queryLogBufferSize) plans := cache.NewDefaultCacheImpl(cache.DefaultConfig) @@ -223,8 +226,12 @@ func createCustomExecutorSetValues(t testing.TB, vschema string, values []*sqlty ctx, cancel = context.WithCancel(context.Background()) cell := "aa" hc := discovery.NewFakeHealthCheck(nil) + s := createSandbox(KsTestSharded) s.VSchema = vschema + sb := createSandbox(KsTestUnsharded) + sb.VSchema = unshardedVSchema + serv := newSandboxForCells(ctx, []string{cell}) resolver := newTestResolver(ctx, hc, serv, cell) shards := []string{"-20", "20-40", "40-60", "60-80", "80-a0", "a0-c0", "c0-e0", "e0-"} @@ -236,10 +243,7 @@ func createCustomExecutorSetValues(t testing.TB, vschema string, values []*sqlty } sbcs = append(sbcs, sbc) } - - sb := createSandbox(KsTestUnsharded) sbclookup = hc.AddTestTablet(cell, "0", 1, KsTestUnsharded, "0", topodatapb.TabletType_PRIMARY, true, 1, nil) - sb.VSchema = unshardedVSchema queryLogger := streamlog.New[*logstats.LogStats]("VTGate", queryLogBufferSize) plans := cache.NewDefaultCacheImpl(cache.DefaultConfig) diff --git a/go/vt/vtgate/schema/tracker.go b/go/vt/vtgate/schema/tracker.go index 61b238d4676..3a03ea83a84 100644 --- a/go/vt/vtgate/schema/tracker.go +++ b/go/vt/vtgate/schema/tracker.go @@ -151,6 +151,10 @@ func (t *Tracker) Start() { for { select { case th := <-t.ch: + if th == nil { + // channel closed + return + } ksUpdater := t.getKeyspaceUpdateController(th) ksUpdater.add(th) case <-ctx.Done(): From 123856a280877321b3265df6be7e7aae525fb027 Mon Sep 17 00:00:00 2001 From: Dirkjan Bussink Date: Tue, 29 Aug 2023 11:53:36 +0200 Subject: [PATCH 2/3] Ensure we remove the test directory Signed-off-by: Dirkjan Bussink --- go/cmd/vttestserver/vttestserver_test.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/go/cmd/vttestserver/vttestserver_test.go b/go/cmd/vttestserver/vttestserver_test.go index b3cb888961b..226d66305be 100644 --- a/go/cmd/vttestserver/vttestserver_test.go +++ b/go/cmd/vttestserver/vttestserver_test.go @@ -114,7 +114,10 @@ func TestPersistentMode(t *testing.T) { // reboot the persistent cluster cluster.TearDown() cluster, err = startPersistentCluster(dir) - defer cluster.TearDown() + defer func() { + cluster.PersistentMode = false // Cleanup the tmpdir as we're done + cluster.TearDown() + }() assert.NoError(t, err) // rerun our sanity checks to make sure vschema is persisted correctly From e27584d26f7dfaa3ccccd94f403ae96f74ff7680 Mon Sep 17 00:00:00 2001 From: Dirkjan Bussink Date: Tue, 29 Aug 2023 12:07:23 +0200 Subject: [PATCH 3/3] Drop check for sockets This seems to be sometimes hanging, so avoid needing to use this. The value here is limited and we care mostly about goroutines. Signed-off-by: Dirkjan Bussink --- go/test/utils/noleak.go | 24 ------------------------ 1 file changed, 24 deletions(-) diff --git a/go/test/utils/noleak.go b/go/test/utils/noleak.go index 21207b6b989..31d454ec789 100644 --- a/go/test/utils/noleak.go +++ b/go/test/utils/noleak.go @@ -18,16 +18,10 @@ package utils import ( "context" - "os" - "os/exec" - "strconv" - "strings" "testing" "time" "go.uber.org/goleak" - - "vitess.io/vitess/go/vt/log" ) // LeakCheckContext returns a Context that will be automatically cancelled at the end @@ -73,9 +67,6 @@ func ensureNoLeaks() error { if err := ensureNoGoroutines(); err != nil { return err } - if err := ensureNoOpenSockets(); err != nil { - return err - } return nil } @@ -103,18 +94,3 @@ func ensureNoGoroutines() error { } return err } - -func ensureNoOpenSockets() error { - cmd := exec.Command("lsof", "-a", "-p", strconv.Itoa(os.Getpid()), "-i", "-P", "-V") - cmd.Stderr = nil - lsof, err := cmd.Output() - if err == nil { - log.Errorf("found open sockets:\n%s", lsof) - } else { - if strings.Contains(string(lsof), "no Internet files located") { - return nil - } - log.Errorf("failed to run `lsof`: %v (%q)", err, lsof) - } - return err -}