roachtest: remove cloud from registry and ClusterSpec

This change is the last step in removing runtime state from the test
registry and the cluster spec. The cloud is no longer accessible at
test registration time.

Fixes cockroachdb#104029
Release note: None

pkg/cmd/roachtest/cluster.go

@@ -622,10 +622,11 @@ type nodeSelector interface {
 
 // It is safe for concurrent use by multiple goroutines.
 type clusterImpl struct {
-	name string
-	tag  string
-	spec spec.ClusterSpec
-	t    test.Test
+	name  string
+	tag   string
+	cloud string
+	spec  spec.ClusterSpec
+	t     test.Test
 	// r is the registry tracking this cluster. Destroying the cluster will
 	// unregister it.
 	r *clusterRegistry
@@ -873,7 +874,9 @@ func (f *clusterFactory) newCluster(
 
 	providerOptsContainer := vm.CreateProviderOptionsContainer()
 
+	cloud := roachtestflags.Cloud
 	params := spec.RoachprodClusterConfig{
+		Cloud:                  cloud,
 		UseIOBarrierOnLocalSSD: cfg.useIOBarrier,
 		PreferredArch:          cfg.arch,
 	}
@@ -890,8 +893,8 @@ func (f *clusterFactory) newCluster(
 
 		return nil, nil, err
 	}
-	if cfg.spec.Cloud != spec.Local {
-		providerOptsContainer.SetProviderOpts(cfg.spec.Cloud, providerOpts)
+	if cloud != spec.Local {
+		providerOptsContainer.SetProviderOpts(cloud, providerOpts)
 	}
 
 	if cfg.spec.UbuntuVersion.IsOverridden() {
@@ -930,6 +933,7 @@ func (f *clusterFactory) newCluster(
 		}
 
 		c := &clusterImpl{
+			cloud:      cloud,
 			name:       genName,
 			spec:       cfg.spec,
 			expiration: cfg.spec.Expiration(),
@@ -2616,7 +2620,7 @@ func (c *clusterImpl) MakeNodes(opts ...option.Option) string {
 }
 
 func (c *clusterImpl) Cloud() string {
-	return c.spec.Cloud
+	return c.cloud
 }
 
 func (c *clusterImpl) IsLocal() bool {
@@ -2698,7 +2702,7 @@ func archForTest(ctx context.Context, l *logger.Logger, testSpec registry.TestSp
 		return testSpec.Cluster.Arch
 	}
 
-	if testSpec.Benchmark && testSpec.Cluster.Cloud != spec.Local {
+	if testSpec.Benchmark && roachtestflags.Cloud != spec.Local {
 		// TODO(srosenberg): enable after https://github.com/cockroachdb/cockroach/issues/104213
 		arch := vm.ArchAMD64
 		l.PrintfCtx(ctx, "Disabling arch randomization for benchmark; arch=%q, %s", arch, testSpec.Name)

pkg/cmd/roachtest/cluster_test.go

@@ -27,7 +27,7 @@ import (
 )
 
 func TestClusterNodes(t *testing.T) {
-	c := &clusterImpl{spec: spec.MakeClusterSpec(spec.GCE, 10)}
+	c := &clusterImpl{spec: spec.MakeClusterSpec(10)}
 	opts := func(opts ...option.Option) []option.Option {
 		return opts
 	}

pkg/cmd/roachtest/github.go

@@ -18,6 +18,7 @@ import (
 
 	"github.com/cockroachdb/cockroach/pkg/cmd/internal/issues"
 	"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/registry"
+	"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/roachtestflags"
 	"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/spec"
 	"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/test"
 	"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/tests"
@@ -207,7 +208,7 @@ func (g *githubIssues) createPostRequest(
 	artifacts := fmt.Sprintf("/%s", testName)
 
 	clusterParams := map[string]string{
-		roachtestPrefix("cloud"):            spec.Cluster.Cloud,
+		roachtestPrefix("cloud"):            roachtestflags.Cloud,
 		roachtestPrefix("cpu"):              fmt.Sprintf("%d", spec.Cluster.CPUs),
 		roachtestPrefix("ssd"):              fmt.Sprintf("%d", spec.Cluster.SSDs),
 		roachtestPrefix("metamorphicBuild"): fmt.Sprintf("%t", metamorphicBuild),
@@ -248,7 +249,7 @@ func (g *githubIssues) createPostRequest(
 		Artifacts:            artifacts,
 		ExtraLabels:          labels,
 		ExtraParams:          clusterParams,
-		HelpCommand:          generateHelpCommand(testName, issueClusterName, spec.Cluster.Cloud, start, end),
+		HelpCommand:          generateHelpCommand(testName, issueClusterName, roachtestflags.Cloud, start, end),
 	}, nil
 }
 

pkg/cmd/roachtest/github_test.go

@@ -83,7 +83,7 @@ func TestShouldPost(t *testing.T) {
 		{false, 1, "token", "master", true, ""},
 	}
 
-	reg := makeTestRegistry(spec.GCE)
+	reg := makeTestRegistry()
 	for _, c := range testCases {
 		t.Setenv("GITHUB_API_TOKEN", c.envGithubAPIToken)
 		t.Setenv("TC_BUILD_BRANCH", c.envTcBuildBranch)
@@ -198,7 +198,7 @@ func TestCreatePostRequest(t *testing.T) {
 		},
 	}
 
-	reg := makeTestRegistry(spec.GCE)
+	reg := makeTestRegistry()
 	for idx, c := range testCases {
 		t.Run(fmt.Sprintf("%d", idx+1), func(t *testing.T) {
 			clusterSpec := reg.MakeClusterSpec(1, spec.Arch(c.arch))

pkg/cmd/roachtest/main.go

@@ -96,7 +96,7 @@ Examples:
    roachtest list --suite weekly --owner kv
 `,
 		RunE: func(cmd *cobra.Command, args []string) error {
-			r := makeTestRegistry(roachtestflags.Cloud)
+			r := makeTestRegistry()
 			tests.RegisterTests(&r)
 
 			filter, err := makeTestFilter(args)

pkg/cmd/roachtest/main_test.go

@@ -30,15 +30,15 @@ func init() {
 }
 
 func makeRegistry(names ...string) testRegistryImpl {
-	r := makeTestRegistry(spec.GCE)
+	r := makeTestRegistry()
 	dummyRun := func(context.Context, test.Test, cluster.Cluster) {}
 
 	for _, name := range names {
 		r.Add(registry.TestSpec{
 			Name:             name,
 			Owner:            OwnerUnitTest,
 			Run:              dummyRun,
-			Cluster:          spec.MakeClusterSpec(spec.GCE, 0),
+			Cluster:          spec.MakeClusterSpec(0),
 			CompatibleClouds: registry.AllExceptAWS,
 			Suites:           registry.Suites(registry.Nightly),
 		})

pkg/cmd/roachtest/registry/registry_interface.go

@@ -21,5 +21,4 @@ type Registry interface {
 	MakeClusterSpec(nodeCount int, opts ...spec.Option) spec.ClusterSpec
 	Add(TestSpec)
 	PromFactory() promauto.Factory
-	Cloud() string
 }

pkg/cmd/roachtest/run.go

@@ -43,7 +43,7 @@ import (
 func runTests(register func(registry.Registry), filter *registry.TestFilter) error {
 	//lint:ignore SA1019 deprecated
 	rand.Seed(roachtestflags.GlobalSeed)
-	r := makeTestRegistry(roachtestflags.Cloud)
+	r := makeTestRegistry()
 
 	// actual registering of tests
 	// TODO: don't register if we can't run on the specified registry cloud

pkg/cmd/roachtest/spec/cluster_spec.go

@@ -80,10 +80,6 @@ const (
 // ClusterSpec represents a test's description of what its cluster needs to
 // look like. It becomes part of a clusterConfig when the cluster is created.
 type ClusterSpec struct {
-	// TODO(#104029): We should remove the Cloud field; the tests now specify
-	// their compatible clouds.
-	Cloud string
-
 	Arch      vm.CPUArch // CPU architecture; auto-chosen if left empty
 	NodeCount int
 	// CPUs is the number of CPUs per node.
@@ -125,8 +121,8 @@ type ClusterSpec struct {
 }
 
 // MakeClusterSpec makes a ClusterSpec.
-func MakeClusterSpec(cloud string, nodeCount int, opts ...Option) ClusterSpec {
-	spec := ClusterSpec{Cloud: cloud, NodeCount: nodeCount}
+func MakeClusterSpec(nodeCount int, opts ...Option) ClusterSpec {
+	spec := ClusterSpec{NodeCount: nodeCount}
 	defaultOpts := []Option{CPU(4), nodeLifetime(12 * time.Hour), ReuseAny()}
 	for _, o := range append(defaultOpts, opts...) {
 		o(&spec)
@@ -244,6 +240,8 @@ func getAzureOpts(machineType string, zones []string) vm.ProviderOpts {
 // does not depend on the test. It is used in conjunction with ClusterSpec to
 // determine the final configuration.
 type RoachprodClusterConfig struct {
+	Cloud string
+
 	// UseIOBarrierOnLocalSSD is set if we don't want to mount local SSDs with the
 	// `-o nobarrier` flag.
 	UseIOBarrierOnLocalSSD bool
@@ -296,25 +294,26 @@ func (s *ClusterSpec) RoachprodOpts(
 	if s.Lifetime != 0 {
 		createVMOpts.Lifetime = s.Lifetime
 	}
-	switch s.Cloud {
+	cloud := params.Cloud
+	switch cloud {
 	case Local:
-		createVMOpts.VMProviders = []string{s.Cloud}
+		createVMOpts.VMProviders = []string{cloud}
 		// remaining opts are not applicable to local clusters
 		return createVMOpts, nil, nil
 	case AWS, GCE, Azure:
-		createVMOpts.VMProviders = []string{s.Cloud}
+		createVMOpts.VMProviders = []string{cloud}
 	default:
-		return vm.CreateOpts{}, nil, errors.Errorf("unsupported cloud %v", s.Cloud)
+		return vm.CreateOpts{}, nil, errors.Errorf("unsupported cloud %v", cloud)
 	}
 
-	if s.Cloud != GCE && s.Cloud != AWS {
+	if cloud != GCE && cloud != AWS {
 		if s.VolumeSize != 0 {
-			return vm.CreateOpts{}, nil, errors.Errorf("specifying volume size is not yet supported on %s", s.Cloud)
+			return vm.CreateOpts{}, nil, errors.Errorf("specifying volume size is not yet supported on %s", cloud)
 		}
 	}
-	if s.Cloud != GCE {
+	if cloud != GCE {
 		if s.SSDs != 0 {
-			return vm.CreateOpts{}, nil, errors.Errorf("specifying SSD count is not yet supported on %s", s.Cloud)
+			return vm.CreateOpts{}, nil, errors.Errorf("specifying SSD count is not yet supported on %s", cloud)
 		}
 	}
 
@@ -323,7 +322,7 @@ func (s *ClusterSpec) RoachprodOpts(
 	ssdCount := s.SSDs
 
 	machineType := params.Defaults.MachineType
-	switch s.Cloud {
+	switch cloud {
 	case AWS:
 		if s.AWS.MachineType != "" {
 			machineType = s.AWS.MachineType
@@ -343,7 +342,7 @@ func (s *ClusterSpec) RoachprodOpts(
 			// If no machine type was specified, choose one
 			// based on the cloud and CPU count.
 			var err error
-			switch s.Cloud {
+			switch cloud {
 			case AWS:
 				machineType, selectedArch, err = SelectAWSMachineType(s.CPUs, s.Mem, preferLocalSSD && s.VolumeSize == 0, arch)
 			case GCE:
@@ -368,8 +367,8 @@ func (s *ClusterSpec) RoachprodOpts(
 		// - if no particular volume size is requested, and,
 		// - on AWS, if the machine type supports it.
 		// - on GCE, if the machine type is not ARM64.
-		if preferLocalSSD && s.VolumeSize == 0 && (s.Cloud != AWS || awsMachineSupportsSSD(machineType)) &&
-			(s.Cloud != GCE || selectedArch != vm.ArchARM64) {
+		if preferLocalSSD && s.VolumeSize == 0 && (cloud != AWS || awsMachineSupportsSSD(machineType)) &&
+			(cloud != GCE || selectedArch != vm.ArchARM64) {
 			// Ensure SSD count is at least 1 if UseLocalSSD is true.
 			if ssdCount == 0 {
 				ssdCount = 1
@@ -382,21 +381,21 @@ func (s *ClusterSpec) RoachprodOpts(
 	}
 
 	if s.FileSystem == Zfs {
-		if s.Cloud != GCE {
+		if cloud != GCE {
 			return vm.CreateOpts{}, nil, errors.Errorf(
-				"node creation with zfs file system not yet supported on %s", s.Cloud,
+				"node creation with zfs file system not yet supported on %s", cloud,
 			)
 		}
 		createVMOpts.SSDOpts.FileSystem = vm.Zfs
-	} else if s.RandomlyUseZfs && s.Cloud == GCE {
+	} else if s.RandomlyUseZfs && cloud == GCE {
 		rng, _ := randutil.NewPseudoRand()
 		if rng.Float64() <= 0.2 {
 			createVMOpts.SSDOpts.FileSystem = vm.Zfs
 		}
 	}
 
 	zonesStr := params.Defaults.Zones
-	switch s.Cloud {
+	switch cloud {
 	case AWS:
 		if s.AWS.Zones != "" {
 			zonesStr = s.AWS.Zones
@@ -414,13 +413,13 @@ func (s *ClusterSpec) RoachprodOpts(
 		}
 	}
 
-	if createVMOpts.Arch == string(vm.ArchFIPS) && !(s.Cloud == GCE || s.Cloud == AWS) {
+	if createVMOpts.Arch == string(vm.ArchFIPS) && !(cloud == GCE || cloud == AWS) {
 		return vm.CreateOpts{}, nil, errors.Errorf(
-			"FIPS not yet supported on %s", s.Cloud,
+			"FIPS not yet supported on %s", cloud,
 		)
 	}
 	var providerOpts vm.ProviderOpts
-	switch s.Cloud {
+	switch cloud {
 	case AWS:
 		providerOpts = getAWSOpts(machineType, zones, s.VolumeSize, s.AWS.VolumeThroughput,
 			createVMOpts.SSDOpts.UseLocalSSD)

pkg/cmd/roachtest/spec/option.go

@@ -19,13 +19,6 @@ import (
 // Option for MakeClusterSpec.
 type Option func(spec *ClusterSpec)
 
-// Cloud controls what cloud is used to create the cluster.
-func Cloud(s string) Option {
-	return func(spec *ClusterSpec) {
-		spec.Cloud = s
-	}
-}
-
 // Arch requests a specific CPU architecture.
 //
 // Note that it is not guaranteed that this architecture will be used (e.g. if

pkg/cmd/roachtest/test_registry.go

@@ -26,7 +26,6 @@ import (
 
 type testRegistryImpl struct {
 	m                map[string]*registry.TestSpec
-	cloud            string
 	snapshotPrefixes map[string]struct{}
 
 	promRegistry *prometheus.Registry
@@ -35,9 +34,8 @@ type testRegistryImpl struct {
 var _ registry.Registry = (*testRegistryImpl)(nil)
 
 // makeTestRegistry constructs a testRegistryImpl and configures it with opts.
-func makeTestRegistry(cloud string) testRegistryImpl {
+func makeTestRegistry() testRegistryImpl {
 	return testRegistryImpl{
-		cloud:            cloud,
 		m:                make(map[string]*registry.TestSpec),
 		snapshotPrefixes: make(map[string]struct{}),
 		promRegistry:     prometheus.NewRegistry(),
@@ -71,7 +69,7 @@ func (r *testRegistryImpl) Add(spec registry.TestSpec) {
 // MakeClusterSpec makes a cluster spec. It should be used over `spec.MakeClusterSpec`
 // because this method also adds options baked into the registry.
 func (r *testRegistryImpl) MakeClusterSpec(nodeCount int, opts ...spec.Option) spec.ClusterSpec {
-	return spec.MakeClusterSpec(r.cloud, nodeCount, opts...)
+	return spec.MakeClusterSpec(nodeCount, opts...)
 }
 
 const testNameRE = "^[a-zA-Z0-9-_=/,]+$"
@@ -141,7 +139,3 @@ func (r testRegistryImpl) AllTests() []registry.TestSpec {
 	})
 	return tests
 }
-
-func (r testRegistryImpl) Cloud() string {
-	return r.cloud
-}

pkg/cmd/roachtest/test_registry_test.go

@@ -21,8 +21,7 @@ import (
 )
 
 func TestMakeTestRegistry(t *testing.T) {
-	r := makeTestRegistry(spec.AWS)
-	require.Equal(t, spec.AWS, r.cloud)
+	r := makeTestRegistry()
 
 	s := r.MakeClusterSpec(100, spec.Geo(), spec.CPU(12),
 		spec.PreferLocalSSD())
@@ -52,7 +51,7 @@ func TestMakeTestRegistry(t *testing.T) {
 // TestPrometheusMetricParser tests that the registry.PromSub()
 // helper properly converts a string into a metric name that Prometheus can read.
 func TestPrometheusMetricParser(t *testing.T) {
-	r := makeTestRegistry(spec.AWS)
+	r := makeTestRegistry()
 	f := r.PromFactory()
 
 	rawName := "restore/nodes=4/duration"

pkg/cmd/roachtest/test_runner.go

@@ -928,7 +928,7 @@ func (r *testRunner) runTest(
 
 	s := t.Spec().(*registry.TestSpec)
 
-	grafanaAvailable := s.Cluster.Cloud == spec.GCE
+	grafanaAvailable := roachtestflags.Cloud == spec.GCE
 	if err := c.addLabels(map[string]string{VmLabelTestName: testRunID}); err != nil {
 		shout(ctx, l, stdout, "failed to add label to cluster [%s] - %s", c.Name(), err)
 		grafanaAvailable = false