raft: don't ack MsgPreVotes when it cannot succeed

[irfansharif]

The changeset tightens the constraint around granted PreVotes.
Previously PreVote requests of the same term as the node's current term
would be granted despite there being no way for it to succeed
(MsgPreVotes currently specify the next term, if the next term of the requesting
node is the same as our current term, when subsequently calling for an
election it will fail). In this case we will now simply reject the vote.

---

While here we add a panic for a comment specified-constraint.

// The m.Term > r.Term clause is for MsgPreVote. For MsgVote m.Term
// should always equal r.Term.

TestRawNodeStep was in violation of this by sending in Msg{Pre,}Vote
messages with unspecified terms (defaulting to 0) when the recipient
node was at a higher term.

+cc @bdarnell @xiang90

[bdarnell]

LGTM, but can you add a test of a PreVote at the current term that would fail without this change?

[bdarnell]

LGTM, but can you add a test of a PreVote at the current term that would fail without this change?

[xiang90]

lgtm after adding the test @bdarnell suggested.

[xiang90]

lgtm after adding the test @bdarnell suggested.

[xiang90]

/cc @javaforfun would you like to help write a test for this code? i would like to get this merged as soon as possible.

[xiang90]

/cc @javaforfun would you like to help write a test for this code? i would like to get this merged as soon as possible.

[lishuai87]

seems no different to previous.
do you mean this?
(r.Vote == None || r.Vote == m.From) && (m.Type == pb.MsgVote || (m.Type == pb.MsgPreVote && m.Term > r.Term)) &&

[xiang90]

the difference that we do not grant prevote if m.term <= t.term like before.

[xiang90]

the if checking seems incorrect though. i am fixing it right now.

[bdarnell]

I'm looking at this again and the change doesn't make sense to me. It doesn't change the behavior of MsgPreVote - for MsgPreVote the condition was m.Term > r.Term both before and after the change. And adding the guard m.Type == pb.MsgPreVote doesn't really change anything because we know that for MsgVote, the terms are equal and the m.Term > r.Term condition will never be true.

The new code is slightly more explicit, so we might want to merge this as a cleanup, but I don't think there's a bug being fixed here. I've been unable to modify the test in #8571 to find any cases where this change makes a difference.

I do see one small bug: If m.Type == MsgPreVote && m.Term == r.Term && r.Vote == None && r.lead != None, then we will grant a PreVote when we shouldn't. However, there are not going to be enough nodes with r.Vote == None && r.lead != None to allow the PreVote to reach a quorum unless there have also been membership changes during the term.

I'll send a PR with some cleanups here. Testing the "leader without vote" scenario is difficult because it relies on membership changes, so unless you feel strongly I think I'll skip the test for now (I'm going to do some work on PreVote testing separately).