Merge #99082

99082: server,upgrades: bootstrap at binaryMinVersion and run upgrades to BinaryVersionOverride r=dt,renatolabs a=adityamaru

Previously, tests that set BinaryVersionOverride would bootstrap
their test clusters at this overridden value. As part of this bootstrap
we would initialize the cluster with data as it appears on `binaryVersion`
i.e. system tables would be created using the schemas defined on the current `binaryVersion`.
Most tests that set BinaryVersionOverride would then manually call into `upgrade.Upgrade`
to move the cluster version forward. Since we have already bootstrapped the initial
data at `binaryVersion` these upgrades would not *really* run. We would just be testing
their idempotency unless the test did some gymnastics to "undo" the bootstrap and
then test the actual migration. So effectively, the `BinaryVersionOverride` made
the server think it was running that version but none of the associated upgrade
logic to reach that version was exercised.

In 23.1 we baked in the initial data that is bootstrapped on `binaryMinSupportedVersion`.
This development allows us to bootstrap the test cluster to `binaryMinSupportedVersion`
and *actually* step through the upgrades until `BinaryVersionOverride` before running the
test.

The new behaviour of setting this override is described below:

This value, when set, influences test cluster/server creation in two
different ways:

	 Case 1:
	 ------
	 If the test has not overridden the
	 `cluster.Settings.Version.BinaryMinSupportedVersion`, then the cluster will
	 be bootstrapped at `binaryMinSupportedVersion`  (if this server is the one
	 bootstrapping the cluster). After all the servers in the test cluster have
	 been started, `SET CLUSTER SETTING version = BinaryVersionOverride` will be
	 run to step through the upgrades until the specified override.

	 Case 2:
	 ------
	 If the test has overridden the
	 `cluster.Settings.Version.BinaryMinSupportedVersion` then it is not safe
	 for us to bootstrap at `binaryMinSupportedVersion` as it might be less than
	 the overridden minimum supported version. Furthermore, we do not have the
	 initial cluster data (system tables etc.) to bootstrap at the overridden
	 minimum supported version. In this case we bootstrap at
	 `BinaryVersionOverride` and populate the cluster with initial data
	 corresponding to the `binaryVersion`. In other words no upgrades are
	 *really* run and the server only thinks that it is running at
	 `BinaryVersionOverride`. Tests that fall in this category should be audited
	 for correctness.
	The version that we bootstrap at is also used when advertising this
	server's binary version when sending out join requests.

Informs: #97762

Release note: None

Co-authored-by: adityamaru <adityamaru@gmail.com>

pkg/server/init.go

@@ -624,12 +624,23 @@ func newInitServerConfig(
 	getDialOpts func(context.Context, string, rpc.ConnectionClass) ([]grpc.DialOption, error),
 ) initServerCfg {
 	binaryVersion := cfg.Settings.Version.BinaryVersion()
+	binaryMinSupportedVersion := cfg.Settings.Version.BinaryMinSupportedVersion()
 	if knobs := cfg.TestingKnobs.Server; knobs != nil {
+		// If BinaryVersionOverride is set, and our `binaryMinSupportedVersion` is
+		// at its default value, we must bootstrap the cluster at
+		// `binaryMinSupportedVersion`. This cluster will then run the necessary
+		// upgrades until `BinaryVersionOverride` before being ready to use in the
+		// test.
+		//
+		// Refer to the header comment on BinaryVersionOverride for more details.
 		if ov := knobs.(*TestingKnobs).BinaryVersionOverride; ov != (roachpb.Version{}) {
-			binaryVersion = ov
+			if binaryMinSupportedVersion.Equal(clusterversion.ByKey(clusterversion.BinaryMinSupportedVersionKey)) {
+				binaryVersion = clusterversion.ByKey(clusterversion.BinaryMinSupportedVersionKey)
+			} else {
+				binaryVersion = ov
+			}
 		}
 	}
-	binaryMinSupportedVersion := cfg.Settings.Version.BinaryMinSupportedVersion()
 	if binaryVersion.Less(binaryMinSupportedVersion) {
 		log.Fatalf(ctx, "binary version (%s) less than min supported version (%s)",
 			binaryVersion, binaryMinSupportedVersion)

pkg/server/migration_test.go

@@ -254,11 +254,13 @@ func TestUpgradeHappensAfterMigrations(t *testing.T) {
 		clusterversion.TestingBinaryMinSupportedVersion,
 		false, /* initializeVersion */
 	)
+	automaticUpgrade := make(chan struct{})
 	s, db, _ := serverutils.StartServer(t, base.TestServerArgs{
 		Settings: st,
 		Knobs: base.TestingKnobs{
 			Server: &TestingKnobs{
-				BinaryVersionOverride: clusterversion.TestingBinaryMinSupportedVersion,
+				DisableAutomaticVersionUpgrade: automaticUpgrade,
+				BinaryVersionOverride:          clusterversion.TestingBinaryMinSupportedVersion,
 			},
 			UpgradeManager: &upgradebase.TestingKnobs{
 				AfterRunPermanentUpgrades: func() {
@@ -272,6 +274,7 @@ func TestUpgradeHappensAfterMigrations(t *testing.T) {
 			},
 		},
 	})
+	close(automaticUpgrade)
 	sqlutils.MakeSQLRunner(db).
 		CheckQueryResultsRetry(t, `
 SELECT version = crdb_internal.node_executable_version()

pkg/server/node.go

@@ -352,9 +352,22 @@ func bootstrapCluster(
 				DefaultSystemZoneConfig: &initCfg.defaultSystemZoneConfig,
 				Codec:                   keys.SystemSQLCodec,
 			}
-			if initCfg.testingKnobs.Server != nil && initCfg.testingKnobs.Server.(*TestingKnobs).BootstrapVersionKeyOverride != 0 {
-				// Bootstrap using the given override key.
-				initialValuesOpts.OverrideKey = initCfg.testingKnobs.Server.(*TestingKnobs).BootstrapVersionKeyOverride
+			if initCfg.testingKnobs.Server != nil {
+				knobs := initCfg.testingKnobs.Server.(*TestingKnobs)
+				// If BinaryVersionOverride is set, and our `binaryMinSupportedVersion`
+				// is at its default value, we must populate the cluster with initial
+				// data from the `binaryMinSupportedVersion`. This cluster will then run
+				// the necessary upgrades until `BinaryVersionOverride` before being
+				// ready to use in the test.
+				if knobs.BinaryVersionOverride != (roachpb.Version{}) {
+					if initCfg.binaryMinSupportedVersion.Equal(
+						clusterversion.ByKey(clusterversion.BinaryMinSupportedVersionKey)) {
+						initialValuesOpts.OverrideKey = clusterversion.BinaryMinSupportedVersionKey
+					}
+				}
+				if knobs.BootstrapVersionKeyOverride != 0 {
+					initialValuesOpts.OverrideKey = initCfg.testingKnobs.Server.(*TestingKnobs).BootstrapVersionKeyOverride
+				}
 			}
 			initialValues, tableSplits, err := initialValuesOpts.GetInitialValuesCheckForOverrides()
 			if err != nil {

pkg/server/testing_knobs.go

@@ -60,26 +60,45 @@ type TestingKnobs struct {
 	// server fails to start.
 	RPCListener net.Listener
 
-	// BinaryVersionOverride overrides the binary version the CRDB server thinks
-	// it's running.
+	// BinaryVersionOverride overrides the binary version that the CRDB server
+	// will end up running. This value could also influence what version the
+	// cluster is bootstrapped at.
 	//
-	// This is consulted when bootstrapping clusters, opting to do it at the
-	// override instead of clusterversion.BinaryVersion (if this server is the
-	// one bootstrapping the cluster). This can als be used by tests to
-	// essentially that a new cluster is not starting from scratch, but instead
-	// is "created" by a node starting up with engines that had already been
-	// bootstrapped, at this BinaryVersionOverride. For example, it allows
-	// convenient creation of a cluster from a 2.1 binary, but that's running at
-	// version 2.0.
+	// This value, when set, influences test cluster/server creation in two
+	// different ways:
 	//
-	// It's also used when advertising this server's binary version when sending
-	// out join requests.
+	// Case 1:
+	// ------
+	// If the test has not overridden the
+	// `cluster.Settings.Version.BinaryMinSupportedVersion`, then the cluster will
+	// be bootstrapped at `binaryMinSupportedVersion`  (if this server is the one
+	// bootstrapping the cluster). After all the servers in the test cluster have
+	// been started, `SET CLUSTER SETTING version = BinaryVersionOverride` will be
+	// run to step through the upgrades until the specified override.
+	//
+	// TODO(adityamaru): We should force tests that set BinaryVersionOverride to
+	// also set BootstrapVersionKeyOverride so as to specify what image they would
+	// like the cluster bootstrapped at before upgrading to BinaryVersionOverride.
+	//
+	// Case 2:
+	// ------
+	// If the test has overridden the
+	// `cluster.Settings.Version.BinaryMinSupportedVersion` then it is not safe
+	// for us to bootstrap at `binaryMinSupportedVersion` as it might be less than
+	// the overridden minimum supported version. Furthermore, we do not have the
+	// initial cluster data (system tables etc.) to bootstrap at the overridden
+	// minimum supported version. In this case we bootstrap at
+	// `BinaryVersionOverride` and populate the cluster with initial data
+	// corresponding to the `binaryVersion`. In other words no upgrades are
+	// *really* run and the server only thinks that it is running at
+	// `BinaryVersionOverride`. Tests that fall in this category should be audited
+	// for correctness.
+	//
+	// The version that we bootstrap at is also used when advertising this
+	// server's binary version when sending out join requests.
 	//
 	// NB: When setting this, you probably also want to set
 	// DisableAutomaticVersionUpgrade.
-	//
-	// TODO(irfansharif): Update users of this testing knob to use the
-	// appropriate clusterversion.Handle instead.
 	BinaryVersionOverride roachpb.Version
 	// An (additional) callback invoked whenever a
 	// node is permanently removed from the cluster.

pkg/server/testserver.go

@@ -1749,6 +1749,15 @@ func (ts *TestServer) RangeDescIteratorFactory() interface{} {
 	return ts.sqlServer.execCfg.RangeDescIteratorFactory
 }
 
+// BinaryVersionOverride is part of the TestServerInterface.
+func (ts *TestServer) BinaryVersionOverride() roachpb.Version {
+	knobs := ts.TestingKnobs().Server
+	if knobs == nil {
+		return roachpb.Version{}
+	}
+	return knobs.(*TestingKnobs).BinaryVersionOverride
+}
+
 type testServerFactoryImpl struct{}
 
 // TestServerFactory can be passed to serverutils.InitTestServerFactory