Merge pull request #117561 from RaduBerinde/backport23.1-114509

release-23.1: roachtest: remove cloud from registry and ClusterSpec

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
@@ -864,7 +865,9 @@ func (f *clusterFactory) newCluster(
 
 	providerOptsContainer := vm.CreateProviderOptionsContainer()
 
+	cloud := roachtestflags.Cloud
 	params := spec.RoachprodClusterConfig{
+		Cloud:                  cloud,
 		UseIOBarrierOnLocalSSD: cfg.useIOBarrier,
 		PreferredArch:          cfg.arch,
 	}
@@ -878,8 +881,8 @@ func (f *clusterFactory) newCluster(
 	if err != nil {
 		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() {
@@ -902,6 +905,7 @@ func (f *clusterFactory) newCluster(
 	// loop assumes maxAttempts is atleast (1).
 	for i := 1; ; i++ {
 		c := &clusterImpl{
+			cloud: cloud,
 			// NB: this intentionally avoids re-using the name across iterations in
 			// the loop. See:
 			//
@@ -2525,7 +2529,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 {
@@ -2642,7 +2646,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"
@@ -204,7 +205,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),
@@ -240,7 +241,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)
@@ -184,7 +184,7 @@ func TestCreatePostRequest(t *testing.T) {
 		{true, false, false, false, false, "", createFailure(gce.ErrDNSOperation), true, false, true, nil},
 	}
 
-	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

@@ -98,7 +98,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 {
 
 	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)
@@ -240,6 +236,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
@@ -292,25 +290,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)
 		}
 	}
 
@@ -319,7 +318,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
@@ -338,7 +337,7 @@ func (s *ClusterSpec) RoachprodOpts(
 		if machineType == "" {
 			// If no machine type was specified, choose one
 			// based on the cloud and CPU count.
-			switch s.Cloud {
+			switch cloud {
 			case AWS:
 				machineType, selectedArch = SelectAWSMachineType(s.CPUs, s.Mem, preferLocalSSD && s.VolumeSize == 0, arch)
 			case GCE:
@@ -359,8 +358,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
@@ -373,21 +372,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
@@ -405,13 +404,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

@@ -24,18 +24,16 @@ import (
 )
 
 type testRegistryImpl struct {
-	m     map[string]*registry.TestSpec
-	cloud string
+	m map[string]*registry.TestSpec
 
 	promRegistry *prometheus.Registry
 }
 
 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),
 		promRegistry: prometheus.NewRegistry(),
 	}
@@ -59,7 +57,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-_=/,]+$"
@@ -129,7 +127,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

@@ -943,7 +943,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