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,6 +281,7 @@ go_library(
         "//pkg/sql/sem/tree",
         "//pkg/sql/ttl/ttlbase",
         "//pkg/storage/enginepb",
+        "//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/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/pebble.go

@@ -1075,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}
 		}
 	}
-
-	// Read the current store cluster version.
-	storeClusterVersion, minVerFileExists, err := fs.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, fs.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, 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)
@@ -1131,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")
@@ -1143,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.

pkg/storage/pebble_test.go

@@ -1307,16 +1307,45 @@ func TestIncompatibleVersion(t *testing.T) {
 	require.NoError(t, err)
 	require.NoError(t, fs.SafeWriteToUnencryptedFile(memFS, "", fs.MinVersionFilename, b, fs.UnspecifiedWriteCategory))
 
-	env = mustInitTestEnv(t, memFS, "")
-	_, err = Open(ctx, env, cluster.MakeTestingClusterSettings())
-	require.Error(t, err)
-	_, err = Open(ctx, env, cluster.MakeTestingClusterSettings())
+	settings := cluster.MakeTestingClusterSettings()
+	_, err = fs.InitEnv(context.Background(), memFS, "", fs.EnvConfig{
+		Version: settings.Version,
+	}, nil /* statsCollector */)
 	msg := err.Error()
 	if !strings.Contains(msg, "is too old for running version") &&
 		!strings.Contains(msg, "cannot be opened by development version") {
 		t.Fatalf("unexpected error %v", err)
 	}
-	env.Close()
+}
+
+func TestPebbleClusterVersionTooNew(t *testing.T) {
+	defer leaktest.AfterTest(t)()
+	defer log.Scope(t).Close(t)
+	ctx := context.Background()
+
+	memFS := vfs.NewMem()
+	env := mustInitTestEnv(t, memFS, "")
+
+	p, err := Open(ctx, env, cluster.MakeTestingClusterSettings())
+	require.NoError(t, err)
+	p.Close()
+
+	// Overwrite the min version file with a future version that's newer than the
+	// latest supported version. Use a development version higher than the current
+	// running version to ensure it will be considered "too new". We use a very
+	// high development version to avoid conflicts with the current version.
+	ver := roachpb.Version{Major: 1000030, Minor: 0}
+	b, err := protoutil.Marshal(&ver)
+	require.NoError(t, err)
+	require.NoError(t, fs.SafeWriteToUnencryptedFile(memFS, "", fs.MinVersionFilename, b, fs.UnspecifiedWriteCategory))
+
+	settings := cluster.MakeTestingClusterSettings()
+	_, err = fs.InitEnv(context.Background(), memFS, "", fs.EnvConfig{
+		Version: settings.Version,
+	}, nil /* statsCollector */)
+	require.Error(t, err)
+	msg := err.Error()
+	require.Contains(t, msg, "is too high for running version")
 }
 
 func TestNoMinVerFile(t *testing.T) {