Merge cockroachdb#120051

120051: roachtest: refactor virtual cluster API r=DarrylWong a=renatolabs

**roachtest: rename `TenantName` to `VirtualClusterName`**
This is a roachtest API; the renaming makes it more consistent with
the naming used by the roachprod API.

**roachprod: remove `KV*` fields from `StartOpts`**
Instead, use `StorageCluster` instead of `KVCluster` (for consistency
across the codebase). `KVAddrs` is also deprecated as a field, as it
can be generated on demand.

**roachtest: make startOpts customizations composable**
Previously, the `option` package would define a number of "default"
start options that could be used when starting cockroach: `NoBackups`,
to disable the roachprod backup schedule; `InMemory`, to start an
in-memory database; `SingleNode`, to skip initialization, etc. This
created a pattern where every different use would lead to the creation
of another method that used the default options except for _some_
setting.

This is less than ideal since it doesn't compose: if we wanted to
create start options to skip init *and* the backup schedule, one would
either need to create a new, bespoke function, or reach directly to
roachprod-level options in the test.

In this commit, we use the well established functional options
pattern, allowing the start options to easily compose. Instead of
writing:

```go
startOpts := option.DefaultStartOptsNoBackups()
```

we now write:

```go
startOpts := option.NewStartOpts(option.NoBackupSchedule)
```

The `NewStartOpts` function takes a list of options that customize the
start options generated.

**roachtest: simplify `StartServiceForVirtualCluster`**
This commit refactors the API exposed to tests when they want to
create virtual clusters. Specifically, we no longer require a "storage
cluster" when creating a shared process virtual cluster, and we also
make use of the functional options pattern introduced earlier in order
to allow the caller to customize where and how the virtual cluster is
to be deployed.

Callers can now create a new shared-process virtual cluster by using
`option.StartSharedVirtualClusterOpts`, which just takes a virtual
cluster name. For separate process tenants, there is
`option.StartVirtualClusterOpts` which takes the virtual cluster name
and the nodes where the SQL server should be deployed.

We also change the API so that callers are only required to pass a
"SQL instance" if the test is deploying more than one instance of the
same virtual cluster on the same node. The concept of "SQL instance"
is confusing and foreign to most engineers, and it only really matters
in the case of several instances of the same VC in the same node.

Co-authored-by: Renato Costa <renato@cockroachlabs.com>

pkg/cmd/roachprod/flags.go

@@ -80,9 +80,6 @@ var (
 
 	// storageCluster is used for cluster virtualization and multi-tenant functionality.
 	storageCluster string
-	// externalProcessNodes indicates the cluster/nodes where external
-	// process SQL instances should be deployed.
-	externalProcessNodes string
 
 	revertUpdate bool
 
@@ -211,7 +208,7 @@ func initFlags() {
 	_ = startInstanceCmd.MarkFlagRequired("storage-cluster")
 	startInstanceCmd.Flags().IntVar(&startOpts.SQLInstance,
 		"sql-instance", 0, "specific SQL/HTTP instance to connect to (this is a roachprod abstraction for separate-process deployments distinct from the internal instance ID)")
-	startInstanceCmd.Flags().StringVar(&externalProcessNodes, "external-cluster", externalProcessNodes, "start service in external mode, as a separate process in the given nodes")
+	startInstanceCmd.Flags().StringVar(&startOpts.VirtualClusterLocation, "external-nodes", startOpts.VirtualClusterLocation, "if set, starts service in external mode, as a separate process in the given nodes")
 
 	// Flags for processes that stop (kill) processes.
 	for _, stopProcessesCmd := range []*cobra.Command{stopCmd, stopInstanceCmd} {

pkg/cmd/roachprod/main.go

@@ -598,13 +598,16 @@ environment variables to the cockroach process.
 		startOpts.AdminUIPort = 0
 
 		startOpts.Target = install.StartSharedProcessForVirtualCluster
-		if externalProcessNodes != "" {
+		// If the user passed an `--external-nodes` option, we are
+		// starting a separate process virtual cluster.
+		if startOpts.VirtualClusterLocation != "" {
 			startOpts.Target = install.StartServiceForVirtualCluster
 		}
 
 		startOpts.VirtualClusterName = args[0]
-		return roachprod.StartServiceForVirtualCluster(context.Background(),
-			config.Logger, externalProcessNodes, storageCluster, startOpts, clusterSettingsOpts...)
+		return roachprod.StartServiceForVirtualCluster(
+			context.Background(), config.Logger, storageCluster, startOpts, clusterSettingsOpts...,
+		)
 	}),
 }
 

pkg/cmd/roachtest/cluster.go

@@ -2079,32 +2079,35 @@ func (c *clusterImpl) StartE(
 	return nil
 }
 
-// StartServiceForVirtualClusterE can start either external or shared process
-// virtual clusters. This can be specified in startOpts.RoachprodOpts. Set the
-// `Target` to the required virtual cluster type. Refer to the virtual cluster
-// section in the struct for more information on what fields are available for
-// virtual clusters.
-//
-// With external process virtual clusters an external process will be started on
-// each node specified in the externalNodes parameter.
-//
-// With shared process virtual clusters the required queries will be run on a
-// storage node of the cluster specified in the opts parameter.
+// StartServiceForVirtualClusterE can start either external or shared
+// process virtual clusters. This can be specified by the `startOpts`
+// passed. See the `option.Start*VirtualClusterOpts` functions.
 func (c *clusterImpl) StartServiceForVirtualClusterE(
 	ctx context.Context,
 	l *logger.Logger,
-	externalNodes option.NodeListOption,
 	startOpts option.StartOpts,
 	settings install.ClusterSettings,
-	opts ...option.Option,
 ) error {
-
-	c.setStatusForClusterOpt("starting virtual cluster", startOpts.RoachtestOpts.Worker, opts...)
-	defer c.clearStatusForClusterOpt(startOpts.RoachtestOpts.Worker)
-
+	l.Printf("starting virtual cluster")
 	clusterSettingsOpts := c.configureClusterSettingOptions(c.virtualClusterSettings, settings)
 
-	if err := roachprod.StartServiceForVirtualCluster(ctx, l, c.MakeNodes(externalNodes), c.MakeNodes(opts...), startOpts.RoachprodOpts, clusterSettingsOpts...); err != nil {
+	// By default, we assume every node in the cluster is part of the
+	// storage cluster the virtual cluster needs to connect to. If the
+	// user customized the storage cluster in the `StartOpts`, we use
+	// that.
+	storageCluster := c.All()
+	if len(startOpts.SeparateProcessStorageNodes) > 0 {
+		storageCluster = startOpts.SeparateProcessStorageNodes
+	}
+
+	// If the user indicated nodes where the virtual cluster should be
+	// started, we indicate that in the roachprod opts.
+	if len(startOpts.SeparateProcessNodes) > 0 {
+		startOpts.RoachprodOpts.VirtualClusterLocation = c.MakeNodes(startOpts.SeparateProcessNodes)
+	}
+	if err := roachprod.StartServiceForVirtualCluster(
+		ctx, l, c.MakeNodes(storageCluster), startOpts.RoachprodOpts, clusterSettingsOpts...,
+	); err != nil {
 		return err
 	}
 
@@ -2119,12 +2122,10 @@ func (c *clusterImpl) StartServiceForVirtualClusterE(
 func (c *clusterImpl) StartServiceForVirtualCluster(
 	ctx context.Context,
 	l *logger.Logger,
-	externalNodes option.NodeListOption,
 	startOpts option.StartOpts,
 	settings install.ClusterSettings,
-	opts ...option.Option,
 ) {
-	if err := c.StartServiceForVirtualClusterE(ctx, l, externalNodes, startOpts, settings, opts...); err != nil {
+	if err := c.StartServiceForVirtualClusterE(ctx, l, startOpts, settings); err != nil {
 		c.t.Fatal(err)
 	}
 }
@@ -2134,18 +2135,22 @@ func (c *clusterImpl) StartServiceForVirtualCluster(
 // process virtual clusters, the corresponding service is stopped. For
 // separate process, the OS process is killed.
 func (c *clusterImpl) StopServiceForVirtualClusterE(
-	ctx context.Context, l *logger.Logger, stopOpts option.StopOpts, opts ...option.Option,
+	ctx context.Context, l *logger.Logger, stopOpts option.StopOpts,
 ) error {
-	c.setStatusForClusterOpt("stopping virtual cluster", stopOpts.RoachtestOpts.Worker, opts...)
-	defer c.clearStatusForClusterOpt(stopOpts.RoachtestOpts.Worker)
+	l.Printf("stoping virtual cluster")
+
+	nodes := c.All()
+	if len(stopOpts.SeparateProcessNodes) > 0 {
+		nodes = stopOpts.SeparateProcessNodes
+	}
 
 	return roachprod.StopServiceForVirtualCluster(
-		ctx, l, c.Name(), c.IsSecure(), stopOpts.RoachprodOpts,
+		ctx, l, c.MakeNodes(nodes), c.IsSecure(), stopOpts.RoachprodOpts,
 	)
 }
 
 func (c *clusterImpl) StopServiceForVirtualCluster(
-	ctx context.Context, l *logger.Logger, stopOpts option.StopOpts, opts ...option.Option,
+	ctx context.Context, l *logger.Logger, stopOpts option.StopOpts,
 ) {
 	if err := c.StopServiceForVirtualClusterE(ctx, l, stopOpts); err != nil {
 		c.t.Fatal(err)
@@ -2739,7 +2744,7 @@ func (c *clusterImpl) ConnE(
 		opt(connOptions)
 	}
 	urls, err := c.ExternalPGUrl(ctx, l, c.Node(node), roachprod.PGURLOptions{
-		VirtualClusterName: connOptions.TenantName,
+		VirtualClusterName: connOptions.VirtualClusterName,
 		SQLInstance:        connOptions.SQLInstance,
 		Auth:               connOptions.AuthMode,
 	})

pkg/cmd/roachtest/cluster/cluster_interface.go

@@ -68,13 +68,13 @@ type Cluster interface {
 
 	// Starting virtual clusters.
 
-	StartServiceForVirtualClusterE(ctx context.Context, l *logger.Logger, externalNodes option.NodeListOption, startOpts option.StartOpts, settings install.ClusterSettings, opts ...option.Option) error
-	StartServiceForVirtualCluster(ctx context.Context, l *logger.Logger, externalNodes option.NodeListOption, startOpts option.StartOpts, settings install.ClusterSettings, opts ...option.Option)
+	StartServiceForVirtualClusterE(ctx context.Context, l *logger.Logger, startOpts option.StartOpts, settings install.ClusterSettings) error
+	StartServiceForVirtualCluster(ctx context.Context, l *logger.Logger, startOpts option.StartOpts, settings install.ClusterSettings)
 
 	// Stopping virtual clusters.
 
-	StopServiceForVirtualClusterE(ctx context.Context, l *logger.Logger, stopOpts option.StopOpts, opts ...option.Option) error
-	StopServiceForVirtualCluster(ctx context.Context, l *logger.Logger, stopOpts option.StopOpts, opts ...option.Option)
+	StopServiceForVirtualClusterE(ctx context.Context, l *logger.Logger, stopOpts option.StopOpts) error
+	StopServiceForVirtualCluster(ctx context.Context, l *logger.Logger, stopOpts option.StopOpts)
 
 	// Hostnames and IP addresses of the nodes.
 

pkg/cmd/roachtest/option/connection_options.go

@@ -18,12 +18,12 @@ import (
 )
 
 type ConnOption struct {
-	User        string
-	DBName      string
-	TenantName  string
-	SQLInstance int
-	AuthMode    install.PGAuthMode
-	Options     map[string]string
+	User               string
+	DBName             string
+	VirtualClusterName string
+	SQLInstance        int
+	AuthMode           install.PGAuthMode
+	Options            map[string]string
 }
 
 func User(user string) func(*ConnOption) {
@@ -32,9 +32,9 @@ func User(user string) func(*ConnOption) {
 	}
 }
 
-func TenantName(tenantName string) func(*ConnOption) {
+func VirtualClusterName(name string) func(*ConnOption) {
 	return func(option *ConnOption) {
-		option.TenantName = tenantName
+		option.VirtualClusterName = name
 	}
 }