From 4e40d3c3abefbb72e237656b831c6c3b1cad3de5 Mon Sep 17 00:00:00 2001 From: Arul Ajmani Date: Tue, 26 Nov 2024 13:38:43 -0500 Subject: [PATCH] kvserver: eagerly unquiesce ranges that use leader leases We eagerly unquiesce expiration based leases when starting/restarting a store. Like expiration based leases, leader leases also don't support quiescence. As such, this patch has them start off life as unquiescent. Found while trying to port TestStoreLoadReplicaQuiescent over to use leader leases. References https://github.com/cockroachdb/cockroach/issues/133763 Closes https://github.com/cockroachdb/cockroach/issues/136231 Release note: None --- pkg/kv/kvserver/client_store_test.go | 45 +++++++++++++++++++++---- pkg/kv/kvserver/replica_raft_quiesce.go | 17 +++------- pkg/kv/kvserver/store.go | 15 +++++---- pkg/roachpb/data.go | 19 +++++++++++ 4 files changed, 70 insertions(+), 26 deletions(-) diff --git a/pkg/kv/kvserver/client_store_test.go b/pkg/kv/kvserver/client_store_test.go index d92fdc1fbcd9..e7a2967a7b8a 100644 --- a/pkg/kv/kvserver/client_store_test.go +++ b/pkg/kv/kvserver/client_store_test.go @@ -11,6 +11,7 @@ import ( "time" "github.com/cockroachdb/cockroach/pkg/base" + "github.com/cockroachdb/cockroach/pkg/kv" "github.com/cockroachdb/cockroach/pkg/kv/kvserver" "github.com/cockroachdb/cockroach/pkg/kv/kvserver/stateloader" "github.com/cockroachdb/cockroach/pkg/roachpb" @@ -22,6 +23,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/testutils/listenerutil" "github.com/cockroachdb/cockroach/pkg/testutils/sqlutils" "github.com/cockroachdb/cockroach/pkg/testutils/testcluster" + "github.com/cockroachdb/cockroach/pkg/util/hlc" "github.com/cockroachdb/cockroach/pkg/util/leaktest" "github.com/cockroachdb/cockroach/pkg/util/log" "github.com/cockroachdb/errors" @@ -115,16 +117,16 @@ func TestStoreLoadReplicaQuiescent(t *testing.T) { defer leaktest.AfterTest(t)() defer log.Scope(t).Close(t) - testutils.RunTrueAndFalse(t, "kv.expiration_leases_only.enabled", func(t *testing.T, expOnly bool) { + testutils.RunValues(t, "lease-type", roachpb.TestingAllLeaseTypes(), func(t *testing.T, leaseType roachpb.LeaseType) { storeReg := fs.NewStickyRegistry() listenerReg := listenerutil.NewListenerRegistry() defer listenerReg.Close() ctx := context.Background() st := cluster.MakeTestingClusterSettings() - kvserver.OverrideLeaderLeaseMetamorphism(ctx, &st.SV) - kvserver.ExpirationLeasesOnly.Override(ctx, &st.SV, expOnly) + kvserver.OverrideDefaultLeaseType(ctx, &st.SV, leaseType) + manualClock := hlc.NewHybridManualClock() tc := testcluster.StartTestCluster(t, 1, base.TestClusterArgs{ ReplicationMode: base.ReplicationManual, ReusableListenerReg: listenerReg, @@ -136,6 +138,7 @@ func TestStoreLoadReplicaQuiescent(t *testing.T) { Knobs: base.TestingKnobs{ Server: &server.TestingKnobs{ StickyVFSRegistry: storeReg, + WallClock: manualClock, }, Store: &kvserver.StoreTestingKnobs{ DisableScanner: true, @@ -161,10 +164,33 @@ func TestStoreLoadReplicaQuiescent(t *testing.T) { repl := tc.GetFirstStoreFromServer(t, 0).GetReplicaIfExists(desc.RangeID) require.NotNil(t, repl) lease, _ := repl.GetLease() - if expOnly { + + if leaseType == roachpb.LeaseLeader { + // The first lease that'll be acquired above is going to be an expiration + // based lease. That's because at that point, there won't be a leader, + // and the lease acquisition will trigger an election. Expire that lease + // and send a request that'll force a re-acquisition. This time, we should + // get a leader lease. + manualClock.Increment(tc.Server(0).RaftConfig().RangeLeaseDuration.Nanoseconds()) + incArgs := incrementArgs(key, int64(5)) + + testutils.SucceedsSoon(t, func() error { + _, err := kv.SendWrapped(ctx, tc.GetFirstStoreFromServer(t, 0).TestSender(), incArgs) + return err.GoError() + }) + lease, _ = repl.GetLease() + require.Equal(t, roachpb.LeaseLeader, lease.Type()) + } + + switch leaseType { + case roachpb.LeaseExpiration: require.Equal(t, roachpb.LeaseExpiration, lease.Type()) - } else { + case roachpb.LeaseEpoch: require.Equal(t, roachpb.LeaseEpoch, lease.Type()) + case roachpb.LeaseLeader: + require.Equal(t, roachpb.LeaseLeader, lease.Type()) + default: + panic("unknown") } // Restart the server and check whether the range starts out quiesced. @@ -175,6 +201,13 @@ func TestStoreLoadReplicaQuiescent(t *testing.T) { repl, _, err = tc.Server(0).GetStores().(*kvserver.Stores).GetReplicaForRangeID(ctx, desc.RangeID) require.NoError(t, err) require.NotNil(t, repl.RaftStatus()) - require.Equal(t, !expOnly, repl.IsQuiescent()) + switch leaseType { + case roachpb.LeaseExpiration, roachpb.LeaseLeader: + require.False(t, repl.IsQuiescent()) + case roachpb.LeaseEpoch: + require.True(t, repl.IsQuiescent()) + default: + panic("unknown") + } }) } diff --git a/pkg/kv/kvserver/replica_raft_quiesce.go b/pkg/kv/kvserver/replica_raft_quiesce.go index 1dc56f138bd5..627f76734f49 100644 --- a/pkg/kv/kvserver/replica_raft_quiesce.go +++ b/pkg/kv/kvserver/replica_raft_quiesce.go @@ -300,19 +300,10 @@ func shouldReplicaQuiesceRaftMuLockedReplicaMuLocked( } return nil, nil, false } - // Fast path: don't quiesce leader leases. A fortified raft leader does not - // send raft heartbeats, so quiescence is not needed. All liveness decisions - // are based on store liveness communication, which is cheap enough to not - // need a notion of quiescence. - if leaseStatus.Lease.Type() == roachpb.LeaseLeader { - log.VInfof(ctx, 4, "not quiescing: leader lease") - return nil, nil, false - } - // Fast path: don't quiesce expiration-based leases, since they'll likely be - // renewed soon. The lease may not be ours, but in that case we wouldn't be - // able to quiesce anyway (see leaseholder condition below). - if l := leaseStatus.Lease; l.Type() == roachpb.LeaseExpiration { - log.VInfof(ctx, 4, "not quiescing: expiration-based lease") + // Fast path: if the lease doesn't support quiescence (leader leases and + // expiration based leases do not), return early. + if !leaseStatus.Lease.SupportsQuiescence() { + log.VInfof(ctx, 4, "not quiescing: %s", leaseStatus.Lease.Type()) return nil, nil, false } if q.hasPendingProposalsRLocked() { diff --git a/pkg/kv/kvserver/store.go b/pkg/kv/kvserver/store.go index 1053b4464a9a..de476c9a54d4 100644 --- a/pkg/kv/kvserver/store.go +++ b/pkg/kv/kvserver/store.go @@ -2325,15 +2325,16 @@ func (s *Store) Start(ctx context.Context, stopper *stop.Stopper) error { return errors.AssertionFailedf("no tenantID for initialized replica %s", rep) } - // Eagerly unquiesce replicas that use expiration-based leases. We don't - // quiesce ranges with expiration leases, and we want to eagerly acquire - // leases for them, which happens during Raft ticks. We rely on Raft - // pre-vote to avoid disturbing established Raft leaders. + // Eagerly unquiece replicas that are using a lease that doesn't support quiescnce. + // In practice, this means we'll unquiesce ranges that are using leader leases + // or expiration leases. We want to eagerly establish raft leaders for such + // ranges and acquire leases on top of this. This happens during Raft ticks. + // We rely on Raft pre-vote to avoid disturbance to Raft leader.s // // NB: cluster settings haven't propagated yet, so we have to check the last - // known lease instead of relying on shouldUseExpirationLeaseRLocked(). We - // also check Sequence > 0 to omit ranges that haven't seen a lease yet. - if l, _ := rep.GetLease(); l.Type() == roachpb.LeaseExpiration && l.Sequence > 0 { + // known lease instead of desiredLeaseTypeRLocked. We also check Sequence > + // 0 to omit ranges that haven't seen a lease yet. + if l, _ := rep.GetLease(); !l.SupportsQuiescence() && l.Sequence > 0 { rep.maybeUnquiesce(ctx, true /* wakeLeader */, true /* mayCampaign */) } } diff --git a/pkg/roachpb/data.go b/pkg/roachpb/data.go index 48ad94c38e6c..e90b454d2c6f 100644 --- a/pkg/roachpb/data.go +++ b/pkg/roachpb/data.go @@ -2021,6 +2021,25 @@ func (l Lease) Type() LeaseType { return LeaseExpiration } +// SupportsQuiescence returns whether the lease supports quiescence or not. +func (l Lease) SupportsQuiescence() bool { + switch l.Type() { + case LeaseExpiration, LeaseLeader: + // Expiration based leases do not support quiescence because they'll likely + // be renewed soon, so there's not much point to it. + // + // Leader leases do not support quiescence because a fortified raft leader + // will not send raft heartbeats, so quiescence is not needed. All liveness + // decisions are based on store liveness communication, which is cheap + // enough to not need a notion of quiescence. + return false + case LeaseEpoch: + return true + default: + panic("unexpected lease type") + } +} + // Speculative returns true if this lease instance doesn't correspond to a // committed lease (or at least to a lease that's *known* to have committed). // For example, nodes sometimes guess who a leaseholder might be and synthesize