testcluster: fix TestDRPCEnabledRandomly for cluster with multiple nodes

This change fixes the behavior where it was deciding the DRPC enablement
for each node independently for TestDRPCEnabledRandomly option, leading to
a cluster startup with some nodes with DRPC enabled, and others not.

Now the DRPC option is resolved once at the cluster level and applied
consistently to all nodes, ensuring uniform DRPC configuration across
the entire test cluster.

Author: shubhamdhama

pkg/server/testserver.go

@@ -333,6 +333,10 @@ func makeTestConfigFromParams(params base.TestServerArgs) Config {
 		cfg.TestingKnobs.AdmissionControlOptions = &admission.Options{}
 	}
 
+	if params.DefaultDRPCOption == base.TestDRPCEnabled {
+		rpc.ExperimentalDRPCEnabled.Override(context.Background(), &st.SV, true)
+	}
+
 	return cfg
 }
 

pkg/testutils/serverutils/test_server_shim.go

@@ -24,10 +24,8 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/kv"
 	"github.com/cockroachdb/cockroach/pkg/multitenant/tenantcapabilitiespb"
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
-	"github.com/cockroachdb/cockroach/pkg/rpc"
 	"github.com/cockroachdb/cockroach/pkg/security/securitytest"
 	"github.com/cockroachdb/cockroach/pkg/security/username"
-	"github.com/cockroachdb/cockroach/pkg/settings/cluster"
 	"github.com/cockroachdb/cockroach/pkg/testutils/pgurlutils"
 	"github.com/cockroachdb/cockroach/pkg/testutils/skip"
 	"github.com/cockroachdb/cockroach/pkg/util/envutil"
@@ -274,11 +272,9 @@ func StartServerOnlyE(t TestLogger, params base.TestServerArgs) (TestServerInter
 	ctx := context.Background()
 	allowAdditionalTenants := params.DefaultTestTenant.AllowAdditionalTenants()
 
-	// Update the flags with the actual decision as to whether we should
-	// start the service for a default test tenant.
+	// Update the flags with the actual decisions for test configuration.
 	params.DefaultTestTenant = ShouldStartDefaultTestTenant(t, params.DefaultTestTenant)
-
-	TryEnableDRPCSetting(ctx, t, &params)
+	params.DefaultDRPCOption = ShouldEnableDRPC(ctx, t, params.DefaultDRPCOption)
 
 	s, err := NewServer(params)
 	if err != nil {
@@ -541,11 +537,12 @@ func WaitForTenantCapabilities(
 	}
 }
 
-// TryEnableDRPCSetting determines whether to enable the DRPC cluster setting
-// based on the `TestServerArgs.DefaultDRPCOption` and updates the
-// `TestServerArgs.Settings` based on that.
-func TryEnableDRPCSetting(ctx context.Context, t TestLogger, args *base.TestServerArgs) {
-	option := args.DefaultDRPCOption
+// ShouldEnableDRPC determines the final DRPC option based on the input
+// option and any global overrides, resolving random choices to a concrete
+// enabled/disabled state.
+func ShouldEnableDRPC(
+	ctx context.Context, t TestLogger, option base.DefaultTestDRPCOption,
+) base.DefaultTestDRPCOption {
 	var logSuffix string
 	if option == base.TestDRPCUnset && globalDefaultDRPCOptionOverride.isSet {
 		option = globalDefaultDRPCOptionOverride.value
@@ -559,13 +556,11 @@ func TryEnableDRPCSetting(ctx context.Context, t TestLogger, args *base.TestServ
 		rng, _ := randutil.NewTestRand()
 		enableDRPC = rng.Intn(2) == 0
 	}
-	if !enableDRPC {
-		return
-	}
 
-	t.Log("DRPC is enabled" + logSuffix)
-	if args.Settings == nil {
-		args.Settings = cluster.MakeClusterSettings()
+	if enableDRPC {
+		t.Log("DRPC is enabled" + logSuffix)
+		return base.TestDRPCEnabled
 	}
-	rpc.ExperimentalDRPCEnabled.Override(ctx, &args.Settings.SV, true)
+
+	return base.TestDRPCDisabled
 }

pkg/testutils/testcluster/testcluster.go

@@ -79,6 +79,7 @@ type TestCluster struct {
 	clusterArgs base.TestClusterArgs
 
 	defaultTestTenantOptions base.DefaultTestTenantOptions
+	defaultDRPCOption        base.DefaultTestDRPCOption
 
 	t serverutils.TestFataler
 }
@@ -260,6 +261,40 @@ func PrintTimings(testMain time.Duration) {
 	}
 }
 
+// validateDefaultTestTenant checks that per-server args don't override
+// the top-level DefaultTestTenant setting inconsistently.
+func (tc *TestCluster) validateDefaultTestTenant(
+	t serverutils.TestFataler, nodes int, defaultTestTenantOptions base.DefaultTestTenantOptions,
+) {
+	for i := range nodes {
+		if args, ok := tc.clusterArgs.ServerArgsPerNode[i]; ok &&
+			args.DefaultTestTenant != (base.DefaultTestTenantOptions{}) &&
+			args.DefaultTestTenant != defaultTestTenantOptions {
+			tc.Stopper().Stop(context.Background())
+			t.Fatalf("improper use of DefaultTestTenantOptions in per-server args: %v vs %v\n"+
+				"Tip: use the top-level ServerArgs to set the default test tenant options.",
+				args.DefaultTestTenant, defaultTestTenantOptions)
+		}
+	}
+}
+
+// validateDefaultDRPCOption checks that per-server args don't override
+// the top-level DefaultDRPCOption setting inconsistently.
+func (tc *TestCluster) validateDefaultDRPCOption(
+	t serverutils.TestFataler, nodes int, defaultDRPCOption base.DefaultTestDRPCOption,
+) {
+	for i := range nodes {
+		if args, ok := tc.clusterArgs.ServerArgsPerNode[i]; ok &&
+			args.DefaultDRPCOption != base.TestDRPCUnset &&
+			args.DefaultDRPCOption != defaultDRPCOption {
+			tc.Stopper().Stop(context.Background())
+			t.Fatalf("improper use of DefaultDRPCOption in per-server args: %v vs %v\n"+
+				"Tip: use the top-level ServerArgs to set the default DRPC option.",
+				args.DefaultDRPCOption, defaultDRPCOption)
+		}
+	}
+}
+
 // StartTestCluster creates and starts up a TestCluster made up of `nodes`
 // in-memory testing servers.
 // The cluster should be stopped using TestCluster.Stopper().Stop().
@@ -314,23 +349,18 @@ func NewTestCluster(
 		noLocalities = false
 	}
 
-	// Find out how to do the default test tenant.
-	// The choice should be made by the top-level ServerArgs.
+	// Resolve the test tenant configuration for the cluster. The choice should
+	// be made by the top-level ServerArgs.
 	defaultTestTenantOptions := tc.clusterArgs.ServerArgs.DefaultTestTenant
-	// API check: verify that no non-default choice was made via per-server args,
-	// and inform the user otherwise.
-	for i := 0; i < nodes; i++ {
-		if args, ok := tc.clusterArgs.ServerArgsPerNode[i]; ok &&
-			args.DefaultTestTenant != (base.DefaultTestTenantOptions{}) &&
-			args.DefaultTestTenant != defaultTestTenantOptions {
-			tc.Stopper().Stop(context.Background())
-			t.Fatalf("improper use of DefaultTestTenantOptions in per-server args: %v vs %v\n"+
-				"Tip: use the top-level ServerArgs to set the default test tenant options.",
-				args.DefaultTestTenant, defaultTestTenantOptions)
-		}
-	}
+	tc.validateDefaultTestTenant(t, nodes, defaultTestTenantOptions)
 	tc.defaultTestTenantOptions = serverutils.ShouldStartDefaultTestTenant(t, defaultTestTenantOptions)
 
+	// Resolve the DRPC configuration for the cluster. The choice should be made
+	// by the top-level ServerArgs.
+	defaultDRPCOption := tc.clusterArgs.ServerArgs.DefaultDRPCOption
+	tc.validateDefaultDRPCOption(t, nodes, defaultDRPCOption)
+	tc.defaultDRPCOption = serverutils.ShouldEnableDRPC(context.Background(), t, defaultDRPCOption)
+
 	var firstListener net.Listener
 	for i := 0; i < nodes; i++ {
 		var serverArgs base.TestServerArgs
@@ -346,8 +376,6 @@ func NewTestCluster(
 			serverArgs.Settings = cluster.TestingCloneClusterSettings(serverArgs.Settings)
 		}
 
-		serverutils.TryEnableDRPCSetting(context.Background(), t, &serverArgs)
-
 		// If a reusable listener registry is provided, create reusable listeners
 		// for every server that doesn't have a custom listener provided. (Only
 		// servers with a reusable listener can be restarted).
@@ -650,9 +678,10 @@ func (tc *TestCluster) AddServer(
 		serverArgs.Addr = serverArgs.Listener.Addr().String()
 	}
 
-	// Inject the decision that was made about whether or not to start a
-	// test tenant server, into this new server's configuration.
+	// Inject the decisions that were made about test configuration
+	// into this new server's configuration.
 	serverArgs.DefaultTestTenant = tc.defaultTestTenantOptions
+	serverArgs.DefaultDRPCOption = tc.defaultDRPCOption
 
 	s, err := serverutils.NewServer(serverArgs)
 	if err != nil {