Merge #96394

96394: storage: check for old store version upfront r=RaduBerinde a=RaduBerinde

#### storage: minor cleanup around MinVersionIsAtLeastTargetVersion

Removing this unused method from the Engine interface.

Release note: None

#### storage: check for old store version upfront

This change adds a check for old versions as soon as we open the min
version file, before opening the store. We currently check this at a
later time; in some cases, it is too late and there is potential for
corruption.

Release note: None

Fixes #89836

Co-authored-by: Radu Berinde <radu@cockroachlabs.com>

pkg/kv/kvserver/kvstorage/cluster_version.go

@@ -168,17 +168,9 @@ func SynthesizeClusterVersionFromEngines(
 	}
 	log.Eventf(ctx, "read clusterVersion %+v", cv)
 
-	// Avoid running a binary too new for this store. This is what you'd catch
-	// if, say, you restarted directly from 1.0 into 1.2 (bumping the min
-	// version) without going through 1.1 first. It would also be what you catch if
-	// you are starting 1.1 for the first time (after 1.0), but it crashes
-	// half-way through the startup sequence (so now some stores have 1.1, but
-	// some 1.0), in which case you are expected to run 1.1 again (hopefully
-	// without the crash this time) which would then rewrite all the stores.
-	//
-	// We only verify this now because as we iterate through the stores, we
-	// may not yet have picked up the final versions we're actually planning
-	// to use.
+	// We now check for old versions up front when we open the database. We leave
+	// this older check for the case where a store is so old that it doesn't have
+	// a min version file.
 	if minStoreVersion.Version.Less(binaryMinSupportedVersion) {
 		return clusterversion.ClusterVersion{}, errors.Errorf("store %s, last used with cockroach version v%s, "+
 			"is too old for running version v%s (which requires data from v%s or later)",

pkg/storage/engine.go

@@ -958,10 +958,6 @@ type Engine interface {
 	// version that it must maintain compatibility with.
 	SetMinVersion(version roachpb.Version) error
 
-	// MinVersionIsAtLeastTargetVersion returns whether the engine's recorded
-	// storage min version is at least the target version.
-	MinVersionIsAtLeastTargetVersion(target roachpb.Version) (bool, error)
-
 	// SetCompactionConcurrency is used to set the engine's compaction
 	// concurrency. It returns the previous compaction concurrency.
 	SetCompactionConcurrency(n uint64) uint64

pkg/storage/min_version.go

@@ -63,38 +63,38 @@ func MinVersionIsAtLeastTargetVersion(
 	if target == (roachpb.Version{}) {
 		return false, errors.New("target version should not be empty")
 	}
-	minVersion, err := getMinVersion(atomicRenameFS, dir)
+	minVersion, ok, err := getMinVersion(atomicRenameFS, dir)
 	if err != nil {
 		return false, err
 	}
-	if minVersion == (roachpb.Version{}) {
+	if !ok {
 		return false, nil
 	}
 	return !minVersion.Less(target), nil
 }
 
-// getMinVersion returns the min version recorded on disk if the min version
-// file exists and nil otherwise.
-func getMinVersion(atomicRenameFS vfs.FS, dir string) (roachpb.Version, error) {
+// getMinVersion returns the min version recorded on disk. If the min version
+// file doesn't exist, returns ok=false.
+func getMinVersion(atomicRenameFS vfs.FS, dir string) (_ roachpb.Version, ok bool, _ error) {
 	// TODO(jackson): Assert that atomicRenameFS supports atomic renames
 	// once Pebble is bumped to the appropriate SHA.
 
 	filename := atomicRenameFS.PathJoin(dir, MinVersionFilename)
 	f, err := atomicRenameFS.Open(filename)
 	if oserror.IsNotExist(err) {
-		return roachpb.Version{}, nil
+		return roachpb.Version{}, false, nil
 	}
 	if err != nil {
-		return roachpb.Version{}, err
+		return roachpb.Version{}, false, err
 	}
 	defer f.Close()
 	b, err := io.ReadAll(f)
 	if err != nil {
-		return roachpb.Version{}, err
+		return roachpb.Version{}, false, err
 	}
 	version := roachpb.Version{}
 	if err := protoutil.Unmarshal(b, &version); err != nil {
-		return version, err
+		return roachpb.Version{}, false, err
 	}
-	return version, nil
+	return version, true, nil
 }

pkg/storage/min_version_test.go

@@ -36,13 +36,14 @@ func TestMinVersion(t *testing.T) {
 	dir := "/foo"
 	require.NoError(t, mem.MkdirAll(dir, os.ModeDir))
 
-	// Expect zero value version when min version file doesn't exist.
-	v, err := getMinVersion(mem, dir)
+	// Expect !ok min version file doesn't exist.
+	v, ok, err := 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 := MinVersionIsAtLeastTargetVersion(mem, dir, version1)
+	ok, err = MinVersionIsAtLeastTargetVersion(mem, dir, version1)
 	require.NoError(t, err)
 	require.False(t, ok)
 	ok, err = MinVersionIsAtLeastTargetVersion(mem, dir, version2)
@@ -53,8 +54,9 @@ func TestMinVersion(t *testing.T) {
 	require.NoError(t, writeMinVersionFile(mem, dir, version1))
 
 	// Expect min version to be version1.
-	v, err = getMinVersion(mem, dir)
+	v, ok, err = 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.
@@ -77,14 +79,16 @@ func TestMinVersion(t *testing.T) {
 	require.True(t, ok)
 
 	// Expect min version to be version2.
-	v, err = getMinVersion(mem, dir)
+	v, ok, err = 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, err = getMinVersion(mem, dir)
+	v, ok, err = getMinVersion(mem, dir)
 	require.NoError(t, err)
+	require.True(t, ok)
 	require.True(t, version2.Equal(v))
 }
 
@@ -128,20 +132,21 @@ func TestMinVersion_IsNotEncrypted(t *testing.T) {
 	v1 := roachpb.Version{Major: 21, Minor: 1, Patch: 0, Internal: 122}
 	v2 := roachpb.Version{Major: 21, Minor: 1, Patch: 0, Internal: 126}
 
-	ok, err := p.MinVersionIsAtLeastTargetVersion(v1)
+	ok, err := MinVersionIsAtLeastTargetVersion(p.unencryptedFS, p.path, v1)
 	require.NoError(t, err)
 	require.False(t, ok)
 
 	require.NoError(t, p.SetMinVersion(v2))
 
-	ok, err = p.MinVersionIsAtLeastTargetVersion(v1)
+	ok, err = MinVersionIsAtLeastTargetVersion(p.unencryptedFS, p.path, v1)
 	require.NoError(t, err)
 	require.True(t, ok)
 
 	// Reading the file directly through the unencrypted MemFS should
 	// succeed and yield the correct version.
-	v, err := getMinVersion(fs, "")
+	v, ok, err := getMinVersion(fs, "")
 	require.NoError(t, err)
+	require.True(t, ok)
 	require.Equal(t, v2, v)
 }
 

pkg/storage/pebble.go

@@ -1018,10 +1018,36 @@ func NewPebble(ctx context.Context, cfg PebbleConfig) (p *Pebble, err error) {
 	p.wrappedIntentWriter = wrapIntentWriter(p)
 
 	// Read the current store cluster version.
-	storeClusterVersion, err := getMinVersion(unencryptedFS, cfg.Dir)
+	storeClusterVersion, minVerFileExists, err := getMinVersion(unencryptedFS, cfg.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 the version file does not exist, we will fail a similar check later,
+		// when we set the min version on all the stores.
+		//
+		// TODO(radu): investigate always requiring the existence of the min version
+		// file (unless we are creating a new store). Note that checkpoints don't
+		// have the min version file and some tests expect to be able to open
+		// checkpoints.
+		if v := cfg.Settings.Version; storeClusterVersion.Less(v.BinaryMinSupportedVersion()) {
+			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.BinaryVersion(), v.BinaryMinSupportedVersion(),
+			)
+		}
+	}
 
 	if WorkloadCollectorEnabled {
 		p.replayer.Attach(cfg.Opts)
@@ -1033,7 +1059,7 @@ func NewPebble(ctx context.Context, cfg PebbleConfig) (p *Pebble, err error) {
 	}
 	p.db = db
 
-	if storeClusterVersion != (roachpb.Version{}) {
+	if minVerFileExists {
 		// The storage engine performs its own internal migrations
 		// through the setting of the store cluster version. When
 		// storage's min version is set, SetMinVersion writes to disk to
@@ -1931,11 +1957,6 @@ func (p *Pebble) SetMinVersion(version roachpb.Version) error {
 	return nil
 }
 
-// MinVersionIsAtLeastTargetVersion implements the Engine interface.
-func (p *Pebble) MinVersionIsAtLeastTargetVersion(target roachpb.Version) (bool, error) {
-	return MinVersionIsAtLeastTargetVersion(p.unencryptedFS, p.path, target)
-}
-
 // BufferedSize implements the Engine interface.
 func (p *Pebble) BufferedSize() int {
 	return 0

pkg/storage/pebble_test.go

@@ -1277,3 +1277,23 @@ func TestShortAttributeExtractor(t *testing.T) {
 		})
 	}
 }
+
+func TestIncompatibleVersion(t *testing.T) {
+	defer leaktest.AfterTest(t)()
+	defer log.Scope(t).Close(t)
+	ctx := context.Background()
+
+	loc := Location{
+		dir: "",
+		fs:  vfs.NewMem(),
+	}
+	oldVer := roachpb.Version{Major: 21, Minor: 1}
+	stOld := cluster.MakeTestingClusterSettingsWithVersions(oldVer, oldVer, true /* initializeVersion */)
+	p, err := Open(ctx, loc, stOld)
+	require.NoError(t, err)
+	p.Close()
+
+	stNew := cluster.MakeTestingClusterSettings()
+	_, err = Open(ctx, loc, stNew)
+	require.ErrorContains(t, err, "is too old for running version")
+}