Merge cockroachdb#106768

106768: base,server,testutils: clarify the multi-tenant test situations r=yuzefovich a=knz

Informs cockroachdb#76378 .
Informs cockroachdb#106772.
Epic: CRDB-18499

Prior to this patch, a test could choose only between the following
options:

- `TestTenantProbabilisticOnly` and `TestTenantProbabilistic`:
  whether or not the load goes to a secondary tenant is randomly
  selected for each test.

- `TestTenantEnabled`: always use a secondary tenant.

- `TestTenantDisabled`: always disable the secondary tenant.

In particular the last option was problematic, as we ended up using it
both in the case "the test doesn't work with secondary tenants and
*we don't know why*" and the case "it doesn't work, we know why
*and it's OK to keep it that way*".

We actually want to distinguish these cases, because the former
case requires further investigation, and we want the test code
to refer to this follow-up work more clearly.

To address this, this commit defines the following options:

```go
// (UNCHANGED)
TestTenantProbabilisticOnly
// (UNCHANGED)
TestTenantProbabilistic

// RENAMED: was previously "TestTenantEnabled"
//
// TestTenantAlwaysEnabled will always start the default test tenant. This is useful
// for quickly verifying that a test works with tenants enabled.
//
// Note: this value should not be used for checked in test code
// unless there is a good reason to do so. We want the common case
// to use TestTenantProbabilistic or TestTenantProbabilisticOnly.
TestTenantAlwaysEnabled

// RENAMED: was previously "TestTenantDisabled"
//
// TODOTestTenantDisabled should not be used anymore. Use the
// other values instead.
// TODO(cockroachdb#76378): Review existing tests and use the proper value instead.
TODOTestTenantDisabled = DefaultTestTenantOptions{testBehavior: ttDisabled}

// (NEW) TestIsSpecificToStorageLayerAndNeedsASystemTenant is used when
// the test needs to be given access to a SQL conn to a tenant with
// sufficient capabilities to access all the storage layer.
// (Initially that'd be "the" system tenant.)
TestIsSpecificToStorageLayerAndNeedsASystemTenant

// (NEW) TestControlsTenantsExplicitly is used when the test wants to
// manage its own secondary tenants and tenant servers.
TestControlsTenantsExplicitly

// (NEW) TestNeedsTightIntegrationBetweenAPIsAndTestingKnobs is used when
// a test wants to use a single set of testing knobs for both the
// storage layer and the SQL layer and for simplicity of the test
// code we want to give that test a simplified environment.
//
// Note: it is debatable whether the gain in test code simplicity is
// worth the cost of never running that test with the virtualization
// layer active.
TestNeedsTightIntegrationBetweenAPIsAndTestingKnobs

// (NEW) TestDoesNotWorkWithSecondaryTenantsButWeDontKnowWhyYet can be used
// to disable virtualization because the test doesn't appear to be compatible
// with it, and we don't understand it yet.
// It should link to a github issue with label C-test-failure.
TestDoesNotWorkWithSecondaryTenantsButWeDontKnowWhyYet(issueNumber int)

// (NEW) TestIsForStuffThatShouldWorkWithSecondaryTenantsButDoesntYet can be
// used to disable virtualization because the test exercises a feature
// known not to work with virtualization enabled yet but we wish it
// to work eventually.
//
// It should link to a github issue with label C-bug
// and the issue should be linked to an epic under INI-213 or INI-214.
TestIsForStuffThatShouldWorkWithSecondaryTenantsButDoesntYet(issueNumber int)
```

Release note: None



Co-authored-by: Raphael 'kena' Poss <[email protected]>

pkg/base/test_server_args.go

@@ -217,26 +217,125 @@ type TestClusterArgs struct {
 
 // DefaultTestTenantOptions specifies the conditions under which the default
 // test tenant will be started.
-type DefaultTestTenantOptions int
+type DefaultTestTenantOptions struct {
+	testBehavior testBehavior
+
+	// Whether the testserver will allow or block attempts to create
+	// additional secondary tenants. (Default is to block.)
+	allowAdditionalTenants bool
+
+	// If test tenant is disabled, issue and label to link in log message.
+	issueNum int
+	label    string
+}
+
+type testBehavior int8
 
 const (
+	ttProb testBehavior = iota
+	ttEnabled
+	ttDisabled
+)
+
+var (
 	// TestTenantProbabilisticOnly will start the default test tenant on a
 	// probabilistic basis. It will also prevent the starting of additional
 	// tenants by raising an error if it is attempted.
 	// This is the default behavior.
-	TestTenantProbabilisticOnly DefaultTestTenantOptions = iota
+	TestTenantProbabilisticOnly = DefaultTestTenantOptions{testBehavior: ttProb, allowAdditionalTenants: false}
 	// TestTenantProbabilistic will start the default test tenant on a
 	// probabilistic basis. It allows the starting of additional tenants.
-	TestTenantProbabilistic
-	// TestTenantEnabled will always start the default test tenant. This is useful
+	TestTenantProbabilistic = DefaultTestTenantOptions{testBehavior: ttProb, allowAdditionalTenants: true}
+	// TestTenantAlwaysEnabled will always start the default test tenant. This is useful
 	// for quickly verifying that a test works with tenants enabled.
-	TestTenantEnabled
-	// TestTenantDisabled will disable the implicit starting of the default test
-	// tenant. This is useful for tests that want to explicitly control the
-	// starting of tenants, or currently don't work with tenants.
-	TestTenantDisabled
+	//
+	// Note: this value should not be used for checked in test code
+	// unless there is a good reason to do so. We want the common case
+	// to use TestTenantProbabilistic or TestTenantProbabilisticOnly.
+	TestTenantAlwaysEnabled = DefaultTestTenantOptions{testBehavior: ttEnabled, allowAdditionalTenants: true}
+
+	// TODOTestTenantDisabled should not be used anymore. Use the
+	// other values instead.
+	// TODO(#76378): Review existing tests and use the proper value instead.
+	TODOTestTenantDisabled = DefaultTestTenantOptions{testBehavior: ttDisabled, allowAdditionalTenants: true}
+
+	// TestControlsTenantsExplicitly is used when the test wants to
+	// manage its own secondary tenants and tenant servers.
+	TestControlsTenantsExplicitly = DefaultTestTenantOptions{testBehavior: ttDisabled, allowAdditionalTenants: true}
+
+	// TestIsSpecificToStorageLayerAndNeedsASystemTenant is used when
+	// the test needs to be given access to a SQL conn to a tenant with
+	// sufficient capabilities to access all the storage layer.
+	// (Initially that'd be "the" system tenant.)
+	TestIsSpecificToStorageLayerAndNeedsASystemTenant = DefaultTestTenantOptions{testBehavior: ttDisabled, allowAdditionalTenants: true}
+
+	// TestNeedsTightIntegrationBetweenAPIsAndTestingKnobs is used when
+	// a test wants to use a single set of testing knobs for both the
+	// storage layer and the SQL layer and for simplicity of the test
+	// code we want to give that test a simplified environment.
+	//
+	// Note: it is debatable whether the gain in test code simplicity is
+	// worth the cost of never running that test with the virtualization
+	// layer active.
+	TestNeedsTightIntegrationBetweenAPIsAndTestingKnobs = TestIsSpecificToStorageLayerAndNeedsASystemTenant
+
+	// 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}
 )
 
+func (do DefaultTestTenantOptions) AllowAdditionalTenants() bool {
+	return do.allowAdditionalTenants
+}
+
+func (do DefaultTestTenantOptions) TestTenantAlwaysEnabled() bool {
+	return do.testBehavior == ttEnabled
+}
+
+func (do DefaultTestTenantOptions) TestTenantAlwaysDisabled() bool {
+	return do.testBehavior == ttDisabled
+}
+
+func (do DefaultTestTenantOptions) IssueRef() (int, string) {
+	return do.issueNum, do.label
+}
+
+// TestDoesNotWorkWithSecondaryTenantsButWeDontKnowWhyYet can be used
+// to disable virtualization because the test doesn't appear to be compatible
+// with it, and we don't understand it yet.
+// It should link to a github issue with label C-test-failure.
+func TestDoesNotWorkWithSecondaryTenantsButWeDontKnowWhyYet(
+	issueNumber int,
+) DefaultTestTenantOptions {
+	return DefaultTestTenantOptions{
+		testBehavior:           ttDisabled,
+		allowAdditionalTenants: true,
+		issueNum:               issueNumber,
+		label:                  "C-test-failure",
+	}
+}
+
+// TestIsForStuffThatShouldWorkWithSecondaryTenantsButDoesntYet can be
+// used to disable virtualization because the test exercises a feature
+// known not to work with virtualization enabled yet but we wish it to
+// eventually.
+//
+// It should link to a github issue with label C-bug
+// and the issue should be linked to an epic under INI-213 or INI-214.
+func TestIsForStuffThatShouldWorkWithSecondaryTenantsButDoesntYet(
+	issueNumber int,
+) DefaultTestTenantOptions {
+	return DefaultTestTenantOptions{
+		testBehavior:           ttDisabled,
+		allowAdditionalTenants: true,
+		issueNum:               issueNumber,
+		label:                  "C-bug",
+	}
+}
+
 var (
 	// DefaultTestStoreSpec is just a single in memory store of 512 MiB
 	// with no special attributes.

pkg/bench/foreachdb.go

@@ -47,7 +47,7 @@ func benchmarkCockroach(b *testing.B, f BenchmarkFn) {
 	s, db, _ := serverutils.StartServer(
 		b, base.TestServerArgs{
 			UseDatabase:       "bench",
-			DefaultTestTenant: base.TestTenantDisabled,
+			DefaultTestTenant: base.TODOTestTenantDisabled,
 		})
 	defer s.Stopper().Stop(context.TODO())
 
@@ -65,7 +65,7 @@ func benchmarkSharedProcessTenantCockroach(b *testing.B, f BenchmarkFn) {
 	ctx := context.Background()
 	s, db, _ := serverutils.StartServer(
 		b, base.TestServerArgs{
-			DefaultTestTenant: base.TestTenantDisabled,
+			DefaultTestTenant: base.TODOTestTenantDisabled,
 		})
 	defer s.Stopper().Stop(ctx)
 
@@ -114,7 +114,7 @@ func benchmarkSepProcessTenantCockroach(b *testing.B, f BenchmarkFn) {
 	ctx := context.Background()
 	s, db, _ := serverutils.StartServer(
 		b, base.TestServerArgs{
-			DefaultTestTenant: base.TestTenantDisabled,
+			DefaultTestTenant: base.TODOTestTenantDisabled,
 		})
 	defer s.Stopper().Stop(ctx)
 
@@ -143,7 +143,7 @@ func benchmarkMultinodeCockroach(b *testing.B, f BenchmarkFn) {
 			ReplicationMode: base.ReplicationAuto,
 			ServerArgs: base.TestServerArgs{
 				UseDatabase:       "bench",
-				DefaultTestTenant: base.TestTenantDisabled,
+				DefaultTestTenant: base.TODOTestTenantDisabled,
 			},
 		})
 	if _, err := tc.Conns[0].Exec(`CREATE DATABASE bench`); err != nil {

pkg/ccl/backupccl/alter_backup_schedule_test.go

@@ -71,7 +71,7 @@ func newAlterSchedulesTestHelper(t *testing.T) (*alterSchedulesTestHelper, func(
 		ExternalIODir: dir,
 		// Some scheduled backup tests fail when run within a tenant. More
 		// investigation is required. Tracked with #76378.
-		DefaultTestTenant: base.TestTenantDisabled,
+		DefaultTestTenant: base.TODOTestTenantDisabled,
 		Knobs: base.TestingKnobs{
 			JobsTestingKnobs: knobs,
 		},

pkg/ccl/backupccl/backup_tenant_test.go

@@ -43,7 +43,7 @@ func TestBackupTenantImportingTable(t *testing.T) {
 			ServerArgs: base.TestServerArgs{
 				// Test is designed to run with explicit tenants. No need to
 				// implicitly create a tenant.
-				DefaultTestTenant: base.TestTenantDisabled,
+				DefaultTestTenant: base.TODOTestTenantDisabled,
 			},
 		})
 	defer tc.Stopper().Stop(ctx)