From ff97cb2881c46766aa8fd93360a13d3dc1b21111 Mon Sep 17 00:00:00 2001 From: Radu Berinde Date: Fri, 3 Feb 2023 11:46:38 -0800 Subject: [PATCH 1/2] storage: minor cleanup around MinVersionIsAtLeastTargetVersion Removing this unused method from the Engine interface. Release note: None --- pkg/storage/engine.go | 4 ---- pkg/storage/min_version_test.go | 4 ++-- pkg/storage/pebble.go | 5 ----- 3 files changed, 2 insertions(+), 11 deletions(-) diff --git a/pkg/storage/engine.go b/pkg/storage/engine.go index 309e046fb87c..5cbb8001195d 100644 --- a/pkg/storage/engine.go +++ b/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 diff --git a/pkg/storage/min_version_test.go b/pkg/storage/min_version_test.go index de903f6dd712..d22f26facb40 100644 --- a/pkg/storage/min_version_test.go +++ b/pkg/storage/min_version_test.go @@ -128,13 +128,13 @@ 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) diff --git a/pkg/storage/pebble.go b/pkg/storage/pebble.go index e77c88412fc1..ea81b3eac794 100644 --- a/pkg/storage/pebble.go +++ b/pkg/storage/pebble.go @@ -1931,11 +1931,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 From 39c7443f0a5d056e6cae508c4ee1edcbd00232dc Mon Sep 17 00:00:00 2001 From: Radu Berinde Date: Wed, 1 Feb 2023 14:53:30 -0800 Subject: [PATCH 2/2] 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 --- pkg/kv/kvserver/kvstorage/cluster_version.go | 14 ++------- pkg/storage/min_version.go | 20 ++++++------- pkg/storage/min_version_test.go | 19 ++++++++----- pkg/storage/pebble.go | 30 ++++++++++++++++++-- pkg/storage/pebble_test.go | 20 +++++++++++++ 5 files changed, 73 insertions(+), 30 deletions(-) diff --git a/pkg/kv/kvserver/kvstorage/cluster_version.go b/pkg/kv/kvserver/kvstorage/cluster_version.go index 6aa10e9e46d7..bf8bf9df0e9e 100644 --- a/pkg/kv/kvserver/kvstorage/cluster_version.go +++ b/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)", diff --git a/pkg/storage/min_version.go b/pkg/storage/min_version.go index 994760363c76..a91229d94496 100644 --- a/pkg/storage/min_version.go +++ b/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 } diff --git a/pkg/storage/min_version_test.go b/pkg/storage/min_version_test.go index d22f26facb40..5d566213068e 100644 --- a/pkg/storage/min_version_test.go +++ b/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)) } @@ -140,8 +144,9 @@ func TestMinVersion_IsNotEncrypted(t *testing.T) { // 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) } diff --git a/pkg/storage/pebble.go b/pkg/storage/pebble.go index ea81b3eac794..e555d148fea3 100644 --- a/pkg/storage/pebble.go +++ b/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 diff --git a/pkg/storage/pebble_test.go b/pkg/storage/pebble_test.go index c806ab0bb7a4..5cba54859c40 100644 --- a/pkg/storage/pebble_test.go +++ b/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") +}