Merge cockroachdb#105006

105006: roachtest: multi-tenant distributed sql fix r=srosenberg,renatolabs a=herkolategan This change fixes the `multitenant_distsql` test by passing `COCKROACH_MIN_RANGE_MAX_BYTES` as part of the env vars when starting tenants as well. The code around port handling on local clusters have also been updated to avoid collisions. There is a mechanism in place to offset the ports, but it still clashed with local ports from other nodes created before the tenants, so a simple naive shift has been put in place. Fixes: cockroachdb#100260 Epic: None Co-authored-by: Herko Lategan <[email protected]>
wenyihu6 · Jun 19, 2023 · 3573201 · 3573201
2 parents 126b677 + ab9f165
commit 3573201
Show file tree

Hide file tree

Showing 2 changed files with 20 additions and 6 deletions.
diff --git a/pkg/cmd/roachtest/tests/multitenant_distsql.go b/pkg/cmd/roachtest/tests/multitenant_distsql.go
@@ -37,7 +37,6 @@ func registerMultiTenantDistSQL(r registry.Registry) {
 			b := bundle
 			to := timeout
 			r.Add(registry.TestSpec{
-				Skip:    "the test is skipped until #100260 is resolved",
 				Name:    fmt.Sprintf("multitenant/distsql/instances=%d/bundle=%s/timeout=%d", numInstances, b, to),
 				Owner:   registry.OwnerSQLQueries,
 				Cluster: r.MakeClusterSpec(4),
@@ -64,6 +63,7 @@ func runMultiTenantDistSQL(
 	// 1 byte to bypass the guardrails.
 	settings := install.MakeClusterSettings(install.SecureOption(true))
 	settings.Env = append(settings.Env, "COCKROACH_MIN_RANGE_MAX_BYTES=1")
+	tenantEnvOpt := createTenantEnvVar(settings.Env[len(settings.Env)-1])
 	c.Start(ctx, t.L(), option.DefaultStartOpts(), settings, c.Node(1))
 	c.Start(ctx, t.L(), option.DefaultStartOpts(), settings, c.Node(2))
 	c.Start(ctx, t.L(), option.DefaultStartOpts(), settings, c.Node(3))
@@ -72,17 +72,20 @@ func runMultiTenantDistSQL(
 		tenantID           = 11
 		tenantBaseHTTPPort = 8081
 		tenantBaseSQLPort  = 26259
+		// localPortOffset is used to avoid port conflicts with nodes on a local
+		// cluster.
+		localPortOffset = 1000
 	)
 
 	tenantHTTPPort := func(offset int) int {
 		if c.IsLocal() || numInstances > c.Spec().NodeCount {
-			return tenantBaseHTTPPort + offset
+			return tenantBaseHTTPPort + localPortOffset + offset
 		}
 		return tenantBaseHTTPPort
 	}
 	tenantSQLPort := func(offset int) int {
 		if c.IsLocal() || numInstances > c.Spec().NodeCount {
-			return tenantBaseSQLPort + offset
+			return tenantBaseSQLPort + localPortOffset + offset
 		}
 		return tenantBaseSQLPort
 	}
@@ -93,7 +96,7 @@ func runMultiTenantDistSQL(
 
 	instances := make([]*tenantNode, 0, numInstances)
 	instance1 := createTenantNode(ctx, t, c, c.Node(1), tenantID, 2 /* node */, tenantHTTPPort(0), tenantSQLPort(0),
-		createTenantCertNodes(c.All()))
+		createTenantCertNodes(c.All()), tenantEnvOpt)
 	instances = append(instances, instance1)
 	defer instance1.stop(ctx, t, c)
 	instance1.start(ctx, t, c, "./cockroach")

diff --git a/pkg/cmd/roachtest/tests/multitenant_utils.go b/pkg/cmd/roachtest/tests/multitenant_utils.go
@@ -44,6 +44,7 @@ type tenantNode struct {
 	httpPort, sqlPort int
 	kvAddrs           []string
 	pgURL             string
+	envVars           []string
 	// Same as pgURL but with relative ssl parameters.
 	relativeSecureURL string
 
@@ -55,13 +56,20 @@ type tenantNode struct {
 type createTenantOptions struct {
 	// Set this to expand the scope of the nodes added to the tenant certs.
 	certNodes option.NodeListOption
+
+	// Set this to add additional environment variables to the tenant.
+	envVars []string
 }
 type createTenantOpt func(*createTenantOptions)
 
 func createTenantCertNodes(nodes option.NodeListOption) createTenantOpt {
 	return func(c *createTenantOptions) { c.certNodes = nodes }
 }
 
+func createTenantEnvVar(envVar string) createTenantOpt {
+	return func(c *createTenantOptions) { c.envVars = append(c.envVars, envVar) }
+}
+
 func createTenantNodeInternal(
 	ctx context.Context,
 	t test.Test,
@@ -88,6 +96,7 @@ func createTenantNodeInternal(
 		kvAddrs:    kvAddrs,
 		node:       node,
 		sqlPort:    sqlPort,
+		envVars:    append(config.DefaultEnvVars(), createOptions.envVars...),
 	}
 	if certs {
 		tn.createTenantCert(ctx, t, c, createOptions.certNodes)
@@ -181,7 +190,7 @@ func (tn *tenantNode) start(ctx context.Context, t test.Test, c cluster.Cluster,
 	require.NoError(t, err)
 	tn.errCh = startTenantServer(
 		ctx, c, c.Node(tn.node), internalIPs[0], binary, tn.kvAddrs, tn.tenantID,
-		tn.httpPort, tn.sqlPort,
+		tn.httpPort, tn.sqlPort, tn.envVars,
 		extraArgs...,
 	)
 
@@ -244,6 +253,7 @@ func startTenantServer(
 	tenantID int,
 	httpPort int,
 	sqlPort int,
+	envVars []string,
 	extraFlags ...string,
 ) chan error {
 	args := []string{
@@ -259,7 +269,7 @@ func startTenantServer(
 	errCh := make(chan error, 1)
 	go func() {
 		errCh <- c.RunE(tenantCtx, node,
-			append(append(append([]string{}, config.DefaultEnvVars()...), binary, "mt", "start-sql"), args...)...,
+			append(append(append([]string{}, envVars...), binary, "mt", "start-sql"), args...)...,
 		)
 		close(errCh)
 	}()
@@ -278,6 +288,7 @@ func newTenantInstance(
 		node:       node,
 		httpPort:   http,
 		sqlPort:    sql,
+		envVars:    tn.envVars,
 	}
 	tenantCertsDir, err := os.MkdirTemp("", "tenant-certs")
 	if err != nil {