Merge #127170

127170: server: do not initialize cluster version to minimum supported r=stevendanna a=renatolabs

Previously, shared-process tenants would initialize their view of the cluster version early and set it to the `MinSupportedVersion` as a "safe" option. However, that setting can likely be wrong, especially now that we plan to support skip-version upgrades.

In this commit, we remove that code and rely on the initialization that happens from storage in `preStart`. This means that we now expose a non-negligible amount of code to a state where we don't have an initialized cluster version. However, having that error out is better than have code assume a potentially wrong version.

A similar discussion and approach happened for separate process tenants a while ago: see #100684 and #104859.

Fixes: #126754

Release note: None

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

Author: craig[bot]

pkg/server/server_controller_new_server.go

@@ -18,7 +18,6 @@ import (
 	"path/filepath"
 
 	"github.com/cockroachdb/cockroach/pkg/base"
-	"github.com/cockroachdb/cockroach/pkg/clusterversion"
 	"github.com/cockroachdb/cockroach/pkg/multitenant/mtinfopb"
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
 	"github.com/cockroachdb/cockroach/pkg/security/clientsecopts"
@@ -189,21 +188,6 @@ func makeSharedProcessTenantServerConfig(
 	stopper *stop.Stopper,
 	nodeMetricsRecorder *status.MetricsRecorder,
 ) (baseCfg BaseConfig, sqlCfg SQLConfig, err error) {
-	// We need a value in the version setting prior to the update
-	// coming from the system.settings table. This value must be valid
-	// and compatible with the state of the tenant's keyspace.
-	//
-	// Since we don't know at which binary version the tenant
-	// keyspace was initialized, we must be conservative and
-	// assume it was created a long time ago; and that we may
-	// have to run all known migrations since then. So initialize
-	// the version setting to the minimum supported version.
-	if err := clusterversion.Initialize(
-		ctx, st.Version.MinSupportedVersion(), &st.SV,
-	); err != nil {
-		return BaseConfig{}, SQLConfig{}, err
-	}
-
 	tr := tracing.NewTracerWithOpt(ctx, tracing.WithClusterSettings(&st.SV))
 
 	// Define a tenant store. This will be used to write the

pkg/server/server_sql.go

@@ -1472,16 +1472,48 @@ func (s *SQLServer) preStart(
 	// NB: In the context of the system tenant, we may not have
 	// the version setting in SQL yet even though the in memory
 	// setting has been initialized in.
-	currentVersion := s.execCfg.Settings.Version.ActiveVersionOrEmpty(ctx).Version
-	if currentVersion.Equal(roachpb.Version{}) {
-		if err := s.execCfg.DB.Txn(ctx, func(ctx context.Context, txn *kv.Txn) (err error) {
-			v, err := s.settingsWatcher.GetClusterVersionFromStorage(ctx, txn)
-			if err != nil {
-				return err
+	initializeClusterVersion := func(ctx context.Context) error {
+		currentVersion := s.execCfg.Settings.Version.ActiveVersionOrEmpty(ctx).Version
+		if currentVersion.Equal(roachpb.Version{}) {
+			if err := s.execCfg.DB.Txn(ctx, func(ctx context.Context, txn *kv.Txn) (err error) {
+				v, err := s.settingsWatcher.GetClusterVersionFromStorage(ctx, txn)
+				if err != nil {
+					return err
+				}
+				return clusterversion.Initialize(ctx, v.Version, &s.execCfg.Settings.SV)
+			}); err != nil {
+				return errors.Wrap(err, "initializing cluster version")
 			}
-			return clusterversion.Initialize(ctx, v.Version, &s.execCfg.Settings.SV)
-		}); err != nil {
-			return errors.Wrap(err, "initializing cluster version")
+		}
+
+		return nil
+	}
+
+	if s.execCfg.Codec.ForSystemTenant() {
+		if err := initializeClusterVersion(ctx); err != nil {
+			return err
+		}
+	} else {
+		// When initializing the cluster version for tenants, we retry if
+		// the error indicates that the KV authorizer hasn't seen our
+		// service-mode transition. This is possible because both the
+		// service controller and the authorizer receive tenant mode
+		// updates asynchronously via rangefeeds.
+		opts := retry.Options{MaxRetries: 5}
+		var nonRetryableErr error
+		err := opts.Do(ctx, func(ctx context.Context) error {
+			if err := initializeClusterVersion(ctx); err != nil {
+				if strings.Contains(err.Error(), `operation not allowed when in service mode "none"`) {
+					return err
+				}
+				nonRetryableErr = err
+			}
+
+			return nil
+		})
+
+		if err != nil || nonRetryableErr != nil {
+			return errors.CombineErrors(err, nonRetryableErr)
 		}
 	}