restart: route dead-mgmt peers to takeover, shrink port-block budget - #996
Merged
Conversation
_check_peer_disconnected used only the data-plane quorum (NVMe controller state on surviving peers) to decide whether a peer was gone. That signal lags the mgmt view: after a peer goes OFFLINE, NVMe-TCP keep-alive (10 s) + loss timeout (1 s) + state propagation to each surviving peer can keep the quorum at "connected" for 10-30 s — well into the window when recreate_lvstore_on_non_leader tries to port-block the dead peer's mgmt API and hits ECONNREFUSED for 5 retries, ~140 s, then aborts the restart with a misleading "LVStore recovery failed" event. Fix (1): short-circuit to "disconnected" when FDB status is already OFFLINE / REMOVED / UNREACHABLE. Mgmt has observed the peer leaving the cluster; trust it. This routes the restart to the takeover path immediately — no wasted port-block attempts against a dead mgmt. IN_SHUTDOWN and RESTARTING are deliberately NOT in this list — those are transient states owned by the runner; preempting another node's leadership during its own restart would be incorrect. Those statuses fall through to the data-plane quorum as before. Fix (3): shrink port-block retry budget so the race with NVMe-TCP propagation is cheap when fix (1) doesn't apply (e.g. a fabric partition where mgmt still reports ONLINE). Block retries: attempts 5 → 3, FirewallClient(timeout=5, retry=5) → (timeout=3, retry=1), inter-attempt sleep 2 s → 1 s. Worst-case abort: ~15 s instead of ~140 s. Same shape for the port-unblock loop that runs after hublvol connect. Tests: tests/test_peer_disconnect.py pins the decision matrix (7 tests): each short-circuit branch asserts the data-plane quorum is NOT called; each fall-through branch asserts it IS called; the ONLINE-with-quorum-disconnect case asserts we still return True. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
PR #996 tightened the port-block retry budget from 5 to 3 in recreate_lvstore_on_non_leader. Update the pre-existing regression tests in test_failover_failback_combinations.py to match the new constant, and extend the docstrings to explain the why (cheaper abort when FDB short-circuit doesn't apply; outer retry loop then re-evaluates peer disconnection after NVMe-TCP keep-alive propagates). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
4 tasks
mxsrc
pushed a commit
to mxsrc/sbcli
that referenced
this pull request
Jul 27, 2026
Problem: in recreate_lvstore(is_takeover=True) at storage_node_ops.py line ~5418, the takeover leader was calling ``snode.create_hublvol()``, which internally issues bdev_lvol_create_hublvol(self.lvstore). But ``self.lvstore`` is the takeover node's OWN primary LVS — not the taken-over one — so the RPC tried to (re-)create self's own primary hublvol, hitting EEXIST on the bdev already created earlier in the same restart. Worse, even if that had been fixed, create_hublvol has no probe-before-create, so any restart retry was guaranteed to fail with the same EEXIST. The bug was latent until PR simplyblock#996: prior to that, routing to the takeover branch required _check_peer_disconnected to return True for the offline peer, which in turn required either (a) a stale jc_get_jm_status quorum to report disconnect (pre-PR simplyblock#993 it never did — mjm_stat is replication-health, not liveness), or (b) the dead-mgmt port-block to succeed (it didn't — ECONNREFUSED + 5 retries aborted first). Only after the FDB short-circuit in PR simplyblock#996 does the takeover path actually fire, exposing this. Fix: new StorageNode.adopt_hublvol(lvs_node, cluster_nqn): - bdev name = "{lvs_node.lvstore}/hublvol" — IDENTICAL to what the offline primary exposed, so clients preserve bdev identity across failover. - NQN = hublvol_nqn_for_lvstore(cluster_nqn, lvs_node.lvstore) — same shared NQN the offline primary used (multipath-friendly). - UUID / NGUID / model_number / nvmf_port reused from lvs_node.hublvol metadata so namespace identity is unchanged. - Probe-before-create: skips bdev_lvol_create_hublvol if the bdev already exists (idempotent across retries). - ana_state=optimized (takeover node is the NEW primary). - Does NOT mutate self.hublvol — two independent hublvols now coexist on the same SPDK instance: self's own primary and the adopted one, under distinct lvstore prefixes. Tests: - tests/test_hublvol_unit.py: 7 new cases pinning bdev-name target, idempotency, peer-metadata reuse, shared NQN, optimized ANA, self.hublvol non-mutation, and fail-loud when peer has no hublvol record. - tests/test_secondary_promotion.py: updated takeover assertion from ``create_hublvol.assert_called_once()`` to ``adopt_hublvol.assert_called_once()`` + verification that the offline primary is passed as ``lvs_node``. Co-Authored-By: Claude Opus 4.7 (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
1 participant
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.
Summary
_check_peer_disconnected: short-circuit to disconnected when FDB status isOFFLINE/REMOVED/UNREACHABLE. Mgmt ground-truth overrides the lagging data-plane quorum.(timeout=5, retry=5)× 2 s sleep (~140 s worst-case abort) → 3 attempts ×(timeout=3, retry=1)× 1 s sleep (~15 s worst-case).tests/test_peer_disconnect.py— 7 unit tests covering every status branch of the decision.Why
Observed on the live cluster during a dual-outage soak (restart task
a7d30699foraa5a2834):1dec421ewentunreachable → offlineat 20:21:36. 24 s later at 20:22:00,recreate_all_lvstoresevaluated_check_peer_disconnected(1dec421e)for Secondary-LVS routing. The data-plane quorum (PR #993's NVMe-controller-state version) still reportedremote_jm_1dec421e.ctrlrs[0].state == "enabled"on surviving peers — NVMe-TCP keep-alive had not yet propagated. Quorum returned "connected" → non-leader path → port-block against1dec421e's dead mgmt → ECONNREFUSED × 5 →_abort_and_unblock→"LVStore recovery failed"cluster event (misleading label — the actual cause is port-block timeout, not lvstore recovery).Each aborted iteration consumed ~3.5 min of the restart task's budget. Multiple iterations before either (a) the quorum catches up or (b) the task hits
max_retryand is re-queued.Fix (1): FDB short-circuit
FDB status transitions reliably via
StorageNodeMonitor's own_check_data_plane_and_escalatewell before NVMe-TCP keep-alive has propagated on every peer. If FDB saysOFFLINE/REMOVED/UNREACHABLE, the peer is gone — skip the quorum and go straight to takeover.IN_SHUTDOWN/RESTARTINGstay out of this list: those are transient states owned by the runner, and preempting another restart would be wrong.Fix (3): shorter port-block budget
Even with (1), there's a narrow race where mgmt hasn't yet observed the peer leaving but the fabric has. The original 140 s port-block budget was tuned for real network blips; for a true dead-mgmt scenario it's pure delay. 15 s is enough for a short FW hiccup to recover, and if it doesn't, the outer retry loop re-evaluates
_check_peer_disconnected— which by then will likely have caught up.Test plan
tests/test_peer_disconnect.py— 7/7 passing. Every status × quorum-outcome branch exercised.🤖 Generated with Claude Code