Merge cockroachdb#127837

127837: roachtest: add RequiresDeprecatedWorkload testSpec r=DarrylWong a=DarrylWong

Previously we required the workload binary to be present for all tests to run even if the test did not use it.

Now that we have removed all unnecessary usage of the deprecated workload, we can switch the validation of the workload binary to be opt in.

Fixes:  cockroachdb#128172, cockroachdb#127954
Epic: none
Release note: none

Co-authored-by: DarrylWong <darryl@cockroachlabs.com>

pkg/cmd/roachtest/cluster.go

@@ -324,7 +324,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)
@@ -2006,6 +2007,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

@@ -105,6 +105,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

@@ -730,6 +730,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
 
@@ -856,6 +861,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,19 +26,20 @@ 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
-		randomized         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
+		randomized                 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:
@@ -77,12 +78,13 @@ 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,
-				randomized:    true,
-				nativeLibs:    registry.LibGEOS,
+				name:                       "version-upgrade",
+				fn:                         runVersionUpgrade,
+				timeout:                    2 * time.Hour, // actually lower in local runs; see `runVersionUpgrade`
+				defaultLeases:              true,
+				randomized:                 true,
+				nativeLibs:                 registry.LibGEOS,
+				requiresDeprecatedWorkload: true, // uses schemachange
 			},
 		},
 		registry.OwnerDisasterRecovery: {
@@ -146,16 +148,17 @@ 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),
-				Randomized:        tc.randomized,
-				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),
+				Randomized:                 tc.randomized,
+				RequiresLicense:            tc.requiresLicense,
+				RequiresDeprecatedWorkload: tc.requiresDeprecatedWorkload,
 			}
 
 			if tc.timeout != 0 {

pkg/cmd/roachtest/tests/admission_control_index_overload.go

@@ -96,7 +96,7 @@ func registerIndexOverload(r registry.Registry) {
 			m.Go(func(ctx context.Context) error {
 				testDurationStr := " --duration=" + testDuration.String()
 				concurrency := ifLocal(c, "  --concurrency=8", " --concurrency=2048")
-				c.Run(ctx, option.WithNodes(c.CRDBNodes()),
+				c.Run(ctx, option.WithNodes(c.WorkloadNode()),
 					"./cockroach workload run kv --read-percent=50 --max-rate=1000 --max-block-bytes=4096"+
 						testDurationStr+concurrency+fmt.Sprintf(" {pgurl%s}", c.CRDBNodes()),
 				)

pkg/cmd/roachtest/tests/admission_control_tpcc_overload.go

@@ -54,10 +54,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) + `"`
@@ -163,16 +159,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

@@ -86,11 +86,12 @@ func registerBackupRestoreRoundTrip(r registry.Registry) {
 			RequiresLicense:   true,
 			NativeLibs:        registry.LibGEOS,
 			// See https://github.com/cockroachdb/cockroach/issues/105968
-			CompatibleClouds:          registry.Clouds(spec.GCE, spec.Local),
-			Suites:                    registry.Suites(registry.Nightly),
-			TestSelectionOptOutSuites: registry.Suites(registry.Nightly),
-			Randomized:                true,
-			Skip:                      sp.skip,
+			CompatibleClouds:           registry.Clouds(spec.GCE, spec.Local),
+			Suites:                     registry.Suites(registry.Nightly),
+			TestSelectionOptOutSuites:  registry.Suites(registry.Nightly),
+			Randomized:                 true,
+			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

@@ -1725,20 +1725,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}"