Merge cockroachdb#86877

86877: multi-tenant: Small cleanup of tenant testing r=stevendanna a=ajstorm This commit addresses two small problems with tenant testing: - We were mistakenly not passing along the testing knobs from the server to the tenant. - We were printing out the "this test is using a tenant" message before we were guaranteed to be using a tenant. To resolve these issues we now: - Pass along the testing knobs from the server to the tenant. - Print out the tenant message only when we're guranteed to be running the test in a tenant. Release justification: Testing only change Release note: None Co-authored-by: Adam Storm <[email protected]>
fqazi · Sep 28, 2022 · 8521da7 · 8521da7
2 parents 85b29ae + 4a9b145
commit 8521da7
Show file tree

Hide file tree

Showing 7 changed files with 30 additions and 58 deletions.
diff --git a/pkg/base/test_server_args.go b/pkg/base/test_server_args.go
@@ -157,12 +157,6 @@ type TestServerArgs struct {
 	// or if some of the functionality being tested is not accessible from
 	// within tenants.
 	DisableDefaultTestTenant bool
-
-	// ShareMostTestingKnobsWithTenant should be set by tests that want their
-	// testing knobs shared with the any DefaultTestTenant that the server starts.
-	// See (*testserver).TestingKnobsForTenant for details on which knobs aren't
-	// shared.
-	ShareMostTestingKnobsWithTenant bool
 }
 
 // TestClusterArgs contains the parameters one can set when creating a test

diff --git a/pkg/ccl/spanconfigccl/spanconfiglimiterccl/BUILD.bazel b/pkg/ccl/spanconfigccl/spanconfiglimiterccl/BUILD.bazel
@@ -28,6 +28,7 @@ go_test(
         "//pkg/sql/gcjob",
         "//pkg/testutils",
         "//pkg/testutils/serverutils",
+        "//pkg/testutils/skip",
         "//pkg/testutils/sqlutils",
         "//pkg/testutils/testcluster",
         "//pkg/util/leaktest",

diff --git a/pkg/ccl/spanconfigccl/spanconfiglimiterccl/datadriven_test.go b/pkg/ccl/spanconfigccl/spanconfiglimiterccl/datadriven_test.go
@@ -21,6 +21,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/spanconfig"
 	"github.com/cockroachdb/cockroach/pkg/spanconfig/spanconfigtestutils/spanconfigtestcluster"
 	"github.com/cockroachdb/cockroach/pkg/testutils"
+	"github.com/cockroachdb/cockroach/pkg/testutils/skip"
 	"github.com/cockroachdb/cockroach/pkg/testutils/sqlutils"
 	"github.com/cockroachdb/cockroach/pkg/testutils/testcluster"
 	"github.com/cockroachdb/cockroach/pkg/util/leaktest"
@@ -134,6 +135,12 @@ func TestDataDriven(t *testing.T) {
 				return output
 
 			case "override":
+				if tc.StartedDefaultTestTenant() {
+					// Tests using an override need to run from the system
+					// tenant. Skip the test if running with a default test
+					// tenant.
+					skip.IgnoreLintf(t, "unable to run with a default test tenant")
+				}
 				d.ScanArgs(t, "limit", &limitOverride)
 
 			default:

diff --git a/pkg/server/BUILD.bazel b/pkg/server/BUILD.bazel
@@ -223,6 +223,7 @@ go_library(
         "//pkg/storage/enginepb",
         "//pkg/storage/fs",
         "//pkg/testutils/serverutils",
+        "//pkg/testutils/skip",
         "//pkg/ts",
         "//pkg/ts/catalog",
         "//pkg/ui",
@@ -247,6 +248,7 @@ go_library(
         "//pkg/util/log/eventpb",
         "//pkg/util/log/logcrash",
         "//pkg/util/log/logpb",
+        "//pkg/util/log/severity",
         "//pkg/util/metric",
         "//pkg/util/mon",
         "//pkg/util/netutil",

diff --git a/pkg/server/testserver.go b/pkg/server/testserver.go
@@ -47,14 +47,15 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/physicalplan"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
 	"github.com/cockroachdb/cockroach/pkg/sql/sessiondata"
-	"github.com/cockroachdb/cockroach/pkg/sql/sqlstats"
 	"github.com/cockroachdb/cockroach/pkg/storage"
 	"github.com/cockroachdb/cockroach/pkg/testutils/serverutils"
+	"github.com/cockroachdb/cockroach/pkg/testutils/skip"
 	"github.com/cockroachdb/cockroach/pkg/ts"
 	"github.com/cockroachdb/cockroach/pkg/util"
 	"github.com/cockroachdb/cockroach/pkg/util/admission"
 	"github.com/cockroachdb/cockroach/pkg/util/hlc"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
+	"github.com/cockroachdb/cockroach/pkg/util/log/severity"
 	"github.com/cockroachdb/cockroach/pkg/util/metric"
 	addrutil "github.com/cockroachdb/cockroach/pkg/util/netutil/addr"
 	"github.com/cockroachdb/cockroach/pkg/util/retry"
@@ -496,17 +497,6 @@ func (ts *TestServer) TestingKnobs() *base.TestingKnobs {
 	return nil
 }
 
-// TestingKnobsForTenant returns a TestingKnobs that is used when starting Test
-// Tenants. These knobs are a copy of the Server's TestingKnobs with any knobs
-// that have been found to be problematic to share by default removed.
-func (ts *TestServer) TestingKnobsForTenant() base.TestingKnobs {
-	knobs := *ts.TestingKnobs()
-	// TODO(ssd): We don't share the Server knobs the testcluster setup code
-	// installed an RPC listener that is inappropriate for the tenant.
-	knobs.Server = nil
-	return knobs
-}
-
 // TenantStatusServer returns the TenantStatusServer used by the TestServer.
 func (ts *TestServer) TenantStatusServer() interface{} {
 	return ts.status
@@ -540,10 +530,6 @@ func (ts *TestServer) maybeStartDefaultTestTenant(ctx context.Context) error {
 	}
 
 	tempStorageConfig := base.DefaultTestTempStorageConfig(cluster.MakeTestingClusterSettings())
-	var useTransactionalDescIDGenerator bool
-	if knobs, ok := ts.params.Knobs.SQLExecutor.(*sql.ExecutorTestingKnobs); ok {
-		useTransactionalDescIDGenerator = knobs.UseTransactionalDescIDGenerator
-	}
 	params := base.TestTenantArgs{
 		// Currently, all the servers leverage the same tenant ID. We may
 		// want to change this down the road, for more elaborate testing.
@@ -557,24 +543,14 @@ func (ts *TestServer) maybeStartDefaultTestTenant(ctx context.Context) error {
 		UseDatabase:                 ts.params.UseDatabase,
 		SSLCertsDir:                 ts.params.SSLCertsDir,
 		AllowSettingClusterSettings: true,
+		TestingKnobs:                ts.params.Knobs,
 	}
-	if ts.params.ShareMostTestingKnobsWithTenant {
-		params.TestingKnobs = ts.TestingKnobsForTenant()
-	} else {
-		// These settings are inherited from the SQL server creation in
-		// logicTest.newCluster, and are required to run the logic test suite
-		// successfully.
-		params.TestingKnobs = base.TestingKnobs{
-			SQLExecutor: &sql.ExecutorTestingKnobs{
-				DeterministicExplain:            true,
-				UseTransactionalDescIDGenerator: useTransactionalDescIDGenerator,
-			},
-			SQLStatsKnobs: &sqlstats.TestingKnobs{
-				AOSTClause: "AS OF SYSTEM TIME '-1us'",
-			},
-			RangeFeed: ts.TestingKnobs().RangeFeed,
-		}
-	}
+
+	// Since we're creating a tenant, it doesn't make sense to pass through the
+	// Server testing knobs, since the bulk of them only apply to the system
+	// tenant. Any remaining knobs which are required by the tenant should be
+	// setup in StartTenant below (like the BlobClientFactory).
+	params.TestingKnobs.Server = &TestingKnobs{}
 
 	tenant, err := ts.StartTenant(ctx, params)
 	if err != nil {
@@ -584,6 +560,16 @@ func (ts *TestServer) maybeStartDefaultTestTenant(ctx context.Context) error {
 	if len(ts.testTenants) == 0 {
 		ts.testTenants = make([]serverutils.TestTenantInterface, 1)
 		ts.testTenants[0] = tenant
+
+		if !skip.UnderBench() {
+			// Now that we've started the first tenant, log this fact for easier
+			// debugging. Skip the logging if we're running a benchmark (because
+			// these INFO messages break the benchstat utility).
+			log.Shout(context.Background(), severity.INFO,
+				"Running test with the default test tenant. "+
+					"If you are only seeing a test case failure when this message appears, there may be a "+
+					"problem with your test case running within tenants.")
+		}
 	} else {
 		// We restrict the creation of multiple default tenants because if
 		// we allow for more than one to be created, it's not clear what we

diff --git a/pkg/testutils/serverutils/BUILD.bazel b/pkg/testutils/serverutils/BUILD.bazel
@@ -32,7 +32,6 @@ go_library(
         "//pkg/util/hlc",
         "//pkg/util/httputil",
         "//pkg/util/log",
-        "//pkg/util/log/severity",
         "//pkg/util/protoutil",
         "//pkg/util/stop",
         "//pkg/util/uuid",

diff --git a/pkg/testutils/serverutils/test_server_shim.go b/pkg/testutils/serverutils/test_server_shim.go
@@ -39,8 +39,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/util/envutil"
 	"github.com/cockroachdb/cockroach/pkg/util/hlc"
 	"github.com/cockroachdb/cockroach/pkg/util/httputil"
-	"github.com/cockroachdb/cockroach/pkg/util/log"
-	"github.com/cockroachdb/cockroach/pkg/util/log/severity"
 	"github.com/cockroachdb/cockroach/pkg/util/protoutil"
 	"github.com/cockroachdb/cockroach/pkg/util/stop"
 	"github.com/cockroachdb/cockroach/pkg/util/uuid"
@@ -103,22 +101,7 @@ func ShouldStartDefaultTestTenant(t testing.TB) bool {
 		t.Fatal("invalid setting of tenantMode flag")
 	}
 
-	if rand.Float64() > probabilityOfStartingDefaultTestTenant {
-		return false
-	}
-
-	if !skip.UnderBench() {
-		// We're starting the default SQL server (i.e. we're running this test
-		// in a tenant). Log this for easier debugging unless we're running a
-		// benchmark (because these INFO messages would break the benchstat
-		// utility).
-		log.Shout(context.Background(), severity.INFO,
-			"Running test with the default test tenant. "+
-				"If you are only seeing a test case failure when this message appears, there may be a "+
-				"problem with your test case running within tenants.")
-	}
-
-	return true
+	return rand.Float64() <= probabilityOfStartingDefaultTestTenant
 }
 
 // TestServerInterface defines test server functionality that tests need; it is