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/cli/auto_decrypt_fs_test.go

@@ -72,11 +72,13 @@ func TestAutoDecryptFS(t *testing.T) {
 	expected := `
 $TMPDIR/path1: mkdir-all:  0777
 $TMPDIR/path1: lock: LOCK
+$TMPDIR/path1: open: STORAGE_MIN_VERSION
 $TMPDIR/path1: mkdir-all: $TMPDIR/path1 0755
 $TMPDIR/path1: create: $TMPDIR/path1/bar
 $TMPDIR/path1: close: $TMPDIR/path1/bar
 $TMPDIR/foo/path2: mkdir-all:  0777
 $TMPDIR/foo/path2: lock: LOCK
+$TMPDIR/foo/path2: open: STORAGE_MIN_VERSION
 $TMPDIR/foo/path2: mkdir-all: $TMPDIR/foo/path2 0755
 $TMPDIR/foo/path2: create: $TMPDIR/foo/path2/baz
 $TMPDIR/foo/path2: close: $TMPDIR/foo/path2/baz

pkg/cmd/roachtest/tests/BUILD.bazel

@@ -281,7 +281,7 @@ go_library(
         "//pkg/sql/sem/tree",
         "//pkg/sql/ttl/ttlbase",
         "//pkg/storage/enginepb",
-        "//pkg/storage/minversion",
+        "//pkg/storage/fs",
         "//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/minversion"
+	"github.com/cockroachdb/cockroach/pkg/storage/fs"
 	"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", minversion.MinVersionFilename), "{store-dir}/"+name)
+	c.Run(ctx, option.WithNodes(c.All()), "cp", fmt.Sprintf("{store-dir}/%s", fs.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/storage/fs/BUILD.bazel

@@ -7,6 +7,7 @@ go_library(
         "encryption_at_rest.go",
         "file_registry.go",
         "fs.go",
+        "min_version.go",
         "sticky_vfs.go",
         "temp_dir.go",
         "test_utils.go",
@@ -22,7 +23,6 @@ go_library(
         "//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

@@ -20,7 +20,6 @@ import (
 	"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"
@@ -204,6 +203,54 @@ func InitEnv(
 		return nil, err
 	}
 
+	// Read the current store cluster version.
+	storeClusterVersion, minVerFileExists, err := 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 when min-version file exists")
+		}
+		// 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.
@@ -241,6 +288,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

pkg/storage/fs/min_version.go

@@ -3,7 +3,7 @@
 // Use of this software is governed by the CockroachDB Software License
 // included in the /LICENSE file.
 
-package minversion
+package fs
 
 import (
 	"io"

pkg/storage/min_version.go

@@ -8,8 +8,7 @@ package storage
 import (
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
 	"github.com/cockroachdb/cockroach/pkg/storage/fs"
-	"github.com/cockroachdb/cockroach/pkg/storage/minversion"
-	"github.com/cockroachdb/cockroach/pkg/util/protoutil"
+		"github.com/cockroachdb/cockroach/pkg/util/protoutil"
 	"github.com/cockroachdb/errors"
 	"github.com/cockroachdb/pebble/vfs"
 )
@@ -22,7 +21,7 @@ func writeMinVersionFile(atomicRenameFS vfs.FS, dir string, version roachpb.Vers
 	if version == (roachpb.Version{}) {
 		return errors.New("min version should not be empty")
 	}
-	ok, err := minversion.MinVersionIsAtLeastTargetVersion(atomicRenameFS, dir, version)
+	ok, err := fs.MinVersionIsAtLeastTargetVersion(atomicRenameFS, dir, version)
 	if err != nil {
 		return err
 	}
@@ -33,7 +32,7 @@ func writeMinVersionFile(atomicRenameFS vfs.FS, dir string, version roachpb.Vers
 	if err != nil {
 		return err
 	}
-	filename := atomicRenameFS.PathJoin(dir, minversion.MinVersionFilename)
+	filename := atomicRenameFS.PathJoin(dir, fs.MinVersionFilename)
 	if err := fs.SafeWriteToUnencryptedFile(atomicRenameFS, dir, filename, b, fs.UnspecifiedWriteCategory); err != nil {
 		return err
 	}

pkg/storage/min_version_test.go

@@ -14,7 +14,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
 	"github.com/cockroachdb/cockroach/pkg/settings/cluster"
 	"github.com/cockroachdb/cockroach/pkg/storage/fs"
-	"github.com/cockroachdb/cockroach/pkg/storage/minversion"
 	"github.com/cockroachdb/cockroach/pkg/storage/storageconfig"
 	"github.com/cockroachdb/cockroach/pkg/util/leaktest"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
@@ -34,56 +33,56 @@ func TestMinVersion(t *testing.T) {
 	require.NoError(t, mem.MkdirAll(dir, os.ModeDir))
 
 	// Expect !ok min version file doesn't exist.
-	v, ok, err := minversion.GetMinVersion(mem, dir)
+	v, ok, err := fs.GetMinVersion(mem, dir)
 	require.NoError(t, err)
 	require.Equal(t, roachpb.Version{}, v)
 	require.False(t, ok)
 
 	// Expect min version to not be at least any target version.
-	ok, err = minversion.MinVersionIsAtLeastTargetVersion(mem, dir, version1)
+	ok, err = fs.MinVersionIsAtLeastTargetVersion(mem, dir, version1)
 	require.NoError(t, err)
 	require.False(t, ok)
-	ok, err = minversion.MinVersionIsAtLeastTargetVersion(mem, dir, version2)
+	ok, err = fs.MinVersionIsAtLeastTargetVersion(mem, dir, version2)
 	require.NoError(t, err)
 	require.False(t, ok)
 
 	// Expect no error when updating min version if no file currently exists.
 	require.NoError(t, writeMinVersionFile(mem, dir, version1))
 
 	// Expect min version to be version1.
-	v, ok, err = minversion.GetMinVersion(mem, dir)
+	v, ok, err = fs.GetMinVersion(mem, dir)
 	require.NoError(t, err)
 	require.True(t, ok)
 	require.True(t, version1.Equal(v))
 
 	// Expect min version to be at least version1 but not version2.
-	ok, err = minversion.MinVersionIsAtLeastTargetVersion(mem, dir, version1)
+	ok, err = fs.MinVersionIsAtLeastTargetVersion(mem, dir, version1)
 	require.NoError(t, err)
 	require.True(t, ok)
-	ok, err = minversion.MinVersionIsAtLeastTargetVersion(mem, dir, version2)
+	ok, err = fs.MinVersionIsAtLeastTargetVersion(mem, dir, version2)
 	require.NoError(t, err)
 	require.False(t, ok)
 
 	// Expect no error when updating min version to a higher version.
 	require.NoError(t, writeMinVersionFile(mem, dir, version2))
 
 	// Expect min version to be at least version1 and version2.
-	ok, err = minversion.MinVersionIsAtLeastTargetVersion(mem, dir, version1)
+	ok, err = fs.MinVersionIsAtLeastTargetVersion(mem, dir, version1)
 	require.NoError(t, err)
 	require.True(t, ok)
-	ok, err = minversion.MinVersionIsAtLeastTargetVersion(mem, dir, version2)
+	ok, err = fs.MinVersionIsAtLeastTargetVersion(mem, dir, version2)
 	require.NoError(t, err)
 	require.True(t, ok)
 
 	// Expect min version to be version2.
-	v, ok, err = minversion.GetMinVersion(mem, dir)
+	v, ok, err = fs.GetMinVersion(mem, dir)
 	require.NoError(t, err)
 	require.True(t, ok)
 	require.True(t, version2.Equal(v))
 
 	// Expect no-op when trying to update min version to a lower version.
 	require.NoError(t, writeMinVersionFile(mem, dir, version1))
-	v, ok, err = minversion.GetMinVersion(mem, dir)
+	v, ok, err = fs.GetMinVersion(mem, dir)
 	require.NoError(t, err)
 	require.True(t, ok)
 	require.True(t, version2.Equal(v))
@@ -138,7 +137,7 @@ func TestMinVersion_IsNotEncrypted(t *testing.T) {
 
 	// Reading the file directly through the unencrypted MemFS should
 	// succeed and yield the correct version.
-	v, ok, err := minversion.GetMinVersion(env.UnencryptedFS, "")
+	v, ok, err := fs.GetMinVersion(env.UnencryptedFS, "")
 	require.NoError(t, err)
 	require.True(t, ok)
 	require.Equal(t, st.Version.LatestVersion(), v)

pkg/storage/pebble.go

@@ -32,7 +32,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/storage/disk"
 	"github.com/cockroachdb/cockroach/pkg/storage/enginepb"
 	"github.com/cockroachdb/cockroach/pkg/storage/fs"
-	"github.com/cockroachdb/cockroach/pkg/storage/minversion"
 	"github.com/cockroachdb/cockroach/pkg/storage/mvccencoding"
 	"github.com/cockroachdb/cockroach/pkg/storage/pebbleiter"
 	"github.com/cockroachdb/cockroach/pkg/util/buildutil"
@@ -1076,54 +1075,25 @@ func newPebble(ctx context.Context, cfg engineConfig) (p *Pebble, err error) {
 			cfg.opts.Experimental.RemoteStorage = remoteStorageAdaptor{p: p, ctx: logCtx, factory: cfg.remoteStorageFactory}
 		}
 	}
+	// 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{})
 
-	// 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 !initFromMinVersionFile && (cfg.opts.ErrorIfNotExists || cfg.opts.ReadOnly) {
+		filename := p.cfg.env.UnencryptedFS.PathJoin(cfg.env.Dir, fs.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)
@@ -1132,10 +1102,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")
@@ -1144,7 +1114,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.