Merge pull request cockroachdb#99587 from cockroachdb/blathers/backpo…

…rt-release-23.1-99082

release-23.1: server,upgrades: bootstrap at binaryMinVersion and run upgrades to BinaryVersionOverride

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