Merge pull request #128189 from DarrylWong/backport24.1-127837

release-24.1: roachtest: add RequiresDeprecatedWorkload testSpec

pkg/cmd/roachtest/cluster.go

@@ -323,7 +323,8 @@ func initBinariesAndLibraries() {
 	cockroachEAPath := roachtestflags.CockroachEAPath
 	workloadPath := roachtestflags.WorkloadPath
 	cockroach[defaultArch], _ = resolveBinary("cockroach", cockroachPath, defaultArch, true, false)
-	workload[defaultArch], _ = resolveBinary("workload", workloadPath, defaultArch, true, false)
+	// Let the test runner verify the workload binary exists if TestSpec.RequiresDeprecatedWorkload is true.
+	workload[defaultArch], _ = resolveBinary("workload", workloadPath, defaultArch, false, false)
 	cockroachEA[defaultArch], err = resolveBinary("cockroach-ea", cockroachEAPath, defaultArch, false, true)
 	if err != nil {
 		fmt.Fprintf(os.Stderr, "WARN: unable to find %q for %q: %s\n", "cockroach-ea", defaultArch, err)
@@ -1965,6 +1966,20 @@ func (c *clusterImpl) PutLibraries(
 	return nil
 }
 
+// PutDeprecatedWorkload checks if the test requires the deprecated
+// workload and that it has a workload node provisioned. If it does,
+// then it auto-uploads the workload binary to the workload node.
+// If the test requires the binary but doesn't have a workload node,
+// it should handle uploading it in the test itself.
+func (c *clusterImpl) PutDeprecatedWorkload(
+	ctx context.Context, l *logger.Logger, t *testImpl,
+) error {
+	if t.spec.RequiresDeprecatedWorkload && t.spec.Cluster.WorkloadNode {
+		return c.PutE(ctx, l, t.DeprecatedWorkload(), test.DefaultDeprecatedWorkloadPath, c.WorkloadNode())
+	}
+	return nil
+}
+
 // Stage stages a binary to the cluster.
 func (c *clusterImpl) Stage(
 	ctx context.Context,

pkg/cmd/roachtest/registry/test_spec.go

@@ -92,6 +92,15 @@ type TestSpec struct {
 	// in the environment.
 	RequiresLicense bool
 
+	// RequiresDeprecatedWorkload indicates that the test requires
+	// the 'workload' binary to be present for the test to run. Use
+	// this to ensure tests will fail-early if a 'workload' binary
+	// does not exist.
+	//
+	// N.B. The workload binary is deprecated, use 'cockroach workload'
+	// instead if the desired workload exists there.
+	RequiresDeprecatedWorkload bool
+
 	// EncryptionSupport encodes to what extent tests supports
 	// encryption-at-rest. See the EncryptionSupport type for details.
 	// Encryption support is opt-in -- i.e., if the TestSpec does not

pkg/cmd/roachtest/test/test_interface.go

@@ -20,6 +20,11 @@ import (
 // cluster.
 const DefaultCockroachPath = "./cockroach"
 
+// DefaultDeprecatedWorkloadPath is the path where the binary passed
+// to the `--workload` flag will be made available in the workload
+// node if one is provisioned.
+const DefaultDeprecatedWorkloadPath = "./workload"
+
 // EnvAssertionsEnabledSeed is the name of the environment variable
 // that, when set, causes roachtest to use a binary with runtime
 // assertions enabled (if available), using the random seed contained

pkg/cmd/roachtest/test_impl.go

@@ -195,6 +195,12 @@ func (t *testImpl) StandardCockroach() string {
 }
 
 func (t *testImpl) DeprecatedWorkload() string {
+	// Discourage usage of the deprecated workload by gating it behind the
+	// 'RequiresDeprecatedWorkload' test spec. Tests should use 'cockroach
+	// workload' instead when possible.
+	if !t.spec.RequiresDeprecatedWorkload {
+		t.Fatal("Using deprecated workload but `RequiresDeprecatedWorkload` is not set in test spec.")
+	}
 	return t.deprecatedWorkload
 }
 

pkg/cmd/roachtest/test_runner.go

@@ -717,6 +717,11 @@ func (r *testRunner) runWorker(
 			return err
 		}
 
+		// Verify that the deprecated workload is available if needed.
+		if testToRun.spec.RequiresDeprecatedWorkload && workload[arch] == "" {
+			return errors.Errorf("%s requires deprecated workload binary but one was not found", testToRun.spec.Name)
+		}
+
 		var clusterCreateErr error
 		var vmCreateOpts *vm.CreateOpts
 
@@ -818,6 +823,9 @@ func (r *testRunner) runWorker(
 			if setupErr == nil {
 				setupErr = c.PutLibraries(ctx, "./lib", t.spec.NativeLibs)
 			}
+			if setupErr == nil {
+				setupErr = c.PutDeprecatedWorkload(ctx, l, t)
+			}
 
 			if setupErr != nil {
 				// If there was an error setting up the cluster (uploading

pkg/cmd/roachtest/tests/acceptance.go

@@ -26,18 +26,19 @@ func registerAcceptance(r registry.Registry) {
 	cloudsWithoutServiceRegistration := registry.AllClouds.Remove(registry.CloudsWithServiceRegistration)
 
 	testCases := map[registry.Owner][]struct {
-		name               string
-		fn                 func(ctx context.Context, t test.Test, c cluster.Cluster)
-		skip               string
-		numNodes           int
-		nodeRegions        []string
-		timeout            time.Duration
-		encryptionSupport  registry.EncryptionSupport
-		defaultLeases      bool
-		requiresLicense    bool
-		nativeLibs         []string
-		workloadNode       bool
-		incompatibleClouds registry.CloudSet
+		name                       string
+		fn                         func(ctx context.Context, t test.Test, c cluster.Cluster)
+		skip                       string
+		numNodes                   int
+		nodeRegions                []string
+		timeout                    time.Duration
+		encryptionSupport          registry.EncryptionSupport
+		defaultLeases              bool
+		requiresLicense            bool
+		nativeLibs                 []string
+		workloadNode               bool
+		incompatibleClouds         registry.CloudSet
+		requiresDeprecatedWorkload bool
 	}{
 		// NOTE: acceptance tests are lightweight tests that run as part
 		// of CI. As such, they must:
@@ -76,11 +77,12 @@ func registerAcceptance(r registry.Registry) {
 		},
 		registry.OwnerTestEng: {
 			{
-				name:          "version-upgrade",
-				fn:            runVersionUpgrade,
-				timeout:       2 * time.Hour, // actually lower in local runs; see `runVersionUpgrade`
-				defaultLeases: true,
-				nativeLibs:    registry.LibGEOS,
+				name:                       "version-upgrade",
+				fn:                         runVersionUpgrade,
+				timeout:                    2 * time.Hour, // actually lower in local runs; see `runVersionUpgrade`
+				defaultLeases:              true,
+				nativeLibs:                 registry.LibGEOS,
+				requiresDeprecatedWorkload: true, // uses schemachange
 			},
 		},
 		registry.OwnerDisasterRecovery: {
@@ -143,15 +145,16 @@ func registerAcceptance(r registry.Registry) {
 			}
 
 			testSpec := registry.TestSpec{
-				Name:              "acceptance/" + tc.name,
-				Owner:             owner,
-				Cluster:           r.MakeClusterSpec(numNodes, extraOptions...),
-				Skip:              tc.skip,
-				EncryptionSupport: tc.encryptionSupport,
-				Timeout:           10 * time.Minute,
-				CompatibleClouds:  registry.AllClouds.Remove(tc.incompatibleClouds),
-				Suites:            registry.Suites(registry.Nightly, registry.Quick, registry.Acceptance),
-				RequiresLicense:   tc.requiresLicense,
+				Name:                       "acceptance/" + tc.name,
+				Owner:                      owner,
+				Cluster:                    r.MakeClusterSpec(numNodes, extraOptions...),
+				Skip:                       tc.skip,
+				EncryptionSupport:          tc.encryptionSupport,
+				Timeout:                    10 * time.Minute,
+				CompatibleClouds:           registry.AllClouds.Remove(tc.incompatibleClouds),
+				Suites:                     registry.Suites(registry.Nightly, registry.Quick, registry.Acceptance),
+				RequiresLicense:            tc.requiresLicense,
+				RequiresDeprecatedWorkload: tc.requiresDeprecatedWorkload,
 			}
 
 			if tc.timeout != 0 {

pkg/cmd/roachtest/tests/admission_control_tpcc_overload.go

@@ -53,10 +53,6 @@ func (s tpccOLAPSpec) run(ctx context.Context, t test.Test, c cluster.Cluster) {
 		ctx, t, t.L(), c, tpccOptions{
 			Warehouses: s.Warehouses, SetupType: usingImport,
 		})
-	// We make use of querybench below, only available through the `workload`
-	// binary.
-	c.Put(ctx, t.DeprecatedWorkload(), "./workload", c.WorkloadNode())
-
 	const queryFileName = "queries.sql"
 	// querybench expects the entire query to be on a single line.
 	queryLine := `"` + strings.Replace(tpccOlapQuery, "\n", " ", -1) + `"`
@@ -162,16 +158,17 @@ func registerTPCCOverload(r registry.Registry) {
 		name := fmt.Sprintf("admission-control/tpcc-olap/nodes=%d/cpu=%d/w=%d/c=%d",
 			s.Nodes, s.CPUs, s.Warehouses, s.Concurrency)
 		r.Add(registry.TestSpec{
-			Name:              name,
-			Owner:             registry.OwnerAdmissionControl,
-			Benchmark:         true,
-			CompatibleClouds:  registry.AllExceptAWS,
-			Suites:            registry.Suites(registry.Weekly),
-			Cluster:           r.MakeClusterSpec(s.Nodes+1, spec.CPU(s.CPUs), spec.WorkloadNode()),
-			Run:               s.run,
-			EncryptionSupport: registry.EncryptionMetamorphic,
-			Leases:            registry.MetamorphicLeases,
-			Timeout:           20 * time.Minute,
+			Name:                       name,
+			Owner:                      registry.OwnerAdmissionControl,
+			Benchmark:                  true,
+			CompatibleClouds:           registry.AllExceptAWS,
+			Suites:                     registry.Suites(registry.Weekly),
+			Cluster:                    r.MakeClusterSpec(s.Nodes+1, spec.CPU(s.CPUs), spec.WorkloadNode()),
+			Run:                        s.run,
+			EncryptionSupport:          registry.EncryptionMetamorphic,
+			Leases:                     registry.MetamorphicLeases,
+			Timeout:                    20 * time.Minute,
+			RequiresDeprecatedWorkload: true, // uses querybench
 		})
 	}
 }

pkg/cmd/roachtest/tests/backup_restore_roundtrip.go

@@ -79,16 +79,17 @@ func registerBackupRestoreRoundTrip(r registry.Registry) {
 	} {
 		sp := sp
 		r.Add(registry.TestSpec{
-			Name:              sp.name,
-			Timeout:           4 * time.Hour,
-			Owner:             registry.OwnerDisasterRecovery,
-			Cluster:           r.MakeClusterSpec(4, spec.WorkloadNode()),
-			EncryptionSupport: registry.EncryptionMetamorphic,
-			RequiresLicense:   true,
-			NativeLibs:        registry.LibGEOS,
-			CompatibleClouds:  registry.OnlyGCE,
-			Suites:            registry.Suites(registry.Nightly),
-			Skip:              sp.skip,
+			Name:                       sp.name,
+			Timeout:                    4 * time.Hour,
+			Owner:                      registry.OwnerDisasterRecovery,
+			Cluster:                    r.MakeClusterSpec(4, spec.WorkloadNode()),
+			EncryptionSupport:          registry.EncryptionMetamorphic,
+			RequiresLicense:            true,
+			NativeLibs:                 registry.LibGEOS,
+			CompatibleClouds:           registry.OnlyGCE,
+			Suites:                     registry.Suites(registry.Nightly),
+			Skip:                       sp.skip,
+			RequiresDeprecatedWorkload: true, // uses schemachange
 			Run: func(ctx context.Context, t test.Test, c cluster.Cluster) {
 				backupRestoreRoundTrip(ctx, t, c, sp)
 			},

pkg/cmd/roachtest/tests/cdc.go

@@ -1351,20 +1351,18 @@ func registerCDC(r registry.Registry) {
 		// TODO(mrtracy): This workload is designed to be running on a 20CPU nodes,
 		// but this cannot be allocated without some sort of configuration outside
 		// of this test. Look into it.
-		Benchmark:        true,
-		Cluster:          r.MakeClusterSpec(4, spec.CPU(16), spec.WorkloadNode()),
-		Leases:           registry.MetamorphicLeases,
-		CompatibleClouds: registry.AllExceptAWS,
-		Suites:           registry.Suites(registry.Nightly),
-		RequiresLicense:  true,
+		Benchmark:                  true,
+		Cluster:                    r.MakeClusterSpec(4, spec.CPU(16), spec.WorkloadNode()),
+		Leases:                     registry.MetamorphicLeases,
+		CompatibleClouds:           registry.AllExceptAWS,
+		Suites:                     registry.Suites(registry.Nightly),
+		RequiresLicense:            true,
+		RequiresDeprecatedWorkload: true, // uses ledger
 		Run: func(ctx context.Context, t test.Test, c cluster.Cluster) {
 			ct := newCDCTester(ctx, t, c)
 			defer ct.Close()
 
 			workloadStart := timeutil.Now()
-			// The ledger workload is not available in the cockroach binary so we
-			// must use the deprecated workload.
-			c.Put(ctx, t.DeprecatedWorkload(), "./workload", ct.workloadNode)
 			ct.runLedgerWorkload(ledgerArgs{duration: "28m"})
 
 			alterStmt := "ALTER DATABASE ledger CONFIGURE ZONE USING range_max_bytes = 805306368, range_min_bytes = 134217728"

pkg/cmd/roachtest/tests/connection_latency.go

@@ -109,9 +109,10 @@ func registerConnectionLatencyTest(r registry.Registry) {
 		Owner:     registry.OwnerSQLFoundations,
 		Benchmark: true,
 		// Add one more node for load node.
-		Cluster:          r.MakeClusterSpec(numNodes+1, spec.WorkloadNode(), spec.GCEZones(regionUsCentral)),
-		CompatibleClouds: registry.OnlyGCE,
-		Suites:           registry.Suites(registry.Nightly),
+		Cluster:                    r.MakeClusterSpec(numNodes+1, spec.WorkloadNode(), spec.GCEZones(regionUsCentral)),
+		CompatibleClouds:           registry.OnlyGCE,
+		Suites:                     registry.Suites(registry.Nightly),
+		RequiresDeprecatedWorkload: true, // uses connectionlatency
 		Run: func(ctx context.Context, t test.Test, c cluster.Cluster) {
 			runConnectionLatencyTest(ctx, t, c, numNodes, 1, false /*password*/)
 		},
@@ -124,24 +125,26 @@ func registerConnectionLatencyTest(r registry.Registry) {
 	loadNodes := numZones
 
 	r.Add(registry.TestSpec{
-		Name:             fmt.Sprintf("connection_latency/nodes=%d/multiregion/certs", numMultiRegionNodes),
-		Owner:            registry.OwnerSQLFoundations,
-		Benchmark:        true,
-		Cluster:          r.MakeClusterSpec(numMultiRegionNodes+loadNodes, spec.Geo(), spec.GCEZones(geoZonesStr)),
-		CompatibleClouds: registry.OnlyGCE,
-		Suites:           registry.Suites(registry.Nightly),
+		Name:                       fmt.Sprintf("connection_latency/nodes=%d/multiregion/certs", numMultiRegionNodes),
+		Owner:                      registry.OwnerSQLFoundations,
+		Benchmark:                  true,
+		Cluster:                    r.MakeClusterSpec(numMultiRegionNodes+loadNodes, spec.Geo(), spec.GCEZones(geoZonesStr)),
+		CompatibleClouds:           registry.OnlyGCE,
+		Suites:                     registry.Suites(registry.Nightly),
+		RequiresDeprecatedWorkload: true, // uses connectionlatency
 		Run: func(ctx context.Context, t test.Test, c cluster.Cluster) {
 			runConnectionLatencyTest(ctx, t, c, numMultiRegionNodes, numZones, false /*password*/)
 		},
 	})
 
 	r.Add(registry.TestSpec{
-		Name:             fmt.Sprintf("connection_latency/nodes=%d/multiregion/password", numMultiRegionNodes),
-		Owner:            registry.OwnerSQLFoundations,
-		Benchmark:        true,
-		Cluster:          r.MakeClusterSpec(numMultiRegionNodes+loadNodes, spec.Geo(), spec.GCEZones(geoZonesStr)),
-		CompatibleClouds: registry.OnlyGCE,
-		Suites:           registry.Suites(registry.Nightly),
+		Name:                       fmt.Sprintf("connection_latency/nodes=%d/multiregion/password", numMultiRegionNodes),
+		Owner:                      registry.OwnerSQLFoundations,
+		Benchmark:                  true,
+		Cluster:                    r.MakeClusterSpec(numMultiRegionNodes+loadNodes, spec.Geo(), spec.GCEZones(geoZonesStr)),
+		CompatibleClouds:           registry.OnlyGCE,
+		Suites:                     registry.Suites(registry.Nightly),
+		RequiresDeprecatedWorkload: true, // uses connectionlatency
 		Run: func(ctx context.Context, t test.Test, c cluster.Cluster) {
 			runConnectionLatencyTest(ctx, t, c, numMultiRegionNodes, numZones, true /*password*/)
 		},

pkg/cmd/roachtest/tests/indexes.go

@@ -44,8 +44,9 @@ func registerNIndexes(r registry.Registry, secondaryIndexes int) {
 			spec.AWSZones(strings.Join(awsGeoZones, ",")),
 		),
 		// TODO(radu): enable this test on AWS.
-		CompatibleClouds: registry.OnlyGCE,
-		Suites:           registry.Suites(registry.Nightly),
+		CompatibleClouds:           registry.OnlyGCE,
+		Suites:                     registry.Suites(registry.Nightly),
+		RequiresDeprecatedWorkload: true, // uses indexes
 		// Uses CONFIGURE ZONE USING ... COPY FROM PARENT syntax.
 		Run: func(ctx context.Context, t test.Test, c cluster.Cluster) {
 			firstAZ := gceGeoZones[0]
@@ -54,9 +55,6 @@ func registerNIndexes(r registry.Registry, secondaryIndexes int) {
 			}
 			gatewayNodes := c.Range(1, nodes/3)
 
-			// The indexes workload is not available in the cockroach binary,
-			// so we must use the deprecated workload.
-			c.Put(ctx, t.DeprecatedWorkload(), "./workload", c.WorkloadNode())
 			c.Start(ctx, t.L(), option.DefaultStartOpts(), install.MakeClusterSettings(), c.CRDBNodes())
 			conn := c.Conn(ctx, t.L(), 1)
 

pkg/cmd/roachtest/tests/inverted_index.go

@@ -26,12 +26,13 @@ import (
 
 func registerSchemaChangeInvertedIndex(r registry.Registry) {
 	r.Add(registry.TestSpec{
-		Name:             "schemachange/invertedindex",
-		Owner:            registry.OwnerSQLFoundations,
-		Cluster:          r.MakeClusterSpec(5, spec.WorkloadNode()),
-		CompatibleClouds: registry.AllExceptAWS,
-		Suites:           registry.Suites(registry.Nightly),
-		Leases:           registry.MetamorphicLeases,
+		Name:                       "schemachange/invertedindex",
+		Owner:                      registry.OwnerSQLFoundations,
+		Cluster:                    r.MakeClusterSpec(5, spec.WorkloadNode()),
+		CompatibleClouds:           registry.AllExceptAWS,
+		Suites:                     registry.Suites(registry.Nightly),
+		Leases:                     registry.MetamorphicLeases,
+		RequiresDeprecatedWorkload: true, // uses json
 		Run: func(ctx context.Context, t test.Test, c cluster.Cluster) {
 			runSchemaChangeInvertedIndex(ctx, t, c)
 		},
@@ -41,9 +42,6 @@ func registerSchemaChangeInvertedIndex(r registry.Registry) {
 // runInvertedIndex tests the correctness and performance of building an
 // inverted index on randomly generated JSON data (from the JSON workload).
 func runSchemaChangeInvertedIndex(ctx context.Context, t test.Test, c cluster.Cluster) {
-	// The json workload is not available in the cockroach binary,
-	// so we must use the deprecated workload.
-	c.Put(ctx, t.DeprecatedWorkload(), "./workload", c.WorkloadNode())
 	c.Start(ctx, t.L(), option.DefaultStartOpts(), install.MakeClusterSettings(), c.CRDBNodes())
 
 	cmdInit := "./workload init json {pgurl:1}"

pkg/cmd/roachtest/tests/ledger.go

@@ -28,19 +28,16 @@ func registerLedger(r registry.Registry) {
 	// https://github.com/cockroachdb/cockroach/issues/66184
 	const azs = "us-central1-f,us-central1-b,us-central1-c"
 	r.Add(registry.TestSpec{
-		Name:             fmt.Sprintf("ledger/nodes=%d/multi-az", nodes),
-		Owner:            registry.OwnerKV,
-		Benchmark:        true,
-		Cluster:          r.MakeClusterSpec(nodes+1, spec.CPU(16), spec.WorkloadNode(), spec.WorkloadNodeCPU(16), spec.Geo(), spec.GCEZones(azs)),
-		CompatibleClouds: registry.OnlyGCE,
-		Suites:           registry.Suites(registry.Nightly),
+		Name:                       fmt.Sprintf("ledger/nodes=%d/multi-az", nodes),
+		Owner:                      registry.OwnerKV,
+		Benchmark:                  true,
+		Cluster:                    r.MakeClusterSpec(nodes+1, spec.CPU(16), spec.WorkloadNode(), spec.WorkloadNodeCPU(16), spec.Geo(), spec.GCEZones(azs)),
+		CompatibleClouds:           registry.OnlyGCE,
+		Suites:                     registry.Suites(registry.Nightly),
+		RequiresDeprecatedWorkload: true, // uses ledger
 		Run: func(ctx context.Context, t test.Test, c cluster.Cluster) {
 			gatewayNodes := c.Range(1, nodes/3)
 
-			// The ledger workload is not available in the cockroach binary,
-			// so we must use the deprecated workload.
-			c.Put(ctx, t.DeprecatedWorkload(), "./workload", c.WorkloadNode())
-
 			// Don't start a scheduled backup on this perf sensitive roachtest that reports to roachperf.
 			c.Start(ctx, t.L(), option.NewStartOpts(option.NoBackupSchedule), install.MakeClusterSettings(), c.CRDBNodes())