diff --git a/simplyblock_core/controllers/health_controller.py b/simplyblock_core/controllers/health_controller.py index 889bb88913..13284c72cc 100644 --- a/simplyblock_core/controllers/health_controller.py +++ b/simplyblock_core/controllers/health_controller.py @@ -958,20 +958,30 @@ def check_node(node_id, with_devices=True): for remote_device in snode.remote_jm_devices: name = remote_device.remote_bdev - bdev_info = rpc_client.get_bdevs(name) - logger.log(INFO if bdev_info else ERROR, - f"Checking bdev: {name} ... " + ('ok' if bdev_info else 'failed')) + # Owner resolved BEFORE the probe, not after. Previously the + # RPC went out unconditionally and the ERROR line was logged + # before anything knew the owner was gone -- so a removed + # node's stale entry cost one RPC per cycle and left an ERROR + # in the log that the very next line classified as expected, + # and never retracted. Live 2026-09-02: 1797 such hits on one + # removed node's JM. try: jm_owner = db_controller.get_storage_node_by_id(remote_device.node_id) except KeyError: jm_owner = None - if _peer_connections_relevant(jm_owner): - node_remote_devices_check &= bool(bdev_info) - elif not bdev_info: + owner_relevant = _peer_connections_relevant(jm_owner) + if not owner_relevant: logger.info( - "Remote JM %s missing, but owning node %s is %s — expected, " - "not failing health", name, remote_device.node_id, + "Remote JM %s belongs to node %s (%s); not probing and not " + "failing health", name, remote_device.node_id, jm_owner.status if jm_owner else "not-found") + connected_jms.append(remote_device.get_id()) + continue + + bdev_info = rpc_client.get_bdevs(name) + logger.log(INFO if bdev_info else ERROR, + f"Checking bdev: {name} ... " + ('ok' if bdev_info else 'failed')) + node_remote_devices_check &= bool(bdev_info) connected_jms.append(remote_device.get_id()) controller_info = rpc_client.bdev_nvme_controller_list(f'remote_{remote_device.jm_bdev}') @@ -1126,6 +1136,26 @@ def check_remote_device(device_id, target_node=None): logger.exception("node not found") return False + # The device's OWNER decides whether a remote connection to it is even + # expected. Skip the probe entirely when it is not -- same rule, and the + # same reason, as the remote-JM loop above: a missing connection to a + # departed owner is the expected consequence of its teardown. + # + # Gating only the verdict is not enough. The caller already discards the + # result for an irrelevant owner, but it calls this function first, so the + # two RPCs below 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_' does not exist`, measured at + # 3-15 errors/min 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. + if not _peer_connections_relevant(snode): + logger.info( + "Remote device %s belongs to node %s (%s); not probing and not " + "failing health", device_id, device.node_id, snode.status) + return True + result = True if target_node: nodes = [target_node] diff --git a/simplyblock_core/controllers/replica_placement.py b/simplyblock_core/controllers/replica_placement.py new file mode 100644 index 0000000000..155c55598f --- /dev/null +++ b/simplyblock_core/controllers/replica_placement.py @@ -0,0 +1,706 @@ +# coding=utf-8 +"""Global, constraint-solving planner for secondary/tertiary replica placement. + +Pure logic -- no DB access, no SPDK calls -- so it can be unit-tested in +isolation. The orchestrator that consumes the plan lives in +``storage_node_ops`` (node removal, phase 3b). + +Why a global planner +-------------------- +The historical relocation path (``_pick_replica_relocation_node`` and +friends) repairs **one stranded role at a time**: it looks at a single +primary, asks "where can this one replica go", and takes the first +candidate that is domain-diverse; when no free candidate exists it splices +into an existing pairing, then patches up at most one further hop of +collateral damage. Every decision is local and irrevocable. + +That is provably insufficient for the invariant it is trying to hold. +Consider the reported case: 4 failure domains x 3 hosts = 12 nodes, FTT2, +and one host removed from each domain in turn. Each removal frees exactly +one secondary slot and one tertiary slot system-wide, so at every step the +greedy picker has at most one free candidate per role. If that one free +candidate happens to sit in the primary's own domain -- or in the domain of +the role that is *not* being relocated -- the picker cannot fix it by +moving the other role too, because it never considers the other role. It +either splices (perturbing an uninvolved third node) or falls back to a +weaker "at least one cross-domain role" floor and logs a warning. After a +few removals the layout has accumulated several such compromises even +though a fully diverse layout existed the whole time and was reachable by +*swapping* two already-placed replicas -- a move no local repair can ever +express. + +This module solves the whole assignment at once instead: + +* every node is primary of exactly one LVS, and hosts at most one + secondary and at most one tertiary (``lvstore_stack_secondary`` / + ``lvstore_stack_tertiary`` are single-valued), so "which node hosts + whose secondary" is a **permutation** of the node set, not a bag of + independent choices; +* full pairwise diversity (``fd(P)``, ``fd(S)``, ``fd(T)`` all distinct) + is a hard edge constraint on that permutation; +* "don't move replicas that are already fine" is an objective, not a + constraint. + +That is a min-cost perfect bipartite matching, solved exactly by the +Hungarian algorithm (:func:`min_cost_matching`) in O(n^3) -- trivial at +cluster scale. Secondary and tertiary are coupled only through the +*domain* of the secondary, so they are solved in two stages with an exact +feasibility test between them (:func:`tertiary_blocking_pairs`, Hall's +condition specialised to this structure) and a penalty retry that steers +the secondary stage away from a domain pattern that would strand the +tertiary stage. + +The result is: full diversity whenever it is mathematically achievable, +with the provably smallest number of physical replica rebuilds, and an +explicit, machine-checkable statement of what is impossible when it is +not -- instead of a warning buried in a log. +""" + +import logging +from typing import Dict, List, Mapping, NamedTuple, Optional, Sequence, Tuple + +logger = logging.getLogger() + + +ROLE_SECONDARY = "secondary" +ROLE_TERTIARY = "tertiary" + +# Cost scale for the matching. FORBIDDEN is not "infinity" on purpose: the +# matcher stays in integer arithmetic, and a solution that had to use a +# forbidden edge is detected afterwards by its total cost, which is both +# simpler and more robust than sentinel-aware relaxation inside the inner +# loop. Any single edge >= FORBIDDEN means at least one hard constraint was +# unsatisfiable, i.e. no valid perfect matching exists. +MOVE_COST = 1000 # rebuilding a replica on a different host +LABEL_PENALTY = 1 # soft physical-label anti-affinity +PAIR_PENALTY = 50 # steer stage 1 away from a tertiary-blocking pattern +FORBIDDEN = 10 ** 9 + +# Bound on the stage-1 retry loop. Each iteration penalises the domain pairs +# that made the tertiary stage infeasible, so progress is monotone in +# practice; the bound only exists so a pathological topology degrades to the +# constructive fallback instead of spinning. +MAX_PAIR_RETRIES = 8 + + +class Placement(NamedTuple): + """Where one LVS's non-leader roles live. ``tertiary`` is ``""`` on FTT1.""" + secondary: str + tertiary: str + + +class ReplicaMove(NamedTuple): + """One planned relocation of a single role. + + ``from_node_id`` is ``""`` when the role currently has no host at all + (its previous host is the node being removed, already torn down). + ``scratch`` marks a move that only exists to break a rotation cycle: the + role is parked on a temporarily free host and moved again later -- see + :func:`order_moves`. + """ + lvs_primary_node_id: str + role: str + from_node_id: str + to_node_id: str + scratch: bool = False + + +class DiversityPlan(NamedTuple): + """Result of :func:`plan_diverse_layout`. + + ``layout`` maps primary node id -> :class:`Placement`. ``full_diversity`` + is True when every LVS in ``layout`` has pairwise-distinct domains across + primary/secondary/tertiary. ``violations`` describes what could not be + satisfied (empty iff ``full_diversity``). ``notes`` records how the plan + was reached (which stage / which fallback), for the operator-facing log. + """ + layout: Dict[str, Placement] + full_diversity: bool + violations: List[str] + notes: List[str] + + +class InfeasiblePlacement(Exception): + """No valid assignment exists at all -- not even ignoring failure domains. + + Raised rather than returned because it means the *host-disjointness* + floor is unsatisfiable (e.g. too few hosts for the fault-tolerance + level), which is a precondition failure the caller must surface, not a + quality degradation it can log and continue past. + """ + + +# --------------------------------------------------------------------------- +# Min-cost perfect matching (Hungarian / Jonker-Volgenant, O(n^3)) +# --------------------------------------------------------------------------- + +def min_cost_matching(cost: Sequence[Sequence[int]]) -> List[int]: + """Min-cost perfect matching on a rectangular cost matrix (rows <= cols). + + ``cost[i][j]`` is the cost of assigning row ``i`` to column ``j``. Returns + a list ``a`` with ``a[i] == j``. + + Forbidden pairs are expressed as :data:`FORBIDDEN`; callers check the + chosen edges against :data:`FORBIDDEN` to detect "no valid assignment + exists" rather than relying on the matcher to reject them (see the module + docstring). + + Shortest-augmenting-path formulation with dual potentials: rows are added + one at a time, each via a Dijkstra pass over the reduced costs, so the + matching stays optimal after every augmentation. + """ + n = len(cost) + if n == 0: + return [] + m = len(cost[0]) + if m < n: + raise ValueError( + f"cost matrix must have at least as many columns as rows ({n}x{m})") + + inf = FORBIDDEN * 8 + u = [0] * (n + 1) + v = [0] * (m + 1) + # p[j] = row currently matched to column j (0 = unmatched); way[j] is the + # predecessor column on the augmenting path being built. + p = [0] * (m + 1) + way = [0] * (m + 1) + + for i in range(1, n + 1): + p[0] = i + j0 = 0 + minv = [inf] * (m + 1) + used = [False] * (m + 1) + while True: + used[j0] = True + i0 = p[j0] + delta = inf + j1 = 0 + row = cost[i0 - 1] + for j in range(1, m + 1): + if used[j]: + continue + cur = row[j - 1] - u[i0] - v[j] + if cur < minv[j]: + minv[j] = cur + way[j] = j0 + if minv[j] < delta: + delta = minv[j] + j1 = j + for j in range(m + 1): + if used[j]: + u[p[j]] += delta + v[j] -= delta + else: + minv[j] -= delta + j0 = j1 + if p[j0] == 0: + break + while j0: + j1 = way[j0] + p[j0] = p[j1] + j0 = j1 + + result = [-1] * n + for j in range(1, m + 1): + if p[j]: + result[p[j] - 1] = j - 1 + return result + + +# --------------------------------------------------------------------------- +# Diversity checking +# --------------------------------------------------------------------------- + +def full_diversity_violations( + layout: Mapping[str, Placement], + fd_by_node: Mapping[str, int], + ftt: int, +) -> List[str]: + """Report every LVS whose roles are NOT pairwise domain-distinct. + + Deliberately stricter than + ``cluster_expansion.planner.compute_fd_layout_violations``, which only + asserts the weaker ">=1 cross-domain role" floor: that floor is satisfied + by a layout whose secondary and tertiary share a domain (one host outage + then costs two of the three copies), which is exactly the state the + incremental relocation path kept producing. A primary with an unset + domain (< 0) is skipped -- the feature is off for it -- but an unset + domain on a *role holder* counts as a violation, since "unknown" cannot + be asserted to be disjoint. + """ + violations: List[str] = [] + for primary_id in sorted(layout): + placement = layout[primary_id] + fd_p = fd_by_node.get(primary_id, -1) + if fd_p < 0: + continue + roles = [(ROLE_SECONDARY, placement.secondary)] + if ftt >= 2: + roles.append((ROLE_TERTIARY, placement.tertiary)) + seen: Dict[int, str] = {fd_p: f"primary {primary_id}"} + for role, holder in roles: + if not holder: + violations.append(f"LVS@{primary_id} (fd={fd_p}) has no {role}") + continue + fd_h = fd_by_node.get(holder, -1) + if fd_h < 0: + violations.append( + f"LVS@{primary_id} (fd={fd_p}) {role}={holder} has no " + f"failure domain set") + continue + if fd_h in seen: + violations.append( + f"LVS@{primary_id} (fd={fd_p}) {role}={holder} (fd={fd_h}) " + f"shares a domain with {seen[fd_h]}") + continue + seen[fd_h] = f"{role} {holder}" + return violations + + +# --------------------------------------------------------------------------- +# Feasibility (Hall's condition, specialised) +# --------------------------------------------------------------------------- + +def secondary_overloaded_domains(domain_sizes: Mapping[int, int]) -> List[int]: + """Domains that make a fully diverse SECONDARY permutation impossible. + + Every node hosts exactly one secondary, so the secondary assignment is a + permutation and the ``n_d`` primaries of domain ``d`` must all be hosted + outside ``d``. By Hall's condition that needs ``n_d <= N - n_d``, i.e. no + domain may hold more than half the cluster. Independent of any particular + assignment -- purely structural. + """ + total = sum(domain_sizes.values()) + return sorted(d for d, size in domain_sizes.items() if 2 * size > total) + + +def tertiary_blocking_pairs( + forbidden_pairs: Mapping[Tuple[int, int], int], + domain_sizes: Mapping[int, int], +) -> List[Tuple[int, int]]: + """Domain pairs that make the TERTIARY stage infeasible under a given + secondary assignment. + + Given the secondary permutation, primary ``p`` may host its tertiary in + any domain except ``F(p) = {fd(p), fd(sec(p))}``. Hall's condition over + the domain-block structure reduces to checking only the subsets whose + complement is contained in some ``F(p)``, and ``|F(p)| <= 2``, so exactly + two families need checking: + + * singletons ``{d}``: ``#{p : d in F(p)} <= N - n_d``. That count is + always ``2 * n_d`` (``n_d`` primaries live in ``d``, and exactly ``n_d`` + secondaries land in ``d`` because the assignment is a permutation), so + this is the assignment-independent condition + :func:`secondary_overloaded_domains` already covers -- not repeated + here. + * pairs ``{d, e}``: ``#{p : F(p) == {d, e}} <= N - n_d - n_e``. THIS one + depends on the secondary assignment, and is what the stage-1 penalty + retry steers away from. + + ``forbidden_pairs`` maps the normalised pair ``(min, max)`` to how many + primaries currently have exactly that ``F(p)``. Returns the violating + pairs. + """ + total = sum(domain_sizes.values()) + blocking: List[Tuple[int, int]] = [] + for (d, e), count in sorted(forbidden_pairs.items()): + capacity = total - domain_sizes.get(d, 0) - domain_sizes.get(e, 0) + if count > capacity: + blocking.append((d, e)) + return blocking + + +# --------------------------------------------------------------------------- +# The planner +# --------------------------------------------------------------------------- + +def _assignment_from_matching( + primaries: Sequence[str], + hosts: Sequence[str], + cost: Sequence[Sequence[int]], +) -> Optional[Dict[str, str]]: + """Run the matcher and reject the result if it had to use a forbidden + edge. ``None`` means no assignment satisfying the hard constraints + exists.""" + matching = min_cost_matching(cost) + assignment: Dict[str, str] = {} + for i, j in enumerate(matching): + if j < 0 or cost[i][j] >= FORBIDDEN: + return None + assignment[primaries[i]] = hosts[j] + return assignment + + +def _pair_key(a: int, b: int) -> Tuple[int, int]: + return (a, b) if a <= b else (b, a) + + +def plan_diverse_layout( + node_ids: Sequence[str], + fd_by_node: Mapping[str, int], + current_layout: Mapping[str, Placement], + ftt: int, + *, + host_by_node: Optional[Mapping[str, str]] = None, + label_by_node: Optional[Mapping[str, int]] = None, +) -> DiversityPlan: + """Compute the cheapest fully domain-diverse layout over ``node_ids``. + + ``node_ids`` is the set of nodes that will be alive AFTER the topology + change; every one of them is primary of its own LVS and is available to + host one secondary and one tertiary. ``current_layout`` is the layout as + it stands right now, keyed by primary; entries pointing at nodes outside + ``node_ids`` (e.g. the node being removed) are treated as "no host", so + the planner naturally re-homes them. Primaries missing from + ``current_layout`` are treated the same way. + + Hard constraints, in both stages: + + * a role never lands on its own primary; + * a role never lands on a host already used by another role of the same + LVS (host-disjointness -- what actually makes a single host loss + survivable); + * with failure domains in play, the domains of primary / secondary / + tertiary are pairwise distinct. + + Soft preferences, expressed as cost: + + * keeping a role where it already is costs 0, moving it costs + :data:`MOVE_COST` -- so the returned layout is a *minimum-rebuild* + layout, not just any valid one; + * sharing a ``physical_label`` with another role of the same LVS costs + :data:`LABEL_PENALTY`, matching the existing best-effort treatment of + that dimension. + + When full diversity is unreachable the domain constraint is dropped (the + host-disjointness floor is kept) and the plan comes back with + ``full_diversity=False`` and the specific ``violations``, so the caller + can decide whether to proceed degraded or refuse -- instead of silently + settling like the incremental path did. + + Raises :class:`InfeasiblePlacement` when even the host-disjoint floor has + no solution. + """ + nodes = list(node_ids) + n = len(nodes) + notes: List[str] = [] + if n == 0: + return DiversityPlan({}, True, [], notes) + if ftt not in (1, 2): + raise ValueError(f"ftt must be 1 or 2, got {ftt}") + if n < ftt + 1: + raise InfeasiblePlacement( + f"{n} node(s) cannot host {ftt + 1} distinct copies (FTT{ftt})") + + host_of = dict(host_by_node or {}) + label_of = dict(label_by_node or {}) + fd_of = {node: fd_by_node.get(node, -1) for node in nodes} + fd_enabled = all(fd_of[node] >= 0 for node in nodes) and len(set(fd_of.values())) > 1 + + domain_sizes: Dict[int, int] = {} + for node in nodes: + domain_sizes[fd_of[node]] = domain_sizes.get(fd_of[node], 0) + 1 + + def _host(node: str) -> str: + return host_of.get(node, node) + + def _label(node: str) -> int: + return label_of.get(node, 0) + + def _current(primary: str) -> Placement: + placement = current_layout.get(primary, Placement("", "")) + secondary = placement.secondary if placement.secondary in fd_of else "" + tertiary = placement.tertiary if placement.tertiary in fd_of else "" + return Placement(secondary, tertiary) + + overloaded = secondary_overloaded_domains(domain_sizes) if fd_enabled else [] + if overloaded: + notes.append( + f"failure domain(s) {overloaded} hold more than half the cluster; " + f"a fully diverse layout is structurally impossible") + + # -- stage 1: secondary permutation ------------------------------------ + def _secondary_cost(enforce_fd: bool, pair_penalties: Mapping[Tuple[int, int], int]): + matrix: List[List[int]] = [] + for primary in nodes: + row: List[int] = [] + for host in nodes: + if host == primary or _host(host) == _host(primary): + row.append(FORBIDDEN) + continue + if enforce_fd and fd_of[host] == fd_of[primary]: + row.append(FORBIDDEN) + continue + cost = 0 if host == _current(primary).secondary else MOVE_COST + if _label(host) > 0 and _label(host) == _label(primary): + cost += LABEL_PENALTY + cost += pair_penalties.get(_pair_key(fd_of[primary], fd_of[host]), 0) + row.append(cost) + matrix.append(row) + return matrix + + enforce_fd = fd_enabled and not overloaded + penalties: Dict[Tuple[int, int], int] = {} + secondary: Optional[Dict[str, str]] = None + for attempt in range(MAX_PAIR_RETRIES + 1): + secondary = _assignment_from_matching( + nodes, nodes, _secondary_cost(enforce_fd, penalties)) + if secondary is None: + break + if not enforce_fd or ftt < 2: + break + pair_counts: Dict[Tuple[int, int], int] = {} + for primary in nodes: + key = _pair_key(fd_of[primary], fd_of[secondary[primary]]) + pair_counts[key] = pair_counts.get(key, 0) + 1 + blocking = tertiary_blocking_pairs(pair_counts, domain_sizes) + if not blocking: + break + if attempt == MAX_PAIR_RETRIES: + notes.append( + f"could not steer the secondary layout away from " + f"tertiary-blocking domain pattern(s) {blocking}") + break + for pair in blocking: + penalties[pair] = penalties.get(pair, 0) + PAIR_PENALTY + + if secondary is None and enforce_fd: + # Domain-diverse secondaries are unreachable; keep the host-disjoint + # floor so the cluster still survives a single host loss. + enforce_fd = False + notes.append( + "no domain-diverse secondary permutation exists; falling back to " + "host-disjoint placement") + secondary = _assignment_from_matching(nodes, nodes, _secondary_cost(False, {})) + if secondary is None: + raise InfeasiblePlacement( + "no host-disjoint secondary placement exists for this node set") + + if ftt < 2: + layout = {primary: Placement(secondary[primary], "") for primary in nodes} + violations = full_diversity_violations(layout, fd_of, ftt) if fd_enabled else [] + return DiversityPlan(layout, not violations, violations, notes) + + # -- stage 2: tertiary permutation, given the secondary domains -------- + def _tertiary_cost(enforce: bool): + matrix: List[List[int]] = [] + for primary in nodes: + sec = secondary[primary] + row: List[int] = [] + for host in nodes: + if host in (primary, sec): + row.append(FORBIDDEN) + continue + if _host(host) in (_host(primary), _host(sec)): + row.append(FORBIDDEN) + continue + if enforce and fd_of[host] in (fd_of[primary], fd_of[sec]): + row.append(FORBIDDEN) + continue + cost = 0 if host == _current(primary).tertiary else MOVE_COST + if _label(host) > 0 and _label(host) in (_label(primary), _label(sec)): + cost += LABEL_PENALTY + row.append(cost) + matrix.append(row) + return matrix + + tertiary = _assignment_from_matching(nodes, nodes, _tertiary_cost(enforce_fd)) + if tertiary is None and enforce_fd: + notes.append( + "no fully domain-diverse tertiary permutation exists for the chosen " + "secondary layout; falling back to host-disjoint placement") + tertiary = _assignment_from_matching(nodes, nodes, _tertiary_cost(False)) + if tertiary is None: + raise InfeasiblePlacement( + "no host-disjoint tertiary placement exists for this node set") + + layout = { + primary: Placement(secondary[primary], tertiary[primary]) + for primary in nodes + } + violations = full_diversity_violations(layout, fd_of, ftt) if fd_enabled else [] + return DiversityPlan(layout, not violations, violations, notes) + + +# --------------------------------------------------------------------------- +# Diffing and ordering +# --------------------------------------------------------------------------- + +def diff_layout( + current_layout: Mapping[str, Placement], + target_layout: Mapping[str, Placement], + ftt: int, +) -> List[ReplicaMove]: + """Unordered set of role relocations turning ``current`` into ``target``. + + Only primaries present in ``target_layout`` are considered -- a primary + that disappeared (the node being removed) has its replicas torn down by + the caller, not relocated. ``from_node_id`` is ``""`` when the role has + no current host. + """ + moves: List[ReplicaMove] = [] + roles = [(ROLE_SECONDARY, 0)] + ([(ROLE_TERTIARY, 1)] if ftt >= 2 else []) + for primary in sorted(target_layout): + target = target_layout[primary] + current = current_layout.get(primary, Placement("", "")) + for role, index in roles: + want = target[index] + have = current[index] + if not want or want == have: + continue + moves.append(ReplicaMove(primary, role, have, want)) + return moves + + +def order_moves( + moves: Sequence[ReplicaMove], + current_layout: Mapping[str, Placement], + all_node_ids: Sequence[str], + ftt: int, +) -> List[ReplicaMove]: + """Order ``moves`` so every one lands on a host slot that is free at the + time it runs -- and insert scratch hops where that is impossible. + + A node's ``lvstore_stack_secondary`` / ``lvstore_stack_tertiary`` is a + single string, so a host can record at most one secondary and one + tertiary. Relocations therefore cannot be applied in arbitrary order: a + move onto an occupied slot has to wait for its occupant to leave. Within + one role, the pending moves form a functional graph over host slots, so: + + * **chains** ending on a currently-free slot execute back-to-front and + always fit (a removal frees exactly one slot per role, which is what + makes the ordinary post-removal repair a single chain); + * **cycles** (a pure rotation, e.g. two primaries swapping hosts) have no + free slot to start from. They are broken by parking one member on a + free host first -- an extra ``scratch=True`` build -- and moving it to + its real target once the rotation has come round. Without this the + recursive vacate in ``_relocate_replica_between`` walks the cycle and + hits its own cycle backstop, failing the whole relocation; a swap + between two already-placed replicas is exactly the repair a local + picker can never express, so it has to be executable here. + + Cycles are broken BEFORE any chain runs, and never on demand once the + ordering is under way. A removal frees exactly one slot per role, and a + chain consumes that slot when it terminates -- so a cycle discovered + after the chains have run would have nowhere left to park. Breaking a + cycle is slot-neutral (parking on the free host immediately frees the + parked member's own host, and the final hop out of the scratch host + gives it back), so doing all of them up front leaves the chains exactly + the one slot they need. + + Raises :class:`InfeasiblePlacement` if a cycle has to be broken and no + host slot is free at all. That cannot happen in the removal flow, which + always frees one; it does happen when repairing a fully-occupied layout + in place, where a rotation is unexecutable while + ``lvstore_stack_secondary`` / ``_tertiary`` stay single-valued. + """ + ordered: List[ReplicaMove] = [] + roles = [(ROLE_SECONDARY, 0)] + ([(ROLE_TERTIARY, 1)] if ftt >= 2 else []) + for role, index in roles: + role_moves = [m for m in moves if m.role == role] + if not role_moves: + continue + occupied: Dict[str, str] = {} + for primary, placement in current_layout.items(): + holder = placement[index] + if holder: + occupied[holder] = primary + pending: Dict[str, ReplicaMove] = {m.lvs_primary_node_id: m for m in role_moves} + # A slot is free when no surviving primary's role currently sits on it. + survivors = set(all_node_ids) + free = sorted(node for node in all_node_ids if node not in occupied) + + def _emit(move: ReplicaMove) -> None: + ordered.append(move) + if move.from_node_id: + occupied.pop(move.from_node_id, None) + # A role vacated off the node being removed frees nothing + # usable: that node is on its way out and must never be + # picked as a scratch host. + if move.from_node_id in survivors and move.from_node_id not in free: + free.append(move.from_node_id) + occupied[move.to_node_id] = move.lvs_primary_node_id + if move.to_node_id in free: + free.remove(move.to_node_id) + + _break_cycles(pending, free, role, _emit) + + while pending: + for primary in sorted(pending): + if pending[primary].to_node_id not in occupied: + _emit(pending.pop(primary)) + break + else: # pragma: no cover - _break_cycles leaves only chains + raise InfeasiblePlacement( + f"cannot order {role} relocations: no move can run without " + f"displacing a replica that is not itself moving") + return ordered + + +def _break_cycles(pending, free, role, emit) -> None: + """Rewrite every rotation cycle in ``pending`` into a chain. + + Each pending move points at the move that vacates its target host -- or + at nothing, when the target is already free. In a valid target layout + each host is the target of exactly one primary, so that mapping is a + partial function and its components are exactly chains (ending on a free + host) and cycles. For each cycle, one member is parked on a free host via + a ``scratch`` move; its own host becomes free, turning the cycle into a + chain that terminates there. + """ + vacated_by = { + move.from_node_id: primary + for primary, move in pending.items() if move.from_node_id + } + visited: set = set() + for start in sorted(pending): + if start in visited: + continue + path: List[str] = [] + position: Dict[str, int] = {} + cursor: Optional[str] = start + while cursor is not None and cursor not in visited: + position[cursor] = len(path) + path.append(cursor) + visited.add(cursor) + cursor = vacated_by.get(pending[cursor].to_node_id) + if cursor is None or cursor not in position: + continue # a chain, or it joined an already-classified component + if not free: + raise InfeasiblePlacement( + f"cannot order {role} relocations: a rotation cycle " + f"({' -> '.join(path[position[cursor]:])}) has to be broken and " + f"no host slot is free to park a replica on") + scratch_host = free[0] + primary = sorted(path[position[cursor]:])[0] + move = pending[primary] + emit(ReplicaMove(primary, role, move.from_node_id, scratch_host, scratch=True)) + pending[primary] = ReplicaMove(primary, role, scratch_host, move.to_node_id) + vacated_by.pop(move.from_node_id, None) + vacated_by[scratch_host] = primary + + +def plan_moves( + current_layout: Mapping[str, Placement], + target_layout: Mapping[str, Placement], + all_node_ids: Sequence[str], + ftt: int, +) -> List[ReplicaMove]: + """:func:`diff_layout` followed by :func:`order_moves`.""" + return order_moves( + diff_layout(current_layout, target_layout, ftt), + current_layout, all_node_ids, ftt) + + +def describe_plan(plan: DiversityPlan, moves: Sequence[ReplicaMove]) -> str: + """One-line operator-facing summary, for the removal log.""" + scratch = sum(1 for m in moves if m.scratch) + status = "fully domain-diverse" if plan.full_diversity else "DEGRADED" + detail = f"{len(moves)} replica move(s)" + if scratch: + detail += f" ({scratch} scratch hop(s) to break rotation cycles)" + if plan.notes: + detail += "; " + "; ".join(plan.notes) + if plan.violations: + detail += f"; {len(plan.violations)} unresolved violation(s)" + return f"{status}: {detail}" diff --git a/simplyblock_core/rpc_client.py b/simplyblock_core/rpc_client.py index 848dfd3a95..bb310510a0 100644 --- a/simplyblock_core/rpc_client.py +++ b/simplyblock_core/rpc_client.py @@ -194,6 +194,17 @@ def __init__(self, message: str, code: int, data: Any = None): RPC_METHOD_NOT_FOUND = -32601 #: Returned instead of a result when the target does not implement the method. RPC_UNSUPPORTED = "__rpc_unsupported__" +#: ``jc_remove_jm``: the JM is still referenced by at least one jm_vuid, so JC +#: refuses to release it and the bdev must NOT be deleted. Deliberately a +#: module-local constant rather than a member of RPCErrorCode: the JC codes are +#: a separate, JC-specific space whose small negatives collide with that enum's +#: generic meanings (-1 there is invalid_state). See RPCClient.jc_remove_jm. +JC_REMOVE_JM_STILL_IN_USE = -22 +#: ``jc_remove_jm``: JC does not know this JM at all. After a successful +#: ``jc_replace_jm`` this is the NORMAL answer, not a failure -- measured live +#: 2026-09-02 on spdk R26.3: replacing the JM out of every vuid on a node also +#: drops it from JC, so the follow-up release finds nothing left to do. +JC_REMOVE_JM_NOT_USED = -13 class RPCClient: @@ -1912,6 +1923,51 @@ def jm_get_events(self): return None return result or [] + def jc_remove_jm(self, name): + """Close JC's descriptor + IO channel on a JM bdev and drop its JC + context, so the bdev itself can then safely be deleted. + + ``name``: the JM bdev name (e.g. ``remote_jm_n1``). It must be + known to JC and **unused by every jm_vuid** -- swap it out of each one + with ``jc_replace_jm`` first. + + This is the step that must come BEFORE deleting the bdev. Detaching the + controller while JC still holds a descriptor leaves JC referencing a + bdev that no longer exists (observed live 2026-09-02: a JC member list + naming a ``remote_jm_*`` bdev absent from that node's + ``bdev_get_bdevs``). + + Only one removal may be in progress cluster-side at a time. + + Returns True on success, or ``RPC_UNSUPPORTED`` on a build without the + RPC (checked, not assumed -- ``spdk:main-latest`` as of 2026-09-02 does + not expose it, so callers must degrade rather than fail). Any other + error raises ``RPCRemoteError``, whose ``.code`` distinguishes: + + -10 JC is closing + -11 invalid (empty) JM name + -12 this JM is already being removed, or another removal is running + -13 this JM is not used by JC (unknown name) + -21 involved in a pending jc_replace_jm + -22 STILL IN USE by one or more jm_vuids + -3 JC started closing during the operation + -6 timed out closing the JM (c_jc_tmo_ms_remove_jm) + + -22 is the useful one for node removal: it is positive proof that some + jm_vuid on this node still references the JM, including one the control + plane cannot enumerate (a vuid whose primary has already been removed + appears in no `decisions` map and under no back-reference). Treat it as + "do not delete this bdev", never as a transient failure. + """ + result, error = self._request2("jc_remove_jm", {"name": name}) + if error: + if error.get("code") == RPC_METHOD_NOT_FOUND: + return RPC_UNSUPPORTED + raise RPCRemoteError( + f"jc_remove_jm({name}) failed: {error.get('message')}", + error.get("code", 0), error.get("data")) + return result + def jc_compression_get_status(self, jm_vuid): """ Return value: diff --git a/simplyblock_core/services/main_distr_event_collector.py b/simplyblock_core/services/main_distr_event_collector.py index e003c4b357..69322f7e1f 100644 --- a/simplyblock_core/services/main_distr_event_collector.py +++ b/simplyblock_core/services/main_distr_event_collector.py @@ -453,6 +453,17 @@ def start_event_collector_on_node(node_id): try: while True: + # Same reason as the JM collector: removal leaves the record in + # place with status=removed, so nothing else here would ever end + # this loop and it would keep polling a node whose SPDK is gone. + try: + if db.get_storage_node_by_id(node_id).status == StorageNode.STATUS_REMOVED: + logger.info(f"Node {node_id} removed; stopping Distr collector") + return + except KeyError: + logger.info(f"Node {node_id} deleted; stopping Distr collector") + return + page = 1 events_groups: dict[Any, dict[Any, dict[Any, EventObj]]] = {} events_list = [] @@ -640,6 +651,12 @@ def start_jm_event_collector_on_node(node_id): try: snode = db.get_storage_node_by_id(node_id) except KeyError: + logger.info(f"Node {node_id} deleted; stopping JM collector") + return + if snode.status == StorageNode.STATUS_REMOVED: + # Removal does not delete the record, so the KeyError above + # never fires for a removed node -- this is what the + # message there always meant to catch. logger.info(f"Node {node_id} removed; stopping JM collector") return backlog_alerted = check_jm_compression_backlog( @@ -701,6 +718,17 @@ def ensure_collectors(nodes): """ for snode in nodes: node_id = snode.get_id() + if snode.status == StorageNode.STATUS_REMOVED: + # A removed node's record is NOT deleted -- it stays with + # status=removed -- so without this check we keep (re)starting + # collectors that RPC a node whose SPDK is gone, forever. Live + # 2026-09-02: 1036 "Failed to process JM events ... connection + # error" in the 1.5h after one removal, still going. The collector + # loops exit on removal too, but on their own that is not enough: + # this function restarts anything not alive within ~5s. + for source in ("distr", "jm"): + threads_maps.pop(f"{node_id}:{source}", None) + continue sources = [("distr", start_event_collector_on_node)] if node_id not in jm_unsupported_nodes: sources.append(("jm", start_jm_event_collector_on_node)) diff --git a/simplyblock_core/services/tasks_runner_failed_migration.py b/simplyblock_core/services/tasks_runner_failed_migration.py index 4b591d3b22..1869977087 100644 --- a/simplyblock_core/services/tasks_runner_failed_migration.py +++ b/simplyblock_core/services/tasks_runner_failed_migration.py @@ -1,6 +1,6 @@ # coding=utf-8 import time -from datetime import datetime +from datetime import datetime, timezone from simplyblock_core import db_controller, utils, constants from simplyblock_core.controllers import tasks_controller, device_controller @@ -48,7 +48,21 @@ def task_runner(task): for node in db.get_storage_nodes_by_cluster_id(task.cluster_id): if node.online_since: try: - diff = datetime.now() - datetime.fromisoformat(node.online_since) + # online_since is stamped timezone-aware + # (storage_node_ops.py:8823). A naive datetime.now() + # here raises TypeError on EVERY call, the except below + # swallows it, and the >>whole guard<< silently never + # fires -- failed-device migrations start immediately + # against a node that just came back online, which is + # exactly what this wait exists to prevent. The three + # sibling call sites (tasks_runner_migration.py:128, + # tasks_runner_new_dev_migration.py:73, + # storage_node_monitor.py:566) already pass + # timezone.utc; this one was missed. Observed 246 + # occurrences of "can't subtract offset-naive and + # offset-aware datetimes" across two node removals on + # 2026-09-11. + diff = datetime.now(timezone.utc) - datetime.fromisoformat(node.online_since) if diff.total_seconds() < 60: task.function_result = "node is online < 1 min, retrying" task.status = JobSchedule.STATUS_SUSPENDED diff --git a/simplyblock_core/services/tasks_runner_node_add.py b/simplyblock_core/services/tasks_runner_node_add.py index a6836bff53..2ef3ce837e 100644 --- a/simplyblock_core/services/tasks_runner_node_add.py +++ b/simplyblock_core/services/tasks_runner_node_add.py @@ -28,7 +28,7 @@ # dispatch loop never hands the same task to two workers. (Cross-host # duplicate execution is separately prevented by the per-task lease in # tasks_controller.claim_task.) -_inflight = set() +_inflight: set[str] = set() _inflight_lock = threading.Lock() # target node_addr values currently being driven by a worker, guarded by the @@ -42,7 +42,7 @@ # produced 6 node records for a 4-slot host). This is belt-and-suspenders # for the task-creation-time dedup — it protects against any duplicate task # that already exists, regardless of how it got created. -_inflight_addrs = set() +_inflight_addrs: set[str] = set() def process_task(task, cl): diff --git a/simplyblock_core/storage_node_ops.py b/simplyblock_core/storage_node_ops.py index e8faa83779..b3432abe8e 100644 --- a/simplyblock_core/storage_node_ops.py +++ b/simplyblock_core/storage_node_ops.py @@ -19,6 +19,7 @@ import docker from docker.types import LogConfig +from kubernetes.client import ApiException from pydantic import SecretStr from tenacity import RetryError, Retrying, before_sleep_log, retry_if_exception_type, stop_after_attempt, wait_fixed @@ -43,7 +44,9 @@ from simplyblock_core.release_upgrades import jc_compression_upgrade from simplyblock_core.models.cluster import Cluster from simplyblock_core.prom_client import PromClient -from simplyblock_core.rpc_client import RPCClient, RPCErrorCode, RPCRemoteError, RPCException, namespace_matches # noqa: F401 (RPCClient kept as a patch target for tests) +from simplyblock_core.rpc_client import ( + JC_REMOVE_JM_NOT_USED, JC_REMOVE_JM_STILL_IN_USE, RPC_UNSUPPORTED, + RPCErrorCode, RPCException, RPCRemoteError, namespace_matches) from simplyblock_core import rpc_client as rpc_client_module from simplyblock_core.snode_client import SNodeClient, SNodeClientException from simplyblock_core.utils import dial_backoff @@ -208,9 +211,17 @@ def _kill_spdk_until_dead(snode: StorageNode, max_attempts=3, poll_per_attempt_s silently left zombies behind. We now retry the kill until SPDK is confirmed down. Bounded total wall-clock = max_attempts * poll_per_attempt_sec so a wedged docker daemon cannot trap the caller. - Returns True if SPDK died, False if all attempts exhausted (caller is - responsible for whatever comes next; the node should still be marked - OFFLINE so it stops being treated as in_restart). + Returns True ONLY on a positive observation that SPDK is down -- a + liveness probe that answered and said "not up". A probe that could not be + made at all (node API unreachable) is UNKNOWN and never counts as death: + callers use the return value to decide whether it is safe to drop the + StorageNode record, and dropping it while the pod is still alive leaves a + pod that nothing references and nothing can reap. + + False means "not confirmed dead" -- either SPDK was still up after every + attempt, or the probe never answered. The caller is responsible for + whatever comes next; the node should still be marked OFFLINE so it stops + being treated as in_restart. """ snode_api = snode.client(timeout=5, retry=5) # Each attempt is bounded BOTH ways, and needs both. The wall-clock deadline @@ -221,6 +232,7 @@ def _kill_spdk_until_dead(snode: StorageNode, max_attempts=3, poll_per_attempt_s # no-op, which turns a bare deadline loop into a hot spin burning the whole # poll_per_attempt_sec of CPU per attempt. rounds_per_attempt = max(1, int(poll_per_attempt_sec / poll_interval)) + last_probe_error = None for attempt in range(1, max_attempts + 1): try: snode_api.spdk_process_kill(snode.rpc_port, snode.cluster_id) @@ -240,9 +252,24 @@ def _kill_spdk_until_dead(snode: StorageNode, max_attempts=3, poll_per_attempt_s # kill loop would never observe SPDK as down (it would burn all # attempts and log a false "did NOT die" even after a clean kill). up, _ = snode_api.spdk_process_is_up(snode.rpc_port, snode.cluster_id) - except Exception: - up = False - if not up: + probed = True + except Exception as probe_err: + # UNKNOWN, not down. snode_client raises SNodeClientException + # whenever the node's API cannot be reached at all, which is + # the normal state in the very situation this function is + # called for -- the add failed BECAUSE that host stopped + # answering. Reporting "confirmed down" here makes the caller + # drop the StorageNode record while the pod is still running, + # and a pod with no record and no ownerReference is + # unreachable by every cleanup path there is: it keeps the + # host's hugepages and every later add-node attempt on it + # stays Pending (observed 2026-09-11, worker tqmtr, rpc_port + # 4426 -- the deploy wedged at 11/12 until the pod was deleted + # by hand). + last_probe_error = probe_err + probed = False + up = True + if probed and not up: logger.info( "SPDK on %s confirmed down (kill attempt %d/%d)", snode.get_id(), attempt, max_attempts, @@ -255,12 +282,22 @@ def _kill_spdk_until_dead(snode: StorageNode, max_attempts=3, poll_per_attempt_s snode.get_id(), poll_per_attempt_sec, attempt, max_attempts, ) - logger.error( - "SPDK on %s did NOT die after %d kill attempts (%ds total) — " - "investigate snode_api / docker daemon health on %s", - snode.get_id(), max_attempts, - max_attempts * poll_per_attempt_sec, snode.mgmt_ip, - ) + if last_probe_error is not None: + logger.error( + "Could not confirm SPDK on %s is down after %d kill attempts (%ds total): " + "the liveness probe never answered (%s). Returning failure so the caller " + "keeps the node record — dropping it now would leave the pod running with " + "nothing referencing it. Investigate snode_api health on %s", + snode.get_id(), max_attempts, + max_attempts * poll_per_attempt_sec, last_probe_error, snode.mgmt_ip, + ) + else: + logger.error( + "SPDK on %s did NOT die after %d kill attempts (%ds total) — " + "investigate snode_api / docker daemon health on %s", + snode.get_id(), max_attempts, + max_attempts * poll_per_attempt_sec, snode.mgmt_ip, + ) return False @@ -2914,6 +2951,136 @@ def apply_cluster_hugepages(snode_api, node_config, req_cpu_count, max_prov): return huge_page_memory +class _SpdkPodStillPresent(Exception): + """The SPDK pod has not disappeared from the API server yet.""" + + +def _delete_spdk_pod_via_k8s(rpc_port, cluster_id): + """Delete the SPDK pod (and its fluentd companion) straight from the + Kubernetes API, without routing the request through the node agent. + + ``snode_api.spdk_process_kill`` already does exactly this work -- see + ``spdk_process_kill`` in + simplyblock_web/api/internal/storage_node/kubernetes.py, which is a plain + ``delete_namespaced_pod``. Nothing about it is node-local. The catch is + WHERE it runs: on the agent hosted by the very node being torn down. In + the most common abort case the add failed BECAUSE that host went away -- + an agent crash, or the one-off CPU-topology reboot -- so the request + cannot be delivered at all and the pod survives with nothing referencing + it (2026-09-11, worker gddnr, rpc_port 4436: the abort handler fired + correctly and then got "Connection refused" from the agent while the node + rebooted). Talking to the API server directly takes the dead host out of + the path. + + Returns ``(ok, err)``. ``ok`` is True only once the pod is confirmed gone, + since a pod still Terminating is still holding the host's hugepages. + """ + six = utils.first_six_chars(cluster_id) + pod_name = f"snode-spdk-pod-{rpc_port}-{six}" + fluent_pod_name = f"simplyblock-fluentd-{rpc_port}-{six}" + namespace = constants.K8S_NAMESPACE + + try: + k8s_core = utils.get_k8s_core_client() + except Exception as e: + return False, f"no Kubernetes API client available: {e}" + + try: + k8s_core.delete_namespaced_pod(pod_name, namespace) + logger.info(f"Deleted SPDK pod {pod_name} directly via the Kubernetes API") + except ApiException as e: + if e.status != 404: + return False, f"failed to delete pod {pod_name}: {e.body}" + logger.info(f"SPDK pod {pod_name} was already gone") + except Exception as e: + return False, f"failed to delete pod {pod_name}: {e}" + + # Companion log shipper: best effort, never fails the teardown. It holds + # no hugepages, so leaving it behind does not block a later add. + try: + k8s_core.delete_namespaced_pod(fluent_pod_name, namespace) + logger.info(f"Deleted fluentd pod {fluent_pod_name}") + except ApiException as e: + if e.status != 404: + logger.warning(f"Failed to delete fluentd pod {fluent_pod_name}: {e.body}") + except Exception as e: + logger.warning(f"Failed to delete fluentd pod {fluent_pod_name}: {e}") + + # Confirm it is really gone. Same budget as the agent-side implementation. + try: + for attempt in Retrying( + stop=stop_after_attempt(10), + wait=wait_fixed(3), + retry=retry_if_exception_type(_SpdkPodStillPresent), + before_sleep=before_sleep_log(logger, logging.WARNING), + reraise=True, + ): + with attempt: + pods = k8s_core.list_namespaced_pod(namespace) + if any(p.metadata.name.startswith(pod_name) for p in pods.items): + raise _SpdkPodStillPresent(pod_name) + except _SpdkPodStillPresent: + return False, f"pod {pod_name} still present 30s after delete" + except Exception as e: + return False, f"could not confirm pod {pod_name} is gone: {e}" + + return True, "" + + +def _abort_started_spdk(snode_api, rpc_port, cluster_id, reason, cluster_mode=None): + """Tear down the SPDK pod add_node just started, before bailing out. + + Between ``spdk_process_start`` and the point the StorageNode record is + written there is no DB row pointing at the pod, so nothing else can ever + find it: it has no owner reference for Kubernetes to reap and no node + record for the control plane to reconcile. Left behind it keeps holding + the host's hugepages, and every later add-node attempt on that host gets + "Insufficient hugepages-2Mi" and stays Pending -- which stalls the + operator's serialised node-add queue and wedges the whole deployment + (observed on fresh 12-node deploys 2026-09-08 and 2026-09-11; recovery + was a manual delete of the unowned pod). + + In kubernetes mode the teardown goes straight to the API server + (``_delete_spdk_pod_via_k8s``) rather than through the node agent, + because the agent runs ON the host we are abandoning and is routinely + unreachable in exactly this situation. The agent call remains the + fallback, and the only path in docker mode, where the control plane has + no way to reach the container itself. + + The rpc_port reservation is deliberately NOT released: it has no release + API and ages out by TTL (see reserve_cluster_nvmf_port), and dropping it + here would hand the port to a concurrent add while this pod is still + shutting down. + """ + logger.error(f"add_node aborting after SPDK start: {reason}") + + if cluster_mode == "kubernetes": + ok, err = _delete_spdk_pod_via_k8s(rpc_port, cluster_id) + if ok: + logger.info( + f"Cleaned up SPDK pod on port {rpc_port} after failed add_node " + f"(direct Kubernetes delete)") + return + logger.warning( + f"Direct Kubernetes teardown of the SPDK pod on port {rpc_port} failed " + f"({err}); falling back to the node agent") + + try: + ok, err = snode_api.spdk_process_kill(rpc_port, cluster_id) + if ok: + logger.info(f"Cleaned up SPDK pod on port {rpc_port} after failed add_node") + else: + logger.error( + f"Could not clean up SPDK pod on port {rpc_port} after failed add_node: " + f"{err}. It holds this host's hugepages and will block further " + f"add-node attempts until deleted by hand.") + except Exception as e: + logger.error( + f"Could not clean up SPDK pod on port {rpc_port} after failed add_node: {e}. " + f"It holds this host's hugepages and will block further add-node attempts " + f"until deleted by hand.") + + def add_node(cluster_id, node_addr, iface_name, data_nics_list, max_snap, spdk_image=None, spdk_debug=False, small_bufsize=0, large_bufsize=0, @@ -3386,11 +3553,15 @@ def add_node(cluster_id, node_addr, iface_name, data_nics_list, time.sleep(5) except Exception as e: - logger.error(e) + # spdk_process_start may have created the pod before raising. + _abort_started_spdk(snode_api, rpc_port, cluster_id, str(e), + cluster_mode=cluster.mode) return False if not results: - logger.error(f"Failed to start spdk: {err}") + _abort_started_spdk(snode_api, rpc_port, cluster_id, + f"Failed to start spdk: {err}", + cluster_mode=cluster.mode) return False number_of_alceml_devices = node_config.get("number_of_alcemls") # Increase number of alcemls by one for the JM @@ -3468,7 +3639,9 @@ def add_node(cluster_id, node_addr, iface_name, data_nics_list, active_tcp = True if not active_tcp and not active_rdma: - logger.error("No usable storage network interface found.") + _abort_started_spdk(snode_api, rpc_port, cluster_id, + "No usable storage network interface found.", + cluster_mode=cluster.mode) return False hostname = node_info['hostname'] + f"_{rpc_port}" @@ -4110,7 +4283,35 @@ def remove_storage_node(node_id, force_remove=False, force_migrate=False): def _check_replica_relocation_feasible(removed_node: StorageNode, db_controller): """Pre-flight Case-B check: a secondary/tertiary replica hosted on ``removed_node`` for some OTHER primary must have a valid relocation target. - Returns (feasible: bool, reason: str).""" + Returns (feasible: bool, reason: str). + + When the global placement planner applies (see + ``_relocation_planner_inputs``) this asks the same planner phase 3b will + use, so admission and execution can never disagree: the removal is + refused only when no host-disjoint layout exists at all, and a layout + that is merely not fully domain-diverse is admitted with a warning naming + every LVS that will end up degraded. The per-role probe below stays as + the fallback for clusters the planner declines.""" + from simplyblock_core.controllers import replica_placement + + inputs = _relocation_planner_inputs(removed_node, db_controller, allow_without_fd=True) + if inputs is not None: + surviving_ids, fd_by_node, host_by_node, label_by_node, current_layout, ftt = inputs + try: + plan = replica_placement.plan_diverse_layout( + surviving_ids, fd_by_node, current_layout, ftt, + host_by_node=host_by_node, label_by_node=label_by_node) + except replica_placement.InfeasiblePlacement as e: + return False, str(e) + if not plan.full_diversity: + logger.warning( + f"[REMOVAL] {removed_node.get_id()}: the post-removal layout " + f"cannot be made fully domain-diverse: " + f"{'; '.join(plan.notes) or 'no reason recorded'}") + for violation in plan.violations: + logger.warning(f"[REMOVAL] {removed_node.get_id()}: {violation}") + return True, "" + for backref, picker in ( ("lvstore_stack_secondary", "secondary"), ("lvstore_stack_tertiary", "tertiary")): @@ -4129,16 +4330,50 @@ def _check_replica_relocation_feasible(removed_node: StorageNode, db_controller) return True, "" -def _pick_replica_relocation_node(primary, removed_node: StorageNode, role, db_controller): +def _pick_replica_relocation_node(primary, removed_node: StorageNode, role, db_controller, + extra_exclude_ids=()): """Choose a node to re-host ``primary``'s ``role`` (secondary|tertiary) replica, currently on ``removed_node``. Returns a node id or None. Reuses the existing anti-affinity-aware placement helpers. - With failure domains enabled, the >=1-cross-domain-role invariant is - enforced HARD here (not best-effort): if the primary's OTHER non-leader - role does not already live in a different domain than the primary, the - replacement must — otherwise a full-domain outage would leave the LVS - with zero surviving paths. + ``extra_exclude_ids``: additional node ids to rule out beyond + ``removed_node`` itself -- used by ``_relocate_replica_between``'s + nested vacate-rotation to exclude the primary currently being spliced + in the ENCLOSING call. Without this, the only candidate this call finds + can be exactly that in-flight primary (structurally invalid: it can't + simultaneously be the thing being relocated onto a node AND the target + something else relocates onto), and the caller's own guard against that + just gives up rather than searching further (2026-08-28 finding: a + three-way chain -- primary A splices into an existing pairing, + displacing B onto A's target, but B's only free candidate for the role + being vacated turns out to be A itself, mid-relocation -- got stuck + retrying the identical failure forever instead of looking past A). + + With failure domains enabled, prefers FULL pairwise domain diversity + across {primary, secondary, tertiary} — not just the weaker ">=1 + cross-domain role" floor this used to settle for. That floor let the + role NOT being relocated stay wherever it already was and placed the + one being relocated in ANY domain at all (including the primary's own, + or the other role's) as long as one non-leader path stayed cross-domain + — a real, live-confirmed gap (2026-08-27: after two uneven removals + shrank two of four domains to 2 hosts, several nodes ended up with + secondary and tertiary sharing a domain, or a tertiary sharing the + primary's own domain, purely because the other role already happened + to be cross-domain when this ran). Tries, in order: + 1. a direct candidate diverse from BOTH the primary and the other + already-assigned role; + 2. splicing into an existing pairing whose far end is also diverse + from both (see ``_find_splice_target_for_relocation``'s + ``avoid_domains``); + 3. the original ">=1 cross-domain" floor (direct candidate, then + splice) if neither of the above found anything — full diversity + genuinely isn't achievable right now (e.g. domains have shrunk + unevenly) and refusing the relocation outright would strand the + node-removal instead. Logged loudly so the degraded outcome is + visible rather than silently matching the old behavior; + 4. same-domain-blind last resort (direct candidate, then splice) + when there's no other role to be diverse from at all, or FD data + is absent/invalid — unchanged from before. ``get_secondary_nodes``/``get_secondary_nodes_2`` only ever offer UNCLAIMED nodes (each node hosts at most one secondary/tertiary at a @@ -4154,10 +4389,10 @@ def _pick_replica_relocation_node(primary, removed_node: StorageNode, role, db_c pairing (see ``_find_splice_target_for_relocation``) — exactly the fix ``splice_stranded_secondary``/``splice_stranded_tertiary`` already apply to the identical dead end at cluster-activation time. Only returning - None here (both searches exhausted) makes + None (every step above exhausted) makes ``_check_replica_relocation_feasible`` refuse the removal up front. """ - exclude_ids = [removed_node.get_id()] + exclude_ids = [removed_node.get_id(), *extra_exclude_ids] if role == "secondary": other_id = primary.tertiary_node_id if primary.tertiary_node_id and primary.tertiary_node_id != removed_node.get_id(): @@ -4177,37 +4412,96 @@ def _pick_replica_relocation_node(primary, removed_node: StorageNode, role, db_c primary, exclude_ids=exclude_ids, exclude_mgmt_ips=exclude_mgmt_ips) cluster = db_controller.get_cluster_by_id(primary.cluster_id) - if cands and getattr(cluster, "enable_failure_domain", False) and primary.failure_domain >= 0: - other_cross = False - if other_id and other_id != removed_node.get_id(): + fd_enabled = bool(getattr(cluster, "enable_failure_domain", False) and primary.failure_domain >= 0) + + def first_diverse(domains): + for cand_id in cands: try: - other = db_controller.get_storage_node_by_id(other_id) - other_cross = (other.failure_domain >= 0 - and other.failure_domain != primary.failure_domain) + cand = db_controller.get_storage_node_by_id(cand_id) except KeyError: - pass - if not other_cross: - # The replacement is the primary's only cross-domain role — - # same-domain candidates are not acceptable. - for cand_id in cands: - try: - cand = db_controller.get_storage_node_by_id(cand_id) - except KeyError: - continue - if (cand.failure_domain >= 0 - and cand.failure_domain != primary.failure_domain): - return cand_id - else: + continue + if cand.failure_domain >= 0 and cand.failure_domain not in domains: + return cand_id + return None + + if not fd_enabled: + if cands: return cands[0] - elif cands: - return cands[0] + splice = _find_splice_target_for_relocation( + primary, role, db_controller, exclude_ids=exclude_ids + [primary.get_id()]) + return splice[1] if splice else None + + other_fd = None + if other_id and other_id != removed_node.get_id(): + try: + other = db_controller.get_storage_node_by_id(other_id) + if other.failure_domain >= 0: + other_fd = other.failure_domain + except KeyError: + # The other role's holder is already gone. other_fd stays None, so + # full_avoid carries only the primary's own domain -- the placement + # is still diverse from everything that actually exists. Relaxing + # by one here is correct rather than merely tolerable: demanding + # diversity from a departed node would rule out hosts that are + # genuinely free. + logger.debug( + "no record for %s's other-role holder %s; planning %s placement " + "without its domain", primary.get_id(), other_id, role) + + full_avoid = {primary.failure_domain} + if other_fd is not None: + full_avoid.add(other_fd) + # 1. Direct candidate, fully diverse from both the primary and the + # other already-assigned role. + if cands: + found = first_diverse(full_avoid) + if found: + return found + + # 2. Splice into an existing pairing whose far end is also fully diverse. + splice = _find_splice_target_for_relocation( + primary, role, db_controller, exclude_ids=exclude_ids + [primary.get_id()], + avoid_domains=full_avoid) + if splice: + return splice[1] + + # 3. Full diversity isn't achievable anywhere in the cluster right now. + # Relax to the >=1-cross-domain floor (diverse from the primary alone) + # rather than refuse the relocation outright — only when there IS an + # other role to have been diverse from, i.e. this is a genuine relax, + # not silently skipping the check. + if other_fd is not None: + weak_avoid = {primary.failure_domain} + found = first_diverse(weak_avoid) if cands else None + if found: + logger.warning( + f"[REMOVAL] {primary.get_id()}: no candidate keeps {role} fully domain-diverse " + f"from both the primary (domain {primary.failure_domain}) and its other " + f"replica (domain {other_fd}); falling back to {found}, cross-domain from " + f"the primary only") + return found + splice = _find_splice_target_for_relocation( + primary, role, db_controller, exclude_ids=exclude_ids + [primary.get_id()], + avoid_domains=weak_avoid) + if splice: + logger.warning( + f"[REMOVAL] {primary.get_id()}: no fully domain-diverse splice target for " + f"{role} either; falling back to splicing {splice[0]} -> {splice[1]}, " + f"cross-domain from the primary only") + return splice[1] + + # 4. Same-domain-blind last resort: no other role to be diverse from, + # or nothing satisfies even the weaker floor. + if cands: + return cands[0] splice = _find_splice_target_for_relocation( primary, role, db_controller, exclude_ids=exclude_ids + [primary.get_id()]) return splice[1] if splice else None -def _find_splice_target_for_relocation(stranded_primary, role, db_controller, exclude_ids=()): +def _find_splice_target_for_relocation(stranded_primary, role, db_controller, exclude_ids=(), + avoid_domains=frozenset()): """Find an already-formed pairing ``P -> X`` (``P. == X``) elsewhere in the cluster to splice ``stranded_primary`` into: ``P -> stranded_primary -> X``. Read-only — callers decide whether and @@ -4222,6 +4516,32 @@ def _find_splice_target_for_relocation(stranded_primary, role, db_controller, ex to splice into a pairing that already has real data on both ends; executing that move (not just picking the edge) is the caller's job. + ``avoid_domains``: X (the node ``stranded_primary`` would actually end + up hosted on) is excluded outright, not just scored down, when its + domain is in this set. ``_pick_replica_relocation_node`` uses this to + keep a spliced-in replacement fully diverse from BOTH the primary and + its other already-assigned role — the plain domain-mismatch scoring + below only ever knows about diversity from ``stranded_primary`` itself, + which isn't enough to keep two independently-placed roles apart. + + P's OWN other role (its tertiary if ``role`` is "secondary", or vice + versa, untouched by this splice) is also considered: repointing P's + ``field`` from X onto ``stranded_primary`` must not put P in the exact + state this diversity fix exists to prevent -- two of P's own roles + sharing a domain. This is a *preference*, not a hard filter -- among + all valid edges, one whose P stays fully diverse afterwards is always + picked over one that doesn't (ties broken by the existing domain- + mismatch score below), but a colliding edge is still accepted, with a + warning, when it's the only one available. In a real multi-domain + cluster there are usually several candidate edges, so this alone + resolves the common case without ever refusing a repair that a less + picky search would have found (2026-08-27 finding: splicing kc25l into + 56mg5's secondary slot collided with 56mg5's pre-existing, untouched + tertiary in the same domain, when another edge elsewhere in the ring + -- t74sg's -- was collision-free the whole time; the old avoid_domains- + only check had no way to prefer it, since avoid_domains only ever + looked at X's domain, never P's). + Returns ``(p_id, x_id)`` or ``None`` if no valid edge exists. """ field = "secondary_node_id" if role == "secondary" else "tertiary_node_id" @@ -4252,8 +4572,9 @@ def _domain_mismatch_score(*nodes): return sum(1 for n in nodes if n.failure_domain != stranded_primary.failure_domain) edges = [n for n in all_nodes if getattr(n, field) and n.get_id() not in exclude] + other_field = "tertiary_node_id" if field == "secondary_node_id" else "secondary_node_id" - best, best_score = None, -1 + best, best_key, best_collides = None, None, False for p in edges: x_id = getattr(p, field) if x_id in exclude: @@ -4261,6 +4582,22 @@ def _domain_mismatch_score(*nodes): x = by_id.get(x_id) if not x or not _online(p, x): continue + if avoid_domains and x.failure_domain in avoid_domains: + continue + # Once spliced, P. is repointed onto stranded_primary itself + # (see _relocate_replica_between) -- P's role-target BECOMES + # stranded_primary, exactly as fundamental an invariant as X's + # domain above: 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. Hard-excluded like X's domain, not merely + # scored down -- unlike P's OTHER role below, there is no + # legitimate "nothing better exists" case here, since this is the + # exact same guarantee _pick_replica_relocation_node's own + # full_avoid already enforces for X (2026-08-28 finding: this stayed + # a soft preference from before today's diversity work and let a + # live splice give p59j8 a tertiary target in its own domain). + if stranded_primary.failure_domain >= 0 and p.failure_domain == stranded_primary.failure_domain: + continue if role == "secondary": if p.mgmt_ip == stranded_primary.mgmt_ip or x.mgmt_ip == stranded_primary.mgmt_ip: continue @@ -4270,10 +4607,37 @@ def _domain_mismatch_score(*nodes): continue if not _valid_tertiary(stranded_primary, stranded_sec, x): continue + + # Prefer an edge that leaves P's OWN other role (its tertiary if + # `role` is "secondary", or vice versa -- untouched by this splice) + # diverse from stranded_primary once P. is repointed onto + # it. A collision here degrades a node that was never part of this + # removal at all, so it's ranked below every non-colliding edge -- + # but still accepted as a last resort rather than refused outright + # (2026-08-27 finding: with several candidate edges usually + # available in a real cluster, one of them is typically collision- + # free; the old code had no way to prefer it, so an outright reject + # here would have been more restrictive than useful). + p_other_id = getattr(p, other_field) + collides = False + if p_other_id and p_other_id != stranded_primary.get_id(): + p_other = by_id.get(p_other_id) + if (p_other and stranded_primary.failure_domain >= 0 + and p_other.failure_domain == stranded_primary.failure_domain): + collides = True + score = _domain_mismatch_score(p, x) - if score > best_score: - best_score, best = score, (p.get_id(), x.get_id()) + key = (not collides, score) + if best_key is None or key > best_key: + best_key, best, best_collides = key, (p.get_id(), x.get_id()), collides + if best and best_collides: + logger.warning( + f"[REMOVAL] splice {best[0]} -> {stranded_primary.get_id()} -> {best[1]}: " + f"no candidate edge leaves {best[0]}'s other role diverse from " + f"{stranded_primary.get_id()}'s domain {stranded_primary.failure_domain}; " + f"using it anyway as the least-bad option -- {best[0]} is now degraded " + f"by a removal it wasn't otherwise part of") return best @@ -4355,6 +4719,56 @@ def node_removal_orchestrate(node_id, force_remove=False): # 2026-08-25: this node's own hosted-replica peer failed the very # next removal after the phase 2/3b reorder that fixed the # relocation-timing gap). + # Captured BEFORE phase 3a, which clears both this node's + # secondary/tertiary pointers and those peers' back-references. + # These two peers are the ones left running a JC instance for THIS + # node's own jm_vuid, and phase 2 cannot find them any other way -- + # see _decommission_node_jm's replica_peer_ids. + replica_peer_ids = tuple( + pid for pid in (snode.secondary_node_id, snode.tertiary_node_id) if pid) + + # Phase 0 — prove phase 3b has a valid layout BEFORE phase 3a + # destroys anything. + # + # 3a is irreversible: it tears down this node's own replicas and + # clears the pointers naming them. 3b, which places the replicas + # this node hosts for OTHER primaries, only discovers whether a + # layout exists when it runs -- after 3a. A 3b failure therefore + # returns False into a task runner that retries the whole + # sequence, and every retry re-enters a 3a with nothing left to + # tear down and reaches the same 3b in the same state. Retry + # cannot help, but it is what happens: observed 2026-09-09, a + # removal retried 68 times over 11 minutes with the node stuck in + # in_removal and one lvstore left on a single member the whole + # time, until the task was cancelled by hand. + # + # Asking the planner here costs one matching computation and + # turns that unrecoverable state into a clean refusal: nothing is + # destroyed, the node stays ONLINE, and the removal can simply be + # retried later once the cluster can host the layout. + # + # This repeats the admission-time check in remove_storage_node on + # purpose. That one runs when the task is QUEUED, which can be + # minutes before it is executed, and the cluster can change in + # between (a peer going unreachable, another removal finishing). + # The check that matters is the one immediately before the + # destruction. + # + # The layout is validated, not persisted: plan_diverse_layout is + # a deterministic min-cost matching over the survivors' forward + # pointers, and 3a/2 change only THIS node's forward pointers and + # the peers' back-references -- neither of which it reads -- so + # 3b recomputes the same answer. Persisting it would add a stale + # plan to apply against a cluster that has since moved. + feasible, reason = _check_replica_relocation_feasible(snode, db_controller) + if not feasible: + logger.error( + f"[REMOVAL] {node_id}: refusing before phase 3a — no valid layout " + f"for the replicas this node hosts: {reason}. Nothing has been torn " + f"down; the node is still usable and the removal can be retried " + f"once the cluster can host the relocation.") + return False + logger.info(f"[REMOVAL] {node_id}: phase 3a — tear down own replicas") if not _teardown_replicas_of_primary(snode): return False @@ -4366,7 +4780,7 @@ def node_removal_orchestrate(node_id, force_remove=False): # in its primary's jm_ids bakes that unreachable member into the # new host's construct permanently. logger.info(f"[REMOVAL] {node_id}: phase 2 — decommission JM") - _decommission_node_jm(snode) + _decommission_node_jm(snode, replica_peer_ids=replica_peer_ids) snode = db_controller.get_storage_node_by_id(node_id) # Phase 3b — relocate replicas this node hosts for OTHER primaries (Case B). @@ -4374,6 +4788,16 @@ def node_removal_orchestrate(node_id, force_remove=False): if not _relocate_replicas_hosted_on(snode): return False + # Phase 3c — prove the relocations actually landed. Every pointer + # phase 3b writes is bookkeeping; this is the only step that asks + # the devices. Reported, not fatal: by here the removal is + # physically done and the node is on its way out, so failing would + # only spin the retry loop against a state it cannot re-drive -- + # but a missing replica must never leave this function silently. + logger.info(f"[REMOVAL] {node_id}: phase 3c — verify replica stacks") + _verify_replica_stacks(snode.cluster_id, db_controller, + context=f" after removing {node_id}") + # Phase 4 — finalize (swarm leave, gpt cleanup) and flip to removed. logger.info(f"[REMOVAL] {node_id}: phase 4 — finalize") _finalize_node_removal(snode) @@ -4395,6 +4819,86 @@ def node_removal_orchestrate(node_id, force_remove=False): return True +def replica_stack_violations(nodes, stack_present): + """Nodes whose PHYSICAL replica stacks disagree with their bookkeeping. + + ``nodes`` is the set of ONLINE nodes; ``stack_present(node, lvstore)`` + reports whether ``lvstore`` is actually surfaced on that node's SPDK. + + The invariant: a node physically holds one lvstore per primary its + back-references claim it hosts. Every forward pointer + (``secondary_node_id`` / ``tertiary_node_id``), back-reference + (``lvstore_stack_secondary`` / ``_tertiary``) and ``lvstore_ports`` entry + can agree perfectly and still describe a replica that is not there -- + they are all bookkeeping, written by the same code path, and none of them + is evidence that ``raid0_`` + ``LVS_`` exist on the host. + + This is the check that was missing. Its absence is why a relocation could + delete a just-installed replica (see _relocate_replica_between's + same-primary/other-role guard) and have the removal report success: + nothing ever compared the claim against the device. Found live + 2026-09-01, and only by dumping bdev_lvol_get_lvstores on all ten + survivors by hand -- two of them were down to a single real replica for + an FTT2 lvstore, with no error logged anywhere in the removal. + + Scoped deliberately to HOSTED replicas (what the back-references claim), + not a node's own primary lvstore: a primary can be legitimately in flux + mid-flow, and a false alarm there would train the reader to ignore this. + + Returns a list of ``(node_id, lvstore, owner_primary_id, role)`` for each + claimed-but-absent stack; empty means the invariant holds. + """ + by_id = {n.get_id(): n for n in nodes} + missing = [] + for node in nodes: + for backref, role in (("lvstore_stack_secondary", "secondary"), + ("lvstore_stack_tertiary", "tertiary")): + owner_id = getattr(node, backref, "") + if not owner_id: + continue + owner = by_id.get(owner_id) + if owner is None or not owner.lvstore: + # Owner gone or has no lvstore -- a bookkeeping problem of a + # different kind, and not something a stack probe can settle. + continue + if not stack_present(node, owner.lvstore): + missing.append((node.get_id(), owner.lvstore, owner_id, role)) + return missing + + +def _verify_replica_stacks(cluster_id, db_controller, context=""): + """Probe every online node's hosted replica stacks and log any that are + missing. Returns the violation list (empty when the invariant holds). + + An unreachable node is NOT reported as a violation: absence of proof is + not proof of absence, and a probe that cries wolf on a transient RPC + error is a check people learn to skip. + """ + nodes = [n for n in db_controller.get_storage_nodes_by_cluster_id(cluster_id) + if n.status == StorageNode.STATUS_ONLINE] + + def stack_present(node, lvstore): + try: + return bool(node.rpc_client(timeout=10, retry=1).bdev_lvol_get_lvstores(lvstore)) + except Exception as e: + logger.warning( + f"[REMOVAL] could not probe {lvstore} on {node.get_id()} " + f"({e}); not counting it as missing") + return True + + violations = replica_stack_violations(nodes, stack_present) + for node_id, lvstore, owner_id, role in violations: + logger.error( + f"[REMOVAL] REPLICA STACK MISSING{context}: {node_id} is recorded as " + f"{role} of {owner_id} but {lvstore} is not present on it -- " + f"{owner_id} is running with one fewer replica than its bookkeeping claims") + if not violations: + logger.info( + f"[REMOVAL] replica-stack invariant holds{context}: every hosted " + f"replica claimed by a back-reference is physically present") + return violations + + def _teardown_replicas_of_primary(removed_node: StorageNode): """Case A: the primary LVS lives on ``removed_node`` (now shut down). Delete its secondary/tertiary replicas from the peers that host them and @@ -4584,9 +5088,20 @@ def _update_lvol_nodes_for_replica_move(primary_id, old_host_id, new_host_id, db def _relocate_replicas_hosted_on(removed_node: StorageNode): """Case B: ``removed_node`` holds a secondary and/or tertiary replica for other primaries. Re-host each on a fresh, anti-affinity-valid node so the - owning primary keeps its fault tolerance after this node leaves.""" + owning primary keeps its fault tolerance after this node leaves. + + With failure domains enabled this goes through the global planner + (``replica_placement``), which solves the whole post-removal layout at + once instead of re-homing each stranded replica in isolation -- see + ``_plan_driven_relocation``. The per-replica greedy path below is the + fallback for the cases the planner deliberately does not take + (FD disabled, dedicated secondary nodes, a peer that is not ONLINE).""" db_controller = DBController() + handled = _plan_driven_relocation(removed_node, db_controller) + if handled is not None: + return handled + removed_node = db_controller.get_storage_node_by_id(removed_node.get_id()) if removed_node.lvstore_stack_secondary: if not _relocate_one_replica(removed_node, removed_node.lvstore_stack_secondary, "secondary"): @@ -4600,6 +5115,182 @@ def _relocate_replicas_hosted_on(removed_node: StorageNode): return True +def _relocation_planner_inputs(removed_node: StorageNode, db_controller, + *, allow_without_fd: bool = False): + """Gather the pure inputs the global placement planner needs, or ``None`` + when this cluster is not a case the planner handles. + + Returns ``(surviving_ids, fd_by_node, host_by_node, label_by_node, + current_layout, ftt)``. ``current_layout`` is read straight off the + primaries' ``secondary_node_id``/``tertiary_node_id`` pointers, including + the ones that still name ``removed_node`` -- the planner treats a holder + outside the surviving set as "no host", and the diff then carries + ``removed_node`` as the move's origin so the existing mover can tear the + old copy down and clear its back-reference exactly as before. + + With ``allow_without_fd`` the planner is also offered clusters that have + failure domains OFF: every host becomes its own pseudo-domain, so the + "domains pairwise distinct" constraint degenerates to exactly the + host-disjointness the planner already enforces. This exists because the + greedy per-role path places one stranded replica at a time and never + backtracks, so on a dense cluster (every survivor already at capacity) it + can consume the one slot another stranded replica needed and then refuse + a removal for which a valid host-disjoint layout demonstrably exists. + + BOTH callers -- the admission check and phase 3b -- must pass the same + value. They are the same decision asked at two moments; if admission + consults the planner and execution does not, an admitted removal reaches + phase 3b, fails there, and retries forever with the node stranded in + ``in_removal`` (observed 2026-09-08, task retried 68 times before being + cancelled by hand). + + Declines (returns ``None``) when: + + * failure domains are off and ``allow_without_fd`` was not passed; + * failure domains are off and there are ``ftt`` or fewer survivors -- + no layout exists, and refusing would change long-standing behaviour + for deliberate shrink-to-tiny removals; + * failure domains are ON but any surviving node has no failure domain + set -- a partial domain map cannot be reasoned about, only guessed at; + * the cluster has dedicated secondary nodes (``is_secondary_node``), + which may host more than one replica each and so break the + one-slot-per-node permutation model the planner is built on; + * any surviving node is not ONLINE -- the planner would happily place a + replica on a node that cannot build it. + """ + from simplyblock_core.controllers import replica_placement + + cluster = db_controller.get_cluster_by_id(removed_node.cluster_id) + fd_enabled = bool(getattr(cluster, "enable_failure_domain", False)) + if not fd_enabled and not allow_without_fd: + return None + ftt = cluster.max_fault_tolerance if cluster.max_fault_tolerance in (1, 2) else 1 + + all_nodes = db_controller.get_storage_nodes_by_cluster_id(removed_node.cluster_id) + survivors = [ + n for n in all_nodes + if n.get_id() != removed_node.get_id() and n.status != StorageNode.STATUS_REMOVED + ] + if not survivors: + return None + if not fd_enabled and len(survivors) <= ftt: + # Too small for the permutation model to have any answer: with ftt or + # fewer survivors no layout exists by construction. Under domains that + # is a real refusal (the caller wants to know), but with domains off + # it would newly reject shrink-to-tiny removals the greedy path has + # always allowed -- e.g. a 2-node cluster dropping to 1. Leave those + # exactly as they were. + return None + if any(n.is_secondary_node for n in survivors): + return None + if any(n.status != StorageNode.STATUS_ONLINE for n in survivors): + return None + if fd_enabled and any(n.failure_domain < 0 for n in survivors): + return None + + surviving_ids = [n.get_id() for n in survivors] + if fd_enabled: + fd_by_node = {n.get_id(): n.failure_domain for n in survivors} + else: + # One pseudo-domain per HOST (not per node): a host may legitimately + # run several storage nodes, and two replicas on one host do not + # survive that host's loss. Keying on the host makes the planner's + # domain constraint and its host-disjointness constraint agree. + _fd_of_host: dict = {} + fd_by_node = { + n.get_id(): _fd_of_host.setdefault(n.mgmt_ip, len(_fd_of_host)) + for n in survivors + } + host_by_node = {n.get_id(): n.mgmt_ip for n in survivors} + label_by_node = {n.get_id(): n.physical_label for n in survivors} + current_layout = { + n.get_id(): replica_placement.Placement( + n.secondary_node_id, n.tertiary_node_id if ftt >= 2 else "") + for n in survivors + } + return surviving_ids, fd_by_node, host_by_node, label_by_node, current_layout, ftt + + +def _plan_driven_relocation(removed_node: StorageNode, db_controller): + """Phase 3b via the global placement planner. + + Returns True when the planned relocations were applied, False when one of + them failed (the caller retries the whole phase), or ``None`` when the + planner does not apply to this cluster and the caller should fall back to + the per-replica greedy path. + + Why this replaces the per-replica path under failure domains: the greedy + path answers "where does THIS stranded replica go" and can only ever move + the replica in front of it, so the one repair that a shrinking cluster + most often needs -- swapping two replicas that are both already placed -- + is not expressible in it at all. It compensates with splices into third + parties' pairings and, when even that fails, by relaxing the invariant to + the weaker ">=1 cross-domain role" floor with a warning. Repeated over + several removals (the reported 4-domain x 3-host case, one host removed + per domain) those relaxations accumulate into a layout with secondaries + and tertiaries sharing domains, even though a fully diverse layout + existed at every step. ``replica_placement`` computes that layout + directly, as a min-cost perfect matching, and returns the provably + smallest set of rebuilds that reaches it -- so no splice heuristic, no + collateral-damage repair hop, and no silent relaxation is needed. + + The move ORDER matters and is part of the plan: every move lands on a + slot the planner has already proved is free at that point, so + ``_relocate_replica_between``'s recursive vacate never has to run here. + A rotation cycle -- which that recursion cannot resolve at all, it hits + its own cycle backstop -- is broken up front by the planner into an extra + hop through the one slot the removal frees. + """ + from simplyblock_core.controllers import replica_placement + + inputs = _relocation_planner_inputs(removed_node, db_controller, allow_without_fd=True) + if inputs is None: + return None + surviving_ids, fd_by_node, host_by_node, label_by_node, current_layout, ftt = inputs + + try: + plan = replica_placement.plan_diverse_layout( + surviving_ids, fd_by_node, current_layout, ftt, + host_by_node=host_by_node, label_by_node=label_by_node) + moves = replica_placement.plan_moves( + current_layout, plan.layout, surviving_ids, ftt) + except replica_placement.InfeasiblePlacement as e: + logger.warning( + f"[REMOVAL] {removed_node.get_id()}: global replica placement is " + f"unusable ({e}); falling back to per-replica relocation") + return None + + logger.info( + f"[REMOVAL] {removed_node.get_id()}: replica placement plan -- " + f"{replica_placement.describe_plan(plan, moves)}") + for violation in plan.violations: + logger.warning(f"[REMOVAL] {removed_node.get_id()}: {violation}") + + for move in moves: + # A role with no current host was hosted on the node being removed: + # name it as the origin so the mover re-points the LVols and clears + # its back-reference, exactly as _relocate_one_replica used to. + old_host_id = move.from_node_id or removed_node.get_id() + if old_host_id == move.to_node_id: + continue + logger.info( + f"[REMOVAL] {removed_node.get_id()}: move {move.role} of " + f"{move.lvs_primary_node_id}: {old_host_id} -> {move.to_node_id}" + f"{' (scratch hop)' if move.scratch else ''}") + if not _relocate_replica_between( + move.lvs_primary_node_id, old_host_id, move.to_node_id, + move.role, db_controller): + logger.error( + f"[REMOVAL] {removed_node.get_id()}: planned move of " + f"{move.lvs_primary_node_id}'s {move.role} from {old_host_id} " + f"to {move.to_node_id} failed; will retry the phase") + return False + + _clear_replica_backref(removed_node, "lvstore_stack_secondary") + _clear_replica_backref(removed_node, "lvstore_stack_tertiary") + return True + + def _relocate_one_replica(removed_node: StorageNode, primary_id, role): """Re-host ``primary_id``'s ``role`` replica off ``removed_node``. @@ -4641,6 +5332,7 @@ def _relocate_one_replica(removed_node: StorageNode, primary_id, role): f"[REMOVAL] failed to splice {primary_id} into the pairing " f"occupying {new_id} (occupant {occupant_id})") return False + _repair_occupants_other_role_after_splice(occupant_id, primary_id, role, db_controller) primary = db_controller.get_storage_node_by_id(primary_id) setattr(primary, field, new_id) @@ -4672,6 +5364,75 @@ def _relocate_one_replica(removed_node: StorageNode, primary_id, role): return True +def _repair_occupants_other_role_after_splice(occupant_id, primary_id, role, db_controller): + """After a splice repoints ``occupant``'s ``role`` replica onto + ``primary``, check whether ``occupant``'s OTHER, untouched role (its + tertiary if ``role`` is "secondary", or vice versa) now shares a domain + with ``primary`` -- and if so, relocate THAT role too, reusing the same + picker (``_pick_replica_relocation_node``) and mover + (``_relocate_replica_between``) already used for the splice itself. + + A splice edge is chosen to protect the node actually being relocated + (``primary``) and, since the diversity fix, to prefer one where + ``occupant`` also stays diverse -- but a colliding edge is still + accepted as a last resort when no clean one exists (see + ``_find_splice_target_for_relocation``'s docstring). This closes that + gap ACTIVELY instead of just warning about it: by the time this runs, + ``occupant.`` already points at ``primary``, so calling the same + picker for occupant's other role automatically avoids both occupant's + own domain and primary's domain -- no extra plumbing needed. Because + the picker itself tries a direct candidate before a further splice, + this one call transparently covers both "a free replacement exists" + and "occupant's other role itself needs splicing into a further + pairing" -- the same machinery, one hop further out. + + Best-effort and never blocks the outer splice, which has already + succeeded by the time this runs: if no replacement is found, or the + relocation itself fails, occupant is left with the collision and a + warning is logged rather than the removal being failed over it. + + Not itself recursive beyond this one hop -- if repairing occupant's + other role creates a NEW collision for some third node, that is not + chased further. + """ + try: + primary = db_controller.get_storage_node_by_id(primary_id) + occupant = db_controller.get_storage_node_by_id(occupant_id) + except KeyError: + return + if primary.failure_domain < 0 or occupant.failure_domain < 0: + return + cluster = db_controller.get_cluster_by_id(primary.cluster_id) + if not getattr(cluster, "enable_failure_domain", False): + return + + other_role = "tertiary" if role == "secondary" else "secondary" + other_field = "tertiary_node_id" if role == "secondary" else "secondary_node_id" + other_target_id = getattr(occupant, other_field) + if not other_target_id or other_target_id == primary_id: + return + try: + other_target = db_controller.get_storage_node_by_id(other_target_id) + except KeyError: + return + if other_target.failure_domain != primary.failure_domain: + return # no collision -- occupant is already fine, nothing to do + + replacement = _pick_replica_relocation_node(occupant, other_target, other_role, db_controller) + if not replacement or replacement == other_target_id: + logger.warning( + f"[REMOVAL] splice: no replacement found to move {occupant_id}'s " + f"{other_role} off {other_target_id} (domain {other_target.failure_domain}) " + f"after splicing it onto {primary_id}'s domain {primary.failure_domain}; " + f"{occupant_id} is left with a domain-diversity gap") + return + if not _relocate_replica_between(occupant_id, other_target_id, replacement, other_role, db_controller): + logger.warning( + f"[REMOVAL] splice: failed to move {occupant_id}'s {other_role} off " + f"{other_target_id} onto {replacement}; {occupant_id} is left with a " + f"domain-diversity gap") + + def _relocate_replica_between(occupant_primary_id, old_host_id, new_host_id, role, db_controller, _seen=None): """Physically move ``occupant_primary_id``'s ``role`` replica off ``old_host_id`` onto ``new_host_id``, updating its forward pointer AND @@ -4750,7 +5511,16 @@ def _relocate_replica_between(occupant_primary_id, old_host_id, new_host_id, rol except KeyError: existing_occupant = None vacate_target = ( - _pick_replica_relocation_node(existing_occupant, new_host, role, db_controller) + _pick_replica_relocation_node( + existing_occupant, new_host, role, db_controller, + # occupant_primary_id is itself mid-relocation onto + # new_host_id in THIS call -- it can't simultaneously be + # the target existing_occupant vacates onto. Excluding + # it upfront lets the picker search past it instead of + # dead-ending on the one candidate that's structurally + # invalid (see _pick_replica_relocation_node's + # extra_exclude_ids docstring). + extra_exclude_ids=(occupant_primary_id,)) if existing_occupant else None) if not vacate_target or vacate_target == occupant_primary_id: logger.error( @@ -4808,7 +5578,36 @@ def _relocate_replica_between(occupant_primary_id, old_host_id, new_host_id, rol old_host = db_controller.get_storage_node_by_id(old_host_id) if getattr(old_host, backref) == occupant_primary_id: cluster = db_controller.get_cluster_by_id(occupant_primary.cluster_id) - if old_host.status == StorageNode.STATUS_ONLINE: + # A node's secondary replica and its tertiary replica OF THE SAME + # PRIMARY are not two resources -- they are ONE physical stack + # (raid0_ + LVS_, keyed by the primary's lvstore, not by + # the role). Vacating one role therefore must not tear that stack + # down while the other role still needs it: _delete_replica_on_peer + # ends in _remove_bdev_stack(remove_distr_only=True) -> + # bdev_raid_delete(raid0_), which hot-removes the lvstore from + # this node outright. + # + # The planner (controllers/replica_placement.py) routinely emits both + # roles of one primary in a single removal -- that is the whole point + # of solving the layout globally, and order_moves proves each move + # lands on a free SLOT. Slots are the wrong granularity here: for a + # given primary, this node's secondary slot and tertiary slot map to + # the same stack. Found live 2026-09-01 on the 12-node/4-domain FTT2 + # cluster, on the FIRST two removals and in the identical shape both + # times -- "secondary: -> X" promoted X (already holding + # that primary as tertiary), then "tertiary: X -> Y" deleted X's + # raid ~1s after Y's was built: + # 14:50:26 nq2mm bdev_raid_create raid0_45 + # 14:50:27 fvgtl bdev_raid_delete raid0_45 <- the new secondary + # leaving pq8h9/LVS_45 and 9s25f/LVS_1 recorded as FTT2 while + # physically down to a single replica (their tertiary), with the + # recorded secondary holding nothing but a stranded hublvol + # controller. Silent: every forward pointer, back-reference and + # lvstore_ports entry still said the replica was there. + other_backref = ("lvstore_stack_tertiary" if backref == "lvstore_stack_secondary" + else "lvstore_stack_secondary") + still_hosts_other_role = getattr(old_host, other_backref) == occupant_primary_id + if old_host.status == StorageNode.STATUS_ONLINE and not still_hosts_other_role: # occupant_primary survives this relocation (only its host is # moving) -- must NOT destroy the shared lvstore, only vacate # old_host's local examine copy. See _delete_replica_on_peer's @@ -4817,6 +5616,15 @@ def _relocate_replica_between(occupant_primary_id, old_host_id, new_host_id, rol destroy_lvstore=False) _teardown_lvol_subsystems_on_vacated_peer(old_host, occupant_primary, db_controller) _prune_stale_lvstore_ports(old_host_id, occupant_primary.lvstore, db_controller) + elif still_hosts_other_role: + # Drop only the back-reference below. The stack, its subsystems + # and its lvstore_ports entry all stay -- they belong to the role + # old_host still holds for this same primary. + logger.info( + f"[REMOVAL] {old_host_id} keeps {occupant_primary_id}'s " + f"{occupant_primary.lvstore} stack: it still hosts that primary as " + f"{'tertiary' if role == 'secondary' else 'secondary'}; " + f"clearing the {role} back-reference only") old_host = db_controller.get_storage_node_by_id(old_host_id) setattr(old_host, backref, "") old_host.write_to_db() @@ -4831,10 +5639,97 @@ def _clear_replica_backref(removed_node: StorageNode, backref): removed_node.write_to_db() -def _decommission_node_jm(removed_node: StorageNode) -> None: +def _release_jm_from_jc(node, name_old) -> bool: + """Hand ``name_old`` back to JC so its bdev can then be deleted. + + JC holds an open descriptor and IO channel on every JM it knows about; + ``jc_remove_jm`` closes them and drops the JM's JC context, and only after + that may the bdev be deleted. Deleting it first is what leaves JC naming a + bdev that no longer exists. + + Returns True when the bdev is safe to delete. The interesting codes: + + * ``-13`` "not used by JC" -- ALREADY RELEASED, and the normal answer after + a successful jc_replace_jm: measured live 2026-09-02 on spdk R26.3, a + replace that swaps the JM out of every vuid on the node also drops it + from JC, so the follow-up finds nothing left to do. Success, not failure. + * ``-22`` "still in use by one or more jm_vuids" -- some vuid still + references it, including one the control plane may be unable to + enumerate (a vuid whose primary is already removed appears in no + `decisions` entry and under no back-reference). Do NOT delete. + * ``RPC_UNSUPPORTED`` -- build predates the RPC (spdk main-latest as of + 2026-09-02 does not expose it, R26.3-latest does). Fall through to the + historical behaviour rather than stranding every superseded controller. + """ + try: + ret = node.rpc_client().jc_remove_jm(name_old) + except RPCRemoteError as re: + if re.code == JC_REMOVE_JM_NOT_USED: + logger.info( + f"[REMOVAL] {node.get_id()}: {name_old} already released " + f"(jc_remove_jm -13); safe to delete the bdev") + return True + if re.code == JC_REMOVE_JM_STILL_IN_USE: + logger.error( + f"[REMOVAL] {node.get_id()}: jc_remove_jm refused {name_old} " + f"(-22: still used by one or more jm_vuids) -- a jm_vuid this removal " + f"could not enumerate still references the departing node's JM " + f"(typically the removed node's OWN lvstore vuid, whose primary is gone " + f"so it appears in no decision and under no back-reference). Leaving the " + f"bdev in place: deleting it now would leave JC pointing at nothing") + return False + logger.error( + f"[REMOVAL] {node.get_id()}: jc_remove_jm({name_old}) failed ({re.code}): " + f"{re}; leaving the bdev in place") + return False + except Exception as e: + logger.error( + f"[REMOVAL] {node.get_id()}: jc_remove_jm({name_old}) raised: {e}; " + f"leaving the bdev in place") + return False + + if ret == RPC_UNSUPPORTED: + logger.warning( + f"[REMOVAL] {node.get_id()}: jc_remove_jm unsupported on this SPDK build; " + f"deleting {name_old}'s controller with JC's descriptor possibly still open " + f"(pre-existing behaviour)") + else: + logger.info(f"[REMOVAL] {node.get_id()}: jc_remove_jm released {name_old}") + return True + + +def _drop_superseded_jm_bdev(node, name_old, removed_jm_id) -> None: + """Detach the controller behind ``name_old`` and drop its bookkeeping. + + Only call once ``_release_jm_from_jc`` has confirmed JC is done with it. + """ + controller = name_old[:-2] if name_old.endswith("n1") else name_old + try: + node.rpc_client().bdev_nvme_detach_controller(controller) + except Exception as de: + logger.warning( + f"Failed to detach superseded controller {controller} on {node.get_id()}: {de}") + node.remote_jm_devices = [ + rd for rd in (node.remote_jm_devices or []) if rd.uuid != removed_jm_id] + + +def _decommission_node_jm(removed_node: StorageNode, replica_peer_ids=()) -> None: """Patch every live JC group that referenced ``removed_node``'s JM out of its redundancy set, replacing it with a freshly picked candidate. + ``replica_peer_ids``: the peers that hosted ``removed_node``'s OWN lvstore + replica (its secondary and tertiary), captured by the caller BEFORE phase + 3a -- which clears both those pointers and the peers' back-references, so + by the time this runs nothing in the DB records who they were. They matter + because each of them still runs a local JC instance for ``removed_node``'s + own jm_vuid, and that instance references the dying JM by name. It is + reachable through neither source Pass 2 consults (its primary is + ``removed_node``, which is not in ``live_nodes`` and therefore in no + ``decisions`` entry; and its back-reference has been cleared), so without + this the batched jc_replace_jm cannot cover it and jc_replace_jm's own -17 + check rejects the whole call. Reproduced live 2026-09-02: the removal's + only failing peer was the one hosting the removed node's own lvstore. + Called TWICE by design, both idempotent (guarded by the JM device's own status, set below): early, as node_removal_orchestrate's own phase 2 -- AFTER phase 3a tears down removed_node's own hosted replicas but BEFORE @@ -4892,8 +5787,26 @@ def _decommission_node_jm(removed_node: StorageNode) -> None: # never resolve/connect (2026-08-11 incident: a prior removal's # leftover jm_ids on 7b8hf sent a later removal's phase 5 chasing # a permanently-dead hostname). + # ...and removed_node itself, by identity. Its status here is + # IN_REMOVAL, not REMOVED, so the status filter alone lets it through + # (and the function is called twice, so its status differs between + # calls -- identity is the only stable guard). Phase 1 has already + # shut it down, but phase 3b has NOT yet relocated the replicas it + # hosts for other primaries, so its lvstore_stack_secondary/_tertiary + # still point at live primaries. Pass 2 therefore picked it up as a + # patch target for a hosted 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. Found live 2026-09-03 removing s25dl: + # "no recorded bdev name for removed JM 601dae11..., + # affected targets=[(1, 'a91a2d46...')]". Harmless only because the + # missing name short-circuited the call; with a name recorded it would + # have issued jc_replace_jm at the dead pod this filter exists to + # avoid. IN_REMOVAL is listed too, on the same grounds as REMOVED: + # such a node is down and its rpc_client cannot resolve. live_nodes = [n for n in db_controller.get_storage_nodes_by_cluster_id(removed_node.cluster_id) - if n.status != StorageNode.STATUS_REMOVED] + if n.status not in (StorageNode.STATUS_REMOVED, + StorageNode.STATUS_IN_REMOVAL) + and n.get_id() != removed_node.get_id()] def _pick_replacement(primary): # get_sorted_ha_jms ranks candidates by host-disjoint (hard) + @@ -4926,6 +5839,32 @@ def _owner_fd(jid): if primary.jm_ids and removed_jm_id in primary.jm_ids: decisions[primary.get_id()] = _pick_replacement(primary) + # removed_node's OWN vuid needs a replacement too: its JC instance + # survives on whichever peers hosted its replica (see + # replica_peer_ids), still naming the dying JM. We are not keeping that + # vuid useful -- its lvstore is gone -- only getting the dead JM out of + # it, so name_old ends up referenced by nothing and jc_remove_jm can + # release it instead of refusing with -22. + # + # Deliberately NOT stored in `decisions`. Every `decisions` entry means + # "this primary's OWN redundancy set lists the dead JM", and Pass 2 + # relies on that: it keys the node's own-vuid target off membership, + # and both jm_ids.remove() calls below assume it. Storing removed_node + # there made Pass 2 treat it as a normal consumer and then remove an id + # its jm_ids never held -- ValueError, phase 2 aborted mid-flight, and + # NO peer got its jc_replace_jm at all (found live 2026-09-02: strictly + # worse than the gap it was meant to close). removed_node is now also + # excluded from live_nodes outright, so Pass 1 and Pass 2 cannot reach + # it by any route; this stays as the statement of intent for the entry + # Pass 1 would otherwise be tempted to add back. + # removed_node's OWN lvstore group lives on as a "leftover" on its + # secondary and tertiary: no live primary, no back-reference, and by + # phase 2 no lvstore, raid or distribs either. It deliberately gets no + # decision and never becomes a replace target -- see the note in Pass 2. + logger.info( + f"[REMOVAL] {removed_node.get_id()}: own lvstore vuid {removed_node.jm_vuid} " + f"leftover on replica peers {list(replica_peer_ids)}; not a replace target") + # Pass 2: a single storage node can run more than one local JC # instance against the removed JM's bdev at once -- its own # redundancy set, plus one instance per primary it hosts as @@ -4943,15 +5882,77 @@ def _owner_fd(jid): if backref and backref in decisions: hosted_primary = db_controller.get_storage_node_by_id(backref) targets.append((hosted_primary.jm_vuid, hosted_primary, decisions[backref])) + # Replace and remove are MUTUALLY EXCLUSIVE on a node -- per the + # SPDK team (2026-09-02), and forced by the two RPCs' own rules: + # + # * jc_replace_jm must cover EVERY local vuid using name_old or + # it rejects the batch (-17). So if any surviving group uses the + # dead JM, the leftover group must be in that same call too -- + # it cannot be left out and handled separately. + # * jc_remove_jm refuses while any vuid still uses the JM (-22). + # So it is only available when nothing else holds it, i.e. when + # the leftover group is the sole user. + # + # Hence: other groups present -> replace all of them plus the + # leftover, and never call remove. Only the leftover -> remove + # alone, and never call replace. Whether the node is secondary or + # tertiary does not enter into it beyond determining whether it + # carries a leftover group at all. + # removed_node's own lvstore group is NEVER a replace target. Its + # lvstore is being destroyed, so there is nothing to keep redundant + # and no reason to burn a spare JM on it -- jc_replace_jm is only + # for groups that keep running. The batch therefore covers the + # surviving groups and nothing else, even on the secondary and + # tertiary, which are the only nodes that carry the leftover at all. + carries_removed_lvs = node.get_id() in replica_peer_ids if not targets: - if any(d.uuid == removed_jm_id for d in (node.remote_jm_devices or [])): - # Stale reference with no corresponding jm_vuid decision - # (e.g. neither this node's own redundancy set nor any - # primary it hosts actually referenced the dead JM) -- - # a plain refresh naturally excludes it since it can no - # longer be reached through either source. - node.remote_jm_devices = _connect_to_remote_jm_devs(node, node.jm_ids) + if not carries_removed_lvs: + # No surviving group here uses the dying JM, and this node + # never carried removed_node's lvstore either -- so no JC + # operation applies: no jc_remove_jm, no detach. Nothing on + # this node references the JM. + # + # The one thing still done is reconciling the DB record. + # 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 -- see + # _connect_to_remote_jm_devs); a record that none of them + # justifies any more is not inert, because it is the lookup + # that later removals use to find jc_replace_jm's name_old. + # A splice reshuffle can leave one behind (2026-08-14 + # incident: a peer reachable only through the hosted-primary + # path, own jm_ids clean, entry never re-derived). Refresh + # only when such a record is actually present. + if any(d.uuid == removed_jm_id for d in (node.remote_jm_devices or [])): + node.remote_jm_devices = _connect_to_remote_jm_devs(node, node.jm_ids) + node.write_to_db() + continue + + # Secondary or tertiary, and no surviving group uses the JM: + # removed_node's own lvstore group is the sole remaining user, + # and jc_remove_jm is the call for it. That group was never a + # replace target -- its lvstore is being destroyed -- so this + # is the only place the JM gets released on this node. + # + # -22 would mean some group still holds it after all; the bdev + # then stays, deliberately, and the error names why. + old_remote_dev = next( + (rd for rd in (node.remote_jm_devices or []) if rd.uuid == removed_jm_id), + None) + if old_remote_dev: + stale_bdev = old_remote_dev.remote_bdev + if not stale_bdev: + # No recorded bdev name: nothing to release and nothing + # to detach, so just drop the unreachable entry. + node.remote_jm_devices = _connect_to_remote_jm_devs(node, node.jm_ids) + elif _release_jm_from_jc(node, stale_bdev): + _drop_superseded_jm_bdev(node, stale_bdev, removed_jm_id) + # else: release refused. Leave BOTH the bdev and its + # remote_jm_devices entry alone -- dropping the entry while + # the bdev is still present and still held by JC is the + # bookkeeping-vs-reality split this sequence exists to + # avoid. Keep describing what is actually there. node.write_to_db() continue @@ -5035,8 +6036,16 @@ def _owner_fd(jid): f"{replacements} replacing removed JM {removed_jm_id}") for _jm_vuid, owner_primary, new_jm_id in targets: if owner_primary.get_id() == node.get_id(): - node.jm_ids.remove(removed_jm_id) - node.jm_ids.append(new_jm_id) + # Membership-conditional: a target can exist for + # a vuid whose OWNER is not this node, and an + # unguarded remove() throws ValueError and + # aborts the whole phase mid-node, leaving every + # peer after it unpatched (found live + # 2026-09-02). + if removed_jm_id in node.jm_ids: + node.jm_ids.remove(removed_jm_id) + if new_jm_id not in node.jm_ids: + node.jm_ids.append(new_jm_id) replaced = True except Exception as e: logger.error( @@ -5044,7 +6053,6 @@ def _owner_fd(jid): f"{name_old} ({replacements}): {e}") if replaced: - # name_old is now unused by any local JC instance -- but # jc_replace_jm only swaps the live membership pointer, it # never tears down the bdev/controller it swapped AWAY # from, and _connect_to_remote_jm_devs' delta mode above @@ -5057,15 +6065,24 @@ def _owner_fd(jid): # ~15+ minutes until some unrelated SPDK-side dead-peer # timeout eventually noticed (found live 2026-08-25). # Clean up both sides here instead of waiting for that. - old_controller_name = name_old[:-2] if name_old.endswith("n1") else name_old - try: - node.rpc_client().bdev_nvme_detach_controller(old_controller_name) - except Exception as de: - logger.warning( - f"Failed to detach superseded controller " - f"{old_controller_name} on {node.get_id()}: {de}") - node.remote_jm_devices = [ - rd for rd in (node.remote_jm_devices or []) if rd.uuid != removed_jm_id] + # + # No release here, on any node. jc_replace_jm and + # jc_remove_jm are alternatives, never a sequence: + # + # * this branch means at least one SURVIVING group used + # the JM and has just been repointed. removed_node's + # own lvstore group is never in that batch -- it is + # being destroyed, not repaired, so a replacement + # member would buy nothing. + # * the release belongs to the other case only: a node + # where NO surviving group used the JM, handled in the + # no-targets branch above. + # + # Measured live 2026-09-02 on spdk R26.3: after any + # successful replace jc_remove_jm answers -13 ("not used + # by JC") on every node, so calling it here would be a + # guaranteed no-op. + _drop_superseded_jm_bdev(node, name_old, removed_jm_id) if not replaced: for controller_name, pre_existing in connected_controllers: @@ -5091,14 +6108,39 @@ def _owner_fd(jid): # currently revisits this automatically; it stays a visible # gap until a future removal/reconnect cycle retries it. if not name_old: + # This single boolean (no entry in remote_jm_devices for + # removed_jm_id) can't by itself distinguish WHY: the + # physical connection may simply never have been made + # for this node (e.g. it picked up this jm_vuid via a + # relocation/splice whose soft-reconnect prelude never + # ran for it -- see _recreate_lvstore_on_non_leader_impl, + # 2026-08-25), or a connection existed and was dropped by + # something else entirely. jm_ids (checked in Pass 1/2 + # above) already confirms this node's OWN redundancy + # membership does reference removed_jm_id -- that part + # is not in question. Log the actual remote_jm_devices + # state so a live occurrence is diagnosable without + # re-deriving it from scratch (2026-08-28 finding): an + # entirely empty list points at "never connected in the + # first place"; a non-empty list missing only this uuid + # points at something explicitly removing/skipping it. + current_remote_uuids = [rd.uuid for rd in (node.remote_jm_devices or [])] logger.error( f"[REMOVAL] {node.get_id()}: no recorded bdev name for removed " - f"JM {removed_jm_id}; cannot call jc_replace_jm") + f"JM {removed_jm_id}; cannot call jc_replace_jm -- " + f"node.jm_ids={node.jm_ids}, " + f"remote_jm_devices uuids={current_remote_uuids} " + f"({'never connected to any remote JM' if not current_remote_uuids else 'connected to other JM(s) but not this one'}), " + f"affected targets={[(jm_vuid, owner_primary.get_id()) for jm_vuid, owner_primary, _ in targets]}") elif any(new_jm_id is None for _, _, new_jm_id in targets): logger.error( f"[REMOVAL] {node.get_id()}: no replacement candidate for jm_vuid(s) " f"{[jm_vuid for jm_vuid, _, new_jm_id in targets if new_jm_id is None]}") - if node.get_id() in decisions: + if node.get_id() in decisions and removed_jm_id in node.jm_ids: + # Guarded for the same reason as the success path above: an + # unguarded remove() here threw ValueError and aborted + # phase 2 before any peer was patched (found live + # 2026-09-02). node.jm_ids.remove(removed_jm_id) node.write_to_db() @@ -8739,6 +9781,26 @@ def execute_on_leader_with_failover(all_nodes, lvs_name, operation_fn, return False, new_leader, f"Operation failed on new leader: {e}" +#: Mgmt statuses that make a peer unusable as a routing, port-block, or +#: failover target, without consulting the data plane at all. Each one means +#: mgmt has already observed the peer leaving the cluster and is not expecting +#: it back, so the peer's mgmt API and SPDK are gone (or going). +#: +#: IN_SHUTDOWN / RESTARTING are deliberately absent: those are transient states +#: the runner owns, and preempting another node's leadership during its own +#: restart would be incorrect. +#: +#: PENDING_REMOVAL is also deliberately absent. node_removal_orchestrate sets +#: it *before* phase 1 shuts the node down, so the node is still up and serving +#: then; treating it as gone would skip a port-block it still needs. +_PEER_DISCONNECTED_STATUSES = ( + StorageNode.STATUS_OFFLINE, + StorageNode.STATUS_REMOVED, + StorageNode.STATUS_UNREACHABLE, + StorageNode.STATUS_IN_REMOVAL, +) + + def _check_peer_disconnected(peer_node: StorageNode, lvs_peer_ids=None): """Check if a peer node should be treated as disconnected for the purpose of routing (takeover vs. non-leader path) and peer-port-block decisions. @@ -8747,15 +9809,25 @@ def _check_peer_disconnected(peer_node: StorageNode, lvs_peer_ids=None): Two signals, first match wins: - 1. Mgmt ground truth (FDB status). If FDB already says the peer is - OFFLINE / REMOVED / UNREACHABLE, trust it immediately — mgmt has - observed the peer leaving the cluster. Attempting to port-block + 1. Mgmt ground truth (FDB status). If FDB already says the peer is in + one of ``_PEER_DISCONNECTED_STATUSES``, trust it immediately — mgmt + has observed the peer leaving the cluster. Attempting to port-block such a peer's mgmt API will only hit ECONNREFUSED and, after 5× retries, abort the entire restart with a misleading "LVStore - recovery failed" event. IN_SHUTDOWN / RESTARTING are deliberately - NOT in this list — those are transient states the runner owns; - preempting another node's leadership during its own restart - would be incorrect. + recovery failed" event. + + IN_REMOVAL is in that list on the same grounds as OFFLINE: phase 1 of + node_removal_orchestrate 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. Without it this check falls through to the JM-quorum + path, which votes "connected" on `0/0 peers report disconnected` + exactly when peers have already torn down their controllers for the + departing node — the abstain-from-all case described below. Callers + then route to, or pre-warm a failover path towards, a node the + control plane is in the middle of deleting (live 2026-09-03: a + tertiary spent its deferred hublvol attach on the node being removed, + "Failed to add deferred hublvol failover path to … + for LVS_21"). 2. Data-plane JM quorum (legacy path). Only reached if mgmt says the peer is in an "alive" state. Useful to detect fabric @@ -8784,9 +9856,7 @@ def _check_peer_disconnected(peer_node: StorageNode, lvs_peer_ids=None): # Peer has been fully removed from the cluster — definitely disconnected. return True - if peer_node.status in (StorageNode.STATUS_OFFLINE, - StorageNode.STATUS_REMOVED, - StorageNode.STATUS_UNREACHABLE): + if peer_node.status in _PEER_DISCONNECTED_STATUSES: logger.info("Peer %s mgmt status is %s — treating as disconnected", peer_node.get_id(), peer_node.status) return True @@ -8888,6 +9958,57 @@ def _handle_rpc_failure_on_peer(snode: StorageNode, peer_node, lvs_jm_vuid, lvs_ return "abort" +def _lvstore_port_entry(node): + """The per-lvstore port pair other nodes must use to reach ``node``'s + lvstore. Keyed by lvstore name by every caller.""" + return { + "lvol_subsys_port": node.lvol_subsys_port, + "hublvol_port": node.hublvol.nvmf_port if node.hublvol else 0, + } + + +def _derive_lvstore_ports(snode, primary_node, db_controller): + """Every lvstore whose lvols ``snode`` serves -> that lvstore's own ports. + + Registration of an lvol on a non-leader looks its ``lvs_name`` up in this + map and REFUSES to add a listener when the entry is missing, rather than + fall back to snode's own leader port and guess (see the comment above the + lvstore_ports check in _register_lvol_on_non_leader -- guessing left two + secondaries listening on the wrong port indefinitely on 2026-08-18). So + the map has to be complete BEFORE registration runs, not merely + eventually consistent with it. + + Deriving it from ``snode``'s own lvstore plus its two back-reference + slots alone is not complete in the one case that matters most: a call + that is ESTABLISHING a new hosting relationship. A node-removal + relocation (_relocate_replica_between) commits + lvstore_stack_secondary/_tertiary only AFTER the build returns, so during + the build the slot still reads empty and primary_node's lvstore was + absent from the map. Every relocation therefore registered its lvols with + no port entry, logged "INCOMPLETE LVOL REGISTRATION ... running below + configured redundancy until repaired", and served nothing from that + replica until the next lvol_monitor repair cycle picked it up ~2 minutes + later (all four relocations of the 2026-09-11 no-FD 2+2 run; the replicas + did heal, but the redundancy gap was real and the error was logged at + ERROR on a run that was otherwise clean). + + ``primary_node`` is included directly because it is the one lvstore this + call is definitionally hosting -- it is the stack being built. Where the + slots already cover it the entry is identical (same node, same ports), so + adding it last is idempotent rather than an override. + """ + ports = {} + if snode.lvstore: + ports[snode.lvstore] = _lvstore_port_entry(snode) + for slot_id in (snode.lvstore_stack_secondary, snode.lvstore_stack_tertiary): + if slot_id: + nd = db_controller.get_storage_node_by_id(slot_id) + ports[nd.lvstore] = _lvstore_port_entry(nd) + if primary_node is not None and primary_node.lvstore: + ports[primary_node.lvstore] = _lvstore_port_entry(primary_node) + return ports + + def recreate_lvstore_on_non_leader(snode, leader_node, primary_node, activation_mode=False, force=False): """Per-LVS-locked wrapper: serialize recreate of ``primary_node.lvstore`` only against a concurrent recreate of the SAME LVS. Activation-mode @@ -9190,26 +10311,9 @@ def _recreate_lvstore_on_non_leader_impl(snode: StorageNode, leader_node, primar logger.warning("Soft reconnect of remote JMs failed on %s: %s", snode.get_id(), e) - # Ensure snode has per-lvstore ports from primary - lvstore_ports = {} - if snode.lvstore: - lvstore_ports[snode.lvstore] = { - "lvol_subsys_port": snode.lvol_subsys_port, - "hublvol_port": snode.hublvol.nvmf_port if snode.hublvol else 0, - } - if snode.lvstore_stack_secondary: - nd = db_controller.get_storage_node_by_id(snode.lvstore_stack_secondary) - lvstore_ports[nd.lvstore] = { - "lvol_subsys_port": nd.lvol_subsys_port, - "hublvol_port": nd.hublvol.nvmf_port if nd.hublvol else 0, - } - if snode.lvstore_stack_tertiary: - nd = db_controller.get_storage_node_by_id(snode.lvstore_stack_tertiary) - lvstore_ports[nd.lvstore] = { - "lvol_subsys_port": nd.lvol_subsys_port, - "hublvol_port": nd.hublvol.nvmf_port if nd.hublvol else 0, - } - snode.lvstore_ports = lvstore_ports + # Ensure snode has per-lvstore ports for every lvstore it serves, + # including the one this call is building. + snode.lvstore_ports = _derive_lvstore_ports(snode, primary_node, db_controller) snode.write_to_db() lvol_list = [] diff --git a/tests/unit/test_add_node_pod_cleanup.py b/tests/unit/test_add_node_pod_cleanup.py new file mode 100644 index 0000000000..8c35b31704 --- /dev/null +++ b/tests/unit/test_add_node_pod_cleanup.py @@ -0,0 +1,173 @@ +# coding=utf-8 +"""add_node must not leave an SPDK pod behind when it bails out. + +Between ``spdk_process_start`` and the StorageNode record being written there +is no DB row pointing at the pod. It has no owner reference, so Kubernetes +will not reap it, and no node record, so the control plane cannot reconcile +it. Left behind it holds the host's hugepages and every later add-node +attempt on that host stays Pending with "Insufficient hugepages-2Mi", which +stalls the operator's serialised queue and wedges the deployment. + +Seen on fresh 12-node deploys on 2026-09-08 and 2026-09-11; both needed a +manual delete of the unowned pod to unblock. +""" +import ast +import functools +import inspect +import unittest +from types import SimpleNamespace +from unittest.mock import MagicMock, patch + +from kubernetes.client import ApiException +from tenacity import Retrying + +from simplyblock_core import storage_node_ops + +# The confirm-gone poll waits 3s between attempts; a test that exercises the +# 'pod never disappears' path must not actually sleep 30s for it. +_NoWaitRetrying = functools.partial(Retrying, sleep=lambda _: None) + + +class TestAbortStartedSpdk(unittest.TestCase): + """The cleanup helper itself, on the node-agent path (docker mode).""" + + def test_kills_the_pod(self): + api = MagicMock() + api.spdk_process_kill = MagicMock(return_value=(True, "")) + storage_node_ops._abort_started_spdk(api, 4420, "cluster-1", "boom") + api.spdk_process_kill.assert_called_once_with(4420, "cluster-1") + + def test_reports_but_does_not_raise_when_the_kill_fails(self): + api = MagicMock() + api.spdk_process_kill = MagicMock(return_value=(False, "pod stuck")) + storage_node_ops._abort_started_spdk(api, 4420, "cluster-1", "boom") + api.spdk_process_kill.assert_called_once() + + def test_swallows_an_exception_from_the_kill(self): + """The caller is already on a failure path; cleanup must not mask it + with a second exception.""" + api = MagicMock() + api.spdk_process_kill = MagicMock(side_effect=RuntimeError("api down")) + storage_node_ops._abort_started_spdk(api, 4420, "cluster-1", "boom") + api.spdk_process_kill.assert_called_once() + + +class TestAbortStartedSpdkInKubernetes(unittest.TestCase): + """In kubernetes mode the teardown must not depend on the node agent. + + The agent runs ON the host being abandoned, and the commonest reason + add_node aborts after starting SPDK is that the host went away -- an + agent crash, or the one-off CPU-topology reboot. On 2026-09-11 the abort + handler fired correctly for worker gddnr / rpc_port 4436 and then got + "Connection refused" from that node's agent, so the kill was never + delivered. The delete is a plain namespaced pod delete with nothing + node-local about it, so it belongs on the API server path. + """ + + def setUp(self): + self.k8s = MagicMock() + self.k8s.list_namespaced_pod = MagicMock( + return_value=SimpleNamespace(items=[])) + self.api = MagicMock() + self.api.spdk_process_kill = MagicMock(return_value=(True, "")) + + def _run(self, cluster_id="cluster-1abcdef", rpc_port=4436): + with patch.object(storage_node_ops.utils, "get_k8s_core_client", + return_value=self.k8s): + storage_node_ops._abort_started_spdk( + self.api, rpc_port, cluster_id, "boom", cluster_mode="kubernetes") + + def test_deletes_the_pod_through_the_api_server_not_the_agent(self): + self._run() + deleted = [c.args[0] for c in self.k8s.delete_namespaced_pod.call_args_list] + self.assertIn("snode-spdk-pod-4436-cluste", deleted) + self.api.spdk_process_kill.assert_not_called() + + def test_also_removes_the_fluentd_companion(self): + self._run() + deleted = [c.args[0] for c in self.k8s.delete_namespaced_pod.call_args_list] + self.assertIn("simplyblock-fluentd-4436-cluste", deleted) + + def test_a_pod_that_is_already_gone_counts_as_cleaned_up(self): + self.k8s.delete_namespaced_pod = MagicMock( + side_effect=ApiException(status=404)) + self._run() + self.api.spdk_process_kill.assert_not_called() + + def test_falls_back_to_the_agent_when_the_api_delete_fails(self): + self.k8s.delete_namespaced_pod = MagicMock( + side_effect=ApiException(status=500)) + self._run() + self.api.spdk_process_kill.assert_called_once_with(4436, "cluster-1abcdef") + + def test_falls_back_to_the_agent_when_there_is_no_api_client(self): + with patch.object(storage_node_ops.utils, "get_k8s_core_client", + side_effect=RuntimeError("not in cluster")): + storage_node_ops._abort_started_spdk( + self.api, 4436, "cluster-1abcdef", "boom", cluster_mode="kubernetes") + self.api.spdk_process_kill.assert_called_once() + + def test_a_pod_that_never_disappears_is_not_reported_as_cleaned_up(self): + """A pod still Terminating still holds the host's hugepages, so the + API delete has not achieved anything yet -- try the agent too.""" + still_there = SimpleNamespace( + items=[SimpleNamespace( + metadata=SimpleNamespace(name="snode-spdk-pod-4436-cluste"))]) + self.k8s.list_namespaced_pod = MagicMock(return_value=still_there) + with patch.object(storage_node_ops, "Retrying", _NoWaitRetrying): + self._run() + self.api.spdk_process_kill.assert_called_once() + + def test_docker_mode_never_touches_the_api_server(self): + with patch.object(storage_node_ops.utils, "get_k8s_core_client", + return_value=self.k8s): + storage_node_ops._abort_started_spdk( + self.api, 4436, "cluster-1abcdef", "boom", cluster_mode="docker") + self.k8s.delete_namespaced_pod.assert_not_called() + self.api.spdk_process_kill.assert_called_once() + + +class TestNoLeakingExitInAddNode(unittest.TestCase): + """Structural guard: once the pod exists, every bail-out must clean up. + + Enforced on the source rather than by driving add_node, which needs far + too much of the world mocked to be a useful regression test. This catches + the case the manual fix cannot: someone adding a FOURTH early return to + that window later on. + """ + + def _add_node_body(self): + src = inspect.getsource(storage_node_ops.add_node) + # normalise the leading indentation so ast can parse the function alone + return ast.parse(inspect.cleandoc("\n".join(src.splitlines()))) + + def test_every_return_false_after_pod_start_cleans_up(self): + src_lines = inspect.getsource(storage_node_ops.add_node).splitlines() + + start = next((i for i, l in enumerate(src_lines) + if "spdk_process_start(" in l), None) + self.assertIsNotNone(start, "spdk_process_start call not found in add_node") + + persisted = next((i for i, l in enumerate(src_lines) + if "snode.rpc_port = rpc_port" in l), None) + self.assertIsNotNone(persisted, "StorageNode persistence point not found") + self.assertGreater(persisted, start) + + offenders = [] + for i in range(start, persisted): + line = src_lines[i].strip() + if line in ("return False", "return None"): + # the preceding few lines must hand off to the cleanup helper + window = " ".join(x.strip() for x in src_lines[max(start, i - 4):i]) + if "_abort_started_spdk" not in window: + offenders.append((i - start, src_lines[i].strip())) + + self.assertEqual( + offenders, [], + "add_node bails out after the SPDK pod exists without calling " + "_abort_started_spdk, which leaks an unowned pod holding the " + f"host's hugepages. Offending exits (offset from pod start): {offenders}") + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/unit/test_failed_migration_online_since.py b/tests/unit/test_failed_migration_online_since.py new file mode 100644 index 0000000000..8f5d308d04 --- /dev/null +++ b/tests/unit/test_failed_migration_online_since.py @@ -0,0 +1,119 @@ +# coding=utf-8 +"""The failed-migration runner must actually wait for a freshly-online node. + +``online_since`` is stamped timezone-aware (storage_node_ops.py, via +``datetime.now(timezone.utc)``). Subtracting it from a NAIVE ``datetime.now()`` +raises TypeError on every call; the surrounding ``except`` swallowed it, so the +"node is online < 1 min, retrying" guard never fired at all and failed-device +migrations -- the tasks node removal's phase 5 creates -- started immediately +against a node that had just come back online. + +246 occurrences of "can't subtract offset-naive and offset-aware datetimes" +were logged across two node removals on 2026-09-11. The three sibling call +sites (tasks_runner_migration.py, tasks_runner_new_dev_migration.py, +storage_node_monitor.py) already passed timezone.utc; this one was missed. +""" +import ast +import inspect +import unittest +from datetime import datetime, timedelta, timezone +from pathlib import Path +from typing import ClassVar, List + +import simplyblock_core.services.tasks_runner_failed_migration as runner + + +ONLINE_SINCE_STAMP_IS_AWARE = True # storage_node_ops stamps datetime.now(timezone.utc) + + +class TestOnlineSinceComparison(unittest.TestCase): + + def _stamp(self, seconds_ago): + return str(datetime.now(timezone.utc) - timedelta(seconds=seconds_ago)) + + def test_an_aware_stamp_can_be_subtracted_without_raising(self): + """The regression: this is exactly the expression the runner evaluates.""" + online_since = self._stamp(10) + diff = datetime.now(timezone.utc) - datetime.fromisoformat(online_since) + self.assertLess(diff.total_seconds(), 60) + + def test_a_naive_now_against_the_real_stamp_raises(self): + """Guards the assumption above: if online_since ever became naive this + test fails and the fix needs revisiting.""" + online_since = self._stamp(10) + with self.assertRaises(TypeError): + datetime.now() - datetime.fromisoformat(online_since) + + +class TestRunnerUsesAnAwareNow(unittest.TestCase): + """Structural guard on the runner's source. + + Driving task_runner() needs FDB and a full task/node fixture; the defect + is a one-token slip that a source assertion catches precisely. + """ + + def _online_since_calls(self): + src = inspect.getsource(runner) + tree = ast.parse(src) + found = [] + for node in ast.walk(tree): + if not isinstance(node, ast.BinOp) or not isinstance(node.op, ast.Sub): + continue + text = ast.dump(node) + if "online_since" not in text: + continue + found.append(node) + return found + + def test_the_online_since_subtraction_passes_a_timezone(self): + calls = self._online_since_calls() + self.assertTrue(calls, "no online_since subtraction found in the runner") + for binop in calls: + left = binop.left + self.assertIsInstance( + left, ast.Call, + "left operand of the online_since subtraction should be datetime.now(...)") + self.assertTrue( + left.args or left.keywords, + "datetime.now() is called with NO timezone: subtracting the " + "timezone-aware online_since stamp raises TypeError, the except " + "swallows it, and the 'node is online < 1 min' wait silently " + "never happens") + + +class TestAllOnlineSinceCallSitesAgree(unittest.TestCase): + """The same slip must not come back in a sibling runner. + + This bug existed because three of four call sites were corrected and one + was not; nothing held them together. + """ + + SITES: ClassVar[List[str]] = [ + "simplyblock_core/services/tasks_runner_failed_migration.py", + "simplyblock_core/services/tasks_runner_migration.py", + "simplyblock_core/services/tasks_runner_new_dev_migration.py", + "simplyblock_core/services/storage_node_monitor.py", + ] + + def test_no_naive_now_is_subtracted_from_online_since(self): + # .../simplyblock_core/services/.py -> repo root + root = Path(runner.__file__).resolve().parents[2] + offenders = [] + checked = 0 + for rel in self.SITES: + path = root / rel + self.assertTrue(path.exists(), f"call site not found, fix SITES: {path}") + checked += 1 + for lineno, line in enumerate(path.read_text(encoding="utf-8").splitlines(), 1): + if "online_since" not in line or "datetime.now()" not in line: + continue + offenders.append(f"{rel}:{lineno}: {line.strip()}") + self.assertEqual(checked, len(self.SITES)) + self.assertEqual( + offenders, [], + "a naive datetime.now() is being subtracted from the aware " + f"online_since stamp: {offenders}") + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/unit/test_jm_event_collector.py b/tests/unit/test_jm_event_collector.py index 8fb024317a..2a8be2c5b4 100644 --- a/tests/unit/test_jm_event_collector.py +++ b/tests/unit/test_jm_event_collector.py @@ -18,6 +18,7 @@ from simplyblock_core import rpc_client from simplyblock_core.controllers import events_controller from simplyblock_core.models.events import EventObj +from simplyblock_core.models.storage_node import StorageNode from simplyblock_core.services import main_distr_event_collector as collector @@ -149,14 +150,48 @@ def setUp(self): collector.threads_maps.clear() collector.jm_unsupported_nodes.clear() - def _nodes(self, *ids): + def _nodes(self, *ids, status=StorageNode.STATUS_ONLINE): nodes = [] for node_id in ids: node = MagicMock() node.get_id.return_value = node_id + node.status = status nodes.append(node) return nodes + def test_a_removed_node_gets_no_collectors(self): + # Removal leaves the record in place with status=removed, so without an + # explicit check we keep (re)spawning collectors that RPC a node whose + # SPDK is gone. Live 2026-09-02: 1036 "Failed to process JM events ... + # connection error" in the 1.5h after one removal, still climbing. + with patch.object(collector.threading, "Thread") as thread: + collector.ensure_collectors( + self._nodes("gone", status=StorageNode.STATUS_REMOVED)) + thread.assert_not_called() + self.assertNotIn("gone:distr", collector.threads_maps) + self.assertNotIn("gone:jm", collector.threads_maps) + + def test_removal_also_forgets_a_node_that_already_had_collectors(self): + # The loops exit on removal themselves, but this function would restart + # anything not alive within ~5s, so the map entries have to go too. + collector.threads_maps["gone:distr"] = MagicMock() + collector.threads_maps["gone:jm"] = MagicMock() + with patch.object(collector.threading, "Thread") as thread: + collector.ensure_collectors( + self._nodes("gone", status=StorageNode.STATUS_REMOVED)) + thread.assert_not_called() + self.assertNotIn("gone:distr", collector.threads_maps) + self.assertNotIn("gone:jm", collector.threads_maps) + + def test_an_online_node_alongside_a_removed_one_is_unaffected(self): + nodes = (self._nodes("live") + + self._nodes("gone", status=StorageNode.STATUS_REMOVED)) + with patch.object(collector.threading, "Thread") as thread: + collector.ensure_collectors(nodes) + self.assertEqual(thread.call_count, 2, "live node still needs distr+jm") + self.assertIn("live:distr", collector.threads_maps) + self.assertNotIn("gone:jm", collector.threads_maps) + def test_each_node_gets_a_thread_per_source(self): with patch.object(collector.threading, "Thread") as thread: collector.ensure_collectors(self._nodes("a", "b")) diff --git a/tests/unit/test_kill_spdk_until_dead.py b/tests/unit/test_kill_spdk_until_dead.py new file mode 100644 index 0000000000..cf98991d78 --- /dev/null +++ b/tests/unit/test_kill_spdk_until_dead.py @@ -0,0 +1,84 @@ +# coding=utf-8 +"""_kill_spdk_until_dead must not report death it did not observe. + +Its return value gates whether the caller drops the StorageNode record +(storage_node_ops.add_node, "did not come up after creation" path). If it +says True while the pod is alive, the record goes and the pod stays -- +referenced by nothing, reaped by nothing, holding the host's hugepages, so +every later add-node attempt on that host stays Pending. + +Seen 2026-09-11: worker tqmtr's API stopped answering, which is *why* the +add failed; the liveness probe therefore raised, the old code read that as +"confirmed down", the record was dropped and pod 4426 was left behind. The +deploy wedged at 11/12 until it was deleted by hand. +""" +import unittest +from unittest.mock import MagicMock, patch + +from simplyblock_core import storage_node_ops +from simplyblock_core.snode_client import SNodeClientException + + +def _node(): + n = MagicMock() + n.get_id = MagicMock(return_value="node-1") + n.rpc_port = 4426 + n.cluster_id = "cluster-1" + n.mgmt_ip = "10.0.0.1" + return n + + +class TestKillSpdkUntilDead(unittest.TestCase): + + def setUp(self): + patcher = patch.object(storage_node_ops.time, "sleep", lambda *_a, **_k: None) + patcher.start() + self.addCleanup(patcher.stop) + + def _run(self, api): + node = _node() + node.client = MagicMock(return_value=api) + return storage_node_ops._kill_spdk_until_dead(node, max_attempts=2, + poll_per_attempt_sec=1, + poll_interval=0.25) + + def test_true_when_probe_answers_not_up(self): + api = MagicMock() + api.spdk_process_kill = MagicMock(return_value=(True, "")) + api.spdk_process_is_up = MagicMock(return_value=(False, "")) + self.assertTrue(self._run(api)) + + def test_false_when_probe_keeps_saying_up(self): + api = MagicMock() + api.spdk_process_kill = MagicMock(return_value=(True, "")) + api.spdk_process_is_up = MagicMock(return_value=(True, "")) + self.assertFalse(self._run(api)) + + def test_false_when_the_probe_cannot_be_made(self): + """The regression: node API unreachable must read as UNKNOWN, not dead.""" + api = MagicMock() + api.spdk_process_kill = MagicMock(side_effect=SNodeClientException("connection refused")) + api.spdk_process_is_up = MagicMock(side_effect=SNodeClientException("connection refused")) + self.assertFalse( + self._run(api), + "an unreachable node API must not be reported as SPDK confirmed down -- " + "the caller would drop the StorageNode record and orphan a live pod") + + def test_false_when_only_the_probe_fails(self): + """Kill 'succeeds' but liveness cannot be verified -- still not death.""" + api = MagicMock() + api.spdk_process_kill = MagicMock(return_value=(True, "")) + api.spdk_process_is_up = MagicMock(side_effect=SNodeClientException("name resolution")) + self.assertFalse(self._run(api)) + + def test_recovers_when_the_probe_answers_on_a_later_round(self): + """A transient probe failure must not abort the wait.""" + api = MagicMock() + api.spdk_process_kill = MagicMock(return_value=(True, "")) + api.spdk_process_is_up = MagicMock( + side_effect=[SNodeClientException("blip"), (False, "")]) + self.assertTrue(self._run(api)) + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/unit/test_lvstore_ports_derivation.py b/tests/unit/test_lvstore_ports_derivation.py new file mode 100644 index 0000000000..fd347bca82 --- /dev/null +++ b/tests/unit/test_lvstore_ports_derivation.py @@ -0,0 +1,101 @@ +# coding=utf-8 +"""The lvstore_ports map must be complete BEFORE lvols are registered. + +Registering an lvol on a non-leader looks its ``lvs_name`` up in +``snode.lvstore_ports`` and refuses to add a listener when the entry is +missing, rather than guessing a port. So a map that is merely eventually +correct is not good enough: the replica serves nothing until something else +repairs it. + +Deriving the map from snode's own lvstore plus its two back-reference slots +missed exactly the case a node-removal relocation creates -- the slot that +records the new hosting relationship is committed only AFTER the build +returns. Every relocation of the 2026-09-11 no-FD 2+2 run logged +"INCOMPLETE LVOL REGISTRATION ... running below configured redundancy until +repaired" and stayed that way for ~2 minutes until lvol_monitor healed it. +""" +import unittest +from unittest.mock import MagicMock + +from simplyblock_core import storage_node_ops + + +def _node(node_id, lvstore, subsys_port, hublvol_port=0, + secondary=None, tertiary=None): + n = MagicMock() + n.get_id.return_value = node_id + n.lvstore = lvstore + n.lvol_subsys_port = subsys_port + n.hublvol = MagicMock(nvmf_port=hublvol_port) if hublvol_port else None + n.lvstore_stack_secondary = secondary + n.lvstore_stack_tertiary = tertiary + return n + + +class TestDeriveLvstorePorts(unittest.TestCase): + + def setUp(self): + self.primary = _node("prim", "LVS_16", 4442, hublvol_port=4443) + self.other = _node("other", "LVS_10", 4438, hublvol_port=4439) + self.db = MagicMock() + self.db.get_storage_node_by_id.side_effect = lambda i: { + "prim": self.primary, "other": self.other}[i] + + def test_includes_the_stack_being_built_even_with_no_backref_yet(self): + """The regression. A relocation calls this while the host's + secondary/tertiary slot for the incoming primary is still empty.""" + host = _node("host", "LVS_4", 4434, hublvol_port=4435) + ports = storage_node_ops._derive_lvstore_ports(host, self.primary, self.db) + self.assertIn( + "LVS_16", ports, + "the lvstore this call is building must be in the map before its " + "lvols are registered, or every listener is refused") + self.assertEqual(ports["LVS_16"], + {"lvol_subsys_port": 4442, "hublvol_port": 4443}) + + def test_keeps_the_hosts_own_lvstore(self): + host = _node("host", "LVS_4", 4434, hublvol_port=4435) + ports = storage_node_ops._derive_lvstore_ports(host, self.primary, self.db) + self.assertEqual(ports["LVS_4"], + {"lvol_subsys_port": 4434, "hublvol_port": 4435}) + + def test_keeps_lvstores_from_both_backref_slots(self): + host = _node("host", "LVS_4", 4434, hublvol_port=4435, + secondary="other", tertiary="prim") + ports = storage_node_ops._derive_lvstore_ports(host, self.primary, self.db) + self.assertEqual(sorted(ports), ["LVS_10", "LVS_16", "LVS_4"]) + + def test_primary_already_in_a_slot_is_not_duplicated_or_changed(self): + """Adding primary_node last must be idempotent, not an override.""" + host = _node("host", "LVS_4", 4434, hublvol_port=4435, secondary="prim") + ports = storage_node_ops._derive_lvstore_ports(host, self.primary, self.db) + self.assertEqual(sorted(ports), ["LVS_16", "LVS_4"]) + self.assertEqual(ports["LVS_16"], + {"lvol_subsys_port": 4442, "hublvol_port": 4443}) + + def test_a_node_without_a_hublvol_reports_port_zero(self): + primary = _node("prim", "LVS_16", 4442) + host = _node("host", "LVS_4", 4434) + ports = storage_node_ops._derive_lvstore_ports(host, primary, self.db) + self.assertEqual(ports["LVS_16"], + {"lvol_subsys_port": 4442, "hublvol_port": 0}) + + def test_no_primary_node_is_tolerated(self): + host = _node("host", "LVS_4", 4434, hublvol_port=4435) + ports = storage_node_ops._derive_lvstore_ports(host, None, self.db) + self.assertEqual(sorted(ports), ["LVS_4"]) + + def test_a_primary_with_no_lvstore_adds_nothing(self): + primary = _node("prim", "", 0) + host = _node("host", "LVS_4", 4434, hublvol_port=4435) + ports = storage_node_ops._derive_lvstore_ports(host, primary, self.db) + self.assertEqual(sorted(ports), ["LVS_4"]) + + def test_a_host_with_no_own_lvstore_still_gets_the_built_stack(self): + host = _node("host", "", 0) + ports = storage_node_ops._derive_lvstore_ports(host, self.primary, self.db) + self.assertEqual(sorted(ports), ["LVS_16"]) + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/unit/test_node_removal.py b/tests/unit/test_node_removal.py index 8abeeae0b8..a6182cef41 100644 --- a/tests/unit/test_node_removal.py +++ b/tests/unit/test_node_removal.py @@ -16,6 +16,7 @@ pure control-flow + bookkeeping tests. """ +import logging import unittest from unittest.mock import DEFAULT, MagicMock, call, patch @@ -48,9 +49,15 @@ def _cluster(ha_type="ha", npcs=1, ndcs=2, ft=1, mode="docker", def _node(node_id, status=StorageNode.STATUS_ONLINE, lvstore="", secondary_id="", tertiary_id="", stack_secondary="", stack_tertiary="", n_devices=0, with_jm=False, - failure_domain=-1, mgmt_ip=None, jm_vuid=0): + failure_domain=-1, mgmt_ip=None, jm_vuid=0, + is_secondary_node=False, physical_label=0): n = MagicMock(spec=StorageNode) n.uuid = node_id + # Defaults matching the real model: without them these come back as + # truthy child mocks, which silently steers placement code down the + # dedicated-secondary-node / physical-label branches. + n.is_secondary_node = is_secondary_node + n.physical_label = physical_label n.get_id = MagicMock(return_value=node_id) n.status = status n.cluster_id = "cluster-1" @@ -255,6 +262,24 @@ def test_pick_secondary_uses_get_secondary_nodes(self): self.assertIn("n1", kwargs["exclude_ids"]) self.assertIn("n9", kwargs["exclude_ids"]) + def test_extra_exclude_ids_forwarded_to_candidate_search(self): + # Regression coverage for the 2026-08-28 finding: _relocate_replica_ + # between's nested vacate must be able to rule out a node beyond + # just the one being removed (specifically, the primary mid- + # relocation in the enclosing call) -- see extra_exclude_ids' + # docstring. + cl = _cluster() + primary = _node("p1", secondary_id="n1", tertiary_id="n9") + removed = _node("n1") + db = FakeDB(cl, [primary, removed]) + with patch.object(storage_node_ops, "get_secondary_nodes", + return_value=["n5"]) as gsn: + got = storage_node_ops._pick_replica_relocation_node( + primary, removed, "secondary", db, extra_exclude_ids=("n7",)) + self.assertEqual(got, "n5") + _, kwargs = gsn.call_args + self.assertIn("n7", kwargs["exclude_ids"]) + # --------------------------------------------------------------------------- # _find_splice_target_for_relocation — the removal-repair fallback used when @@ -327,16 +352,111 @@ def test_skips_edge_not_host_disjoint_from_stranded(self): got = storage_node_ops._find_splice_target_for_relocation(stranded, "secondary", db) self.assertIsNone(got) + def test_hard_excludes_edge_where_p_shares_strandeds_domain(self): + # Once spliced, p. is repointed onto stranded itself (see + # _relocate_replica_between) -- if p's own domain matches + # stranded's, p ends up with a role-target in its own domain, the + # same violation X's domain is already hard-excluded against. This + # must refuse (None) even when it's the ONLY candidate edge -- + # degrading a node uninvolved in this removal is worse than + # refusing the relocation (2026-08-28 finding: this was only ever + # soft-scored before, and a live splice let it through). + cl = _cluster(enable_failure_domain=True) + stranded = _node("s", failure_domain=3, mgmt_ip="10.0.0.9") + p = _node("p", secondary_id="x", failure_domain=3, mgmt_ip="10.0.0.1") # same domain as stranded + x = _node("x", failure_domain=2, mgmt_ip="10.0.0.2") + db = FakeDB(cl, [stranded, p, x]) + got = storage_node_ops._find_splice_target_for_relocation(stranded, "secondary", db) + self.assertIsNone(got) + def test_tertiary_edge_respects_secondary_host_disjointness(self): cl = _cluster(enable_failure_domain=True) stranded = _node("s", failure_domain=0, secondary_id="s_sec", mgmt_ip="10.0.0.9") s_sec = _node("s_sec", failure_domain=1, mgmt_ip="10.0.0.50") - p = _node("p", tertiary_id="x", failure_domain=0, mgmt_ip="10.0.0.60") + p = _node("p", tertiary_id="x", failure_domain=2, mgmt_ip="10.0.0.60") x = _node("x", failure_domain=1, mgmt_ip="10.0.0.61") db = FakeDB(cl, [stranded, s_sec, p, x]) got = storage_node_ops._find_splice_target_for_relocation(stranded, "tertiary", db) self.assertEqual(got, ("p", "x")) + # ----------------------------------------------------------------- + # P's OWN other role is PREFERRED to stay diverse from stranded once + # P's `field` is repointed onto it, but this is a soft preference, not + # a hard filter -- regression coverage for the 2026-08-27 live finding: + # splicing kc25l into 56mg5's secondary slot collided with 56mg5's + # pre-existing, untouched tertiary in the same domain, when another + # edge elsewhere in the ring was collision-free the whole time. The old + # avoid_domains-only check had no way to prefer it (it only ever looked + # at X's domain, never P's) -- but an outright reject would have been + # more restrictive than useful, since a real cluster usually has + # several candidate edges and one of them is typically clean. + # ----------------------------------------------------------------- + + def test_prefers_edge_whose_p_stays_diverse_over_one_that_collides(self): + cl = _cluster(enable_failure_domain=True) + stranded = _node("s", failure_domain=3, tertiary_id="s_ter", mgmt_ip="10.0.0.9") + s_ter = _node("s_ter", failure_domain=4, mgmt_ip="10.0.0.10") + # bad_p's own domain differs from stranded's (so it isn't hard- + # excluded), but its OWN tertiary shares stranded's domain (3) -- + # splicing stranded into bad_p's secondary slot would collide with + # bad_p's own untouched tertiary. + bad_p = _node("bad_p", secondary_id="bad_x", tertiary_id="bad_p_ter", + failure_domain=1, mgmt_ip="10.0.0.1") + bad_p_ter = _node("bad_p_ter", failure_domain=3, mgmt_ip="10.0.0.11") + bad_x = _node("bad_x", failure_domain=2, mgmt_ip="10.0.0.2") + # good_p's own tertiary does NOT collide -- same domain-mismatch + # score as bad_p/bad_x (both ends unlike stranded's domain), tied + # only broken by the other-role preference, so it must still win. + good_p = _node("good_p", secondary_id="good_x", tertiary_id="good_p_ter", + failure_domain=2, mgmt_ip="10.0.0.3") + good_p_ter = _node("good_p_ter", failure_domain=1, mgmt_ip="10.0.0.13") + good_x = _node("good_x", failure_domain=4, mgmt_ip="10.0.0.4") + db = FakeDB(cl, [stranded, s_ter, bad_p, bad_p_ter, bad_x, good_p, good_p_ter, good_x]) + got = storage_node_ops._find_splice_target_for_relocation(stranded, "secondary", db) + self.assertEqual(got, ("good_p", "good_x")) + + def test_falls_back_to_colliding_edge_with_warning_when_its_the_only_one(self): + cl = _cluster(enable_failure_domain=True) + stranded = _node("s", failure_domain=3, tertiary_id="s_ter", mgmt_ip="10.0.0.9") + s_ter = _node("s_ter", failure_domain=4, mgmt_ip="10.0.0.10") + p = _node("p", secondary_id="x", tertiary_id="p_ter", failure_domain=1, mgmt_ip="10.0.0.1") + p_ter = _node("p_ter", failure_domain=3, mgmt_ip="10.0.0.20") + x = _node("x", failure_domain=2, mgmt_ip="10.0.0.2") + db = FakeDB(cl, [stranded, s_ter, p, p_ter, x]) + with patch.object(storage_node_ops, "logger") as log: + got = storage_node_ops._find_splice_target_for_relocation(stranded, "secondary", db) + self.assertEqual(got, ("p", "x")) + log.warning.assert_called_once() + + def test_uses_edge_when_ps_other_role_does_not_collide(self): + cl = _cluster(enable_failure_domain=True) + stranded = _node("s", failure_domain=3, tertiary_id="s_ter", mgmt_ip="10.0.0.9") + s_ter = _node("s_ter", failure_domain=4, mgmt_ip="10.0.0.10") + p = _node("p", secondary_id="x", tertiary_id="p_ter", failure_domain=1, mgmt_ip="10.0.0.1") + p_ter = _node("p_ter", failure_domain=2, mgmt_ip="10.0.0.20") # no collision + x = _node("x", failure_domain=2, mgmt_ip="10.0.0.2") + db = FakeDB(cl, [stranded, s_ter, p, p_ter, x]) + with patch.object(storage_node_ops, "logger") as log: + got = storage_node_ops._find_splice_target_for_relocation(stranded, "secondary", db) + self.assertEqual(got, ("p", "x")) + log.warning.assert_not_called() + + def test_falls_back_to_colliding_tertiary_edge_when_its_the_only_one(self): + cl = _cluster(enable_failure_domain=True) + stranded = _node("s", failure_domain=3, secondary_id="s_sec", mgmt_ip="10.0.0.9") + s_sec = _node("s_sec", failure_domain=4, mgmt_ip="10.0.0.10") + # p's OWN secondary shares stranded's domain (3) -- splicing stranded + # into p's tertiary slot collides with p's own untouched secondary, + # but it's the only edge available, so it's used anyway. + p = _node("p", tertiary_id="x", secondary_id="p_sec", failure_domain=1, mgmt_ip="10.0.0.60") + p_sec = _node("p_sec", failure_domain=3, mgmt_ip="10.0.0.70") + x = _node("x", failure_domain=2, mgmt_ip="10.0.0.61") + db = FakeDB(cl, [stranded, s_sec, p, p_sec, x]) + with patch.object(storage_node_ops, "logger") as log: + got = storage_node_ops._find_splice_target_for_relocation(stranded, "tertiary", db) + self.assertEqual(got, ("p", "x")) + log.warning.assert_called_once() + # --------------------------------------------------------------------------- # _pick_replica_relocation_node — falls back to the splice finder above when @@ -410,6 +530,112 @@ def test_fd_disabled_never_needs_splice_when_candidate_exists(self): finder.assert_not_called() +# --------------------------------------------------------------------------- +# _pick_replica_relocation_node — full pairwise diversity across +# {primary, secondary, tertiary}. +# +# Regression coverage for the 2026-08-27 finding: the old logic only ever +# enforced ">=1 cross-domain role" -- once the OTHER already-assigned role +# happened to be cross-domain, 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 in the other role's domain. These tests cover the tiered +# replacement: (1) prefer a direct candidate diverse from BOTH the primary +# and the other role, (2) splice for the same full diversity, (3) relax to +# the old weaker floor (diverse from the primary alone) only when full +# diversity is unreachable anywhere, logging the degraded outcome. +# --------------------------------------------------------------------------- + +class TestPickReplicaRelocationFullDiversity(unittest.TestCase): + + def test_prefers_direct_candidate_diverse_from_both_primary_and_other_role(self): + cl = _cluster(enable_failure_domain=True) + primary = _node("p1", secondary_id="n1", tertiary_id="t1", + failure_domain=0, mgmt_ip="10.0.0.9") + removed = _node("n1", failure_domain=1, mgmt_ip="10.0.0.99") + other = _node("t1", failure_domain=1, mgmt_ip="10.0.0.50") + same_as_other = _node("bad", failure_domain=1, mgmt_ip="10.0.0.2") + fully_diverse = _node("good", failure_domain=2, mgmt_ip="10.0.0.3") + db = FakeDB(cl, [primary, removed, other, same_as_other, fully_diverse]) + with patch.object(storage_node_ops, "get_secondary_nodes", + return_value=["bad", "good"]): + got = storage_node_ops._pick_replica_relocation_node( + primary, removed, "secondary", db) + self.assertEqual(got, "good") + + def test_falls_back_to_splice_for_full_diversity_when_direct_only_matches_other_role(self): + cl = _cluster(enable_failure_domain=True) + primary = _node("p1", secondary_id="n1", tertiary_id="t1", + failure_domain=0, mgmt_ip="10.0.0.9") + removed = _node("n1", failure_domain=1, mgmt_ip="10.0.0.99") + other = _node("t1", failure_domain=1, mgmt_ip="10.0.0.50") + same_as_other = _node("bad", failure_domain=1, mgmt_ip="10.0.0.2") + edge_p = _node("edge_p", secondary_id="edge_x", failure_domain=3, mgmt_ip="10.0.0.10") + edge_x = _node("edge_x", failure_domain=2, mgmt_ip="10.0.0.11") + db = FakeDB(cl, [primary, removed, other, same_as_other, edge_p, edge_x]) + with patch.object(storage_node_ops, "get_secondary_nodes", + return_value=["bad"]): + got = storage_node_ops._pick_replica_relocation_node( + primary, removed, "secondary", db) + self.assertEqual(got, "edge_x") + + def test_relaxes_to_weaker_floor_with_warning_when_full_diversity_unreachable(self): + # Only domains 0 (primary) and 1 (other role + every candidate) + # exist -- no direct candidate or splice target can be diverse from + # BOTH roles, so this must relax to "diverse from the primary alone" + # rather than refuse the relocation outright. + cl = _cluster(enable_failure_domain=True) + primary = _node("p1", secondary_id="n1", tertiary_id="t1", + failure_domain=0, mgmt_ip="10.0.0.9") + removed = _node("n1", failure_domain=1, mgmt_ip="10.0.0.99") + other = _node("t1", failure_domain=1, mgmt_ip="10.0.0.50") + weak_cand = _node("weak_cand", failure_domain=1, mgmt_ip="10.0.0.2") + db = FakeDB(cl, [primary, removed, other, weak_cand]) + with patch.object(storage_node_ops, "get_secondary_nodes", + return_value=["weak_cand"]), \ + patch.object(storage_node_ops, "logger") as log: + got = storage_node_ops._pick_replica_relocation_node( + primary, removed, "secondary", db) + self.assertEqual(got, "weak_cand") + log.warning.assert_called_once() + + def test_relaxes_to_weaker_splice_with_warning_when_full_diversity_unreachable(self): + # No direct candidate at all; the only splice target's far end sits + # in the other role's domain, so full diversity is unreachable -- + # must relax to the weaker splice (diverse from the primary alone) + # instead of returning None. + cl = _cluster(enable_failure_domain=True) + primary = _node("p1", secondary_id="n1", tertiary_id="t1", + failure_domain=0, mgmt_ip="10.0.0.9") + removed = _node("n1", failure_domain=1, mgmt_ip="10.0.0.99") + other = _node("t1", failure_domain=1, mgmt_ip="10.0.0.50") + edge_p = _node("edge_p", secondary_id="edge_x", failure_domain=3, mgmt_ip="10.0.0.10") + edge_x = _node("edge_x", failure_domain=1, mgmt_ip="10.0.0.11") + db = FakeDB(cl, [primary, removed, other, edge_p, edge_x]) + with patch.object(storage_node_ops, "get_secondary_nodes", return_value=[]), \ + patch.object(storage_node_ops, "logger") as log: + got = storage_node_ops._pick_replica_relocation_node( + primary, removed, "secondary", db) + self.assertEqual(got, "edge_x") + log.warning.assert_called_once() + + def test_no_relaxation_warning_when_no_other_role_is_assigned(self): + # With no other role placed yet, full_avoid == weak_avoid already -- + # this is the ordinary single-constraint case, not a degraded + # fallback, so no warning should fire. + cl = _cluster(enable_failure_domain=True) + primary = _node("p1", secondary_id="n1", failure_domain=0, mgmt_ip="10.0.0.9") + removed = _node("n1", failure_domain=1, mgmt_ip="10.0.0.99") + cand = _node("cand", failure_domain=1, mgmt_ip="10.0.0.2") + db = FakeDB(cl, [primary, removed, cand]) + with patch.object(storage_node_ops, "get_secondary_nodes", + return_value=["cand"]), \ + patch.object(storage_node_ops, "logger") as log: + got = storage_node_ops._pick_replica_relocation_node( + primary, removed, "secondary", db) + self.assertEqual(got, "cand") + log.warning.assert_not_called() + + # --------------------------------------------------------------------------- # Case A — teardown of own primary's replicas # --------------------------------------------------------------------------- @@ -850,6 +1076,31 @@ def test_relocate_via_splice_evicts_occupant_first(self): self.assertEqual(rec.call_count, 2) # occupant's rebuild + stranded's own rebuild self.assertEqual(removed.lvstore_stack_secondary, "") + def test_relocate_via_splice_repairs_occupants_other_role_afterwards(self): + # Wiring check: after a successful splice, _relocate_one_replica must + # call _repair_occupants_other_role_after_splice so occupant's OTHER, + # untouched role gets a chance to be moved away from a collision with + # stranded's domain (see that function's own docstring/tests for the + # actual repair logic). + cl = _cluster() + removed = _node("n1", stack_secondary="stranded") + stranded = _node("stranded", secondary_id="n1", lvstore="LVS_stranded") + occupant = _node("occupant", secondary_id="x", lvstore="LVS_occupant") + x = _node("x", stack_secondary="occupant") + db = FakeDB(cl, [removed, stranded, occupant, x]) + with patch.object(storage_node_ops, "DBController", return_value=db), \ + patch.object(storage_node_ops, "_pick_replica_relocation_node", + return_value="x"), \ + patch.object(storage_node_ops, "recreate_lvstore_on_non_leader", + return_value=True), \ + patch.object(storage_node_ops, "_delete_replica_on_peer"), \ + patch.object(storage_node_ops, + "_repair_occupants_other_role_after_splice") as repair: + ret = storage_node_ops._relocate_one_replica(removed, "stranded", "secondary") + + self.assertTrue(ret) + repair.assert_called_once_with("occupant", "stranded", "secondary", db) + def test_relocate_via_splice_repoints_lvol_nodes_for_both_moved_primaries(self): # Regression: BOTH moves this splice performs -- occupant's replica # x -> stranded, and stranded's own replica n1 -> x -- must repoint @@ -1028,7 +1279,7 @@ def test_relocate_via_splice_vacates_strandeds_preexisting_occupant_first(self): free_node = _node("free", lvstore="LVS_free") # genuinely unclaimed db = FakeDB(cl, [removed, stranded, occupant, x, z, free_node]) - def pick_side_effect(primary, exclude_node, role, db_controller): + def pick_side_effect(primary, exclude_node, role, db_controller, extra_exclude_ids=()): if primary.get_id() == "stranded": self.assertEqual(exclude_node.get_id(), "n1") return "x" @@ -1061,6 +1312,47 @@ def pick_side_effect(primary, exclude_node, role, db_controller): self.assertEqual(rec.call_count, 3) # z's + occupant's + stranded's own rebuild self.assertEqual(drp.call_count, 2) # old z copy off stranded, old occupant copy off x + def test_relocate_via_splice_vacate_excludes_the_in_flight_occupant(self): + # 2026-08-28 finding: occupant is being spliced onto stranded in + # THIS call, but stranded already hosts z. Picking z's new target + # must rule out occupant explicitly -- occupant can't simultaneously + # be the thing moving onto stranded AND the target z vacates onto. + # Without passing that exclusion through, the picker's one and only + # candidate can BE occupant, and the whole splice dead-ends + # retrying the identical failure forever instead of looking past it. + cl = _cluster() + removed = _node("n1", stack_secondary="stranded") + stranded = _node("stranded", secondary_id="n1", lvstore="LVS_stranded", + stack_secondary="z") + occupant = _node("occupant", secondary_id="x", lvstore="LVS_occupant") + x = _node("x", stack_secondary="occupant") + z = _node("z", secondary_id="stranded", lvstore="LVS_z") + free_node = _node("free", lvstore="LVS_free") + db = FakeDB(cl, [removed, stranded, occupant, x, z, free_node]) + + def pick_side_effect(primary, exclude_node, role, db_controller, extra_exclude_ids=()): + if primary.get_id() == "stranded": + return "x" + if primary.get_id() == "z": + # The exclusion must be in place BEFORE the search runs, not + # discovered as a dead end after the fact. + self.assertIn("occupant", extra_exclude_ids) + return "free" + raise AssertionError(f"unexpected pick for {primary.get_id()}") + + with patch.object(storage_node_ops, "DBController", return_value=db), \ + patch.object(storage_node_ops, "_pick_replica_relocation_node", + side_effect=pick_side_effect), \ + patch.object(storage_node_ops, "recreate_lvstore_on_non_leader", + return_value=True), \ + patch.object(storage_node_ops, "_delete_replica_on_peer"): + ret = storage_node_ops._relocate_one_replica(removed, "stranded", "secondary") + + self.assertTrue(ret) + self.assertEqual(z.secondary_node_id, "free") + self.assertEqual(stranded.secondary_node_id, "x") + self.assertEqual(occupant.secondary_node_id, "stranded") + def test_relocate_via_splice_refuses_when_preexisting_occupant_has_no_target(self): # Same setup, but z has nowhere to go. Must fail closed -- refuse the # whole splice rather than overload stranded's single-value slot. @@ -1073,7 +1365,7 @@ def test_relocate_via_splice_refuses_when_preexisting_occupant_has_no_target(sel z = _node("z", secondary_id="stranded", lvstore="LVS_z") db = FakeDB(cl, [removed, stranded, occupant, x, z]) - def pick_side_effect(primary, exclude_node, role, db_controller): + def pick_side_effect(primary, exclude_node, role, db_controller, extra_exclude_ids=()): if primary.get_id() == "stranded": return "x" if primary.get_id() == "z": @@ -1139,7 +1431,7 @@ def test_relocate_via_splice_tertiary_vacates_strandeds_preexisting_occupant_fir free_node = _node("free", lvstore="LVS_free") db = FakeDB(cl, [removed, stranded, occupant, x, z, free_node]) - def pick_side_effect(primary, exclude_node, role, db_controller): + def pick_side_effect(primary, exclude_node, role, db_controller, extra_exclude_ids=()): self.assertEqual(role, "tertiary") if primary.get_id() == "stranded": return "x" @@ -1190,6 +1482,393 @@ def test_relocate_free_target_never_triggers_splice_eviction(self): self.assertEqual(free_node.lvstore_stack_secondary, "p1") +# --------------------------------------------------------------------------- +# Plan-driven phase 3b — with failure domains on, relocation goes through the +# global planner (simplyblock_core.controllers.replica_placement) instead of +# the per-replica greedy picker. The planner is unit-tested on its own in +# tests/unit/test_replica_placement.py; what matters here is the wiring: +# which clusters it takes, which it declines, and that the moves it plans are +# actually executed against the DB bookkeeping. +# --------------------------------------------------------------------------- + +def _fd_cluster_nodes(domains=4, per_domain=3, ftt=2): + """A cluster laid out the way cluster_activate leaves it: nodes + round-robined across domains, secondary/tertiary one and two steps along + that order, so every LVS starts fully domain-diverse.""" + order = [f"d{d}n{i}" for i in range(per_domain) for d in range(domains)] + nodes = {} + for node_id in order: + nodes[node_id] = _node( + node_id, lvstore=f"LVS_{node_id}", + failure_domain=int(node_id[1]), + mgmt_ip=f"10.0.{node_id[1]}.{node_id[-1]}") + size = len(order) + for k, node_id in enumerate(order): + sec = order[(k + 1) % size] + tert = order[(k + 2) % size] if ftt >= 2 else "" + nodes[node_id].secondary_node_id = sec + nodes[node_id].tertiary_node_id = tert + nodes[sec].lvstore_stack_secondary = node_id + if tert: + nodes[tert].lvstore_stack_tertiary = node_id + return nodes + + +def _layout_of(db, ftt=2): + return { + n.get_id(): (n.secondary_node_id, n.tertiary_node_id if ftt >= 2 else "") + for n in db.nodes.values() + if n.status != StorageNode.STATUS_REMOVED + } + + +class TestRelocationPlannerApplicability(unittest.TestCase): + + def _db(self, **cluster_kwargs): + cl = _cluster(npcs=2, ft=2, enable_failure_domain=True, **cluster_kwargs) + nodes = _fd_cluster_nodes() + return cl, FakeDB(cl, list(nodes.values())), nodes + + def test_takes_an_fd_enabled_cluster(self): + cl, db, nodes = self._db() + got = storage_node_ops._relocation_planner_inputs(nodes["d0n0"], db) + self.assertIsNotNone(got) + surviving_ids, fd_by_node, host_by_node, _, current_layout, ftt = got + self.assertNotIn("d0n0", surviving_ids) + self.assertEqual(len(surviving_ids), 11) + self.assertEqual(ftt, 2) + self.assertEqual(fd_by_node["d1n0"], 1) + self.assertEqual(host_by_node["d1n0"], nodes["d1n0"].mgmt_ip) + # the layout is read raw, still naming the node being removed + self.assertIn("d0n0", [pl.secondary for pl in current_layout.values()]) + + def test_applies_when_failure_domains_are_off(self): + """FD off no longer declines: the planner solves host-disjointness + with one pseudo-domain per host. Previously this returned None and + the greedy path could refuse a feasible removal.""" + cl = _cluster(npcs=2, ft=2, enable_failure_domain=False) + nodes = _fd_cluster_nodes() + db = FakeDB(cl, list(nodes.values())) + inputs = storage_node_ops._relocation_planner_inputs( + nodes["d0n0"], db, allow_without_fd=True) + self.assertIsNotNone(inputs) + _ids, fd_by_node, host_by_node, _lbl, _layout, _ftt = inputs + self.assertEqual(len(set(fd_by_node.values())), len(set(host_by_node.values()))) + + def test_declines_when_survivors_do_not_exceed_ftt(self): + """Too few nodes for the permutation model: leave it to the greedy + path rather than refuse a trivially-safe removal.""" + cl = _cluster(ft=2, enable_failure_domain=False) + a = _node("a", lvstore="L1", mgmt_ip="10.0.0.1") + b = _node("b", lvstore="L2", mgmt_ip="10.0.0.2") + removed = _node("z", lvstore="L3", mgmt_ip="10.0.0.9") + db = FakeDB(cl, [a, b, removed]) + self.assertIsNone( + storage_node_ops._relocation_planner_inputs(removed, db)) + + def test_declines_on_a_node_without_a_domain(self): + cl, db, nodes = self._db() + nodes["d2n1"].failure_domain = -1 + self.assertIsNone( + storage_node_ops._relocation_planner_inputs(nodes["d0n0"], db)) + + def test_declines_when_a_dedicated_secondary_node_exists(self): + # Such a node may host more than one replica, breaking the + # one-slot-per-node permutation the planner is built on. + cl, db, nodes = self._db() + nodes["d2n1"].is_secondary_node = True + self.assertIsNone( + storage_node_ops._relocation_planner_inputs(nodes["d0n0"], db)) + + def test_declines_when_a_peer_is_not_online(self): + cl, db, nodes = self._db() + nodes["d2n1"].status = StorageNode.STATUS_OFFLINE + self.assertIsNone( + storage_node_ops._relocation_planner_inputs(nodes["d0n0"], db)) + + def test_already_removed_nodes_are_not_survivors(self): + cl, db, nodes = self._db() + nodes["d3n2"].status = StorageNode.STATUS_REMOVED + got = storage_node_ops._relocation_planner_inputs(nodes["d0n0"], db) + self.assertIsNotNone(got) + self.assertNotIn("d3n2", got[0]) + + def test_ftt1_cluster_ignores_the_tertiary(self): + cl = _cluster(npcs=1, ft=1, enable_failure_domain=True) + nodes = _fd_cluster_nodes(ftt=1) + db = FakeDB(cl, list(nodes.values())) + got = storage_node_ops._relocation_planner_inputs(nodes["d0n0"], db) + self.assertEqual(got[5], 1) + self.assertTrue(all(pl.tertiary == "" for pl in got[4].values())) + + +class TestPlanDrivenRelocation(unittest.TestCase): + """The reported case end to end against the DB bookkeeping: 4 domains x 3 + hosts, FTT2 ("2+2"), removing one host per domain. Every surviving LVS + must keep primary/secondary/tertiary in three distinct domains -- the + guarantee the per-replica picker could not hold, because the repair it + needs (swapping two replicas that are both already placed) is not + expressible one stranded role at a time.""" + + def _run_removal(self, db, nodes, victim_id, ftt=2): + victim = nodes[victim_id] + # phase 3a: the victim's own LVS replicas come down. + for field, backref in (("secondary_node_id", "lvstore_stack_secondary"), + ("tertiary_node_id", "lvstore_stack_tertiary")): + peer_id = getattr(victim, field) + if peer_id and getattr(nodes[peer_id], backref) == victim_id: + setattr(nodes[peer_id], backref, "") + setattr(victim, field, "") + victim.status = StorageNode.STATUS_IN_REMOVAL + + moved = [] + + def _fake_move(primary_id, old_host_id, new_host_id, role, _db, _seen=None): + field = "secondary_node_id" if role == "secondary" else "tertiary_node_id" + backref = ("lvstore_stack_secondary" if role == "secondary" + else "lvstore_stack_tertiary") + self.assertEqual(getattr(nodes[new_host_id], backref), "", + f"planned move onto an occupied {role} slot on {new_host_id}") + moved.append((primary_id, role, old_host_id, new_host_id)) + setattr(nodes[primary_id], field, new_host_id) + setattr(nodes[new_host_id], backref, primary_id) + if getattr(nodes[old_host_id], backref) == primary_id: + setattr(nodes[old_host_id], backref, "") + return True + + with patch.object(storage_node_ops, "_relocate_replica_between", + side_effect=_fake_move): + ret = storage_node_ops._relocate_replicas_hosted_on(victim) + self.assertTrue(ret) + + victim.status = StorageNode.STATUS_REMOVED + del db.nodes[victim_id] + del nodes[victim_id] + return moved + + def _assert_fully_diverse(self, nodes, ftt=2): + for node_id, node in nodes.items(): + domains = [node.failure_domain, nodes[node.secondary_node_id].failure_domain] + if ftt >= 2: + domains.append(nodes[node.tertiary_node_id].failure_domain) + self.assertEqual( + len(set(domains)), len(domains), + f"{node_id} roles share a domain: sec={node.secondary_node_id} " + f"tert={node.tertiary_node_id} domains={domains}") + for field, backref in (("secondary_node_id", "lvstore_stack_secondary"), + ("tertiary_node_id", "lvstore_stack_tertiary")): + if ftt < 2 and field == "tertiary_node_id": + continue + holders = [getattr(n, field) for n in nodes.values()] + self.assertCountEqual(holders, list(nodes), f"{field} is not a permutation") + for node_id, node in nodes.items(): + self.assertEqual(getattr(nodes[getattr(node, field)], backref), node_id) + + def test_one_removal_per_domain_keeps_full_diversity(self): + cl = _cluster(npcs=2, ft=2, enable_failure_domain=True) + nodes = _fd_cluster_nodes() + db = FakeDB(cl, list(nodes.values())) + with patch.object(storage_node_ops, "DBController", return_value=db): + for domain in range(4): + self._run_removal(db, nodes, f"d{domain}n0") + self._assert_fully_diverse(nodes) + self.assertEqual(len(nodes), 8) + + def test_the_removed_nodes_backrefs_are_cleared(self): + cl = _cluster(npcs=2, ft=2, enable_failure_domain=True) + nodes = _fd_cluster_nodes() + db = FakeDB(cl, list(nodes.values())) + victim = nodes["d0n0"] + with patch.object(storage_node_ops, "DBController", return_value=db): + self._run_removal(db, nodes, "d0n0") + self.assertEqual(victim.lvstore_stack_secondary, "") + self.assertEqual(victim.lvstore_stack_tertiary, "") + + def test_a_failed_move_fails_the_phase_for_retry(self): + cl = _cluster(npcs=2, ft=2, enable_failure_domain=True) + nodes = _fd_cluster_nodes() + db = FakeDB(cl, list(nodes.values())) + victim = nodes["d0n0"] + with patch.object(storage_node_ops, "DBController", return_value=db), \ + patch.object(storage_node_ops, "_relocate_replica_between", + return_value=False): + ret = storage_node_ops._relocate_replicas_hosted_on(victim) + self.assertFalse(ret) + # nothing cleared -> the retry re-plans from the same state + self.assertNotEqual(victim.lvstore_stack_secondary, "") + + def test_an_already_correct_cluster_plans_no_moves(self): + # Removing a node whose slots are already free and whose own replicas + # are already torn down must not churn the rest of the cluster. + cl = _cluster(npcs=2, ft=2, enable_failure_domain=True) + nodes = _fd_cluster_nodes() + db = FakeDB(cl, list(nodes.values())) + with patch.object(storage_node_ops, "DBController", return_value=db): + moved = self._run_removal(db, nodes, "d0n0") + # exactly the two roles the victim hosted, plus whatever re-shuffle + # full diversity needs -- never the whole cluster. + self.assertLess(len(moved), 8, moved) + self.assertGreaterEqual(len(moved), 2, moved) + + def test_falls_back_to_the_greedy_path_when_the_planner_declines(self): + # FD off no longer declines, so force a decline the planner still + # honours: a cluster with dedicated secondary nodes. + cl = _cluster(npcs=2, ft=2, enable_failure_domain=False) + nodes = _fd_cluster_nodes() + for n in nodes.values(): + n.is_secondary_node = True + db = FakeDB(cl, list(nodes.values())) + victim = nodes["d0n0"] + with patch.object(storage_node_ops, "DBController", return_value=db), \ + patch.object(storage_node_ops, "_relocate_one_replica", + return_value=True) as one: + ret = storage_node_ops._relocate_replicas_hosted_on(victim) + self.assertTrue(ret) + self.assertEqual(one.call_count, 2) + + +class TestFeasibilityUsesThePlanner(unittest.TestCase): + + def test_admits_a_removal_the_planner_can_satisfy(self): + cl = _cluster(npcs=2, ft=2, enable_failure_domain=True) + nodes = _fd_cluster_nodes() + db = FakeDB(cl, list(nodes.values())) + with patch.object(storage_node_ops, "_pick_replica_relocation_node") as pick: + ok, reason = storage_node_ops._check_replica_relocation_feasible( + nodes["d0n0"], db) + self.assertTrue(ok, reason) + pick.assert_not_called() + + def test_admits_but_warns_when_full_diversity_is_unreachable(self): + # 2 domains at FTT2: a tertiary can never avoid both the primary's + # and the secondary's domain. Admitted (host-disjointness still + # holds) but every degraded LVS is named in the log. + cl = _cluster(npcs=2, ft=2, enable_failure_domain=True) + nodes = _fd_cluster_nodes(domains=2, per_domain=3) + db = FakeDB(cl, list(nodes.values())) + with self.assertLogs(storage_node_ops.logger, level="WARNING") as logs: + ok, _ = storage_node_ops._check_replica_relocation_feasible( + nodes["d0n0"], db) + self.assertTrue(ok) + self.assertTrue(any("cannot be made fully domain-diverse" in line + for line in logs.output)) + + def test_refuses_when_no_host_disjoint_layout_exists(self): + cl = _cluster(npcs=2, ft=2, enable_failure_domain=True) + nodes = _fd_cluster_nodes(domains=3, per_domain=1) + db = FakeDB(cl, list(nodes.values())) + ok, reason = storage_node_ops._check_replica_relocation_feasible( + nodes["d0n0"], db) + self.assertFalse(ok) + self.assertTrue(reason) + + +# --------------------------------------------------------------------------- +# _repair_occupants_other_role_after_splice — a splice edge protects the node +# actually being relocated (and, since the diversity fix, prefers one where +# the occupant it repoints stays diverse too) but still accepts a colliding +# edge as a last resort. This actively closes that gap: after the splice, +# check whether occupant's OTHER, untouched role now shares a domain with +# the node it was just repointed onto, and if so, relocate that role too via +# the same picker + mover (2026-08-28 finding, following directly from the +# "prefer, don't require" splice fix). +# --------------------------------------------------------------------------- + +class TestRepairOccupantsOtherRoleAfterSplice(unittest.TestCase): + + def test_no_op_when_occupants_other_role_does_not_collide(self): + cl = _cluster(enable_failure_domain=True) + primary = _node("primary", failure_domain=3) + ter = _node("ter", failure_domain=2) # no collision with primary's domain(3) + occupant = _node("occupant", failure_domain=1, secondary_id="primary", tertiary_id="ter") + db = FakeDB(cl, [primary, ter, occupant]) + with patch.object(storage_node_ops, "_pick_replica_relocation_node") as pick, \ + patch.object(storage_node_ops, "_relocate_replica_between") as move: + storage_node_ops._repair_occupants_other_role_after_splice( + "occupant", "primary", "secondary", db) + pick.assert_not_called() + move.assert_not_called() + + def test_relocates_occupants_other_role_when_it_collides(self): + cl = _cluster(enable_failure_domain=True) + primary = _node("primary", failure_domain=3) + old_ter = _node("old_ter", failure_domain=3) # collides with primary's domain + occupant = _node("occupant", failure_domain=1, secondary_id="primary", tertiary_id="old_ter") + db = FakeDB(cl, [primary, old_ter, occupant]) + with patch.object(storage_node_ops, "_pick_replica_relocation_node", + return_value="replacement") as pick, \ + patch.object(storage_node_ops, "_relocate_replica_between", + return_value=True) as move: + storage_node_ops._repair_occupants_other_role_after_splice( + "occupant", "primary", "secondary", db) + pick.assert_called_once_with(occupant, old_ter, "tertiary", db) + move.assert_called_once_with("occupant", "old_ter", "replacement", "tertiary", db) + + def test_tertiary_role_checks_secondary_as_the_other_role(self): + cl = _cluster(enable_failure_domain=True) + primary = _node("primary", failure_domain=3) + old_sec = _node("old_sec", failure_domain=3) + occupant = _node("occupant", failure_domain=1, tertiary_id="primary", secondary_id="old_sec") + db = FakeDB(cl, [primary, old_sec, occupant]) + with patch.object(storage_node_ops, "_pick_replica_relocation_node", + return_value="replacement") as pick, \ + patch.object(storage_node_ops, "_relocate_replica_between", + return_value=True) as move: + storage_node_ops._repair_occupants_other_role_after_splice( + "occupant", "primary", "tertiary", db) + pick.assert_called_once_with(occupant, old_sec, "secondary", db) + move.assert_called_once_with("occupant", "old_sec", "replacement", "secondary", db) + + def test_logs_warning_when_no_replacement_found(self): + cl = _cluster(enable_failure_domain=True) + primary = _node("primary", failure_domain=3) + old_ter = _node("old_ter", failure_domain=3) + occupant = _node("occupant", failure_domain=1, secondary_id="primary", tertiary_id="old_ter") + db = FakeDB(cl, [primary, old_ter, occupant]) + with patch.object(storage_node_ops, "_pick_replica_relocation_node", return_value=None), \ + patch.object(storage_node_ops, "_relocate_replica_between") as move, \ + patch.object(storage_node_ops, "logger") as log: + storage_node_ops._repair_occupants_other_role_after_splice( + "occupant", "primary", "secondary", db) + move.assert_not_called() + log.warning.assert_called_once() + + def test_logs_warning_when_relocation_itself_fails(self): + cl = _cluster(enable_failure_domain=True) + primary = _node("primary", failure_domain=3) + old_ter = _node("old_ter", failure_domain=3) + occupant = _node("occupant", failure_domain=1, secondary_id="primary", tertiary_id="old_ter") + db = FakeDB(cl, [primary, old_ter, occupant]) + with patch.object(storage_node_ops, "_pick_replica_relocation_node", + return_value="replacement"), \ + patch.object(storage_node_ops, "_relocate_replica_between", return_value=False), \ + patch.object(storage_node_ops, "logger") as log: + storage_node_ops._repair_occupants_other_role_after_splice( + "occupant", "primary", "secondary", db) + log.warning.assert_called_once() + + def test_no_op_when_fd_disabled(self): + cl = _cluster(enable_failure_domain=False) + primary = _node("primary", failure_domain=3) + old_ter = _node("old_ter", failure_domain=3) + occupant = _node("occupant", failure_domain=1, secondary_id="primary", tertiary_id="old_ter") + db = FakeDB(cl, [primary, old_ter, occupant]) + with patch.object(storage_node_ops, "_pick_replica_relocation_node") as pick: + storage_node_ops._repair_occupants_other_role_after_splice( + "occupant", "primary", "secondary", db) + pick.assert_not_called() + + def test_no_op_when_occupant_has_no_other_role_assigned(self): + cl = _cluster(enable_failure_domain=True) + primary = _node("primary", failure_domain=3) + occupant = _node("occupant", failure_domain=1, secondary_id="primary") # no tertiary at all + db = FakeDB(cl, [primary, occupant]) + with patch.object(storage_node_ops, "_pick_replica_relocation_node") as pick: + storage_node_ops._repair_occupants_other_role_after_splice( + "occupant", "primary", "secondary", db) + pick.assert_not_called() + + # --------------------------------------------------------------------------- # Device decommission completion gate # --------------------------------------------------------------------------- @@ -2185,5 +2864,936 @@ def test_failed_migration_runner_still_refuses_inactive(self): "the gate must still hold for genuinely non-serving clusters") +# --------------------------------------------------------------------------- +# _relocate_replica_between — vacating one role must not tear down the stack +# when the SAME node still holds the OTHER role for the SAME primary. +# +# Live regression (2026-09-01, 12-node/4-domain FTT2). The global planner +# emits both of a primary's roles in one removal; for pq8h9/LVS_45 it emitted +# 1. secondary: 94dht -> fvgtl (fvgtl already held LVS_45 as tertiary) +# 2. tertiary: fvgtl -> nq2mm +# Step 2's teardown fired on fvgtl because it only consulted +# lvstore_stack_tertiary, and bdev_raid_delete(raid0_45) removed the very +# replica step 1 had just promoted -- 1s after nq2mm's copy was built: +# 14:50:26 nq2mm bdev_raid_create raid0_45 +# 14:50:27 fvgtl bdev_raid_delete raid0_45 +# pq8h9 was left recorded as FTT2 with a single physical replica. Secondary +# and tertiary of one primary are ONE stack (raid0_ + LVS_), so +# the guard has to be per-primary, not per-role. +# --------------------------------------------------------------------------- +class TestRelocateReplicaBetweenSamePrimaryOtherRole(unittest.TestCase): + + def _run(self, x_stack_secondary, x_stack_tertiary, role="tertiary"): + cl = _cluster() + primary = _node("P", lvstore="LVS_P", secondary_id="X", tertiary_id="X") + x = _node("X", stack_secondary=x_stack_secondary, stack_tertiary=x_stack_tertiary) + y = _node("Y") + db = FakeDB(cl, [primary, x, y]) + with patch.object(storage_node_ops, "recreate_lvstore_on_non_leader", + return_value=True), \ + patch.object(storage_node_ops, "_delete_replica_on_peer") as drp, \ + patch.object(storage_node_ops, + "_teardown_lvol_subsystems_on_vacated_peer") as tls, \ + patch.object(storage_node_ops, "_prune_stale_lvstore_ports") as psp, \ + patch.object(storage_node_ops, "_update_lvol_nodes_for_replica_move"): + ret = storage_node_ops._relocate_replica_between("P", "X", "Y", role, db) + return ret, x, primary, drp, tls, psp + + def test_keeps_stack_when_node_still_holds_other_role_for_same_primary(self): + # Moving P's TERTIARY off X while X is still P's SECONDARY. + ret, x, primary, drp, tls, psp = self._run( + x_stack_secondary="P", x_stack_tertiary="P", role="tertiary") + + self.assertTrue(ret) + drp.assert_not_called() + tls.assert_not_called() + psp.assert_not_called() + # Only the role being vacated is cleared; the other stays. + self.assertEqual(x.lvstore_stack_tertiary, "") + self.assertEqual(x.lvstore_stack_secondary, "P") + self.assertEqual(primary.tertiary_node_id, "Y") + + def test_keeps_stack_in_the_mirror_case_moving_secondary_away(self): + # Same shape with the roles swapped: moving P's SECONDARY off X while + # X remains P's TERTIARY. + ret, x, primary, drp, tls, psp = self._run( + x_stack_secondary="P", x_stack_tertiary="P", role="secondary") + + self.assertTrue(ret) + drp.assert_not_called() + tls.assert_not_called() + psp.assert_not_called() + self.assertEqual(x.lvstore_stack_secondary, "") + self.assertEqual(x.lvstore_stack_tertiary, "P") + self.assertEqual(primary.secondary_node_id, "Y") + + def test_still_tears_down_when_node_holds_no_other_role(self): + # Control: the ordinary case must be unchanged. + ret, x, _p, drp, tls, psp = self._run( + x_stack_secondary="", x_stack_tertiary="P", role="tertiary") + + self.assertTrue(ret) + drp.assert_called_once() + tls.assert_called_once() + psp.assert_called_once() + self.assertEqual(x.lvstore_stack_tertiary, "") + + def test_still_tears_down_when_other_role_belongs_to_a_different_primary(self): + # The guard must compare the PRIMARY, not merely "other backref set". + # X hosting some unrelated primary's secondary shares no stack with P, + # so P's tertiary copy on X must still be torn down. + ret, x, _p, drp, tls, psp = self._run( + x_stack_secondary="OTHER", x_stack_tertiary="P", role="tertiary") + + self.assertTrue(ret) + drp.assert_called_once() + tls.assert_called_once() + psp.assert_called_once() + self.assertEqual(x.lvstore_stack_tertiary, "") + self.assertEqual(x.lvstore_stack_secondary, "OTHER") + + +# --------------------------------------------------------------------------- +# replica_stack_violations / _verify_replica_stacks — the invariant whose +# absence let the bug above ship silently. Bookkeeping agreed with itself at +# every layer; only the device knew. +# --------------------------------------------------------------------------- +class TestReplicaStackViolations(unittest.TestCase): + + def _cluster_nodes(self): + # P1 hosted by A (secondary) and B (tertiary); P2 hosted by A (tertiary). + p1 = _node("P1", lvstore="LVS_1", secondary_id="A", tertiary_id="B") + p2 = _node("P2", lvstore="LVS_2", tertiary_id="A") + a = _node("A", lvstore="LVS_A", stack_secondary="P1", stack_tertiary="P2") + b = _node("B", lvstore="LVS_B", stack_tertiary="P1") + return [p1, p2, a, b] + + def test_no_violations_when_every_claimed_stack_is_present(self): + nodes = self._cluster_nodes() + self.assertEqual( + storage_node_ops.replica_stack_violations(nodes, lambda n, lvs: True), []) + + def test_flags_a_claimed_but_absent_stack(self): + nodes = self._cluster_nodes() + + def present(node, lvstore): + # Exactly the live failure: A is recorded as P1's secondary but + # LVS_1 is not on it. + return not (node.get_id() == "A" and lvstore == "LVS_1") + + self.assertEqual( + storage_node_ops.replica_stack_violations(nodes, present), + [("A", "LVS_1", "P1", "secondary")]) + + def test_reports_every_missing_stack_not_just_the_first(self): + nodes = self._cluster_nodes() + found = storage_node_ops.replica_stack_violations(nodes, lambda n, lvs: False) + self.assertEqual( + sorted(found), + sorted([("A", "LVS_1", "P1", "secondary"), + ("A", "LVS_2", "P2", "tertiary"), + ("B", "LVS_1", "P1", "tertiary")])) + + def test_ignores_nodes_that_claim_nothing(self): + idle = _node("idle", lvstore="LVS_idle") + calls = [] + + def present(node, lvstore): + calls.append((node.get_id(), lvstore)) + return True + + self.assertEqual( + storage_node_ops.replica_stack_violations([idle], present), []) + self.assertEqual(calls, [], "a node with no back-references must not be probed") + + def test_ignores_a_backref_whose_owner_has_no_lvstore(self): + # Nothing to probe for -- a different kind of bookkeeping problem, and + # reporting it here would be a false positive on the stack invariant. + owner = _node("P", lvstore="") + host = _node("H", stack_secondary="P") + self.assertEqual( + storage_node_ops.replica_stack_violations([owner, host], + lambda n, lvs: False), []) + + def test_ignores_a_backref_to_a_node_not_in_the_online_set(self): + host = _node("H", stack_secondary="gone") + self.assertEqual( + storage_node_ops.replica_stack_violations([host], lambda n, lvs: False), []) + + +class TestVerifyReplicaStacks(unittest.TestCase): + + def _db(self, probe_result): + cl = _cluster() + p = _node("P", lvstore="LVS_P", secondary_id="A") + a = _node("A", lvstore="LVS_A", stack_secondary="P") + a.rpc_client.return_value.bdev_lvol_get_lvstores = MagicMock(**probe_result) + return FakeDB(cl, [p, a]), a + + def test_reports_a_missing_stack(self): + db, _a = self._db({"return_value": []}) + self.assertEqual( + storage_node_ops._verify_replica_stacks("cluster-1", db), + [("A", "LVS_P", "P", "secondary")]) + + def test_clean_when_the_probe_finds_the_stack(self): + db, _a = self._db({"return_value": [{"name": "LVS_P"}]}) + self.assertEqual(storage_node_ops._verify_replica_stacks("cluster-1", db), []) + + def test_an_unreachable_node_is_not_reported_as_a_violation(self): + # Absence of proof is not proof of absence. A probe that cries wolf on + # a transient RPC error is a check people learn to ignore. + db, _a = self._db({"side_effect": RPCConnectionError("node unreachable")}) + self.assertEqual(storage_node_ops._verify_replica_stacks("cluster-1", db), []) + + def test_offline_nodes_are_not_probed(self): + cl = _cluster() + p = _node("P", lvstore="LVS_P", secondary_id="A") + a = _node("A", status=StorageNode.STATUS_OFFLINE, stack_secondary="P") + a.rpc_client.return_value.bdev_lvol_get_lvstores = MagicMock(return_value=[]) + db = FakeDB(cl, [p, a]) + self.assertEqual(storage_node_ops._verify_replica_stacks("cluster-1", db), []) + + +# --------------------------------------------------------------------------- +# jc_remove_jm — hand the JM back to JC before deleting its bdev. +# +# JC holds an open descriptor + IO channel on the JM bdev. Deleting the bdev +# first leaves JC naming something that no longer exists (observed live +# 2026-09-02: a peer's JC member list carried a remote_jm_* absent from that +# node's own bdev_get_bdevs). jc_remove_jm closes JC's side, and its -22 is +# positive proof that some jm_vuid still references the JM -- including one the +# control plane cannot enumerate, because a vuid whose primary was already +# removed appears in no `decisions` entry and under no back-reference. +# --------------------------------------------------------------------------- +class TestJcRemoveJmBeforeBdevDelete(unittest.TestCase): + + def _rpc(self, jc_remove_jm): + rpc = MagicMock() + rpc.jc_remove_jm = jc_remove_jm + rpc.jc_replace_jm = MagicMock(return_value=True) + rpc.bdev_nvme_detach_controller = MagicMock(return_value=True) + rpc.get_bdevs = MagicMock(return_value=[]) + return rpc + + def _run(self, jc_remove_jm, replica_peer_ids=("peer",)): + """Drive _decommission_node_jm with one peer that needs its JM replaced. + + ``replica_peer_ids`` defaults to naming the peer, because jc_remove_jm + is only issued for a node that carries the removed node's OWN lvstore + group -- its secondary or tertiary. Everywhere else a replace alone + already drops the JM from JC and the release would be a no-op. + """ + cl = _cluster() + removed = _node("dead", with_jm=True, jm_vuid=9, lvstore="LVS_9", failure_domain=1) + peer = _node("peer", with_jm=True, jm_vuid=37, lvstore="LVS_37", failure_domain=2) + spare = _node("spare", with_jm=True, jm_vuid=41, lvstore="LVS_41", failure_domain=3) + peer.jm_ids = ["jm-dead", "jm-peer"] + removed.jm_ids = ["jm-dead"] + spare.jm_ids = ["jm-spare"] + rd = RemoteJMDevice() + rd.uuid = "jm-dead" + rd.remote_bdev = "remote_jm_deadn1" + peer.remote_jm_devices = [rd] + rpc = self._rpc(jc_remove_jm) + peer.rpc_client = MagicMock(return_value=rpc) + + db = FakeDB(cl, [removed, peer, spare]) + db.get_jm_device_by_id = MagicMock( + side_effect=lambda i: {"jm-dead": removed.jm_device, + "jm-peer": peer.jm_device, + "jm-spare": spare.jm_device}.get(i)) + new_rd = RemoteJMDevice() + new_rd.uuid = "jm-spare" + new_rd.remote_bdev = "remote_jm_sparen1" + # Faithful to _connect_to_remote_jm_devs' delta mode: it carries the + # existing different-owner entries over untouched and adds the new one. + with patch.object(storage_node_ops, "DBController", return_value=db), \ + patch.object(storage_node_ops, "device_controller"), \ + patch.object(storage_node_ops, "get_sorted_ha_jms", return_value=["jm-spare"]), \ + patch.object(storage_node_ops, "_connect_to_remote_jm_devs", + return_value=[rd, new_rd]): + storage_node_ops._decommission_node_jm( + removed, replica_peer_ids=replica_peer_ids) + return rpc, peer + + def test_a_node_with_another_group_using_the_jm_replaces_and_never_removes(self): + # Mutually exclusive: this peer's own vuid 37 uses the dead JM, so the + # replace covers everything (leftover included) and remove is not + # called at all -- on the secondary/tertiary just as much as anywhere. + rpc, peer = self._run(jc_remove_jm=MagicMock(return_value=True)) + rpc.jc_replace_jm.assert_called_once() + rpc.jc_remove_jm.assert_not_called() + rpc.bdev_nvme_detach_controller.assert_called_once_with("remote_jm_dead") + self.assertNotIn("jm-dead", [rd.uuid for rd in peer.remote_jm_devices]) + + def test_same_holds_for_a_node_carrying_no_leftover_group(self): + rpc, peer = self._run(jc_remove_jm=MagicMock(return_value=True), + replica_peer_ids=()) + rpc.jc_remove_jm.assert_not_called() + rpc.bdev_nvme_detach_controller.assert_called_once_with("remote_jm_dead") + self.assertNotIn("jm-dead", [rd.uuid for rd in peer.remote_jm_devices]) + + +# --------------------------------------------------------------------------- +# Replace and remove are mutually exclusive per node (SPDK team, 2026-09-02): +# if any OTHER group on the node uses the dead JM, one jc_replace_jm covers +# them all plus the leftover; if the leftover group is the only user, it is +# jc_remove_jm alone. Secondary/tertiary matters only in that those are the +# nodes that carry a leftover group at all. +# --------------------------------------------------------------------------- +class TestLeftoverVuidOnReplicaPeers(unittest.TestCase): + + def _run(self, replica_peer_ids, jc_replace_jm=None, peer_jm_ids=None): + cl = _cluster() + removed = _node("dead", with_jm=True, jm_vuid=2, lvstore="LVS_2", failure_domain=1) + peer = _node("peer", with_jm=True, jm_vuid=37, lvstore="LVS_37", failure_domain=2) + spare = _node("spare", with_jm=True, jm_vuid=41, lvstore="LVS_41", failure_domain=3) + removed.jm_ids = ["jm-dead"] + peer.jm_ids = list(peer_jm_ids) if peer_jm_ids else ["jm-dead", "jm-peer"] + spare.jm_ids = ["jm-spare"] + rd = RemoteJMDevice() + rd.uuid = "jm-dead" + rd.remote_bdev = "remote_jm_deadn1" + peer.remote_jm_devices = [rd] + rpc = MagicMock() + rpc.jc_replace_jm = jc_replace_jm or MagicMock(return_value=True) + rpc.jc_remove_jm = MagicMock(return_value=True) + rpc.bdev_nvme_detach_controller = MagicMock(return_value=True) + rpc.get_bdevs = MagicMock(return_value=[]) + peer.rpc_client = MagicMock(return_value=rpc) + + db = FakeDB(cl, [removed, peer, spare]) + db.get_jm_device_by_id = MagicMock( + side_effect=lambda i: {"jm-dead": removed.jm_device, + "jm-peer": peer.jm_device, + "jm-spare": spare.jm_device}.get(i)) + new_rd = RemoteJMDevice() + new_rd.uuid = "jm-spare" + new_rd.remote_bdev = "remote_jm_sparen1" + with patch.object(storage_node_ops, "DBController", return_value=db), \ + patch.object(storage_node_ops, "device_controller"), \ + patch.object(storage_node_ops, "get_sorted_ha_jms", return_value=["jm-spare"]), \ + patch.object(storage_node_ops, "_connect_to_remote_jm_devs", + return_value=[rd, new_rd]): + storage_node_ops._decommission_node_jm( + removed, replica_peer_ids=replica_peer_ids) + return rpc + + def test_the_leftover_is_never_a_replace_target(self): + # The removed primary's lvstore is being destroyed, so its group gets + # no replacement member -- only the peer's own surviving vuid 37 is in + # the batch, even though this peer IS the secondary/tertiary. + rpc = self._run(("peer",)) + rpc.jc_replace_jm.assert_called_once() + kw = rpc.jc_replace_jm.call_args.kwargs + vuids = sorted(r["jm_vuid"] for r in kw["replacements"]) + self.assertEqual(vuids, [37]) + self.assertEqual(kw["name_old"], "remote_jm_deadn1") + + def test_and_then_remove_is_not_called_at_all(self): + # Mutually exclusive: the replace already covered every user, so JC has + # dropped the JM and a release would be a guaranteed no-op. + rpc = self._run(("peer",)) + rpc.jc_remove_jm.assert_not_called() + rpc.bdev_nvme_detach_controller.assert_called_once_with("remote_jm_dead") + + def test_a_node_with_no_leftover_group_covers_only_its_own_vuids(self): + rpc = self._run(("someone-else",)) + vuids = sorted(r["jm_vuid"] for r in rpc.jc_replace_jm.call_args.kwargs["replacements"]) + self.assertEqual(vuids, [37]) + rpc.jc_remove_jm.assert_not_called() + rpc.bdev_nvme_detach_controller.assert_called_once_with("remote_jm_dead") + + def test_removed_node_whose_own_jm_ids_lack_the_dead_jm_does_not_abort_phase_2(self): + # Regression, found live 2026-09-02. The removed node is itself in + # live_nodes (status in_removal), so if the leftover replacement is + # stored in `decisions` it becomes a normal Pass-2 consumer -- and the + # unguarded node.jm_ids.remove(removed_jm_id) then raised ValueError, + # aborting phase 2 before ANY peer was patched. Strictly worse than the + # gap it was meant to close. + cl = _cluster() + removed = _node("dead", with_jm=True, jm_vuid=2, lvstore="LVS_2", failure_domain=1) + peer = _node("peer", with_jm=True, jm_vuid=37, lvstore="LVS_37", failure_domain=2) + spare = _node("spare", with_jm=True, jm_vuid=41, lvstore="LVS_41", failure_domain=3) + # The removed node's OWN jm_ids does NOT list its own dying JM. + removed.jm_ids = ["jm-other-a", "jm-other-b"] + removed.status = StorageNode.STATUS_IN_REMOVAL + peer.jm_ids = ["jm-dead", "jm-peer"] + spare.jm_ids = ["jm-spare"] + rd = RemoteJMDevice() + rd.uuid = "jm-dead" + rd.remote_bdev = "remote_jm_deadn1" + peer.remote_jm_devices = [rd] + rpc = MagicMock() + rpc.jc_replace_jm = MagicMock(return_value=True) + rpc.jc_remove_jm = MagicMock(return_value=True) + rpc.bdev_nvme_detach_controller = MagicMock(return_value=True) + rpc.get_bdevs = MagicMock(return_value=[]) + peer.rpc_client = MagicMock(return_value=rpc) + + db = FakeDB(cl, [removed, peer, spare]) + db.get_jm_device_by_id = MagicMock( + side_effect=lambda i: {"jm-dead": removed.jm_device, + "jm-peer": peer.jm_device, + "jm-spare": spare.jm_device}.get(i)) + new_rd = RemoteJMDevice() + new_rd.uuid = "jm-spare" + new_rd.remote_bdev = "remote_jm_sparen1" + with patch.object(storage_node_ops, "DBController", return_value=db), \ + patch.object(storage_node_ops, "device_controller"), \ + patch.object(storage_node_ops, "get_sorted_ha_jms", return_value=["jm-spare"]), \ + patch.object(storage_node_ops, "_connect_to_remote_jm_devs", + return_value=[rd, new_rd]): + # Must not raise. + storage_node_ops._decommission_node_jm(removed, replica_peer_ids=("peer",)) + + # And the peer must still have been patched for its surviving group. + # The leftover is not a replace target. + rpc.jc_replace_jm.assert_called_once() + vuids = sorted(r["jm_vuid"] for r in rpc.jc_replace_jm.call_args.kwargs["replacements"]) + self.assertEqual(vuids, [37]) + rpc.jc_remove_jm.assert_not_called() + self.assertEqual(removed.jm_ids, ["jm-other-a", "jm-other-b"], + "the removed node's unrelated jm_ids must be left alone") + + +class TestOrchestratorCapturesReplicaPeersBeforeTeardown(unittest.TestCase): + + def test_peer_ids_are_captured_before_phase_3a_clears_them(self): + # phase 3a clears snode.secondary_node_id/_tertiary_node_id, so the + # capture has to happen first or phase 2 gets an empty tuple. + cl = _cluster() + snode = _node("dead", secondary_id="sec", tertiary_id="ter", + with_jm=True, jm_vuid=2, lvstore="LVS_2") + sec = _node("sec", stack_secondary="dead") + ter = _node("ter", stack_tertiary="dead") + db = FakeDB(cl, [snode, sec, ter]) + seen = {} + + def fake_teardown(node): + # emulate phase 3a wiping the pointers + snode.secondary_node_id = "" + snode.tertiary_node_id = "" + return True + + with patch.object(storage_node_ops, "DBController", return_value=db), \ + patch.object(storage_node_ops, "cluster_ops"), \ + patch.object(storage_node_ops, "shutdown_storage_node", return_value=True), \ + patch.object(storage_node_ops, "set_node_status"), \ + patch.object(storage_node_ops, "_teardown_replicas_of_primary", + side_effect=fake_teardown), \ + patch.object(storage_node_ops, "_decommission_node_jm", + side_effect=lambda n, replica_peer_ids=(): seen.update( + ids=replica_peer_ids)), \ + patch.object(storage_node_ops, "_relocate_replicas_hosted_on", return_value=True), \ + patch.object(storage_node_ops, "_verify_replica_stacks", return_value=[]), \ + patch.object(storage_node_ops, "_finalize_node_removal"), \ + patch.object(storage_node_ops, "_decommission_node_devices", return_value=True): + storage_node_ops.node_removal_orchestrate("dead") + + self.assertEqual(sorted(seen.get("ids", ())), ["sec", "ter"]) + + +# --------------------------------------------------------------------------- +# The no-targets branch: a node that has the dying JM connected but no vuid +# this removal can patch. Until now it fell straight through -- no release, no +# delete -- while the jc_remove_jm below the replace ran only where a replace +# had already made it a no-op (-13). Redundant where it fired, absent where it +# mattered. +# --------------------------------------------------------------------------- +class TestJcRemoveJmOnNodeWithNoTargets(unittest.TestCase): + + def _run(self, jc_remove_jm): + cl = _cluster() + removed = _node("dead", with_jm=True, jm_vuid=2, lvstore="LVS_2", failure_domain=1) + # peer holds the dying JM but NONE of its vuids reference it, so Pass 2 + # produces no targets for it. + peer = _node("peer", with_jm=True, jm_vuid=37, lvstore="LVS_37", failure_domain=2) + removed.jm_ids = ["jm-dead"] + peer.jm_ids = ["jm-peer"] + rd = RemoteJMDevice() + rd.uuid = "jm-dead" + rd.remote_bdev = "remote_jm_deadn1" + peer.remote_jm_devices = [rd] + rpc = MagicMock() + rpc.jc_remove_jm = jc_remove_jm + rpc.jc_replace_jm = MagicMock(return_value=True) + rpc.bdev_nvme_detach_controller = MagicMock(return_value=True) + peer.rpc_client = MagicMock(return_value=rpc) + db = FakeDB(cl, [removed, peer]) + db.get_jm_device_by_id = MagicMock( + side_effect=lambda i: {"jm-dead": removed.jm_device, + "jm-peer": peer.jm_device}.get(i)) + with patch.object(storage_node_ops, "DBController", return_value=db), \ + patch.object(storage_node_ops, "device_controller"), \ + patch.object(storage_node_ops, "get_sorted_ha_jms", return_value=[]), \ + patch.object(storage_node_ops, "_connect_to_remote_jm_devs", return_value=[]): + storage_node_ops._decommission_node_jm(removed, replica_peer_ids=("peer",)) + return rpc, peer + + def test_release_is_attempted_even_though_no_replace_happened(self): + rpc, _peer = self._run(jc_remove_jm=MagicMock(return_value=True)) + rpc.jc_replace_jm.assert_not_called() + rpc.jc_remove_jm.assert_called_once_with("remote_jm_deadn1") + + def test_successful_release_deletes_the_bdev_and_drops_bookkeeping(self): + rpc, peer = self._run(jc_remove_jm=MagicMock(return_value=True)) + rpc.bdev_nvme_detach_controller.assert_called_once_with("remote_jm_dead") + self.assertNotIn("jm-dead", [rd.uuid for rd in peer.remote_jm_devices]) + + def test_minus_22_here_keeps_the_bdev(self): + # The leftover vuid genuinely still references it -- exactly the case + # this branch exists to surface. + rpc, peer = self._run( + jc_remove_jm=MagicMock(side_effect=RPCRemoteError("in use", -22))) + rpc.bdev_nvme_detach_controller.assert_not_called() + self.assertIn("jm-dead", [rd.uuid for rd in peer.remote_jm_devices]) + + def test_unsupported_build_still_cleans_up(self): + from simplyblock_core.rpc_client import RPC_UNSUPPORTED + rpc, peer = self._run(jc_remove_jm=MagicMock(return_value=RPC_UNSUPPORTED)) + rpc.bdev_nvme_detach_controller.assert_called_once_with("remote_jm_dead") + self.assertNotIn("jm-dead", [rd.uuid for rd in peer.remote_jm_devices]) + + def test_minus_13_not_used_by_jc_is_the_success_path(self): + # JC already has no record of the JM, so there is nothing to release + # and the bdev is safe to delete. Treating it as a failure would strand + # the controller and its bookkeeping entry. + rpc, peer = self._run( + jc_remove_jm=MagicMock(side_effect=RPCRemoteError("not used by JC", -13))) + rpc.bdev_nvme_detach_controller.assert_called_once_with("remote_jm_dead") + self.assertNotIn("jm-dead", [rd.uuid for rd in peer.remote_jm_devices]) + + def test_other_jc_error_codes_keep_the_bdev(self): + for code in (-3, -6, -10, -12, -21): + with self.subTest(code=code): + rpc, peer = self._run( + jc_remove_jm=MagicMock(side_effect=RPCRemoteError("nope", code))) + rpc.bdev_nvme_detach_controller.assert_not_called() + self.assertIn("jm-dead", [rd.uuid for rd in peer.remote_jm_devices]) + + def test_a_raised_exception_keeps_the_bdev(self): + rpc, peer = self._run( + jc_remove_jm=MagicMock(side_effect=RPCConnectionError("unreachable"))) + rpc.bdev_nvme_detach_controller.assert_not_called() + self.assertIn("jm-dead", [rd.uuid for rd in peer.remote_jm_devices]) + + def test_node_without_the_dying_jm_is_left_completely_alone(self): + cl = _cluster() + removed = _node("dead", with_jm=True, jm_vuid=2, lvstore="LVS_2") + peer = _node("peer", with_jm=True, jm_vuid=37, lvstore="LVS_37") + removed.jm_ids = ["jm-dead"] + peer.jm_ids = ["jm-peer"] + peer.remote_jm_devices = [] # never had it + rpc = MagicMock() + peer.rpc_client = MagicMock(return_value=rpc) + db = FakeDB(cl, [removed, peer]) + db.get_jm_device_by_id = MagicMock( + side_effect=lambda i: {"jm-dead": removed.jm_device}.get(i)) + with patch.object(storage_node_ops, "DBController", return_value=db), \ + patch.object(storage_node_ops, "device_controller"), \ + patch.object(storage_node_ops, "get_sorted_ha_jms", return_value=[]), \ + patch.object(storage_node_ops, "_connect_to_remote_jm_devs", return_value=[]): + storage_node_ops._decommission_node_jm(removed, replica_peer_ids=("peer",)) + rpc.jc_remove_jm.assert_not_called() + rpc.bdev_nvme_detach_controller.assert_not_called() + + +class TestJcRemoveJmClient(unittest.TestCase): + """The client wrapper's contract: success / unsupported / coded error.""" + + def _client(self, response): + from simplyblock_core import rpc_client as rc + c = rc.RPCClient.__new__(rc.RPCClient) + c._request2 = MagicMock(return_value=response) # type: ignore[method-assign] + return c + + def test_success_returns_the_result(self): + self.assertTrue(self._client((True, None)).jc_remove_jm("remote_jm_xn1")) + + def test_method_not_found_returns_the_unsupported_sentinel(self): + from simplyblock_core.rpc_client import RPC_UNSUPPORTED + c = self._client((None, {"code": -32601, "message": "Method not found"})) + self.assertEqual(c.jc_remove_jm("remote_jm_xn1"), RPC_UNSUPPORTED) + + def test_still_in_use_raises_with_code_22(self): + c = self._client((None, {"code": -22, "message": "still in use"})) + with self.assertRaises(RPCRemoteError) as ctx: + c.jc_remove_jm("remote_jm_xn1") + self.assertEqual(ctx.exception.code, -22) + + def test_other_errors_raise_with_their_code(self): + c = self._client((None, {"code": -12, "message": "another removal in progress"})) + with self.assertRaises(RPCRemoteError) as ctx: + c.jc_remove_jm("remote_jm_xn1") + self.assertEqual(ctx.exception.code, -12) + + +class TestDecommissionSkipsTheNodeBeingRemoved(unittest.TestCase): + """The node being removed must never become a jc_* patch target itself. + + Regression for live 2026-09-03, removing s25dl. ``live_nodes`` filtered + STATUS_REMOVED, but at phase 2 the node is IN_REMOVAL, so it stayed in the + sweep. Phase 3b had not yet relocated the replicas it hosts *for other + primaries*, so its lvstore_stack_* backrefs still pointed at live primaries + and Pass 2 adopted it as a target for a hosted primary's vuid: + + no recorded bdev name for removed JM 601dae11...; cannot call + jc_replace_jm -- affected targets=[(1, 'a91a2d46...')] + + It stayed harmless only by accident: a node's OWN JM never appears in its + own remote_jm_devices, so the name lookup failed and short-circuited the + call. With a name recorded it would have issued jc_replace_jm against the + pod phase 1 had already killed -- exactly what the REMOVED filter exists + to prevent. + """ + + def _run(self): + cl = _cluster() + # 'dead' is mid-removal and still hosts 'other''s replica: phase 3a + # clears a node's own secondary/tertiary pointers, not the backrefs + # recording what it hosts. Phase 3b does that, and runs after phase 2. + removed = _node("dead", status=StorageNode.STATUS_IN_REMOVAL, + with_jm=True, jm_vuid=9, lvstore="LVS_9", + failure_domain=1, stack_secondary="other") + other = _node("other", with_jm=True, jm_vuid=1, lvstore="LVS_1", + failure_domain=2) + spare = _node("spare", with_jm=True, jm_vuid=41, lvstore="LVS_41", + failure_domain=3) + other.jm_ids = ["jm-dead", "jm-other"] + removed.jm_ids = ["jm-dead"] + spare.jm_ids = ["jm-spare"] + + rd = RemoteJMDevice() + rd.uuid, rd.remote_bdev = "jm-dead", "remote_jm_deadn1" + other.remote_jm_devices = [rd] + # The removed node carries no remote entry for its OWN jm -- the shape + # that made the live failure log an error instead of issuing an RPC. + removed.remote_jm_devices = [] + + dead_rpc = MagicMock() + other_rpc = MagicMock() + for r in (dead_rpc, other_rpc): + r.jc_replace_jm = MagicMock(return_value=True) + r.jc_remove_jm = MagicMock(return_value=True) + r.bdev_nvme_detach_controller = MagicMock(return_value=True) + r.get_bdevs = MagicMock(return_value=[]) + removed.rpc_client = MagicMock(return_value=dead_rpc) + other.rpc_client = MagicMock(return_value=other_rpc) + + db = FakeDB(cl, [removed, other, spare]) + db.get_jm_device_by_id = MagicMock( + side_effect=lambda i: {"jm-dead": removed.jm_device, + "jm-other": other.jm_device, + "jm-spare": spare.jm_device}.get(i)) + new_rd = RemoteJMDevice() + new_rd.uuid, new_rd.remote_bdev = "jm-spare", "remote_jm_sparen1" + + # Capture the module's log records: the live symptom was an ERROR, not + # a stray RPC. Sweeping the removed node only failed to issue one + # because a node's own JM is absent from its own remote_jm_devices, so + # asserting "no RPC" alone passes with the bug still in place. + records = [] + + class _Capture(logging.Handler): + def emit(self, record): + records.append(record) + + cap = _Capture() + storage_node_ops.logger.addHandler(cap) + try: + with patch.object(storage_node_ops, "DBController", return_value=db), \ + patch.object(storage_node_ops, "device_controller"), \ + patch.object(storage_node_ops, "get_sorted_ha_jms", + return_value=["jm-spare"]), \ + patch.object(storage_node_ops, "_connect_to_remote_jm_devs", + return_value=[new_rd]): + storage_node_ops._decommission_node_jm(removed, replica_peer_ids=()) + finally: + storage_node_ops.logger.removeHandler(cap) + return dead_rpc, other_rpc, records + + def test_the_node_being_removed_is_never_adopted_as_a_target(self): + dead_rpc, _, records = self._run() + errors = [r.getMessage() for r in records if r.levelno >= logging.ERROR] + self.assertEqual(errors, [], "phase 2 tried to patch the node it is removing") + dead_rpc.jc_replace_jm.assert_not_called() + dead_rpc.jc_remove_jm.assert_not_called() + + def test_a_surviving_consumer_is_still_patched(self): + # The exclusion must not cost the live peer its replacement. + _, other_rpc, _ = self._run() + other_rpc.jc_replace_jm.assert_called_once() + + +class TestCheckPeerDisconnectedMgmtStatus(unittest.TestCase): + """The mgmt-status short-circuit of _check_peer_disconnected. + + Regression for live 2026-09-03: while removing 2vk79, its peer hxmr8 was + rebuilding as the tertiary of another primary (s7457/LVS_21) and picked + its deferred hublvol failover target by asking whether LVS_21's secondary + was alive. That secondary *was* 2vk79 — already shut down by phase 1 and + in IN_REMOVAL — but IN_REMOVAL was missing from the short-circuit, so the + check fell through to the JM-quorum path, which abstained ("0/0 peers + report disconnected") and voted "connected". The attach then failed with + -5 against a dead SPDK: + + Failed to add deferred hublvol failover path to ddbf964e... for LVS_21 + + The verdict has to come from the control plane's own intent, not from a + data-plane probe that cannot tell "gone" from "nobody voted". + """ + + def _probe(self, peer_status, quorum_verdict=False): + """Run the check against a single peer, returning (verdict, probed). + + The peer id embeds the status so that cases sharing one test method + (subTest loops) get distinct quorum-cache keys — the cache is cleared + per test, not per subTest, so a reused id would serve the first case's + cached verdict to the second and leave its mock uncalled. + """ + peer = _node(f"PEER-{peer_status}", status=peer_status) + db = FakeDB(_cluster(), [peer]) + quorum = MagicMock(return_value=quorum_verdict) + with patch.object(storage_node_ops, "DBController", return_value=db), \ + patch("simplyblock_core.services.storage_node_monitor" + ".is_node_data_plane_disconnected_quorum", quorum): + verdict = storage_node_ops._check_peer_disconnected(peer) + return verdict, quorum.called + + def test_in_removal_is_disconnected_without_probing_the_data_plane(self): + # The exact bug: quorum would have voted "connected" (False). + verdict, probed = self._probe(StorageNode.STATUS_IN_REMOVAL, + quorum_verdict=False) + self.assertTrue(verdict) + self.assertFalse(probed, "IN_REMOVAL must not reach the quorum probe") + + def test_the_states_mgmt_has_given_up_on_all_short_circuit(self): + for status in (StorageNode.STATUS_OFFLINE, + StorageNode.STATUS_REMOVED, + StorageNode.STATUS_UNREACHABLE, + StorageNode.STATUS_IN_REMOVAL): + with self.subTest(status=status): + verdict, probed = self._probe(status, quorum_verdict=False) + self.assertTrue(verdict) + self.assertFalse(probed) + + def test_pending_removal_still_consults_the_data_plane(self): + # Deliberately NOT short-circuited: the task runner sets + # PENDING_REMOVAL before phase 1 shuts the node down, so it is still + # up and serving and still needs its port-block. + verdict, probed = self._probe(StorageNode.STATUS_PENDING_REMOVAL, + quorum_verdict=False) + self.assertFalse(verdict) + self.assertTrue(probed, "PENDING_REMOVAL must fall through to the probe") + + def test_transient_runner_owned_states_still_consult_the_data_plane(self): + # Preempting another node's leadership during its own restart would + # be incorrect, so these keep asking the data plane. + for status in (StorageNode.STATUS_IN_SHUTDOWN, + StorageNode.STATUS_RESTARTING): + with self.subTest(status=status): + verdict, probed = self._probe(status, quorum_verdict=False) + self.assertFalse(verdict) + self.assertTrue(probed) + + def test_the_data_plane_can_still_condemn_an_online_peer(self): + verdict, probed = self._probe(StorageNode.STATUS_ONLINE, + quorum_verdict=True) + self.assertTrue(verdict) + self.assertTrue(probed) + + def test_a_peer_gone_from_fdb_is_disconnected(self): + db = FakeDB(_cluster(), []) + quorum = MagicMock(return_value=False) + with patch.object(storage_node_ops, "DBController", return_value=db), \ + patch("simplyblock_core.services.storage_node_monitor" + ".is_node_data_plane_disconnected_quorum", quorum): + self.assertTrue(storage_node_ops._check_peer_disconnected( + _node("PEER"))) + self.assertFalse(quorum.called) + + def test_a_node_in_removal_is_not_fabric_connected(self): + # _is_fabric_connected / _count_fabric_disconnected_nodes are the two + # wrappers callers use to pick targets; both must inherit the fix. + peer = _node("PEER", status=StorageNode.STATUS_IN_REMOVAL) + db = FakeDB(_cluster(), [peer]) + with patch.object(storage_node_ops, "DBController", return_value=db): + self.assertFalse(storage_node_ops._is_fabric_connected(peer)) + self.assertEqual( + storage_node_ops._count_fabric_disconnected_nodes([peer]), 1) + + if __name__ == "__main__": unittest.main() + + +class TestRelocationPlannerWithoutFailureDomains(unittest.TestCase): + """The global planner must also drive removals on clusters with failure + domains OFF. + + Regression for a live refusal (2026-09-08, 6-node 2x2 cluster, FD + disabled): with every survivor already at capacity the greedy per-role + fallback consumed the one free slot the second stranded replica needed + and refused the removal, even though a valid host-disjoint layout + existed. The planner solves both placements together, so it does not. + """ + + def _dense_nofd_cluster(self): + """5 nodes, ftt=2, FD off, every node hosting primary+secondary+tertiary. + + Layout mirrors the cluster that hit the refusal: + LVS_1 (a): sec=b ter=d + LVS_4 (b): sec=c ter=e + LVS_7 (d): sec=e ter=c + LVS_10(c): sec=a ter=b + LVS_13(e): sec=d ter=a + Removing ``e`` strands LVS_4's tertiary and LVS_7's secondary. + """ + cl = _cluster(ft=2, enable_failure_domain=False) + a = _node("a", lvstore="LVS_1", secondary_id="b", tertiary_id="d", mgmt_ip="10.0.0.1") + b = _node("b", lvstore="LVS_4", secondary_id="c", tertiary_id="e", mgmt_ip="10.0.0.2") + c = _node("c", lvstore="LVS_10", secondary_id="a", tertiary_id="b", mgmt_ip="10.0.0.3") + d = _node("d", lvstore="LVS_7", secondary_id="e", tertiary_id="c", mgmt_ip="10.0.0.4") + # Back-references: e is LVS_7's secondary and LVS_4's tertiary. These + # are what the greedy fallback keys off, so they must be set for the + # fallback path to engage at all. + e = _node("e", lvstore="LVS_13", secondary_id="d", tertiary_id="a", + stack_secondary="d", stack_tertiary="b", mgmt_ip="10.0.0.5") + return cl, [a, b, c, d, e], e + + def test_planner_applies_when_failure_domains_are_off(self): + cl, nodes, removed = self._dense_nofd_cluster() + db = FakeDB(cl, nodes) + inputs = storage_node_ops._relocation_planner_inputs( + removed, db, allow_without_fd=True) + self.assertIsNotNone( + inputs, "planner must accept an FD-off cluster when asked for a second opinion") + surviving_ids, fd_by_node, host_by_node, _label, _layout, ftt = inputs + self.assertEqual(sorted(surviving_ids), ["a", "b", "c", "d"]) + self.assertEqual(ftt, 2) + # one pseudo-domain per distinct host + self.assertEqual(len(set(fd_by_node.values())), len(set(host_by_node.values()))) + + def test_pseudo_domains_are_per_host_not_per_node(self): + """Two storage nodes on one host share a pseudo-domain, so the planner + will not place two roles of one LVS on that single host.""" + cl = _cluster(ft=2, enable_failure_domain=False) + # a1 and a2 share a host; b, c, d are distinct hosts + a1 = _node("a1", lvstore="L1", secondary_id="b", tertiary_id="c", mgmt_ip="10.0.0.1") + a2 = _node("a2", lvstore="L2", secondary_id="c", tertiary_id="d", mgmt_ip="10.0.0.1") + b = _node("b", lvstore="L3", secondary_id="d", tertiary_id="a1", mgmt_ip="10.0.0.2") + c = _node("c", lvstore="L4", secondary_id="a1", tertiary_id="a2", mgmt_ip="10.0.0.3") + d = _node("d", lvstore="L5", secondary_id="a2", tertiary_id="b", mgmt_ip="10.0.0.4") + db = FakeDB(cl, [a1, a2, b, c, d]) + inputs = storage_node_ops._relocation_planner_inputs(d, db, allow_without_fd=True) + self.assertIsNotNone(inputs) + _ids, fd_by_node, _host, _label, _layout, _ftt = inputs + self.assertEqual(fd_by_node["a1"], fd_by_node["a2"]) + self.assertNotEqual(fd_by_node["a1"], fd_by_node["b"]) + + def test_dense_nofd_removal_is_admitted(self): + """The refusal this regresses: a valid layout exists, so admission + must not reject the removal.""" + cl, nodes, removed = self._dense_nofd_cluster() + db = FakeDB(cl, nodes) + # get_secondary_nodes() builds its own DBController, so the greedy + # probe needs it patched to see this fixture. + with patch.object(storage_node_ops, "DBController", return_value=db): + feasible, reason = storage_node_ops._check_replica_relocation_feasible( + removed, db) + self.assertTrue(feasible, f"removal wrongly refused: {reason}") + self.assertEqual(reason, "") + + def test_still_declines_when_domains_on_but_unset(self): + """FD enabled with a partial domain map is still not plannable.""" + cl = _cluster(ft=1, enable_failure_domain=True) + a = _node("a", lvstore="L1", secondary_id="b", failure_domain=0, mgmt_ip="10.0.0.1") + b = _node("b", lvstore="L2", secondary_id="a", failure_domain=-1, mgmt_ip="10.0.0.2") + removed = _node("z", lvstore="L3", failure_domain=1, mgmt_ip="10.0.0.9") + db = FakeDB(cl, [a, b, removed]) + self.assertIsNone(storage_node_ops._relocation_planner_inputs(removed, db)) + + +# --------------------------------------------------------------------------- +# node_removal_orchestrate — prove phase 3b is planable before phase 3a runs +# +# 3a is irreversible and 3b only discovers whether a layout exists when it +# runs, after 3a. A 3b failure returns False into a runner that retries the +# whole sequence, and the retry re-enters a 3a with nothing to tear down and +# hits the same 3b in the same state. Live on 2026-09-09: 68 retries over 11 +# minutes, node stuck in in_removal, one lvstore left on a single member the +# whole time. +# --------------------------------------------------------------------------- + +class TestOrchestrateChecksRelocationBeforeTeardown(unittest.TestCase): + + def _patch_all(self): + return patch.multiple( + storage_node_ops, + DBController=DEFAULT, + cluster_ops=DEFAULT, + shutdown_storage_node=DEFAULT, + _check_replica_relocation_feasible=DEFAULT, + _decommission_node_jm=DEFAULT, + _teardown_replicas_of_primary=DEFAULT, + _relocate_replicas_hosted_on=DEFAULT, + _verify_replica_stacks=DEFAULT, + _finalize_node_removal=DEFAULT, + set_node_status=DEFAULT, + _decommission_node_devices=DEFAULT, + ) + + def _db(self): + cl = _cluster() + node = _node("n1", status=StorageNode.STATUS_IN_REMOVAL, + secondary_id="p1", tertiary_id="p2") + return FakeDB(cl, [node, _node("p1"), _node("p2")]), node + + def test_refuses_before_teardown_when_no_layout_exists(self): + db, _node_obj = self._db() + with self._patch_all() as mocks: + mocks["DBController"].return_value = db + mocks["shutdown_storage_node"].return_value = True + mocks["_check_replica_relocation_feasible"].return_value = (False, "no host") + ret = storage_node_ops.node_removal_orchestrate("n1") + + self.assertFalse(ret) + # nothing destructive may have run + mocks["_teardown_replicas_of_primary"].assert_not_called() + mocks["_decommission_node_jm"].assert_not_called() + mocks["_relocate_replicas_hosted_on"].assert_not_called() + mocks["_finalize_node_removal"].assert_not_called() + + def test_proceeds_through_teardown_when_a_layout_exists(self): + db, _node_obj = self._db() + with self._patch_all() as mocks: + mocks["DBController"].return_value = db + mocks["shutdown_storage_node"].return_value = True + mocks["_check_replica_relocation_feasible"].return_value = (True, "") + mocks["_teardown_replicas_of_primary"].return_value = True + mocks["_relocate_replicas_hosted_on"].return_value = True + mocks["_decommission_node_devices"].return_value = True + storage_node_ops.node_removal_orchestrate("n1") + + mocks["_teardown_replicas_of_primary"].assert_called_once() + mocks["_relocate_replicas_hosted_on"].assert_called_once() + + def test_the_check_runs_before_the_teardown_not_after(self): + """Ordering is the whole point: a check that runs after 3a is useless.""" + order = [] + db, _node_obj = self._db() + with self._patch_all() as mocks: + mocks["DBController"].return_value = db + mocks["shutdown_storage_node"].return_value = True + mocks["_check_replica_relocation_feasible"].side_effect = \ + lambda *a, **k: (order.append("check"), (True, ""))[1] + mocks["_teardown_replicas_of_primary"].side_effect = \ + lambda *a, **k: (order.append("teardown"), True)[1] + mocks["_relocate_replicas_hosted_on"].return_value = True + mocks["_decommission_node_devices"].return_value = True + storage_node_ops.node_removal_orchestrate("n1") + + self.assertEqual(order[:2], ["check", "teardown"], + "the relocation check must precede the irreversible teardown") diff --git a/tests/unit/test_remote_device_probe_gate.py b/tests/unit/test_remote_device_probe_gate.py new file mode 100644 index 0000000000..d04d6c8250 --- /dev/null +++ b/tests/unit/test_remote_device_probe_gate.py @@ -0,0 +1,94 @@ +# coding=utf-8 +""" +``health_controller.check_remote_device`` must not probe for a device whose +owning node has departed. + +Found live 2026-09-03. The function gated only the health *verdict*, never the +probe. The caller at health_controller.py:761 discards the result when the +owning node is gone, but it calls this 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_' does not exist``, measured at 3-15 +errors/min and still climbing 35 minutes after the removal that made those +devices failed_and_migrated (devices 04fce724 / b0ada39d / ddf660f5 of the +removed node 2vk79, probed by 9 surviving nodes). Real faults then drown in a +permanent error stream. + +The remote-JM loop in the same file already skips the RPC for an irrelevant +owner; this is the same rule applied to the device path. +""" + +import unittest +from unittest.mock import MagicMock, patch + +from simplyblock_core.controllers import health_controller +from simplyblock_core.models.storage_node import StorageNode + + +def _node(node_id, status=StorageNode.STATUS_ONLINE): + n = MagicMock(spec=StorageNode) + n.uuid = node_id + n.get_id = MagicMock(return_value=node_id) + n.status = status + n.cluster_id = "c1" + return n + + +class TestRemoteDeviceProbeSkipsDepartedOwners(unittest.TestCase): + + def _run(self, owner_status): + owner = _node("owner", owner_status) + prober = _node("prober", StorageNode.STATUS_ONLINE) + rpc = MagicMock() + rpc.get_bdevs = MagicMock(return_value=[{"name": "x"}]) + rpc.bdev_nvme_controller_list = MagicMock(return_value=[]) + prober.rpc_client = MagicMock(return_value=rpc) + + device = MagicMock() + device.node_id = "owner" + device.alceml_bdev = "alceml_d1" + device.nvmf_multipath = False + + db = MagicMock() + db.get_storage_device_by_id = MagicMock(return_value=device) + db.get_storage_node_by_id = MagicMock(return_value=owner) + db.get_storage_nodes_by_cluster_id = MagicMock( + return_value=[owner, prober]) + with patch.object(health_controller, "DBController", return_value=db): + result = health_controller.check_remote_device("d1") + return result, rpc + + def test_no_rpc_is_issued_for_a_removed_owner(self): + # Asserting on the RPCs, not 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. + result, rpc = self._run(StorageNode.STATUS_REMOVED) + rpc.get_bdevs.assert_not_called() + rpc.bdev_nvme_controller_list.assert_not_called() + self.assertTrue(result, "a departed owner must not fail health") + + def test_no_rpc_for_other_departed_states(self): + for status in (StorageNode.STATUS_OFFLINE, + StorageNode.STATUS_IN_REMOVAL, + StorageNode.STATUS_RESTARTING): + with self.subTest(status=status): + _result, rpc = self._run(status) + rpc.get_bdevs.assert_not_called() + rpc.bdev_nvme_controller_list.assert_not_called() + + def test_a_live_owner_is_still_probed(self): + # _peer_connections_relevant: ONLINE / DOWN / UNREACHABLE are the + # states where the connection is genuinely expected to exist. + for status in (StorageNode.STATUS_ONLINE, + StorageNode.STATUS_DOWN, + StorageNode.STATUS_UNREACHABLE): + with self.subTest(status=status): + _result, rpc = self._run(status) + rpc.get_bdevs.assert_called_once_with("remote_alceml_d1n1") + rpc.bdev_nvme_controller_list.assert_called_once_with( + "remote_alceml_d1") + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/unit/test_replica_placement.py b/tests/unit/test_replica_placement.py new file mode 100644 index 0000000000..dbc3a6d260 --- /dev/null +++ b/tests/unit/test_replica_placement.py @@ -0,0 +1,511 @@ +# coding=utf-8 +""" +Unit tests for the global replica placement planner +(``simplyblock_core.controllers.replica_placement``). + +The planner replaces the per-replica greedy relocation used by node removal +under failure domains. What is asserted here: + + * the matching itself (optimality, forbidden-edge rejection); + * full pairwise domain diversity is reached whenever it is mathematically + reachable -- including the reported 4-domain x 3-host cluster shrunk one + host per domain, which the greedy path could not hold; + * when it is NOT reachable, that is reported rather than silently relaxed; + * the planned moves are minimal, and ordered so each one lands on a slot + that is genuinely free at that point -- including the rotation cycles + that a per-replica mover cannot execute at all. + +Pure logic: no DB, no RPC, no mocks. +""" + +import itertools +import random +import unittest + +from simplyblock_core.controllers import replica_placement as rp +from simplyblock_core.controllers.replica_placement import ( + InfeasiblePlacement, Placement) + + +# --------------------------------------------------------------------------- +# Helpers +# --------------------------------------------------------------------------- + +def _grid(domains, per_domain): + """``domains`` x ``per_domain`` nodes named ``dn``.""" + nodes = [f"d{d}n{i}" for d in range(domains) for i in range(per_domain)] + fd = {n: int(n[1:].split("n")[0]) for n in nodes} + return nodes, fd + + +def _rotation(nodes, fd, ftt): + """The FD-interleaved rotation cluster_activate produces: round-robin the + domains, then take the next / next-next node as secondary / tertiary.""" + by_fd = {} + for n in nodes: + by_fd.setdefault(fd[n], []).append(n) + order = [] + for idx in range(max(len(v) for v in by_fd.values())): + for d in sorted(by_fd): + if idx < len(by_fd[d]): + order.append(by_fd[d][idx]) + size = len(order) + return { + p: Placement(order[(k + 1) % size], + order[(k + 2) % size] if ftt >= 2 else "") + for k, p in enumerate(order) + } + + +def _apply(current, moves, ftt): + """Replay ``moves`` against ``current``, asserting each one lands on a + slot that is free at that moment -- the property that makes the plan + executable by a mover with single-valued back-reference fields.""" + state = dict(current) + slots = {} + for primary, placement in current.items(): + if placement.secondary: + slots[(rp.ROLE_SECONDARY, placement.secondary)] = primary + if ftt >= 2 and placement.tertiary: + slots[(rp.ROLE_TERTIARY, placement.tertiary)] = primary + for move in moves: + key = (move.role, move.to_node_id) + assert key not in slots, ( + f"{move} lands on a slot already held by {slots[key]}") + if move.from_node_id: + slots.pop((move.role, move.from_node_id), None) + slots[key] = move.lvs_primary_node_id + placement = state[move.lvs_primary_node_id] + state[move.lvs_primary_node_id] = ( + Placement(move.to_node_id, placement.tertiary) + if move.role == rp.ROLE_SECONDARY + else Placement(placement.secondary, move.to_node_id)) + return state + + +def _remove(layout, victim): + """The layout as phase 3b sees it: the victim's own LVS is gone (phase + 3a) and every role it hosted is homeless.""" + alive = [p for p in layout if p != victim] + current = { + p: Placement( + layout[p].secondary if layout[p].secondary != victim else "", + layout[p].tertiary if layout[p].tertiary != victim else "") + for p in alive + } + return alive, current + + +# --------------------------------------------------------------------------- +# The matcher +# --------------------------------------------------------------------------- + +class TestMinCostMatching(unittest.TestCase): + + def test_finds_the_optimum_on_a_small_matrix(self): + cost = [[4, 1, 3], [2, 0, 5], [3, 2, 2]] + got = rp.min_cost_matching(cost) + best = min( + sum(cost[i][perm[i]] for i in range(3)) + for perm in itertools.permutations(range(3))) + self.assertEqual(sum(cost[i][got[i]] for i in range(3)), best) + + def test_agrees_with_brute_force_on_random_matrices(self): + rng = random.Random(1) + for _ in range(50): + n = rng.randint(1, 6) + cost = [[rng.randint(0, 20) for _ in range(n)] for _ in range(n)] + got = rp.min_cost_matching(cost) + best = min( + sum(cost[i][perm[i]] for i in range(n)) + for perm in itertools.permutations(range(n))) + self.assertEqual(sum(cost[i][got[i]] for i in range(n)), best) + + def test_routes_around_forbidden_edges(self): + cost = [[rp.FORBIDDEN, 1], [1, rp.FORBIDDEN]] + self.assertEqual(rp.min_cost_matching(cost), [1, 0]) + + def test_empty_matrix(self): + self.assertEqual(rp.min_cost_matching([]), []) + + def test_rejects_more_rows_than_columns(self): + with self.assertRaises(ValueError): + rp.min_cost_matching([[1], [2]]) + + +# --------------------------------------------------------------------------- +# Diversity checking +# --------------------------------------------------------------------------- + +class TestFullDiversityViolations(unittest.TestCase): + + def test_clean_layout_has_none(self): + layout = {"a": Placement("b", "c")} + fd = {"a": 0, "b": 1, "c": 2} + self.assertEqual(rp.full_diversity_violations(layout, fd, 2), []) + + def test_secondary_and_tertiary_sharing_a_domain_is_a_violation(self): + # The exact state the ">=1 cross-domain role" floor accepts and the + # incremental relocation path kept producing: the secondary IS + # cross-domain from the primary, so the old check passed, but one + # domain outage still costs two of the three copies. + layout = {"a": Placement("b", "c")} + fd = {"a": 0, "b": 1, "c": 1} + violations = rp.full_diversity_violations(layout, fd, 2) + self.assertEqual(len(violations), 1) + self.assertIn("shares a domain with secondary b", violations[0]) + + def test_role_in_the_primarys_own_domain_is_a_violation(self): + layout = {"a": Placement("b", "c")} + fd = {"a": 0, "b": 1, "c": 0} + violations = rp.full_diversity_violations(layout, fd, 2) + self.assertEqual(len(violations), 1) + self.assertIn("shares a domain with primary a", violations[0]) + + def test_tertiary_ignored_on_ftt1(self): + layout = {"a": Placement("b", "")} + fd = {"a": 0, "b": 1} + self.assertEqual(rp.full_diversity_violations(layout, fd, 1), []) + + def test_unset_domain_on_a_holder_is_a_violation(self): + layout = {"a": Placement("b", "c")} + fd = {"a": 0, "b": 1, "c": -1} + self.assertIn("no failure domain set", + rp.full_diversity_violations(layout, fd, 2)[0]) + + def test_primary_without_a_domain_is_skipped(self): + layout = {"a": Placement("b", "c")} + fd = {"a": -1, "b": 1, "c": 1} + self.assertEqual(rp.full_diversity_violations(layout, fd, 2), []) + + def test_missing_role_is_a_violation(self): + layout = {"a": Placement("b", "")} + fd = {"a": 0, "b": 1} + self.assertIn("has no tertiary", + rp.full_diversity_violations(layout, fd, 2)[0]) + + +# --------------------------------------------------------------------------- +# Structural feasibility +# --------------------------------------------------------------------------- + +class TestFeasibilityConditions(unittest.TestCase): + + def test_a_domain_holding_more_than_half_blocks_diversity(self): + self.assertEqual(rp.secondary_overloaded_domains({0: 5, 1: 4}), [0]) + self.assertEqual(rp.secondary_overloaded_domains({0: 4, 1: 4}), []) + + def test_tertiary_blocking_pair_detected(self): + # 3 domains sized 3/3/2: every primary in domain 0 or 1 whose + # secondary is in the other must put its tertiary in domain 2, which + # only has 2 slots. + sizes = {0: 3, 1: 3, 2: 2} + self.assertEqual(rp.tertiary_blocking_pairs({(0, 1): 3}, sizes), [(0, 1)]) + self.assertEqual(rp.tertiary_blocking_pairs({(0, 1): 2}, sizes), []) + + +# --------------------------------------------------------------------------- +# The planner +# --------------------------------------------------------------------------- + +class TestPlanDiverseLayout(unittest.TestCase): + + def _assert_diverse(self, plan, fd, ftt): + self.assertTrue(plan.full_diversity, plan.violations) + self.assertEqual(plan.violations, []) + for primary, placement in plan.layout.items(): + domains = [fd[primary], fd[placement.secondary]] + if ftt >= 2: + domains.append(fd[placement.tertiary]) + self.assertEqual(len(set(domains)), len(domains), + f"{primary} -> {placement} domains {domains}") + + def test_builds_a_diverse_layout_from_scratch(self): + nodes, fd = _grid(4, 3) + empty = {n: Placement("", "") for n in nodes} + plan = rp.plan_diverse_layout(nodes, fd, empty, 2) + self._assert_diverse(plan, fd, 2) + + def test_roles_are_permutations(self): + nodes, fd = _grid(4, 3) + empty = {n: Placement("", "") for n in nodes} + plan = rp.plan_diverse_layout(nodes, fd, empty, 2) + for index in (0, 1): + holders = [pl[index] for pl in plan.layout.values()] + self.assertCountEqual(holders, nodes) + + def test_an_already_diverse_layout_is_left_alone(self): + nodes, fd = _grid(4, 2) + layout = _rotation(nodes, fd, 2) + plan = rp.plan_diverse_layout(nodes, fd, layout, 2) + self.assertEqual(plan.layout, layout) + self.assertEqual(rp.plan_moves(layout, plan.layout, nodes, 2), []) + + def test_repairs_one_bad_placement_with_the_fewest_moves(self): + nodes, fd = _grid(4, 2) + layout = dict(_rotation(nodes, fd, 2)) + # Break exactly one LVS by swapping two tertiaries into a collision. + victim = "d0n0" + other = next(p for p in nodes + if layout[p].tertiary != layout[victim].tertiary and p != victim) + layout[victim] = Placement(layout[victim].secondary, layout[other].tertiary) + layout[other] = Placement(layout[other].secondary, + _rotation(nodes, fd, 2)[victim].tertiary) + plan = rp.plan_diverse_layout(nodes, fd, layout, 2) + self._assert_diverse(plan, fd, 2) + moves = rp.plan_moves(layout, plan.layout, nodes, 2) + # Only the two swapped tertiaries move (plus one scratch hop to break + # the rotation, if the planner needs one). + self.assertLessEqual(len(moves), 3) + self.assertTrue(all(m.role == rp.ROLE_TERTIARY for m in moves)) + + def test_honours_host_anti_affinity_beyond_the_domain(self): + # Two nodes per host, two hosts per domain: FD diversity alone would + # let two roles share a host. + nodes, fd, host = [], {}, {} + for d in range(4): + for h in range(2): + for s in range(2): + nid = f"d{d}h{h}s{s}" + nodes.append(nid) + fd[nid] = d + host[nid] = f"d{d}h{h}" + empty = {n: Placement("", "") for n in nodes} + plan = rp.plan_diverse_layout(nodes, fd, empty, 2, host_by_node=host) + self._assert_diverse(plan, fd, 2) + for primary, placement in plan.layout.items(): + hosts = {host[primary], host[placement.secondary], host[placement.tertiary]} + self.assertEqual(len(hosts), 3) + + def test_ftt1_leaves_the_tertiary_empty(self): + nodes, fd = _grid(3, 2) + empty = {n: Placement("", "") for n in nodes} + plan = rp.plan_diverse_layout(nodes, fd, empty, 1) + self._assert_diverse(plan, fd, 1) + self.assertTrue(all(pl.tertiary == "" for pl in plan.layout.values())) + + def test_two_domains_at_ftt2_is_reported_degraded_not_faked(self): + # A 2-domain layout can never place a tertiary outside both the + # primary's and the secondary's domain. The planner must say so. + nodes, fd = _grid(2, 3) + empty = {n: Placement("", "") for n in nodes} + plan = rp.plan_diverse_layout(nodes, fd, empty, 2) + self.assertFalse(plan.full_diversity) + self.assertTrue(plan.violations) + self.assertTrue(plan.notes) + # ...but the host-disjointness floor is still held. + for primary, placement in plan.layout.items(): + self.assertEqual(len({primary, placement.secondary, placement.tertiary}), 3) + + def test_unbalanced_three_domain_layout_is_reported_degraded(self): + # 3 domains sized 3/3/2 at FTT2 is provably unsatisfiable (verified by + # brute force): the tertiaries of every 0<->1 pairing all have to fit + # into domain 2's two slots. + nodes = [f"d0n{i}" for i in range(3)] + [f"d1n{i}" for i in range(3)] \ + + [f"d2n{i}" for i in range(2)] + fd = {n: int(n[1]) for n in nodes} + empty = {n: Placement("", "") for n in nodes} + plan = rp.plan_diverse_layout(nodes, fd, empty, 2) + self.assertFalse(plan.full_diversity) + self.assertTrue(plan.notes) + + def test_domain_holding_more_than_half_is_noted(self): + nodes = [f"d0n{i}" for i in range(5)] + [f"d1n{i}" for i in range(2)] + fd = {n: int(n[1]) for n in nodes} + empty = {n: Placement("", "") for n in nodes} + plan = rp.plan_diverse_layout(nodes, fd, empty, 1) + self.assertFalse(plan.full_diversity) + self.assertTrue(any("more than half" in note for note in plan.notes)) + + def test_too_few_nodes_raises(self): + with self.assertRaises(InfeasiblePlacement): + rp.plan_diverse_layout(["a", "b"], {"a": 0, "b": 1}, + {"a": Placement("", ""), "b": Placement("", "")}, 2) + + def test_rejects_a_bad_ftt(self): + with self.assertRaises(ValueError): + rp.plan_diverse_layout(["a"], {"a": 0}, {}, 3) + + def test_empty_cluster(self): + plan = rp.plan_diverse_layout([], {}, {}, 2) + self.assertEqual(plan.layout, {}) + self.assertTrue(plan.full_diversity) + + def test_disabled_domains_still_produce_a_host_disjoint_layout(self): + # All nodes in one domain -- the feature is effectively off. The + # planner must not claim a diversity it cannot have, but must still + # return a usable host-disjoint layout. + nodes = [f"n{i}" for i in range(4)] + fd = {n: 0 for n in nodes} + empty = {n: Placement("", "") for n in nodes} + plan = rp.plan_diverse_layout(nodes, fd, empty, 2) + self.assertTrue(plan.full_diversity) # nothing to violate + for primary, placement in plan.layout.items(): + self.assertEqual(len({primary, placement.secondary, placement.tertiary}), 3) + + +# --------------------------------------------------------------------------- +# The reported scenario +# --------------------------------------------------------------------------- + +class TestFourDomainShrink(unittest.TestCase): + """The live case the greedy path could not hold: 4 failure domains x 3 + hosts, FTT2 (npcs=2, "2+2"), removing one host from each domain in turn. + Full pairwise diversity must survive all four removals.""" + + def _shrink(self, victims): + nodes, fd = _grid(4, 3) + layout = _rotation(nodes, fd, 2) + self.assertEqual(rp.full_diversity_violations(layout, fd, 2), []) + for victim in victims: + alive, current = _remove(layout, victim) + plan = rp.plan_diverse_layout(alive, fd, current, 2) + moves = rp.plan_moves(current, plan.layout, alive, 2) + layout = _apply(current, moves, 2) + self.assertEqual(layout, plan.layout) + self.assertEqual( + rp.full_diversity_violations(layout, fd, 2), [], + f"diversity lost after removing {victim}") + for index in (0, 1): + holders = [pl[index] for pl in layout.values()] + self.assertCountEqual(holders, alive) + return layout, fd + + def test_one_removal_per_domain_keeps_full_diversity(self): + self._shrink([f"d{d}n0" for d in range(4)]) + + def test_order_of_removals_does_not_matter(self): + for victims in itertools.permutations([f"d{d}n0" for d in range(4)]): + self._shrink(list(victims)) + + def test_removing_all_four_in_one_planning_pass(self): + nodes, fd = _grid(4, 3) + layout = _rotation(nodes, fd, 2) + alive = [n for n in nodes if not n.endswith("n0")] + current = { + p: Placement(layout[p].secondary if layout[p].secondary in alive else "", + layout[p].tertiary if layout[p].tertiary in alive else "") + for p in alive + } + plan = rp.plan_diverse_layout(alive, fd, current, 2) + moves = rp.plan_moves(current, plan.layout, alive, 2) + final = _apply(current, moves, 2) + self.assertEqual(rp.full_diversity_violations(final, fd, 2), []) + + +# --------------------------------------------------------------------------- +# Move planning and ordering +# --------------------------------------------------------------------------- + +class TestPlanMoves(unittest.TestCase): + + def test_diff_only_reports_actual_changes(self): + current = {"a": Placement("b", "c")} + target = {"a": Placement("b", "d")} + moves = rp.diff_layout(current, target, 2) + self.assertEqual( + moves, [rp.ReplicaMove("a", rp.ROLE_TERTIARY, "c", "d")]) + + def test_diff_ignores_the_tertiary_on_ftt1(self): + current = {"a": Placement("b", "c")} + target = {"a": Placement("b", "d")} + self.assertEqual(rp.diff_layout(current, target, 1), []) + + def test_chain_is_ordered_so_every_target_is_free(self): + # b's slot is free (nobody hosts a secondary there); a wants b, and + # c wants a's current host -- so a must move first. + current = {"p1": Placement("h1", ""), "p2": Placement("", "")} + target = {"p1": Placement("h2", ""), "p2": Placement("h1", "")} + moves = rp.order_moves( + rp.diff_layout(current, target, 1), current, ["h1", "h2", "p1", "p2"], 1) + self.assertEqual([m.lvs_primary_node_id for m in moves], ["p1", "p2"]) + + def test_rotation_cycle_is_broken_with_a_scratch_hop(self): + # p1 and p2 swap secondaries; h3 is free and used to park one of them. + current = {"p1": Placement("h1", ""), "p2": Placement("h2", "")} + target = {"p1": Placement("h2", ""), "p2": Placement("h1", "")} + nodes = ["h1", "h2", "h3", "p1", "p2"] + moves = rp.order_moves(rp.diff_layout(current, target, 1), current, nodes, 1) + self.assertEqual(sum(1 for m in moves if m.scratch), 1) + final = _apply(current, moves, 1) + self.assertEqual(final, target) + + def test_rotation_cycle_without_a_free_slot_is_refused(self): + # A full permutation with no free slot cannot rotate while + # lvstore_stack_secondary holds a single value. Refusing beats + # emitting a plan the mover would deadlock on. + current = {"p1": Placement("p2", ""), "p2": Placement("p1", "")} + target = {"p1": Placement("p1", ""), "p2": Placement("p2", "")} + with self.assertRaises(InfeasiblePlacement): + rp.order_moves(rp.diff_layout(current, target, 1), current, ["p1", "p2"], 1) + + def test_node_being_removed_is_never_used_as_scratch(self): + # "gone" is not in the surviving set: freeing its slot must not make + # it a parking spot. + current = {"p1": Placement("gone", ""), "p2": Placement("h1", "")} + target = {"p1": Placement("h1", ""), "p2": Placement("h2", "")} + nodes = ["h1", "h2", "p1", "p2"] + moves = rp.order_moves(rp.diff_layout(current, target, 1), current, nodes, 1) + self.assertNotIn("gone", [m.to_node_id for m in moves]) + self.assertEqual(_apply(current, moves, 1), target) + + def test_describe_plan_names_the_degradation(self): + nodes, fd = _grid(2, 3) + empty = {n: Placement("", "") for n in nodes} + plan = rp.plan_diverse_layout(nodes, fd, empty, 2) + self.assertIn("DEGRADED", rp.describe_plan(plan, [])) + nodes, fd = _grid(4, 2) + clean = rp.plan_diverse_layout(nodes, fd, _rotation(nodes, fd, 2), 2) + summary = rp.describe_plan(clean, []) + self.assertIn("fully domain-diverse", summary) + self.assertNotIn("DEGRADED", summary) + + +# --------------------------------------------------------------------------- +# Randomised end-to-end properties +# --------------------------------------------------------------------------- + +class TestRandomisedProperties(unittest.TestCase): + + def _run(self, domains, per_domain, ftt, trials, seed): + rng = random.Random(seed) + nodes, fd = _grid(domains, per_domain) + for trial in range(trials): + # An arbitrarily bad starting layout -- the state repeated greedy + # relocations can leave behind -- then one more removal on top. + while True: + sec = nodes[:] + rng.shuffle(sec) + if all(s != p for p, s in zip(nodes, sec)): + break + ter = [""] * len(nodes) + if ftt >= 2: + while True: + ter = nodes[:] + rng.shuffle(ter) + if all(t != p and t != s for p, s, t in zip(nodes, sec, ter)): + break + layout = {p: Placement(s, t) for p, s, t in zip(nodes, sec, ter)} + alive, current = _remove(layout, rng.choice(nodes)) + plan = rp.plan_diverse_layout(alive, fd, current, ftt) + moves = rp.plan_moves(current, plan.layout, alive, ftt) + final = _apply(current, moves, ftt) + self.assertEqual(final, plan.layout, f"trial {trial}") + self.assertEqual( + rp.full_diversity_violations(final, fd, ftt), [], + f"trial {trial}: {plan.notes}") + for index in range(ftt): + self.assertCountEqual([pl[index] for pl in final.values()], alive) + + def test_four_domains_ftt2(self): + self._run(4, 3, 2, trials=60, seed=11) + + def test_five_domains_ftt2(self): + self._run(5, 2, 2, trials=60, seed=17) + + def test_three_domains_ftt1(self): + self._run(3, 2, 1, trials=60, seed=23) + + +if __name__ == "__main__": + unittest.main()