a NonVoter node should never be able to transition to a Candidate state #483
Conversation
There was a problem hiding this comment.
I think this looks good. It's a shame it's so hard to validate deterministically with a unit test.
This is probably another case for our "one day" refactor of this library to make it easier to mock and drive in a controlled way in unit tests. I don't think we should block this fix on that though as it seems pretty clear that it's never right for a non-voter to enter Candidate state.
…`HeartbeatTimeout` is reached
@banks reading your comment about testing I had the idea to test this at a more unit level to at least avoid a future regression and have the transition out of follower state defined in a test. Let me know what you think about that? |
| go env1.raft.runFollower() | ||
|
|
||
| // wait enough time to have HeartbeatTimeout | ||
| time.Sleep(tt.fields.conf.HeartbeatTimeout * 3) |
There was a problem hiding this comment.
I like having a test for this. I'm not sure how reliable this will be over time given that it's concurrent and timing dependent. Perhaps worth re-running this in CI a few times and seeing if it fails? We could increase the multiplier here if it does too I guess.
There was a problem hiding this comment.
I ran it locally few times, I will try on the CI. The wait is deterministic because the follower loop will run after HeartbeatTimeout and there is no concurrency other then the test and the runFollower loop ( because we avoid starting the raft routines) so the state is in a controlled environment.
There was a problem hiding this comment.
Sounds low risk then. In the past we've found even "safe" waits like this can eventually end up causing flakes in highly CPU starved CI environments where t.Parallel means that many more tests are running than real CPU cores available and so scheduling delays can last hundreds of milliseconds. If we also avoid running in parallel that probably helps too.
For now it seems OK just wanted to note that based on previous tests that have ended up flaky even 3 * heartbeat might not be enough in the long run!
There was a problem hiding this comment.
Yes in consul we have a lot of those. I will retry the CI a couple of times before merging to see how it hold.
PR #12130 refactored the test to use the `wantPeers` helper, but this function only returns the number of voting peers, which in this test should be equal to 2. I think the tests were passing back them because of a bug in Raft (hashicorp/raft#483) where a non-voting server was able to transition to candidate state. One possible evidence of this is that a successful test run would have the following log line: ``` [email protected]/raft.go:1058: nomad.raft: updating configuration: command=AddVoter server-id=127.0.0.1:9101 server-addr=127.0.0.1:9101 servers="[{Suffrage:Voter ID:127.0.0.1:9107 Address:127.0.0.1:9107} {Suffrage:Voter ID:127.0.0.1:9105 Address:127.0.0.1:9105} {Suffrage:Voter ID:127.0.0.1:9103 Address:127.0.0.1:9103} {Suffrage:Voter ID:127.0.0.1:9101 Address:127.0.0.1:9101}]" ``` This commit reverts the test logic to check for peer count, regardless of voting status.
PR #12130 refactored the test to use the `wantPeers` helper, but this function only returns the number of voting peers, which in this test should be equal to 2. I think the tests were passing back them because of a bug in Raft (hashicorp/raft#483) where a non-voting server was able to transition to candidate state. One possible evidence of this is that a successful test run would have the following log line: ``` [email protected]/raft.go:1058: nomad.raft: updating configuration: command=AddVoter server-id=127.0.0.1:9101 server-addr=127.0.0.1:9101 servers="[{Suffrage:Voter ID:127.0.0.1:9107 Address:127.0.0.1:9107} {Suffrage:Voter ID:127.0.0.1:9105 Address:127.0.0.1:9105} {Suffrage:Voter ID:127.0.0.1:9103 Address:127.0.0.1:9103} {Suffrage:Voter ID:127.0.0.1:9101 Address:127.0.0.1:9101}]" ``` This commit reverts the test logic to check for peer count, regardless of voting status.
This behaviour was observed in Consul for read-replicas nodes (which are raft nonVoters node). When the cluster is not stable (leader transition) there is a possibility to a nonVoter node to step up as leader (which is not supposed to happen)
@mkeeler and me where able to reproduce the behaviour of having a nonVoter node transition to a Candidate state, which is an indication that this node is possibly able to win an election. The test do the following steps:
appendEntrylog to the nonVoter to update the latest configuration and before it send theappendEntryto commit the new configuration, partition the leaderCandidatestateUnfortunately step 3 is very time sensitive and we were not able to come with a test that would consistently reproduce the issue but we were able to confirm that after the fix in this PR the issue could not be reproduced.