raft: introduce/fix TestNodeWithSmallerTermCanCompleteElection #8288

irfansharif · Jul 20, 2017

TestNodeWithSmallerTermCanCompleteElection tests the scenario where a
node that has been partitioned away (and fallen behind) rejoins the
cluster at about the same time the leader node gets partitioned away.
Previously the cluster would come to a standstill when run with PreVote
enabled.

When responding to Msg{Pre,}Vote messages we now include the term from
the message, not the local term. To see why consider the case where a
single node was previously partitioned away and it's local term is now
of date. If we include the local term (recall that for pre-votes we
don't update the local term), the (pre-)campaigning node on the other
end will proceed to ignore the message (it ignores all out of date
messages).
The term in the original message and current local term are the same in
the case of regular votes, but different for pre-votes.

NB: Had to change TestRecvMsgVote to include pb.Message.Term when
sending MsgVote messages. The new sanity checks on MsgVoteResp
(m.Term != 0) would panic with the old test as raft.Term would be equal
to 0 when responding with MsgVoteResp messages.

+cc @bdarnell @xiang90.

bdarnell · Jul 20, 2017

bdarnell reviewed Jul 20, 2017

View changes

bdarnell · Jul 20, 2017

raft/raft.go

-		} else {
+			return nil
+		case m.Type == pb.MsgPreVote || m.Type == pb.MsgPreVoteResp:
+			// If we receive a MsgPreVote from a node with a lower term number,


Why is it important that we reject MsgPreVote instead of dropping it? We just drop MsgVote messages with older terms.

It isn't, it can be dropped as well. Rejecting it just puts the slower (previously partitioned) node into the ~~candidate~~ follower state (which would happen anyway when the faster node sends a MsgPreVote message with a higher term).
As an aside, why do we just drop MsgVote messages with older terms instead of rejecting them directly?

How does rejecting a message lead to a transition from pre-candidate to candidate?

As for why we drop instead of reject MsgVotes with low terms, I don't think there's any deep reason for it. It's just easier to drop since we drop all other messages with lower terms.

Mistyped. Meant transition back to follower state.

bdarnell · Jul 20, 2017

bdarnell reviewed Jul 20, 2017

View changes

bdarnell · Jul 20, 2017

raft/raft.go

+			// If we receive a MsgPreVote from a node with a lower term number,
+			// we reject it. For a MsgPreVoteResp we simply pass it to our
+			// stepFunc. For a stale MsgPreVoteResp (think of late responses,
+			// we've already transitioned to a leader or follower state) this


But if we've transitioned to follower and then back to pre-candidate, there's no other Term check in StepCandidate. We could accept a MsgPreVoteResp for term T as a valid pre-vote in term T+1.

there's no other Term check in StepCandidate

Isn't this then an existing problem for MsgVoteResp? We could accept granted MsgVoteResps from earlier elections? I see nothing in raft.go: *raft.poll that does this check.

No, because they're dropped by this block (the term check at line 713). Step handles all messages from other terms, so that the stepState methods can safely assume that the message is for the current term (with the exception of preVotes, which may have a higher term).

I see. Short of augmenting pb.Message (or repurposing pb.Message.Term, hacky & single purpose for me but might be the only option) I see no solution to this (unless I'm missing something). Say I have a three node cluster (A, B & C) where there is an asymmetric partition between A & C in that it C can receive incoming messages but cannot deliver outgoing ones to A. Consider the following:

A campaigns, send out a MsgPreVote to C

C responds with MsgPreVoteResp, doesn't deliver (yet)

A returns back to follower state (B wins election)
...

Timeout, A campaigns again (later term), sends out MsgPreVote to C

C responds with another MsgPreVoteResp, doesn't deliver (yet)
[partition heals]

A receives two MsgPreVoteResp messagess, cannot distinguish which one is in response to which election. (This is because C doesn't change terms when it receives MsgPreVote messages, pb.Message.Term in each MsgPreVoteResp message is simply C's term when it received said message.)

Ignored is the fact that in this specific example with the three node cluster necessarily C's term has increased between the two messages. Point stands where we still have no good way to determine what election cycle each MsgPreVoteResp message is from.

It's true that there's an ambiguity in pre-votes - because they don't increment the term, two consecutive pre-vote election cycles look identical. But it's OK if there are (rare) false positives here - all that will happen is that we'll continue to a full election, which is slightly disruptive but will guarantee that only an up-to-date node can become the new leader.

To clarify, you're OK with granted MsgPreVoteResp messages from earlier election cycles count towards the total # votes needed to pass the PreVote phase? (With a comment to this effect of course.)

Yeah. There are other possibilities for false positives too (for example, a node may grant pre-vote requests to two different nodes in the same term, but only one of them can win the real election).

When we send the MsgPreVoteResp at the end of r.Step, we use r.Term (which is out of date). This is the problem: the PreVoteResp is dropped because it is out of date when the (pre-)campaigning node receives it. Vote responses should use the term from the message, not the node's local term (these are the same for regular votes, but different for pre-votes).

bdarnell · Jul 20, 2017

When we send the MsgPreVoteResp at the end of r.Step, we use r.Term (which is out of date). This is the problem: the PreVoteResp is dropped because it is out of date when the (pre-)campaigning node receives it. Vote responses should use the term from the message, not the node's local term (these are the same for regular votes, but different for pre-votes).

PTAL.

irfansharif · Jul 20, 2017

PTAL.

bdarnell · Jul 20, 2017

bdarnell approved these changes Jul 20, 2017

View changes

bdarnell · Jul 20, 2017

raft/raft.go

+			// the message (it ignores all out of date messages).
+			// The term in the original message and current local term are the
+			// same in the case of regular votes, but different for pre-votes.
+			if m.Type == pb.MsgVote || m.Type == pb.MsgPreVote {


This condition is redundant, we're already in case pb.MsgVote, pb.MsgPreVote:.

Codecov Report

❗️ No coverage uploaded for pull request base (master@a64d15e). Click here to learn what that means.
The diff coverage is 100%.

@@            Coverage Diff            @@
##             master    #8288   +/-   ##
=========================================
  Coverage          ?   76.34%           
=========================================
  Files             ?      345           
  Lines             ?    27074           
  Branches          ?        0           
=========================================
  Hits              ?    20670           
  Misses            ?     4920           
  Partials          ?     1484

Impacted Files	Coverage Δ
raft/raft.go	`92.58% <100%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a64d15e...a92ceee. Read the comment docs.

codecov-io · Jul 21, 2017

Codecov Report

❗️ No coverage uploaded for pull request base (master@a64d15e). Click here to learn what that means.
The diff coverage is 100%.

@@            Coverage Diff            @@
##             master    #8288   +/-   ##
=========================================
  Coverage          ?   76.34%           
=========================================
  Files             ?      345           
  Lines             ?    27074           
  Branches          ?        0           
=========================================
  Hits              ?    20670           
  Misses            ?     4920           
  Partials          ?     1484

Impacted Files	Coverage Δ
raft/raft.go	`92.58% <100%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a64d15e...a92ceee. Read the comment docs.

LGTM. Thanks for fixing this!

xiang90 · Jul 25, 2017

LGTM. Thanks for fixing this!

irfansharif referenced this pull request Jul 20, 2017
Closed
storage: re-enable Raft PreVote RPC #16950

bdarnell requested a review from xiang90 Jul 20, 2017

bdarnell added the Raft label Jul 20, 2017

siddontang referenced this pull request Jul 25, 2017
Closed
raft: introduce/fix TestNodeWithSmallerTermCanCompleteElection #2078

This was referenced Jul 25, 2017

Closed

Update coreos/etcd cockroachdb/vendored#5

Merged

vendor: Update coreos/etcd; pick up pre-vote patch cockroachdb/cockroach#17210

lishuai87 referenced this pull request Aug 2, 2017
Closed
[DNM] raft: enable Pre-Vote #2110

irfansharif referenced this pull request Sep 5, 2017
Open
raft: lower test coverage for {CheckQuorum,PreVote} permutations #8498

siddontang referenced this pull request Jan 27, 2018
Closed
raft: introduce/fix TestNodeWithSmallerTermCanCompleteElection #21

etcd-io/etcd

Join GitHub today

raft: introduce/fix TestNodeWithSmallerTermCanCompleteElection #8288

Conversation

irfansharif commented Jul 20, 2017 • edited Edited 1 time irfansharif edited Jul 21, 2017 (most recent)

irfansharif referenced this pull request Jul 20, 2017

bdarnell reviewed Jul 20, 2017 View changes

This comment has been minimized.

This comment has been minimized.

irfansharif Jul 20, 2017 • edited Edited 1 time irfansharif edited Jul 20, 2017 (most recent)

This comment has been minimized.

This comment has been minimized.

bdarnell reviewed Jul 20, 2017 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

irfansharif Jul 20, 2017 • edited Edited 1 time irfansharif edited Jul 20, 2017 (most recent)

This comment has been minimized.

This comment has been minimized.

irfansharif Jul 20, 2017 • edited Edited 1 time irfansharif edited Jul 20, 2017 (most recent)

This comment has been minimized.

This comment has been minimized.

bdarnell commented Jul 20, 2017

This comment has been minimized.

irfansharif commented Jul 20, 2017

bdarnell approved these changes Jul 20, 2017 View changes

This comment has been minimized.

This comment has been minimized.

bdarnell requested a review from xiang90 Jul 20, 2017

bdarnell added the Raft label Jul 20, 2017

This comment has been minimized.

Codecov Report

codecov-io commented Jul 21, 2017

Codecov Report

This comment has been minimized.

xiang90 commented Jul 25, 2017

Hide details View details xiang90 merged commit 93826f2 into etcd-io:master Jul 25, 2017 5 checks passed

5 checks passed

siddontang referenced this pull request Jul 25, 2017

This was referenced Jul 25, 2017

lishuai87 referenced this pull request Aug 2, 2017

irfansharif referenced this pull request Sep 5, 2017

siddontang referenced this pull request Jan 27, 2018

irfansharif commented Jul 20, 2017 •

edited

Edited 1 time

irfansharif edited Jul 21, 2017 (most recent)

bdarnell reviewed Jul 20, 2017

View changes

irfansharif Jul 20, 2017 •

edited

Edited 1 time

irfansharif edited Jul 20, 2017 (most recent)

bdarnell reviewed Jul 20, 2017

View changes

irfansharif Jul 20, 2017 •

edited

Edited 1 time

irfansharif edited Jul 20, 2017 (most recent)

irfansharif Jul 20, 2017 •

edited

Edited 1 time

irfansharif edited Jul 20, 2017 (most recent)

bdarnell approved these changes Jul 20, 2017

View changes

xiang90 merged commit `93826f2` into etcd-io:master Jul 25, 2017
5 checks passed