Fix/cluster activate stranded secondary node - #1210
Open
wmousa wants to merge 27 commits into
Open
Conversation
wmousa
force-pushed
the
fix/cluster-activate-stranded-secondary-node
branch
8 times, most recently
from
August 12, 2026 09:27
d397afb to
11564e2
Compare
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
wmousa
force-pushed
the
fix/cluster-activate-stranded-secondary-node
branch
from
August 12, 2026 21:03
4da1a09 to
6911e4b
Compare
…ation get_secondary_nodes() and get_secondary_nodes_2() each pair nodes one at a time via a greedy walk, preferring a domain/host-disjoint candidate from a shrinking shared pool. Nothing guarantees that walk closes a single cycle spanning every online node: it can close a cycle over a strict subset and strand the rest with zero candidates, even though a perfect pairing exists whenever there are 2+ online nodes (hit live: 12 nodes across 3 failure domains formed an 11-node secondary-pairing cycle, stranding the 12th and aborting activation with "No enough secondary nodes"). The tertiary assignment used by max_fault_tolerance >= 2 clusters (e.g. 2+2) has the identical structure and is subject to the same failure mode. Add splice_stranded_secondary() and splice_stranded_tertiary(): when a node is left with no candidates, splice it into an already-formed pairing edge (P->X becomes P->stranded->X) instead of giving up, preferring an edge where both sides differ from the stranded node's failure domain. The tertiary splice additionally re-validates host-disjointness against each side's own secondary partner, since a tertiary must be host-disjoint from both a primary and that primary's secondary. _cluster_activate falls back to these before raising, and only still fails if no pairing has been made at all yet.
_check_ftt_allows_node_removal gates node shutdown/suspend purely on raw not-online node count (cap = npcs), independent of the operator's drain-gate. This is stricter than necessary with failure domains enabled: placement guarantees at most one erasure-coding chunk per domain once there are ndcs+npcs distinct domains, so losing up to npcs domains at once is already tolerated the same way losing up to npcs nodes is tolerated without FD � but this check still blocked a second node in the same domain, undermining the drain-coordinator's FD-aware concurrency (observed live: a same-domain concurrent drain was correctly waved through by the operator's gate, then independently rejected here with "FTT=1: cluster already has 1 not-online node(s)"). With FD enabled and the target node's domain assigned, the capacity check now counts distinct affected domains instead of raw node count: piling onto an already-affected domain is always free; a new domain is gated on distinct-domain count against npcs when there are enough domains for full one-chunk-per-domain isolation, or falls back to the plain node-count cap otherwise (mirrors the operator's fdDrainGate). FD disabled or an unassigned node falls back to the original node-count logic unchanged. The npcs=2/ft=1 primary-secondary pairing constraint is unaffected.
…ndependent get_secondary_nodes/get_secondary_nodes_2/splice_stranded_secondary/ splice_stranded_tertiary each fetch their own candidate list fresh from the DB and scan/score it in that order, independent of whatever order the caller processes primaries in. With failure domains enabled, this made the resulting primary/secondary/tertiary assignment sensitive to arbitrary node ordering: even when a fully domain-disjoint assignment exists (e.g. equal- sized domains), ~1 in 5 arbitrary orderings left some node with a same- domain secondary or tertiary (verified by simulation), and the live deployment hit exactly this. All four functions now sort their fetched node list by failure_domain before scanning, which makes equal-sized domains fully order-independent (0 conflicts across 50 arbitrary orderings, verified). _cluster_activate's own pairing loop also sorts its processing order the same way: once domain sizes are uneven and splice-repair is required, the repair works off whatever partial assignment already exists, so the caller's processing order still mattered even with the candidate-scan fix alone. With both in place, unequal-domain conflict counts become deterministic instead of order-dependent. Both sorts are no-ops when failure domains are disabled.
…y check The previous rule treated piling additional nodes onto an already-affected failure domain as always free, only falling back to a raw node-count cap when opening a brand-new domain under-provisioned. Cross-checked against the backend team's confirmed tolerance (2 FD: one whole FD down OR one node in each FD, nothing else; 3 FD: one whole FD only; 4 FD: two whole FDs), that rule incorrectly allowed unsafe combinations such as one node in FD1 plus two nodes in FD2 on a 2-FD cluster. Each domain's worst-case contribution to a stripe's chunk loss is now capped at chunks_per_domain = ceil((ndcs+npcs) / domains_available). A domain already at or above that count has maxed its risk contribution, so further nodes in the same domain are free; otherwise the summed capped risk across all affected domains plus the node being removed must stay within npcs. This collapses to the existing "npcs whole domains free" behavior once there are >= ndcs+npcs domains, and reproduces the confirmed 2/3/4-FD tolerance exactly -- verified both by direct simulation of the formula and by driving the real function through every scheme/domain-count combination in tests/unit/test_ftt_protection.py.
…ctivation The bare correctness minimum for the interleaved rotation layout is npcs+1 distinct domains (2 for npcs=1, 3 for npcs=2) -- below that even the initial static placement is wrong: at exactly 2 domains the tertiary role mathematically always lands back in the primary's own domain, since "2 steps ahead" in a period-2 round-robin wraps to where it started (verified directly against rotation_layout: 8/8 tertiary placements landed same-domain at 2 domains, 0/12 at 3+). But a minimum-correct STATIC layout has zero spare hosts per domain, and the moment a single node is added or removed, the relocation logic (_pick_replica_relocation_node) has no spare candidate left to reassign the stranded role to. Verified directly: removing one node from a bare-minimum npcs=1/2-domain or npcs=2/3-domain layout strands another node's secondary/tertiary with _pick_replica_relocation_node returning None -- blocking the removal outright, not just degrading placement quality. This also matches the backend team's confirmed stance that a 2-FD layout can never absorb a second independent failure once one domain is down, so it's excluded at any npcs level. Fresh activation now hard-requires npcs+2 distinct domains (3 for npcs=1, 4 for npcs=2) -- one domain of spare capacity beyond the bare correctness floor, so a later single add/remove has somewhere to place the relocated role. Extracted as fd_activation_domain_count_violation() in planner.py (alongside fd_balance_violation, same pattern) since _cluster_activate itself has no unit-test mocking infrastructure and was otherwise untestable; 7 new tests cover the boundary directly.
… when no free cross-domain candidate exists get_secondary_nodes/get_secondary_nodes_2 only ever offer UNCLAIMED nodes (each node hosts at most one secondary/tertiary at a time). A node removal frees exactly one node cluster-wide -- whoever hosted the removed node's own role -- so _pick_replica_relocation_node has exactly one candidate to work with. If that one candidate lands in the wrong failure domain (or nothing is free at all), the direct search had nothing else to offer even though a valid rearrangement exists elsewhere in the cluster. Verified directly: two removals in a row (each individually fine) can chain into exactly this dead end -- the second removal's repair needs a new cross-domain home, the only free node is same-domain, and the search gave up. Confirmed the same 9-node/3-domain topology that hit this now resolves via the new fallback. Adds _find_splice_target_for_relocation, generalizing splice_stranded_secondary/splice_stranded_tertiary's fix for the identical dead end at cluster-activation time (splice into an already-formed pairing P->X instead of requiring an idle node) to the removal-repair path, with an exclude list for the node being removed. _pick_replica_relocation_node now falls back to it whenever the direct search comes up empty. Unlike the activation-time splice helpers -- which only ever run before any physical LVS exists -- this can be asked to splice into a pairing that already has real data on both ends, so _relocate_one_replica gained _relocate_replica_between to execute it: evict the existing occupant onto the node being relocated (tear down + rebuild), then claim the freed slot. Both legs follow the same idempotent, commit-pointers-then-build pattern _relocate_one_replica already uses, so a crash mid-splice resumes cleanly on retry.
_relocate_replica_between tore down the occupant's existing, healthy replica BEFORE building its replacement -- between those two steps the occupant had zero surviving copies. Under FTT1 (no tertiary) that's a real gap: a cluster only tolerates one node down at a time, and that budget belongs to the node actually being removed, not to whatever unrelated, healthy node the splice happens to touch. Confirmed live (2026-08-07): removing a node correctly triggered the splice fallback, but the rebuild step hit a hublvol attach failure and RAISED an exception instead of returning False. That propagated uncaught, the task retried repeatedly, and the occupant's only copy sat torn down the whole time -- the cluster's health monitor eventually suspended it. Reorders to create-before-destroy: build the replacement on the new host first (old copy stays live and serving throughout); only tear down the old copy once the new one is confirmed. A raised exception from the rebuild is now caught and treated the same as a returned False -- both leave the old copy untouched and safe to retry. The teardown step is guarded by the old host's own back-reference (not the occupant's forward pointer), so a crash between the two commits still resumes the teardown on the next pass instead of leaking a stale replica.
…, and stop masking an incomplete phase 5 on retry Two related bugs found live-testing FD-aware node removal's second (splice-fallback) path on a real cluster: 1. _connect_to_remote_jm_devs' fallback bdev-existence poll called rpc_client.get_bdevs() unguarded, right after the primary connect_device() failure had already been correctly degraded (logged, not raised). A transient DNS/RPC blip against the connecting peer's own SPDK-proxy hostname hit that second call too, but this one propagated -- raising RPCException out of _decommission_node_devices and killing the whole node-removal task. Now wrapped in a bounded retry (3 attempts, 1s apart, tenacity Retrying/RetryError, matching the existing pattern in tasks_runner_lvol_migration.py) so a blip that clears within a few seconds is caught transparently; only once that's exhausted does it degrade to "this JM not connected" (self-heals later via the periodic health-check service's topology-diff sweep) instead of raising. 2. node_removal_orchestrate's top-of-function guard treated `status == REMOVED` as "fully done" and returned True immediately. But phase 4 flips that status *before* phase 5 (device/JM decommission) runs -- so if phase 5 raised (as in bug 1, before the retry existed) after phase 4 had already committed, every resumed attempt hit this guard and reported "Node removed" without phase 5 ever actually completing. Now only phases 1/3a/3b/4 are skipped on resume; phase 5 always (re)runs -- it's already idempotent, so this is a no-op once it has genuinely finished. Observed live: an RPC connection error mid phase-5 JM reassignment left a peer's LVS un-rebuilt on its new host (bdev_lvol_get_lvstores "No such device") while the task still reported done, and the cluster cycled IN_ACTIVATION <-> SUSPENDED. Adds: - TestNodeRemovalOrchestrateResumesPhase5 (3 tests) - TestConnectToRemoteJmDevsDegradesOnRpcException (4 tests, incl. one verifying the bounded retry actually recovers a blip that clears within budget, and one verifying it still degrades gracefully once exhausted) Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ase 5 get_storage_nodes_by_cluster_id returns every node regardless of status, including ones already REMOVED. The JM-device peer-reassignment loop in _decommission_node_devices never filtered on that: an earlier-removed node can still carry the currently-removed node's JM id in its own jm_ids (never cleared on ITS OWN removal), so a later removal's phase 5 would try to "fix" that dead node's JM connections using its own rpc_client -- which points at a pod that no longer exists and can never resolve, let alone connect. 2026-08-11 incident: removing node A, then later removing node B (whose JM device A used to reference) sent phase 5 chasing A's permanently-dead hostname (NameResolutionError -> uncaught RPCException, same class of failure as the bounded-retry fix targets, but that retry can't save a hostname that will never resolve). B's own devices never reached failed/failed_and_migrated because the crash happened before the device loop further down in the same function, and the task still reported "done" on the next attempt (status already REMOVED short-circuits phase 1-4, per the earlier phase-5-resume fix) -- leaving B's devices stuck at unavailable with no task left to retry them. Skip node.status == STATUS_REMOVED outright in that loop -- a removed node's own bookkeeping is dead weight, not something to reconnect.
…es during teardown
…pant
_relocate_replica_between claimed new_host's lvstore_stack_secondary/
_tertiary slot for the evicted occupant but never wrote it -- and even
if it had, that slot was already occupied: every node in a full ring
already hosts exactly one other node's replica before any removal
starts, so a splice claiming it for a new occupant would silently
drop the pre-existing one (single-value field, can't hold both).
Confirmed live (2026-08-12): after a splice, sn list showed two
different primaries both pointing their secondary_node_id at the same
host. The physical build was fine -- SPDK doesn't mind a node hosting
a second lvstore -- but the pre-existing relationship's own
back-reference was never touched, leaving that replica untracked for
any future failover, and missing from lvstore_ports.
Fix, in _relocate_replica_between:
* Actually write new_host's backref + lvstore_ports entry for the
newly-placed replica (previously never written at all).
* Before claiming the slot, check whether it's already occupied by
an unrelated primary. If so, relocate that occupant first --
recursively, via this same function -- onto a fresh target. This
is a rotation, not a retry: _seen guards against a cycle, but the
rotation is otherwise always finite since each hop heads toward
the one slot the original removal freed. Fails closed (refuses
the whole splice) if the displaced occupant has nowhere to go,
rather than overloading the slot.
Also: _delete_replica_on_peer's callers now prune the torn-down
replica's stale lvstore_ports entry (_prune_stale_lvstore_ports) --
previously left in place indefinitely after a node removal, even
though (unlike the restart-reconnect path that intentionally keeps it
for port reuse) it was never coming back.
Added regression tests for both the secondary and tertiary (FTT2)
cases: the cascade correctly vacating a pre-existing occupant, and
failing closed when that occupant has no relocation target.
Case A/B relocation (_teardown_replicas_of_primary, _relocate_replicas_hosted_on) clears every forward/back-reference field on the removed node as each relationship moves elsewhere, but neither touches lvstore_ports -- it isn't part of any relocation, just a port-reuse cache for the node's own restarts (recreate_lvstore_on_non_leader). Confirmed live (2026-08-13): after removing a node, sn list kept showing its old LVS Ports entries indefinitely even though its SPDK process was gone and its status read removed -- there's no restart to reuse those ports for, since removal is terminal. _finalize_node_removal now clears it right before the node flips to REMOVED, alongside its other best-effort cleanup.
The DELETE /storage-nodes/{id} endpoint raised a bare ValueError when
remove_storage_node()/delete_storage_node() returned False. With no
registered handler for ValueError, FastAPI turns that into an
unhandled-exception 500.
Confirmed live (2026-08-13): the operator's drain reconciler suspends
a node, then calls this same DELETE to remove it. When sbcli refused
the removal (failure-domain balance would drop below the allowed
spread), the 500 response landed on the operator's *retryable* status
list (webapi/errorclass.go) instead of the non-retryable one -- so it
just requeued and retried the identical, permanently-doomed DELETE
forever, rather than reaching resumeAndFail and un-suspending the
node. The node sat suspended indefinitely with no self-healing path.
400 is correctly classified as non-retryable on the operator side, so
this alone fixes the stuck-suspended symptom without any operator
change. (remove_storage_node's precondition gates still only signal
via return False rather than a specific exception carrying the actual
reason string they already log -- that's a bigger refactor, not done
here.)
_decommission_node_devices' phase-5 JM cleanup only checked a peer's OWN jm_ids (its redundancy set for its own JM) to decide whether to fix it. But _connect_to_remote_jm_devs populates remote_jm_devices from a SECOND source too: whichever primary a node hosts as secondary/tertiary pulls in that primary's jm_ids as well, so the hublvol journal stays consistent with the primary being replicated. A peer reachable only through that second path never touches its own jm_ids at all, so the jm_ids-only guard never even looked at it -- its remote_jm_devices entry for the dead JM was left stale forever. Confirmed live (2026-08-14): after two node removals -- the first a plain relocation, the second triggering the splice-cascade fallback -- two peers had the second removed node's JM lingering in remote_jm_devices while their own jm_ids were already clean. The first removal never reshuffled who-hosts-whom, so it never exposed the gap; the splice's reshuffling did. Adds an elif branch: when a peer's remote_jm_devices contains the dead JM but its own jm_ids doesn't, refresh via _connect_to_remote_jm_devs (no replacement pick needed, unlike the jm_ids branch -- not a fixed-size redundancy slot, just a stale connection that a plain refresh naturally drops).
… its bdev stack _delete_replica_on_peer tore down the evicted peer's bdev stack for the primary's replica but never detached the peer's NVMe-oF controller consuming the primary's hublvol - that connection stays live the whole time the peer holds the replica, for fast failover. Left dangling, it can later be found wedged in a non-enabled state if the same peer is re-selected to host a replica of the same primary again before its next restart, and HublvolReconnectCoordinator's detach-and-wait-gone then times out, aborting the rebuild. Live sequence that hit this (2026-08-14): a splice eviction during one node removal moved a primary's replica off ffznh onto another peer, leaving ffznh's controller to that primary's hublvol connected but idle. A later, unrelated node removal re-selected ffznh to host that same primary's replica again; the stale controller's detach-wait-gone timed out, which killed SPDK on ffznh and marked it offline. The peer's own hublvol subsystem (its own exposed endpoint for this replica, dormant with no consumers) stays commented out per f5a052f - only the consumer-side controller detach is being added here, mirroring the existing pattern in teardown_non_leader_lvstore. Rewrote TestDeleteReplicaOnPeer, which pre-dates this session and still asserted the subsystem_get/subsystem_delete/bdev_lvol_delete_hublvol calls f5a052f commented out; it now covers the new detach call instead.
…ing a non-leader replica _delete_replica_on_peer called _remove_bdev_stack without remove_distr_only=True, so it always took the bdev_lvol_delete_lvstore branch -- destroying the shared on-disk blobstore metadata, not just the peer's local examine copy. teardown_non_leader_lvstore (the single-node-expansion sibling) already gets this right and spells out why in its docstring: the lvstore must never be deleted on a non-leader, since bdev_lvol_delete_lvstore wipes metadata shared by every replica. That's correct for _teardown_replicas_of_primary (Case A): primary IS the node being removed, so destroying its lvstore there is intentional -- nothing will ever read it again once its own devices are decommissioned. It's wrong for _relocate_replica_between's splice/relocation eviction: occupant_primary there is a SURVIVING node whose replica is only being moved to a new host. old_host held nothing but a non-leader examine copy of a still-live lvstore, and destroying it destroys the shared blobstore metadata out from under the primary and any other surviving replica. Live sequence that hit this (2026-08-16): a splice eviction moved vdr27's LVS_1 replica off 66gqf via this exact path, corrupting LVS_1's on-disk metadata. It surfaced later as "unsupported version on super block" (blobstore.c:bs_super_validate) when vdr27 tried to reload LVS_1 on restart -- by then indistinguishable from the leadership-loss symptom under investigation, and only traced back here afterward via log analysis. Added a destroy_lvstore parameter (default True, preserving Case A's existing/correct behavior) and pass destroy_lvstore=False from the splice eviction call site, threading through to _remove_bdev_stack(remove_distr_only=not destroy_lvstore) -- the same mechanism teardown_non_leader_lvstore already uses.
…ctor Surfaced by the rebase onto origin/main: mypy flagged SimplyblockCollector as not satisfying the CollectorRegistry.register() parameter type, which is typed against prometheus_client's Collector protocol/base class.
wmousa
force-pushed
the
fix/cluster-activate-stranded-secondary-node
branch
from
August 17, 2026 16:13
a8185b7 to
b812ee8
Compare
…removal status Do not cancel fail device migration task on node shutdown
Hamdy-khader
force-pushed
the
fix/cluster-activate-stranded-secondary-node
branch
from
August 17, 2026 17:28
b812ee8 to
1fcbd9d
Compare
…ust the storage-node bookkeeping _relocate_one_replica (normal Case B relocation) and _relocate_replica_between (the splice/rotation fallback) both moved a peer's secondary/tertiary replica from one host to another correctly at the storage-node level -- updating secondary_node_id/tertiary_node_id and the lvstore_stack_secondary/_tertiary back-references -- but never touched the nodes list of any LVol hosted on the surviving primary. lvol.nodes is a separate record from that storage-node bookkeeping: it's what the CSI/host initiator actually connects to for multipath failover. Leaving the old (now removed/vacated) host listed there strands every lvol hosted on that primary on a single path once the old host is gone, with nothing ever repointing it afterward -- confirmed live (2026-08-18): after a node removal relocated a peer's secondary replica elsewhere, an lvol's nodes field still named the just-removed node, and the NVMe-oF initiator showed only one live path (the primary) instead of two. The codebase already has the identical fix for the analogous case in cluster_expansion/executor.py (expansion-triggered rebalancing repoints lvol.nodes when a donor's role moves to a recipient) -- this was simply missing from the node-removal relocation path. Fix: a shared _update_lvol_nodes_for_replica_move() helper, called from both _relocate_one_replica and _relocate_replica_between right after the new replica's build is confirmed (and unconditionally, not gated on whether the build ran this pass, so a retry that resumes past an already-applied build still catches up if an earlier attempt crashed between the build and this step).
…'s lvstore_ports entry hasn't landed yet add_lvol_thread's per-lvstore port lookup used snode.get_lvol_subsys_port(lvol.lvs_name), whose fallback to the node's own lvol_subsys_port is correct ONLY when lvs_name is that node's own primary lvstore. For any other lvs_name, a missing lvstore_ports entry means the relocation that assigned this node a new non-leader role hasn't finished committing that bookkeeping yet -- not "no per-lvstore override configured". add_lvol_thread already has one documented "callers hold stale objects" guard (the in_deletion check at the top of the function, for a stale lvol); snode carried the identical hazard uncounted. Found live (2026-08-18): a node-removal splice relocated two lvols' secondaries onto new hosts. lvol.nodes was correctly repointed (the previous fix in this area), which woke lvol_monitor's repair loop for both almost immediately -- racing _relocate_replica_between's own lvstore_ports commit on the same new host. add_lvol_thread silently fell back to the new host's OWN leader port for both, and nvmf_subsystem_add_listener published a live listener on the wrong port. Nothing ever revisits or corrects it afterward: the CSI initiator correctly detects the subsystem is degraded (active=1 expected=2) and retries the connect indefinitely, but the wrong port is never reachable, so the secondary path never comes up. Fix: when lvol.lvs_name differs from snode.lvstore (this node is a non-leader host, not the lvstore's own primary) and lvol.lvs_name is missing from snode.lvstore_ports, re-fetch snode once and check again before trusting the port lookup. If it's still missing, refuse the registration instead of guessing -- the next lvol_monitor repair cycle retries once the commit has actually landed. The node's own leader-lvstore case (lvs_name == snode.lvstore, which legitimately has no lvstore_ports entry) is untouched -- get_lvol_subsys_port's existing fallback-to-node-default remains correct and unchanged for that case, and its own dedicated integration test (test_dual_ft_secondary_fixes.py) is unaffected.
…tems, not just its bdev stack _delete_replica_on_peer(destroy_lvstore=False) correctly tears down a vacated peer's local raid/distrib bdev stack for the lvstore it's giving up -- which cascades to remove each hosted LVol's namespace -- but never touches the per-LVol NVMe-oF subsystem+listener, which is registered separately (via add_lvol_thread) on top of that lvstore for every individual lvol. Left behind, the listener keeps accepting connections in front of a now-empty subsystem -- exactly the "live but no path" failure test_missing_namespace_ path_loss.py guards against, just reached through a different door this time: the CSI/host initiator's existing connection to the vacated peer stays live (the endpoint genuinely still answers), and since the peer is no longer in lvol.nodes after the previous fix in this area, nothing ever tells the initiator to drop that connection either. The volume ends up with a third, live-but-empty path sitting alongside its two correct ones indefinitely. Found live (2026-08-18) immediately after verifying the lvol.nodes and wrong-port fixes: both corrected lvols still carried this stale-but-live third path to their pre-relocation host, confirmed via nvmf_get_subsystems showing the listener present with "namespaces": []. Fix: a new _teardown_lvol_subsystems_on_vacated_peer() helper, called right after _delete_replica_on_peer in the splice-eviction path (_relocate_replica_between), deletes every LVol-of-the-relocated-primary's subsystem on the vacated peer via subsystem_delete(). Best-effort, matching _delete_replica_on_peer's own pattern: RPC failures are logged, not fatal. Not needed in Case A (_teardown_replicas_of_primary): the node being removed there is guaranteed to have zero LVols (enforced at remove_storage_node's entry precondition), so there is nothing to iterate.
…e_device.jm_bdev
_connect_to_remote_jm_devs() already resolves the correct name to connect
under for a replacement JM device: when override_name_on_node carries an
entry for this_node (set by _decommission_node_devices when a node's JM
gets replaced after a peer removal, so the consumer's already-built JM
raid doesn't need to change), the actual NVMe-oF connect uses that
override name via controller_name/expected_bdev. But remote_device.jm_bdev
was still hardcoded to org_dev.jm_bdev -- the JM owner's own natural name,
regardless of any override -- so the stored record didn't match what was
actually connected.
Found live (2026-08-19) immediately after a node removal relocated three
consumers' JM redundancy onto two replacement hosts: health_controller's
diagnostic controller lookup (line ~751, f'remote_{remote_device.jm_bdev}')
reads this field back to query bdev_nvme_get_controllers for logging
IP/port info, and with the stale natural name it queried a controller that
was never created, producing a spurious SPDK "ctrlr ... does not exist"
error on every health-check pass for as long as the override is in effect.
Not a false health-check failure today: the actual pass/fail gate in that
same function uses remote_device.remote_bdev (the real connected name),
which was already correct. The damage was log noise plus a silently-empty
diagnostic IP/port line -- but a field named jm_bdev that doesn't reflect
the name actually connected under is a landmine for the next piece of code
that trusts it (reconnect logic, another health gate, etc).
Fix: resolve the override BEFORE building remote_device, and use that same
resolved name for jm_bdev, expected_bdev, and controller_name alike.
Added TestConnectToRemoteJmDevsRecordsResolvedName (3 tests) to
tests/unit/test_node_removal.py: no-override keeps the owner's natural
name, an override for this consumer is recorded correctly, and an override
keyed to a different consumer doesn't leak in.
Systemic note: this is a second, independent defect in the same
override_name_on_node mechanism -- separate from (and not fixed by) the
open issue that _create_jm_stack_on_raid/_create_jm_stack_on_device build
a fresh JMDevice with no override_name_on_node carried forward, so a
replacement JM host's own restart silently drops the override. That one
still needs a restart to reproduce and is left for a follow-up fix.
…ds, not when the replacement host does override_name_on_node exists to keep an ALREADY-BUILT distrib/JM-raid on a consumer node pointed at a stable bdev name across a JM replacement elsewhere (a node's JM is removed, another node's JM stands in for it, but the consumer's raid member name can't be changed live -- see _decommission_node_devices). Nothing ever explicitly cleared that entry. It only ever disappeared as an accident of _create_jm_stack_on_raid / _create_jm_stack_on_device building the replacement host a brand-new JMDevice object on ITS OWN restart -- which drops override_name_on_node along with it, regardless of whether the consumer that entry names has rebuilt anything at all. That ties the override's lifetime to the wrong node's restart: - If the replacement host restarts before the consumer does, the override vanishes and the consumer's next reconnect (driven automatically by the replacement's own _reconnect_peers_to_restarted_node) switches to the replacement's natural name -- while the consumer's own distrib/raid, unchanged, still expects the old one. Live break, not a clean handoff. - If the consumer restarts first, its own full JM-set refresh and any distrib rebuild still consult the (untouched) override and keep reconnecting under the legacy name, even though it just rebuilt from scratch and had every chance to adopt the current one. Fix: _connect_to_remote_jm_devs gains drop_stale_overrides (default False, preserving existing behavior everywhere). Every call site that runs immediately ahead of the CONSUMER rebuilding its own JM-consuming construct from scratch -- node restart (_prepare_cluster_devices_on_restart), LVS recreate (recreate_lvstore_on_non_leader, _recreate_lvstore_impl), and create_lvstore (leader and secondary) -- now passes True: the override for THIS node is ignored (the current/natural name is used instead) and the stale entry is dropped from the replacement host's JMDevice via db_controller.atomic_update, so a concurrent override for a different consumer on the same replacement device is never clobbered. Every other caller (the decommission-time reconnect that freshly establishes the override, and DELTA reconnects where the consumer's own construct is unchanged) keeps the default and continues honoring it. get_node_jm_names, which builds the jm_names list actually baked into a freshly (re)created distrib, needed no change: at every rebuild call site the corresponding _connect_to_remote_jm_devs(..., drop_stale_overrides=True) call runs first in the same flow, so by the time get_node_jm_names reads the override it has already been cleared and naturally resolves to the current name. Added TestConnectToRemoteJmDevsDropsStaleOverrideOnRebuild (4 tests): a rebuild ignores the override and uses the owner's current name; the atomic_update mutate_fn removes only this consumer's entry, leaving a different consumer's override on the same device untouched; a DELTA reconnect (drop_stale_overrides left False) leaves the override and never calls atomic_update; and no-override present never calls atomic_update either.
…ond removal, not the removed node's own name _decommission_node_devices' replacement-picking branch always handed the newly-picked replacement JM device the REMOVED node's own natural jm_bdev name (removed_node.jm_device.jm_bdev) as the override for the affected consumer. That's only correct when removed_node was never itself standing in as an override for that consumer. It breaks on a two-hop chain with no restart in between: node A removed, node C picked as consumer B's replacement JM under override name "jm_A" (B's still-unrebuilt raid construct references that legacy name, never C's own). If C is then removed before B -- or C -- ever restarts, _decommission_node_devices(C) finds B still carrying C's JM id in B.jm_ids, picks a fresh replacement D, and previously handed D the override name "jm_C" -- C's own natural name, which B's construct never referenced at all (B only ever connected to C under "jm_A"). D would end up serving a name B never uses and never will, silently breaking that redundancy slot with no restart required to trigger it. Fix: check removed_node.jm_device.override_name_on_node for the consumer first, and chain that inherited name forward; only fall back to removed_node's own jm_bdev when removed_node was never itself a stand-in for this consumer (the normal, single-hop case, unchanged). Added two tests to TestDecommissionDevices: the baseline single-hop case (no prior chain -- replacement inherits removed_node's own name, as before) and the two-hop chain (replacement inherits the older "jm_A" name, not removed_node's "jm_n1"). Neither test previously existed for this branch at all -- the main replacement-picking path had no coverage of what name actually gets recorded.
…eaches by another path SPDK will not attach a second, distinctly-named local controller to a target it already has a live connection to under a different name. The override_name_on_node mechanism (a consumer keeps its already-built JM raid/journal member pointed at a legacy name after that name's owner is replaced) implicitly assumed the opposite: that a fresh alias to the replacement's target could always be attached. That assumption breaks whenever the consumer already reaches the picked replacement through some OTHER, unrelated path -- most commonly: the consumer hosts some OTHER primary's secondary/tertiary lvstore copy, and that other primary's own jm_ids set already includes the same replacement node. The consumer then has a live connection to the replacement's JM subsystem under the replacement's own natural name before the override was ever set. When _connect_to_remote_jm_devs later tries to attach the SAME target under the override's legacy alias, the attach RPC returns without a bdev name; _connect_to_remote_jm_devs' own fallback then silently reuses the pre-existing connection's name for remote_bdev while jm_bdev stays at the never-connected override name, and the DB ends up recording a healthy redundancy slot that was never actually live. Found live (2026-08-19), immediately after removing a node with zero lvols (the simplest possible removal): on the affected consumer, SPDK reported "Bdev name not returned from controller attach" for the override name, "Controller ... does not exist" on a direct query, and the JC layer was in an active retry loop -- "helper_sync_setter ... failed, JM is excluded from further operation" -- every few seconds, ongoing. Fix: when picking a replacement in _decommission_node_devices, skip any candidate the consumer already reaches via ANY path -- not just its own jm_ids -- by checking node.remote_jm_devices before picking. A candidate that's already connected can never serve as an override stand-in for this consumer, so it's never a valid pick regardless of how get_sorted_ha_jms ranks it. Also fixed the test fixture that surfaced this: tests/unit/test_node_ removal.py's _node() helper built each JMDevice() bare, and BaseModel. from_dict() binds a dict-typed field's default to the CLASS-level object itself when the field is absent from the constructor's data -- every bare-built JMDevice ends up sharing ONE override_name_on_node dict until something reassigns it. Production is unaffected (every DB read round-trips through from_dict(data) with the key present, which always constructs a fresh dict via dict(data[attr])), but the bare-constructed test fixtures aren't, and an in-place mutation in one test (exactly what the decommission code does) was bleeding into unrelated later tests via the shared class default. _node() now gives each JMDevice its own dict explicitly. Added a regression test: a "colliding" candidate already in the consumer's remote_jm_devices (but never in its own jm_ids) is skipped in favor of the next, genuinely unreached candidate.
…eaving the slot permanently unfilled
_decommission_node_devices' replacement pick, after the collision-avoidance
fix, could come up empty when every remaining candidate collides (the
consumer already reaches it via some other path). The only handling for
"no candidate" was logger.error(f"no jm_id found for {node.get_id()}") --
nothing else. That's worse than it looks:
- node.write_to_db() only ran inside the "found a candidate" branch, so the
node.jm_ids.remove(removed_node's dead id) a few lines earlier -- which
DID run unconditionally -- was never persisted either. The DB kept
referencing a JM device that no longer exists, forever.
- Nothing anywhere else revisits or retries this later: no reconciler, no
follow-up task. The slot stays silently short one redundancy member
until node itself happens to restart.
- It's invisible in normal tooling: a bare logger.error() never becomes a
cluster event (unlike e.g. device_status errors, which go through
storage_events.*() and show up in `sbctl cluster get-logs`).
Fix: when no collision-free candidate exists, fall back to a colliding one
rather than leaving the slot empty. This won't actually connect until
node's own next restart -- that restart's own full-refresh reconnect
(_connect_to_remote_jm_devs' drop_stale_overrides=True, from the prior fix
in this area) drops this exact stale entry and reconnects under the name
that's already live, self-healing cleanly. Until then it's a visible,
ongoing SPDK JC retry/exclude loop instead of an invisible, permanent gap --
visible-and-self-healing beats invisible-and-permanent. Also moved
node.write_to_db() to run unconditionally after the if/else, so the
.remove() is persisted even in the genuinely-empty case (no candidate at
all, not even a colliding one) instead of being silently discarded.
Added two tests: falling back to a colliding candidate when no clean one
exists (sets the override, appends to jm_ids, persists both writes), and
persisting the .remove() even when there's no candidate whatsoever.
…replace_jm Retire override_name_on_node entirely. It existed only because there was no way to swap a JM device backing a live JC (journal consistency) member without rebuilding the consumer's already-built distrib/JM-raid construct -- so a replacement JM was instead forced to answer under the removed peer's OLD bdev name, faking continuity. That naming trick was the root cause of five separate bugs fixed on this branch: the resolved name not being recorded on remote_device.jm_bdev, the override never being retired when its consumer rebuilt, the override not chaining across a second removal, a colliding candidate silently producing a phantom "connected" override that was never actually live, and a missing candidate silently dropping the node.jm_ids.remove() that should have been persisted. SPDK now ships jc_replace_jm(name_old, name_new): it swaps a live JC member in place and re-syncs the new JM's journal in the background. This removes the entire reason the naming trick existed: - _connect_to_remote_jm_devs always connects under the JM owner's own natural name now. Its drop_stale_overrides parameter (and the 6 call sites that passed it) is gone -- there's nothing left to drop. - _decommission_node_devices connects a picked replacement under its own name, then calls jc_replace_jm(name_old, name_new) on the consumer's own SPDK, where name_old is read back from the consumer's own remote_jm_devices record (already the resolved name, whatever it is -- no more chaining logic needed to guess it). - The collision-avoidance pick (skip a candidate the consumer already reaches via another path) stays, since jc_replace_jm rejects a name_new already in use by JC (-14) for the same underlying reason SPDK could never attach a second connection to it. But the old "fall back to a colliding candidate anyway, self-heal on the consumer's next restart" path is gone: jc_replace_jm would just reject it outright, so there's no point attempting it. No collision-free candidate now leaves the redundancy slot honestly short instead. - A jc_replace_jm failure (any code) leaves the slot short and best-effort detaches the now-unused connection it just made -- except for -14, where the bdev is legitimately already claimed by JC and detaching it would tear down something in active use. - get_node_jm_names() and JMDevice.override_name_on_node are gone; a node's jm_ids now always reflects the live JC membership directly. No mixed-version rollout path is needed: this assumes jc_replace_jm is available on every node's SPDK by the time this code runs.
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.
No description provided.