Repository: nats-io/nats-server
Pull request #5315 (2024-04-15 15:32:27)

NRG: Only reset election timeout on vote request if granting
With the current behaviour, a node that is behind but running in the candidate state could cause nodes that are current to delay switching to the candidate state themselves because they keep resetting the election timer each time they receive a vote request.

Changing this to reset the election timeout only when granting means that we will allow other possibly more up-to-date nodes to transition to the candidate state faster.

This might explain why we've seen some unexpected delays in starting a new election after a leader has gone away.

Signed-off-by: Neil Twigg <neil@nats.io>


Patch:
diff --git a/server/raft.go b/server/raft.go
index 15d6ba8f..dea1c784 100644
--- a/server/raft.go
+++ b/server/raft.go
@@ -150,6 +150,7 @@ type raft struct {
 	pae     map[uint64]*appendEntry        // Pending append entries
 
 	elect  *time.Timer // Election timer, normally accessed via electTimer
+	etlr   time.Time   // Election timer last reset time, for unit tests only
 	active time.Time   // Last activity time, i.e. for heartbeats
 	llqrt  time.Time   // Last quorum lost time
 	lsut   time.Time   // Last scale-up time
@@ -1766,6 +1767,7 @@ func (n *raft) resetElectionTimeoutWithLock() {
 
 // Lock should be held.
 func (n *raft) resetElect(et time.Duration) {
+	n.etlr = time.Now()
 	if n.elect == nil {
 		n.elect = time.NewTimer(et)
 	} else {
@@ -3884,7 +3886,6 @@ func (n *raft) processVoteRequest(vr *voteRequest) error {
 	}
 
 	n.Lock()
-	n.resetElectionTimeout()
 
 	vresp := &voteResponse{n.term, n.id, false}
 	defer n.debug("Sending a voteResponse %+v -> %q", vresp, vr.reply)
@@ -3916,6 +3917,7 @@ func (n *raft) processVoteRequest(vr *voteRequest) error {
 		n.term = vr.term
 		n.vote = vr.candidate
 		n.writeTermVote()
+		n.resetElectionTimeout()
 	} else {
 		if vr.term >= n.term && n.vote == noVote {
 			n.term = vr.term


Test patch:
diff --git a/server/raft_test.go b/server/raft_test.go
index 4e01f6d8..a8b43190 100644
--- a/server/raft_test.go
+++ b/server/raft_test.go
@@ -350,3 +350,51 @@ func TestNRGStepDownOnSameTermDoesntClearVote(t *testing.T) {
 	require_ChanRead(t, lsm.lch, time.Second)
 	require_NotEqual(t, leader.vote, noVote)
 }
+
+func TestNRGUnsuccessfulVoteRequestDoesntResetElectionTimer(t *testing.T) {
+	c := createJetStreamClusterExplicit(t, "R3S", 3)
+	defer c.shutdown()
+	c.waitOnLeader()
+
+	nc, _ := jsClientConnect(t, c.leader(), nats.UserInfo("admin", "s3cr3t!"))
+	defer nc.Close()
+
+	rg := c.createRaftGroup("TEST", 3, newStateAdder)
+	rg.waitOnLeader()
+	leader := rg.leader().node().(*raft)
+	follower := rg.nonLeader().node().(*raft)
+
+	// Set up a new inbox for the vote responses to go to.
+	vsubj, vreply := leader.vsubj, nc.NewInbox()
+	ch := make(chan *nats.Msg, 3)
+	_, err := nc.ChanSubscribe(vreply, ch)
+	require_NoError(t, err)
+
+	// Keep a track of the last time the election timer was reset before this.
+	leaderOriginal := leader.etlr
+	followerOriginal := follower.etlr
+
+	// Now send a vote request that's obviously behind, so other nodes should
+	// not do anything with it.
+	vr := &voteRequest{
+		term:      follower.term,
+		lastTerm:  follower.term - 1,
+		lastIndex: 0,
+		candidate: follower.id,
+	}
+	require_NoError(t, nc.PublishMsg(&nats.Msg{
+		Subject: vsubj,
+		Reply:   vreply,
+		Data:    vr.encode(),
+	}))
+
+	// Wait for everyone to respond.
+	require_ChanRead(t, ch, time.Second)
+	require_ChanRead(t, ch, time.Second)
+	require_ChanRead(t, ch, time.Second)
+
+	// Neither the leader nor our chosen follower should have updated their
+	// election timer as a result of this.
+	require_Equal(t, leaderOriginal, leader.etlr)
+	require_Equal(t, followerOriginal, follower.etlr)
+}