Fix/cluster activate stranded secondary node - #1210
Open
wmousa wants to merge 9 commits into
Open
Conversation
wmousa
force-pushed
the
fix/cluster-activate-stranded-secondary-node
branch
6 times, most recently
from
August 10, 2026 10:56
0d3fe0c to
6fa309b
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.
wmousa
force-pushed
the
fix/cluster-activate-stranded-secondary-node
branch
from
August 10, 2026 12:52
6fa309b to
5287fcd
Compare
…, 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.
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.
No description provided.