Merge cockroachdb#97595

97595: clusterversion: allow tests to pretend to be a release branch r=srosenberg a=renatolabs

This commit adds a new environment variable that can be used by tests to force a development binary to treated as a release binary: `COCKROACH_TESTING_FORCE_RELEASE_BRANCH`.

Previously, upgrade roachtests were setting the
`COCKROACH_UPGRADE_TO_DEV_VERSION` variable to allow `master` to be deployed during tests. The main downside of this approach is that it is not the logic that real cluster would run when upgrading. In addition, it could lead to upgrades that seem to go "backwards", which can be quite confusing.

With this change, upgrade roachtests bypass the version offsetting logic and are treated as the "next release" during tests.

This commit also reverts cockroachdb#95904, as the workaround introduced there is no longer necessary.

Resolves: cockroachdb#92608.

Epic: CRDB-19321

Release note: None

Co-authored-by: Renato Costa <renato@cockroachlabs.com>

pkg/clusterversion/cockroach_versions.go

@@ -795,12 +795,16 @@ var rawVersionsSingleton = keyedVersions{
 	// *************************************************
 }
 
-// developmentBranch must true on the main development branch but should be set
-// to false on a release branch once the set of versions becomes append-only and
-// associated upgrade implementations are frozen. It can be forced to true via
-// an env var even on a release branch, to allow running a release binary in a
-// dev cluster.
-var developmentBranch = true || envutil.EnvOrDefaultBool("COCKROACH_FORCE_DEV_VERSION", false)
+// developmentBranch must true on the main development branch but
+// should be set to false on a release branch once the set of versions
+// becomes append-only and associated upgrade implementations are
+// frozen. It can be forced to a specific value in two circumstances:
+// 1. forced to `false` on development branches: this is used for
+// upgrade testing purposes and should never be done in real clusters;
+// 2. forced to `false` on release branches: this allows running a
+// release binary in a dev cluster.
+var developmentBranch = !envutil.EnvOrDefaultBool("COCKROACH_TESTING_FORCE_RELEASE_BRANCH", false) ||
+	envutil.EnvOrDefaultBool("COCKROACH_FORCE_DEV_VERSION", false)
 
 const (
 	// finalVersion should be set on a release branch to the minted final cluster

pkg/cmd/roachtest/tests/BUILD.bazel

@@ -181,7 +181,6 @@ go_library(
         "//pkg/cloud",
         "//pkg/cloud/amazon",
         "//pkg/cloud/gcp",
-        "//pkg/clusterversion",
         "//pkg/cmd/cmpconn",
         "//pkg/cmd/roachtest/cluster",
         "//pkg/cmd/roachtest/clusterstats",

pkg/cmd/roachtest/tests/gossip.go

@@ -429,8 +429,9 @@ SELECT count(replicas)
 	// Restart node 1, but have it listen on a different port for internal
 	// connections. This will require node 1 to reach out to the other nodes in
 	// the cluster for gossip info.
+	defaultEnv := strings.Join(install.MakeClusterSettings().Env, " ")
 	err := c.RunE(ctx, c.Node(1),
-		`./cockroach start --insecure --background --store={store-dir} `+
+		defaultEnv+` ./cockroach start --insecure --background --store={store-dir} `+
 			`--log-dir={log-dir} --cache=10% --max-sql-memory=10% `+
 			`--listen-addr=:$[{pgport:1}+10000] --http-port=$[{pgport:1}+1] `+
 			`--join={pghost:1}:{pgport:1}`+

pkg/cmd/roachtest/tests/multitenant_upgrade.go

@@ -17,13 +17,11 @@ import (
 	"runtime"
 	"time"
 
-	"github.com/cockroachdb/cockroach/pkg/clusterversion"
 	"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/cluster"
 	"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/option"
 	"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/registry"
 	"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/roachtestutil/clusterupgrade"
 	"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/test"
-	"github.com/cockroachdb/cockroach/pkg/roachpb"
 	"github.com/cockroachdb/cockroach/pkg/roachprod/install"
 	"github.com/cockroachdb/cockroach/pkg/testutils/sqlutils"
 	"github.com/cockroachdb/cockroach/pkg/util/version"
@@ -109,8 +107,7 @@ func runMultiTenantUpgrade(ctx context.Context, t test.Test, c cluster.Cluster,
 
 	verifySQL(t, tenant11.pgURL,
 		mkStmt("SHOW CLUSTER SETTING version").
-			withResults([][]string{{initialVersion}}).
-			withCustomResultsEqualFn(requireEqualVersionsIgnoreDevOffset),
+			withResults([][]string{{initialVersion}}),
 	)
 
 	t.Status("preserving downgrade option on system tenant")
@@ -154,8 +151,7 @@ func runMultiTenantUpgrade(ctx context.Context, t test.Test, c cluster.Cluster,
 		mkStmt(`SELECT * FROM foo LIMIT 1`).
 			withResults([][]string{{"1", "bar"}}),
 		mkStmt("SHOW CLUSTER SETTING version").
-			withResults([][]string{{initialVersion}}).
-			withCustomResultsEqualFn(requireEqualVersionsIgnoreDevOffset),
+			withResults([][]string{{initialVersion}}),
 	)
 
 	t.Status("creating a new tenant 13")
@@ -177,8 +173,7 @@ func runMultiTenantUpgrade(ctx context.Context, t test.Test, c cluster.Cluster,
 		mkStmt(`SELECT * FROM foo LIMIT 1`).
 			withResults([][]string{{"1", "bar"}}),
 		mkStmt("SHOW CLUSTER SETTING version").
-			withResults([][]string{{initialVersion}}).
-			withCustomResultsEqualFn(requireEqualVersionsIgnoreDevOffset),
+			withResults([][]string{{initialVersion}}),
 	)
 
 	t.Status("stopping the tenant 11 server ahead of upgrading")
@@ -193,9 +188,7 @@ func runMultiTenantUpgrade(ctx context.Context, t test.Test, c cluster.Cluster,
 			mkStmt(`SELECT * FROM foo LIMIT 1`).
 				withResults([][]string{{"1", "bar"}}),
 			mkStmt("SHOW CLUSTER SETTING version").
-				withResults([][]string{{initialVersion}}).
-				withCustomResultsEqualFn(requireEqualVersionsIgnoreDevOffset),
-		)
+				withResults([][]string{{initialVersion}}))
 	}
 
 	t.Status("attempting to upgrade tenant 11 before storage cluster is finalized and expecting a failure")
@@ -215,8 +208,7 @@ func runMultiTenantUpgrade(ctx context.Context, t test.Test, c cluster.Cluster,
 		mkStmt(`SELECT * FROM foo LIMIT 1`).
 			withResults([][]string{{"1", "bar"}}),
 		mkStmt("SHOW CLUSTER SETTING version").
-			withResults([][]string{{initialVersion}}).
-			withCustomResultsEqualFn(requireEqualVersionsIgnoreDevOffset),
+			withResults([][]string{{initialVersion}}),
 		mkStmt("SET CLUSTER SETTING version = crdb_internal.node_executable_version()"),
 		mkStmt("SELECT version = crdb_internal.node_executable_version() FROM [SHOW CLUSTER SETTING version]").
 			withResults([][]string{{"true"}}),
@@ -233,9 +225,7 @@ func runMultiTenantUpgrade(ctx context.Context, t test.Test, c cluster.Cluster,
 		mkStmt(`SELECT * FROM foo LIMIT 1`).
 			withResults([][]string{{"1", "bar"}}),
 		mkStmt("SHOW CLUSTER SETTING version").
-			withResults([][]string{{initialVersion}}).
-			withCustomResultsEqualFn(requireEqualVersionsIgnoreDevOffset),
-	)
+			withResults([][]string{{initialVersion}}))
 
 	// Upgrade the tenant created in the mixed version state to the final version.
 	t.Status("migrating tenant 12 to the current version")
@@ -307,45 +297,19 @@ func runMultiTenantUpgrade(ctx context.Context, t test.Test, c cluster.Cluster,
 			withResults([][]string{{"true"}}))
 }
 
-// EqualFn is implemented by both `require.Equal` and `requireEqualVersionsIgnoreDevOffset`.
-type EqualFn = func(t require.TestingT, expected interface{}, actual interface{}, msgAndArgs ...interface{})
-
-// TODO: replace this with require.Equal once #92608 is closed because
-// asserting on specific cluster versions makes for a stronger
-// test (and having or not having the version offset is part of that).
-func requireEqualVersionsIgnoreDevOffset(
-	t require.TestingT, expected interface{}, actual interface{}, msgAndArgs ...interface{},
-) {
-	normalizeVersion := func(v roachpb.Version) roachpb.Version {
-		if v.Major > clusterversion.DevOffset {
-			v.Major -= clusterversion.DevOffset
-		}
-		return v
-	}
-	normalizedExpectedVersion := normalizeVersion(roachpb.MustParseVersion(expected.([][]string)[0][0]))
-	normalizedActualVersion := normalizeVersion(roachpb.MustParseVersion(actual.([][]string)[0][0]))
-	require.Equal(t, normalizedExpectedVersion, normalizedActualVersion, msgAndArgs...)
-}
-
 type sqlVerificationStmt struct {
-	stmt                   string
-	args                   []interface{}
-	optionalResults        [][]string
-	optionalResultsEqualFn EqualFn
+	stmt            string
+	args            []interface{}
+	optionalResults [][]string
 }
 
 func (s sqlVerificationStmt) withResults(res [][]string) sqlVerificationStmt {
 	s.optionalResults = res
 	return s
 }
 
-func (s sqlVerificationStmt) withCustomResultsEqualFn(fn EqualFn) sqlVerificationStmt {
-	s.optionalResultsEqualFn = fn
-	return s
-}
-
 func mkStmt(stmt string, args ...interface{}) sqlVerificationStmt {
-	return sqlVerificationStmt{stmt: stmt, args: args, optionalResultsEqualFn: require.Equal}
+	return sqlVerificationStmt{stmt: stmt, args: args}
 }
 
 func openDBAndMakeSQLRunner(t test.Test, url string) (*sqlutils.SQLRunner, func()) {
@@ -366,7 +330,7 @@ func verifySQL(t test.Test, url string, stmts ...sqlVerificationStmt) {
 			tdb.Exec(t, stmt.stmt, stmt.args...)
 		} else {
 			res := tdb.QueryStr(t, stmt.stmt, stmt.args...)
-			stmt.optionalResultsEqualFn(t, stmt.optionalResults, res)
+			require.Equal(t, stmt.optionalResults, res)
 		}
 	}
 }

pkg/roachprod/config/config.go

@@ -82,11 +82,12 @@ const (
 // See 'generateStartCmd' which sets 'ENV_VARS' for the systemd startup script (start.sh).
 func DefaultEnvVars() []string {
 	return []string{
-		// Allow upgrading a stable release data-dir to a dev version.
-		// N.B. many roachtests which perform upgrade scenarios require this env. var after changes in [1]; otherwise,
-		// the tests will fail even on release branches when attempting to upgrade previous (stable) release to an alpha.
-		// [1] https://github.com/cockroachdb/cockroach/pull/87468
-		"COCKROACH_UPGRADE_TO_DEV_VERSION=true",
+		// We set the following environment variable to pretend that the
+		// current development build is the next binary release (which
+		// disables version offsetting). In upgrade tests, we're interested
+		// in testing the upgrade logic that users would actually run when
+		// they upgrade from one release to another.
+		"COCKROACH_TESTING_FORCE_RELEASE_BRANCH=true",
 	}
 }