roachtest: update multitenant/distsql to use new roachprod service APIs #115599

herkolategan · 2023-12-05T13:05:03Z

Previously the multitenant distsql roachtest relied on an internal util in roachtest to start virtual clusters. This PR updates the test to use the new official roachtest and roachprod APIs for starting virtual clusters.

Some additional changes were required to support upgrading the test. The cluster interface only exposed a method to start storage nodes, but that is insufficient to start virtual clusters that have a separate method on the roachprod API (for starting).

This change adds a new method StartServiceForVirtualCluster to the cluster interface to enable roachtests to start virtual clusters. Some refactoring was required to enable different sets of cluster settings, depending on what service
type is going to be started.

There are now two sets of cluster settings that can be utilised in test_runner. For virtual clusters virtualClusterSettings will be used, and for storage clusters clusterSettings will be utilised.

Fixes: #116019

Release Note: None
Epic: CRDB-31933

cockroach-teamcity · 2023-12-05T13:05:15Z

This change is

renatolabs

Very excited about this change! 🎉

Reviewed 1 of 1 files at r1, 4 of 4 files at r2, 1 of 1 files at r3, 2 of 2 files at r4, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @herkolategan and @srosenberg)

pkg/cmd/roachtest/cluster.go line 1929 at r2 (raw file):

func (c *clusterImpl) configureClusterSettingOptions(
	defaultEnv map[string]interface{},

What's the rationale for this defaultEnv parameter? From what I can see, we only use it for COCKROACH_CRASH_ON_SPAN_USE_AFTER_FINISH. Is there a reason why we wouldn't want that variable in SQL processes? I'm wondering if we should enforce all cockroach processes to have the same default variables (just like we're doing for the random seed var) and eliminate this argument.

pkg/cmd/roachtest/cluster.go line 2031 at r2 (raw file):

	if settings.Secure {
		if err := c.RefetchCertsFromNode(ctx, 1); err != nil {

Should this really be 1 and not first node in c.MakeNodes(opts...)?

pkg/cmd/roachtest/cluster.go line 2011 at r4 (raw file):

}

func (c *clusterImpl) StartServiceForVirtualClusterE(

This function could use some documentation, especially about externalNodes and in-memory vs external process tenants.

pkg/cmd/roachtest/tests/multitenant_distsql.go line 71 at r3 (raw file):

	c.Start(ctx, t.L(), option.DefaultStartOpts(), settings, c.Node(2))
	c.Start(ctx, t.L(), option.DefaultStartOpts(), settings, c.Node(3))
	storNodes := c.Range(1, 3)

Did you mean storageNodes?

pkg/cmd/roachtest/tests/multitenant_distsql.go line 81 at r3 (raw file):

		instStartOps.RoachprodOpts.Target = install.StartServiceForVirtualCluster
		instStartOps.RoachprodOpts.VirtualClusterName = tenantName
		instStartOps.RoachprodOpts.SQLInstance = sqlInstance

Interesting, we should probably find a way to package these into something higher level too.

pkg/cmd/roachtest/tests/multitenant_distsql.go line 85 at r3 (raw file):

		t.L().Printf("Starting instance %d on node %d", i, node)
		instStartOps.RoachprodOpts.SQLPort = 0
		instStartOps.RoachprodOpts.AdminUIPort = 0

Should we add a TODO to remove this once #111052 is resolved?

pkg/cmd/roachtest/tests/multitenant_distsql.go line 86 at r3 (raw file):

		instStartOps.RoachprodOpts.SQLPort = 0
		instStartOps.RoachprodOpts.AdminUIPort = 0
		err := c.StartServiceForVirtualClusterE(ctx, t.L(), c.Node(node), instStartOps, settings, storNodes)

Why not StartServiceForVirtualCluster?

pkg/cmd/roachtest/tests/multitenant_distsql.go line 101 at r3 (raw file):

	m := c.NewMonitor(ctx, c.Nodes(1, 2, 3))

	//inst1Conn, err := gosql.Open("postgres", c.PGUrl(ctx, 1)

Delete?

pkg/cmd/roachtest/tests/multitenant_distsql.go line 146 at r3 (raw file):

		m.Wait()
		wg.Done()
	}()

I don't understand this block, why do we need it? I also don't see a wg.Wait() call.

srosenberg

Reviewed all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @herkolategan)

pkg/cmd/roachtest/tests/multitenant_distsql.go line 81 at r4 (raw file):

		instStartOps.RoachprodOpts.Target = install.StartServiceForVirtualCluster
		instStartOps.RoachprodOpts.VirtualClusterName = tenantName
		instStartOps.RoachprodOpts.SQLInstance = sqlInstance

Can someone remind me what SQLInstance denotes? I can see that it's expected to be 0 in case of StartSharedProcessForVirtualCluster, but is exact meaning is somewhat obscured by the implementation of service registry/discovery.

srosenberg

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @herkolategan)

pkg/cmd/roachtest/tests/multitenant_distsql.go line 65 at r4 (raw file):

	// performance and resource limitations. We set the minimum range max bytes to
	// 1 byte to bypass the guardrails.
	install.MakeClusterSettings(install.SecureOption(true))

Remove?

pkg/cmd/roachtest/tests/multitenant_distsql.go line 81 at r4 (raw file):
Ok, I found the definition in the PR commit message [1] :).

A SQL Instance can be viewed as a unique identifier for the SQL Instance that is running on a VM in the event that there are more than one SQL Instance serving the same virtual cluster on that VM.

[1] #111582

herkolategan

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @renatolabs and @srosenberg)

pkg/cmd/roachtest/cluster.go line 1929 at r2 (raw file):