serverccl: simplify TestServerStartupGuardrails

Prior to this patch, the test was not cleaning up its server stopper reliably at the end of each sub-test. This patch fixes it. Release note: None
cockroachdb · Apr 6, 2023 · 1491929 · 1491929
1 parent 49f13fc
commit 1491929
Show file tree

Hide file tree

Showing 2 changed files with 46 additions and 57 deletions.
diff --git a/pkg/ccl/serverccl/BUILD.bazel b/pkg/ccl/serverccl/BUILD.bazel
@@ -90,7 +90,6 @@ go_test(
         "//pkg/util/metric",
         "//pkg/util/protoutil",
         "//pkg/util/randutil",
-        "//pkg/util/stop",
         "//pkg/util/timeutil",
         "@com_github_cockroachdb_datadriven//:datadriven",
         "@com_github_cockroachdb_errors//:errors",

diff --git a/pkg/ccl/serverccl/server_startup_guardrails_test.go b/pkg/ccl/serverccl/server_startup_guardrails_test.go
@@ -22,7 +22,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/testutils"
 	"github.com/cockroachdb/cockroach/pkg/testutils/serverutils"
 	"github.com/cockroachdb/cockroach/pkg/util/leaktest"
-	"github.com/cockroachdb/cockroach/pkg/util/stop"
+	"github.com/cockroachdb/cockroach/pkg/util/log"
 )
 
 // TestServerStartupGuardrails ensures that a SQL server will fail to start if
@@ -74,69 +74,59 @@ func TestServerStartupGuardrails(t *testing.T) {
 	}
 
 	for i, test := range tests {
-		storageSettings := cluster.MakeTestingClusterSettingsWithVersions(
-			test.storageBinaryVersion,
-			test.storageBinaryMinSupportedVersion,
-			false, /* initializeVersion */
-		)
+		t.Run(fmt.Sprintf("%d", i), func(t *testing.T) {
+			defer log.Scope(t).Close(t)
 
-		s, _, _ := serverutils.StartServer(t, base.TestServerArgs{
-			// Disable the default test tenant, since we create one explicitly
-			// below.
-			DefaultTestTenant: base.TestTenantDisabled,
-			Settings:          storageSettings,
-			Knobs: base.TestingKnobs{
-				Server: &server.TestingKnobs{
-					BinaryVersionOverride:          test.storageBinaryVersion,
-					BootstrapVersionKeyOverride:    clusterversion.V22_2,
-					DisableAutomaticVersionUpgrade: make(chan struct{}),
-				},
-				SQLEvalContext: &eval.TestingKnobs{
-					TenantLogicalVersionKeyOverride: test.TenantLogicalVersionKey,
-				},
-			},
-		})
+			storageSettings := cluster.MakeTestingClusterSettingsWithVersions(
+				test.storageBinaryVersion,
+				test.storageBinaryMinSupportedVersion,
+				false, /* initializeVersion */
+			)
 
-		tenantSettings := cluster.MakeTestingClusterSettingsWithVersions(
-			test.tenantBinaryVersion,
-			test.tenantBinaryMinSupportedVersion,
-			true, /* initializeVersion */
-		)
-
-		// The tenant will be created with an active version equal to the version
-		// corresponding to TenantLogicalVersionKey. Tenant creation is expected
-		// to succeed for all test cases but server creation is expected to succeed
-		// only if tenantBinaryVersion is at least equal to the version corresponding
-		// to TenantLogicalVersionKey.
-		stopper := stop.NewStopper()
-		tenantServer, err := s.StartTenant(context.Background(),
-			base.TestTenantArgs{
-				Settings: tenantSettings,
-				TenantID: serverutils.TestTenantID(),
-				Stopper:  stopper,
-				TestingKnobs: base.TestingKnobs{
+			s, _, _ := serverutils.StartServer(t, base.TestServerArgs{
+				// Disable the default test tenant, since we create one explicitly
+				// below.
+				DefaultTestTenant: base.TestTenantDisabled,
+				Settings:          storageSettings,
+				Knobs: base.TestingKnobs{
 					Server: &server.TestingKnobs{
-						BinaryVersionOverride:          test.tenantBinaryVersion,
+						BinaryVersionOverride:          test.storageBinaryVersion,
+						BootstrapVersionKeyOverride:    clusterversion.V22_2,
 						DisableAutomaticVersionUpgrade: make(chan struct{}),
 					},
+					SQLEvalContext: &eval.TestingKnobs{
+						TenantLogicalVersionKeyOverride: test.TenantLogicalVersionKey,
+					},
 				},
 			})
+			defer s.Stopper().Stop(context.Background())
 
-		if !testutils.IsError(err, test.expErrMatch) {
-			t.Fatalf("test %d: got error %s, wanted error matching '%s'", i, err, test.expErrMatch)
-		}
+			tenantSettings := cluster.MakeTestingClusterSettingsWithVersions(
+				test.tenantBinaryVersion,
+				test.tenantBinaryMinSupportedVersion,
+				true, /* initializeVersion */
+			)
 
-		// Only attempt to stop the tenant if it was started successfully.
-		if err == nil {
-			tenantServer.Stopper().Stop(context.Background())
-		} else {
-			// Test - stop the failed SQL server using a custom stopper
-			// NOTE: This custom stopper should not be required, but is because
-			// currently, if a SQL server fails to start it will not be cleaned
-			// up immediately without invoking the custom stopper. This could
-			// be a problem, and is tracked with #98868.
-			stopper.Stop(context.Background())
-		}
-		s.Stopper().Stop(context.Background())
+			// The tenant will be created with an active version equal to the version
+			// corresponding to TenantLogicalVersionKey. Tenant creation is expected
+			// to succeed for all test cases but server creation is expected to succeed
+			// only if tenantBinaryVersion is at least equal to the version corresponding
+			// to TenantLogicalVersionKey.
+			_, err := s.StartTenant(context.Background(),
+				base.TestTenantArgs{
+					Settings: tenantSettings,
+					TenantID: serverutils.TestTenantID(),
+					TestingKnobs: base.TestingKnobs{
+						Server: &server.TestingKnobs{
+							BinaryVersionOverride:          test.tenantBinaryVersion,
+							DisableAutomaticVersionUpgrade: make(chan struct{}),
+						},
+					},
+				})
+
+			if !testutils.IsError(err, test.expErrMatch) {
+				t.Fatalf("test %d: got error %s, wanted error matching '%s'", i, err, test.expErrMatch)
+			}
+		})
 	}
 }