Merge cockroachdb#132915 cockroachdb#133984 cockroachdb#133987

132915: raft: consult store liveness before campaigning r=iskettaneh a=iskettaneh This commit makes followers to make sure they are supported by a quorum before campaigning. Fixes: cockroachdb#132755 Release note: None 133984: workload: add --active-workers to tpcc r=dt a=dt Release note: none. Epic: none. 133987: sql: deflake TestStatementTimeoutForSchemaChangeCommit r=rafiss a=rafiss Now we release the lock in a defer so that the test can't hang during shutdown. fixes cockroachdb#133397 Release note: None Co-authored-by: Ibrahim Kettaneh <[email protected]> Co-authored-by: David Taylor <[email protected]> Co-authored-by: Rafi Shamim <[email protected]>
pav-kv · Oct 31, 2024 · 4788502 · 4788502
4 parents 7a88127 + ae16e22 + 0b332b4 + 14361b0
commit 4788502
Show file tree

Hide file tree

Showing 14 changed files with 318 additions and 64 deletions.
diff --git a/pkg/kv/kvclient/kvcoord/dist_sender_server_test.go b/pkg/kv/kvclient/kvcoord/dist_sender_server_test.go
@@ -4764,7 +4764,13 @@ func TestPartialPartition(t *testing.T) {
 				require.NoError(t, tc.WaitForFullReplication())
 
 				desc := tc.LookupRangeOrFatal(t, scratchKey)
-				tc.TransferRangeLeaseOrFatal(t, desc, tc.Target(1))
+				err := tc.TransferRangeLease(desc, tc.Target(1))
+				if leaseType != roachpb.LeaseLeader {
+					// In leader leases, the leader won't campaign if it's not supported
+					// by a majority. We will keep trying to transfer the lease until it
+					// succeeds below.
+					require.NoError(t, err)
+				}
 
 				// TODO(baptist): This test should work without this block.
 				// After the lease is transferred, the lease might still be on
@@ -4778,6 +4784,16 @@ func TestPartialPartition(t *testing.T) {
 				// DistSender we will never succeed once we partition. Remove
 				// this block once #118943 is fixed.
 				testutils.SucceedsSoon(t, func() error {
+					if leaseType == roachpb.LeaseLeader {
+						// In leader leases, the leader won't campaign if it's not supported
+						// by a majority of voters. This causes a flake in this test
+						// where the new leader is not elected if it doesn't yet have
+						// support from a majority. We need to keep trying to transfer the
+						// lease until the new leader is elected.
+						if err := tc.TransferRangeLease(desc, tc.Target(1)); err != nil {
+							return err
+						}
+					}
 					sl := tc.StorageLayer(1)
 					store, err := sl.GetStores().(*kvserver.Stores).GetStore(sl.GetFirstStoreID())
 					require.NoError(t, err)
@@ -4795,7 +4811,7 @@ func TestPartialPartition(t *testing.T) {
 				// to fail faster.
 				cancelCtx, cancel := context.WithTimeout(ctx, 5*time.Second)
 				defer cancel()
-				err := txn.Put(cancelCtx, scratchKey, "abc")
+				err = txn.Put(cancelCtx, scratchKey, "abc")
 				if test.useProxy {
 					require.NoError(t, err)
 					require.NoError(t, txn.Commit(cancelCtx))

diff --git a/pkg/kv/kvserver/client_raft_helpers_test.go b/pkg/kv/kvserver/client_raft_helpers_test.go
@@ -472,6 +472,8 @@ func dropRaftMessagesFrom(
 				drop := shouldDropFromStore(msg.From.StoreID)
 				if drop {
 					t.Logf("dropping msg %s from store %d: to %d", msg.Type, msg.From.StoreID, msg.To.StoreID)
+				} else {
+					t.Logf("allowing msg %s from store %d: to %d", msg.Type, msg.From.StoreID, msg.To.StoreID)
 				}
 				return drop
 			},

diff --git a/pkg/kv/kvserver/client_raft_test.go b/pkg/kv/kvserver/client_raft_test.go
@@ -1132,11 +1132,12 @@ func TestRequestsOnLaggingReplica(t *testing.T) {
 		// handler on the other two stores only filters out messages from the
 		// partitioned store. The configuration looks like:
 		//
-		//           [0]
-		//          x  x
-		//         /    \
-		//        x      x
-		//      [1]<---->[2]
+		//     [symmetric=false]      [symmetric=true]
+		//           [0]                     [0]
+		//          ^  ^                    x   x
+		//         /    \                  /     \
+		//        x      x                x       x
+		//      [1]<---->[2]            [1]<----->[2]
 		//
 		log.Infof(ctx, "test: partitioning node")
 		const partitionNodeIdx = 0
@@ -1161,8 +1162,8 @@ func TestRequestsOnLaggingReplica(t *testing.T) {
 		// partitioned one.
 		log.Infof(ctx, "test: waiting for leadership to fail over")
 		testutils.SucceedsSoon(t, func() error {
-			if partRepl.RaftStatus().Lead != raft.None {
-				return errors.New("partitioned replica should stepped down")
+			if partRepl.RaftStatus().RaftState == raftpb.StateLeader {
+				return errors.New("partitioned replica should have stepped down")
 			}
 			lead := otherRepl.RaftStatus().Lead
 			if lead == raft.None {
@@ -1239,7 +1240,16 @@ func TestRequestsOnLaggingReplica(t *testing.T) {
 		require.Error(t, pErr.GoError(), "unexpected success")
 		nlhe := &kvpb.NotLeaseHolderError{}
 		require.ErrorAs(t, pErr.GetDetail(), &nlhe, pErr)
-		require.True(t, nlhe.Lease.Empty())
+
+		if symmetric {
+			// In symmetric=true, we expect that the partitioned replica to not know
+			// about the leader. As a result, it returns a NotLeaseHolderError without
+			// a speculative lease.
+			require.True(t, nlhe.Lease.Empty(), "expected empty lease, got %v", nlhe.Lease)
+		} else {
+			require.False(t, nlhe.Lease.Empty())
+			require.Equal(t, leaderReplicaID, nlhe.Lease.Replica.ReplicaID)
+		}
 
 		// Resolve the partition, but continue blocking snapshots destined for the
 		// previously-partitioned replica. The point of blocking the snapshots is to
@@ -1270,7 +1280,7 @@ func TestRequestsOnLaggingReplica(t *testing.T) {
 				continue
 			}
 			store := tc.GetFirstStoreFromServer(t, i)
-			store.Transport().ListenIncomingRaftMessages(store.Ident.StoreID, store)
+			store.Transport().ListenIncomingRaftMessages(store.StoreID(), store)
 			store.StoreLivenessTransport().ListenMessages(
 				store.StoreID(), store.TestingStoreLivenessMessageHandler(),
 			)
@@ -6536,16 +6546,13 @@ func TestRaftCheckQuorum(t *testing.T) {
 		require.False(t, repl1.IsQuiescent())
 		t.Logf("n1 not quiesced")
 
-		// Wait for the leader to become a candidate.
+		// Wait for the leader to step down.
 		require.Eventually(t, func() bool {
 			status := repl1.RaftStatus()
 			logStatus(status)
-			// TODO(ibrahim): once we start checking StoreLiveness before
-			// transitioning to a pre-candidate, we'll need to switch this (and the
-			// conditional below) to handle this.
-			return status.RaftState == raftpb.StatePreCandidate
+			return status.RaftState != raftpb.StateLeader
 		}, 10*time.Second, 500*time.Millisecond)
-		t.Logf("n1 became pre-candidate")
+		t.Logf("n1 stepped down as a leader")
 
 		// n2 or n3 should elect a new leader.
 		var leaderStatus *raft.Status
@@ -6561,23 +6568,17 @@ func TestRaftCheckQuorum(t *testing.T) {
 		}, 10*time.Second, 500*time.Millisecond)
 		t.Logf("n%d became leader", leaderStatus.ID)
 
-		// n1 should remain pre-candidate, since it doesn't hear about the new
-		// leader.
+		// n1 shouldn't become a leader.
 		require.Never(t, func() bool {
 			status := repl1.RaftStatus()
 			logStatus(status)
-			// TODO(ibrahim): uncomment this once we start checking StoreLiveness
-			// before transitioning to a pre-candidate.
-			//expState := status.RaftState == raftpb.StateFollower && status.Lead == raft.None
-			//return !expState // require.Never
-			return status.RaftState != raftpb.StatePreCandidate
+			expState := status.RaftState != raftpb.StateLeader && status.Lead == raft.None
+			return !expState // require.Never
 		}, 3*time.Second, 500*time.Millisecond)
-		t.Logf("n1 remains pre-candidate")
+		t.Logf("n1 remains not leader")
 
-		// The existing leader shouldn't have been affected by n1's prevotes.
-		// TODO(ibrahim): This portion of the test can be removed entirely once we
-		// don't even transition to a pre-candidate because StoreLiveness doesn't
-		// let us.
+		// The existing leader shouldn't have been affected by the possible n1's
+		// prevotes.
 		var finalStatus *raft.Status
 		for _, status := range []*raft.Status{repl2.RaftStatus(), repl3.RaftStatus()} {
 			logStatus(status)

diff --git a/pkg/raft/raft.go b/pkg/raft/raft.go
@@ -1356,6 +1356,14 @@ func (r *raft) hup(t CampaignType) {
 		r.logger.Debugf("%x ignoring MsgHup due to leader fortification", r.id)
 		return
 	}
+
+	// We shouldn't campaign if we don't have quorum support in store liveness.
+	if r.fortificationTracker.RequireQuorumSupportOnCampaign() &&
+		!r.fortificationTracker.QuorumSupported() {
+		r.logger.Debugf("%x cannot campaign since it's not supported by a quorum in store liveness", r.id)
+		return
+	}
+
 	if r.hasUnappliedConfChanges() {
 		r.logger.Warningf("%x cannot campaign at term %d since there are still pending configuration changes to apply", r.id, r.Term)
 		return

diff --git a/pkg/raft/testdata/de_fortification_checkquorum.txt b/pkg/raft/testdata/de_fortification_checkquorum.txt
@@ -46,7 +46,6 @@ withdraw-support 3 1
 2 x 1 1
 3 x 1 1
 
-
 tick-election 1
 ----
 ok
@@ -77,20 +76,16 @@ set-randomized-election-timeout 1 timeout=3
 ----
 ok
 
-# Even though the leader doesn't have store liveness support, it'll still call
-# a pre-vote election until we start checking store liveness before doing so.
+# Since we don't have store liveness support, we can't campaign.
 # However, on this ticker, it'll also broadcast a de-fortify to all peers --
 # which is what we're interested in for this test.
 tick-election 1
 ----
-INFO 1 is starting a new election at term 1
-INFO 1 became pre-candidate at term 1
-INFO 1 [logterm: 1, index: 11] sent MsgPreVote request to 2 at term 1
-INFO 1 [logterm: 1, index: 11] sent MsgPreVote request to 3 at term 1
+DEBUG 1 cannot campaign since it's not supported by a quorum in store liveness
 
 raft-state 1
 ----
-1: StatePreCandidate (Voter) Term:1 Lead:0 LeadEpoch:0
+1: StateFollower (Voter) Term:1 Lead:0 LeadEpoch:0
 2: StateFollower (Voter) Term:1 Lead:1 LeadEpoch:1
 3: StateFollower (Voter) Term:1 Lead:1 LeadEpoch:1
 
@@ -99,35 +94,27 @@ stabilize
 ----
 > 1 handling Ready
   Ready MustSync=true:
-  State:StatePreCandidate
-  HardState Term:1 Vote:1 Commit:11 Lead:0 LeadEpoch:0
+  State:StateFollower
+  HardState Term:1 Vote:1 Commit:11 Lead:1 LeadEpoch:0
   Messages:
   1->2 MsgDeFortifyLeader Term:1 Log:0/0
   1->3 MsgDeFortifyLeader Term:1 Log:0/0
   1->2 MsgDeFortifyLeader Term:1 Log:0/0
   1->3 MsgDeFortifyLeader Term:1 Log:0/0
   1->2 MsgDeFortifyLeader Term:1 Log:0/0
   1->3 MsgDeFortifyLeader Term:1 Log:0/0
-  1->2 MsgPreVote Term:2 Log:1/11
-  1->3 MsgPreVote Term:2 Log:1/11
-  INFO 1 received MsgPreVoteResp from 1 at term 1
-  INFO 1 has received 1 MsgPreVoteResp votes and 0 vote rejections
 > 2 receiving messages
   1->2 MsgDeFortifyLeader Term:1 Log:0/0
   1->2 MsgDeFortifyLeader Term:1 Log:0/0
   DEBUG 2 is not fortifying 1; de-fortification is a no-op
   1->2 MsgDeFortifyLeader Term:1 Log:0/0
   DEBUG 2 is not fortifying 1; de-fortification is a no-op
-  1->2 MsgPreVote Term:2 Log:1/11
-  INFO 2 [logterm: 1, index: 11, vote: 1] ignored MsgPreVote from 1 [logterm: 1, index: 11] at term 1: recently received communication from leader (remaining ticks: 3)
 > 3 receiving messages
   1->3 MsgDeFortifyLeader Term:1 Log:0/0
   1->3 MsgDeFortifyLeader Term:1 Log:0/0
   DEBUG 3 is not fortifying 1; de-fortification is a no-op
   1->3 MsgDeFortifyLeader Term:1 Log:0/0
   DEBUG 3 is not fortifying 1; de-fortification is a no-op
-  1->3 MsgPreVote Term:2 Log:1/11
-  INFO 3 [logterm: 1, index: 11, vote: 1] ignored MsgPreVote from 1 [logterm: 1, index: 11] at term 1: recently received communication from leader (remaining ticks: 3)
 > 2 handling Ready
   Ready MustSync=true:
   HardState Term:1 Vote:1 Commit:11 Lead:1 LeadEpoch:0
@@ -138,7 +125,7 @@ stabilize
 # All peers have been de-fortified successfully.
 raft-state
 ----
-1: StatePreCandidate (Voter) Term:1 Lead:0 LeadEpoch:0
+1: StateFollower (Voter) Term:1 Lead:0 LeadEpoch:0
 2: StateFollower (Voter) Term:1 Lead:1 LeadEpoch:0
 3: StateFollower (Voter) Term:1 Lead:1 LeadEpoch:0
 

diff --git a/pkg/raft/testdata/forget_leader_prevote_checkquorum.txt b/pkg/raft/testdata/forget_leader_prevote_checkquorum.txt
@@ -238,6 +238,18 @@ withdraw-support 1 3
 2 x 1 x
 3 x 1 1
 
+# At this point we can't campaign because we are not supported by a quorum.
+campaign 1
+----
+DEBUG 1 cannot campaign since it's not supported by a quorum in store liveness
+
+grant-support 3 1
+----
+  1 2 3
+1 3 1 x
+2 x 1 x
+3 3 1 1
+
 campaign 1
 ----
 INFO 1 is starting a new election at term 2

diff --git a/pkg/raft/testdata/fortification_support_tracking.txt b/pkg/raft/testdata/fortification_support_tracking.txt
@@ -23,13 +23,6 @@ withdraw-support 2 1
 2 x 1 1
 3 1 1 1
 
-withdraw-support 3 1
-----
-  1 2 3
-1 1 1 1
-2 x 1 1
-3 x 1 1
-
 campaign 1
 ----
 INFO 1 is starting a new election at term 0
@@ -81,11 +74,13 @@ stabilize
   Entries:
   1/11 EntryNormal ""
   Messages:
+  1->3 MsgFortifyLeader Term:1 Log:0/0
   1->2 MsgApp Term:1 Log:1/10 Commit:10 Entries:[1/11 EntryNormal ""]
   1->3 MsgApp Term:1 Log:1/10 Commit:10 Entries:[1/11 EntryNormal ""]
 > 2 receiving messages
   1->2 MsgApp Term:1 Log:1/10 Commit:10 Entries:[1/11 EntryNormal ""]
 > 3 receiving messages
+  1->3 MsgFortifyLeader Term:1 Log:0/0
   1->3 MsgApp Term:1 Log:1/10 Commit:10 Entries:[1/11 EntryNormal ""]
 > 2 handling Ready
   Ready MustSync=true:
@@ -96,13 +91,15 @@ stabilize
   2->1 MsgAppResp Term:1 Log:0/11 Commit:10
 > 3 handling Ready
   Ready MustSync=true:
-  HardState Term:1 Vote:1 Commit:10 Lead:1 LeadEpoch:0
+  HardState Term:1 Vote:1 Commit:10 Lead:1 LeadEpoch:1
   Entries:
   1/11 EntryNormal ""
   Messages:
+  3->1 MsgFortifyLeaderResp Term:1 Log:0/0 LeadEpoch:1
   3->1 MsgAppResp Term:1 Log:0/11 Commit:10
 > 1 receiving messages
   2->1 MsgAppResp Term:1 Log:0/11 Commit:10
+  3->1 MsgFortifyLeaderResp Term:1 Log:0/0 LeadEpoch:1
   3->1 MsgAppResp Term:1 Log:0/11 Commit:10
 > 1 handling Ready
   Ready MustSync=true:
@@ -111,10 +108,12 @@ stabilize
   1/11 EntryNormal ""
   Messages:
   1->2 MsgApp Term:1 Log:1/11 Commit:11
+  1->3 MsgApp Term:1 Log:1/10 Commit:11 Entries:[1/11 EntryNormal ""]
   1->3 MsgApp Term:1 Log:1/11 Commit:11
 > 2 receiving messages
   1->2 MsgApp Term:1 Log:1/11 Commit:11
 > 3 receiving messages
+  1->3 MsgApp Term:1 Log:1/10 Commit:11 Entries:[1/11 EntryNormal ""]
   1->3 MsgApp Term:1 Log:1/11 Commit:11
 > 2 handling Ready
   Ready MustSync=true:
@@ -125,18 +124,28 @@ stabilize
   2->1 MsgAppResp Term:1 Log:0/11 Commit:11
 > 3 handling Ready
   Ready MustSync=true:
-  HardState Term:1 Vote:1 Commit:11 Lead:1 LeadEpoch:0
+  HardState Term:1 Vote:1 Commit:11 Lead:1 LeadEpoch:1
   CommittedEntries:
   1/11 EntryNormal ""
   Messages:
   3->1 MsgAppResp Term:1 Log:0/11 Commit:11
+  3->1 MsgAppResp Term:1 Log:0/11 Commit:11
 > 1 receiving messages
   2->1 MsgAppResp Term:1 Log:0/11 Commit:11
   3->1 MsgAppResp Term:1 Log:0/11 Commit:11
+  3->1 MsgAppResp Term:1 Log:0/11 Commit:11
 
 print-fortification-state 1
 ----
 1 : 1
+3 : 1
+
+withdraw-support 3 1
+----
+  1 2 3
+1 1 1 1
+2 x 1 1
+3 x 1 1
 
 bump-epoch 2
 ----

diff --git a/pkg/raft/testdata/prevote_checkquorum.txt b/pkg/raft/testdata/prevote_checkquorum.txt
@@ -228,6 +228,18 @@ withdraw-support 1 3
 2 x 1 1
 3 x 1 1
 
+# At this point we can't campaign because we are not supported by a quorum.
+campaign 1
+----
+DEBUG 1 cannot campaign since it's not supported by a quorum in store liveness
+
+grant-support 3 1
+----
+  1 2 3
+1 2 1 x
+2 x 1 1
+3 2 1 1
+
 # Node 3 is now the leader. Even though the leader is active, nodes 1 and 2 can
 # still win a prevote and election if they both explicitly campaign, since the
 # PreVote+CheckQuorum recent leader condition only applies to follower voters.
@@ -264,9 +276,9 @@ stabilize
 withdraw-support 2 3
 ----
   1 2 3
-1 1 1 x
+1 2 1 x
 2 x 1 x
-3 x 1 1
+3 2 1 1
 
 campaign 2
 ----