Skip to content

Commit 011a51d

Browse files
committed
drpc: add DefaultTestDRPCEnableOption to TestServerArgs
This entire setup is similar to `DefaultTestTenantOptions`. This option can be configured to run a server with DRPC enabled. The plan is to enable as many test packages as possible at the global level using `TestingGlobalDRPCEnableOption` in `TestMain` and, for a select few others, just specify this option individually in their `TestServerArgs`. One follow-up to this PR is to simulate a mixed-mode server where, in `TestClusterArgs.ServerArgsPerNode`, we specify some nodes with DRPC enabled and others not. This would require some testing knobs because enabling a setting on one node will take effect for others as well, and we would require some refactoring in other production code paths too. Release note: none Epic: CRDB-48935
1 parent e1c1794 commit 011a51d

File tree

4 files changed

+95
-2
lines changed

4 files changed

+95
-2
lines changed

pkg/base/test_server_args.go

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -159,6 +159,11 @@ type TestServerArgs struct {
159159
// below for alternative options that suits your test case.
160160
DefaultTestTenant DefaultTestTenantOptions
161161

162+
// DefaultDRPCOption specifies the DRPC enablement mode for a test
163+
// server. This controls whether inter-node connectivity uses DRPC, just
164+
// gRPC, or is chosen randomly.
165+
DefaultDRPCOption DefaultTestDRPCOption
166+
162167
// DefaultTenantName is the name of the tenant created implicitly according
163168
// to DefaultTestTenant. It is typically `test-tenant` for unit tests and
164169
// always `demoapp` for the cockroach demo.
@@ -224,6 +229,25 @@ type SlimTestServerConfig struct {
224229
Options slimOptions
225230
}
226231

232+
// DefaultTestDRPCOption specifies the DRPC enablement mode for a test
233+
// server. This controls whether inter-node connectivity uses DRPC, just gRPC,
234+
// or is chosen randomly.
235+
type DefaultTestDRPCOption uint8
236+
237+
const (
238+
// TestDRPCDisabled disables DRPC; all inter-node connectivity will use gRPC
239+
// only.
240+
TestDRPCDisabled DefaultTestDRPCOption = iota
241+
242+
// TestDRPCEnabled enables DRPC. Some services may still use gRPC if they
243+
// have not yet migrated to DRPC.
244+
TestDRPCEnabled
245+
246+
// TestDRPCEnabledRandomly randomly chooses between the behavior of
247+
// TestDRPCDisabled or TestDRPCEnabled.
248+
TestDRPCEnabledRandomly
249+
)
250+
227251
// TestClusterArgs contains the parameters one can set when creating a test
228252
// cluster. It contains a TestServerArgs instance which will be copied over to
229253
// every server.

pkg/server/drpc_test.go

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,8 @@ import (
1515
"github.com/cockroachdb/cockroach/pkg/rpc"
1616
"github.com/cockroachdb/cockroach/pkg/settings/cluster"
1717
"github.com/cockroachdb/cockroach/pkg/testutils"
18+
"github.com/cockroachdb/cockroach/pkg/testutils/serverutils"
19+
"github.com/cockroachdb/cockroach/pkg/testutils/sqlutils"
1820
"github.com/cockroachdb/cockroach/pkg/testutils/testcluster"
1921
"github.com/cockroachdb/cockroach/pkg/util/leaktest"
2022
"github.com/cockroachdb/cockroach/pkg/util/log"
@@ -159,3 +161,21 @@ func TestStreamContextCancel(t *testing.T) {
159161
singleRequest()
160162
}
161163
}
164+
165+
func TestDefaultDRPCOption(t *testing.T) {
166+
defer leaktest.AfterTest(t)()
167+
defer log.Scope(t).Close(t)
168+
169+
testutils.RunTrueAndFalse(t, "drpc-enabled", func(t *testing.T, option bool) {
170+
opt := base.TestDRPCDisabled
171+
if option {
172+
opt = base.TestDRPCEnabled
173+
}
174+
s := serverutils.StartServerOnly(t, base.TestServerArgs{DefaultDRPCOption: opt})
175+
defer s.Stopper().Stop(context.Background())
176+
177+
var enabled bool
178+
sqlutils.MakeSQLRunner(s.SQLConn(t)).QueryRow(t, "SHOW CLUSTER SETTING rpc.experimental_drpc.enabled").Scan(&enabled)
179+
require.Equal(t, option, enabled)
180+
})
181+
}

pkg/testutils/serverutils/test_server_shim.go

Lines changed: 49 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -24,8 +24,10 @@ import (
2424
"github.com/cockroachdb/cockroach/pkg/kv"
2525
"github.com/cockroachdb/cockroach/pkg/multitenant/tenantcapabilitiespb"
2626
"github.com/cockroachdb/cockroach/pkg/roachpb"
27+
"github.com/cockroachdb/cockroach/pkg/rpc"
2728
"github.com/cockroachdb/cockroach/pkg/security/securitytest"
2829
"github.com/cockroachdb/cockroach/pkg/security/username"
30+
"github.com/cockroachdb/cockroach/pkg/settings/cluster"
2931
"github.com/cockroachdb/cockroach/pkg/testutils/pgurlutils"
3032
"github.com/cockroachdb/cockroach/pkg/testutils/skip"
3133
"github.com/cockroachdb/cockroach/pkg/util/envutil"
@@ -203,6 +205,23 @@ func testTenantDecisionFromEnvironment(
203205
return baseArg, false
204206
}
205207

208+
var globalDefaultDRPCOptionOverride struct {
209+
isSet bool
210+
value base.DefaultTestDRPCOption
211+
}
212+
213+
// TestingGlobalDRPCOption sets the package-level DefaultTestDRPCOption.
214+
//
215+
// Note: This override will be superseded by any more specific options provided
216+
// when starting the server or cluster.
217+
func TestingGlobalDRPCOption(v base.DefaultTestDRPCOption) func() {
218+
globalDefaultDRPCOptionOverride.isSet = true
219+
globalDefaultDRPCOptionOverride.value = v
220+
return func() {
221+
globalDefaultDRPCOptionOverride.isSet = false
222+
}
223+
}
224+
206225
// globalDefaultSelectionOverride is used when an entire package needs
207226
// to override the probabilistic behavior.
208227
var globalDefaultSelectionOverride struct {
@@ -252,11 +271,15 @@ type TestFataler interface {
252271
// The first argument is optional. If non-nil; it is used for logging
253272
// server configuration messages.
254273
func StartServerOnlyE(t TestLogger, params base.TestServerArgs) (TestServerInterface, error) {
274+
ctx := context.Background()
255275
allowAdditionalTenants := params.DefaultTestTenant.AllowAdditionalTenants()
276+
256277
// Update the flags with the actual decision as to whether we should
257278
// start the service for a default test tenant.
258279
params.DefaultTestTenant = ShouldStartDefaultTestTenant(t, params.DefaultTestTenant)
259280

281+
TryEnableDRPCSetting(ctx, t, &params)
282+
260283
s, err := NewServer(params)
261284
if err != nil {
262285
return nil, err
@@ -269,8 +292,6 @@ func StartServerOnlyE(t TestLogger, params base.TestServerArgs) (TestServerInter
269292
}
270293
}
271294

272-
ctx := context.Background()
273-
274295
if err := s.Start(ctx); err != nil {
275296
return nil, err
276297
}
@@ -519,3 +540,29 @@ func WaitForTenantCapabilities(
519540
t.Fatal(err)
520541
}
521542
}
543+
544+
// TryEnableDRPCSetting determines whether to enable the DRPC cluster setting
545+
// based on the `TestServerArgs.DefaultDRPCOption` and updates the
546+
// `TestServerArgs.Settings` based on that.
547+
func TryEnableDRPCSetting(ctx context.Context, t TestLogger, args *base.TestServerArgs) {
548+
option := args.DefaultDRPCOption
549+
if option == base.TestDRPCDisabled && globalDefaultDRPCOptionOverride.isSet {
550+
option = globalDefaultDRPCOptionOverride.value
551+
}
552+
enableDRPC := false
553+
switch option {
554+
case base.TestDRPCEnabled:
555+
enableDRPC = true
556+
case base.TestDRPCEnabledRandomly:
557+
rng, _ := randutil.NewTestRand()
558+
enableDRPC = rng.Intn(2) == 0
559+
}
560+
if !enableDRPC {
561+
return
562+
}
563+
564+
if args.Settings == nil {
565+
args.Settings = cluster.MakeClusterSettings()
566+
}
567+
rpc.ExperimentalDRPCEnabled.Override(ctx, &args.Settings.SV, true)
568+
}

pkg/testutils/testcluster/testcluster.go

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -346,6 +346,8 @@ func NewTestCluster(
346346
serverArgs.Settings = cluster.TestingCloneClusterSettings(serverArgs.Settings)
347347
}
348348

349+
serverutils.TryEnableDRPCSetting(context.Background(), t, &serverArgs)
350+
349351
// If a reusable listener registry is provided, create reusable listeners
350352
// for every server that doesn't have a custom listener provided. (Only
351353
// servers with a reusable listener can be restarted).

0 commit comments

Comments
 (0)