From d1a192480ecce71d40c1c12921d17ba37353e5f9 Mon Sep 17 00:00:00 2001 From: Radu Berinde Date: Fri, 17 Feb 2023 11:56:15 -0800 Subject: [PATCH] storage: deprecate cluster version key This change deprecates the cluster version key. We have the version outside the store (in the min version file) so we no longer need to store it inside a KV. To keep backward compatibility with 22.2, we still write the key but we never read it. Once we stop supporting 22.2, we can stop writing it altogether. This required updating the `TestClusterVersionWriteSynthesize` to use versions that work with the binary. We also clean up the test and move it to `kvstorage` where it belongs. Release note: None Epic: none --- pkg/keys/doc.go | 16 +-- pkg/keys/keys.go | 7 +- pkg/keys/keys_test.go | 2 +- pkg/keys/printer_test.go | 2 +- pkg/kv/kvserver/kvstorage/BUILD.bazel | 2 + pkg/kv/kvserver/kvstorage/cluster_version.go | 63 ++------ .../kvstorage/cluster_version_test.go | 135 ++++++++++++++++++ pkg/kv/kvserver/kvstorage/init.go | 2 +- pkg/kv/kvserver/stores.go | 11 -- pkg/kv/kvserver/stores_test.go | 135 ------------------ pkg/server/init.go | 2 +- pkg/server/node.go | 17 +-- pkg/server/server_test.go | 2 +- pkg/server/version_cluster_test.go | 15 +- pkg/storage/engine.go | 6 + pkg/storage/pebble.go | 11 ++ 16 files changed, 197 insertions(+), 231 deletions(-) diff --git a/pkg/keys/doc.go b/pkg/keys/doc.go index 4c75ccb6128d..fe42458b864e 100644 --- a/pkg/keys/doc.go +++ b/pkg/keys/doc.go @@ -211,14 +211,14 @@ var _ = [...]interface{}{ // 4. Store local keys: These contain metadata about an individual store. // They are unreplicated and unaddressable. The typical example is the // store 'ident' record. They all share `localStorePrefix`. - StoreClusterVersionKey, // "cver" - StoreGossipKey, // "goss" - StoreHLCUpperBoundKey, // "hlcu" - StoreIdentKey, // "iden" - StoreUnsafeReplicaRecoveryKey, // "loqr" - StoreNodeTombstoneKey, // "ntmb" - StoreCachedSettingsKey, // "stng" - StoreLastUpKey, // "uptm" + DeprecatedStoreClusterVersionKey, // "cver" + StoreGossipKey, // "goss" + StoreHLCUpperBoundKey, // "hlcu" + StoreIdentKey, // "iden" + StoreUnsafeReplicaRecoveryKey, // "loqr" + StoreNodeTombstoneKey, // "ntmb" + StoreCachedSettingsKey, // "stng" + StoreLastUpKey, // "uptm" // 5. Range lock keys for all replicated locks. All range locks share // LocalRangeLockTablePrefix. Locks can be acquired on global keys and on diff --git a/pkg/keys/keys.go b/pkg/keys/keys.go index ee7b14fcc76c..6a31638b454e 100644 --- a/pkg/keys/keys.go +++ b/pkg/keys/keys.go @@ -61,8 +61,11 @@ func StoreGossipKey() roachpb.Key { return MakeStoreKey(localStoreGossipSuffix, nil) } -// StoreClusterVersionKey returns a store-local key for the cluster version. -func StoreClusterVersionKey() roachpb.Key { +// DeprecatedStoreClusterVersionKey returns a store-local key for the cluster version. +// +// We no longer use this key, but still write it out for interoperability with +// older versions. +func DeprecatedStoreClusterVersionKey() roachpb.Key { return MakeStoreKey(localStoreClusterVersionSuffix, nil) } diff --git a/pkg/keys/keys_test.go b/pkg/keys/keys_test.go index 37dcd51a4ae1..1bbf9fd79f3c 100644 --- a/pkg/keys/keys_test.go +++ b/pkg/keys/keys_test.go @@ -33,7 +33,7 @@ func TestStoreKeyEncodeDecode(t *testing.T) { }{ {key: StoreIdentKey(), expSuffix: localStoreIdentSuffix, expDetail: nil}, {key: StoreGossipKey(), expSuffix: localStoreGossipSuffix, expDetail: nil}, - {key: StoreClusterVersionKey(), expSuffix: localStoreClusterVersionSuffix, expDetail: nil}, + {key: DeprecatedStoreClusterVersionKey(), expSuffix: localStoreClusterVersionSuffix, expDetail: nil}, {key: StoreLastUpKey(), expSuffix: localStoreLastUpSuffix, expDetail: nil}, {key: StoreHLCUpperBoundKey(), expSuffix: localStoreHLCUpperBoundSuffix, expDetail: nil}, } diff --git a/pkg/keys/printer_test.go b/pkg/keys/printer_test.go index 9f6b46e82ced..4b7ecd0c258f 100644 --- a/pkg/keys/printer_test.go +++ b/pkg/keys/printer_test.go @@ -233,7 +233,7 @@ func TestPrettyPrint(t *testing.T) { // local {keys.StoreIdentKey(), "/Local/Store/storeIdent", revertSupportUnknown}, {keys.StoreGossipKey(), "/Local/Store/gossipBootstrap", revertSupportUnknown}, - {keys.StoreClusterVersionKey(), "/Local/Store/clusterVersion", revertSupportUnknown}, + {keys.DeprecatedStoreClusterVersionKey(), "/Local/Store/clusterVersion", revertSupportUnknown}, {keys.StoreNodeTombstoneKey(123), "/Local/Store/nodeTombstone/n123", revertSupportUnknown}, {keys.StoreCachedSettingsKey(roachpb.Key("a")), `/Local/Store/cachedSettings/"a"`, revertSupportUnknown}, {keys.StoreUnsafeReplicaRecoveryKey(loqRecoveryID), fmt.Sprintf(`/Local/Store/lossOfQuorumRecovery/applied/%s`, loqRecoveryID), revertSupportUnknown}, diff --git a/pkg/kv/kvserver/kvstorage/BUILD.bazel b/pkg/kv/kvserver/kvstorage/BUILD.bazel index cdde4176d5a9..4e971abe6ddf 100644 --- a/pkg/kv/kvserver/kvstorage/BUILD.bazel +++ b/pkg/kv/kvserver/kvstorage/BUILD.bazel @@ -45,12 +45,14 @@ go_test( "//pkg/keys", "//pkg/kv/kvserver/logstore", "//pkg/roachpb", + "//pkg/settings/cluster", "//pkg/storage", "//pkg/testutils", "//pkg/testutils/datapathutils", "//pkg/util/hlc", "//pkg/util/leaktest", "//pkg/util/log", + "//pkg/util/stop", "//pkg/util/tracing", "//pkg/util/uuid", "@com_github_cockroachdb_datadriven//:datadriven", diff --git a/pkg/kv/kvserver/kvstorage/cluster_version.go b/pkg/kv/kvserver/kvstorage/cluster_version.go index 5123a012ac8b..e098c3f9f491 100644 --- a/pkg/kv/kvserver/kvstorage/cluster_version.go +++ b/pkg/kv/kvserver/kvstorage/cluster_version.go @@ -24,17 +24,21 @@ import ( "github.com/cockroachdb/errors" ) -// WriteClusterVersion writes the given cluster version to the store-local -// cluster version key. We only accept a raw engine to ensure we're persisting -// the write durably. +// WriteClusterVersion writes the given cluster version to the min version file +// and to the store-local cluster version key. We only accept a raw engine to +// ensure we're persisting the write durably. func WriteClusterVersion( ctx context.Context, eng storage.Engine, cv clusterversion.ClusterVersion, ) error { + // We no longer read this key, but v22.2 still does. We continue writing the key + // for interoperability. + // TODO(radu): Remove this when we are no longer compatible with 22.2 (keeping + // just the SetMinVersion call). err := storage.MVCCPutProto( ctx, eng, nil, - keys.StoreClusterVersionKey(), + keys.DeprecatedStoreClusterVersionKey(), hlc.Timestamp{}, hlc.ClockTimestamp{}, nil, @@ -43,38 +47,9 @@ func WriteClusterVersion( if err != nil { return err } - - // The storage engine sometimes must make backwards incompatible - // changes. However, the store cluster version key is a key stored - // within the storage engine, so it's unavailable when the store is - // opened. - // - // The storage engine maintains its own minimum version on disk that - // it may consult it before opening the Engine. This version is - // stored in a separate file on the filesystem. For now, write to - // this file in combination with the store cluster version key. - // - // This parallel version state is a bit of a wart and an eventual - // goal is to replace the store cluster version key with the storage - // engine's flat file. This requires that there are no writes to the - // engine until either bootstrapping or joining an existing cluster. - // Writing the version to this file would happen before opening the - // engine for completing the rest of bootstrapping/joining the - // cluster. return eng.SetMinVersion(cv.Version) } -// ReadClusterVersion reads the cluster version from the store-local version -// key. Returns an empty version if the key is not found. -func ReadClusterVersion( - ctx context.Context, reader storage.Reader, -) (clusterversion.ClusterVersion, error) { - var cv clusterversion.ClusterVersion - _, err := storage.MVCCGetProto(ctx, reader, keys.StoreClusterVersionKey(), hlc.Timestamp{}, - &cv, storage.MVCCGetOptions{}) - return cv, err -} - // WriteClusterVersionToEngines writes the given version to the given engines, // Returns nil on success; otherwise returns first error encountered writing to // the stores. It makes no attempt to validate the supplied version. @@ -128,30 +103,22 @@ func SynthesizeClusterVersionFromEngines( // constraints, which at the latest the second loop will achieve (because // then minStoreVersion don't change any more). for _, eng := range engines { - eng := eng.(storage.Reader) // we're read only - var cv clusterversion.ClusterVersion - cv, err := ReadClusterVersion(ctx, eng) - if err != nil { - return clusterversion.ClusterVersion{}, err - } - if cv.Version == (roachpb.Version{}) { - // This is needed when a node first joins an existing cluster, in - // which case it won't know what version to use until the first - // Gossip update comes in. - cv.Version = binaryMinSupportedVersion + engVer := eng.MinVersion() + if engVer == (roachpb.Version{}) { + return clusterversion.ClusterVersion{}, errors.AssertionFailedf("store %s has no version", eng) } // Avoid running a binary with a store that is too new. For example, // restarting into 1.1 after having upgraded to 1.2 doesn't work. - if binaryVersion.Less(cv.Version) { + if binaryVersion.Less(engVer) { return clusterversion.ClusterVersion{}, errors.Errorf( "cockroach version v%s is incompatible with data in store %s; use version v%s or later", - binaryVersion, eng, cv.Version) + binaryVersion, eng, engVer) } // Track smallest use version encountered. - if cv.Version.Less(minStoreVersion.Version) { - minStoreVersion.Version = cv.Version + if engVer.Less(minStoreVersion.Version) { + minStoreVersion.Version = engVer minStoreVersion.origin = fmt.Sprint(eng) } } diff --git a/pkg/kv/kvserver/kvstorage/cluster_version_test.go b/pkg/kv/kvserver/kvstorage/cluster_version_test.go index 8340c98e5d34..57db56c4d59d 100644 --- a/pkg/kv/kvserver/kvstorage/cluster_version_test.go +++ b/pkg/kv/kvserver/kvstorage/cluster_version_test.go @@ -12,14 +12,17 @@ package kvstorage import ( "context" + "reflect" "testing" "github.com/cockroachdb/cockroach/pkg/clusterversion" "github.com/cockroachdb/cockroach/pkg/roachpb" + "github.com/cockroachdb/cockroach/pkg/settings/cluster" "github.com/cockroachdb/cockroach/pkg/storage" "github.com/cockroachdb/cockroach/pkg/testutils" "github.com/cockroachdb/cockroach/pkg/util/leaktest" "github.com/cockroachdb/cockroach/pkg/util/log" + "github.com/cockroachdb/cockroach/pkg/util/stop" ) // TestStoresClusterVersionIncompatible verifies an error occurs when @@ -80,3 +83,135 @@ func TestStoresClusterVersionIncompatible(t *testing.T) { }) } } + +// TestStoresClusterVersionWriteSynthesize verifies that the cluster version is +// written to all stores and that missing versions are filled in appropriately. +func TestClusterVersionWriteSynthesize(t *testing.T) { + defer leaktest.AfterTest(t)() + defer log.Scope(t).Close(t) + ctx := context.Background() + stopper := stop.NewStopper() + defer stopper.Stop(ctx) + + // We can't hardcode versions here because the can become too old to create + // stores with. create a store. + // For example's sake, let's assume that minV is 1.0. Then binV is 1.1 and + // development versions are 1.0-1 and 1.0-2. + minV := clusterversion.TestingBinaryMinSupportedVersion + binV := minV + binV.Minor += 1 + + // Development versions. + versionA := minV + versionA.Internal = 1 + + versionB := minV + versionB.Internal = 2 + + engines := make([]storage.Engine, 3) + for i := range engines { + // Create the stores without an initialized cluster version. + st := cluster.MakeTestingClusterSettingsWithVersions(binV, minV, false /* initializeVersion */) + eng, err := storage.Open( + ctx, storage.InMemory(), st, + storage.ForTesting, storage.MaxSize(1<<20), + ) + if err != nil { + t.Fatal(err) + } + stopper.AddCloser(eng) + engines[i] = eng + } + e0 := engines[:1] + e01 := engines[:2] + e2 := engines[2:3] + e012 := engines[:3] + + // If there are no stores, default to minV. + if initialCV, err := SynthesizeClusterVersionFromEngines(ctx, nil, binV, minV); err != nil { + t.Fatal(err) + } else { + expCV := clusterversion.ClusterVersion{ + Version: minV, + } + if !reflect.DeepEqual(initialCV, expCV) { + t.Fatalf("expected %+v; got %+v", expCV, initialCV) + } + } + + // Verify that the initial read of an empty store synthesizes minV. This + // is the code path that runs after starting the binV binary for the first + // time after the rolling upgrade from minV. + if initialCV, err := SynthesizeClusterVersionFromEngines(ctx, e0, binV, minV); err != nil { + t.Fatal(err) + } else { + expCV := clusterversion.ClusterVersion{ + Version: minV, + } + if !reflect.DeepEqual(initialCV, expCV) { + t.Fatalf("expected %+v; got %+v", expCV, initialCV) + } + } + + // Bump a version to something more modern (but supported by this binary). + // Note that there's still only one store. + { + cv := clusterversion.ClusterVersion{ + Version: versionB, + } + if err := WriteClusterVersionToEngines(ctx, e0, cv); err != nil { + t.Fatal(err) + } + + // Verify the same thing comes back on read. + if newCV, err := SynthesizeClusterVersionFromEngines(ctx, e0, binV, minV); err != nil { + t.Fatal(err) + } else { + expCV := cv + if !reflect.DeepEqual(newCV, cv) { + t.Fatalf("expected %+v; got %+v", expCV, newCV) + } + } + } + + // Use stores 0 and 1. It reads as minV because store 1 has no entry, lowering + // the use version to minV. + { + expCV := clusterversion.ClusterVersion{ + Version: minV, + } + if cv, err := SynthesizeClusterVersionFromEngines(ctx, e01, binV, minV); err != nil { + t.Fatal(err) + } else if !reflect.DeepEqual(cv, expCV) { + t.Fatalf("expected %+v, got %+v", expCV, cv) + } + + // Write an updated Version to both stores. + cv := clusterversion.ClusterVersion{ + Version: versionB, + } + if err := WriteClusterVersionToEngines(ctx, e01, cv); err != nil { + t.Fatal(err) + } + } + + // Third node comes along, for now it's alone. It has a lower use version. + cv := clusterversion.ClusterVersion{ + Version: versionA, + } + + if err := WriteClusterVersionToEngines(ctx, e2, cv); err != nil { + t.Fatal(err) + } + + // Reading across all stores, we expect to pick up the lowest useVersion both + // from the third store. + expCV := clusterversion.ClusterVersion{ + Version: versionA, + } + if cv, err := SynthesizeClusterVersionFromEngines(ctx, e012, binV, minV); err != nil { + t.Fatal(err) + } else if !reflect.DeepEqual(cv, expCV) { + t.Fatalf("expected %+v, got %+v", expCV, cv) + } +} diff --git a/pkg/kv/kvserver/kvstorage/init.go b/pkg/kv/kvserver/kvstorage/init.go index f791d8d99217..12a54d1f1f37 100644 --- a/pkg/kv/kvserver/kvstorage/init.go +++ b/pkg/kv/kvserver/kvstorage/init.go @@ -127,7 +127,7 @@ func checkCanInitializeEngine(ctx context.Context, eng storage.Engine) error { if k, err = getMVCCKey(); err != nil { return err } - if !k.Key.Equal(keys.StoreClusterVersionKey()) { + if !k.Key.Equal(keys.DeprecatedStoreClusterVersionKey()) { return errors.New("no cluster version found on uninitialized engine") } valid, err = iter.NextEngineKey() diff --git a/pkg/kv/kvserver/stores.go b/pkg/kv/kvserver/stores.go index d1849ca05f34..f47ed9b91f62 100644 --- a/pkg/kv/kvserver/stores.go +++ b/pkg/kv/kvserver/stores.go @@ -300,14 +300,3 @@ func (ls *Stores) updateBootstrapInfoLocked(bi *gossip.BootstrapInfo) error { }) return err } - -func (ls *Stores) engines() []storage.Engine { - var engines []storage.Engine - ls.storeMap.Range(func(_ int64, v unsafe.Pointer) bool { - // TODO(sep-raft-log): at time of writing the only caller to this is - // TestClusterVersionWriteSynthesize. - engines = append(engines, (*Store)(v).TODOEngine()) - return true // want more - }) - return engines -} diff --git a/pkg/kv/kvserver/stores_test.go b/pkg/kv/kvserver/stores_test.go index 2f1df4e8182b..d7c68d864adc 100644 --- a/pkg/kv/kvserver/stores_test.go +++ b/pkg/kv/kvserver/stores_test.go @@ -15,9 +15,7 @@ import ( "reflect" "testing" - "github.com/cockroachdb/cockroach/pkg/clusterversion" "github.com/cockroachdb/cockroach/pkg/gossip" - "github.com/cockroachdb/cockroach/pkg/kv/kvserver/kvstorage" "github.com/cockroachdb/cockroach/pkg/kv/kvserver/logstore" "github.com/cockroachdb/cockroach/pkg/roachpb" "github.com/cockroachdb/cockroach/pkg/storage" @@ -340,136 +338,3 @@ func TestStoresGossipStorageReadLatest(t *testing.T) { t.Errorf("bootstrap info %+v not equal to expected %+v", verifyBI, bi) } } - -// TestStoresClusterVersionWriteSynthesize verifies that the cluster version is -// written to all stores and that missing versions are filled in appropriately. -// -// TODO(tbg): if this test were a little more principled about only creating -// what it needs, it could move closer to SynthesizeClusterVersionFromEngines. -func TestClusterVersionWriteSynthesize(t *testing.T) { - defer leaktest.AfterTest(t)() - defer log.Scope(t).Close(t) - _, stores, _, stopper := createStores(3) - ctx := context.Background() - defer stopper.Stop(ctx) - - v1_0 := roachpb.Version{Major: 1} - // Hard-code binaryVersion of 1.1 for this test. - // Hard-code binaryMinSupportedVersion of 1.0 for this test. - binV := roachpb.Version{Major: 1, Minor: 1} - minV := v1_0 - - makeStores := func() *Stores { - ls := NewStores(log.MakeTestingAmbientCtxWithNewTracer(), stores[0].Clock()) - return ls - } - - ls0 := makeStores() - - // If there are no stores, default to binaryMinSupportedVersion - // (v1_0 in this test) - if initialCV, err := kvstorage.SynthesizeClusterVersionFromEngines(ctx, ls0.engines(), binV, minV); err != nil { - t.Fatal(err) - } else { - expCV := clusterversion.ClusterVersion{ - Version: v1_0, - } - if !reflect.DeepEqual(initialCV, expCV) { - t.Fatalf("expected %+v; got %+v", expCV, initialCV) - } - } - - ls0.AddStore(stores[0]) - - versionA := roachpb.Version{Major: 1, Minor: 0, Internal: 1} // 1.0-1 - versionB := roachpb.Version{Major: 1, Minor: 0, Internal: 2} // 1.0-2 - - // Verify that the initial read of an empty store synthesizes v1.0-0. This - // is the code path that runs after starting the 1.1 binary for the first - // time after the rolling upgrade from 1.0. - if initialCV, err := kvstorage.SynthesizeClusterVersionFromEngines(ctx, ls0.engines(), binV, minV); err != nil { - t.Fatal(err) - } else { - expCV := clusterversion.ClusterVersion{ - Version: v1_0, - } - if !reflect.DeepEqual(initialCV, expCV) { - t.Fatalf("expected %+v; got %+v", expCV, initialCV) - } - } - - // Bump a version to something more modern (but supported by this binary). - // Note that there's still only one store. - { - cv := clusterversion.ClusterVersion{ - Version: versionB, - } - if err := kvstorage.WriteClusterVersionToEngines(ctx, ls0.engines(), cv); err != nil { - t.Fatal(err) - } - - // Verify the same thing comes back on read. - if newCV, err := kvstorage.SynthesizeClusterVersionFromEngines(ctx, ls0.engines(), binV, minV); err != nil { - t.Fatal(err) - } else { - expCV := cv - if !reflect.DeepEqual(newCV, cv) { - t.Fatalf("expected %+v; got %+v", expCV, newCV) - } - } - } - - // Make a stores with store0 and store1. It reads as v1.0 because store1 has - // no entry, lowering the use version to v1.0 (but not the min version). - { - ls01 := makeStores() - ls01.AddStore(stores[0]) - ls01.AddStore(stores[1]) - - expCV := clusterversion.ClusterVersion{ - Version: v1_0, - } - if cv, err := kvstorage.SynthesizeClusterVersionFromEngines(ctx, ls01.engines(), binV, minV); err != nil { - t.Fatal(err) - } else if !reflect.DeepEqual(cv, expCV) { - t.Fatalf("expected %+v, got %+v", expCV, cv) - } - - // Write an updated Version to both stores. - cv := clusterversion.ClusterVersion{ - Version: versionB, - } - if err := kvstorage.WriteClusterVersionToEngines(ctx, ls01.engines(), cv); err != nil { - t.Fatal(err) - } - } - - // Third node comes along, for now it's alone. It has a lower use version. - cv := clusterversion.ClusterVersion{ - Version: versionA, - } - - { - ls3 := makeStores() - ls3.AddStore(stores[2]) - if err := kvstorage.WriteClusterVersionToEngines(ctx, ls3.engines(), cv); err != nil { - t.Fatal(err) - } - } - - ls012 := makeStores() - for _, store := range stores { - ls012.AddStore(store) - } - - // Reading across all stores, we expect to pick up the lowest useVersion both - // from the third store. - expCV := clusterversion.ClusterVersion{ - Version: versionA, - } - if cv, err := kvstorage.SynthesizeClusterVersionFromEngines(ctx, ls012.engines(), binV, minV); err != nil { - t.Fatal(err) - } else if !reflect.DeepEqual(cv, expCV) { - t.Fatalf("expected %+v, got %+v", expCV, cv) - } -} diff --git a/pkg/server/init.go b/pkg/server/init.go index 785afd7e9e21..b9bc0fd28322 100644 --- a/pkg/server/init.go +++ b/pkg/server/init.go @@ -554,7 +554,7 @@ func (s *initServer) initializeFirstStoreAfterJoin( } func assertEnginesEmpty(engines []storage.Engine) error { - storeClusterVersionKey := keys.StoreClusterVersionKey() + storeClusterVersionKey := keys.DeprecatedStoreClusterVersionKey() for _, engine := range engines { err := func() error { diff --git a/pkg/server/node.go b/pkg/server/node.go index abec3bd3a881..34726d2f82fa 100644 --- a/pkg/server/node.go +++ b/pkg/server/node.go @@ -311,17 +311,15 @@ func bootstrapCluster( // other than the first one, and let regular node startup code deal with them. var bootstrapVersion clusterversion.ClusterVersion for i, eng := range engines { - cv, err := kvstorage.ReadClusterVersion(ctx, eng) - if err != nil { - return nil, errors.Wrapf(err, "reading cluster version of %s", eng) - } else if cv.Major == 0 { + cv := eng.MinVersion() + if cv.Major == 0 { return nil, errors.Errorf("missing bootstrap version") } // bootstrapCluster requires matching cluster versions on all engines. if i == 0 { - bootstrapVersion = cv - } else if bootstrapVersion != cv { + bootstrapVersion.Version = cv + } else if bootstrapVersion.Version != cv { return nil, errors.Errorf("found cluster versions %s and %s", bootstrapVersion, cv) } @@ -629,11 +627,8 @@ func (n *Node) SetHLCUpperBound(ctx context.Context, hlcUpperBound int64) error } func (n *Node) addStore(ctx context.Context, store *kvserver.Store) { - cv, err := kvstorage.ReadClusterVersion(context.TODO(), store.TODOEngine()) - if err != nil { - log.Fatalf(ctx, "%v", err) - } - if cv == (clusterversion.ClusterVersion{}) { + cv := store.TODOEngine().MinVersion() + if cv == (roachpb.Version{}) { // The store should have had a version written to it during the store // initialization process. log.Fatal(ctx, "attempting to add a store without a version") diff --git a/pkg/server/server_test.go b/pkg/server/server_test.go index 598f3ecd6487..eeea6f3e6add 100644 --- a/pkg/server/server_test.go +++ b/pkg/server/server_test.go @@ -1080,7 +1080,7 @@ func TestAssertEnginesEmpty(t *testing.T) { require.NoError(t, assertEnginesEmpty([]storage.Engine{eng})) - require.NoError(t, storage.MVCCPutProto(ctx, eng, nil, keys.StoreClusterVersionKey(), + require.NoError(t, storage.MVCCPutProto(ctx, eng, nil, keys.DeprecatedStoreClusterVersionKey(), hlc.Timestamp{}, hlc.ClockTimestamp{}, nil, &roachpb.Version{Major: 21, Minor: 1, Internal: 122})) require.NoError(t, assertEnginesEmpty([]storage.Engine{eng})) diff --git a/pkg/server/version_cluster_test.go b/pkg/server/version_cluster_test.go index 7082ba189164..5d925d3f876d 100644 --- a/pkg/server/version_cluster_test.go +++ b/pkg/server/version_cluster_test.go @@ -21,7 +21,6 @@ import ( "github.com/cockroachdb/cockroach/pkg/base" "github.com/cockroachdb/cockroach/pkg/clusterversion" "github.com/cockroachdb/cockroach/pkg/kv/kvserver" - "github.com/cockroachdb/cockroach/pkg/kv/kvserver/kvstorage" "github.com/cockroachdb/cockroach/pkg/roachpb" "github.com/cockroachdb/cockroach/pkg/server" "github.com/cockroachdb/cockroach/pkg/settings/cluster" @@ -193,11 +192,8 @@ func TestClusterVersionPersistedOnJoin(t *testing.T) { for i := 0; i < len(tc.TestCluster.Servers); i++ { for _, engine := range tc.TestCluster.Servers[i].Engines() { - cv, err := kvstorage.ReadClusterVersion(ctx, engine) - if err != nil { - t.Fatal(err) - } - if cv.Version != newVersion { + cv := engine.MinVersion() + if cv != newVersion { t.Fatalf("n%d: expected version %v, got %v", i+1, newVersion, cv) } } @@ -324,11 +320,8 @@ func TestClusterVersionUpgrade(t *testing.T) { // Since the wrapped version setting exposes the new versions, it must // definitely be present on all stores on the first try. if err := tc.Servers[1].GetStores().(*kvserver.Stores).VisitStores(func(s *kvserver.Store) error { - cv, err := kvstorage.ReadClusterVersion(ctx, s.TODOEngine()) - if err != nil { - return err - } - if act := cv.Version.String(); act != exp { + cv := s.TODOEngine().MinVersion() + if act := cv.String(); act != exp { t.Fatalf("%s: %s persisted, but should be %s", s, act, exp) } return nil diff --git a/pkg/storage/engine.go b/pkg/storage/engine.go index 23f678e19f0b..05caa963cc0f 100644 --- a/pkg/storage/engine.go +++ b/pkg/storage/engine.go @@ -950,6 +950,12 @@ type Engine interface { // SSTs that don't overlap with any of these key spans. CreateCheckpoint(dir string, spans []roachpb.Span) error + // MinVersion is the minimum CockroachDB version that is compatible with this + // store. For newly created stores, this matches the currently active cluster + // version. + // Must never return an empty version. + MinVersion() roachpb.Version + // SetMinVersion is used to signal to the engine the current minimum // version that it must maintain compatibility with. SetMinVersion(version roachpb.Version) error diff --git a/pkg/storage/pebble.go b/pkg/storage/pebble.go index e21cf82c05d1..796f3f6f8b22 100644 --- a/pkg/storage/pebble.go +++ b/pkg/storage/pebble.go @@ -751,6 +751,9 @@ type Pebble struct { } asyncDone sync.WaitGroup + // minVersion is the minimum CockroachDB version that can open this store. + minVersion roachpb.Version + // closer is populated when the database is opened. The closer is associated // with the filesyetem closer io.Closer @@ -1951,10 +1954,13 @@ func (p *Pebble) CreateCheckpoint(dir string, spans []roachpb.Span) error { // SetMinVersion implements the Engine interface. func (p *Pebble) SetMinVersion(version roachpb.Version) error { + p.minVersion = version + if p.readOnly { // Don't make any on-disk changes. return nil } + // NB: SetMinVersion must be idempotent. It may called multiple // times with the same version. @@ -2010,6 +2016,11 @@ func (p *Pebble) SetMinVersion(version roachpb.Version) error { return nil } +// MinVersion implements the Engine interface. +func (p *Pebble) MinVersion() roachpb.Version { + return p.minVersion +} + // BufferedSize implements the Engine interface. func (p *Pebble) BufferedSize() int { return 0