Merge #152358

152358: storage: fix pebble benchmarks r=xinhaoz a=xinhaoz

Some benchmarks copy over initial fs state, which includes the min version file,
 after fs/env initialization. Make sure to set and validate the store cluster
version after the copy.

Epic: none

Release note: None

Co-authored-by: Xin Hao Zhang <[email protected]>

Author: craig[bot]

pkg/storage/bench_data_test.go

@@ -107,6 +107,11 @@ func getInitialStateEngine(
 	require.NoError(b, err)
 	require.True(b, ok)
 
+	// Validate and set the minversion for the env after copy.
+	settings := cluster.MakeClusterSettings()
+	env.StoreClusterVersion, err = fs.ValidateMinVersionFile(env.UnencryptedFS, env.Dir, settings.Version)
+	require.NoError(b, err)
+
 	if !inMemory {
 		// Load all the files into the OS buffer cache for better determinism.
 		testutils.ReadAllFiles(filepath.Join(env.Dir, "*"))

pkg/storage/fs/fs.go

@@ -203,54 +203,11 @@ func InitEnv(
 		return nil, err
 	}
 
-	// Read the current store cluster version.
-	storeClusterVersion, minVerFileExists, err := GetMinVersion(e.UnencryptedFS, e.Dir)
+	e.StoreClusterVersion, err = ValidateMinVersionFile(e.UnencryptedFS, e.Dir, cfg.Version)
 	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.

pkg/storage/fs/min_version.go

@@ -8,6 +8,7 @@ package fs
 import (
 	"io"
 
+	"github.com/cockroachdb/cockroach/pkg/clusterversion"
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
 	"github.com/cockroachdb/cockroach/pkg/util/protoutil"
 	"github.com/cockroachdb/errors"
@@ -66,3 +67,57 @@ func GetMinVersion(atomicRenameFS vfs.FS, dir string) (_ roachpb.Version, ok boo
 	}
 	return version, true, nil
 }
+
+// ValidateMinVersionFile validates the store cluster version found
+// in the min-version file in against the provided cluster version.
+func ValidateMinVersionFile(
+	fs vfs.FS, dir string, clusterVersion clusterversion.Handle,
+) (roachpb.Version, error) {
+	// Read the current store cluster version.
+	storeClusterVersion, minVerFileExists, err := GetMinVersion(fs, dir)
+	if !minVerFileExists || err != nil {
+		return storeClusterVersion, err
+	}
+
+	v := clusterVersion
+	if v == nil {
+		return storeClusterVersion, errors.New("cluster version must be provided if 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 storeClusterVersion, errors.Errorf(
+				"store last used with cockroach non-development version v%s "+
+					"cannot be opened by development version v%s",
+				storeClusterVersion, v.LatestVersion(),
+			)
+		}
+		return storeClusterVersion, 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 storeClusterVersion, errors.Errorf(
+			"store last used with cockroach version v%s is too high for running "+
+				"version v%s",
+			storeClusterVersion, v.LatestVersion(),
+		)
+	}
+
+	return storeClusterVersion, nil
+}

pkg/storage/pebble.go

@@ -1080,20 +1080,23 @@ func newPebble(ctx context.Context, cfg engineConfig) (p *Pebble, err error) {
 	// 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,
-		)
+	if !initFromMinVersionFile {
+		if 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,
+			)
+		}
+		// 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
+	} else {
+		cfg.opts.ErrorIfNotExists = true
 	}
-	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)