storage, fs: move pebble cluster version compatibility check to InitEnv

Previously, we performed the CRDB version compatibility check for pebble and the
currently running cluster in the pebble constructor. This commit moves that
check to the file system environment initialization so that we can  colocate
with fs encyrption initialization.

We also extend the version checking to validate that pebble's minversion is not
too high for the current cockroach version, to guard against backwards
incompatible changes.
Fixes: #148213

Epic: none
Release note: None

Author: xinhaoz

pkg/cmd/roachtest/tests/BUILD.bazel

@@ -280,8 +280,8 @@ go_library(
         "//pkg/sql/pgwire/pgerror",
         "//pkg/sql/sem/tree",
         "//pkg/sql/ttl/ttlbase",
-        "//pkg/storage",
         "//pkg/storage/enginepb",
+        "//pkg/storage/minversion",
         "//pkg/testutils",
         "//pkg/testutils/fingerprintutils",
         "//pkg/testutils/floatcmp",

pkg/cmd/roachtest/tests/versionupgrade.go

@@ -23,7 +23,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/cmd/roachtest/test"
 	"github.com/cockroachdb/cockroach/pkg/roachprod/install"
 	"github.com/cockroachdb/cockroach/pkg/roachprod/logger"
-	"github.com/cockroachdb/cockroach/pkg/storage"
+	"github.com/cockroachdb/cockroach/pkg/storage/minversion"
 	"github.com/cockroachdb/cockroach/pkg/testutils/release"
 	"github.com/cockroachdb/cockroach/pkg/ts/tspb"
 	"github.com/cockroachdb/cockroach/pkg/util/timeutil"
@@ -281,7 +281,7 @@ func makeVersionFixtureAndFatal(
 	// #54761.
 	c.Run(ctx, option.WithNodes(c.Node(1)), "cp", "{store-dir}/cluster-bootstrapped", "{store-dir}/"+name)
 	// Similar to the above - newer versions require the min version file to open a store.
-	c.Run(ctx, option.WithNodes(c.All()), "cp", fmt.Sprintf("{store-dir}/%s", storage.MinVersionFilename), "{store-dir}/"+name)
+	c.Run(ctx, option.WithNodes(c.All()), "cp", fmt.Sprintf("{store-dir}/%s", minversion.MinVersionFilename), "{store-dir}/"+name)
 	c.Run(ctx, option.WithNodes(c.All()), "tar", "-C", "{store-dir}/"+name, "-czf", "{log-dir}/"+name+".tgz", ".")
 	t.Fatalf(`successfully created checkpoints; failing test on purpose.
 

pkg/kv/kvserver/logstore/sideload_test.go

@@ -598,7 +598,6 @@ func TestSideloadStorageSync(t *testing.T) {
 		// Reset filesystem to the last synced state.
 
 		// Emulate process restart. Load from the last synced state.
-		settings := cluster.MakeTestingClusterSettings()
 		env, err = fs.InitEnv(ctx, crashFS, "", fs.EnvConfig{
 			Version: settings.Version,
 		}, nil /* statsCollector */)

pkg/storage/engine_test.go

@@ -1072,7 +1072,10 @@ func TestCreateCheckpoint(t *testing.T) {
 }
 
 func mustInitTestEnv(t testing.TB, baseFS vfs.FS, dir string) *fs.Env {
-	e, err := fs.InitEnv(context.Background(), baseFS, dir, fs.EnvConfig{}, nil /* statsCollector */)
+	settings := cluster.MakeTestingClusterSettings()
+	e, err := fs.InitEnv(context.Background(), baseFS, dir, fs.EnvConfig{
+		Version: settings.Version,
+	}, nil /* statsCollector */)
 	require.NoError(t, err)
 	return e
 }

pkg/storage/fs/BUILD.bazel

@@ -17,9 +17,12 @@ go_library(
         "//pkg/base",
         "//pkg/cli/exit",
         "//pkg/clusterversion",
+        "//pkg/roachpb",
         "//pkg/settings",
+        "//pkg/settings/cluster",
         "//pkg/storage/disk",
         "//pkg/storage/enginepb",
+        "//pkg/storage/minversion",
         "//pkg/storage/storageconfig",
         "//pkg/util/buildutil",
         "//pkg/util/envutil",

pkg/storage/fs/fs.go

@@ -16,8 +16,11 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/base"
 	"github.com/cockroachdb/cockroach/pkg/cli/exit"
 	"github.com/cockroachdb/cockroach/pkg/clusterversion"
+	"github.com/cockroachdb/cockroach/pkg/roachpb"
 	"github.com/cockroachdb/cockroach/pkg/settings"
+	"github.com/cockroachdb/cockroach/pkg/settings/cluster"
 	"github.com/cockroachdb/cockroach/pkg/storage/disk"
+	"github.com/cockroachdb/cockroach/pkg/storage/minversion"
 	"github.com/cockroachdb/cockroach/pkg/storage/storageconfig"
 	"github.com/cockroachdb/cockroach/pkg/util/buildutil"
 	"github.com/cockroachdb/cockroach/pkg/util/envutil"
@@ -201,6 +204,54 @@ func InitEnv(
 		return nil, err
 	}
 
+	// Read the current store cluster version.
+	storeClusterVersion, minVerFileExists, err := minversion.GetMinVersion(e.UnencryptedFS, e.Dir)
+	if err != nil {
+		return nil, err
+	}
+
+	if minVerFileExists {
+		v := cfg.Version
+		if v == nil {
+			return nil, errors.New("version must not be nil")
+		}
+		// Avoid running a binary too new for this store. This is what you'd catch
+		// if, say, you restarted directly from v21.2 into v22.2 (bumping the min
+		// version) without going through v22.1 first.
+		//
+		// Note that "going through" above means that v22.1 successfully upgrades
+		// all existing stores. If v22.1 crashes half-way through the startup
+		// sequence (so now some stores have v21.2, but others v22.1) you are
+		// expected to run v22.1 again (hopefully without the crash this time) which
+		// would then rewrite all the stores.
+		if storeClusterVersion.Less(v.MinSupportedVersion()) {
+			if storeClusterVersion.Major < clusterversion.DevOffset && v.LatestVersion().Major >= clusterversion.DevOffset {
+				return nil, errors.Errorf(
+					"store last used with cockroach non-development version v%s "+
+						"cannot be opened by development version v%s",
+					storeClusterVersion, v.LatestVersion(),
+				)
+			}
+			return nil, errors.Errorf(
+				"store last used with cockroach version v%s "+
+					"is too old for running version v%s (which requires data from v%s or later)",
+				storeClusterVersion, v.LatestVersion(), v.MinSupportedVersion(),
+			)
+		}
+
+		// Avoid running a binary too old for this store. This protects against
+		// scenarios where an older binary attempts to open a store created by
+		// a newer version that may have incompatible data structures or formats.
+		if v.LatestVersion().Less(storeClusterVersion) {
+			return nil, errors.Errorf(
+				"store last used with cockroach version v%s is too high for running "+
+					"version v%s",
+				storeClusterVersion, v.LatestVersion(),
+			)
+		}
+		e.StoreClusterVersion = storeClusterVersion
+	}
+
 	// Validate and configure encryption-at-rest. If no encryption-at-rest
 	// configuration was provided, resolveEncryptedEnvOptions will validate that
 	// there is no file registry.
@@ -238,6 +289,11 @@ type Env struct {
 	// the store. It provides access to encryption-at-rest stats, etc.
 	Encryption *EncryptionEnv
 
+	// StoreClusterVersion is the version of the store as read from the
+	// min-version file. This value will be empty if the min-version file
+	// does not exist (eg, the store is being created for the first time).
+	StoreClusterVersion roachpb.Version
+
 	// defaultFS is the primary VFS that most users should use. If
 	// encryption-at-rest is enabled, this VFS will handle transparently
 	// encrypting and decrypting as necessary. When the Env is used as an
@@ -317,7 +373,12 @@ func (e *Env) onDiskSlow(info vfs.DiskSlowInfo) {
 
 // InMemory constructs a new in-memory environment.
 func InMemory() *Env {
-	e, err := InitEnv(context.Background(), vfs.NewMem(), "" /* dir */, EnvConfig{}, nil /* diskWriteStats */)
+	// For in-memory environments, we create a dummy cluster settings to provide
+	// a version handle, since these environments don't persist version information.
+	settings := cluster.MakeTestingClusterSettings()
+	e, err := InitEnv(context.Background(), vfs.NewMem(), "" /* dir */, EnvConfig{
+		Version: settings.Version,
+	}, nil /* diskWriteStats */)
 	// In practice InitEnv is infallible with this configuration.
 	if err != nil {
 		panic(err)
@@ -330,7 +391,10 @@ func InMemory() *Env {
 // encryption-at-rest. Since this function ignores the possibility of
 // encryption-at-rest, it should only be used as a testing convenience.
 func MustInitPhysicalTestingEnv(dir string) *Env {
-	e, err := InitEnv(context.Background(), vfs.Default, dir, EnvConfig{}, nil /* diskWriteStats */)
+	settings := cluster.MakeTestingClusterSettings()
+	e, err := InitEnv(context.Background(), vfs.Default, dir, EnvConfig{
+		Version: settings.Version,
+	}, nil /* diskWriteStats */)
 	if err != nil {
 		panic(err)
 	}

pkg/storage/metamorphic/generator.go

@@ -36,7 +36,10 @@ type engineConfig struct {
 }
 
 func (e *engineConfig) create(path string, baseFS vfs.FS) (storage.Engine, error) {
-	env, err := fs.InitEnv(context.Background(), baseFS, path, fs.EnvConfig{}, nil /* diskWriteStats */)
+	settings := cluster.MakeTestingClusterSettings()
+	env, err := fs.InitEnv(context.Background(), baseFS, path, fs.EnvConfig{
+		Version: settings.Version,
+	}, nil /* diskWriteStats */)
 	if err != nil {
 		return nil, err
 	}

pkg/storage/min_version_test.go

@@ -93,7 +93,13 @@ func TestSetMinVersion(t *testing.T) {
 	defer leaktest.AfterTest(t)()
 	defer log.Scope(t).Close(t)
 
-	p, err := Open(context.Background(), InMemory(), cluster.MakeClusterSettings(), CacheSize(0))
+	settings := cluster.MakeClusterSettings()
+	e, err := fs.InitEnv(context.Background(), vfs.NewMem(), "" /* dir */, fs.EnvConfig{
+		Version: settings.Version,
+	}, nil /* diskWriteStats */)
+	// In practice InitEnv is infallible with this configuration.
+	require.NoError(t, err)
+	p, err := Open(context.Background(), e, settings, CacheSize(0))
 	require.NoError(t, err)
 	defer p.Close()
 	require.Equal(t, MinimumSupportedFormatVersion, p.db.FormatMajorVersion())
@@ -121,7 +127,7 @@ func TestMinVersion_IsNotEncrypted(t *testing.T) {
 	baseFS := vfs.NewMem()
 	env, err := fs.InitEnv(ctx, baseFS, "", fs.EnvConfig{
 		EncryptionOptions: &storageconfig.EncryptionOptions{},
-		Version:     st.Version,
+		Version:           st.Version,
 	}, nil /* statsCollector */)
 	require.NoError(t, err)
 

pkg/storage/open_test.go

@@ -50,10 +50,27 @@ func TestWALFailover(t *testing.T) {
 		return nil
 	}
 
+	var settings *cluster.Settings
 	datadriven.RunTest(t, datapathutils.TestDataPath(t, "wal_failover_config"),
 		func(t *testing.T, td *datadriven.TestData) string {
 			switch td.Cmd {
 			case "mkenv":
+				// Mock a cluster version, defaulting to latest.
+				version := clusterversion.Latest.Version()
+				if td.HasArg("min-version") {
+					var major, minor int
+					td.ScanArgs(t, "min-version", &major, &minor)
+					version = roachpb.Version{
+						Major: int32(major),
+						Minor: int32(minor),
+					}
+				}
+				// Match the current offsetting policy.
+				if clusterversion.Latest.Version().Major > clusterversion.DevOffset {
+					version.Major += clusterversion.DevOffset
+				}
+				settings = cluster.MakeTestingClusterSettingsWithVersions(version, version, true /* initializeVersion */)
+
 				dir := td.CmdArgs[0].String()
 				if e := getEnv(dir); e != nil {
 					return fmt.Sprintf("env %s already exists", e.Dir)
@@ -62,6 +79,7 @@ func TestWALFailover(t *testing.T) {
 				require.NoError(t, memfs.MkdirAll(dir, os.ModePerm))
 
 				var envConfig fs.EnvConfig
+				envConfig.Version = settings.Version
 				if td.HasArg("encrypted-at-rest") {
 					envConfig.EncryptionOptions = &storageconfig.EncryptionOptions{}
 				}
@@ -107,22 +125,6 @@ func TestWALFailover(t *testing.T) {
 				}
 				openEnv.Ref()
 
-				// Mock a cluster version, defaulting to latest.
-				version := clusterversion.Latest.Version()
-				if td.HasArg("min-version") {
-					var major, minor int
-					td.ScanArgs(t, "min-version", &major, &minor)
-					version = roachpb.Version{
-						Major: int32(major),
-						Minor: int32(minor),
-					}
-				}
-				// Match the current offsetting policy.
-				if clusterversion.Latest.Version().Major > clusterversion.DevOffset {
-					version.Major += clusterversion.DevOffset
-				}
-				settings := cluster.MakeTestingClusterSettingsWithVersions(version, version, true /* initializeVersion */)
-
 				engine, err := Open(context.Background(), openEnv, settings, WALFailover(cfg, envs, defaultFS, nil))
 				if err != nil {
 					openEnv.Close()

pkg/storage/pebble.go

@@ -1078,54 +1078,25 @@ func newPebble(ctx context.Context, cfg engineConfig) (p *Pebble, err error) {
 			cfg.opts.Experimental.RemoteStorage = remoteStorageAdaptor{p: p, ctx: logCtx, factory: cfg.remoteStorageFactory}
 		}
 	}
-
-	// Read the current store cluster version.
-	storeClusterVersion, minVerFileExists, err := minversion.GetMinVersion(p.cfg.env.UnencryptedFS, cfg.env.Dir)
-	if err != nil {
-		return nil, err
-	}
-	if minVerFileExists {
-		// Avoid running a binary too new for this store. This is what you'd catch
-		// if, say, you restarted directly from v21.2 into v22.2 (bumping the min
-		// version) without going through v22.1 first.
-		//
-		// Note that "going through" above means that v22.1 successfully upgrades
-		// all existing stores. If v22.1 crashes half-way through the startup
-		// sequence (so now some stores have v21.2, but others v22.1) you are
-		// expected to run v22.1 again (hopefully without the crash this time) which
-		// would then rewrite all the stores.
-		if v := cfg.settings.Version; storeClusterVersion.Less(v.MinSupportedVersion()) {
-			if storeClusterVersion.Major < clusterversion.DevOffset && v.LatestVersion().Major >= clusterversion.DevOffset {
-				return nil, errors.Errorf(
-					"store last used with cockroach non-development version v%s "+
-						"cannot be opened by development version v%s",
-					storeClusterVersion, v.LatestVersion(),
-				)
-			}
-			return nil, errors.Errorf(
-				"store last used with cockroach version v%s "+
-					"is too old for running version v%s (which requires data from v%s or later)",
-				storeClusterVersion, v.LatestVersion(), v.MinSupportedVersion(),
-			)
-		}
-		cfg.opts.ErrorIfNotExists = true
-	} else {
-		if cfg.opts.ErrorIfNotExists || cfg.opts.ReadOnly {
-			// Make sure the message is not confusing if the store does exist but
-			// there is no min version file.
-			filename := p.cfg.env.UnencryptedFS.PathJoin(cfg.env.Dir, minversion.MinVersionFilename)
-			return nil, errors.Errorf(
-				"pebble: database %q does not exist (missing required file %q)",
-				cfg.env.Dir, filename,
-			)
-		}
-		// If there is no min version file, there should be no store. If there is
-		// one, it's either 1) a store from a very old version (which we don't want
-		// to open) or 2) an empty store that was created from a previous bootstrap
-		// attempt that failed right before writing out the min version file. We set
-		// a flag to disallow the open in case 1.
-		cfg.opts.ErrorIfNotPristine = true
+	// If the store cluster version is not empty, it means that we initialized
+	// the env using a min-version file. We can use that to determine if the
+	// store should already exist or not.
+	initFromMinVersionFile := cfg.env.StoreClusterVersion != (roachpb.Version{})
+
+	if !initFromMinVersionFile && (cfg.opts.ErrorIfNotExists || cfg.opts.ReadOnly) {
+		filename := p.cfg.env.UnencryptedFS.PathJoin(cfg.env.Dir, minversion.MinVersionFilename)
+		return nil, errors.Errorf(
+			"pebble: database %q does not exist (missing required file %q)",
+			cfg.env.Dir, filename,
+		)
 	}
+	cfg.opts.ErrorIfNotExists = cfg.opts.ErrorIfNotExists || initFromMinVersionFile
+	// If there is no min version file, there should be no store. If there is
+	// one, it's either 1) a store from a very old version (which we don't want
+	// to open) or 2) an empty store that was created from a previous bootstrap
+	// attempt that failed right before writing out the min version file. We set
+	// a flag to disallow the open in case 1.
+	cfg.opts.ErrorIfNotPristine = !initFromMinVersionFile
 
 	if WorkloadCollectorEnabled {
 		p.replayer.Attach(cfg.opts)
@@ -1134,10 +1105,10 @@ func newPebble(ctx context.Context, cfg engineConfig) (p *Pebble, err error) {
 	db, err := pebble.Open(cfg.env.Dir, cfg.opts)
 	if err != nil {
 		// Decorate the errors caused by the flags we set above.
-		if minVerFileExists && errors.Is(err, pebble.ErrDBDoesNotExist) {
+		if initFromMinVersionFile && errors.Is(err, pebble.ErrDBDoesNotExist) {
 			err = errors.Wrap(err, "min version file exists but store doesn't")
 		}
-		if !minVerFileExists && errors.Is(err, pebble.ErrDBNotPristine) {
+		if !initFromMinVersionFile && errors.Is(err, pebble.ErrDBNotPristine) {
 			err = errors.Wrap(err, "store has no min-version file; this can "+
 				"happen if the store was created by an old CockroachDB version that is no "+
 				"longer supported")
@@ -1146,7 +1117,8 @@ func newPebble(ctx context.Context, cfg engineConfig) (p *Pebble, err error) {
 	}
 	p.db = db
 
-	if !minVerFileExists {
+	storeClusterVersion := cfg.env.StoreClusterVersion
+	if storeClusterVersion == (roachpb.Version{}) {
 		storeClusterVersion = cfg.settings.Version.ActiveVersionOrEmpty(ctx).Version
 		if storeClusterVersion == (roachpb.Version{}) {
 			// If there is no active version, use the minimum supported version.