Fix/fd full diversity relocation - #1304
Open
wmousa wants to merge 23 commits into
Open
Conversation
cluster_grace_shutdown iterated every node record with no status filter at all and force-shut-down each one. shutdown_storage_node drives in_shutdown -> offline, so a node that had been REMOVED came back as a plain offline member. Live 2026-09-03: a single graceful-shutdown resurrected all four nodes removed earlier that day, and the records had to be repaired by hand before the cluster could be activated again. That is not cosmetic. failure_domain_host_map skips only STATUS_REMOVED, so the resurrected records immediately start occupying failure-domain host slots again -- the cluster went from 8 hosts at 2/2/2/2 to 12 at 3/3/3/3 -- and a later activation or startup then acts on nodes whose devices are already failed_and_migrated and which own no lvstore. REMOVED and IN_REMOVAL are now skipped; IN_REMOVAL because node_removal_orchestrate has already shut that node down and owns the rest of its lifecycle. PENDING_REMOVAL deliberately is not skipped: the node is still up and serving then, so a full-cluster shutdown must stop it like any other member. The test asserts on which nodes were swept rather than on a return value -- the function returns None, so behaviour is only observable through the calls it makes. Verified by mutation: emptying the status tuple fails two of the three cases.
…starts auto_restart_disabled is enforced at ENQUEUE time only (tasks_controller.add_node_to_auto_restart, "the single chokepoint for every auto-restart queue path"); tasks_runner_restart never consults it. So an FN_NODE_RESTART row queued BEFORE the flag was set survives, executes unconditionally, and its ONLINE transition clears the flag again -- the deliberate-stop intent destroyed by the very task it was meant to prevent. Live 2026-09-03: a cluster graceful-shutdown returned successfully having left s7457 and zdgtb ONLINE, because two such rows fired seconds after the sweep passed them. Both had to be shut down by hand. shutdown_storage_node now reaps those rows where it commits the intent, mirroring what set_node_status already does on the opposite transition (ONLINE cancels obsolete restart rows). This also gives the runner its dequeue-side check for free -- it already honours task.canceled -- with no new field, and without breaking ensure_node_restart_task, which deliberately bypasses the flag because an explicit `sn restart` is the operator intervention the flag waits for. A restart queued after that point is exactly that and is left alone. cancel_pending_node_restart_tasks gains exclude_task_id, and that exclusion is load-bearing rather than defensive: the restart runner drives shutdown_storage_node as its own kill step (tasks_runner_restart.py:542, passing current_restart_task_id), so a blanket cancel would abort the very restart doing the shutting down -- turning a fix for a rare race into a failure on every node restart. The suspend-recovery path is unaffected; it passes keep_auto_restart=True and never reaches this block. cluster_grace_shutdown additionally verifies its own end state instead of assuming it. The sweep is serial, so a node it already passed can come back behind it; stragglers get one more shutdown, and anything still not offline is logged by name. It does not raise -- an operator following up needs the list, not a traceback. Mutation-verified: dropping the exclusion fails the own-task test, disabling the settle pass fails the comes-back test.
…cation
_pick_replica_relocation_node previously only guaranteed >=1 cross-domain
role between {primary, secondary, tertiary}: once the OTHER already-
assigned role happened to be cross-domain from the primary, the role
actually being relocated was placed on cands[0] with zero domain check,
so it could land in the primary's own domain or share the other role's
domain. Live-confirmed on the 12-node/4-domain test cluster (2026-08-27):
several nodes ended up with secondary and tertiary sharing a domain after
two uneven removals.
Now tries, in order: (1) a direct candidate diverse from both the primary
and the other role, (2) splicing into an existing pairing whose far end
is also diverse from both (_find_splice_target_for_relocation gained an
avoid_domains parameter for this), (3) relax to the old >=1-cross-domain
floor -- direct then splice -- only when full diversity is unreachable
anywhere, logged via logger.warning so the degraded outcome is visible,
(4) the original same-domain-blind fallback when there's no other role to
diverge from or FD data is invalid.
Adds 5 regression tests covering each tier plus the no-warning case when
there's no other role assigned yet.
…s no name_old The 'no recorded bdev name for removed JM; cannot call jc_replace_jm' error only reports the absence of a matching remote_jm_devices entry -- it can't distinguish 'this node never connected to that JM at all' from 'a connection existed and was dropped by something else'. Log the current remote_jm_devices uuids (empty vs non-empty-but-missing-this-one is the key signal) and the affected (jm_vuid, owner_primary) targets so a live occurrence is diagnosable without re-deriving the state from scratch.
_find_splice_target_for_relocation's avoid_domains only ever protected the far end (X, the node the stranded primary ends up hosted on) -- it had no way to keep the near end (P, whose <field> pointer gets repointed from X onto the stranded primary) diverse from its OWN other, untouched role. Live-confirmed on the 12-node test cluster (2026-08-27): splicing kc25l into 56mg5's secondary slot collided with 56mg5's pre-existing, untouched tertiary in the same domain, degrading a node that had nothing to do with the removal -- while another edge elsewhere in the ring (t74sg's) was collision-free the whole time and simply never got a chance to be preferred. Ranks edges by (does P stay diverse afterwards, existing domain-mismatch score) instead of score alone, so a collision-free edge always wins when one exists. A colliding edge is still accepted, with a warning, when it's the only option -- this is a preference to steer around collateral damage, not a new hard requirement, since an outright reject would be more restrictive than the old behavior in exactly the cases where no better edge exists. Adds 4 regression tests: prefers the collision-free edge over a higher-raw-score colliding one, falls back to a colliding edge with a logged warning for both secondary and tertiary roles when it's the only edge available, and confirms no warning fires on the ordinary non-colliding path.
…lision The splice preference fix (previous commit) steers toward a collision-free edge when one exists, but still accepts a colliding edge as a last resort, leaving the occupant with a real domain-diversity gap. This closes that gap actively instead of just warning about it. Adds _repair_occupants_other_role_after_splice, called right after a splice succeeds in _relocate_one_replica: by that point occupant.<role> already points at primary, so calling the existing picker (_pick_replica_relocation_node) for occupant's OTHER role automatically avoids both occupant's own domain and primary's domain -- no new plumbing needed, since that's exactly the field the picker already reads to compute what to avoid. Because the picker itself tries a direct candidate before a further splice, and _relocate_replica_between already handles a busy target via its own eviction rotation, this one call transparently covers both cases -- a free replacement exists, or occupant's other role itself needs splicing into a further pairing -- reusing all the existing machinery one hop further out, rather than a new bespoke cascade. Best-effort, not blocking: if no replacement is found or the relocation itself fails, occupant is left with the collision and a warning is logged, same as before -- this is not itself recursive beyond one hop (a new collision introduced by fixing occupant's other role is not chased further). Adds a wiring test confirming _relocate_one_replica calls the new function after a successful splice, plus 6 tests covering the repair function's own decision logic (collides/doesn't, both roles, no replacement found, relocation failure, FD disabled, no other role set).
…earch Live-stuck finding (2026-08-28, 12-node/4-domain cluster, 4th removal cycle): a splice can chain into a nested vacate (stranded already hosts z, whose slot must be freed before occupant claims it) whose ONLY candidate turns out to be occupant itself -- the primary currently being relocated onto stranded in the SAME outer call. That's structurally circular: occupant can't simultaneously be the thing moving onto stranded and the target z vacates onto. The existing guard correctly refused it (`vacate_target == occupant_primary_id`), but then just gave up rather than searching past it -- and since nothing about the DB state changes between attempts, the outer retry loop repeated the identical failure every ~10s indefinitely, live, with no path to progress. Adds `extra_exclude_ids` to _pick_replica_relocation_node (folded into its existing exclude_ids list, so every candidate path -- direct search, both splice tiers -- respects it automatically with no other code change). _relocate_replica_between's nested vacate now passes the in-flight occupant_primary_id through this parameter, so the picker looks past it instead of finding it, returning None, or looping. Adds a direct test confirming extra_exclude_ids reaches the candidate search, and a wiring-level test reproducing the exact three-way chain (occupant -> stranded splice, stranded's pre-existing occupant z's only candidate is occupant) confirming the exclusion is passed and z is relocated onto a real alternative instead of dead-ending. Also fixes 3 pre-existing test fixtures whose _pick_replica_relocation_ node side_effect signatures didn't accept the new keyword argument.
…'s domain Live finding (2026-08-28, same 4th-removal cycle as the previous commit): p59j8 ended up with its own tertiary pointed at v56vg -- both domain 4. Once a splice executes, P.<field> is repointed onto stranded_primary itself (see _relocate_replica_between): P's role-target BECOMES stranded_primary. If P's own domain matches stranded_primary's, P now holds a role-target in its own domain -- the most basic diversity violation there is, exactly as fundamental as X's domain (already hard-excluded via avoid_domains). This dimension was only ever soft-scored via _domain_mismatch_score, predating all of today's diversity work, and let a live splice pick an edge where P's own domain collided outright with no warning at all (unlike the other two soft-preference cases, which at least log when they settle). Hard-excludes it unconditionally (not gated behind avoid_domains, so it applies at every _pick_replica_relocation_node tier) -- unlike P's OTHER role (still a soft preference with a last-resort fallback), there is no legitimate "nothing better exists, degrade P a little" case here: this is the same guarantee already enforced for X, just never enforced for P. Refusing the relocation outright when this is the only option is the correct failure mode -- degrading an uninvolved node is worse. Fixes two pre-existing test fixtures whose premise (P's own domain matching stranded's, expected to still win via the other-role preference) is now correctly hard-excluded before that preference is even considered; adjusts their domains to isolate what each actually tests. Adds a direct regression test confirming the exclusion holds even when it's the only candidate edge (no fallback).
… at a time
_pick_replica_relocation_node answers "where does THIS one stranded replica
go". In a shrinking cluster that question often has no answer: a removal
frees exactly one secondary slot and one tertiary slot cluster-wide, so the
picker's candidate list has at most one entry, and if that node sits in the
primary's own domain -- or in the domain of the role it is NOT relocating --
it cannot fix it, because it never looks at the other role. It splices into
a third party's pairing, patches one hop of collateral damage, and failing
that relaxes to the weak ">=1 cross-domain role" floor with a warning.
The repair a shrinking cluster actually needs is swapping two replicas that
are BOTH already placed. That is not expressible one stranded role at a
time, at any tier. Live-confirmed on the 4-domain x 3-host FTT2 cluster:
removing one host per domain accumulated one relaxation per removal, leaving
several LVSs with secondary and tertiary sharing a domain -- while a fully
diverse layout existed at every single step.
Adds simplyblock_core/controllers/replica_placement.py, a pure planner that
solves the whole post-removal layout at once. Because
lvstore_stack_secondary/_tertiary are single-valued, "who hosts whose
secondary" is a permutation, not a bag of independent choices -- which makes
this a min-cost perfect bipartite matching (Hungarian, O(n^3), trivial at
cluster scale):
* hard edges: role never on its own primary, host-disjoint, and
fd(P)/fd(S)/fd(T) pairwise distinct;
* cost: staying put 0, moving MOVE_COST -- so the layout returned is the
minimum-rebuild one, not merely a valid one;
* secondary and tertiary couple only through the secondary's DOMAIN, so
two stages with an exact Hall's-condition test between them
(tertiary_blocking_pairs) and a penalty retry that steers stage 1 away
from a pattern that would strand stage 2;
* order_moves emits moves that each land on a slot proved free at that
point, breaking rotation cycles up front with a scratch hop through the
one slot the removal frees -- _relocate_replica_between's recursive
vacate cannot resolve a cycle at all, it hits its own cycle backstop.
full_diversity_violations is the invariant that was missing: unlike
planner.compute_fd_layout_violations, which only asserts ">=1 cross-domain
role" and therefore PASSES a layout whose secondary and tertiary share a
domain, it checks pairwise distinctness -- so the degraded state this bug
produced is now expressible as a failing assertion.
Wired into phase 3b (_relocate_replicas_hosted_on) and into admission
(_check_replica_relocation_feasible) via the same inputs, so the two can
never disagree: a removal is refused only when no host-disjoint layout
exists at all, and a merely-not-fully-diverse outcome is admitted with every
affected LVS named. The planner declines -- falling back to the existing
per-replica path, unchanged -- when failure domains are off, a node has no
domain, a dedicated is_secondary_node exists (it may host more than one
replica, breaking the permutation model), or a peer is not ONLINE.
When full diversity is genuinely unreachable it is reported rather than
silently relaxed. Verified by brute force: 3 domains at FTT2 needs balanced
domains (survivors 3/3/2 is unsatisfiable -- every 0<->1 pairing's tertiary
must fit domain 2's two slots); 2 domains at FTT2 never works. At 4 domains
-- the FTT2 minimum -- all 25 size splits the +/-1 admission rule permits are
feasible, so the planner's feasible region and check_fd_admission_for_remove
line up exactly, with no gap where admission says yes and placement has to
degrade.
Coverage: 40 planner tests (matching optimality vs brute force, forbidden
edges, diversity checking, feasibility conditions, move ordering, cycle
breaking, randomised end-to-end properties) plus 15 wiring tests, including
the reported 12-node/4-domain shrink driven through the DB bookkeeping for
all 24 removal orders. The _node fixture now defaults is_secondary_node and
physical_label to the real model's values -- as truthy child mocks they
silently steered placement down the dedicated-secondary branch.
…ll needs A node's secondary replica and its tertiary replica OF THE SAME PRIMARY are not two resources -- they are one physical stack, raid0_<vuid> + LVS_<vuid>, keyed by the primary's lvstore and not by the role. _relocate_replica_between's teardown guard consulted only the backref of the role being vacated, so vacating one role deleted the stack the other role was still using: _delete_replica_on_peer ends in _remove_bdev_stack(remove_distr_only=True) -> bdev_raid_delete(raid0_<vuid>), which hot-removes the lvstore from that node outright. The global planner made this reachable on essentially every removal. Solving the layout at once is the whole point of replica_placement, and it routinely emits both of a primary's roles in a single removal; order_moves proves each move lands on a free SLOT, but slots are the wrong granularity -- for one primary, a node's secondary slot and tertiary slot are the same stack. The old per-replica picker relaxed to the weak floor instead of producing this shape, which is why it never surfaced before. Found live 2026-09-01 on the 12-node/4-domain FTT2 cluster, on the first two removals and in an identical shape both times. For pq8h9/LVS_45 the plan was "secondary: 94dht -> fvgtl" (promoting fvgtl, which already held that primary as tertiary) then "tertiary: fvgtl -> nq2mm": 14:50:26 nq2mm bdev_raid_create raid0_45 14:50:27 fvgtl bdev_raid_delete raid0_45 <- the new secondary 9s25f/LVS_1 went the same way one removal later (kszbd promoted, raid0_1 deleted 2s after qs4dd's was built). Both primaries were left recorded as FTT2 while physically down to a single replica -- their tertiary -- with the recorded secondary holding nothing but a stranded LVS_<vuid>/hublvol controller. The failed jc_replace_jm and the leftover remote_jm_devices entry observed alongside are downstream of this: the JC instance outlived the lvstore it belonged to. Guards the teardown on the primary rather than the role: when old_host still back-references occupant_primary under the other role, only the vacated role's backref is cleared and the stack, its subsystems and its lvstore_ports entry all stay. Adds the invariant whose absence let this ship silently. Every forward pointer, back-reference and lvstore_ports entry agreed with itself throughout -- they are all written by the same code path, and none is evidence that the stack exists. replica_stack_violations(nodes, stack_present) asks the devices instead: a node must physically hold one lvstore per primary its back-references claim. _verify_replica_stacks probes it over RPC and runs as removal phase 3c. Reported, not fatal -- by 3c the removal is physically done and failing would only spin the retry loop against a state it cannot re-drive -- and an unreachable node is never counted as missing, since a check that cries wolf is one people learn to skip. Scoped to hosted replicas, not a node's own primary lvstore, which can be legitimately in flux mid-flow. Coverage: 4 tests on the teardown guard (both role directions, plus controls that the ordinary teardown still fires and that a node hosting a DIFFERENT primary's other role is not spared), 6 on replica_stack_violations and 4 on the RPC wrapper including the unreachable-node case. Verified the two regression tests fail with the guard disabled while both controls still pass.
JC holds an open descriptor and IO channel on each JM bdev it knows about.
jc_remove_jm closes them and drops the JM's JC context, and only after that
may the bdev be deleted -- we were skipping it entirely and going straight to
bdev_nvme_detach_controller, deleting the bdev out from under JC. That is
consistent with what was observed live on 2026-09-02: a peer's JC member list
still naming a remote_jm_* bdev that was absent from that same node's
bdev_get_bdevs.
The RPC also verifies the assumption this branch previously just asserted in
a comment -- "name_old is now unused by any local JC instance". It is not
always true. A jm_vuid whose primary has already been removed appears in no
`decisions` entry and under no back-reference, so _decommission_node_jm's
target-gathering cannot enumerate it and the batched jc_replace_jm cannot
cover it; jc_replace_jm's own -17 check then rejects a LATER call on that
node. Reproduced live on the first removal of the 2026-09-02 deploy:
jc_replace_jm failed replacing remote_jm_ad5dec09-...n1
([{'jm_vuid': 37, ...}]): the replacements do not cover all jm_vuids
that use name_old
on the peer that had hosted the removed node's own lvstore -- whose vuid was
still alive there. -22 ("still in use by one or more jm_vuids") turns that
invisible leftover into a definite answer at the one moment we can act on it,
so the response is to LEAVE THE BDEV IN PLACE rather than delete it and leave
JC pointing at nothing. Same for every other JC error and for a raised
exception: the bdev stays, and the remote_jm_devices entry stays with it, so
bookkeeping keeps describing what is actually there.
Feature-detected, not assumed: docker.io/simplyblock/spdk:main-latest as of
2026-09-02 does not expose jc_remove_jm (verified via rpc_get_methods), so
RPC_UNSUPPORTED falls back to the previous delete. That keeps this a no-op on
current builds instead of stranding every superseded controller forever, and
makes it take effect automatically once the RPC ships.
Adds RPCClient.jc_remove_jm documenting the full JC error space (-3, -6, -10
through -13, -21, -22) and JC_REMOVE_JM_STILL_IN_USE. The constant is
module-local rather than an RPCErrorCode member on purpose: the JC codes are a
separate space whose small negatives collide with that enum's generic
meanings (-1 there is invalid_state).
Coverage: 5 tests on the removal path (ordering before the delete, -22 leaves
the bdev, every other JC code leaves the bdev, a raised exception leaves the
bdev, unsupported build degrades to the old behaviour) and 4 on the client
wrapper's success/unsupported/coded-error contract.
Not yet exercised end-to-end: the deployed SPDK cannot run this path. It
needs a rerun once an image with jc_remove_jm is available.
…eers
Completes the JM decommission sequence: replace the dying JM out of EVERY
vuid on a peer, then release it from JC, then delete the bdev. The first step
was incomplete, which is why the second could only ever refuse.
removed_node's own jm_vuid keeps running as a local JC instance on whichever
peers hosted its lvstore replica, and that instance references the dying JM by
name. Pass 2 gathers targets from two sources and the leftover is in neither:
its primary is removed_node, which is excluded from live_nodes and therefore
absent from `decisions`; and its back-reference on the peer was already
cleared by phase 3a. So the batched jc_replace_jm omitted it, and
jc_replace_jm's own -17 check ("the replacements do not cover all jm_vuids
that use name_old") then rejected the entire call for that peer -- taking the
peer's legitimate vuid down with it.
Live 2026-09-02, first removal of the deploy: of eleven survivors, the single
peer whose replace failed -17 was exactly the one hosting the removed node's
own lvstore. Its own vuid 37 was never patched as a result, and the JM stayed
an active member of a live lvstore's journal cluster.
_decommission_node_jm now takes replica_peer_ids and adds
(removed_node.jm_vuid, removed_node, replacement) to those peers' target lists
-- in the SAME call, since one jc_replace_jm must cover every local vuid using
name_old. node_removal_orchestrate captures the ids before phase 3a runs,
because phase 3a clears both removed_node's secondary/tertiary pointers and
the peers' back-references, leaving nothing in the DB that records which peers
they were.
The leftover vuid is not being kept useful -- its lvstore is gone. The point
is only to get the dead JM out of it so name_old ends up referenced by
nothing, which is what lets jc_remove_jm release it rather than refuse -22.
The phase-5 defensive call keeps the empty default: by then the pointers are
long gone, and that call exists to be a no-op for tasks resuming from before
phase 2 was a separate phase.
Coverage: 4 tests on target gathering (a replica peer gets both vuids in one
call; without the captured ids the leftover is missed, i.e. the pre-fix
behaviour that produced the live -17; coverage lets jc_remove_jm run and the
bdev be deleted; a non-replica peer is untouched) plus one on the orchestrator
capturing the ids before a teardown that wipes them.
Still unvalidated end-to-end, for two independent reasons: the deployed SPDK
has no jc_remove_jm, and whether jc_replace_jm accepts a vuid whose lvstore
and raid are already deleted is an open question with the SPDK team. On the
cluster where this was found the vuid's distribs had leaked (bdev_distrib_
delete hung 180s), so it was still fully live there.
Regression in the previous commit, found live 2026-09-02 on the first removal
after it shipped -- and strictly worse than the gap it was meant to close.
Every `decisions` entry carries an invariant Pass 1 establishes and Pass 2
relies on: "this primary's OWN redundancy set (jm_ids) lists the dead JM".
Pass 2 keys the node's own-vuid target off it and both jm_ids.remove() calls
assume it. Storing the leftover replacement under decisions[removed_node]
broke that, because removed_node is itself in live_nodes -- its status is
in_removal, not removed. Pass 2 therefore treated it as an ordinary consumer,
found no name_old for it, fell into the not-replaced path, and
node.jm_ids.remove(removed_jm_id)
ValueError: list.remove(x): x not in list
aborted _decommission_node_jm mid-loop, before a single peer had been
patched. No jc_replace_jm ran at all, so both replica peers kept the dead JM
in their vuid-2 JC group -- the exact outcome the commit was written to
prevent, now reached by a crash instead of an omission.
Keeps the leftover replacement in its own local instead, so it can only ever
reach the target list for a node in replica_peer_ids and can never be
mistaken for a redundancy-set membership claim.
Also makes both jm_ids.remove() calls membership-conditional. They were
latent landmines independent of this: targets legitimately contain vuids
whose owner is not the node being patched, and an unguarded remove() there
takes down the whole phase for every peer after it, converting one node's
bookkeeping mismatch into a cluster-wide failure to decommission. Same shape
as the swallowed-error problems CONTRIBUTING.md warns about, inverted -- an
exception where a no-op belonged.
Adds a regression test driving _decommission_node_jm with a removed node
whose own jm_ids does NOT list its dying JM (the live shape): it must not
raise, the peer must still be patched, both vuids must appear in the one
call, and the removed node's unrelated jm_ids must be left untouched.
… failure
Measured live 2026-09-02 against spdk R26.3-latest, the first run where
jc_remove_jm actually exists (main-latest does not expose it): a successful
jc_replace_jm that swaps the dying JM out of every vuid on a node ALSO drops
it from JC entirely. The follow-up jc_remove_jm therefore finds nothing left
to release and answers
-13 "this JM is not used by JC"
on every node -- seven for seven in that removal. The previous commit read any
non-(-22) error as "leave the bdev in place", so every peer kept both the
superseded controller and its remote_jm_devices entry: the exact dangling
state this sequence exists to prevent, produced by being too careful rather
than not careful enough.
-13 is now the success path and the delete proceeds. -22 keeps its meaning
(some vuid still references the JM; do not delete), and every other JC code
still leaves the bdev alone.
Also logs the leftover-vuid replacement decision. Two runs have now ended with
the leftover uncovered, and from outside the log "the leftover was never
patched" and "there was no leftover to patch" are indistinguishable -- the
replacement being None is silent. Naming it makes the next run say which.
jc_remove_jm was in the wrong branch: redundant where it ran, absent where it mattered. Where it ran -- after a successful jc_replace_jm -- JC has already dropped the JM, so it answers -13 and there is nothing left to release. Measured live 2026-09-02 on spdk R26.3: seven nodes, seven replaces, seven -13s. Where it did not run is the branch that needs it. A node holding the dying JM but with no vuid this removal can patch produces no targets and fell straight through: no release, no detach, only a bookkeeping refresh. That is exactly the peer whose sole user of the JM is the removed node's OWN vuid -- no live primary, no back-reference, no target -- and nothing else in the flow will ever free it there. Now it gets the release, and the bdev only goes once JC confirms it is done with it. Extracts _release_jm_from_jc and _drop_superseded_jm_bdev so both call sites share one implementation of the -13 / -22 / unsupported handling rather than the second growing its own copy. On a refused release (-22 or any error) the no-targets branch deliberately leaves BOTH the bdev and its remote_jm_devices entry in place. Refreshing there -- what the historical code did unconditionally -- drops the entry while the bdev is still present and still held by JC, producing precisely the bookkeeping-vs-reality split this sequence exists to prevent. Bookkeeping keeps describing what is actually on the node. The refresh survives only for the case it was written for: no recorded bdev name, so nothing to release and nothing to detach. Coverage: 5 tests on the no-targets branch (release attempted with no replace, success deletes bdev and drops bookkeeping, -22 keeps both, unsupported build still cleans up, and a node that never had the JM is untouched). Still unverified end-to-end: reaching this branch needs a peer whose only user of the dying JM is the removed node's own vuid, which last run's topology did not produce -- both replica peers had other vuids to patch and so took the replace path.
…node
Per the SPDK team (2026-09-02), the two calls are alternatives on a given
node, not a sequence, and which one applies is decided by what else on that
node still uses the dying JM -- not by whether the node is a secondary or a
tertiary:
* any OTHER distrib group on the node uses it -> jc_replace_jm covering ALL
of them, the removed primary's own group included. Never a remove.
* only the removed primary's group uses it -> jc_remove_jm alone. Never a
replace.
Both RPCs force this. jc_replace_jm rejects a batch that misses any local
vuid using name_old (-17), so the removed primary's group cannot be left out
of a call that happens at all; jc_remove_jm refuses while any vuid still holds
the JM (-22), so it is only available when that group is the sole user.
The removed primary's group is added blind, on the strength of the peer ids
captured before phase 3a. It cannot be discovered at phase-2 time: phase 3a
has already deleted its lvstore, raid and distribs and cleared both the
forward pointers and the back-references. Its JC membership outlives the stack
it belonged to -- live 2026-09-02, mg6fm's SYNCD still showed vuid 45 holding
the dead JM in slot 0 with blocked=1 long after the lvstore was gone, while
that same removal's jc_replace_jm on mg6fm substituted only vuid 21 and left
slot 0 pointing at a node that no longer existed.
Previous commits had this in both wrong shapes: first calling remove after
every successful replace (where it is a guaranteed -13 no-op), then treating
the removed primary's group as remove-only everywhere (which would strand the
other groups' -17). Neither matched the constraint.
Tests updated to the exclusive rule: the leftover joins the batch when another
group is present and no remove follows; a node whose only user is the leftover
gets remove alone; error handling for the remove path (-13 success, -22 and
other codes keep the bdev, raised exception keeps the bdev) moves to the class
that exercises the path where remove actually runs.
Not yet exercised live: reaching the remove-only branch needs a node whose
sole user of the dying JM is the removed primary's group. In the last removal
all seven affected nodes had another group and would take the replace path.
…oved
Node removal does not delete the StorageNode record -- it leaves it with
status=removed. Two monitors were written as if it did, and both kept working
against a node whose SPDK no longer exists.
main_distr_event_collector
--------------------------
start_jm_event_collector_on_node ended its loop only on KeyError from
get_storage_node_by_id, and its log message ("Node %s removed; stopping JM
collector") shows removal is what the author meant to catch. The record never
disappears, so the KeyError never fired and the loop polled forever. Measured
live 2026-09-02: 1036 "Failed to process JM events on <removed node>:
connection error" in the 1.5h after a single removal, still climbing. The
distrib collector had no status check at all.
Fixing only the loops would not have held: ensure_collectors respawns any
collector that is not alive, from a node list it never filtered, so a clean
exit came back within ~5s. It now skips removed nodes and forgets their
threads_maps entries; both loops also exit on removal, which matters for a
node removed while its thread is mid-poll.
health_controller
-----------------
The remote-JM probe fired its RPC first and established context second: it
called get_bdevs, logged "Checking bdev: ... failed" at ERROR, and only then
looked up whether the owning node was removed -- at which point it logged a
second line saying the failure was expected. The health verdict was already
correct (a removed owner never failed the check), but every cycle spent an RPC
per stale entry and left an ERROR in the log that the next line contradicted
and nothing retracted: 1797 hits on one removed node's JM.
Owner is now resolved before the probe. When it is not relevant the entry is
logged once and skipped -- no get_bdevs, and no bdev_nvme_controller_list
either, since that would fail for the same reason. connected_jms is still
appended so the jm_ids reconciliation below behaves identically.
Coverage: 3 tests on ensure_collectors (a removed node gets no collectors, an
already-tracked removed node is forgotten, an online node alongside it is
unaffected).
…cts it
The removed primary's group is added to the batch without being able to
inspect it -- by phase 2 its lvstore, raid and distribs are gone and both the
forward pointers and back-references are cleared, so it goes in purely on the
strength of the peer ids captured before phase 3a.
If SPDK does not regard that group as a user of name_old it answers -19
("unknown jm_vuid") or -20 ("this jm_vuid does not use name_old") and rejects
the WHOLE call. The node's real, surviving groups would then keep pointing at
a JM that no longer exists -- strictly worse than the leak the blind entry is
there to close, and the same damage already seen once on 2026-09-02 when a
rejected replace left a node holding a dead JM in its own lvstore's group.
On exactly those two codes the call is retried once with the blind entry
dropped, so a wrong assumption degrades to the previous behaviour instead of
losing the node. Every other code still propagates: -17 in particular means
the batch was incomplete, and dropping an entry would only make the next
attempt more incomplete. No retry either when the blind entry was the only
target, since the remaining batch would be empty.
This is deliberately defensive rather than derived. Whether a torn-down group
can be a jc_replace_jm target is an open question with the SPDK team: in the
2026-09-02 removal a replace covering only the surviving vuid succeeded with
no -17, even though SYNCD showed the leftover group still holding the JM in
slot 0 with blocked=1. The two observations cannot both be complete, and until
that is settled the cost of being wrong should not be a node left unpatched.
Coverage: retry-on--19/-20 asserting the first call carries [2, 37] and the
retry [37]; no retry on -17; no retry when the blind entry is the only target;
and a failed replace never detaches the superseded JM's own controller.
…arget
Correction to the two previous commits. jc_replace_jm is for groups that keep
running: it swaps a dead member for a live one so the group stays redundant.
removed_node's own lvstore group is not one of those -- that lvstore is being
destroyed, so giving it a fresh JM buys nothing and consumes a spare that has
no reason to exist. The batch therefore covers the surviving groups and
nothing else, including on the secondary and tertiary, which are the only
nodes carrying the leftover at all.
That leaves the two calls cleanly split by case rather than by node:
* at least one surviving group used the JM -> jc_replace_jm for those groups,
then delete the bdev. No release: after a successful replace JC has
dropped the JM anyway (spdk R26.3 answers -13, "not used by JC", measured
live 2026-09-02 on all seven affected nodes).
* no surviving group used it -> jc_remove_jm, and delete only if it confirms.
Also drops the -19/-20 retry added in the previous commit. It existed to
survive SPDK rejecting a batch that named a group it did not consider a user
of name_old -- a risk that only arose from putting the leftover in the batch.
With the leftover gone the batch names nothing that cannot be observed, so
there is nothing to fall back from, and keeping the retry would only suggest
otherwise. The two error constants go with it; the jc_replace_jm docstring
already documents that error space.
Settles which of the two JC calls applies where. The choice is made per node,
from what still uses the dying JM there:
secondary/tertiary, no surviving group uses it -> jc_remove_jm, then
delete the bdev only if JC confirms. removed_node's own lvstore group
is the sole remaining user and this is the only place it is released.
secondary/tertiary, some surviving group uses it -> jc_replace_jm for
those groups only, then delete the bdev. No release.
any other node, some surviving group uses it -> jc_replace_jm, same.
any other node, nothing uses it -> no JC operation at all.
removed_node's own lvstore group is never a replace target anywhere: that
lvstore is being destroyed, so a replacement member buys nothing.
The last case still reconciles the DB record, and only that. remote_jm_devices
is derived from three sources -- an explicit jm_ids list, the node's own
jm_ids, and the JM of whichever primary it hosts -- and a record none of them
justifies is not inert: it is the lookup a later removal uses to find
jc_replace_jm's name_old, so a leftover makes that removal believe the node
has a connection to patch. A splice reshuffle can produce one (2026-08-14
incident: peer reachable only through the hosted-primary path, own jm_ids
clean, entry never re-derived). The refresh runs only when such a record is
actually present, and touches nothing in JC.
Tests updated: the leftover is asserted absent from every batch; the release
path is reached only for a node named in replica_peer_ids; the 2026-08-14
refresh test keeps its original expectation.
While removing 2vk79, its peer hxmr8 was rebuilding as the tertiary of another primary (s7457/LVS_21) and chose its deferred hublvol failover target by asking whether LVS_21's secondary was alive. That secondary was 2vk79 itself -- already shut down by phase 1 and in IN_REMOVAL -- so the attach went to a dead SPDK: bdev_nvme_attach_controller failed for LVS_21/hublvol ...: -5 I/O error Failed to add deferred hublvol failover path to ddbf964e... for LVS_21 _check_peer_disconnected short-circuits on mgmt ground truth for OFFLINE / REMOVED / UNREACHABLE, but IN_REMOVAL was missing, so the check fell through to the data-plane JM-quorum path. That path votes "connected" on `0/0 peers report disconnected` -- precisely the abstain-from-all case that arises once peers have torn down their controllers for the departing node. The verdict therefore contradicted the control plane's own intent to delete the node. IN_REMOVAL belongs in the short-circuit on the same grounds as OFFLINE: phase 1 has already killed the node's SPDK and mgmt API by the time the status is set, and unlike IN_SHUTDOWN it is never coming back. Both wrappers callers use to pick targets (_is_fabric_connected, _count_fabric_disconnected_nodes) inherit the fix. PENDING_REMOVAL is deliberately left out: the task runner sets it before phase 1 shuts the node down, so the node is still up and serving and still needs its port-block. The status list moves into a named _PEER_DISCONNECTED_STATUSES constant so the membership decision and its rationale sit in one place instead of inside a tuple literal.
_decommission_node_jm builds live_nodes by filtering STATUS_REMOVED, but at
phase 2 the node being removed is IN_REMOVAL, so it stayed in its own sweep.
The function's docstring already asserted the opposite ("removed_node, which
is not in live_nodes"), and a later comment documented the discrepancy and
worked around it in one place only -- Pass 1's `decisions` entry. Pass 2 was
left reachable.
Phase 3a clears a node's own secondary/tertiary pointers and its peers'
back-references, but not the lvstore_stack_* backrefs recording the replicas
it hosts for OTHER primaries; phase 3b does that, and runs after phase 2. So
Pass 2 found the shut-down node still pointing at a live primary, adopted it
as a patch target for that primary's vuid, and went looking for the dying
JM's bdev name in its own remote_jm_devices -- where a node's own JM never
appears. Live 2026-09-03 removing s25dl:
ERROR: [REMOVAL] 56d8add2...: no recorded bdev name for removed JM
601dae11...; cannot call jc_replace_jm --
affected targets=[(1, 'a91a2d46...')]
It was harmless only by accident: the missing name short-circuited the call.
Had one been recorded it would have issued jc_replace_jm at the pod phase 1
already killed -- precisely what the REMOVED filter exists to prevent, per
the 2026-08-11 incident its comment cites.
removed_node is now excluded by identity, which is stable across the two
calls this function makes (its status differs between them), and IN_REMOVAL
joins REMOVED in the status filter on the same grounds: such a node is down
and its rpc_client cannot resolve. This is the same missing-status class as
_PEER_DISCONNECTED_STATUSES in the preceding commit.
The regression test asserts on the ERROR record rather than on a stray RPC:
"no RPC issued" passes with the bug still present, because of the accident
above. Verified by mutation -- reverting the filter reproduces the live
message verbatim.
check_remote_device gated only the health verdict, never the probe. The caller at :761 discards the result when the owning node is gone, but it calls the function first, so both RPCs still went out on every cycle for every surviving node. For a removed node's devices that never stops: each miss makes SPDK log `*ERROR*: ctrlr 'remote_alceml_<uuid>' does not exist`, measured at 3-15 errors/min and still climbing 35 minutes after the removal that made those devices failed_and_migrated (2026-09-03, devices 04fce724 / b0ada39d / ddf660f5 of the removed 2vk79, probed by 9 surviving nodes). Real faults then drown in a permanent error stream. The owner is now checked before the loop, matching what the remote-JM path at :818 already does -- the two were the same rule implemented two different ways, one gating the RPC and the other only the verdict. The test asserts on the RPCs rather than the return value: the caller already discards the verdict for a departed owner, so a verdict-only assertion passes with the bug still in place. Verified by mutation.
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.