Merge cockroachdb#106275

106275: roachtest: support `versions-binary-override` in `mixedversion` r=srosenberg a=renatolabs

The `BinaryPathFromVersion` (now renamed to `BinaryPathForVersion` based on an early pull request review comment) was not taking into account the `versions-binary-override` flag passed to roachtest when determining the path where the binary for a certain version should live in a node.

This commit updates that function to take that into account. This improves the debugging experience for mixedversion tests by letting the user swap the binary of a certain version for a custom one, possibly with more debug code.

Epic: CRDB-19321

Release note: None

Co-authored-by: Renato Costa <[email protected]>

Author: craig[bot]

pkg/cmd/roachtest/main.go

@@ -237,7 +237,7 @@ Use --bench to list benchmarks instead of tests.
 
 Each test has a set of tags. The tags are used to skip tests which don't match
 the tag filter. The tag filter is specified by specifying a pattern with the
-"tag:" prefix. 
+"tag:" prefix.
 
 If multiple "tag:" patterns are specified, the test must match at
 least one of them.
@@ -404,8 +404,8 @@ runner itself.
 		cmd.Flags().StringToStringVar(
 			&versionsBinaryOverride, "versions-binary-override", nil,
 			"List of <version>=<path to cockroach binary>. If a certain version <ver> "+
-				"is present in the list,"+"the respective binary will be used when a "+
-				"multi-version test asks for the respective binary, instead of "+
+				"is present in the list, the respective binary will be used when a "+
+				"mixed-version test asks for the respective binary, instead of "+
 				"`roachprod stage <ver>`. Example: 20.1.4=cockroach-20.1,20.2.0=cockroach-20.2.")
 	}
 

pkg/cmd/roachtest/roachtestutil/clusterupgrade/clusterupgrade.go

@@ -81,33 +81,35 @@ func UploadVersion(
 	nodes option.NodeListOption,
 	newVersion string,
 ) (string, error) {
-	binaryName := "./cockroach"
-	if newVersion == MainVersion {
-		if err := c.PutE(ctx, l, t.Cockroach(), binaryName, nodes); err != nil {
-			return "", err
-		}
-	} else if binary, ok := t.VersionsBinaryOverride()[newVersion]; ok {
-		// If an override has been specified for newVersion, use that binary.
-		l.Printf("using binary override for version %s: %s", newVersion, binary)
-		binaryName = "./cockroach-" + newVersion
-		if err := c.PutE(ctx, l, binary, binaryName, nodes); err != nil {
+	dstBinary := BinaryPathForVersion(t, newVersion)
+	srcBinary := t.Cockroach()
+
+	overrideBinary, isOverriden := t.VersionsBinaryOverride()[newVersion]
+	if isOverriden {
+		l.Printf("using binary override for version %s: %s", newVersion, overrideBinary)
+		srcBinary = overrideBinary
+	}
+
+	if newVersion == MainVersion || isOverriden {
+		if err := c.PutE(ctx, l, srcBinary, dstBinary, nodes); err != nil {
 			return "", err
 		}
 	} else {
 		v := "v" + newVersion
-		dir := v
-		binaryName = filepath.Join(dir, "cockroach")
+		dir := filepath.Dir(dstBinary)
+
 		// Check if the cockroach binary already exists.
-		if err := c.RunE(ctx, nodes, "test", "-e", binaryName); err != nil {
-			if err := c.RunE(ctx, nodes, "mkdir", "-p", dir); err != nil {
-				return "", err
-			}
-			if err := c.Stage(ctx, l, "release", v, dir, nodes); err != nil {
-				return "", err
-			}
+		cmd := fmt.Sprintf("test -e %s || mkdir -p %s", dstBinary, dir)
+		if err := c.RunE(ctx, nodes, cmd); err != nil {
+			return "", err
+		}
+
+		if err := c.Stage(ctx, l, "release", v, dir, nodes); err != nil {
+			return "", err
 		}
 	}
-	return BinaryPathFromVersion(newVersion), nil
+
+	return dstBinary, nil
 }
 
 // InstallFixtures copies the previously created fixtures (in
@@ -157,14 +159,19 @@ func StartWithSettings(
 	return c.StartE(ctx, l, startOpts, settings, nodes)
 }
 
-// BinaryPathFromVersion shows where the binary for the given version
-// can be found on roachprod nodes. It's either `./cockroach` or the
-// path to which a released binary is staged.
-func BinaryPathFromVersion(v string) string {
-	if v == "" {
+// BinaryPathForVersion shows where the binary for the given version
+// is expected to be found on roachprod nodes. The file will only
+// actually exist if there was a previous call to `UploadVersion` with
+// the same version parameter.
+func BinaryPathForVersion(t test.Test, v string) string {
+	if v == MainVersion {
 		return "./cockroach"
+	} else if _, ok := t.VersionsBinaryOverride()[v]; ok {
+		// If an override has been specified for `v`, use that binary.
+		return "./cockroach-" + v
+	} else {
+		return filepath.Join("v"+v, "cockroach")
 	}
-	return filepath.Join("v"+v, "cockroach")
 }
 
 // RestartNodesWithNewBinary uploads a given cockroach version to the

pkg/cmd/roachtest/tests/mixed_version_backup.go

@@ -1971,7 +1971,7 @@ func (mvb *mixedVersionBackup) resetCluster(
 		return fmt.Errorf("failed to wipe cluster: %w", err)
 	}
 
-	cockroachPath := clusterupgrade.BinaryPathFromVersion(version)
+	cockroachPath := clusterupgrade.BinaryPathForVersion(mvb.t, version)
 	return clusterupgrade.StartWithSettings(
 		ctx, l, mvb.cluster, mvb.roachNodes, option.DefaultStartOptsNoBackups(),
 		install.BinaryOption(cockroachPath), install.SecureOption(true),

pkg/cmd/roachtest/tests/versionupgrade.go

@@ -378,7 +378,7 @@ func makeVersionFixtureAndFatal(
 			name := clusterupgrade.CheckpointName(u.binaryVersion(ctx, t, 1).String())
 			u.c.Stop(ctx, t.L(), option.DefaultStopOpts(), c.All())
 
-			binaryPath := clusterupgrade.BinaryPathFromVersion(fixtureVersion)
+			binaryPath := clusterupgrade.BinaryPathForVersion(t, fixtureVersion)
 			c.Run(ctx, c.All(), binaryPath, "debug", "pebble", "db", "checkpoint",
 				"{store-dir}", "{store-dir}/"+name)
 			// The `cluster-bootstrapped` marker can already be found within