Merge #106760 #106899

106760: kvserver: deflake TestMergeQueueSeesNonVoters r=tbg a=miraradeva

TestMergeQueueSeesNonVoters was flaky because of a race between turning the merge queue on and off. The merge queue was first turned on via a cluster config, and then turned off using the store directly. However, it's possible for the cluster config to take effect after the queue was turned off via the store and leave the merge queue on for the rest of the test, causing unexpected range merges. The cluster config is actually unnecessary, so this change just removes it.

Fixes: #105726
Epic: CRDB-29164

Release note: None

106899: diagnosticsccl: remove uses of `TODOTestTenantDisabled` r=dhartunian a=knz

Informs #76378 
Epic: CRDB-18499
First commit from #106900.

Followup issue: #106901


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

pkg/ccl/serverccl/BUILD.bazel

@@ -16,7 +16,6 @@ go_library(
         "//pkg/server/serverpb",
         "//pkg/sql",
         "//pkg/sql/contention",
-        "//pkg/sql/pgwire",
         "//pkg/sql/sqlstats/persistedsqlstats",
         "//pkg/sql/tests",
         "//pkg/testutils/serverutils",

pkg/ccl/serverccl/diagnosticsccl/BUILD.bazel

@@ -10,6 +10,7 @@ go_test(
     tags = ["ccl_test"],
     deps = [
         "//pkg/base",
+        "//pkg/ccl",
         "//pkg/ccl/kvccl/kvtenantccl",
         "//pkg/clusterversion",
         "//pkg/config/zonepb",

pkg/ccl/serverccl/diagnosticsccl/main_test.go

@@ -12,6 +12,7 @@ import (
 	"os"
 	"testing"
 
+	"github.com/cockroachdb/cockroach/pkg/ccl"
 	"github.com/cockroachdb/cockroach/pkg/ccl/kvccl/kvtenantccl"
 	"github.com/cockroachdb/cockroach/pkg/security/securityassets"
 	"github.com/cockroachdb/cockroach/pkg/security/securitytest"
@@ -23,5 +24,6 @@ func TestMain(m *testing.M) {
 	securityassets.SetLoader(securitytest.EmbeddedAssets)
 	serverutils.InitTestServerFactory(server.TestServerFactory)
 	kvtenantccl.InitConnectorFactory()
+	defer ccl.TestingEnableEnterprise()()
 	os.Exit(m.Run())
 }

pkg/ccl/serverccl/diagnosticsccl/reporter_test.go

@@ -45,7 +45,7 @@ func TestTenantReport(t *testing.T) {
 	defer leaktest.AfterTest(t)()
 	defer log.Scope(t).Close(t)
 
-	rt := startReporterTest(t, base.TODOTestTenantDisabled)
+	rt := startReporterTest(t, base.TestControlsTenantsExplicitly)
 	defer rt.Close()
 
 	tenantArgs := base.TestTenantArgs{
@@ -99,8 +99,7 @@ func TestServerReport(t *testing.T) {
 	defer leaktest.AfterTest(t)()
 	defer log.Scope(t).Close(t)
 
-	var defaultTestTenant base.DefaultTestTenantOptions
-	rt := startReporterTest(t, defaultTestTenant)
+	rt := startReporterTest(t, base.TestIsSpecificToStorageLayerAndNeedsASystemTenant)
 	defer rt.Close()
 
 	ctx := context.Background()
@@ -289,6 +288,8 @@ func TestUsageQuantization(t *testing.T) {
 
 	url := r.URL()
 	s, db, _ := serverutils.StartServer(t, base.TestServerArgs{
+		DefaultTestTenant: base.TestDoesNotWorkWithSecondaryTenantsButWeDontKnowWhyYet(106901),
+
 		Settings: st,
 		Knobs: base.TestingKnobs{
 			Server: &server.TestingKnobs{
@@ -299,7 +300,6 @@ func TestUsageQuantization(t *testing.T) {
 		},
 	})
 	defer s.Stopper().Stop(ctx)
-	ts := s.(*server.TestServer)
 
 	// Disable periodic reporting so it doesn't interfere with the test.
 	if _, err := db.Exec(`SET CLUSTER SETTING diagnostics.reporting.enabled = false`); err != nil {
@@ -331,6 +331,8 @@ func TestUsageQuantization(t *testing.T) {
 		require.NoError(t, err)
 	}
 
+	ts := s.TenantOrServer()
+
 	// Flush the SQL stat pool.
 	ts.SQLServer().(*sql.Server).GetSQLStatsController().ResetLocalSQLStats(ctx)
 

pkg/ccl/serverccl/tenant_test_utils.go

@@ -22,7 +22,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/server/serverpb"
 	"github.com/cockroachdb/cockroach/pkg/sql"
 	"github.com/cockroachdb/cockroach/pkg/sql/contention"
-	"github.com/cockroachdb/cockroach/pkg/sql/pgwire"
 	"github.com/cockroachdb/cockroach/pkg/sql/sqlstats/persistedsqlstats"
 	"github.com/cockroachdb/cockroach/pkg/sql/tests"
 	"github.com/cockroachdb/cockroach/pkg/testutils/serverutils"
@@ -100,7 +99,7 @@ func newTestTenant(
 	tenant, tenantConn := serverutils.StartTenant(t, server, args)
 	sqlDB := sqlutils.MakeSQLRunner(tenantConn)
 	status := tenant.StatusServer().(serverpb.SQLStatusServer)
-	sqlStats := tenant.PGServer().(*pgwire.Server).SQLServer.
+	sqlStats := tenant.SQLServer().(*sql.Server).
 		GetSQLStatsProvider().(*persistedsqlstats.PersistedSQLStats)
 	contentionRegistry := tenant.ExecutorConfig().(sql.ExecutorConfig).ContentionRegistry
 

pkg/kv/kvserver/client_merge_test.go

@@ -4722,10 +4722,6 @@ func TestMergeQueueSeesNonVoters(t *testing.T) {
 		t.Run(subtest.name, func(t *testing.T) {
 			tc, _ := setupTestClusterWithDummyRange(t, clusterArgs, dbName, "kv", numNodes)
 			defer tc.Stopper().Stop(ctx)
-			// We're controlling merge queue operation via
-			// `store.SetMergeQueueActive`, so enable the cluster setting here.
-			_, err := tc.ServerConn(0).Exec(`SET CLUSTER SETTING kv.range_merge.queue_enabled=true`)
-			require.NoError(t, err)
 
 			store, err := tc.Server(0).GetStores().(*kvserver.Stores).GetStore(1)
 			require.Nil(t, err)

pkg/server/testserver.go

@@ -665,7 +665,7 @@ func (ts *TestServer) Start(ctx context.Context) error {
 // calling the TestServerInterface.StartTenant method or by calling the wrapper
 // serverutils.StartTenant method.
 type TestTenant struct {
-	*SQLServer
+	sql    *SQLServer
 	Cfg    *BaseConfig
 	SQLCfg *SQLConfig
 	*httpTestServer
@@ -678,6 +678,16 @@ type TestTenant struct {
 
 var _ serverutils.TestTenantInterface = &TestTenant{}
 
+// AnnotateCtx is part of TestTenantInterface.
+func (t *TestTenant) AnnotateCtx(ctx context.Context) context.Context {
+	return t.sql.AnnotateCtx(ctx)
+}
+
+// SQLInstanceID is part of TestTenantInterface.
+func (t *TestTenant) SQLInstanceID() base.SQLInstanceID {
+	return t.sql.SQLInstanceID()
+}
+
 // SQLAddr is part of TestTenantInterface interface.
 func (t *TestTenant) SQLAddr() string {
 	return t.Cfg.SQLAddr
@@ -695,12 +705,12 @@ func (t *TestTenant) RPCAddr() string {
 
 // DB is part of the TestTenantInterface.
 func (t *TestTenant) DB() *kv.DB {
-	return t.execCfg.DB
+	return t.sql.execCfg.DB
 }
 
 // PGServer is part of TestTenantInterface.
 func (t *TestTenant) PGServer() interface{} {
-	return t.pgServer
+	return t.sql.pgServer
 }
 
 // PGPreServer exposes the pgwire.PreServeConnHandler instance used by
@@ -714,47 +724,52 @@ func (ts *TestTenant) PGPreServer() interface{} {
 
 // DiagnosticsReporter is part of TestTenantInterface.
 func (t *TestTenant) DiagnosticsReporter() interface{} {
-	return t.diagnosticsReporter
+	return t.sql.diagnosticsReporter
 }
 
 // StatusServer is part of TestTenantInterface.
 func (t *TestTenant) StatusServer() interface{} {
-	return t.execCfg.SQLStatusServer
+	return t.sql.execCfg.SQLStatusServer
 }
 
 // TenantStatusServer is part of TestTenantInterface.
 func (t *TestTenant) TenantStatusServer() interface{} {
-	return t.execCfg.TenantStatusServer
+	return t.sql.execCfg.TenantStatusServer
+}
+
+// SQLServer is part of TestTenantInterface.
+func (t *TestTenant) SQLServer() interface{} {
+	return t.sql.pgServer.SQLServer
 }
 
 // DistSQLServer is part of TestTenantInterface.
 func (t *TestTenant) DistSQLServer() interface{} {
-	return t.SQLServer.distSQLServer
+	return t.sql.distSQLServer
 }
 
 // DistSenderI is part of the TestTenantInterface.
 func (t *TestTenant) DistSenderI() interface{} {
-	return t.SQLServer.execCfg.DistSender
+	return t.sql.execCfg.DistSender
 }
 
 // RPCContext is part of TestTenantInterface.
 func (t *TestTenant) RPCContext() *rpc.Context {
-	return t.execCfg.RPCContext
+	return t.sql.execCfg.RPCContext
 }
 
 // JobRegistry is part of TestTenantInterface.
 func (t *TestTenant) JobRegistry() interface{} {
-	return t.SQLServer.jobRegistry
+	return t.sql.jobRegistry
 }
 
 // ExecutorConfig is part of TestTenantInterface.
 func (t *TestTenant) ExecutorConfig() interface{} {
-	return *t.SQLServer.execCfg
+	return *t.sql.execCfg
 }
 
 // RangeFeedFactory is part of TestTenantInterface.
 func (t *TestTenant) RangeFeedFactory() interface{} {
-	return t.SQLServer.execCfg.RangeFeedFactory
+	return t.sql.execCfg.RangeFeedFactory
 }
 
 // ClusterSettings is part of TestTenantInterface.
@@ -764,12 +779,12 @@ func (t *TestTenant) ClusterSettings() *cluster.Settings {
 
 // Stopper is part of TestTenantInterface.
 func (t *TestTenant) Stopper() *stop.Stopper {
-	return t.stopper
+	return t.sql.stopper
 }
 
 // Clock is part of TestTenantInterface.
 func (t *TestTenant) Clock() *hlc.Clock {
-	return t.SQLServer.execCfg.Clock
+	return t.sql.execCfg.Clock
 }
 
 // AmbientCtx implements serverutils.TestTenantInterface. This
@@ -786,32 +801,32 @@ func (t *TestTenant) TestingKnobs() *base.TestingKnobs {
 
 // SpanConfigKVAccessor is part TestTenantInterface.
 func (t *TestTenant) SpanConfigKVAccessor() interface{} {
-	return t.SQLServer.tenantConnect
+	return t.sql.tenantConnect
 }
 
 // SpanConfigReporter is part TestTenantInterface.
 func (t *TestTenant) SpanConfigReporter() interface{} {
-	return t.SQLServer.tenantConnect
+	return t.sql.tenantConnect
 }
 
 // SpanConfigReconciler is part TestTenantInterface.
 func (t *TestTenant) SpanConfigReconciler() interface{} {
-	return t.SQLServer.spanconfigMgr.Reconciler
+	return t.sql.spanconfigMgr.Reconciler
 }
 
 // SpanConfigSQLTranslatorFactory is part TestTenantInterface.
 func (t *TestTenant) SpanConfigSQLTranslatorFactory() interface{} {
-	return t.SQLServer.spanconfigSQLTranslatorFactory
+	return t.sql.spanconfigSQLTranslatorFactory
 }
 
 // SpanConfigSQLWatcher is part TestTenantInterface.
 func (t *TestTenant) SpanConfigSQLWatcher() interface{} {
-	return t.SQLServer.spanconfigSQLWatcher
+	return t.sql.spanconfigSQLWatcher
 }
 
 // SystemConfigProvider is part TestTenantInterface.
 func (t *TestTenant) SystemConfigProvider() config.SystemConfigProvider {
-	return t.SQLServer.systemConfigWatcher
+	return t.sql.systemConfigWatcher
 }
 
 // DrainClients exports the drainClients() method for use by tests.
@@ -821,27 +836,27 @@ func (t *TestTenant) DrainClients(ctx context.Context) error {
 
 // MustGetSQLCounter implements TestTenantInterface.
 func (t *TestTenant) MustGetSQLCounter(name string) int64 {
-	return mustGetSQLCounterForRegistry(t.metricsRegistry, name)
+	return mustGetSQLCounterForRegistry(t.sql.metricsRegistry, name)
 }
 
 // RangeDescIteratorFactory implements the TestTenantInterface.
 func (t *TestTenant) RangeDescIteratorFactory() interface{} {
-	return t.SQLServer.execCfg.RangeDescIteratorFactory
+	return t.sql.execCfg.RangeDescIteratorFactory
 }
 
 // Codec is part of the TestTenantInterface.
 func (t *TestTenant) Codec() keys.SQLCodec {
-	return t.execCfg.Codec
+	return t.sql.execCfg.Codec
 }
 
 // Tracer is part of the TestTenantInterface.
 func (t *TestTenant) Tracer() *tracing.Tracer {
-	return t.SQLServer.ambientCtx.Tracer
+	return t.sql.ambientCtx.Tracer
 }
 
 // SettingsWatcher is part of the TestTenantInterface.
 func (t *TestTenant) SettingsWatcher() interface{} {
-	return t.SQLServer.settingsWatcher
+	return t.sql.settingsWatcher
 }
 
 // StartSharedProcessTenant is part of TestServerInterface.
@@ -930,7 +945,7 @@ func (ts *TestServer) StartSharedProcessTenant(
 	hts.t.tenantName = args.TenantName
 
 	testTenant := &TestTenant{
-		SQLServer:      sqlServer,
+		sql:            sqlServer,
 		Cfg:            sqlServer.cfg,
 		SQLCfg:         sqlServerWrapper.sqlCfg,
 		pgPreServer:    sqlServerWrapper.pgPreServer,
@@ -953,7 +968,7 @@ func (ts *TestServer) DisableStartTenant(reason error) {
 
 // MigrationServer is part of the TestTenantInterface.
 func (t *TestTenant) MigrationServer() interface{} {
-	return t.migrationServer
+	return t.sql.migrationServer
 }
 
 // StartTenant is part of TestServerInterface.
@@ -1202,7 +1217,7 @@ func (ts *TestServer) StartTenant(
 	hts.t.sqlServer = sw.sqlServer
 
 	return &TestTenant{
-		SQLServer:      sw.sqlServer,
+		sql:            sw.sqlServer,
 		Cfg:            &baseCfg,
 		SQLCfg:         &sqlCfg,
 		pgPreServer:    sw.pgPreServer,
@@ -1460,12 +1475,12 @@ func (ts *TestServer) SpanConfigSQLWatcher() interface{} {
 	return ts.sqlServer.spanconfigSQLWatcher
 }
 
-// SQLServer is part of TestServerInterface.
+// SQLServer is part of TestTenantInterface.
 func (ts *TestServer) SQLServer() interface{} {
 	return ts.sqlServer.pgServer.SQLServer
 }
 
-// DistSQLServer is part of TestServerInterface.
+// DistSQLServer is part of TestTenantInterface.
 func (ts *TestServer) DistSQLServer() interface{} {
 	return ts.sqlServer.distSQLServer
 }

pkg/sql/internal_test.go

@@ -154,9 +154,9 @@ func TestInternalFullTableScan(t *testing.T) {
 		err.Error())
 
 	mon := sql.MakeInternalExecutorMemMonitor(sql.MemoryMetrics{}, s.ClusterSettings())
-	mon.StartNoReserved(ctx, s.(*server.TestServer).Server.PGServer().SQLServer.GetBytesMonitor())
+	mon.StartNoReserved(ctx, s.SQLServer().(*sql.Server).GetBytesMonitor())
 	ie := sql.MakeInternalExecutor(
-		s.(*server.TestServer).Server.PGServer().SQLServer, sql.MemoryMetrics{}, mon,
+		s.SQLServer().(*sql.Server), sql.MemoryMetrics{}, mon,
 	)
 	ie.SetSessionData(
 		&sessiondata.SessionData{
@@ -193,9 +193,9 @@ func TestInternalStmtFingerprintLimit(t *testing.T) {
 	require.NoError(t, err)
 
 	mon := sql.MakeInternalExecutorMemMonitor(sql.MemoryMetrics{}, s.ClusterSettings())
-	mon.StartNoReserved(ctx, s.(*server.TestServer).Server.PGServer().SQLServer.GetBytesMonitor())
+	mon.StartNoReserved(ctx, s.SQLServer().(*sql.Server).GetBytesMonitor())
 	ie := sql.MakeInternalExecutor(
-		s.(*server.TestServer).Server.PGServer().SQLServer, sql.MemoryMetrics{}, mon,
+		s.SQLServer().(*sql.Server), sql.MemoryMetrics{}, mon,
 	)
 	_, err = ie.Exec(ctx, "stmt-exceeds-fingerprint-limit", nil, "SELECT 1")
 	require.NoError(t, err)
@@ -324,9 +324,9 @@ func TestSessionBoundInternalExecutor(t *testing.T) {
 
 	expDB := "foo"
 	mon := sql.MakeInternalExecutorMemMonitor(sql.MemoryMetrics{}, s.ClusterSettings())
-	mon.StartNoReserved(ctx, s.(*server.TestServer).Server.PGServer().SQLServer.GetBytesMonitor())
+	mon.StartNoReserved(ctx, s.SQLServer().(*sql.Server).GetBytesMonitor())
 	ie := sql.MakeInternalExecutor(
-		s.(*server.TestServer).Server.PGServer().SQLServer, sql.MemoryMetrics{}, mon,
+		s.SQLServer().(*sql.Server), sql.MemoryMetrics{}, mon,
 	)
 	ie.SetSessionData(
 		&sessiondata.SessionData{
@@ -391,9 +391,9 @@ func TestInternalExecAppNameInitialization(t *testing.T) {
 		defer s.Stopper().Stop(context.Background())
 
 		mon := sql.MakeInternalExecutorMemMonitor(sql.MemoryMetrics{}, s.ClusterSettings())
-		mon.StartNoReserved(context.Background(), s.(*server.TestServer).Server.PGServer().SQLServer.GetBytesMonitor())
+		mon.StartNoReserved(context.Background(), s.SQLServer().(*sql.Server).GetBytesMonitor())
 		ie := sql.MakeInternalExecutor(
-			s.(*server.TestServer).Server.PGServer().SQLServer, sql.MemoryMetrics{}, mon,
+			s.SQLServer().(*sql.Server), sql.MemoryMetrics{}, mon,
 		)
 		ie.SetSessionData(
 			&sessiondata.SessionData{

pkg/testutils/serverutils/test_server_shim.go

@@ -160,9 +160,6 @@ type TestServerInterface interface {
 	// MigrationServer returns the internal *migrationServer as in interface{}
 	MigrationServer() interface{}
 
-	// SQLServer returns the *sql.Server as an interface{}.
-	SQLServer() interface{}
-
 	// SQLLivenessProvider returns the sqlliveness.Provider as an interface{}.
 	SQLLivenessProvider() interface{}