server,testutils: remove complexity

There is a saying (paraphrasing) that it always takes more work
removing unwanted complexity than it takes to add it. This is an
example of that.

Prior to this commit, there was an "interesting" propagation of the
flag that decides whether or not to define a test tenant for test
servers and clusters. In a nutshell, we had:

- an "input" flag in `base.TestServerArgs`, which remained mostly immutable
- a boolean decided once by `ShouldStartDefaultTestTenant()` either in:
  - `serverutils.StartServerOnlyE`
  - or `testcluster.Start`
- that boolean choice was then propagated to `server.testServer` via
  _another_ boolean config flag in `server.BaseConfig`
- both the 2nd boolean and the original input flag were then again
  checked when the time came to do the work (in `maybeStartDefaultTestTenant`).

Additional complexity was then incurred by the need of `TestCluster`
to make the determination just once (and not once per server).

This commit cuts through all the layers of complexity by simply
propagating the choice of `ShouldStartDefaultTestTenant()` back into
the `TestServerArgs` and only ever reading from that subsequently.

Release note: None

pkg/base/test_server_args.go

@@ -281,9 +281,6 @@ var (
 
 	// InternalNonDefaultDecision is a sentinel value used inside a
 	// mechanism in serverutils. Should not be used by tests directly.
-	//
-	// TODO(#76378): Investigate how we can remove the need for this
-	// sentinel value.
 	InternalNonDefaultDecision = DefaultTestTenantOptions{testBehavior: ttDisabled, allowAdditionalTenants: true}
 )
 

pkg/ccl/backupccl/backup_test.go

@@ -337,9 +337,11 @@ func TestBackupRestorePartitioned(t *testing.T) {
 
 	// Disabled to run within tenant as certain MR features are not available to tenants.
 	args := base.TestClusterArgs{
+		ServerArgs: base.TestServerArgs{
+			DefaultTestTenant: base.TODOTestTenantDisabled,
+		},
 		ServerArgsPerNode: map[int]base.TestServerArgs{
 			0: {
-				DefaultTestTenant: base.TODOTestTenantDisabled,
 				Locality: roachpb.Locality{Tiers: []roachpb.Tier{
 					{Key: "region", Value: "west"},
 					// NB: This has the same value as an az in the east region
@@ -349,7 +351,6 @@ func TestBackupRestorePartitioned(t *testing.T) {
 				}},
 			},
 			1: {
-				DefaultTestTenant: base.TODOTestTenantDisabled,
 				Locality: roachpb.Locality{Tiers: []roachpb.Tier{
 					{Key: "region", Value: "east"},
 					// NB: This has the same value as an az in the west region
@@ -359,7 +360,6 @@ func TestBackupRestorePartitioned(t *testing.T) {
 				}},
 			},
 			2: {
-				DefaultTestTenant: base.TODOTestTenantDisabled,
 				Locality: roachpb.Locality{Tiers: []roachpb.Tier{
 					{Key: "region", Value: "east"},
 					{Key: "az", Value: "az2"},
@@ -490,34 +490,33 @@ func TestBackupRestoreExecLocality(t *testing.T) {
 
 	// Disabled to run within tenant as certain MR features are not available to tenants.
 	args := base.TestClusterArgs{
+		ServerArgs: base.TestServerArgs{
+			DefaultTestTenant: base.TODOTestTenantDisabled,
+		},
 		ServerArgsPerNode: map[int]base.TestServerArgs{
 			0: {
-				ExternalIODir:     "/west0",
-				DefaultTestTenant: base.TODOTestTenantDisabled,
+				ExternalIODir: "/west0",
 				Locality: roachpb.Locality{Tiers: []roachpb.Tier{
 					{Key: "tier", Value: "0"},
 					{Key: "region", Value: "west"},
 				}},
 			},
 			1: {
-				ExternalIODir:     "/west1",
-				DefaultTestTenant: base.TODOTestTenantDisabled,
+				ExternalIODir: "/west1",
 				Locality: roachpb.Locality{Tiers: []roachpb.Tier{
 					{Key: "tier", Value: "1"},
 					{Key: "region", Value: "west"},
 				}},
 			},
 			2: {
-				ExternalIODir:     "/east0",
-				DefaultTestTenant: base.TODOTestTenantDisabled,
+				ExternalIODir: "/east0",
 				Locality: roachpb.Locality{Tiers: []roachpb.Tier{
 					{Key: "tier", Value: "0"},
 					{Key: "region", Value: "east"},
 				}},
 			},
 			3: {
-				ExternalIODir:     "/east1",
-				DefaultTestTenant: base.TODOTestTenantDisabled,
+				ExternalIODir: "/east1",
 				Locality: roachpb.Locality{Tiers: []roachpb.Tier{
 					{Key: "tier", Value: "1"},
 					{Key: "region", Value: "east"},

pkg/ccl/changefeedccl/changefeed_test.go

@@ -5689,10 +5689,7 @@ func TestChangefeedHandlesRollingRestart(t *testing.T) {
 		nodeDrainChannels[n].Store(make(chan struct{}))
 
 		return base.TestServerArgs{
-			// Test uses SPLIT AT, which isn't currently supported for
-			// secondary tenants. Tracked with #76378.
-			DefaultTestTenant: base.TODOTestTenantDisabled,
-			UseDatabase:       "test",
+			UseDatabase: "test",
 			Knobs: base.TestingKnobs{
 				DistSQL: &execinfra.TestingKnobs{
 					DrainFast: true,
@@ -5770,6 +5767,11 @@ func TestChangefeedHandlesRollingRestart(t *testing.T) {
 			}
 			return perNode
 		}(),
+		ServerArgs: base.TestServerArgs{
+			// Test uses SPLIT AT, which isn't currently supported for
+			// secondary tenants. Tracked with #76378.
+			DefaultTestTenant: base.TODOTestTenantDisabled,
+		},
 	})
 	defer tc.Stopper().Stop(context.Background())
 
@@ -5882,9 +5884,6 @@ func TestChangefeedPropagatesTerminalError(t *testing.T) {
 	perServerKnobs := make(map[int]base.TestServerArgs, numNodes)
 	for i := 0; i < numNodes; i++ {
 		perServerKnobs[i] = base.TestServerArgs{
-			// Test uses SPLIT AT, which isn't currently supported for
-			// secondary tenants. Tracked with #76378.
-			DefaultTestTenant: base.TODOTestTenantDisabled,
 			Knobs: base.TestingKnobs{
 				DistSQL: &execinfra.TestingKnobs{
 					DrainFast:  true,
@@ -5901,6 +5900,11 @@ func TestChangefeedPropagatesTerminalError(t *testing.T) {
 		base.TestClusterArgs{
 			ServerArgsPerNode: perServerKnobs,
 			ReplicationMode:   base.ReplicationManual,
+			ServerArgs: base.TestServerArgs{
+				// Test uses SPLIT AT, which isn't currently supported for
+				// secondary tenants. Tracked with #76378.
+				DefaultTestTenant: base.TODOTestTenantDisabled,
+			},
 		})
 	defer tc.Stopper().Stop(context.Background())
 
@@ -8241,13 +8245,17 @@ func TestChangefeedExecLocality(t *testing.T) {
 	str := strconv.Itoa
 
 	const nodes = 4
-	args := base.TestClusterArgs{ServerArgsPerNode: map[int]base.TestServerArgs{}}
+	args := base.TestClusterArgs{
+		ServerArgs: base.TestServerArgs{
+			DefaultTestTenant: base.TODOTestTenantDisabled, // need nodelocal and splits.
+		},
+		ServerArgsPerNode: map[int]base.TestServerArgs{},
+	}
 	for i := 0; i < nodes; i++ {
 		args.ServerArgsPerNode[i] = base.TestServerArgs{
 			ExternalIODir: path.Join(dir, str(i)),
 			Locality: roachpb.Locality{
 				Tiers: []roachpb.Tier{{Key: "x", Value: str(i / 2)}, {Key: "y", Value: str(i % 2)}}},
-			DefaultTestTenant: base.TODOTestTenantDisabled, // need nodelocal and splits.
 		}
 	}
 

pkg/ccl/kvccl/kvfollowerreadsccl/boundedstaleness_test.go

@@ -268,14 +268,16 @@ func TestBoundedStalenessDataDriven(t *testing.T) {
 	ctx := context.Background()
 
 	clusterArgs := base.TestClusterArgs{
+		ServerArgs: base.TestServerArgs{
+			DefaultTestTenant: base.TODOTestTenantDisabled,
+		},
 		ServerArgsPerNode: map[int]base.TestServerArgs{},
 	}
 	const numNodes = 3
 	var bse boundedStalenessEvents
 	for i := 0; i < numNodes; i++ {
 		i := i
 		clusterArgs.ServerArgsPerNode[i] = base.TestServerArgs{
-			DefaultTestTenant: base.TODOTestTenantDisabled,
 			Knobs: base.TestingKnobs{
 				SQLExecutor: &sql.ExecutorTestingKnobs{
 					WithStatementTrace: func(trace tracingpb.Recording, stmt string) {

pkg/ccl/kvccl/kvfollowerreadsccl/followerreads_test.go

@@ -714,8 +714,7 @@ func TestFollowerReadsWithStaleDescriptor(t *testing.T) {
 			// Also, we're going to collect a trace of the test's final query.
 			ServerArgsPerNode: map[int]base.TestServerArgs{
 				3: {
-					DefaultTestTenant: base.TODOTestTenantDisabled,
-					UseDatabase:       "t",
+					UseDatabase: "t",
 					Knobs: base.TestingKnobs{
 						KVClient: &kvcoord.ClientTestingKnobs{
 							// Inhibit the checking of connection health done by the
@@ -901,13 +900,15 @@ func TestSecondaryTenantFollowerReadsRouting(t *testing.T) {
 			}
 			localities[i] = locality
 			serverArgs[i] = base.TestServerArgs{
-				Locality:          localities[i],
-				DefaultTestTenant: base.TODOTestTenantDisabled, // we'll create one ourselves below.
+				Locality: localities[i],
 			}
 		}
 		tc := testcluster.StartTestCluster(t, numNodes, base.TestClusterArgs{
 			ReplicationMode:   base.ReplicationManual,
 			ServerArgsPerNode: serverArgs,
+			ServerArgs: base.TestServerArgs{
+				DefaultTestTenant: base.TODOTestTenantDisabled, // we'll create one ourselves below.
+			},
 		})
 		ctx := context.Background()
 		defer tc.Stopper().Stop(ctx)

pkg/ccl/multiregionccl/cold_start_latency_test.go

@@ -79,8 +79,7 @@ func TestColdStartLatency(t *testing.T) {
 	for i := 0; i < numNodes; i++ {
 		i := i
 		args := base.TestServerArgs{
-			DefaultTestTenant: base.TODOTestTenantDisabled,
-			Locality:          localities[i],
+			Locality: localities[i],
 		}
 		signalAfter[i] = make(chan struct{})
 		serverKnobs := &server.TestingKnobs{
@@ -120,6 +119,9 @@ func TestColdStartLatency(t *testing.T) {
 	tc := testcluster.NewTestCluster(t, numNodes, base.TestClusterArgs{
 		ParallelStart:     true,
 		ServerArgsPerNode: perServerArgs,
+		ServerArgs: base.TestServerArgs{
+			DefaultTestTenant: base.TODOTestTenantDisabled,
+		},
 	})
 	go func() {
 		for _, c := range signalAfter {

pkg/ccl/multiregionccl/datadriven_test.go

@@ -153,12 +153,6 @@ func TestMultiRegionDataDriven(t *testing.T) {
 					}
 					serverArgs[i] = base.TestServerArgs{
 						Locality: localityCfg,
-						// We need to disable the default test tenant here
-						// because it appears as though operations like
-						// "wait-for-zone-config-changes" only work correctly
-						// when called from the system tenant. More
-						// investigation is required (tracked with #76378).
-						DefaultTestTenant: base.TODOTestTenantDisabled,
 						Knobs: base.TestingKnobs{
 							SQLExecutor: &sql.ExecutorTestingKnobs{
 								WithStatementTrace: func(trace tracingpb.Recording, stmt string) {
@@ -175,6 +169,14 @@ func TestMultiRegionDataDriven(t *testing.T) {
 				numServers := len(localityNames)
 				tc := testcluster.StartTestCluster(t, numServers, base.TestClusterArgs{
 					ServerArgsPerNode: serverArgs,
+					ServerArgs: base.TestServerArgs{
+						// We need to disable the default test tenant here
+						// because it appears as though operations like
+						// "wait-for-zone-config-changes" only work correctly
+						// when called from the system tenant. More
+						// investigation is required (tracked with #76378).
+						DefaultTestTenant: base.TODOTestTenantDisabled,
+					},
 				})
 				ds.tc = tc
 

pkg/ccl/multiregionccl/multiregionccltestutils/testutils.go

@@ -110,13 +110,6 @@ func TestingCreateMultiRegionClusterWithRegionList(
 				Knobs:         knobs,
 				ExternalIODir: params.baseDir,
 				UseDatabase:   params.useDatabase,
-				// Disabling this due to failures in the rtt_analysis tests. Ideally
-				// we could disable multi-tenancy just for those tests, but this function
-				// is used to create the MR cluster for all test cases. For
-				// bonus points, the code to re-enable this should also provide more
-				// flexibility in disabling the default test tenant by callers of this
-				// function. Re-enablement is tracked with #76378.
-				DefaultTestTenant: base.TODOTestTenantDisabled,
 				Locality: roachpb.Locality{
 					Tiers: []roachpb.Tier{{Key: "region", Value: region}},
 				},
@@ -128,6 +121,15 @@ func TestingCreateMultiRegionClusterWithRegionList(
 	tc := testcluster.StartTestCluster(t, totalServerCount, base.TestClusterArgs{
 		ReplicationMode:   params.replicationMode,
 		ServerArgsPerNode: serverArgs,
+		ServerArgs: base.TestServerArgs{
+			// Disabling this due to failures in the rtt_analysis tests. Ideally
+			// we could disable multi-tenancy just for those tests, but this function
+			// is used to create the MR cluster for all test cases. For
+			// bonus points, the code to re-enable this should also provide more
+			// flexibility in disabling the default test tenant by callers of this
+			// function. Re-enablement is tracked with #76378.
+			DefaultTestTenant: base.TODOTestTenantDisabled,
+		},
 	})
 
 	ctx := context.Background()

pkg/server/config.go

@@ -204,9 +204,6 @@ type BaseConfig struct {
 	// Environment Variable: COCKROACH_DISABLE_SPAN_CONFIGS
 	SpanConfigsDisabled bool
 
-	// Disables the default test tenant.
-	DisableDefaultTestTenant bool
-
 	// TestingKnobs is used for internal test controls only.
 	TestingKnobs base.TestingKnobs
 

pkg/server/multi_store_test.go

@@ -62,11 +62,12 @@ func TestAddNewStoresToExistingNodes(t *testing.T) {
 			// again explicitly.
 			ReplicationMode:   base.ReplicationAuto,
 			ServerArgsPerNode: map[int]base.TestServerArgs{},
+			ServerArgs: base.TestServerArgs{
+				DefaultTestTenant: base.TODOTestTenantDisabled,
+			},
 		}
 		for srvIdx := 0; srvIdx < numNodes; srvIdx++ {
-			serverArgs := base.TestServerArgs{
-				DefaultTestTenant: base.TODOTestTenantDisabled,
-			}
+			serverArgs := base.TestServerArgs{}
 			serverArgs.Knobs.Server = &server.TestingKnobs{StickyVFSRegistry: ser}
 			for storeIdx := 0; storeIdx < numStoresPerNode; storeIdx++ {
 				id := fmt.Sprintf("s%d.%d", srvIdx+1, storeIdx+1)

pkg/server/testserver.go

@@ -308,9 +308,6 @@ func makeTestConfigFromParams(params base.TestServerArgs) Config {
 		cfg.TempStorageConfig.Settings = st
 	}
 
-	// TODO(#76378): Review this assignment to ensure it does not interfere with randomization.
-	cfg.DisableDefaultTestTenant = params.DefaultTestTenant.TestTenantAlwaysDisabled()
-
 	if cfg.TestingKnobs.Store == nil {
 		cfg.TestingKnobs.Store = &kvserver.StoreTestingKnobs{}
 	}
@@ -569,26 +566,23 @@ func (ts *testServer) TestTenants() []serverutils.ApplicationLayerInterface {
 	return ts.testTenants
 }
 
-// DefaultTestTenantDisabled is part of the serverutils.TenantControlInterface.
-func (ts *testServer) DefaultTestTenantDisabled() bool {
-	return ts.cfg.DisableDefaultTestTenant
-}
-
-// DisableDefaultTestTenant is part of the serverutils.TenantControlInterface.
-func (ts *testServer) DisableDefaultTestTenant() {
-	ts.cfg.DisableDefaultTestTenant = true
-}
-
 // maybeStartDefaultTestTenant might start a test tenant. This can then be used
 // for multi-tenant testing, where the default SQL connection will be made to
 // this tenant instead of to the system tenant. Note that we will
 // currently only attempt to start a test tenant if we're running in an
 // enterprise enabled build. This is due to licensing restrictions on the MT
 // capabilities.
 func (ts *testServer) maybeStartDefaultTestTenant(ctx context.Context) error {
+	if !(ts.params.DefaultTestTenant.TestTenantAlwaysDisabled() ||
+		ts.params.DefaultTestTenant.TestTenantAlwaysEnabled()) {
+		return errors.WithHint(
+			errors.AssertionFailedf("programming error: no decision taken about the default test tenant"),
+			"Maybe add the missing call to serverutils.ShouldStartDefaultTestTenant()?")
+	}
+
 	// If the flag has been set to disable the default test tenant, don't start
 	// it here.
-	if ts.params.DefaultTestTenant.TestTenantAlwaysDisabled() || ts.cfg.DisableDefaultTestTenant {
+	if ts.params.DefaultTestTenant.TestTenantAlwaysDisabled() {
 		return nil
 	}
 
@@ -597,7 +591,10 @@ func (ts *testServer) maybeStartDefaultTestTenant(ctx context.Context) error {
 		log.Shoutf(ctx, severity.WARNING, "test tenant requested by configuration, but code organization prevents start!\n%v", err)
 		// If not enterprise enabled, we won't be able to use SQL Servers so eat
 		// the error and return without creating/starting a SQL server.
-		ts.cfg.DisableDefaultTestTenant = true
+		//
+		// TODO(knz/yahor): Remove this - as we discussed this ought to work
+		// now even when not enterprise enabled.
+		ts.params.DefaultTestTenant = base.TODOTestTenantDisabled
 		return nil // nolint:returnerrcheck
 	}
 
@@ -668,7 +665,15 @@ func (ts *testServer) maybeStartDefaultTestTenant(ctx context.Context) error {
 // testServer.AdvRPCAddr() after Start() for client connections.
 // Use testServer.Stopper().Stop() to shutdown the server after the test
 // completes.
-func (ts *testServer) Start(ctx context.Context) error {
+func (ts *testServer) Start(ctx context.Context) (retErr error) {
+	defer func() {
+		if retErr != nil {
+			// Use a separate context to avoid using an already-cancelled
+			// context in closers.
+			ts.Stopper().Stop(context.Background())
+		}
+	}()
+
 	if err := ts.topLevelServer.PreStart(ctx); err != nil {
 		return err
 	}
@@ -681,16 +686,16 @@ func (ts *testServer) Start(ctx context.Context) error {
 	); err != nil {
 		return err
 	}
+
+	// Let clients connect.
 	if err := ts.topLevelServer.AcceptClients(ctx); err != nil {
 		return err
 	}
 
 	if err := ts.maybeStartDefaultTestTenant(ctx); err != nil {
-		// We're failing the call to this function but we've already started
-		// the testServer above. Stop it here to avoid leaking the server.
-		ts.Stopper().Stop(context.Background())
 		return err
 	}
+
 	go func() {
 		// If the server requests a shutdown, do that simply by stopping the
 		// stopper.