roachtest: make path to default cockroach public

The roachtest test runner already uploads the `cockroach` binary
passed with `--cockroach` to every node in the cluster. By making the
path to that file public, we allow tests to use that binary, stopping
avoidable uploads.

Concretely, this commit removes a step from `mixedversion` tests where
the same logic of uploading the current binary took place. Tests can
use the new constant in the `test` package instead.

Epic: none

Release note: None

Author: Renato Costa

pkg/cmd/roachtest/cluster.go

@@ -115,7 +115,6 @@ const (
 	defaultEncryptionProbability = 1
 	defaultFIPSProbability       = 0
 	defaultARM64Probability      = 0
-	defaultCockroachPath         = "./cockroach-default"
 )
 
 type errBinaryOrLibraryNotFound struct {
@@ -1347,7 +1346,7 @@ func (c *clusterImpl) FetchLogs(ctx context.Context, l *logger.Logger) error {
 			}
 		}
 
-		if err := c.RunE(ctx, c.All(), fmt.Sprintf("mkdir -p logs/redacted && %s debug merge-logs --redact logs/*.log > logs/redacted/combined.log", defaultCockroachPath)); err != nil {
+		if err := c.RunE(ctx, c.All(), fmt.Sprintf("mkdir -p logs/redacted && %s debug merge-logs --redact logs/*.log > logs/redacted/combined.log", test.DefaultCockroachPath)); err != nil {
 			l.Printf("failed to redact logs: %v", err)
 			if ctx.Err() != nil {
 				return err
@@ -1428,7 +1427,7 @@ func (c *clusterImpl) FetchTimeseriesData(ctx context.Context, l *logger.Logger)
 			sec = fmt.Sprintf("--certs-dir=%s", certs)
 		}
 		if err := c.RunE(
-			ctx, c.Node(node), fmt.Sprintf("%s debug tsdump %s --format=raw > tsdump.gob", defaultCockroachPath, sec),
+			ctx, c.Node(node), fmt.Sprintf("%s debug tsdump %s --format=raw > tsdump.gob", test.DefaultCockroachPath, sec),
 		); err != nil {
 			return err
 		}
@@ -1499,15 +1498,15 @@ func (c *clusterImpl) FetchDebugZip(ctx context.Context, l *logger.Logger, dest
 			// Ignore the files in the the log directory; we pull the logs separately anyway
 			// so this would only cause duplication.
 			excludeFiles := "*.log,*.txt,*.pprof"
-			cmd := roachtestutil.NewCommand("%s debug zip", defaultCockroachPath).
+			cmd := roachtestutil.NewCommand("%s debug zip", test.DefaultCockroachPath).
 				Option("include-range-info").
 				Flag("exclude-files", fmt.Sprintf("'%s'", excludeFiles)).
 				Flag("url", fmt.Sprintf("{pgurl:%d}", i)).
 				MaybeFlag(c.IsSecure(), "certs-dir", "certs").
 				Arg(zipName).
 				String()
 			if err := c.RunE(ctx, c.Node(i), cmd); err != nil {
-				l.Printf("%s debug zip failed on node %d: %v", defaultCockroachPath, i, err)
+				l.Printf("%s debug zip failed on node %d: %v", test.DefaultCockroachPath, i, err)
 				if i < c.spec.NodeCount {
 					continue
 				}
@@ -1928,15 +1927,15 @@ func (c *clusterImpl) PutE(
 }
 
 // PutDefaultCockroach uploads the cockroach binary passed in the
-// command line to `defaultCockroachPath` in every node in the
+// command line to `test.DefaultCockroachPath` in every node in the
 // cluster. This binary is used by the test runner to collect failure
 // artifacts since tests are free to upload the cockroach binary they
 // use to any location they desire.
 func (c *clusterImpl) PutDefaultCockroach(
 	ctx context.Context, l *logger.Logger, cockroachPath string,
 ) error {
 	c.status("uploading default cockroach binary to nodes")
-	return c.PutE(ctx, l, cockroachPath, defaultCockroachPath, c.All())
+	return c.PutE(ctx, l, cockroachPath, test.DefaultCockroachPath, c.All())
 }
 
 // PutLibraries inserts the specified libraries, by name, into all nodes on the cluster

pkg/cmd/roachtest/roachtestutil/mixedversion/mixedversion.go

@@ -111,13 +111,6 @@ const (
 	// cluster that can use the test fixtures in
 	// `pkg/cmd/roachtest/fixtures`.
 	numNodesInFixtures = 4
-
-	// CurrentCockroachPath is the path to the binary where the current
-	// version of cockroach being tested is located. This file is
-	// uploaded before any user functions are run. The primary use case
-	// are tests that need long runnig background functions on startup
-	// (such as running a workload).
-	CurrentCockroachPath = "./cockroach-current"
 )
 
 var (
@@ -581,35 +574,6 @@ func (s startStep) Run(ctx context.Context, l *logger.Logger, c cluster.Cluster,
 	return clusterupgrade.StartWithSettings(ctx, l, c, s.crdbNodes, startOpts, clusterSettings...)
 }
 
-// uploadCurrentVersionStep uploads the current cockroach binary to
-// all DB nodes in the test. This is so that startup steps can use
-// them (if, for instance, they need to run a workload). The binary
-// will be located in `dest`.
-type uploadCurrentVersionStep struct {
-	id        int
-	rt        test.Test
-	crdbNodes option.NodeListOption
-	dest      string
-}
-
-func (s uploadCurrentVersionStep) ID() int                { return s.id }
-func (s uploadCurrentVersionStep) Background() shouldStop { return nil }
-
-func (s uploadCurrentVersionStep) Description() string {
-	return fmt.Sprintf("upload current binary to all cockroach nodes (%v)", s.crdbNodes)
-}
-
-func (s uploadCurrentVersionStep) Run(
-	ctx context.Context, l *logger.Logger, c cluster.Cluster, h *Helper,
-) error {
-	_, err := clusterupgrade.UploadVersion(ctx, s.rt, l, c, s.crdbNodes, clusterupgrade.MainVersion)
-	if err != nil {
-		return err
-	}
-
-	return c.RunE(ctx, s.crdbNodes, fmt.Sprintf("mv ./cockroach %s", s.dest))
-}
-
 // waitForStableClusterVersionStep implements the process of waiting
 // for the `version` cluster setting being the same on all nodes of
 // the cluster and equal to the binary version of the first node in

pkg/cmd/roachtest/roachtestutil/mixedversion/planner.go

@@ -132,7 +132,6 @@ func (p *testPlanner) initSteps() []testStep {
 
 	return append(
 		append(steps,
-			uploadCurrentVersionStep{id: p.nextID(), rt: p.rt, crdbNodes: p.crdbNodes, dest: CurrentCockroachPath},
 			waitForStableClusterVersionStep{id: p.nextID(), nodes: p.crdbNodes, timeout: p.options.upgradeTimeout},
 			preserveDowngradeOptionStep{id: p.nextID(), prng: p.newRNG(), crdbNodes: p.crdbNodes},
 		),

pkg/cmd/roachtest/roachtestutil/mixedversion/planner_test.go

@@ -69,45 +69,44 @@ func TestTestPlanner(t *testing.T) {
 
 	plan, err := mvt.plan()
 	require.NoError(t, err)
-	require.Len(t, plan.steps, 11)
+	require.Len(t, plan.steps, 10)
 
 	// Assert on the pretty-printed version of the test plan as that
 	// asserts the ordering of the steps we want to take, and as a bonus
 	// tests the printing function itself.
 	expectedPrettyPlan := fmt.Sprintf(`
 mixed-version test plan for upgrading from %[1]s to <current>:
 ├── starting cluster at version "%[1]s" (1)
-├── upload current binary to all cockroach nodes (:1-4) (2)
-├── wait for nodes :1-4 to all have the same cluster version (same as binary version of node 1) (3)
-├── preventing auto-upgrades by setting `+"`preserve_downgrade_option`"+` (4)
-├── run "initialize bank workload" (5)
+├── wait for nodes :1-4 to all have the same cluster version (same as binary version of node 1) (2)
+├── preventing auto-upgrades by setting `+"`preserve_downgrade_option`"+` (3)
+├── run "initialize bank workload" (4)
 ├── start background hooks concurrently
-│   ├── run "bank workload", after 50ms delay (6)
-│   ├── run "rand workload", after 200ms delay (7)
-│   └── run "csv server", after 500ms delay (8)
+│   ├── run "bank workload", after 50ms delay (5)
+│   ├── run "rand workload", after 200ms delay (6)
+│   └── run "csv server", after 500ms delay (7)
 ├── upgrade nodes :1-4 from "%[1]s" to "<current>"
-│   ├── restart node 1 with binary version <current> (9)
-│   ├── run "mixed-version 1" (10)
-│   ├── restart node 4 with binary version <current> (11)
-│   ├── restart node 3 with binary version <current> (12)
-│   ├── run "mixed-version 2" (13)
-│   └── restart node 2 with binary version <current> (14)
+│   ├── restart node 1 with binary version <current> (8)
+│   ├── run "mixed-version 1" (9)
+│   ├── restart node 4 with binary version <current> (10)
+│   ├── restart node 3 with binary version <current> (11)
+│   ├── run "mixed-version 2" (12)
+│   └── restart node 2 with binary version <current> (13)
 ├── downgrade nodes :1-4 from "<current>" to "%[1]s"
-│   ├── restart node 4 with binary version %[1]s (15)
-│   ├── run "mixed-version 2" (16)
-│   ├── restart node 2 with binary version %[1]s (17)
-│   ├── restart node 3 with binary version %[1]s (18)
-│   ├── restart node 1 with binary version %[1]s (19)
-│   └── run "mixed-version 1" (20)
+│   ├── restart node 4 with binary version %[1]s (14)
+│   ├── run "mixed-version 2" (15)
+│   ├── restart node 2 with binary version %[1]s (16)
+│   ├── restart node 3 with binary version %[1]s (17)
+│   ├── restart node 1 with binary version %[1]s (18)
+│   └── run "mixed-version 1" (19)
 ├── upgrade nodes :1-4 from "%[1]s" to "<current>"
-│   ├── restart node 4 with binary version <current> (21)
-│   ├── run "mixed-version 1" (22)
-│   ├── restart node 1 with binary version <current> (23)
-│   ├── restart node 2 with binary version <current> (24)
-│   ├── run "mixed-version 2" (25)
-│   └── restart node 3 with binary version <current> (26)
-├── finalize upgrade by resetting `+"`preserve_downgrade_option`"+` (27)
-└── wait for nodes :1-4 to all have the same cluster version (same as binary version of node 1) (28)
+│   ├── restart node 4 with binary version <current> (20)
+│   ├── run "mixed-version 1" (21)
+│   ├── restart node 1 with binary version <current> (22)
+│   ├── restart node 2 with binary version <current> (23)
+│   ├── run "mixed-version 2" (24)
+│   └── restart node 3 with binary version <current> (25)
+├── finalize upgrade by resetting `+"`preserve_downgrade_option`"+` (26)
+└── wait for nodes :1-4 to all have the same cluster version (same as binary version of node 1) (27)
 `, predecessorVersion)
 
 	expectedPrettyPlan = expectedPrettyPlan[1:] // remove leading newline
@@ -121,7 +120,7 @@ mixed-version test plan for upgrading from %[1]s to <current>:
 	mvt.OnStartup("startup 2", dummyHook)
 	plan, err = mvt.plan()
 	require.NoError(t, err)
-	requireConcurrentHooks(t, plan.steps[4], "startup 1", "startup 2")
+	requireConcurrentHooks(t, plan.steps[3], "startup 1", "startup 2")
 
 	// Assert that AfterUpgradeFinalized hooks are scheduled to run in
 	// the last step of the test.
@@ -131,8 +130,8 @@ mixed-version test plan for upgrading from %[1]s to <current>:
 	mvt.AfterUpgradeFinalized("finalizer 3", dummyHook)
 	plan, err = mvt.plan()
 	require.NoError(t, err)
-	require.Len(t, plan.steps, 10)
-	requireConcurrentHooks(t, plan.steps[9], "finalizer 1", "finalizer 2", "finalizer 3")
+	require.Len(t, plan.steps, 9)
+	requireConcurrentHooks(t, plan.steps[8], "finalizer 1", "finalizer 2", "finalizer 3")
 }
 
 // TestDeterministicTestPlan tests that generating a test plan with
@@ -199,15 +198,15 @@ func TestDeterministicHookSeeds(t *testing.T) {
 
 		// We can hardcode these paths since we are using a fixed seed in
 		// these tests.
-		firstRun := plan.steps[4].(sequentialRunStep).steps[4].(runHookStep)
+		firstRun := plan.steps[3].(sequentialRunStep).steps[4].(runHookStep)
 		require.Equal(t, "do something", firstRun.hook.name)
 		require.NoError(t, firstRun.Run(ctx, nilLogger, nilCluster, emptyHelper))
 
-		secondRun := plan.steps[5].(sequentialRunStep).steps[1].(runHookStep)
+		secondRun := plan.steps[4].(sequentialRunStep).steps[1].(runHookStep)
 		require.Equal(t, "do something", secondRun.hook.name)
 		require.NoError(t, secondRun.Run(ctx, nilLogger, nilCluster, emptyHelper))
 
-		thirdRun := plan.steps[6].(sequentialRunStep).steps[3].(runHookStep)
+		thirdRun := plan.steps[5].(sequentialRunStep).steps[3].(runHookStep)
 		require.Equal(t, "do something", thirdRun.hook.name)
 		require.NoError(t, thirdRun.Run(ctx, nilLogger, nilCluster, emptyHelper))
 

pkg/cmd/roachtest/test/test_interface.go

@@ -15,6 +15,11 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/util/version"
 )
 
+// DefaultCockroachPath is the path where the binary passed to the
+// `--cockroach` flag will be made available in every node in the
+// cluster.
+const DefaultCockroachPath = "./cockroach-default"
+
 // Test is the interface through which roachtests interact with the
 // test harness.
 type Test interface {

pkg/cmd/roachtest/tests/mixed_version_backup.go

@@ -813,7 +813,7 @@ func (sc *systemTableContents) loadShowResults(
 	}
 
 	query := fmt.Sprintf("SELECT * FROM [%s]%s", showStmt, aostFor(timestamp))
-	showCmd := roachtestutil.NewCommand("%s sql", mixedversion.CurrentCockroachPath).
+	showCmd := roachtestutil.NewCommand("%s sql", test.DefaultCockroachPath).
 		Flag("certs-dir", "certs").
 		Flag("e", fmt.Sprintf("%q", query)).
 		String()