Merge #139436

139436: server: enable tenant testing for server/admin APIs r=cthumuluru-crdb a=shubhamdhama

`` server: copy `StubTimeNow` server testing knobs to tenants ``

`` server: enable tenant testing for server/admin APIs ``

This change is mostly straightforward:
* Removed remaining usages of `TODOTestTenantDisabled`, enabling tenant
  testing for those tests.
* Made some improvements to a couple of tests.
* Created a new function (hopefully temporary)
  `TestSkippedForExternalModeDueToPerformance` to exclude external mode for
  `TestAdminAPIEvents` due to poor performance.

Informs: #138912
Epic: CRDB-38970
Release note: None

Co-authored-by: Shubham Dhama <[email protected]>

Author: craig[bot]

pkg/base/test_server_args.go

@@ -519,6 +519,21 @@ func TestIsForStuffThatShouldWorkWithSharedProcessModeButDoesntYet(
 	}
 }
 
+// TestSkippedForExternalModeDueToPerformance can be used to disable selecting
+// the external process virtual cluster due to significant performance
+// degradation compared to other modes. However, the goal is to eventually make
+// it work efficiently in external mode.
+//
+// It should link to a github issue with label C-investigation.
+func TestSkippedForExternalModeDueToPerformance(issueNumber int) DefaultTestTenantOptions {
+	return DefaultTestTenantOptions{
+		testBehavior:           ttSharedProcess,
+		allowAdditionalTenants: true,
+		issueNum:               issueNumber,
+		label:                  "C-investigation",
+	}
+}
+
 // InternalNonDefaultDecision builds a sentinel value used inside a
 // mechanism in serverutils. Should not be used by tests directly.
 func InternalNonDefaultDecision(

pkg/ccl/serverccl/admin_test.go

@@ -118,10 +118,7 @@ func TestAdminAPIJobs(t *testing.T) {
 
 	dir, dirCleanupFn := testutils.TempDir(t)
 	defer dirCleanupFn()
-	s, conn, _ := serverutils.StartServer(t, base.TestServerArgs{
-		// Fails with the default test tenant. Tracked with #76378.
-		DefaultTestTenant: base.TODOTestTenantDisabled,
-		ExternalIODir:     dir})
+	s, conn, _ := serverutils.StartServer(t, base.TestServerArgs{ExternalIODir: dir})
 	defer s.Stopper().Stop(context.Background())
 	sqlDB := sqlutils.MakeSQLRunner(conn)
 

pkg/server/application_api/BUILD.bazel

@@ -75,8 +75,10 @@ go_test(
         "//pkg/sql/sqlstats",
         "//pkg/sql/sqlstats/persistedsqlstats",
         "//pkg/sql/sqlstats/persistedsqlstats/sqlstatstestutil",
+        "//pkg/storage/fs",
         "//pkg/testutils",
         "//pkg/testutils/diagutils",
+        "//pkg/testutils/listenerutil",
         "//pkg/testutils/serverutils",
         "//pkg/testutils/skip",
         "//pkg/testutils/sqlutils",

pkg/server/application_api/events_test.go

@@ -13,22 +13,50 @@ import (
 	"time"
 
 	"github.com/cockroachdb/cockroach/pkg/base"
+	"github.com/cockroachdb/cockroach/pkg/server"
 	"github.com/cockroachdb/cockroach/pkg/server/serverpb"
 	"github.com/cockroachdb/cockroach/pkg/server/srvtestutils"
-	"github.com/cockroachdb/cockroach/pkg/testutils/serverutils"
+	"github.com/cockroachdb/cockroach/pkg/storage/fs"
+	"github.com/cockroachdb/cockroach/pkg/testutils/listenerutil"
+	"github.com/cockroachdb/cockroach/pkg/testutils/testcluster"
 	"github.com/cockroachdb/cockroach/pkg/util/leaktest"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
+	"github.com/stretchr/testify/require"
 )
 
 func TestAdminAPIEvents(t *testing.T) {
 	defer leaktest.AfterTest(t)()
 	defer log.Scope(t).Close(t)
-	s, db, _ := serverutils.StartServer(t, base.TestServerArgs{
-		// Disable the default test tenant for now as this tests fails
-		// with it enabled. Tracked with #81590.
-		DefaultTestTenant: base.TODOTestTenantDisabled,
+	ctx := context.Background()
+
+	stickyVFSRegistry := fs.NewStickyRegistry()
+	lisReg := listenerutil.NewListenerRegistry()
+	defer lisReg.Close()
+
+	tc := testcluster.StartTestCluster(t, 1, base.TestClusterArgs{
+		ReusableListenerReg: lisReg,
+		ServerArgs: base.TestServerArgs{
+			// TODO: time taken by first /events API call is unusually high
+			// (>40s) for external mode.
+			DefaultTestTenant: base.TestSkippedForExternalModeDueToPerformance(142381),
+			// Below parameters are required to make tc.Restart work
+			StoreSpecs: []base.StoreSpec{
+				{
+					InMemory:    true,
+					StickyVFSID: "1",
+				},
+			},
+			Knobs: base.TestingKnobs{
+				Server: &server.TestingKnobs{
+					StickyVFSRegistry: stickyVFSRegistry,
+				},
+			},
+		},
 	})
-	defer s.Stopper().Stop(context.Background())
+	require.NoError(t, tc.Restart())
+	defer tc.Stopper().Stop(ctx)
+	s := tc.Server(0)
+	db := tc.ServerConn(0)
 
 	setupQueries := []string{
 		"CREATE DATABASE api_test",
@@ -37,6 +65,7 @@ func TestAdminAPIEvents(t *testing.T) {
 		"CREATE TABLE api_test.tbl3 (a INT)",
 		"DROP TABLE api_test.tbl1",
 		"DROP TABLE api_test.tbl2",
+		"DROP DATABASE api_test",
 		"SET CLUSTER SETTING cluster.label = 'somestring';",
 	}
 	for _, q := range setupQueries {
@@ -54,9 +83,7 @@ func TestAdminAPIEvents(t *testing.T) {
 		expCount   int
 	}
 	testcases := []testcase{
-		{"node_join", false, 0, false, 1},
-		{"node_restart", false, 0, false, 0},
-		{"drop_database", false, 0, false, 0},
+		{"drop_database", false, 0, false, 1},
 		{"create_database", false, 0, false, 3},
 		{"drop_table", false, 0, false, 2},
 		{"create_table", false, 0, false, 3},
@@ -68,6 +95,19 @@ func TestAdminAPIEvents(t *testing.T) {
 		{"create_table", true, -1, false, 3},
 		{"create_table", true, 2, false, 2},
 	}
+
+	if s.StartedDefaultTestTenant() {
+		testcases = append(testcases, []testcase{
+			{"node_join", false, 0, false, 0},
+			{"node_restart", false, 0, false, 0},
+		}...)
+	} else {
+		testcases = append(testcases, []testcase{
+			{"node_join", false, 0, false, 1},
+			{"node_restart", false, 0, false, 1},
+		}...)
+	}
+
 	minTotalEvents := 0
 	for _, tc := range testcases {
 		if !tc.hasLimit {

pkg/server/application_api/insights_test.go

@@ -74,9 +74,6 @@ func TestDatabaseAndTableIndexRecommendations(t *testing.T) {
 	stubDropUnusedDuration := time.Hour
 
 	s, sqlDB, _ := serverutils.StartServer(t, base.TestServerArgs{
-		// Disable the default test tenant for now as this tests fails
-		// with it enabled. Tracked with #81590.
-		DefaultTestTenant: base.TODOTestTenantDisabled,
 		Knobs: base.TestingKnobs{
 			UnusedIndexRecommendKnobs: &idxusage.UnusedIndexRecommendationTestingKnobs{
 				GetCreatedAt:   stubTime.getCreatedAt,

pkg/server/application_api/jobs_test.go

@@ -77,9 +77,6 @@ func TestAdminAPIJobs(t *testing.T) {
 	now := timeutil.Now()
 	retentionTime := 336 * time.Hour
 	s, conn, _ := serverutils.StartServer(t, base.TestServerArgs{
-		// Disable the default test tenant for now as this tests fails
-		// with it enabled. Tracked with #81590.
-		DefaultTestTenant: base.TODOTestTenantDisabled,
 		Knobs: base.TestingKnobs{
 			JobsTestingKnobs: &jobs.TestingKnobs{
 				IntervalOverrides: jobs.TestingIntervalOverrides{
@@ -249,8 +246,8 @@ func TestAdminAPIJobs(t *testing.T) {
 		},
 	}
 
-	testutils.RunTrueAndFalse(t, "isAdmin", func(t *testing.T, isAdmin bool) {
-		for i, testCase := range testCases {
+	for i, testCase := range testCases {
+		testutils.RunTrueAndFalse(t, fmt.Sprintf("%s-isAdmin", testCase.uri), func(t *testing.T, isAdmin bool) {
 			var res serverpb.JobsResponse
 			if err := srvtestutils.GetAdminJSONProtoWithAdminOption(s, testCase.uri, &res, isAdmin); err != nil {
 				t.Fatal(err)
@@ -278,19 +275,15 @@ func TestAdminAPIJobs(t *testing.T) {
 			// We don't use require.Equal() because timestamps don't necessarily
 			// compare == due to only one of them having a monotonic clock reading.
 			require.True(t, now.Add(-retentionTime).Equal(res.EarliestRetainedTime))
-		}
-	})
+		})
+	}
 }
 
 func TestAdminAPIJobsDetails(t *testing.T) {
 	defer leaktest.AfterTest(t)()
 	defer log.Scope(t).Close(t)
 
-	s, conn, _ := serverutils.StartServer(t, base.TestServerArgs{
-		// Disable the default test tenant for now as this tests fails
-		// with it enabled. Tracked with #81590.
-		DefaultTestTenant: base.TODOTestTenantDisabled,
-	})
+	s, conn, _ := serverutils.StartServer(t, base.TestServerArgs{})
 	defer s.Stopper().Stop(context.Background())
 	sqlDB := sqlutils.MakeSQLRunner(conn)
 

pkg/server/application_api/query_plan_test.go

@@ -25,11 +25,7 @@ func TestAdminAPIQueryPlan(t *testing.T) {
 	defer leaktest.AfterTest(t)()
 	defer log.Scope(t).Close(t)
 
-	s, conn, _ := serverutils.StartServer(t, base.TestServerArgs{
-		// Disable the default test tenant for now as this tests fails
-		// with it enabled. Tracked with #81590.
-		DefaultTestTenant: base.TODOTestTenantDisabled,
-	})
+	s, conn, _ := serverutils.StartServer(t, base.TestServerArgs{})
 	defer s.Stopper().Stop(context.Background())
 	sqlDB := sqlutils.MakeSQLRunner(conn)
 

pkg/server/application_api/security_test.go

@@ -22,11 +22,7 @@ import (
 func TestAdminAPIUsers(t *testing.T) {
 	defer leaktest.AfterTest(t)()
 	defer log.Scope(t).Close(t)
-	s, db, _ := serverutils.StartServer(t, base.TestServerArgs{
-		// Disable the default test tenant for now as this tests fails
-		// with it enabled. Tracked with #81590.
-		DefaultTestTenant: base.TODOTestTenantDisabled,
-	})
+	s, db, _ := serverutils.StartServer(t, base.TestServerArgs{})
 	defer s.Stopper().Stop(context.Background())
 
 	// Create sample users.

pkg/server/application_api/zcfg_test.go

@@ -27,11 +27,7 @@ import (
 func TestAdminAPIZoneDetails(t *testing.T) {
 	defer leaktest.AfterTest(t)()
 	defer log.Scope(t).Close(t)
-	s, db, _ := serverutils.StartServer(t, base.TestServerArgs{
-		// Disable the default test tenant for now as this tests fails
-		// with it enabled. Tracked with #81590.
-		DefaultTestTenant: base.TODOTestTenantDisabled,
-	})
+	s, db, _ := serverutils.StartServer(t, base.TestServerArgs{})
 	defer s.Stopper().Stop(context.Background())
 
 	// Create database and table.

pkg/server/storage_api/rangelog_test.go

@@ -25,11 +25,7 @@ import (
 func TestAdminAPIRangeLogByRangeID(t *testing.T) {
 	defer leaktest.AfterTest(t)()
 	defer log.Scope(t).Close(t)
-	s, db, _ := serverutils.StartServer(t, base.TestServerArgs{
-		// Disable the default test tenant for now as this tests fails
-		// with it enabled. Tracked with #81590.
-		DefaultTestTenant: base.TODOTestTenantDisabled,
-	})
+	s, db, _ := serverutils.StartServer(t, base.TestServerArgs{})
 	defer s.Stopper().Stop(context.Background())
 
 	rangeID := 654321
@@ -95,9 +91,6 @@ func TestAdminAPIFullRangeLog(t *testing.T) {
 	defer log.Scope(t).Close(t)
 	s, db, _ := serverutils.StartServer(t,
 		base.TestServerArgs{
-			// Disable the default test tenant for now as this tests fails
-			// with it enabled. Tracked with #81590.
-			DefaultTestTenant: base.TODOTestTenantDisabled,
 			Knobs: base.TestingKnobs{
 				Store: &kvserver.StoreTestingKnobs{
 					DisableSplitQueue: true,

pkg/server/testserver.go

@@ -681,6 +681,7 @@ func (ts *testServer) setupTenantTestingKnobs(tenantKnobs *base.TestingKnobs) {
 			InjectedLatencyOracle:  ts.params.Knobs.Server.(*TestingKnobs).ContextTestingKnobs.InjectedLatencyOracle,
 			InjectedLatencyEnabled: ts.params.Knobs.Server.(*TestingKnobs).ContextTestingKnobs.InjectedLatencyEnabled,
 		}
+		tenantKnobs.Server.(*TestingKnobs).StubTimeNow = ts.params.Knobs.Server.(*TestingKnobs).StubTimeNow
 	}
 	if ts.params.Knobs.UpgradeManager != nil {
 		tenantKnobs.UpgradeManager.(*upgradebase.TestingKnobs).SkipSomeUpgradeSteps = ts.params.Knobs.UpgradeManager.(*upgradebase.TestingKnobs).SkipSomeUpgradeSteps