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