Enable pre-vote by default #7462
Conversation
| nodes=[node], log_capture=logs, timeout=1 | ||
| ) | ||
| if min_view is None or view > min_view: | ||
| if min_view is None or view >= min_view: |
There was a problem hiding this comment.
The comment above this says explicitly >= min_view, but the condition was >.
There are a couple of other places where we have a min_view, but then the condition requires >= min_view + 1...
This is the one place where it was clearly a mistake, so I've fixed it to align it with the variable name and the comment.
| wait_for_new_view(backup, r0.view, 4) | ||
| check_sessions_dead( | ||
| ( | ||
| # This session wrote state which is now at risk of being lost | ||
| client_primary_A, | ||
| # This session only read old state which is still valid | ||
| client_primary_B, | ||
| # This is also immediately true for forwarded sessions on the backup | ||
| client_backup_C, | ||
| ) | ||
| primary.wait_for_leadership_state( | ||
| r0.view - 1, | ||
| ["Follower", "PreVoteCandidate", "Candidate"], | ||
| timeout=2 * args.election_timeout_ms / 1000, | ||
| ) | ||
|
|
||
| # The backup may not have received any view increment yet, so a non-forwarded | ||
| # session on the backup may still be valid. This is a temporary, racey situation, | ||
| # and safe (the backup has not rolled back, and is still reporting state in the | ||
| # old session). | ||
| # Test that once the view has advanced, that backup session is also terminated. | ||
| wait_for_new_view(backup, r0.view, 1) | ||
| check_sessions_dead((client_backup_D,)) | ||
|
|
||
| # Wait for network stability after healing partition | ||
| network.wait_for_primary_unanimity(min_view=r0.view) | ||
| # Once we remove the network partition, a new primary will be elected in a higher term, | ||
| network.wait_for_primary_unanimity(min_view=r0.view + 1) | ||
|
|
||
| # This will break all of the previous sessions | ||
| check_sessions_dead( | ||
| ( | ||
| # This session wrote state to the partitioned primary which was at risk of being lost | ||
| client_primary_A, | ||
| # These sessions only read old state which is still valid | ||
| client_primary_B, | ||
| client_backup_C, | ||
| client_backup_D, | ||
| ) | ||
| ) |
There was a problem hiding this comment.
The partitioned nodes cannot become candidates as they cannot pass pre-vote. So we need to heal the partition before the sessions will break.
Co-authored-by: Eddy Ashton <[email protected]>
…rimary will not accidentally be deposed!
| @@ -0,0 +1,52 @@ | |||
| pre_vote_enabled,true | |||
There was a problem hiding this comment.
This test is a pre-vote enabled variant as the demo of the pre-pre-vote behaviour is still useful to be able to test.
| @@ -1,3 +1,4 @@ | |||
| pre_vote_enabled,false | |||
There was a problem hiding this comment.
This test seems to heavily rely on the pre-prevote behaviour. So for this test pre-vote is disabled.
There was a problem hiding this comment.
Pull request overview
This PR enables pre-vote by default in the Raft consensus implementation. Pre-vote is a Raft optimization that prevents unnecessary term increases by requiring candidates to win a "pre-vote" before calling a real election. This improves cluster stability and reduces disruption from partitioned or out-of-date nodes attempting to become leaders.
Key changes:
- Flipped the default value for
pre_vote_enabledfromfalsetotruein core state and test driver files - Updated numerous test scenarios to handle the new pre-vote phase in elections
- Refactored partition tests to accommodate the more stable election behavior with pre-vote
- Moved and improved the
force_become_primaryutility function frome2e_operations.pytopartitions_test.py
Reviewed changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/consensus/aft/impl/state.h |
Changed default value of pre_vote_enabled from false to true |
src/consensus/aft/test/driver.h |
Updated test driver to enable pre-vote by default |
src/consensus/aft/test/main.cpp |
Added test assertions for pre-vote message exchanges |
src/consensus/aft/test/committable_suffix.cpp |
Updated tests to dispatch pre-vote messages before election messages |
tests/raft_scenarios/* |
Updated test scenarios to assert PreVoteCandidate state and add explicit pre-vote enable/disable flags |
tests/partitions_test.py |
Refactored partition tests; added force_become_primary function; updated expectations for pre-vote behavior; contains leftover debug literals |
tests/infra/network.py |
Enhanced wait_for_node_commit_sync to accept optional nodes parameter; changed view comparison from > to >= |
tests/e2e_operations.py |
Removed force_become_primary and run_ledger_chunk_bytes_check functions (moved to partitions_test.py) |
| 51869 | ||
| 51893 |
There was a problem hiding this comment.
These numeric literals (51869 and 51893) appear to be leftover debugging code or accidentally committed values. They should be removed as they serve no purpose in the code.
| 51869 | |
| 51893 |
There was a problem hiding this comment.
@eddyashton what were these for originally?
| @@ -1,3 +1,4 @@ | |||
| pre_vote_enabled,false | |||
There was a problem hiding this comment.
This test I believe relies heavily on non-pre-vote behaviour. I'm currently investigating an equivalent which works for pre-vote.
2 participants
This PR follows up on the previous pre-vote PRs to enable it by default.
This closes #7361
Todos