fix(restart): block non-leader ports before the leader, demote only once all blocked - #1327
Merged
Merged
Conversation
…nce all blocked
On a primary restart the leader path (_recreate_lvstore_impl) blocked the
leader's LVS port first, suspended its replication, and only THEN blocked the
non-leader peers. That left a window -- between the leader's replication-disable
and the non-leader block -- in which a non-leader (typically the tertiary) kept
its LVOL listener open, accepted client IO, and redirected it through the
hublvol to a leader whose leadership was mid-transition, producing
writer_conflict events on the journal.
Reorder ### 3 so the non-leader peer ports are blocked FIRST, then the leader's
port + replication suspend, and only then the drain + leadership drop. Two
guarantees follow, both requested:
* non-leaders are blocked before the leader;
* the demote (jc_disable_replication -> drain -> bdev_lvol_set_leader
leader=False) starts only once EVERY port is blocked, so no peer is still
serving into a leader that is being demoted.
The non-leader block is moved, not duplicated; its skip conditions (leader,
disconnected, already-blocked) and abort-on-failure behaviour are unchanged.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…lock order The reorder (non-leaders blocked before the leader) changes this test's contract: a non-leader port-block failure now aborts BEFORE the leader is ever blocked, so the leader keeps serving (nothing to strand or unwind on it). The test previously asserted the leader was blocked-then-unblocked, which was the old leader-first order. It now asserts the leader is never blocked, and that any non-leader blocked before the failing one is unwound.
schmidt-scaled
added a commit
that referenced
this pull request
Sep 9, 2026
…nce all blocked (#1327) * fix(restart): block non-leader ports before the leader, demote only once all blocked On a primary restart the leader path (_recreate_lvstore_impl) blocked the leader's LVS port first, suspended its replication, and only THEN blocked the non-leader peers. That left a window -- between the leader's replication-disable and the non-leader block -- in which a non-leader (typically the tertiary) kept its LVOL listener open, accepted client IO, and redirected it through the hublvol to a leader whose leadership was mid-transition, producing writer_conflict events on the journal. Reorder ### 3 so the non-leader peer ports are blocked FIRST, then the leader's port + replication suspend, and only then the drain + leadership drop. Two guarantees follow, both requested: * non-leaders are blocked before the leader; * the demote (jc_disable_replication -> drain -> bdev_lvol_set_leader leader=False) starts only once EVERY port is blocked, so no peer is still serving into a leader that is being demoted. The non-leader block is moved, not duplicated; its skip conditions (leader, disconnected, already-blocked) and abort-on-failure behaviour are unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(restart): update non-leader-block-failure test to the new port-block order The reorder (non-leaders blocked before the leader) changes this test's contract: a non-leader port-block failure now aborts BEFORE the leader is ever blocked, so the leader keeps serving (nothing to strand or unwind on it). The test previously asserted the leader was blocked-then-unblocked, which was the old leader-first order. It now asserts the leader is never blocked, and that any non-leader blocked before the failing one is unwound. --------- Co-authored-by: michael <michael.schmidt1379@gmail.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
schmidt-scaled
added a commit
that referenced
this pull request
Sep 9, 2026
…nce all blocked (#1327) * fix(restart): block non-leader ports before the leader, demote only once all blocked On a primary restart the leader path (_recreate_lvstore_impl) blocked the leader's LVS port first, suspended its replication, and only THEN blocked the non-leader peers. That left a window -- between the leader's replication-disable and the non-leader block -- in which a non-leader (typically the tertiary) kept its LVOL listener open, accepted client IO, and redirected it through the hublvol to a leader whose leadership was mid-transition, producing writer_conflict events on the journal. Reorder ### 3 so the non-leader peer ports are blocked FIRST, then the leader's port + replication suspend, and only then the drain + leadership drop. Two guarantees follow, both requested: * non-leaders are blocked before the leader; * the demote (jc_disable_replication -> drain -> bdev_lvol_set_leader leader=False) starts only once EVERY port is blocked, so no peer is still serving into a leader that is being demoted. The non-leader block is moved, not duplicated; its skip conditions (leader, disconnected, already-blocked) and abort-on-failure behaviour are unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(restart): update non-leader-block-failure test to the new port-block order The reorder (non-leaders blocked before the leader) changes this test's contract: a non-leader port-block failure now aborts BEFORE the leader is ever blocked, so the leader keeps serving (nothing to strand or unwind on it). The test previously asserted the leader was blocked-then-unblocked, which was the old leader-first order. It now asserts the leader is never blocked, and that any non-leader blocked before the failing one is unwound. --------- Co-authored-by: michael <michael.schmidt1379@gmail.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
wmousa
added a commit
that referenced
this pull request
Sep 11, 2026
recreate_lvstore fences every non-leader peer's client port before it suspends the leader's journal replication (#1327 reordered ### 3 that way, correctly). But step (a) of that suspend loop calls wait_for_jm_rep_tasks_to_finish with its defaults -- retry=10/delay=20, so a single sleep is 20s -- against a FENCE_DEADLINE_SEC of 7.5s. The ambient fence RPC budget clamps the helper's RPCs but cannot clamp its time.sleep(). Measured worst case per attempt: 10 polls, 9 sleeps, 180s. 2026-09-10 14:32:20, LVS_13: a91b9596 was fenced, the leader 588fdb5b reported an active replication task, and the wait slept 20s. The next fence check -- the inflight drain, ~350 lines later -- aborted the restart at 20.087s. SPDK converts a port block to reject at ack_timeout * 4 = 8s, quiescing every qpair on the port, so the fenced peer's clients lost the path rather than waiting for it. With replication active on the leader that abort was certain, not a flake. Pay the patient wait before the window gate, where waiting costs no client IO, and leave only an unpaced confirmation poll (retry=1, delay=0: one poll, no sleep) inside the fence. A False return there is still not fatal -- jc_disable_replication remains the authority. A RAISING wait still aborts the restart with "replication-wait" in the message, exactly as it did from step (a): the contract moves with the call, it does not weaken. Failing there is strictly cheaper, since no port is fenced yet. Check the fence clock immediately after that poll, outside the try so _abort_restart_and_unblock's exception is not re-wrapped as "replication-wait failed", closing the stretch that went unchecked all the way from the non-leader fence to the drain. The helper's own defaults are unchanged: tasks_runner_port_allow wants the patient budget. tests/unit/test_restart_portblock_order.py located the leader-block section by the first match of "if current_leader and ... not in disconnected_peers:" -- which the new pre-fence guard also matches, and which sits earlier by design. Retargeted to the section's own first statement, _REPL_SUSPEND_MAX_ATTEMPTS; the seven existing assertions are unchanged and still pass with and without the fix. test_failover_failback_combinations expected the wait to run once per suspend attempt. It now runs once before the gate plus once per attempt, so the count moves 2 -> 3; the assertion says which is which. Added 7 unit tests. The 4 ordering ones fail on the unfixed tree: - the patient wait precedes any port fence (located by argument shape, not source text, so reformatting cannot silently disarm it); - every in-fence wait passes an explicit retry=/delay= override; - the fence clock is checked between the fence and the suspend; - that check is not inside the replication-wait try. The 3 behavioural ones pin retry=1/delay=0 to one poll and zero sleeps, and assert the default delay still exceeds FENCE_DEADLINE_SEC -- so if the override ever stops being load-bearing, a test says so. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
wmousa
added a commit
that referenced
this pull request
Sep 11, 2026
…1342) recreate_lvstore fences every non-leader peer's client port before it suspends the leader's journal replication (#1327 reordered ### 3 that way, correctly). But step (a) of that suspend loop calls wait_for_jm_rep_tasks_to_finish with its defaults -- retry=10/delay=20, so a single sleep is 20s -- against a FENCE_DEADLINE_SEC of 7.5s. The ambient fence RPC budget clamps the helper's RPCs but cannot clamp its time.sleep(). Measured worst case per attempt: 10 polls, 9 sleeps, 180s. 2026-09-10 14:32:20, LVS_13: a91b9596 was fenced, the leader 588fdb5b reported an active replication task, and the wait slept 20s. The next fence check -- the inflight drain, ~350 lines later -- aborted the restart at 20.087s. SPDK converts a port block to reject at ack_timeout * 4 = 8s, quiescing every qpair on the port, so the fenced peer's clients lost the path rather than waiting for it. With replication active on the leader that abort was certain, not a flake. Pay the patient wait before the window gate, where waiting costs no client IO, and leave only an unpaced confirmation poll (retry=1, delay=0: one poll, no sleep) inside the fence. A False return there is still not fatal -- jc_disable_replication remains the authority. A RAISING wait still aborts the restart with "replication-wait" in the message, exactly as it did from step (a): the contract moves with the call, it does not weaken. Failing there is strictly cheaper, since no port is fenced yet. Check the fence clock immediately after that poll, outside the try so _abort_restart_and_unblock's exception is not re-wrapped as "replication-wait failed", closing the stretch that went unchecked all the way from the non-leader fence to the drain. The helper's own defaults are unchanged: tasks_runner_port_allow wants the patient budget. tests/unit/test_restart_portblock_order.py located the leader-block section by the first match of "if current_leader and ... not in disconnected_peers:" -- which the new pre-fence guard also matches, and which sits earlier by design. Retargeted to the section's own first statement, _REPL_SUSPEND_MAX_ATTEMPTS; the seven existing assertions are unchanged and still pass with and without the fix. test_failover_failback_combinations expected the wait to run once per suspend attempt. It now runs once before the gate plus once per attempt, so the count moves 2 -> 3; the assertion says which is which. Added 7 unit tests. The 4 ordering ones fail on the unfixed tree: - the patient wait precedes any port fence (located by argument shape, not source text, so reformatting cannot silently disarm it); - every in-fence wait passes an explicit retry=/delay= override; - the fence clock is checked between the fence and the suspend; - that check is not inside the replication-wait try. The 3 behavioural ones pin retry=1/delay=0 to one poll and zero sleeps, and assert the default delay still exceeds FENCE_DEADLINE_SEC -- so if the override ever stops being load-bearing, a test says so. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
wmousa
added a commit
that referenced
this pull request
Sep 11, 2026
…1347) recreate_lvstore fences every non-leader peer's client port before it suspends the leader's journal replication (#1327 reordered ### 3 that way, correctly). But step (a) of that suspend loop calls wait_for_jm_rep_tasks_to_finish with its defaults -- retry=10/delay=20, so a single sleep is 20s -- against a FENCE_DEADLINE_SEC of 7.5s. The ambient fence RPC budget clamps the helper's RPCs but cannot clamp its time.sleep(). Measured worst case per attempt: 10 polls, 9 sleeps, 180s. 2026-09-10 14:32:20, LVS_13: a91b9596 was fenced, the leader 588fdb5b reported an active replication task, and the wait slept 20s. The next fence check -- the inflight drain, ~350 lines later -- aborted the restart at 20.087s. SPDK converts a port block to reject at ack_timeout * 4 = 8s, quiescing every qpair on the port, so the fenced peer's clients lost the path rather than waiting for it. With replication active on the leader that abort was certain, not a flake. Pay the patient wait before the window gate, where waiting costs no client IO, and leave only an unpaced confirmation poll (retry=1, delay=0: one poll, no sleep) inside the fence. A False return there is still not fatal -- jc_disable_replication remains the authority. A RAISING wait still aborts the restart with "replication-wait" in the message, exactly as it did from step (a): the contract moves with the call, it does not weaken. Failing there is strictly cheaper, since no port is fenced yet. Check the fence clock immediately after that poll, outside the try so _abort_restart_and_unblock's exception is not re-wrapped as "replication-wait failed", closing the stretch that went unchecked all the way from the non-leader fence to the drain. The helper's own defaults are unchanged: tasks_runner_port_allow wants the patient budget. tests/unit/test_restart_portblock_order.py located the leader-block section by the first match of "if current_leader and ... not in disconnected_peers:" -- which the new pre-fence guard also matches, and which sits earlier by design. Retargeted to the section's own first statement, _REPL_SUSPEND_MAX_ATTEMPTS; the seven existing assertions are unchanged and still pass with and without the fix. test_failover_failback_combinations expected the wait to run once per suspend attempt. It now runs once before the gate plus once per attempt, so the count moves 2 -> 3; the assertion says which is which. Added 7 unit tests. The 4 ordering ones fail on the unfixed tree: - the patient wait precedes any port fence (located by argument shape, not source text, so reformatting cannot silently disarm it); - every in-fence wait passes an explicit retry=/delay= override; - the fence clock is checked between the fence and the suspend; - that check is not inside the replication-wait try. The 3 behavioural ones pin retry=1/delay=0 to one poll and zero sleeps, and assert the default delay still exceeds FENCE_DEADLINE_SEC -- so if the override ever stops being load-bearing, a test says so. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
On a primary restart, the leader path (
_recreate_lvstore_impl, section ### 3) blocked ports in the wrong order:jc_disable_replication),That left a window between steps 2 and 3 in which a non-leader (typically the tertiary) still had its LVOL listener open, accepted client IO, and redirected it through the hublvol to a leader whose leadership was mid-transition →
writer_conflicton the journal.Fix
Reorder ### 3 to:
Two guarantees, both requested:
jc_disable_replication→ drain →bdev_lvol_set_leader(leader=False)) starts only once every port is blocked, so no peer is still serving into a leader that is being demoted.The non-leader block is moved, not duplicated — its skip conditions (leader / disconnected / already-blocked) and its abort-on-failure behaviour are unchanged.
Tests
tests/unit/test_restart_portblock_order.py(7) — non-leader block precedes the leader block; demote (repl-suspend + leadership drop) follows the non-leader block; leader block precedes the demote; abort-on-failure preserved; skip conditions preserved; no duplicate block. Plus 204 existing restart/ftt2 tests still green. Full suite 1874 passed / 7 skipped; ruff + mypy clean.Note
This is the leader-path counterpart of the same "tertiary serving during the leader flap" class as the port-fence work. It changes ordering only within the existing port-blocked window; the client-visible block span is unchanged (peers were blocked in the same window either way, just in a safer order).