Update CheckQuorum condition from quorum in any config to quorum in every active config #7375
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR addresses a bug in the CheckQuorum implementation where the leader's quorum check condition was inconsistent with the commit condition. The change ensures that a leader only remains active if it has a committing quorum in every active configuration, not just any configuration. This prevents scenarios where a leader believes it has quorum but is actually unable to commit entries.
Key Changes:
- Modified CheckQuorum logic in
raft.hto require quorum in all configurations instead of any configuration - Added test scenarios demonstrating the old incorrect behavior and validating the fix
- Introduced new test assertions (
assert_config,assert_missing_config) to verify configuration state in tests
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/consensus/aft/raft.h |
Changed CheckQuorum from OR logic (quorum in any config) to AND logic (quorum in every config) using std::all_of |
src/consensus/aft/test/driver.h |
Added assert_config and assert_missing_config helper methods for validating node configurations in tests |
src/consensus/aft/test/driver.cpp |
Registered the new assertion commands in the test driver command dispatcher |
tests/raft_scenarios/check_quorum_0_012 |
Test scenario demonstrating transition from single node to three-node cluster with CheckQuorum behavior |
tests/raft_scenarios/check_quorum_012_0 |
Test scenario demonstrating retirement of nodes while partitioned, verifying correct leader stepdown |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…e actual state of it during rollbacks
|
The reason getting trace validation to work required reducing the constraints on IsBecomeFollower is as follows: Actions from raft.h trace:
Required actions from Traceccfraft.tla pov:
So if IsBecomeFollower asserts that the spec state matches the raft.h state, and that it is unchanged, then IsReceiveAppendEntries's UpdateTerm will be in conflict with this. Hence to fix this specific issue we should not roll back the log or increment the term until the message is sent. |
achamayou
added
run-long-test
achamayou
changed the title
|
The change looks good to me, but holding off until the long tests, including TV have run. |
|
Passing verification runs: https://github.com/microsoft/CCF/actions/runs/18776933466 https://github.com/microsoft/CCF/actions/runs/18776933496, all looks good. The license/cla looks stuck again, I've kicked off another run with an update branch, if this is not in on Monday I will force merge it. |
|
Failures all look like they come down to I/O: Except a redirect issue in the long test: Which could be related to recently merged #7395? Unclear, needs investigation. |
|
Double checking I think this might be related directly or indirectly to long writes to disk. 
I've opened a PR to add time bound logging around those write calls. Regardless, at 15:26 the snapshot is requested, and the call fails with a 404. We should probably add info logging if we get a repro for the snapshot fetching. |
…very active config (microsoft#7375) Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: Amaury Chamayou <amchamay@microsoft.com>
…very active config (microsoft#7375) Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: Amaury Chamayou <amchamay@microsoft.com>
|
Adding backported label - this was backported in #7436. |
3 participants
We have CheckQuorum to ensure that a leader should step down if it is not a good leader, as this acts as a good liveness probe of the system at large.
Our CheckQuorum condition is that the leader has a committing quorum in any configuration.
Our commit condition is that the leader has a committing quorum in every configuration.
This PR includes a scenario (check_quorum_2) which demonstrates the difference between these two conditions.
Suppose we have a 3 node cluster with n0 the leader initially, and then retire both n1 and n2 but do not replicate this to them.
Then we partition n0.
Due to CheckQuorum n0 is still a good leader, however it is unable to commit.
While the remainder of the cluster elects a new replacement leader and continues to function.
The aim of the change to raft.h is to make clear what the condition is, as well as fixing this.
Note
The other direction of this bug would be more severe, (going from n0, to n0,n1,n2, check_quorum_3) with n0 being inaccessible but staying alive due to CheckQuorum.
But the backups are unable to elect themselves without a vote from n0, so this isn't a problem in practise.