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

Author: knz

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)