diff --git a/simplyblock_core/cluster_ops.py b/simplyblock_core/cluster_ops.py index 7e3b91376d..f4cd88ff47 100644 --- a/simplyblock_core/cluster_ops.py +++ b/simplyblock_core/cluster_ops.py @@ -976,14 +976,16 @@ def _cluster_activate(cl_id, force=False, force_lvstore_create=False) -> None: set_cluster_status(cl_id, ols_status) raise - # Failure-domain coverage check (best-effort: warn, don't block). To - # survive losing a whole failure domain we need at least npcs+1 distinct - # domains; with fewer, placement falls back to host-disjoint and a domain - # outage may exceed the cluster's fault tolerance. + # Failure-domain coverage check (best-effort: warn, don't block). A 2-FD + # layout can never absorb a second independent failure once one domain + # is down, so the hard minimum below (enforced at fresh activation) is + # npcs+2, not npcs+1 -- this warning uses the same number so a + # reactivation that's short of it gets the same signal without being + # blocked (recovering a drifted layout must not turn into an outage). fd_desired_layout: t.Dict[str, t.Tuple[str, str]] = {} if cluster.enable_failure_domain: distinct_domains = {node.failure_domain for node in online_nodes if node.failure_domain >= 0} - min_domains = cluster.distr_npcs + 1 + min_domains = cluster.distr_npcs + 2 if len(distinct_domains) < min_domains: logger.warning( "Failure-domain feature is enabled but only %d distinct failure " @@ -1023,9 +1025,15 @@ def _fd_fail(msg: str) -> None: f"a host must sit entirely in one domain") fd_host_counts = Counter(host_fd.values()) - if len(fd_host_counts) < 2: - _fd_fail("failure domains are enabled but all hosts are in a " - "single domain; at least two domains are required") + # See fd_activation_domain_count_violation's docstring: npcs+2 + # domains, not just the bare rotation-correctness minimum, so a + # later single add/remove has a spare candidate instead of + # stranding another node's secondary/tertiary with none at all. + # This also subsumes the plain "at least two domains" floor. + domain_count_violation = fd_planner.fd_activation_domain_count_violation( + cluster.distr_npcs, len(fd_host_counts)) + if domain_count_violation: + _fd_fail(domain_count_violation) if len(set(fd_host_counts.values())) != 1: _fd_fail( f"failure domains must hold an EQUAL number of hosts at " @@ -1062,6 +1070,18 @@ def _fd_fail(msg: str) -> None: used_nodes_as_sec: t.List[str] = [] used_nodes_as_tertiary: t.List[str] = [] snodes = db_controller.get_storage_nodes_by_cluster_id(cl_id) + # Process primaries grouped by failure domain. get_secondary_nodes/ + # get_secondary_nodes_2 (and their splice repairs) already sort their own + # candidate scan by domain, which alone is enough to keep the assignment + # domain-disjoint when domains are evenly sized. But once any node needs + # splice-repair (uneven domain sizes, some conflict unavoidable), the + # repair works off whatever partial assignment already exists -- so which + # primary gets processed first still changes the outcome. Grouping here + # too makes the result deterministic instead of order-dependent in that + # case. A no-op when FD is disabled (all nodes share one failure_domain). + # Fresh FD+HA activation bypasses this fallback via fd_desired_layout, + # but reactivation and non-HA/non-fresh paths still rely on it. + snodes = sorted(snodes, key=lambda n: n.failure_domain) if cluster.ha_type == "ha": for snode in snodes: # Do not assign secondary to removed node @@ -1081,16 +1101,21 @@ def _fd_fail(msg: str) -> None: secondary_nodes = [fd_desired_layout[snode.get_id()][0]] else: secondary_nodes = storage_node_ops.get_secondary_nodes(snode) - if not secondary_nodes: + if secondary_nodes: + snode = db_controller.get_storage_node_by_id(snode.get_id()) + snode.secondary_node_id = secondary_nodes[0] + snode.write_to_db() + sec_node = db_controller.get_storage_node_by_id(snode.secondary_node_id) + sec_node.lvstore_stack_secondary = snode.get_id() + sec_node.write_to_db() + elif not storage_node_ops.splice_stranded_secondary(snode): + # get_secondary_nodes()'s greedy walk closed a cycle that + # excludes this node, and there isn't even one existing + # pairing left to splice it into (only possible this early + # in the pass, before 2+ pairings exist). set_cluster_status(cl_id, ols_status) raise ValueError("Failed to activate cluster, No enough secondary nodes") - snode = db_controller.get_storage_node_by_id(snode.get_id()) - snode.secondary_node_id = secondary_nodes[0] - snode.write_to_db() - sec_node = db_controller.get_storage_node_by_id(snode.secondary_node_id) - sec_node.lvstore_stack_secondary = snode.get_id() - sec_node.write_to_db() used_nodes_as_sec.append(snode.secondary_node_id) # Assign second secondary when max_fault_tolerance >= 2 @@ -1109,15 +1134,19 @@ def _fd_fail(msg: str) -> None: exclude_failure_domains=[sec_node.failure_domain], exclude_physical_labels=[sec_node.physical_label], ) - if not secondary_nodes_2: + if secondary_nodes_2: + snode.tertiary_node_id = secondary_nodes_2[0] + snode.write_to_db() + sec_node_2 = db_controller.get_storage_node_by_id(snode.tertiary_node_id) + sec_node_2.lvstore_stack_tertiary = snode.get_id() + sec_node_2.write_to_db() + elif not storage_node_ops.splice_stranded_tertiary(snode): + # get_secondary_nodes_2()'s greedy walk closed a cycle that + # excludes this node, and there isn't even one existing + # tertiary pairing left to splice it into. set_cluster_status(cl_id, ols_status) raise ValueError("Failed to activate cluster, not enough nodes for dual fault tolerance") - - snode.tertiary_node_id = secondary_nodes_2[0] - snode.write_to_db() - sec_node_2 = db_controller.get_storage_node_by_id(snode.tertiary_node_id) - sec_node_2.lvstore_stack_tertiary = snode.get_id() - sec_node_2.write_to_db() + snode = db_controller.get_storage_node_by_id(snode.get_id()) used_nodes_as_tertiary.append(snode.tertiary_node_id) # Pass 1: bring up the primary LVS on every online primary node. diff --git a/simplyblock_core/controllers/cluster_expansion/planner.py b/simplyblock_core/controllers/cluster_expansion/planner.py index 324a2b79de..4f4adf9ac1 100644 --- a/simplyblock_core/controllers/cluster_expansion/planner.py +++ b/simplyblock_core/controllers/cluster_expansion/planner.py @@ -503,6 +503,47 @@ def fd_balance_violation( return None +def fd_activation_domain_count_violation( + npcs: int, distinct_domain_count: int, +) -> Optional[str]: + """Validate the number of distinct failure domains for fresh activation. + + A 2-FD layout can never absorb a second independent failure once one + domain is fully down (confirmed with the backend team), so it is not + supported at any npcs level. + + The bare *correctness* minimum for the rotation layout itself is + npcs+1 (e.g. 2 domains for npcs=1, 3 for npcs=2 -- below that even the + initial static placement is wrong: at exactly 2 domains the tertiary + role mathematically always lands back in the primary's own domain, + since "2 steps ahead" in a period-2 round-robin wraps to where it + started; verified directly against rotation_layout()). But a + minimum-correct STATIC layout has zero spare hosts per domain, and the + moment a single node is added or removed, the relocation logic + (_pick_replica_relocation_node) has no spare candidate left to + reassign the stranded role to -- verified directly: removing one node + from a bare-minimum npcs=1/2-domain or npcs=2/3-domain layout strands + another node's secondary/tertiary with no replacement at all, blocking + the removal outright rather than just degrading placement quality. + + Requiring npcs+2 domains (3 for npcs=1, 4 for npcs=2, which also rules + out exactly 2 for both) keeps one domain of spare capacity beyond the + bare correctness floor, so a single add/remove has somewhere to place + the relocated role instead of failing immediately. Returns a + human-readable reason on violation, ``None`` when the count is + acceptable. + """ + min_domains = npcs + 2 + if distinct_domain_count < min_domains: + return ( + f"failure domains are enabled with npcs={npcs}, which requires at " + f"least {min_domains} distinct failure domains (2 domains is not " + f"supported at any npcs level); currently have " + f"{distinct_domain_count}. Add hosts in additional domains, or " + f"disable failure domains, then activate.") + return None + + # --------------------------------------------------------------------------- # Persistence helpers for ``Cluster.expand_state``. # diff --git a/simplyblock_core/models/nvme_device.py b/simplyblock_core/models/nvme_device.py index e36a80c700..4c82223f13 100644 --- a/simplyblock_core/models/nvme_device.py +++ b/simplyblock_core/models/nvme_device.py @@ -128,9 +128,6 @@ class JMDevice(NVMeDevice): # the per-leg member partitions. Empty for single-device (no-raid) JMs. jm_leg_bdevs: List[str] = [] jm_leg_members: List = [] - # When attaching this JM to a node, override the device name on that node. - # This is needed when a JM device is removed and needed to be replaced, but the name must be the same. - override_name_on_node: dict[str, str] = {} # node_id: new_name class RemoteDevice(BaseModel): diff --git a/simplyblock_core/rpc_client.py b/simplyblock_core/rpc_client.py index 1bba9177e3..04fc1a2dae 100644 --- a/simplyblock_core/rpc_client.py +++ b/simplyblock_core/rpc_client.py @@ -1418,6 +1418,31 @@ def jc_explicit_synchronization(self, jm_vuid): } return self._request("jc_explicit_synchronization", params) + def jc_replace_jm(self, name_old: str, name_new: str): + """Swap the JM bdev backing a live JC member from ``name_old`` to + ``name_new`` in place -- JC re-syncs the new JM's journal in the + background and, from then on, treats it as the member for this slot. + Replaces the old override_name_on_node naming trick (which faked the + replacement under the removed peer's old name so JC wouldn't need + touching): this RPC updates JC's live state directly, so the caller + can connect the replacement under its own natural name. + + ``name_new`` must already exist as a bdev (the caller connects it + first) and must not already be in use by JC. Raises RPCRemoteError + with one of the documented codes on rejection/failure: + -10 JC is closing + -11 invalid JM names (empty, or name_old == name_new) + -12 another JM replacement is already in progress + -13 name_old is not currently used by JC + -14 name_new is already used by JC + -15 the JM of name_old is being removed + -3 JC started closing during the operation + -4 the JM context disappeared during the operation + -5 failed to re-key the JM lists (OOM) -- affected vuids stopped + -6 timed out connecting to the new JM bdev + """ + return self._request3("jc_replace_jm", name_old=name_old, name_new=name_new) + def listeners_del(self, nqn, trtype, traddr, trsvcid): """" nqn: Subsystem NQN. diff --git a/simplyblock_core/services/tasks_runner_migration.py b/simplyblock_core/services/tasks_runner_migration.py index a457f914a5..405dc63b8c 100644 --- a/simplyblock_core/services/tasks_runner_migration.py +++ b/simplyblock_core/services/tasks_runner_migration.py @@ -86,6 +86,12 @@ def task_runner(task): _migration_retry_allowed(task, unavailable) return False + if snode.status == StorageNode.STATUS_REMOVED: + task.status = JobSchedule.STATUS_DONE + task.function_result = f"Node found in r: {task.node_id}" + task.write_to_db(db.kv_store) + return True + cluster = db.get_cluster_by_id(task.cluster_id) if cluster.status not in Cluster.OPERABLE_STATUSES: task.function_result = "cluster is not active, retrying" diff --git a/simplyblock_core/storage_node_ops.py b/simplyblock_core/storage_node_ops.py index dd3e7d6d9e..39a3c9c030 100644 --- a/simplyblock_core/storage_node_ops.py +++ b/simplyblock_core/storage_node_ops.py @@ -2,6 +2,7 @@ import copy import datetime import json +import logging import math import platform import socket @@ -19,6 +20,7 @@ import docker from docker.types import LogConfig from pydantic import SecretStr +from tenacity import RetryError, Retrying, before_sleep_log, retry_if_exception_type, stop_after_attempt, wait_fixed from simplyblock_core import constants, scripts, distr_controller, cluster_ops from simplyblock_core import utils @@ -2031,6 +2033,18 @@ def _connect_to_remote_jm_devs(this_node: StorageNode, jm_ids=None, only_node_id node are (re)connected; records for JMs owned by other nodes are carried over from ``this_node.remote_jm_devices`` untouched (same rationale and measurement as _connect_to_remote_devs delta mode). + + Always connects under the JM owner's own natural name. A replacement JM + picked for a removed peer (see _decommission_node_devices) used to be + forced to answer under the removed peer's OLD name here, via + JMDevice.override_name_on_node, so this_node's already-built distrib/ + JM-raid construct (which has that name baked in as a member) wouldn't + need touching. That naming trick is retired now that SPDK's + jc_replace_jm RPC can swap a live JC member by name directly: + _decommission_node_devices connects the replacement under its own name + and calls jc_replace_jm to update the construct in place, so nothing + downstream needs to keep pretending the new device is named after the + old one. """ db_controller = DBController() @@ -2108,20 +2122,20 @@ def _connect_to_remote_jm_devs(this_node: StorageNode, jm_ids=None, only_node_id # member of the new jm_vuid. Runtime re-attach paths (rejoin, # restart-task) carry their own reachability gating. + # Always connect under org_dev's own current name -- see the + # docstring for why no override resolution happens here anymore. + resolved_name = org_dev.jm_bdev + remote_device = RemoteJMDevice() remote_device.uuid = org_dev.uuid remote_device.alceml_name = org_dev.alceml_name remote_device.node_id = org_dev.node_id remote_device.size = org_dev.size - remote_device.jm_bdev = org_dev.jm_bdev + remote_device.jm_bdev = resolved_name remote_device.status = NVMeDevice.STATUS_ONLINE remote_device.nvmf_multipath = org_dev.nvmf_multipath - expected_bdev = f"remote_{org_dev.jm_bdev}n1" - controller_name = f"remote_{org_dev.jm_bdev}" - if org_dev.override_name_on_node and this_node.get_id() in org_dev.override_name_on_node: - new_name = org_dev.override_name_on_node[this_node.get_id()] - expected_bdev = f"remote_{new_name}n1" - controller_name = f"remote_{new_name}" + expected_bdev = f"remote_{resolved_name}n1" + controller_name = f"remote_{resolved_name}" connect_failed = False try: remote_device.remote_bdev = str(connect_device( @@ -2144,17 +2158,42 @@ def _connect_to_remote_jm_devs(this_node: StorageNode, jm_ids=None, only_node_id # 5s for one that can never appear. During whole-cluster recovery the # 10x0.5s wait ran per dead peer JM (~30 of them), adding minutes to # every restart attempt (2026-07-13). - for _ in range(1 if connect_failed else 10): - if remote_device.remote_bdev and rpc_client.get_bdevs(remote_device.remote_bdev): - break - if rpc_client.get_bdevs(expected_bdev): - remote_device.remote_bdev = expected_bdev - break - time.sleep(0.5) - if not remote_device.remote_bdev and org_dev.get_id() in existing_remote_jm_devices: - existing_remote_device = existing_remote_jm_devices[org_dev.get_id()] - if existing_remote_device.remote_bdev and rpc_client.get_bdevs(existing_remote_device.remote_bdev): - remote_device.remote_bdev = existing_remote_device.remote_bdev + def _poll_for_remote_jm_bdev(): + for _ in range(1 if connect_failed else 10): + if remote_device.remote_bdev and rpc_client.get_bdevs(remote_device.remote_bdev): + return + if rpc_client.get_bdevs(expected_bdev): + remote_device.remote_bdev = expected_bdev + return + time.sleep(0.5) + if not remote_device.remote_bdev and org_dev.get_id() in existing_remote_jm_devices: + existing_remote_device = existing_remote_jm_devices[org_dev.get_id()] + if existing_remote_device.remote_bdev and rpc_client.get_bdevs(existing_remote_device.remote_bdev): + remote_device.remote_bdev = existing_remote_device.remote_bdev + + try: + # Bounded retry: a transient RPC/DNS blip against this_node's + # own proxy (the same one connect_device just hit above) is + # given a few seconds to clear before giving up. Same + # degrade-not-crash rationale as the connect_device catch + # above -- only RPCException (the transport-level failure) is + # retried; anything else propagates immediately. + Retrying( + stop=stop_after_attempt(3), + wait=wait_fixed(1), + retry=retry_if_exception_type(RPCException), + before_sleep=before_sleep_log(logger, logging.WARNING), + )(_poll_for_remote_jm_bdev) + except RetryError as e: + # Still failing after 3 attempts -- degrade to "this JM not + # connected" instead of aborting the whole node-removal / + # restart operation (2026-08-10 incident: this exact call + # raised uncaught and killed a node-removal task mid phase 5, + # leaving a peer's lvstore un-rebuilt while the task still + # reported "done"). + logger.warning( + f'get_bdevs kept failing while polling for {expected_bdev} ' + f'on {this_node.get_id()} after 3 attempts: {e}') if not remote_device.remote_bdev: logger.error(f"Failed to connect to remote JM device {org_dev.alceml_name}") continue @@ -3560,7 +3599,23 @@ def _pick_replica_relocation_node(primary, removed_node: StorageNode, role, db_c 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. Returning None makes + with zero surviving paths. + + ``get_secondary_nodes``/``get_secondary_nodes_2`` only ever offer + UNCLAIMED nodes (each node hosts at most one secondary/tertiary at a + time — ``lvstore_stack_secondary``/``_tertiary`` is a single field, not + a list). A removal frees exactly one node system-wide (whoever hosted + ``removed_node``'s own role); if that one lands in the wrong domain — + or nothing is free at all — the direct search has nothing else to + offer even though a valid rearrangement exists elsewhere in the + cluster (2026-08-07, chained-removal incident: two removals in a row + stranded a third node's secondary with zero free cross-domain + candidates, while an existing pairing two hops away could have + absorbed it). Falls back to splicing ``primary`` into an already-formed + 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 ``_check_replica_relocation_feasible`` refuse the removal up front. """ exclude_ids = [removed_node.get_id()] @@ -3581,12 +3636,9 @@ def _pick_replica_relocation_node(primary, removed_node: StorageNode, role, db_c pass cands = get_secondary_nodes_2( primary, exclude_ids=exclude_ids, exclude_mgmt_ips=exclude_mgmt_ips) - if not cands: - return None cluster = db_controller.get_cluster_by_id(primary.cluster_id) - if (getattr(cluster, "enable_failure_domain", False) - and primary.failure_domain >= 0): + 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(): try: @@ -3606,8 +3658,84 @@ def _pick_replica_relocation_node(primary, removed_node: StorageNode, role, db_c if (cand.failure_domain >= 0 and cand.failure_domain != primary.failure_domain): return cand_id - return None - return cands[0] + else: + 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 + + +def _find_splice_target_for_relocation(stranded_primary, role, db_controller, exclude_ids=()): + """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 + how to execute the resulting move (see ``_relocate_one_replica``). + + Generalizes ``splice_stranded_secondary``/``splice_stranded_tertiary``'s + edge search (same scoring: prefer both ends domain-disjoint from the + stranded node, then relax) with an ``exclude_ids`` list, so the + node-removal repair path can rule out the node being removed and any + other already-claimed id. Unlike the activation-time splice helpers — + which only ever run before any physical LVS exists — this can be asked + to splice into a pairing that already has real data on both ends; + executing that move (not just picking the edge) is the caller's job. + + Returns ``(p_id, x_id)`` or ``None`` if no valid edge exists. + """ + field = "secondary_node_id" if role == "secondary" else "tertiary_node_id" + all_nodes = db_controller.get_storage_nodes_by_cluster_id(stranded_primary.cluster_id) + all_nodes = sorted(all_nodes, key=lambda n: n.failure_domain) + by_id = {n.get_id(): n for n in all_nodes} + exclude = set(exclude_ids) | {stranded_primary.get_id()} + + stranded_sec = None + if role == "tertiary" and stranded_primary.secondary_node_id: + stranded_sec = by_id.get(stranded_primary.secondary_node_id) + + def _online(*nodes): + return all(n.status == StorageNode.STATUS_ONLINE for n in nodes) + + def _valid_tertiary(node, node_sec, candidate): + if candidate.get_id() == node.get_id(): + return False + if candidate.mgmt_ip == node.mgmt_ip: + return False + if node_sec and candidate.mgmt_ip == node_sec.mgmt_ip: + return False + return True + + def _domain_mismatch_score(*nodes): + if stranded_primary.failure_domain < 0: + return 0 + 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] + + best, best_score = None, -1 + for p in edges: + x_id = getattr(p, field) + if x_id in exclude: + continue + x = by_id.get(x_id) + if not x or not _online(p, x): + continue + if role == "secondary": + if p.mgmt_ip == stranded_primary.mgmt_ip or x.mgmt_ip == stranded_primary.mgmt_ip: + continue + else: + p_sec = by_id.get(p.secondary_node_id) if p.secondary_node_id else None + if not _valid_tertiary(p, p_sec, stranded_primary): + continue + if not _valid_tertiary(stranded_primary, stranded_sec, x): + continue + score = _domain_mismatch_score(p, x) + if score > best_score: + best_score, best = score, (p.get_id(), x.get_id()) + + return best def node_removal_orchestrate(node_id, force_remove=False): @@ -3627,8 +3755,18 @@ def node_removal_orchestrate(node_id, force_remove=False): logger.error(f"node_removal_orchestrate: node {node_id} not found") return False - if snode.status == StorageNode.STATUS_REMOVED: - return True + # Phase 4 (below) flips status to REMOVED *before* phase 5 (device/JM + # decommission) runs -- so "status == REMOVED" means phases 1/3a/3b/4 + # committed, NOT that removal is fully done. A bare `return True` here + # would let a transient failure inside phase 5 (e.g. an RPC error + # against a peer) get permanently masked: the retry re-enters, hits this + # guard, and reports "done" forever without phase 5 ever completing + # (2026-08-10 incident: a mid-phase-5 RPC error left a peer's lvstore + # un-rebuilt while the task reported "Node removed"). Only phases + # 1/3a/3b/4 are skipped below when already_removed; phase 5 always + # runs and is itself idempotent (skips devices/JM already migrated), so + # resuming it here is a no-op once it has genuinely finished. + already_removed = snode.status == StorageNode.STATUS_REMOVED # Node removal is a recognised restart-phase owner: phase 3b relocates # replicas onto an ONLINE target and sets a restart phase there, which @@ -3641,40 +3779,43 @@ def node_removal_orchestrate(node_id, force_remove=False): prev_cluster_status = cluster.status cluster_ops.set_cluster_status(cluster.get_id(), Cluster.STATUS_IN_SHRINK) try: - # Phase 1 — shut the node down (graceful). Skipped on re-entry. - if snode.status in [StorageNode.STATUS_ONLINE, StorageNode.STATUS_SUSPENDED]: - logger.info(f"[REMOVAL] {node_id}: phase 1 — shutdown") - ret = shutdown_storage_node(node_id, force=force_remove) - if isinstance(ret, tuple): - ret, reason = ret - if not ret: - logger.error(f"[REMOVAL] {node_id}: shutdown failed: {reason}") + if not already_removed: + # Phase 1 — shut the node down (graceful). Skipped on re-entry. + if snode.status in [StorageNode.STATUS_ONLINE, StorageNode.STATUS_SUSPENDED]: + logger.info(f"[REMOVAL] {node_id}: phase 1 — shutdown") + ret = shutdown_storage_node(node_id, force=force_remove) + if isinstance(ret, tuple): + ret, reason = ret + if not ret: + logger.error(f"[REMOVAL] {node_id}: shutdown failed: {reason}") + return False + elif not ret: + logger.error(f"[REMOVAL] {node_id}: shutdown failed") return False - elif not ret: - logger.error(f"[REMOVAL] {node_id}: shutdown failed") - return False - snode = db_controller.get_storage_node_by_id(node_id) + snode = db_controller.get_storage_node_by_id(node_id) - # Phase 3a — tear down the (empty) secondary/tertiary replicas of THIS - # node's own primary LVS, on the peers that host them (Case A). - logger.info(f"[REMOVAL] {node_id}: phase 3a — tear down own replicas") - if not _teardown_replicas_of_primary(snode): - return False + # Phase 3a — tear down the (empty) secondary/tertiary replicas of THIS + # node's own primary LVS, on the peers that host them (Case A). + logger.info(f"[REMOVAL] {node_id}: phase 3a — tear down own replicas") + if not _teardown_replicas_of_primary(snode): + return False - # Phase 3b — relocate replicas this node hosts for OTHER primaries (Case B). - logger.info(f"[REMOVAL] {node_id}: phase 3b — relocate hosted replicas") - if not _relocate_replicas_hosted_on(snode): - return False + # Phase 3b — relocate replicas this node hosts for OTHER primaries (Case B). + logger.info(f"[REMOVAL] {node_id}: phase 3b — relocate hosted replicas") + if not _relocate_replicas_hosted_on(snode): + return False - # Phase 5 — finalize (swarm leave, gpt cleanup) and flip to removed. - logger.info(f"[REMOVAL] {node_id}: phase 4 — finalize") - _finalize_node_removal(snode) - set_node_status(node_id, StorageNode.STATUS_REMOVED, caused_by="remove") - snode = db_controller.get_storage_node_by_id(node_id) - # storage_events.snode_status_change( - # snode, StorageNode.STATUS_REMOVED, StorageNode.STATUS_IN_REMOVAL, caused_by="remove") + # Phase 4 — finalize (swarm leave, gpt cleanup) and flip to removed. + logger.info(f"[REMOVAL] {node_id}: phase 4 — finalize") + _finalize_node_removal(snode) + set_node_status(node_id, StorageNode.STATUS_REMOVED, caused_by="remove") + snode = db_controller.get_storage_node_by_id(node_id) + # storage_events.snode_status_change( + # snode, StorageNode.STATUS_REMOVED, StorageNode.STATUS_IN_REMOVAL, caused_by="remove") - # Phase 4 — remove + fail devices, then wait for failure-migration to finish. + # Phase 5 — remove + fail devices, then wait for failure-migration to + # finish. Always attempted, even on resume after status already + # flipped to REMOVED -- see the already_removed comment above. logger.info(f"[REMOVAL] {node_id}: phase 5 — devices remove/fail/migrate") if not _decommission_node_devices(snode): return False @@ -3710,6 +3851,7 @@ def _teardown_replicas_of_primary(removed_node: StorageNode): if peer.status == StorageNode.STATUS_ONLINE: _delete_replica_on_peer(peer, removed_node, cluster) + _prune_stale_lvstore_ports(peer_id, removed_node.lvstore, db_controller) # Clear the peer's back-reference if it still points at us. peer = db_controller.get_storage_node_by_id(peer_id) @@ -3724,32 +3866,152 @@ def _teardown_replicas_of_primary(removed_node: StorageNode): return True -def _delete_replica_on_peer(peer, primary, cluster): +def _delete_replica_on_peer(peer, primary, cluster, destroy_lvstore=True): """Best-effort teardown of ``primary``'s replica lvstore (+ hublvol) on the online ``peer``. RPC failures are logged, not fatal: the peer is healthy and - a lingering empty bdev is harmless, while blocking removal on it is not.""" + a lingering empty bdev is harmless, while blocking removal on it is not. + + ``destroy_lvstore``: whether the shared on-disk blobstore backing this + lvstore may be destroyed once the local bdev stack comes down. + + - ``True`` (default -- Case A, node removal): ``primary`` IS the node + being removed; nothing else will ever read this lvstore again once + its own devices are decommissioned, so destroying it here is correct. + - ``False`` (splice/relocation eviction): ``primary`` is a SURVIVING + node whose replica is only being *moved* off ``peer`` onto a new + host -- the lvstore itself stays alive and in use elsewhere. + ``peer`` only ever held a non-leader examine copy, so + ``bdev_lvol_delete_lvstore`` here would destroy the shared blobstore + metadata out from under the still-live primary and any other + surviving replica (2026-08-16: this is what actually corrupted + LVS_1's on-disk metadata during a splice eviction, surfacing later + as `bs_super_validate: unsupported version on super block` when the + primary tried to reload it on restart -- caught only after the fact + via log analysis, not before). Pass ``False`` here; only the local + raid/distrib examine bdevs get hot-removed, matching + ``teardown_non_leader_lvstore``'s existing, correct pattern for the + identical non-leader-eviction case.""" rpc_client = peer.rpc_client() lvstore = primary.lvstore if not lvstore: return - try: - nqn = peer.hublvol_nqn_for_lvstore(cluster.nqn, lvstore) - if rpc_client.subsystem_get(nqn): - rpc_client.subsystem_delete(nqn) - except RPCException as e: - logger.warning(f"hublvol subsystem teardown for {lvstore} on {peer.get_id()} failed: {e}") - try: - rpc_client.bdev_lvol_delete_hublvol(lvstore) - except RPCException as e: - logger.warning(f"hublvol bdev teardown for {lvstore} on {peer.get_id()} failed: {e}") + # peer's own hublvol subsystem for this replica has no consumers -- only + # a promoted primary's hublvol is ever attached-to -- so it sits dormant + # and is cleaned up naturally on peer's next restart; left alone here. + # try: + # nqn = peer.hublvol_nqn_for_lvstore(cluster.nqn, lvstore) + # if rpc_client.subsystem_get(nqn): + # rpc_client.subsystem_delete(nqn) + # except RPCException as e: + # logger.warning(f"hublvol subsystem teardown for {lvstore} on {peer.get_id()} failed: {e}") + # try: + # rpc_client.bdev_lvol_delete_hublvol(lvstore) + # except RPCException as e: + # logger.warning(f"hublvol bdev teardown for {lvstore} on {peer.get_id()} failed: {e}") + + # UNLIKE the subsystem above, peer's NVMe-oF controller connecting TO + # primary's hublvol *is* a live consumer connection (kept attached the + # whole time peer held this replica, for fast failover) and must be + # detached here -- mirrors teardown_non_leader_lvstore's step 2. Leaving + # it dangling is not harmless: if peer is later re-selected to host a + # replica of this same primary again before its next restart, the stale + # controller can be found wedged in a non-enabled state, and the + # reconcile's detach-and-wait-gone can then time out and abort the + # rebuild (2026-08-14: this exact sequence took ffznh's SPDK down -- + # this connection was left behind by an earlier splice eviction and + # ffznh was never restarted before being re-selected as 5bc9k's + # secondary again). + if primary.hublvol and primary.hublvol.bdev_name: + try: + rpc_client.bdev_nvme_detach_controller(primary.hublvol.bdev_name) + except RPCException as e: + logger.warning(f"hublvol controller detach for {lvstore} on {peer.get_id()} failed: {e}") try: # deepcopy: _remove_bdev_stack stamps bdev['status']; don't mutate the # primary's stored stack definition. - _remove_bdev_stack(copy.deepcopy(primary.lvstore_stack), rpc_client) + _remove_bdev_stack(copy.deepcopy(primary.lvstore_stack), rpc_client, + remove_distr_only=not destroy_lvstore) except RPCException as e: logger.warning(f"replica bdev-stack teardown for {lvstore} on {peer.get_id()} failed: {e}") +def _prune_stale_lvstore_ports(node_id, lvstore, db_controller): + """Drop ``lvstore``'s port reservation from ``node_id``'s + ``lvstore_ports`` after its replica has been torn down there for good. + + Unlike ``teardown_non_leader_lvstore``'s donor-reconnect path (which + deliberately keeps the entry so a returning node reuses its old ports), + callers of this helper -- node removal, splice eviction -- know the + replica isn't coming back to this node. A stale entry there only + misrepresents `sn list`'s "LVS Ports" column (2026-08-12: found live via + bdev_lvol_get_lvstores disagreeing with the DB after a node removal). + Re-fetches fresh to avoid clobbering unrelated concurrent edits.""" + if not lvstore: + return + node = db_controller.get_storage_node_by_id(node_id) + if node.lvstore_ports and lvstore in node.lvstore_ports: + del node.lvstore_ports[lvstore] + node.write_to_db() + + +def _teardown_lvol_subsystems_on_vacated_peer(peer, primary, db_controller): + """Best-effort: delete every LVol-of-``primary``'s own NVMe-oF subsystem + on ``peer`` after ``peer`` stops hosting ``primary``'s replica. + + ``_delete_replica_on_peer(..., destroy_lvstore=False)`` tears down + ``peer``'s local raid/distrib bdev stack for the lvstore, which cascades + to remove each hosted LVol's *namespace* -- but the per-LVol NVMe-oF + *subsystem and listener* (registered separately, per lvol, via + add_lvol_thread) are never touched by that teardown. Left behind, the + listener keeps accepting connections in front of a now-empty subsystem + -- exactly the "live but no path" failure test_missing_namespace_path_ + loss.py guards against, just reached by a different door: the CSI/host + initiator's own connection to peer stays live, and since peer is no + longer in lvol.nodes nothing ever tells it to drop that connection + either. The volume ends up with an extra path that looks healthy but + carries no I/O, alongside the correct new one (2026-08-18: found live + after the lvol.nodes/wrong-port fixes above -- both corrected lvols + still carried this stale-but-live third path to their pre-relocation + host). RPC failures are logged, not fatal: peer is healthy and a + lingering empty subsystem is harmless to leave for the next restart to + clear, while blocking the relocation on it is not.""" + rpc_client = peer.rpc_client() + for lvol in db_controller.get_lvols_by_node_id(primary.get_id()): + try: + rpc_client.subsystem_delete(lvol.nqn) + except RPCException as e: + logger.warning( + f"subsystem teardown for lvol {lvol.get_id()} ({lvol.nqn}) " + f"on vacated peer {peer.get_id()} failed: {e}") + + +def _update_lvol_nodes_for_replica_move(primary_id, old_host_id, new_host_id, db_controller): + """Re-point every LVol hosted on ``primary_id`` from ``old_host_id`` to + ``new_host_id`` in its own ``nodes`` list, once that primary's + secondary/tertiary replica has been relocated between the two hosts. + + ``lvol.nodes`` is what the CSI/host initiator actually connects to for + multipath failover -- a separate record from the storage-node-level + ``secondary_node_id``/``tertiary_node_id`` bookkeeping the relocation + itself already updates. Leaving the old host listed here strands every + LVol hosted on ``primary_id`` on a single path (the primary only) once + the old host is gone/unreachable, with nothing ever repointing it -- + mirrors cluster_expansion/executor.py's identical fix for the + expansion-rebalancing case. (2026-08-18: found live during node + removal -- a splice-relocated secondary left a still-online lvol's + ``nodes`` naming the just-removed node; the CSI initiator never + reconnected to the actual new secondary.) + + Safe to call redundantly (e.g. on a retry after an earlier attempt + already applied it): each lvol is only rewritten if ``old_host_id`` is + still present in its ``nodes``.""" + for lvol in db_controller.get_lvols_by_node_id(primary_id): + nodes = list(lvol.nodes or []) + if old_host_id in nodes: + lvol.nodes = [new_host_id if n == old_host_id else n for n in nodes] + lvol.write_to_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 @@ -3796,6 +4058,21 @@ def _relocate_one_replica(removed_node: StorageNode, primary_id, role): logger.error( f"[REMOVAL] no relocation target for {role} replica of {primary_id}") return False + + new_node = db_controller.get_storage_node_by_id(new_id) + occupant_id = getattr(new_node, backref) + if occupant_id and occupant_id not in (primary_id, removed_node.get_id()): + # _pick_replica_relocation_node fell back to a splice candidate: + # new_id is currently busy hosting occupant_id's replica. Evict + # that occupant onto `primary` (the node whose replica we're + # relocating) before claiming new_id for primary's own role — + # see _find_splice_target_for_relocation's docstring. + if not _relocate_replica_between(occupant_id, new_id, primary_id, role, db_controller): + logger.error( + f"[REMOVAL] failed to splice {primary_id} into the pairing " + f"occupying {new_id} (occupant {occupant_id})") + return False + primary = db_controller.get_storage_node_by_id(primary_id) setattr(primary, field, new_id) primary.write_to_db() @@ -3815,10 +4092,168 @@ def _relocate_one_replica(removed_node: StorageNode, primary_id, role): f"[REMOVAL] failed to rebuild {role} replica of {primary_id} on {new_id}, will retry") return False + # Re-point every LVol hosted on primary before dropping removed_node's + # side of the relationship -- see _update_lvol_nodes_for_replica_move's + # docstring. Unconditional (not gated on "did we just build it above") + # so a retry that resumes past the build-skip branch still catches up + # if an earlier attempt crashed between the build and this step. + _update_lvol_nodes_for_replica_move(primary_id, removed_node.get_id(), new_id, db_controller) + _clear_replica_backref(removed_node, backref) return True +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 + ``new_host_id``'s back-reference. + + Used by the splice fallback in ``_relocate_one_replica``: before an + already-busy node can be claimed for the primary being relocated, its + current occupant must move onto that primary's node instead (see + ``_find_splice_target_for_relocation``'s docstring for why an + already-formed pairing, not an idle node, is what's available). + + ``new_host_id`` itself may ALREADY be hosting a different primary's + replica via this same single-value ``lvstore_stack_secondary`` / + ``lvstore_stack_tertiary`` slot: every node in a full ring hosts exactly + one other node's replica before any removal starts, and that + relationship has nothing to do with whichever edge the splice search + happened to pick. (2026-08-12 incident: a splice claimed a node whose + slot already held an unrelated pre-existing occupant. The physical + build succeeded -- SPDK doesn't mind hosting a second lvstore -- but the + slot could not record it, silently untracking that replica for any + future failover, and leaving `sn list`'s "LVS Ports" column short by + one entry.) When the slot is occupied, the existing occupant is + relocated first -- recursively, via this same function -- onto a fresh + target, before ``occupant_primary``'s replica claims the freed slot. + This is a rotation, not a retry loop: ``_seen`` accumulates visited + ``new_host_id``s across the recursion purely as a cycle backstop against + a topology bug; the rotation itself is always finite, since each hop + heads toward the one slot the original removal freed. + + Create-before-destroy: the new replica is built on ``new_host_id`` + BEFORE the old one on ``old_host_id`` is torn down, so + ``occupant_primary`` never has zero surviving copies -- critical on + FTT1 (no tertiary): a cluster only tolerates one node down at a time, + and that budget belongs to the node actually being removed, not to + whatever healthy node this splice happens to touch. (2026-08-07 + incident: the old destroy-then-build order tore down the occupant's + only copy up front; a hublvol attach failure on the rebuild then + retried for minutes with that copy already gone.) A raised exception + from the rebuild is treated the same as a returned False -- both leave + the old copy untouched and safe to retry. + + Idempotent and retry-safe: "already built" is read from the occupant's + forward pointer, so a retry after a confirmed build skips straight to + the teardown check without re-running the rebuild. The teardown itself + is guarded separately by ``old_host``'s own back-reference (not the + occupant's forward pointer), so a crash between the two commits still + resumes the teardown on the next pass instead of leaking a stale + replica on ``old_host`` forever. + + Returns True if ``occupant_primary_id`` no longer exists — nothing left + to relocate. + """ + field = "secondary_node_id" if role == "secondary" else "tertiary_node_id" + backref = "lvstore_stack_secondary" if role == "secondary" else "lvstore_stack_tertiary" + seen = _seen if _seen is not None else set() + try: + occupant_primary = db_controller.get_storage_node_by_id(occupant_primary_id) + except KeyError: + return True + + if getattr(occupant_primary, field) != new_host_id: + new_host = db_controller.get_storage_node_by_id(new_host_id) + + existing_occupant_id = getattr(new_host, backref) + if existing_occupant_id and existing_occupant_id != occupant_primary_id: + if new_host_id in seen: + logger.error( + f"[REMOVAL] splice: cycle detected while vacating " + f"{new_host_id}'s existing {role} occupant " + f"{existing_occupant_id} to make room for " + f"{occupant_primary_id}; refusing") + return False + seen.add(new_host_id) + try: + existing_occupant = db_controller.get_storage_node_by_id(existing_occupant_id) + except KeyError: + existing_occupant = None + vacate_target = ( + _pick_replica_relocation_node(existing_occupant, new_host, role, db_controller) + if existing_occupant else None) + if not vacate_target or vacate_target == occupant_primary_id: + logger.error( + f"[REMOVAL] splice: no relocation target to vacate " + f"{new_host_id}'s existing {role} occupant " + f"{existing_occupant_id}; cannot free the slot for " + f"{occupant_primary_id}") + return False + if not _relocate_replica_between( + existing_occupant_id, new_host_id, vacate_target, role, + db_controller, _seen=seen): + logger.error( + f"[REMOVAL] splice: failed to vacate {new_host_id}'s " + f"existing {role} occupant {existing_occupant_id} onto " + f"{vacate_target}") + return False + new_host = db_controller.get_storage_node_by_id(new_host_id) + + try: + built = recreate_lvstore_on_non_leader(new_host, occupant_primary, occupant_primary) + except Exception as e: + logger.error( + f"[REMOVAL] splice: failed to build {role} replica of " + f"{occupant_primary_id} on {new_host_id}, old copy on " + f"{old_host_id} left untouched: {e}") + return False + if not built: + return False + occupant_primary = db_controller.get_storage_node_by_id(occupant_primary_id) + setattr(occupant_primary, field, new_host_id) + occupant_primary.write_to_db() + + # Record the new host's side of the relationship too. Re-fetch: the + # build above (recreate_lvstore_on_non_leader) persists its own + # lvstore_ports snapshot for new_host, derived only from its + # (now-vacated) lvstore_stack_secondary/_tertiary -- it has no way to + # know about occupant_primary, so this entry has to be added here. + new_host = db_controller.get_storage_node_by_id(new_host_id) + setattr(new_host, backref, occupant_primary_id) + if not new_host.lvstore_ports: + new_host.lvstore_ports = {} + new_host.lvstore_ports[occupant_primary.lvstore] = { + "lvol_subsys_port": occupant_primary.lvol_subsys_port, + "hublvol_port": occupant_primary.hublvol.nvmf_port if occupant_primary.hublvol else 0, + } + new_host.write_to_db() + + # Re-point every LVol hosted on occupant_primary before the old_host + # teardown below -- see _update_lvol_nodes_for_replica_move's docstring. + # Unconditional (outside the "if not already built" guard above) so a + # retry that skips straight past that guard still catches up if an + # earlier attempt crashed between the build and this step. + _update_lvol_nodes_for_replica_move(occupant_primary_id, old_host_id, new_host_id, db_controller) + + 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: + # 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 + # destroy_lvstore docstring. + _delete_replica_on_peer(old_host, occupant_primary, cluster, + 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) + old_host = db_controller.get_storage_node_by_id(old_host_id) + setattr(old_host, backref, "") + old_host.write_to_db() + return True + + def _clear_replica_backref(removed_node: StorageNode, backref): db_controller = DBController() removed_node = db_controller.get_storage_node_by_id(removed_node.get_id()) @@ -3843,25 +4278,134 @@ def _decommission_node_devices(removed_node: StorageNode): device_controller.remove_jm_device(removed_node.jm_device.get_id(), force=True) # look for other nodes who use this JM and replace it for node in db_controller.get_storage_nodes_by_cluster_id(removed_node.cluster_id): + # get_storage_nodes_by_cluster_id returns every node regardless of + # status, including ones already REMOVED. An already-removed node + # can still carry the just-removed node's JM id in its own stale + # jm_ids (never cleared on ITS removal) -- without this guard we'd + # try to "fix" that dead node's JM connections using its own + # rpc_client, which points at a pod that no longer exists and can + # never resolve/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). + if node.status == StorageNode.STATUS_REMOVED: + continue if node.jm_ids and removed_node.jm_device.get_id() in node.jm_ids: - node.jm_ids.remove(removed_node.jm_device.get_id()) + removed_jm_id = removed_node.jm_device.get_id() + # Capture the exact bdev name node's JC currently has live + # for this slot BEFORE any bookkeeping changes below -- this + # is jc_replace_jm's ``name_old``. It's already the resolved + # name whatever it is (this node's own natural connect, or a + # leftover override name from before this RPC existed), so + # there's no need to separately consult the removed device's + # own naming the way the old override-chaining logic did. + old_remote_dev = next( + (rd for rd in (node.remote_jm_devices or []) if rd.uuid == removed_jm_id), None) + name_old = old_remote_dev.remote_bdev if old_remote_dev else None + + node.jm_ids.remove(removed_jm_id) jm_ids = get_sorted_ha_jms(node) logger.debug(f"online_jms: {str(jm_ids)}") + # A candidate node already reaches via some path OTHER than + # its own jm_ids -- most commonly the hosted-primary route: + # node hosts some OTHER primary's secondary/tertiary copy, + # and that primary's own jm_ids already includes this + # candidate. SPDK won't attach a second, distinctly-named + # local controller to a target it already has a live + # connection to under another name (the attach RPC returns + # without a bdev name), and jc_replace_jm itself rejects a + # name_new already in use by JC (-14) -- so a candidate node + # already reaches, however it reaches it, can never actually + # serve as the replacement. Skip it so a fresh pick never + # collides this way (found live 2026-08-19: "Bdev name not + # returned from controller attach", the JM group "excluded + # from further operation" on an ongoing SPDK retry loop, + # while the DB metadata reported success -- back when a + # collision here was silently accepted anyway). + already_reachable = {rd.uuid for rd in (node.remote_jm_devices or [])} new_jm_dev = "" for jm_id in jm_ids: - if jm_id not in node.jm_ids: - new_jm_dev = jm_id - break - if new_jm_dev: + if jm_id in node.jm_ids or jm_id in already_reachable: + continue + new_jm_dev = jm_id + break + + replaced = False + if new_jm_dev and name_old: d = db_controller.get_jm_device_by_id(new_jm_dev) - jm_node = db_controller.get_storage_node_by_id(d.node_id) - jm_node.jm_device.override_name_on_node[node.get_id()]=removed_node.jm_device.jm_bdev - jm_node.write_to_db() - node.jm_ids.append(new_jm_dev) - node.remote_jm_devices = _connect_to_remote_jm_devs(node, node.jm_ids) - node.write_to_db() - else: - logger.error(f"no jm_id found for {node.get_id()}") + controller_name = f"remote_{d.jm_bdev}" + try: + # Connect the replacement under its own name first -- + # jc_replace_jm hands off to an already-live bdev, it + # doesn't create the connection itself. + connected = _connect_to_remote_jm_devs( + node, jm_ids=[new_jm_dev], only_node_id=d.node_id) + new_remote_dev = next( + (rd for rd in connected if rd.uuid == new_jm_dev), None) + if not new_remote_dev or not new_remote_dev.remote_bdev: + raise RPCException( + f"failed to connect replacement JM device {new_jm_dev}") + name_new = new_remote_dev.remote_bdev + node.rpc_client(timeout=30, retry=2).jc_replace_jm( + name_old=name_old, name_new=name_new) + logger.info( + f"[REMOVAL] {node.get_id()}: jc_replace_jm {name_old} -> " + f"{name_new} ({new_jm_dev}) replacing removed JM {removed_jm_id}") + node.jm_ids.append(new_jm_dev) + node.remote_jm_devices = connected + replaced = True + except Exception as e: + logger.error( + f"[REMOVAL] {node.get_id()}: jc_replace_jm failed replacing " + f"{name_old} with candidate {new_jm_dev} ({controller_name}): {e}") + # Best-effort: don't leave an attached-but-unused + # connection sitting around. -14 (name_new already + # used by JC) is the one failure where the bdev is + # legitimately claimed by JC already -- detaching it + # would tear down something in active use, so skip + # the cleanup in that specific case. + if getattr(e, "code", None) != -14: + try: + node.rpc_client().bdev_nvme_detach_controller(controller_name) + except Exception as de: + logger.warning( + f"Failed to detach unused controller " + f"{controller_name} on {node.get_id()}: {de}") + if not replaced: + # No collision-free candidate, or the replace RPC itself + # failed -- leave the redundancy slot honestly short + # (jm_ids already has the dead id removed above) rather + # than claim a replacement that isn't actually live in + # JC. Nothing currently revisits this automatically; it + # stays a visible gap until a future removal/reconnect + # cycle retries it. + if not new_jm_dev: + logger.error(f"no jm_id found for {node.get_id()}") + elif not name_old: + logger.error( + f"[REMOVAL] {node.get_id()}: no recorded bdev name for removed " + f"JM {removed_jm_id}; cannot call jc_replace_jm") + node.write_to_db() + elif any(d.uuid == removed_node.jm_device.get_id() for d in (node.remote_jm_devices or [])): + # node.jm_ids is this node's OWN redundancy set for its OWN JM + # -- but _connect_to_remote_jm_devs also connects to a SECOND + # source: whichever primary this node hosts as secondary/ + # tertiary pulls in THAT primary's jm_ids too (so the hublvol + # journal stays consistent with the primary it's replicating). + # A node reachable only via that second path never touches + # node.jm_ids at all, so the branch above never even looks at + # it, and its remote_jm_devices entry for the dead JM is left + # stale forever (2026-08-14 incident: exposed by a splice + # reshuffling who hosts whom -- a plain removal that never + # changes any hosting relationship never surfaces this gap, + # which is why the FIRST of two removals in the same test + # showed no symptom and the second, spliced one did). No + # "replacement" pick needed here, unlike the jm_ids branch -- + # this isn't a fixed-size redundancy slot, just a stale + # connection to drop; a plain refresh naturally excludes the + # now-removed JM since it can no longer be reached through + # either source. + node.remote_jm_devices = _connect_to_remote_jm_devs(node, node.jm_ids) + node.write_to_db() removed_node = db_controller.get_storage_node_by_id(removed_node.get_id()) for dev in removed_node.nvme_devices: @@ -3897,6 +4441,20 @@ def _finalize_node_removal(removed_node: StorageNode): removed_node = db_controller.get_storage_node_by_id(removed_node.get_id()) cluster = db_controller.get_cluster_by_id(removed_node.cluster_id) + # Case A/B (_teardown_replicas_of_primary, _relocate_replicas_hosted_on) + # already cleared this node's forward/back-reference fields as they + # relocated each side of the relationship elsewhere. lvstore_ports is + # the one piece of bookkeeping neither touches -- it isn't part of any + # relocation, just a port-reuse cache for THIS node's own restarts (see + # recreate_lvstore_on_non_leader) -- and by the time this function runs + # there won't be one: the node is about to flip to REMOVED for good. + # Left uncleared, `sn list`'s "LVS Ports" column keeps showing entries + # for a node with no SPDK process left to back them (2026-08-13, found + # live after a removal). + if removed_node.lvstore_ports: + removed_node.lvstore_ports = {} + removed_node.write_to_db() + if cluster.mode == "docker": logger.info("Leaving swarm...") try: @@ -5293,10 +5851,58 @@ def _check_ftt_allows_node_removal(node_id, db_controller): if jm_replication_active: not_online_count += 1 - if npcs == 1: + fd_on = cluster.enable_failure_domain and snode.failure_domain >= 0 + blocked_by_capacity = False + capacity_reason = "" + + if fd_on: + # Placement spreads a stripe's ndcs+npcs chunks as evenly as possible + # across the domains that actually exist. With fewer domains than + # chunks, at least one domain holds ceil((ndcs+npcs)/domains) chunks + # -- that many nodes can go down within a SINGLE domain for free + # (mirrors that domain's worst-case chunk contribution going down + # outright, already priced in), but once a domain hits that many + # down, it has maxed its contribution to the npcs risk budget and a + # DIFFERENT domain can only add a node if the combined risk across + # every affected domain (each capped at chunks_per_domain) still + # leaves room. This reduces to the familiar "up to npcs whole domains + # are free" rule when there are >= ndcs+npcs domains (chunks_per_domain + # == 1). See the analogous fdDrainGate in nodedrain_controller.go. + domains_available = len({n.failure_domain for n in snodes if n.failure_domain >= 0}) + domains_needed = ndcs + npcs + chunks_per_domain = -(-domains_needed // domains_available) if domains_available > 0 else domains_needed + + domain_down_counts: dict[int, int] = {} + for node in not_online_nodes: + if node.failure_domain >= 0: + domain_down_counts[node.failure_domain] = domain_down_counts.get(node.failure_domain, 0) + 1 + + my_domain = snode.failure_domain + my_domain_down = domain_down_counts.get(my_domain, 0) + + if my_domain_down < chunks_per_domain: + current_risk = sum(min(c, chunks_per_domain) for c in domain_down_counts.values()) + # jm_replication_active can't be attributed to a specific domain + # (the probe only says "some online node's journal is behind"), + # so treat it conservatively as always adding a fresh risk unit. + if jm_replication_active: + current_risk += 1 + if current_risk + 1 > npcs: + blocked_by_capacity = True + capacity_reason = ( + f"FTT={ft} (npcs={npcs}): cannot remove node in failure domain {my_domain}; " + f"{current_risk}/{npcs} failure-domain risk budget already committed " + f"({domains_available} domain(s) available, {chunks_per_domain} chunk(s)/domain worst case)" + f"{' (including in-progress journal replication)' if jm_replication_active else ''}" + ) + # else: this domain already holds >= chunks_per_domain down nodes -- + # it has maxed its contribution to the risk budget, so one more node + # in the SAME domain adds no additional risk. + elif npcs == 1: # FTT=1: no room at all if anything is already not online or journal replicating if not_online_count > 0: - return False, ( + blocked_by_capacity = True + capacity_reason = ( f"FTT=1 (npcs=1): cannot remove node, cluster already has " f"{len(not_online_nodes)} not-online node(s)" f"{' and journal replication in progress' if jm_replication_active else ''}" @@ -5306,53 +5912,59 @@ def _check_ftt_allows_node_removal(node_id, db_controller): if ft >= 2: # FTT=2: room for one not-online node, block if already have one+ if not_online_count >= 2: - return False, ( + blocked_by_capacity = True + capacity_reason = ( f"FTT=2 (npcs=2): cannot remove node, cluster already has " f"{len(not_online_nodes)} not-online node(s)" f"{' and journal replication in progress' if jm_replication_active else ''}" ) else: # npcs=2, ft=1: like FTT=2 for capacity, but additionally - # cannot remove both primary and its secondary + # cannot remove both primary and its secondary (checked below). if not_online_count >= 2: - return False, ( + blocked_by_capacity = True + capacity_reason = ( f"npcs=2/ft=1: cannot remove node, cluster already has " f"{len(not_online_nodes)} not-online node(s)" f"{' and journal replication in progress' if jm_replication_active else ''}" ) - # Check primary-secondary pair constraint: - # If the node being removed is a primary, check its secondary is online. - # If the node being removed is a secondary, check its primary is online. - for not_online_node in not_online_nodes: - # Is any not-online node the secondary of the node we're removing? - if snode.secondary_node_id == not_online_node.get_id(): - return False, ( - f"npcs=2/ft=1: cannot remove node {node_id}, " - f"its secondary {not_online_node.get_id()} is not online " - f"(status: {not_online_node.status})" - ) - if snode.tertiary_node_id == not_online_node.get_id(): - return False, ( - f"npcs=2/ft=1: cannot remove node {node_id}, " - f"its secondary {not_online_node.get_id()} is not online " - f"(status: {not_online_node.status})" - ) + if blocked_by_capacity: + return False, capacity_reason - # Is the node we're removing a secondary of any not-online primary? - for not_online_node in not_online_nodes: - if not_online_node.secondary_node_id == node_id: - return False, ( - f"npcs=2/ft=1: cannot remove node {node_id}, " - f"it is secondary of not-online primary {not_online_node.get_id()} " - f"(status: {not_online_node.status})" - ) - if not_online_node.tertiary_node_id == node_id: - return False, ( - f"npcs=2/ft=1: cannot remove node {node_id}, " - f"it is secondary of not-online primary {not_online_node.get_id()} " - f"(status: {not_online_node.status})" - ) + if npcs == 2 and ft == 1: + # npcs=2, ft=1: beyond the capacity cap above, cannot remove both a + # primary and its own secondary/tertiary at once -- a per-relationship + # constraint, orthogonal to failure domains. + for not_online_node in not_online_nodes: + # Is any not-online node the secondary of the node we're removing? + if snode.secondary_node_id == not_online_node.get_id(): + return False, ( + f"npcs=2/ft=1: cannot remove node {node_id}, " + f"its secondary {not_online_node.get_id()} is not online " + f"(status: {not_online_node.status})" + ) + if snode.tertiary_node_id == not_online_node.get_id(): + return False, ( + f"npcs=2/ft=1: cannot remove node {node_id}, " + f"its secondary {not_online_node.get_id()} is not online " + f"(status: {not_online_node.status})" + ) + + # Is the node we're removing a secondary of any not-online primary? + for not_online_node in not_online_nodes: + if not_online_node.secondary_node_id == node_id: + return False, ( + f"npcs=2/ft=1: cannot remove node {node_id}, " + f"it is secondary of not-online primary {not_online_node.get_id()} " + f"(status: {not_online_node.status})" + ) + if not_online_node.tertiary_node_id == node_id: + return False, ( + f"npcs=2/ft=1: cannot remove node {node_id}, " + f"it is secondary of not-online primary {not_online_node.get_id()} " + f"(status: {not_online_node.status})" + ) return True, "" @@ -5720,7 +6332,6 @@ def shutdown_storage_node(node_id, force=False, keep_auto_restart=False, continue if task.function_name in [ JobSchedule.FN_DEV_MIG, - JobSchedule.FN_FAILED_DEV_MIG, JobSchedule.FN_NEW_DEV_MIG, ]: task.canceled = True @@ -5774,6 +6385,28 @@ def shutdown_storage_node(node_id, force=False, keep_auto_restart=False, "Loop 2: peer-side detach pass raised %s (continuing to kill)", e) + if snode.hublvol: + # Disconnect hublvol from secondary + if snode.secondary_node_id: + sec_node = db_controller.get_storage_node_by_id(snode.secondary_node_id) + if sec_node.status == StorageNode.STATUS_ONLINE: + logger.info("Disconnecting hublvol from %s", sec_node.get_id()) + try: + sec_node.rpc_client().bdev_nvme_detach_controller(snode.hublvol.bdev_name) + except Exception as e: + logger.warning("Disconnecting hublvol failed: %s", e) + + # Disconnect hublvol from tertiary + if snode.tertiary_node_id: + ter_node = db_controller.get_storage_node_by_id(snode.tertiary_node_id) + if ter_node.status == StorageNode.STATUS_ONLINE: + logger.info("Disconnecting hublvol from %s", ter_node.get_id()) + try: + ter_node.rpc_client().bdev_nvme_detach_controller(snode.hublvol.bdev_name) + except Exception as e: + logger.warning("Disconnecting hublvol failed: %s", e) + + # Step 5: hard-kill SPDK. Same code path as the existing --force # shutdown — peers see the TCP drop and host multipath retries on # surviving paths. Any IO inside SPDK at this instant is lost; @@ -9569,7 +10202,27 @@ def add_lvol_thread(lvol, snode: StorageNode, lvol_ana_state="optimized"): logger.error(msg) return False, msg - # Use per-lvstore port for this lvol's lvstore + # Use per-lvstore port for this lvol's lvstore. get_lvol_subsys_port()'s + # fallback to snode.lvol_subsys_port is only correct for lvol.lvs_name == + # snode.lvstore (this node's OWN primary, which legitimately has no + # lvstore_ports entry -- it uses the plain node-level port). For any + # OTHER lvs_name, a missing entry means the relocation that assigned + # snode this non-leader role hasn't finished committing lvstore_ports + # yet -- snode here can be a stale, caller-held object (same hazard as + # the in_deletion check above). Silently falling back would register + # the listener on snode's OWN leader port instead of lvol.lvs_name's + # real one (2026-08-18: raced a node-removal relocation live, leaving + # two lvols' secondaries listening on the wrong port indefinitely, with + # nothing to ever revisit or correct it). Re-fetch once and refuse + # rather than guess; the next lvol_monitor repair cycle retries. + if lvol.lvs_name != snode.lvstore and lvol.lvs_name not in snode.lvstore_ports: + snode = db_controller.get_storage_node_by_id(snode.get_id()) + if lvol.lvs_name not in snode.lvstore_ports: + msg = (f"{snode.get_id()} has no lvstore_ports entry for " + f"{lvol.lvs_name} yet; refusing to add a listener for " + f"{lvol.nqn} on a guessed port") + logger.warning(msg) + return False, msg listener_port = snode.get_lvol_subsys_port(lvol.lvs_name) for iface in snode.data_nics: if iface.ip4_address and lvol.fabric == iface.trtype.lower(): @@ -9814,10 +10467,7 @@ def get_node_jm_names(current_node: StorageNode, remote_node=None): continue jm_dev = DBController().get_jm_device_by_id(jm_id) - if jm_dev.override_name_on_node and current_node.get_id() in jm_dev.override_name_on_node: - jm_list.append(f"remote_{jm_dev.override_name_on_node[current_node.get_id()]}n1") - else: - jm_list.append(f"remote_{jm_dev.jm_bdev}n1") + jm_list.append(f"remote_{jm_dev.jm_bdev}n1") return jm_list[:current_node.ha_jm_count] @@ -9828,6 +10478,18 @@ def get_secondary_nodes(current_node: StorageNode, exclude_ids=None, removed_nod db_controller = DBController() cluster = db_controller.get_cluster_by_id(current_node.cluster_id) all_nodes = db_controller.get_storage_nodes_by_cluster_id(current_node.cluster_id) + # Group by failure domain (stable sort, preserves DB order within each + # domain) before scanning candidates. The "first valid candidate after my + # own position" logic below skips same-domain nodes as forbidden, so on an + # arbitrary/interleaved node order it can still land back on a same-domain + # pick once every other domain's nodes are already claimed -- purely an + # artifact of iteration order, not availability (verified by simulation: + # ~1 in 5 arbitrary orderings produces an avoidable same-domain pick even + # when a fully domain-disjoint assignment exists). Grouping first removes + # that sensitivity: every node's forward scan cleanly skips past the rest + # of its own domain into the next one. A no-op when FD is disabled (all + # nodes share the same failure_domain, so the sort is order-preserving). + all_nodes = sorted(all_nodes, key=lambda n: n.failure_domain) if len(all_nodes) == 2: for node in all_nodes: if node.get_id() != current_node.get_id() and node.get_id() not in exclude_ids: @@ -9884,6 +10546,81 @@ def _candidates(forbidden_fds, forbidden_labels): return [] +def splice_stranded_secondary(stranded_node) -> bool: + """Fold a node get_secondary_nodes() could not place into the pairing + graph already built by the in-progress cluster_activate() pass. + + get_secondary_nodes() walks primaries in order, greedily picking the most + domain/host-disjoint unclaimed candidate for each. That greedy walk has no + mechanism to guarantee the resulting secondary_node_id/lvstore_stack_secondary + edges close a cycle spanning every online node: it can close a cycle over a + strict subset and leave the remaining node(s) with zero unclaimed + candidates, even though a perfect pairing trivially exists whenever there + are 2+ online nodes (observed 2026-08-03: 12 nodes across 3 failure + domains formed an 11-node cycle, stranding the 12th and aborting + activation). + + Rather than reworking the greedy walk into a global matching solver, this + repairs the one failure mode it has: pick any already-formed edge P->X + (P.secondary_node_id == X.get_id()) and splice the stranded node in + between, P->stranded->X. This always succeeds as long as at least one + edge already exists (guaranteed once 2+ pairings have been made this + activation pass) and turns the cycle that edge belongs to into one that + also covers the stranded node, without disturbing any other node. Prefers + an edge where both P and X differ from the stranded node's failure domain + (falling back to a host-disjoint-only edge), mirroring get_secondary_nodes' + own anti-affinity tiering. + """ + db_controller = DBController() + all_nodes = db_controller.get_storage_nodes_by_cluster_id(stranded_node.cluster_id) + # Deterministic tie-breaking among equally domain-scored edges -- see + # get_secondary_nodes for why this sort matters. + all_nodes = sorted(all_nodes, key=lambda n: n.failure_domain) + edges = [n for n in all_nodes if n.secondary_node_id and n.get_id() != stranded_node.get_id()] + + def _host_disjoint(p, x): + return p.mgmt_ip != stranded_node.mgmt_ip and x.mgmt_ip != stranded_node.mgmt_ip + + def _domain_mismatch_score(p, x): + if stranded_node.failure_domain < 0: + return 0 + return sum(1 for n in (p, x) if n.failure_domain != stranded_node.failure_domain) + + best = None + best_score = -1 + for p in edges: + x = db_controller.get_storage_node_by_id(p.secondary_node_id) + if not x or x.get_id() == stranded_node.get_id() or not _host_disjoint(p, x): + continue + score = _domain_mismatch_score(p, x) + if score > best_score: + best_score, best = score, (p, x) + + if best is None: + return False + + p, x = best + logger.warning( + "get_secondary_nodes found no candidate for node %s; splicing it into " + "the existing pairing %s -> %s (domain-mismatch score %d/2).", + stranded_node.get_id(), p.get_id(), x.get_id(), best_score) + + p = db_controller.get_storage_node_by_id(p.get_id()) + p.secondary_node_id = stranded_node.get_id() + p.write_to_db() + + stranded_node = db_controller.get_storage_node_by_id(stranded_node.get_id()) + stranded_node.lvstore_stack_secondary = p.get_id() + stranded_node.secondary_node_id = x.get_id() + stranded_node.write_to_db() + + x = db_controller.get_storage_node_by_id(x.get_id()) + x.lvstore_stack_secondary = stranded_node.get_id() + x.write_to_db() + + return True + + def get_secondary_nodes_2(current_node: StorageNode, exclude_ids=None, exclude_mgmt_ips=None, exclude_failure_domains=None, exclude_physical_labels=None): """Get candidate nodes for second secondary assignment (dual fault tolerance). @@ -9911,6 +10648,9 @@ def get_secondary_nodes_2(current_node: StorageNode, exclude_ids=None, exclude_m db_controller = DBController() cluster = db_controller.get_cluster_by_id(current_node.cluster_id) all_nodes = db_controller.get_storage_nodes_by_cluster_id(current_node.cluster_id) + # See get_secondary_nodes for why this sort matters: it removes the + # pairing algorithm's sensitivity to arbitrary/interleaved node order. + all_nodes = sorted(all_nodes, key=lambda n: n.failure_domain) if len(all_nodes) == 2: for node in all_nodes: if node.get_id() != current_node.get_id() and node.get_id() not in exclude_ids: @@ -9974,6 +10714,89 @@ def _candidates(forbidden_fds, forbidden_labels): return [] +def splice_stranded_tertiary(stranded_node) -> bool: + """Tertiary-assignment counterpart to splice_stranded_secondary. + + get_secondary_nodes_2()'s greedy walk has the identical dead-end risk as + get_secondary_nodes(): it can close a tertiary-pairing cycle over a + subset of online nodes and strand the rest, even though a valid + assignment exists — this can surface on any cluster with + max_fault_tolerance >= 2 (e.g. a 2+2 layout), the same way + splice_stranded_secondary's bug surfaced on the plain secondary pass. + + Splices the stranded node into an already-formed tertiary edge P->X + (P.tertiary_node_id == X.get_id()), same idea as the secondary case: + P->stranded->X. The extra wrinkle here is that a tertiary must be + host-disjoint from BOTH a primary and that primary's OWN secondary (a + single host outage must not take out two of the four HA journal members) + — so splicing changes what "valid" means on both sides of the edge, and + each side is re-checked against the other's current secondary_node_id, + not just against each other. + """ + db_controller = DBController() + all_nodes = db_controller.get_storage_nodes_by_cluster_id(stranded_node.cluster_id) + # Deterministic tie-breaking among equally domain-scored edges -- see + # get_secondary_nodes for why this sort matters. + all_nodes = sorted(all_nodes, key=lambda n: n.failure_domain) + by_id = {n.get_id(): n for n in all_nodes} + stranded_sec = by_id.get(stranded_node.secondary_node_id) if stranded_node.secondary_node_id else None + + def _valid_tertiary(primary, primary_sec, candidate): + if candidate.get_id() == primary.get_id(): + return False + if candidate.mgmt_ip == primary.mgmt_ip: + return False + if primary_sec and candidate.mgmt_ip == primary_sec.mgmt_ip: + return False + return True + + def _domain_mismatch_score(*nodes): + if stranded_node.failure_domain < 0: + return 0 + return sum(1 for n in nodes if n.failure_domain != stranded_node.failure_domain) + + edges = [n for n in all_nodes if n.tertiary_node_id and n.get_id() != stranded_node.get_id()] + + best = None + best_score = -1 + for p in edges: + x = by_id.get(p.tertiary_node_id) + if not x or x.get_id() == stranded_node.get_id(): + continue + p_sec = by_id.get(p.secondary_node_id) if p.secondary_node_id else None + if not _valid_tertiary(p, p_sec, stranded_node): + continue + if not _valid_tertiary(stranded_node, stranded_sec, x): + continue + score = _domain_mismatch_score(p, x) + if score > best_score: + best_score, best = score, (p, x) + + if best is None: + return False + + p, x = best + logger.warning( + "get_secondary_nodes_2 found no candidate for node %s; splicing it into " + "the existing tertiary pairing %s -> %s (domain-mismatch score %d/2).", + stranded_node.get_id(), p.get_id(), x.get_id(), best_score) + + p = db_controller.get_storage_node_by_id(p.get_id()) + p.tertiary_node_id = stranded_node.get_id() + p.write_to_db() + + stranded_node = db_controller.get_storage_node_by_id(stranded_node.get_id()) + stranded_node.lvstore_stack_tertiary = p.get_id() + stranded_node.tertiary_node_id = x.get_id() + stranded_node.write_to_db() + + x = db_controller.get_storage_node_by_id(x.get_id()) + x.lvstore_stack_tertiary = stranded_node.get_id() + x.write_to_db() + + return True + + def create_lvstore(snode: StorageNode, ndcs, npcs, distr_bs, distr_chunk_bs, page_size_in_blocks, max_size): db_controller = DBController() cluster = db_controller.get_cluster_by_id(snode.cluster_id) diff --git a/simplyblock_web/api/v2/cluster/storage_node/__init__.py b/simplyblock_web/api/v2/cluster/storage_node/__init__.py index 1f68785187..f25ca6ab10 100644 --- a/simplyblock_web/api/v2/cluster/storage_node/__init__.py +++ b/simplyblock_web/api/v2/cluster/storage_node/__init__.py @@ -115,18 +115,28 @@ def get(cluster: Cluster, storage_node: StorageNode): @instance_api.delete('/', name='clusters:storage-nodes:delete') def delete( cluster: Cluster, storage_node: StorageNode, force_remove: bool = False, force_migrate: bool = False, force_delete: bool = False) -> Response: + # remove_storage_node's precondition gates (FTT, failure-domain balance, + # replica-relocation feasibility, ...) reject via `return False` rather + # than a specific exception (see the reason string it logs via + # logger.error) -- tracked as a bigger refactor, not done here. But an + # unhandled ValueError with no registered handler becomes an HTTP 500, + # and 500 is on the operator's *retryable* list (webapi/errorclass.go) + # -- so a permanently-infeasible removal (e.g. would unbalance failure + # domains) was retried forever instead of the operator resuming the + # node it had already suspended and failing cleanly (2026-08-13 + # incident). 400 is correctly classified as non-retryable there. none_or_false = storage_node_ops.remove_storage_node( storage_node.get_id(), force_remove=force_remove, force_migrate=force_migrate ) if none_or_false == False: # noqa - raise ValueError('Failed to remove storage node') + raise HTTPException(400, 'Failed to remove storage node') if force_delete: none_or_false = storage_node_ops.delete_storage_node( storage_node.get_id(), force=force_delete ) if none_or_false == False: # noqa - raise ValueError('Failed to delete storage node') + raise HTTPException(400, 'Failed to delete storage node') return Response(status_code=204) diff --git a/simplyblock_web/api/v2/metrics.py b/simplyblock_web/api/v2/metrics.py index 98a8afa020..74ff2d1884 100644 --- a/simplyblock_web/api/v2/metrics.py +++ b/simplyblock_web/api/v2/metrics.py @@ -20,6 +20,7 @@ from fastapi.responses import Response from prometheus_client import CONTENT_TYPE_LATEST, CollectorRegistry, generate_latest from prometheus_client.core import CounterMetricFamily, GaugeMetricFamily, Metric +from prometheus_client.registry import Collector from simplyblock_core.db_controller import DBController from simplyblock_core.models.stats import CpuStats, ReactorStats, ThreadStats @@ -257,7 +258,7 @@ def _cpu_families(entries: Iterable[tuple[list[str], CpuStats]]) -> Iterator[Met yield family -class SimplyblockCollector: +class SimplyblockCollector(Collector): """Builds the full metric set from FoundationDB on every scrape.""" def collect(self) -> Iterator[Metric]: diff --git a/tests/unit/test_failure_domain.py b/tests/unit/test_failure_domain.py index 94eab3b191..e16d08bb07 100644 --- a/tests/unit/test_failure_domain.py +++ b/tests/unit/test_failure_domain.py @@ -24,6 +24,7 @@ All external dependencies (FDB, RPC) are mocked. """ +import random import unittest from unittest.mock import MagicMock, patch @@ -152,6 +153,98 @@ def test_enabled_falls_back_when_no_other_domain(self, MockDBCtrl): assert any("falling back" in m for m in cm.output) +# =========================================================================== +# 2b. splice_stranded_secondary +# +# get_secondary_nodes()'s greedy walk can close a pairing cycle over a strict +# subset of online nodes and strand the rest with zero candidates, even though +# a perfect pairing exists (observed 2026-08-03: 12 nodes / 3 domains formed +# an 11-node cycle, aborting activation for the 12th). splice_stranded_secondary +# repairs this by inserting the stranded node into an already-formed edge. +# =========================================================================== + +class TestSpliceStrandedSecondary(unittest.TestCase): + + def _mock_db(self, cluster, nodes): + mock_db = MagicMock() + by_id = {n.get_id(): n for n in nodes} + mock_db.get_cluster_by_id.return_value = cluster + mock_db.get_storage_nodes_by_cluster_id.return_value = nodes + mock_db.get_storage_node_by_id.side_effect = lambda nid: by_id.get(nid) + return mock_db + + @staticmethod + def _stub_writes(*nodes): + for n in nodes: + n.write_to_db = MagicMock() + + @patch("simplyblock_core.storage_node_ops.DBController") + def test_splices_into_existing_edge(self, MockDBCtrl): + import simplyblock_core.storage_node_ops as ops + # p -> x is an existing pairing from earlier in the activation pass. + p = _node("p", "10.0.0.1", failure_domain=0) + x = _node("x", "10.0.0.2", failure_domain=1) + p.secondary_node_id = x.get_id() + s = _node("s", "10.0.0.3", failure_domain=2) + MockDBCtrl.return_value = self._mock_db(_cluster(True), [p, x, s]) + self._stub_writes(p, x, s) + + assert ops.splice_stranded_secondary(s) is True + # p -> s -> x: p now points at s, s sits between p and x. + assert p.secondary_node_id == "s" + assert s.lvstore_stack_secondary == "p" + assert s.secondary_node_id == "x" + assert x.lvstore_stack_secondary == "s" + + @patch("simplyblock_core.storage_node_ops.DBController") + def test_prefers_edge_domain_disjoint_on_both_ends(self, MockDBCtrl): + import simplyblock_core.storage_node_ops as ops + s = _node("s", "10.0.0.9", failure_domain=0) + + # Same-domain edge (worse fit: 0/2 mismatch against s's domain). + bad_p = _node("bad_p", "10.0.0.1", failure_domain=0) + bad_x = _node("bad_x", "10.0.0.2", failure_domain=0) + bad_p.secondary_node_id = bad_x.get_id() + + # Domain-disjoint-on-both-ends edge (best fit: 2/2 mismatch). + good_p = _node("good_p", "10.0.0.3", failure_domain=1) + good_x = _node("good_x", "10.0.0.4", failure_domain=2) + good_p.secondary_node_id = good_x.get_id() + + nodes = [s, bad_p, bad_x, good_p, good_x] + MockDBCtrl.return_value = self._mock_db(_cluster(True), nodes) + self._stub_writes(*nodes) + + assert ops.splice_stranded_secondary(s) is True + assert good_p.secondary_node_id == "s" + assert s.secondary_node_id == "good_x" + assert bad_p.secondary_node_id == "bad_x" # untouched + + @patch("simplyblock_core.storage_node_ops.DBController") + def test_returns_false_when_no_edge_exists_yet(self, MockDBCtrl): + import simplyblock_core.storage_node_ops as ops + s = _node("s", "10.0.0.9", failure_domain=0) + other = _node("other", "10.0.0.1", failure_domain=1) # no pairing yet + MockDBCtrl.return_value = self._mock_db(_cluster(True), [s, other]) + self._stub_writes(s, other) + + assert ops.splice_stranded_secondary(s) is False + + @patch("simplyblock_core.storage_node_ops.DBController") + def test_skips_edge_not_host_disjoint_from_stranded(self, MockDBCtrl): + import simplyblock_core.storage_node_ops as ops + # x shares mgmt_ip with the stranded node -- splicing there would + # violate host-disjointness, so this edge must be skipped entirely. + s = _node("s", "10.0.0.5", failure_domain=0) + p = _node("p", "10.0.0.1", failure_domain=1) + x = _node("x", "10.0.0.5", failure_domain=2) + p.secondary_node_id = x.get_id() + MockDBCtrl.return_value = self._mock_db(_cluster(True), [s, p, x]) + self._stub_writes(s, p, x) + + assert ops.splice_stranded_secondary(s) is False + + # =========================================================================== # 3. get_secondary_nodes_2 (tertiary) # =========================================================================== @@ -199,6 +292,141 @@ def test_falls_back_when_only_shared_domains(self, MockDBCtrl): assert any("falling back" in m for m in cm.output) +# =========================================================================== +# 3b. splice_stranded_tertiary +# +# get_secondary_nodes_2() has the identical greedy-walk dead-end risk as +# get_secondary_nodes() (see TestSpliceStrandedSecondary above), reachable on +# any cluster with max_fault_tolerance >= 2 (e.g. a 2+2 layout). The extra +# wrinkle: a tertiary must be host-disjoint from both a primary and that +# primary's OWN secondary, so a valid splice must be re-checked against each +# side's current secondary_node_id, not just against each other. +# =========================================================================== + +class TestSpliceStrandedTertiary(unittest.TestCase): + + def _mock_db(self, cluster, nodes): + mock_db = MagicMock() + by_id = {n.get_id(): n for n in nodes} + mock_db.get_cluster_by_id.return_value = cluster + mock_db.get_storage_nodes_by_cluster_id.return_value = nodes + mock_db.get_storage_node_by_id.side_effect = lambda nid: by_id.get(nid) + return mock_db + + @staticmethod + def _stub_writes(*nodes): + for n in nodes: + n.write_to_db = MagicMock() + + @patch("simplyblock_core.storage_node_ops.DBController") + def test_splices_into_existing_tertiary_edge(self, MockDBCtrl): + import simplyblock_core.storage_node_ops as ops + # p -> x is an existing tertiary edge. s is stranded, with its own + # secondary (s_sec) on a distinct host from everyone else involved. + p_sec = _node("p_sec", "10.0.0.10", failure_domain=1) + p = _node("p", "10.0.0.1", failure_domain=0) + p.secondary_node_id = "p_sec" + x = _node("x", "10.0.0.2", failure_domain=1) + p.tertiary_node_id = "x" + + s_sec = _node("s_sec", "10.0.0.20", failure_domain=1) + s = _node("s", "10.0.0.3", failure_domain=2) + s.secondary_node_id = "s_sec" + + nodes = [p, p_sec, x, s, s_sec] + MockDBCtrl.return_value = self._mock_db(_cluster(True), nodes) + self._stub_writes(*nodes) + + assert ops.splice_stranded_tertiary(s) is True + # p -> s -> x: p now points at s, s sits between p and x. + assert p.tertiary_node_id == "s" + assert s.lvstore_stack_tertiary == "p" + assert s.tertiary_node_id == "x" + assert x.lvstore_stack_tertiary == "s" + + @patch("simplyblock_core.storage_node_ops.DBController") + def test_returns_false_when_no_edge_exists_yet(self, MockDBCtrl): + import simplyblock_core.storage_node_ops as ops + s = _node("s", "10.0.0.3", failure_domain=0) + other = _node("other", "10.0.0.1", failure_domain=1) # no tertiary edge yet + MockDBCtrl.return_value = self._mock_db(_cluster(True), [s, other]) + self._stub_writes(s, other) + + assert ops.splice_stranded_tertiary(s) is False + + @patch("simplyblock_core.storage_node_ops.DBController") + def test_rejects_edge_when_stranded_shares_host_with_primarys_secondary(self, MockDBCtrl): + import simplyblock_core.storage_node_ops as ops + # s sits on the same host as p's secondary (p_sec) -- s can never + # become p's tertiary, so the only existing edge must be rejected. + p_sec = _node("p_sec", "10.0.0.9", failure_domain=1) + p = _node("p", "10.0.0.1", failure_domain=0) + p.secondary_node_id = "p_sec" + x = _node("x", "10.0.0.2", failure_domain=1) + p.tertiary_node_id = "x" + + s = _node("s", "10.0.0.9", failure_domain=2) # same mgmt_ip as p_sec + s_sec = _node("s_sec", "10.0.0.20", failure_domain=1) + s.secondary_node_id = "s_sec" + + nodes = [p, p_sec, x, s, s_sec] + MockDBCtrl.return_value = self._mock_db(_cluster(True), nodes) + self._stub_writes(*nodes) + + assert ops.splice_stranded_tertiary(s) is False + + @patch("simplyblock_core.storage_node_ops.DBController") + def test_rejects_edge_when_x_shares_host_with_stranded_secondary(self, MockDBCtrl): + import simplyblock_core.storage_node_ops as ops + # x sits on the same host as s's secondary (s_sec) -- x can never + # become s's tertiary, so the only existing edge must be rejected. + p_sec = _node("p_sec", "10.0.0.9", failure_domain=1) + p = _node("p", "10.0.0.1", failure_domain=0) + p.secondary_node_id = "p_sec" + x = _node("x", "10.0.0.30", failure_domain=1) + p.tertiary_node_id = "x" + + s_sec = _node("s_sec", "10.0.0.30", failure_domain=1) # same mgmt_ip as x + s = _node("s", "10.0.0.3", failure_domain=2) + s.secondary_node_id = "s_sec" + + nodes = [p, p_sec, x, s, s_sec] + MockDBCtrl.return_value = self._mock_db(_cluster(True), nodes) + self._stub_writes(*nodes) + + assert ops.splice_stranded_tertiary(s) is False + + @patch("simplyblock_core.storage_node_ops.DBController") + def test_prefers_edge_domain_disjoint_on_both_ends(self, MockDBCtrl): + import simplyblock_core.storage_node_ops as ops + s_sec = _node("s_sec", "10.0.0.90", failure_domain=9) + s = _node("s", "10.0.0.9", failure_domain=0) + s.secondary_node_id = "s_sec" + + # Same-domain edge (worse fit: 0/2 mismatch against s's domain). + bad_p_sec = _node("bad_p_sec", "10.0.0.11", failure_domain=9) + bad_p = _node("bad_p", "10.0.0.1", failure_domain=0) + bad_p.secondary_node_id = "bad_p_sec" + bad_x = _node("bad_x", "10.0.0.2", failure_domain=0) + bad_p.tertiary_node_id = "bad_x" + + # Domain-disjoint-on-both-ends edge (best fit: 2/2 mismatch). + good_p_sec = _node("good_p_sec", "10.0.0.13", failure_domain=9) + good_p = _node("good_p", "10.0.0.3", failure_domain=1) + good_p.secondary_node_id = "good_p_sec" + good_x = _node("good_x", "10.0.0.4", failure_domain=2) + good_p.tertiary_node_id = "good_x" + + nodes = [s, s_sec, bad_p, bad_p_sec, bad_x, good_p, good_p_sec, good_x] + MockDBCtrl.return_value = self._mock_db(_cluster(True), nodes) + self._stub_writes(*nodes) + + assert ops.splice_stranded_tertiary(s) is True + assert good_p.tertiary_node_id == "s" + assert s.tertiary_node_id == "good_x" + assert bad_p.tertiary_node_id == "bad_x" # untouched + + # =========================================================================== # 4. get_sorted_ha_jms # =========================================================================== @@ -645,5 +873,124 @@ def test_tertiary_excludes_secondary_label(self, MockDBCtrl): assert "c2" not in result # primary's label +# =========================================================================== +# 9. Domain-grouped ordering eliminates pairing order-sensitivity +# +# Two independent things had to be fixed for the pairing loop to stop being +# order-sensitive: +# +# 1. get_secondary_nodes/get_secondary_nodes_2/splice_stranded_secondary/ +# splice_stranded_tertiary each fetch their own candidate list fresh via +# db_controller.get_storage_nodes_by_cluster_id() and scan/score it in +# that order -- so their domain-disjointness used to depend on whatever +# order the DB happened to return nodes in, regardless of caller. All +# four now sort their fetched node list by failure_domain first. +# 2. Even with (1) fixed, _cluster_activate's own pairing loop still +# determines which primary is processed first, and once a domain-size +# imbalance forces splice-repair, the repair works off whatever partial +# assignment already exists -- so the CALLER's processing order still +# changes the outcome. _cluster_activate now also sorts its local +# `snodes` by failure_domain before the loop. +# +# Verified here by running the full secondary+tertiary pairing sequence with +# the DB mock returning nodes in an arbitrary (shuffled) order -- proving (1) +# -- while the caller-side loop mirrors _cluster_activate's own domain sort +# -- proving (1)+(2) together give a fully order-independent result. +# =========================================================================== + +class TestDomainGroupedCandidateScanOrderIndependence(unittest.TestCase): + + def _mock_db(self, cluster, nodes): + mock_db = MagicMock() + by_id = {n.get_id(): n for n in nodes} + mock_db.get_cluster_by_id.return_value = cluster + mock_db.get_storage_nodes_by_cluster_id.return_value = nodes + mock_db.get_storage_node_by_id.side_effect = lambda nid: by_id.get(nid) + return mock_db + + @staticmethod + def _build_nodes(domain_sizes): + nodes = [] + idx = 0 + for fd, size in enumerate(domain_sizes, start=1): + for _ in range(size): + idx += 1 + nodes.append(_node(f"n{idx}", f"10.0.0.{idx}", failure_domain=fd)) + return nodes + + def _run_pairing(self, db_order_nodes, cluster): + """Mirrors _cluster_activate's secondary+tertiary pairing loop + (cluster_ops.py), including its own failure_domain sort of the + processing order. `db_order_nodes` is deliberately left in an + arbitrary order to represent whatever get_storage_nodes_by_cluster_id + naturally returns -- proving get_secondary_nodes/_2's own internal + sort is what keeps candidate scanning domain-grouped regardless.""" + import simplyblock_core.storage_node_ops as ops + by_id = {n.get_id(): n for n in db_order_nodes} + for n in db_order_nodes: + n.write_to_db = MagicMock() + + processing_order = sorted(db_order_nodes, key=lambda n: n.failure_domain) + + with patch("simplyblock_core.storage_node_ops.DBController", + return_value=self._mock_db(cluster, db_order_nodes)): + for snode in processing_order: + secs = ops.get_secondary_nodes(snode) + if secs: + snode.secondary_node_id = secs[0] + by_id[secs[0]].lvstore_stack_secondary = snode.get_id() + else: + assert ops.splice_stranded_secondary(snode), f"{snode.get_id()} stranded on secondary" + + used_tertiary = [] + for snode in processing_order: + sec = by_id[snode.secondary_node_id] + t2 = ops.get_secondary_nodes_2( + snode, + exclude_ids=[snode.secondary_node_id] + used_tertiary, + exclude_mgmt_ips=[sec.mgmt_ip], + exclude_failure_domains=[sec.failure_domain], + exclude_physical_labels=[sec.physical_label], + ) + if t2: + snode.tertiary_node_id = t2[0] + by_id[t2[0]].lvstore_stack_tertiary = snode.get_id() + used_tertiary.append(t2[0]) + else: + assert ops.splice_stranded_tertiary(snode), f"{snode.get_id()} stranded on tertiary" + used_tertiary.append(snode.tertiary_node_id) + + conflicts = 0 + for n in db_order_nodes: + sec = by_id[n.secondary_node_id] + ter = by_id[n.tertiary_node_id] + if len({n.failure_domain, sec.failure_domain, ter.failure_domain}) != 3: + conflicts += 1 + return conflicts + + def test_equal_domains_conflict_free_across_many_arbitrary_db_orders(self): + cluster = _cluster(True, distr_npcs=2) + cluster.distr_ndcs = 2 + for seed in range(50): + nodes = self._build_nodes([4, 4, 4]) + random.Random(seed).shuffle(nodes) # arbitrary "DB return" order + conflicts = self._run_pairing(nodes, cluster) + assert conflicts == 0, f"seed={seed} produced {conflicts} conflicts" + + def test_unequal_domains_conflict_count_is_deterministic(self): + # 4/5/4: a full zero-conflict assignment is mathematically impossible + # (domains aren't equal size), but the combined sort (candidate scan + # + processing order) makes the resulting conflict count the same + # regardless of the arbitrary DB return order. + cluster = _cluster(True, distr_npcs=2) + cluster.distr_ndcs = 2 + counts = set() + for seed in range(20): + nodes = self._build_nodes([4, 5, 4]) + random.Random(seed).shuffle(nodes) + counts.add(self._run_pairing(nodes, cluster)) + assert len(counts) == 1, f"expected one consistent conflict count, got {counts}" + + if __name__ == "__main__": unittest.main() diff --git a/tests/unit/test_fd_topology_policy.py b/tests/unit/test_fd_topology_policy.py index f8540d977a..46478d0c3f 100644 --- a/tests/unit/test_fd_topology_policy.py +++ b/tests/unit/test_fd_topology_policy.py @@ -178,6 +178,41 @@ def test_unset_domain_ignored(self): self.assertIsNone(planner.fd_balance_violation({-1: 7, 0: 2, 1: 2})) +# --------------------------------------------------------------------------- +# planner.fd_activation_domain_count_violation +# --------------------------------------------------------------------------- + +class TestFdActivationDomainCount(unittest.TestCase): + + def test_npcs1_two_domains_violates(self): + self.assertIsNotNone( + planner.fd_activation_domain_count_violation(1, 2)) + + def test_npcs1_three_domains_ok(self): + self.assertIsNone( + planner.fd_activation_domain_count_violation(1, 3)) + + def test_npcs1_one_domain_violates(self): + self.assertIsNotNone( + planner.fd_activation_domain_count_violation(1, 1)) + + def test_npcs2_two_domains_violates(self): + self.assertIsNotNone( + planner.fd_activation_domain_count_violation(2, 2)) + + def test_npcs2_three_domains_violates(self): + self.assertIsNotNone( + planner.fd_activation_domain_count_violation(2, 3)) + + def test_npcs2_four_domains_ok(self): + self.assertIsNone( + planner.fd_activation_domain_count_violation(2, 4)) + + def test_npcs2_more_than_four_domains_ok(self): + self.assertIsNone( + planner.fd_activation_domain_count_violation(2, 6)) + + # --------------------------------------------------------------------------- # preconditions: add / remove / current admission # --------------------------------------------------------------------------- diff --git a/tests/unit/test_ftt_protection.py b/tests/unit/test_ftt_protection.py index f63ffe162c..65c95cbb76 100644 --- a/tests/unit/test_ftt_protection.py +++ b/tests/unit/test_ftt_protection.py @@ -23,7 +23,7 @@ # Helpers # --------------------------------------------------------------------------- -def _cluster(ha_type="ha", npcs=1, ndcs=2, ft=1, rebalancing=False): +def _cluster(ha_type="ha", npcs=1, ndcs=2, ft=1, rebalancing=False, enable_failure_domain=False): cl = Cluster() cl.uuid = "cluster-1" cl.ha_type = ha_type @@ -31,12 +31,14 @@ def _cluster(ha_type="ha", npcs=1, ndcs=2, ft=1, rebalancing=False): cl.distr_ndcs = ndcs cl.max_fault_tolerance = ft cl.is_re_balancing = rebalancing + cl.enable_failure_domain = enable_failure_domain cl.status = Cluster.STATUS_ACTIVE return cl def _node(node_id, status=StorageNode.STATUS_ONLINE, cluster_id="cluster-1", - secondary_id="", secondary_id_2="", jm_vuid=8881, lvstore="LVS_1"): + secondary_id="", secondary_id_2="", jm_vuid=8881, lvstore="LVS_1", + failure_domain=-1): n = MagicMock(spec=StorageNode) n.uuid = node_id n.get_id = MagicMock(return_value=node_id) @@ -47,6 +49,7 @@ def _node(node_id, status=StorageNode.STATUS_ONLINE, cluster_id="cluster-1", n.jm_vuid = jm_vuid n.lvstore = lvstore n.mgmt_ip = f"10.0.0.{hash(node_id) % 256}" + n.failure_domain = failure_domain # rpc_client mock: journal replication not active by default rpc = MagicMock() rpc.bdev_lvol_get_lvstores = MagicMock(return_value=[{"name": lvstore}]) @@ -573,6 +576,281 @@ def test_jm_replication_counts_but_pair_still_checked(self): self.assertFalse(allowed) +# --------------------------------------------------------------------------- +# Failure-domain-aware capacity check +# +# With FD enabled, the risk budget is npcs, spent per domain at a rate of +# min(nodes_down_in_that_domain, chunks_per_domain), where chunks_per_domain +# = ceil((ndcs+npcs)/domains_available). A domain that already has +# chunks_per_domain nodes down has maxed its contribution -- further nodes in +# THAT SAME domain are free -- but a node in a domain that hasn't maxed out +# yet is only allowed if the combined risk across every affected domain still +# leaves room in the npcs budget. This mirrors the operator's fdDrainGate +# (nodedrain_controller.go) and reduces to "up to npcs whole domains are +# free" when there are >= ndcs+npcs domains (chunks_per_domain == 1). +# +# Confirmed against the backend team's stated requirements (2026-08, Dmitrii +# Iakovlev): for a 2+2 layout, +# - 2 failure domains: safe combos are "1 whole FD down" (any node count +# within it) OR "1 node in each of the 2 FDs" -- nothing beyond that. +# - 3 failure domains: only 1 FD may be fully down, not 2. +# - 4 failure domains: 2 FDs may be fully down (the well-provisioned case). +# --------------------------------------------------------------------------- + +class TestFailureDomainAwareCapacity(unittest.TestCase): + + def test_piling_onto_active_domain_always_allowed(self): + """Two nodes already down in domain 1 (at cap already) -- removing a + THIRD node also in domain 1 must still be allowed.""" + cl = _cluster(npcs=2, ndcs=2, ft=2, enable_failure_domain=True) + nodes = [ + _node("n1", failure_domain=1), + _node("n2", status=StorageNode.STATUS_OFFLINE, failure_domain=1), + _node("n3", status=StorageNode.STATUS_OFFLINE, failure_domain=1), + _node("n4", failure_domain=2), + _node("n5", failure_domain=3), + _node("n6", failure_domain=4), + ] + db = _db(cl, nodes) + allowed, _ = _check_ftt_allows_node_removal("n1", db) + self.assertTrue(allowed) + + def test_well_provisioned_new_domain_blocked_at_cap(self): + """ndcs=2/npcs=2 needs 4 domains for full isolation; 4 are available. + Domains 1 and 2 already have not-online nodes (cap=npcs=2 domains + active) -- removing a node in a THIRD domain must be blocked.""" + cl = _cluster(npcs=2, ndcs=2, ft=2, enable_failure_domain=True) + nodes = [ + _node("n1", failure_domain=3), + _node("n2", status=StorageNode.STATUS_OFFLINE, failure_domain=1), + _node("n3", status=StorageNode.STATUS_OFFLINE, failure_domain=2), + _node("n4", failure_domain=4), + _node("n5", failure_domain=1), + _node("n6", failure_domain=2), + ] + db = _db(cl, nodes) + allowed, reason = _check_ftt_allows_node_removal("n1", db) + self.assertFalse(allowed) + self.assertIn("domain", reason) + + def test_well_provisioned_new_domain_within_cap_allowed(self): + """Same layout, but only ONE domain active so far -- opening a + second domain is still within the npcs=2 domain budget.""" + cl = _cluster(npcs=2, ndcs=2, ft=2, enable_failure_domain=True) + nodes = [ + _node("n1", failure_domain=3), + _node("n2", status=StorageNode.STATUS_OFFLINE, failure_domain=1), + _node("n3", failure_domain=2), + _node("n4", failure_domain=4), + ] + db = _db(cl, nodes) + allowed, _ = _check_ftt_allows_node_removal("n1", db) + self.assertTrue(allowed) + + def test_under_provisioned_second_domain_within_node_budget_allowed(self): + """ndcs=2/npcs=2 needs 4 domains, but only 2 exist -- under-provisioned. + One node down in domain 1; removing a node in domain 2 (opening a + second domain) is still allowed while raw not-online count (1) is + under the npcs=2 node budget.""" + cl = _cluster(npcs=2, ndcs=2, ft=2, enable_failure_domain=True) + nodes = [ + _node("n1", failure_domain=2), + _node("n2", status=StorageNode.STATUS_OFFLINE, failure_domain=1), + _node("n3", failure_domain=1), + _node("n4", failure_domain=2), + ] + db = _db(cl, nodes) + allowed, _ = _check_ftt_allows_node_removal("n1", db) + self.assertTrue(allowed) + + def test_under_provisioned_second_domain_blocked_at_node_budget(self): + """Same under-provisioned layout, but the node-count budget (npcs=2) + is already spent -- opening a second domain must now be blocked.""" + cl = _cluster(npcs=2, ndcs=2, ft=2, enable_failure_domain=True) + nodes = [ + _node("n1", failure_domain=2), + _node("n2", status=StorageNode.STATUS_OFFLINE, failure_domain=1), + _node("n3", status=StorageNode.STATUS_OFFLINE, failure_domain=1), + _node("n4", failure_domain=2), + ] + db = _db(cl, nodes) + allowed, reason = _check_ftt_allows_node_removal("n1", db) + self.assertFalse(allowed) + self.assertIn("risk budget", reason) + + # --- Dmitrii's confirmed scenarios: 2 FD / 2+2 --------------------------- + # chunks_per_domain = ceil(4/2) = 2. Safe: "1 whole FD" or "1 node per FD", + # nothing more -- verified against the specific combinations he ruled out. + + def test_2fd_one_whole_fd_down_is_safe(self): + """Domain 1 already has 3-of-4 nodes down; the 4th is still allowed + (piling within a single domain, which may go fully down).""" + cl = _cluster(npcs=2, ndcs=2, ft=2, enable_failure_domain=True) + nodes = [ + _node("n1", failure_domain=1), + _node("n2", status=StorageNode.STATUS_OFFLINE, failure_domain=1), + _node("n3", status=StorageNode.STATUS_OFFLINE, failure_domain=1), + _node("n4", status=StorageNode.STATUS_OFFLINE, failure_domain=1), + _node("n5", failure_domain=2), _node("n6", failure_domain=2), + _node("n7", failure_domain=2), _node("n8", failure_domain=2), + ] + db = _db(cl, nodes) + allowed, _ = _check_ftt_allows_node_removal("n1", db) + self.assertTrue(allowed) + + def test_2fd_one_node_per_fd_is_safe(self): + """1 node down in domain 1; removing 1 node in domain 2 is safe.""" + cl = _cluster(npcs=2, ndcs=2, ft=2, enable_failure_domain=True) + nodes = [ + _node("n1", failure_domain=2), + _node("n2", status=StorageNode.STATUS_OFFLINE, failure_domain=1), + _node("n3", failure_domain=1), + _node("n4", failure_domain=2), + ] + db = _db(cl, nodes) + allowed, _ = _check_ftt_allows_node_removal("n1", db) + self.assertTrue(allowed) + + def test_2fd_whole_fd_plus_other_fd_node_is_unsafe(self): + """Domain 1 fully down (4 nodes) -- a node in domain 2 must now be + blocked (not one of the two safe combinations).""" + cl = _cluster(npcs=2, ndcs=2, ft=2, enable_failure_domain=True) + nodes = [ + _node("n1", failure_domain=2), + _node("n2", status=StorageNode.STATUS_OFFLINE, failure_domain=1), + _node("n3", status=StorageNode.STATUS_OFFLINE, failure_domain=1), + _node("n4", status=StorageNode.STATUS_OFFLINE, failure_domain=1), + _node("n5", status=StorageNode.STATUS_OFFLINE, failure_domain=1), + _node("n6", failure_domain=2), _node("n7", failure_domain=2), _node("n8", failure_domain=2), + ] + db = _db(cl, nodes) + allowed, _ = _check_ftt_allows_node_removal("n1", db) + self.assertFalse(allowed) + + def test_2fd_one_per_fd_plus_second_node_in_same_fd_is_unsafe(self): + """1 node down in EACH of domains 1 and 2 already (the safe combo) -- + piling a SECOND node onto domain 2 must now be blocked, even though + domain 2 is already 'active'. This is the exact gap the old + unconditional-piling logic missed.""" + cl = _cluster(npcs=2, ndcs=2, ft=2, enable_failure_domain=True) + nodes = [ + _node("n1", failure_domain=2), + _node("n2", status=StorageNode.STATUS_OFFLINE, failure_domain=1), + _node("n3", status=StorageNode.STATUS_OFFLINE, failure_domain=2), + _node("n4", failure_domain=1), + ] + db = _db(cl, nodes) + allowed, _ = _check_ftt_allows_node_removal("n1", db) + self.assertFalse(allowed) + + def test_2fd_one_per_fd_plus_second_node_in_other_fd_is_unsafe(self): + """Same setup, but piling the extra node onto domain 1 (the first, + 'already active' domain) instead of domain 2 -- also unsafe.""" + cl = _cluster(npcs=2, ndcs=2, ft=2, enable_failure_domain=True) + nodes = [ + _node("n1", failure_domain=1), + _node("n2", status=StorageNode.STATUS_OFFLINE, failure_domain=1), + _node("n3", status=StorageNode.STATUS_OFFLINE, failure_domain=2), + _node("n4", failure_domain=2), + ] + db = _db(cl, nodes) + allowed, _ = _check_ftt_allows_node_removal("n1", db) + self.assertFalse(allowed) + + # --- Dmitrii's confirmed scenarios: 3 FD / 2+2 --------------------------- + # chunks_per_domain = ceil(4/3) = 2 -- same per-domain cap as 2 FD, but + # spread across 3 domains. Confirmed: 1 FD fully down is safe, 2 FDs is not. + + def test_3fd_one_whole_fd_down_is_safe(self): + cl = _cluster(npcs=2, ndcs=2, ft=2, enable_failure_domain=True) + nodes = [ + _node("n1", failure_domain=1), + _node("n2", status=StorageNode.STATUS_OFFLINE, failure_domain=1), + _node("n3", status=StorageNode.STATUS_OFFLINE, failure_domain=1), + _node("n4", status=StorageNode.STATUS_OFFLINE, failure_domain=1), + _node("n5", failure_domain=2), _node("n6", failure_domain=3), + ] + db = _db(cl, nodes) + allowed, _ = _check_ftt_allows_node_removal("n1", db) + self.assertTrue(allowed) + + def test_3fd_two_whole_fds_down_is_unsafe(self): + """Domain 1 fully down already (2+ nodes, maxing its chunks_per_domain + budget) -- a node in domain 2 must be blocked.""" + cl = _cluster(npcs=2, ndcs=2, ft=2, enable_failure_domain=True) + nodes = [ + _node("n1", failure_domain=2), + _node("n2", status=StorageNode.STATUS_OFFLINE, failure_domain=1), + _node("n3", status=StorageNode.STATUS_OFFLINE, failure_domain=1), + _node("n4", failure_domain=2), _node("n5", failure_domain=3), + ] + db = _db(cl, nodes) + allowed, _ = _check_ftt_allows_node_removal("n1", db) + self.assertFalse(allowed) + + # --- Dmitrii's confirmed scenarios: 4 FD / 2+2 (well-provisioned) -------- + # chunks_per_domain = ceil(4/4) = 1. Confirmed: 2 whole FDs down is safe. + + def test_4fd_two_whole_fds_down_is_safe(self): + """Domain 1 already has 3-of-4 nodes down (maxed, chunks_per_domain=1 + means it only ever contributes 1 to the budget regardless of count); + removing a node in domain 2 -- opening the second domain -- is still + within the npcs=2 budget.""" + cl = _cluster(npcs=2, ndcs=2, ft=2, enable_failure_domain=True) + nodes = [ + _node("n1", failure_domain=2), + _node("n2", status=StorageNode.STATUS_OFFLINE, failure_domain=1), + _node("n3", status=StorageNode.STATUS_OFFLINE, failure_domain=1), + _node("n4", status=StorageNode.STATUS_OFFLINE, failure_domain=1), + _node("n5", failure_domain=2), _node("n6", failure_domain=3), _node("n7", failure_domain=4), + ] + db = _db(cl, nodes) + allowed, _ = _check_ftt_allows_node_removal("n1", db) + self.assertTrue(allowed) + + def test_4fd_three_whole_fds_down_is_unsafe(self): + """Domains 1 and 2 already have a node down each (budget=2/2 spent); + removing a node in a THIRD domain (3) must be blocked.""" + cl = _cluster(npcs=2, ndcs=2, ft=2, enable_failure_domain=True) + nodes = [ + _node("n1", failure_domain=3), + _node("n2", status=StorageNode.STATUS_OFFLINE, failure_domain=1), + _node("n3", status=StorageNode.STATUS_OFFLINE, failure_domain=2), + _node("n4", failure_domain=4), + ] + db = _db(cl, nodes) + allowed, _ = _check_ftt_allows_node_removal("n1", db) + self.assertFalse(allowed) + + def test_fd_enabled_but_node_unassigned_falls_back_to_node_count(self): + """FD enabled cluster-wide, but this specific node has no domain + assignment (-1) -- must fall back to the plain node-count cap.""" + cl = _cluster(npcs=1, ndcs=2, ft=1, enable_failure_domain=True) + nodes = [ + _node("n1", failure_domain=-1), + _node("n2", status=StorageNode.STATUS_OFFLINE, failure_domain=1), + _node("n3", failure_domain=1), + ] + db = _db(cl, nodes) + allowed, reason = _check_ftt_allows_node_removal("n1", db) + self.assertFalse(allowed) + self.assertIn("not-online", reason) + + def test_fd_disabled_ignores_domains_uses_node_count(self): + """enable_failure_domain=False: domain tags present but irrelevant -- + behaves exactly like the non-FD npcs=1 case.""" + cl = _cluster(npcs=1, ndcs=2, ft=1, enable_failure_domain=False) + nodes = [ + _node("n1", failure_domain=1), + _node("n2", status=StorageNode.STATUS_OFFLINE, failure_domain=2), + _node("n3", failure_domain=3), + ] + db = _db(cl, nodes) + allowed, reason = _check_ftt_allows_node_removal("n1", db) + self.assertFalse(allowed) + self.assertIn("FTT=1", reason) + + # --------------------------------------------------------------------------- # Rebalancing additional scenarios # --------------------------------------------------------------------------- diff --git a/tests/unit/test_missing_namespace_path_loss.py b/tests/unit/test_missing_namespace_path_loss.py index 95ac17e873..3ba2da92f3 100644 --- a/tests/unit/test_missing_namespace_path_loss.py +++ b/tests/unit/test_missing_namespace_path_loss.py @@ -57,6 +57,12 @@ def _make_node(node_id="tertiary"): node.get_lvol_subsys_port.return_value = 4440 node.data_nics = [] node.cluster_id = "cluster-1" + # Distinct from lvol.lvs_name ("LVS_25") so the node is a non-leader + # host for it, with lvstore_ports already populated -- the normal, + # settled state. Tests targeting the guard in + # TestAddLvolThreadRefusesGuessedPort override this explicitly. + node.lvstore = "" + node.lvstore_ports = {"LVS_25": {"lvol_subsys_port": 4440, "hublvol_port": 4441}} return node @@ -157,5 +163,104 @@ def test_publishes_listener_when_namespace_present(self): self.rpc.listeners_create.assert_called() +class TestAddLvolThreadRefusesGuessedPort(unittest.TestCase): + """``add_lvol_thread`` must never fall back to snode's OWN leader port + when registering a listener for a lvstore it hosts as a non-leader. + + Regression coverage for a real bug found live (2026-08-18): a + node-removal relocation correctly repointed lvol.nodes to the new + secondary, but lvol_monitor's repair loop raced the relocation's own + lvstore_ports commit -- add_lvol_thread was handed a stale snode object + (same "callers hold stale objects" hazard as the in_deletion check + above) whose lvstore_ports had no entry yet for the lvol's lvstore. + get_lvol_subsys_port()'s fallback-to-node-default (correct ONLY when + lvs_name IS the node's own primary) silently returned the node's OWN + leader port instead, and a listener was published on the wrong port + with nothing ever revisiting or correcting it.""" + + def setUp(self): + self.rpc = MagicMock(name="rpc") + self.lvol = _make_lvol() + self.snode = _make_node("secondary") + self.snode.rpc_client.return_value = self.rpc + # This node leads its OWN lvstore (LVS_9) and is only a non-leader + # host for the lvol's lvstore (LVS_25) -- the exact shape that hit + # the bug live (zqmjp leading LVS_26 while hosting LVS_14 as a + # freshly-relocated secondary). + self.snode.lvstore = "LVS_9" + + nic = MagicMock() + nic.ip4_address = "192.168.10.12" + nic.trtype = "TCP" + self.snode.data_nics = [nic] + self.lvol.fabric = "tcp" + self.lvol.lvol_type = "lvol" + + self.rpc.nvmf_subsystem_add_ns.return_value = 1 + + p = patch.object(storage_node_ops, "DBController") + self.db = p.start() + self.addCleanup(p.stop) + self.db.return_value.get_lvol_by_id.return_value = self.lvol + self.db.return_value.get_pool_by_id.return_value.has_qos.return_value = False + + def test_refuses_when_lvstore_ports_missing_even_after_refetch(self): + # Stale caller-held snode has no entry; a fresh re-fetch (simulated + # here as the SAME missing state) confirms it's genuinely not ready. + self.snode.lvstore_ports = {} + fresh = _make_node("secondary") + fresh.lvstore = "LVS_9" + fresh.lvstore_ports = {} + self.db.return_value.get_storage_node_by_id.return_value = fresh + + with patch.object(storage_node_ops, "_rpc_subsystem_has_ns", + side_effect=[False, True]): + ok, err = storage_node_ops.add_lvol_thread(self.lvol, self.snode) + + self.assertFalse(ok) + self.assertIn("lvstore_ports", err) + self.rpc.listeners_create.assert_not_called() + # Must not have guessed using snode's OWN leader port either. + self.snode.get_lvol_subsys_port.assert_not_called() + + def test_proceeds_with_fresh_port_once_refetch_finds_it(self): + # Stale caller-held snode has no entry yet, but the relocation's + # commit has already landed by the time we re-fetch -- must use + # the FRESH object's real port, not snode's own leader port. + self.snode.lvstore_ports = {} + fresh = _make_node("secondary") + fresh.lvstore = "LVS_9" + fresh.lvstore_ports = {"LVS_25": {"lvol_subsys_port": 5150, "hublvol_port": 5151}} + fresh.get_lvol_subsys_port = MagicMock(return_value=5150) + fresh.rpc_client.return_value = self.rpc + fresh.data_nics = self.snode.data_nics + self.db.return_value.get_storage_node_by_id.return_value = fresh + + with patch.object(storage_node_ops, "_rpc_subsystem_has_ns", + side_effect=[False, True]): + ok, err = storage_node_ops.add_lvol_thread(self.lvol, self.snode) + + self.assertTrue(ok, err) + self.rpc.listeners_create.assert_called_once() + _, kwargs = self.rpc.listeners_create.call_args + self.assertEqual(self.rpc.listeners_create.call_args.args[3], 5150) + + def test_own_leader_lvstore_never_triggers_the_guard(self): + # The normal case: lvol.lvs_name IS this node's own primary + # lvstore, which legitimately has no lvstore_ports entry -- must + # use the plain node-level port without re-fetching or refusing. + self.snode.lvstore = self.lvol.lvs_name + self.snode.lvstore_ports = {} + + with patch.object(storage_node_ops, "_rpc_subsystem_has_ns", + side_effect=[False, True]): + ok, err = storage_node_ops.add_lvol_thread(self.lvol, self.snode) + + self.assertTrue(ok, err) + self.db.return_value.get_storage_node_by_id.assert_not_called() + self.rpc.listeners_create.assert_called_once() + self.assertEqual(self.rpc.listeners_create.call_args.args[3], 4440) + + if __name__ == "__main__": unittest.main() diff --git a/tests/unit/test_node_removal.py b/tests/unit/test_node_removal.py index 3a0b78840a..58df97389d 100644 --- a/tests/unit/test_node_removal.py +++ b/tests/unit/test_node_removal.py @@ -17,20 +17,21 @@ """ import unittest -from unittest.mock import MagicMock, patch +from unittest.mock import DEFAULT, MagicMock, patch from simplyblock_core import storage_node_ops from simplyblock_core.models.storage_node import StorageNode -from simplyblock_core.models.nvme_device import NVMeDevice, JMDevice +from simplyblock_core.models.nvme_device import NVMeDevice, JMDevice, RemoteJMDevice from simplyblock_core.models.cluster import Cluster -from simplyblock_core.rpc_client import RPCConnectionError +from simplyblock_core.rpc_client import RPCConnectionError, RPCException, RPCRemoteError # --------------------------------------------------------------------------- # Fixtures # --------------------------------------------------------------------------- -def _cluster(ha_type="ha", npcs=1, ndcs=2, ft=1, mode="docker"): +def _cluster(ha_type="ha", npcs=1, ndcs=2, ft=1, mode="docker", + enable_failure_domain=False): cl = Cluster() cl.uuid = "cluster-1" cl.ha_type = ha_type @@ -40,12 +41,14 @@ def _cluster(ha_type="ha", npcs=1, ndcs=2, ft=1, mode="docker"): cl.mode = mode cl.status = Cluster.STATUS_ACTIVE cl.nqn = "nqn.2023-01.io.simplyblock:cluster-1" + cl.enable_failure_domain = enable_failure_domain return cl def _node(node_id, status=StorageNode.STATUS_ONLINE, lvstore="", secondary_id="", tertiary_id="", - stack_secondary="", stack_tertiary="", n_devices=0, with_jm=False): + stack_secondary="", stack_tertiary="", n_devices=0, with_jm=False, + failure_domain=-1, mgmt_ip=None): n = MagicMock(spec=StorageNode) n.uuid = node_id n.get_id = MagicMock(return_value=node_id) @@ -59,7 +62,8 @@ def _node(node_id, status=StorageNode.STATUS_ONLINE, lvstore="", n.tertiary_node_id = tertiary_id n.lvstore_stack_secondary = stack_secondary n.lvstore_stack_tertiary = stack_tertiary - n.mgmt_ip = f"10.0.0.{abs(hash(node_id)) % 250 + 1}" + n.failure_domain = failure_domain + n.mgmt_ip = mgmt_ip or f"10.0.0.{abs(hash(node_id)) % 250 + 1}" n.write_to_db = MagicMock() n.rpc_client = MagicMock(return_value=MagicMock()) n.hublvol_nqn_for_lvstore = MagicMock(return_value=f"nqn:hub:{lvstore}") @@ -260,6 +264,160 @@ def test_pick_secondary_uses_get_secondary_nodes(self): self.assertIn("n9", kwargs["exclude_ids"]) +# --------------------------------------------------------------------------- +# _find_splice_target_for_relocation — the removal-repair fallback used when +# get_secondary_nodes/_2 offer no free (unclaimed) cross-domain candidate. +# +# Regression coverage for the 2026-08-07 chained-removal incident: two +# removals in a row can strand a third node's secondary with zero free +# cross-domain candidates, even though an existing pairing two hops away +# could absorb it. Mirrors splice_stranded_secondary/_tertiary's edge search +# (used for the identical dead end at activation time), generalized with an +# exclude list for the removal path. +# --------------------------------------------------------------------------- + +class TestFindSpliceTargetForRelocation(unittest.TestCase): + + def test_splices_into_existing_secondary_edge(self): + cl = _cluster(enable_failure_domain=True) + p = _node("p", secondary_id="x", failure_domain=0, mgmt_ip="10.0.0.1") + x = _node("x", failure_domain=1, mgmt_ip="10.0.0.2") + stranded = _node("s", failure_domain=2, mgmt_ip="10.0.0.3") + db = FakeDB(cl, [p, x, stranded]) + got = storage_node_ops._find_splice_target_for_relocation(stranded, "secondary", db) + self.assertEqual(got, ("p", "x")) + + def test_prefers_edge_domain_disjoint_on_both_ends(self): + cl = _cluster(enable_failure_domain=True) + stranded = _node("s", failure_domain=0, mgmt_ip="10.0.0.9") + bad_p = _node("bad_p", secondary_id="bad_x", failure_domain=0, mgmt_ip="10.0.0.1") + bad_x = _node("bad_x", failure_domain=0, mgmt_ip="10.0.0.2") + good_p = _node("good_p", secondary_id="good_x", failure_domain=1, mgmt_ip="10.0.0.3") + good_x = _node("good_x", failure_domain=2, mgmt_ip="10.0.0.4") + db = FakeDB(cl, [stranded, bad_p, bad_x, good_p, good_x]) + got = storage_node_ops._find_splice_target_for_relocation(stranded, "secondary", db) + self.assertEqual(got, ("good_p", "good_x")) + + def test_excludes_ids_passed_by_caller(self): + cl = _cluster(enable_failure_domain=True) + p = _node("p", secondary_id="x", failure_domain=0, mgmt_ip="10.0.0.1") + x = _node("x", failure_domain=1, mgmt_ip="10.0.0.2") + stranded = _node("s", failure_domain=2, mgmt_ip="10.0.0.3") + db = FakeDB(cl, [p, x, stranded]) + got = storage_node_ops._find_splice_target_for_relocation( + stranded, "secondary", db, exclude_ids=["x"]) + self.assertIsNone(got) + + def test_no_edge_exists_returns_none(self): + cl = _cluster(enable_failure_domain=True) + stranded = _node("s", failure_domain=0, mgmt_ip="10.0.0.1") + other = _node("other", failure_domain=1, mgmt_ip="10.0.0.2") + db = FakeDB(cl, [stranded, other]) + got = storage_node_ops._find_splice_target_for_relocation(stranded, "secondary", db) + self.assertIsNone(got) + + def test_skips_edge_with_offline_endpoint(self): + cl = _cluster(enable_failure_domain=True) + p = _node("p", secondary_id="x", failure_domain=0, mgmt_ip="10.0.0.1") + x = _node("x", failure_domain=1, mgmt_ip="10.0.0.2", + status=StorageNode.STATUS_OFFLINE) + stranded = _node("s", failure_domain=2, mgmt_ip="10.0.0.3") + db = FakeDB(cl, [p, x, stranded]) + got = storage_node_ops._find_splice_target_for_relocation(stranded, "secondary", db) + self.assertIsNone(got) + + def test_skips_edge_not_host_disjoint_from_stranded(self): + stranded = _node("s", failure_domain=0, mgmt_ip="10.0.0.5") + p = _node("p", secondary_id="x", failure_domain=1, mgmt_ip="10.0.0.1") + x = _node("x", failure_domain=2, mgmt_ip="10.0.0.5") # shares stranded's host + cl = _cluster(enable_failure_domain=True) + 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") + 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")) + + +# --------------------------------------------------------------------------- +# _pick_replica_relocation_node — falls back to the splice finder above when +# the direct free-candidate search comes up empty (no candidates at all, or +# none that satisfy the hard cross-domain requirement). +# --------------------------------------------------------------------------- + +class TestPickReplicaRelocationSpliceFallback(unittest.TestCase): + + def test_falls_back_to_splice_when_no_free_candidate_at_all(self): + cl = _cluster(enable_failure_domain=True) + primary = _node("p1", secondary_id="n1", failure_domain=2, mgmt_ip="10.0.0.9") + removed = _node("n1", failure_domain=1, mgmt_ip="10.0.0.99") + edge_p = _node("edge_p", secondary_id="edge_x", failure_domain=0, mgmt_ip="10.0.0.1") + edge_x = _node("edge_x", failure_domain=1, mgmt_ip="10.0.0.2") + db = FakeDB(cl, [primary, removed, edge_p, edge_x]) + with patch.object(storage_node_ops, "get_secondary_nodes", return_value=[]): + got = storage_node_ops._pick_replica_relocation_node( + primary, removed, "secondary", db) + self.assertEqual(got, "edge_x") + + def test_falls_back_to_splice_when_only_candidate_is_same_domain(self): + cl = _cluster(enable_failure_domain=True) + primary = _node("p1", secondary_id="n1", failure_domain=2, mgmt_ip="10.0.0.9") + removed = _node("n1", failure_domain=1, mgmt_ip="10.0.0.99") + same_domain_cand = _node("same_domain_cand", failure_domain=2, mgmt_ip="10.0.0.8") + edge_p = _node("edge_p", secondary_id="edge_x", failure_domain=0, mgmt_ip="10.0.0.1") + edge_x = _node("edge_x", failure_domain=1, mgmt_ip="10.0.0.2") + db = FakeDB(cl, [primary, removed, same_domain_cand, edge_p, edge_x]) + with patch.object(storage_node_ops, "get_secondary_nodes", + return_value=["same_domain_cand"]): + got = storage_node_ops._pick_replica_relocation_node( + primary, removed, "secondary", db) + self.assertEqual(got, "edge_x") + + def test_returns_none_when_no_free_and_no_splice_candidate(self): + 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") + db = FakeDB(cl, [primary, removed]) + with patch.object(storage_node_ops, "get_secondary_nodes", return_value=[]): + got = storage_node_ops._pick_replica_relocation_node( + primary, removed, "secondary", db) + self.assertIsNone(got) + + def test_does_not_use_splice_when_free_cross_domain_candidate_exists(self): + 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") + free1 = _node("free1", failure_domain=1, mgmt_ip="10.0.0.1") + db = FakeDB(cl, [primary, removed, free1]) + with patch.object(storage_node_ops, "get_secondary_nodes", return_value=["free1"]), \ + patch.object(storage_node_ops, "_find_splice_target_for_relocation") as finder: + got = storage_node_ops._pick_replica_relocation_node( + primary, removed, "secondary", db) + self.assertEqual(got, "free1") + finder.assert_not_called() + + def test_fd_disabled_never_needs_splice_when_candidate_exists(self): + # Non-FD clusters take the unconditional cands[0] path -- splice is + # only relevant once cands is genuinely empty. + cl = _cluster(enable_failure_domain=False) + primary = _node("p1", secondary_id="n1", mgmt_ip="10.0.0.9") + removed = _node("n1", mgmt_ip="10.0.0.99") + db = FakeDB(cl, [primary, removed]) + with patch.object(storage_node_ops, "get_secondary_nodes", return_value=["free1"]), \ + patch.object(storage_node_ops, "_find_splice_target_for_relocation") as finder: + got = storage_node_ops._pick_replica_relocation_node( + primary, removed, "secondary", db) + self.assertEqual(got, "free1") + finder.assert_not_called() + + # --------------------------------------------------------------------------- # Case A — teardown of own primary's replicas # --------------------------------------------------------------------------- @@ -282,63 +440,222 @@ def test_clears_bookkeeping_both_sides(self): self.assertEqual(sec.lvstore_stack_secondary, "") self.assertEqual(tert.lvstore_stack_tertiary, "") self.assertEqual(drp.call_count, 2) + # Case A: removed IS the node going away -- destroying its lvstore + # here is correct (default destroy_lvstore=True, not overridden). + drp.assert_any_call(sec, removed, cl) + drp.assert_any_call(tert, removed, cl) # --------------------------------------------------------------------------- # _delete_replica_on_peer — the peer-side hublvol+bdev teardown _teardown_ # replicas_of_primary and _relocate_replica_between both call. # -# Regression coverage for a real bug found live (2026-08-10): the hublvol -# subsystem check called rpc_client.subsystem_list(nqn), but subsystem_list() -# takes no arguments at all (it lists everything, unfiltered) -- every call -# raised TypeError, silently swallowed by the surrounding best-effort -# try/except, so the hublvol subsystem was never actually torn down on any -# peer during node removal. subsystem_get(nqn) is the existing, correct, -# server-side-filtered method for this. +# peer's own hublvol subsystem/bdev teardown (subsystem_get/subsystem_delete/ +# bdev_lvol_delete_hublvol) is deliberately commented out as of f5a052f3 -- +# that subsystem has no consumers and is harmless to leave until peer's next +# restart -- so these tests do NOT assert those calls. +# +# Regression coverage for a real bug found live (2026-08-14): peer's NVMe-oF +# controller connecting TO primary's hublvol (kept live the whole time peer +# held this replica) was never detached on eviction. Left dangling, it can +# later be found wedged in a non-enabled state when peer is re-selected to +# host a replica again, and the reconcile's detach-and-wait-gone can then +# time out and abort the rebuild -- this exact sequence took ffznh's SPDK +# down after a splice eviction left this connection behind. # --------------------------------------------------------------------------- class TestDeleteReplicaOnPeer(unittest.TestCase): - def test_deletes_subsystem_when_present(self): + def test_detaches_hublvol_controller_when_present(self): cl = _cluster() primary = _node("p1", lvstore="LVS_1") - peer = _node("peer1", lvstore="LVS_1") # hublvol_nqn_for_lvstore's mocked return value bakes in this lvstore + primary.hublvol = MagicMock(bdev_name="LVS_1/hublvol") + peer = _node("peer1", lvstore="LVS_1") rpc = peer.rpc_client() - rpc.subsystem_get.return_value = {"nqn": "nqn:hub:LVS_1"} storage_node_ops._delete_replica_on_peer(peer, primary, cl) - rpc.subsystem_get.assert_called_once_with("nqn:hub:LVS_1") - rpc.subsystem_delete.assert_called_once_with("nqn:hub:LVS_1") + rpc.bdev_nvme_detach_controller.assert_called_once_with("LVS_1/hublvol") - def test_skips_delete_when_subsystem_absent(self): + def test_skips_hublvol_detach_when_primary_has_no_hublvol(self): cl = _cluster() primary = _node("p1", lvstore="LVS_1") - peer = _node("peer1", lvstore="LVS_1") # hublvol_nqn_for_lvstore's mocked return value bakes in this lvstore + primary.hublvol = None + peer = _node("peer1", lvstore="LVS_1") rpc = peer.rpc_client() - rpc.subsystem_get.return_value = None storage_node_ops._delete_replica_on_peer(peer, primary, cl) - rpc.subsystem_get.assert_called_once_with("nqn:hub:LVS_1") - rpc.subsystem_delete.assert_not_called() + rpc.bdev_nvme_detach_controller.assert_not_called() - def test_subsystem_get_failure_is_caught_not_raised(self): + def test_hublvol_detach_failure_is_caught_not_raised(self): # Best-effort: an RPC failure here must not propagate and block removal. cl = _cluster() primary = _node("p1", lvstore="LVS_1") - peer = _node("peer1", lvstore="LVS_1") # hublvol_nqn_for_lvstore's mocked return value bakes in this lvstore + primary.hublvol = MagicMock(bdev_name="LVS_1/hublvol") + peer = _node("peer1", lvstore="LVS_1") rpc = peer.rpc_client() - rpc.subsystem_get.side_effect = RPCConnectionError("connection error") + rpc.bdev_nvme_detach_controller.side_effect = RPCConnectionError("connection error") storage_node_ops._delete_replica_on_peer(peer, primary, cl) # must not raise - rpc.subsystem_delete.assert_not_called() - # Teardown of the other artifacts still proceeds despite this failure. - rpc.bdev_lvol_delete_hublvol.assert_called_once_with("LVS_1") def test_no_op_when_primary_has_no_lvstore(self): cl = _cluster() primary = _node("p1", lvstore="") - peer = _node("peer1", lvstore="LVS_1") # hublvol_nqn_for_lvstore's mocked return value bakes in this lvstore + primary.hublvol = MagicMock(bdev_name="LVS_1/hublvol") + peer = _node("peer1", lvstore="LVS_1") + rpc = peer.rpc_client() + storage_node_ops._delete_replica_on_peer(peer, primary, cl) + rpc.bdev_nvme_detach_controller.assert_not_called() + + # ----------------------------------------------------------------- + # destroy_lvstore -- regression coverage for a real bug found live + # (2026-08-16): the splice/relocation eviction path called this with + # the default (destroy) behavior, which calls bdev_lvol_delete_lvstore + # on a peer holding only a non-leader examine copy -- destroying the + # SHARED on-disk blobstore metadata out from under the still-live + # primary elsewhere. This corrupted LVS_1's on-disk metadata during a + # splice eviction, surfacing later as a superblock validation failure + # when the primary tried to reload it on restart. + # ----------------------------------------------------------------- + + def test_destroy_lvstore_default_true_deletes_shared_blobstore(self): + # Case A (node removal): primary IS the node going away, so + # destroying its lvstore here is correct. + cl = _cluster() + primary = _node("p1", lvstore="LVS_1") + peer = _node("peer1", lvstore="LVS_1") rpc = peer.rpc_client() storage_node_ops._delete_replica_on_peer(peer, primary, cl) - rpc.subsystem_get.assert_not_called() - rpc.bdev_lvol_delete_hublvol.assert_not_called() + rpc.bdev_lvol_delete_lvstore.assert_called_once_with("LVS_1") + + def test_destroy_lvstore_false_never_deletes_shared_blobstore(self): + # Splice/relocation eviction: primary survives, only its host on + # this peer is moving -- the shared blobstore must NOT be touched, + # only the local raid/distrib examine bdevs hot-removed. + cl = _cluster() + primary = _node("p1", lvstore="LVS_1") + peer = _node("peer1", lvstore="LVS_1") + rpc = peer.rpc_client() + storage_node_ops._delete_replica_on_peer(peer, primary, cl, destroy_lvstore=False) + rpc.bdev_lvol_delete_lvstore.assert_not_called() + rpc.bdev_raid_delete.assert_called_once_with("raid_1") + rpc.bdev_distrib_delete.assert_called_once_with("distrib_1") + + +# --------------------------------------------------------------------------- +# _update_lvol_nodes_for_replica_move — regression coverage for a real bug +# found live (2026-08-18): a node-removal relocation correctly repointed the +# storage-node-level secondary_node_id/tertiary_node_id bookkeeping, but left +# every LVol hosted on the surviving primary with the just-removed/vacated +# host still listed in its own `nodes` -- the field the CSI/host initiator +# actually connects to for multipath failover. That stranded a live lvol on +# a single path (the primary only) with nothing ever repointing it, since +# nothing re-syncs `nodes` later. Mirrors the identical fix already applied +# for expansion-triggered rebalancing in cluster_expansion/executor.py. +# --------------------------------------------------------------------------- + +def _lvol(node_id, nodes, lvol_id=None, nqn=None): + lv = MagicMock() + lv.nodes = list(nodes) + lv.write_to_db = MagicMock() + lv.get_id = MagicMock(return_value=lvol_id or f"lvol-{node_id}") + lv.nqn = nqn or f"nqn:{node_id}" + return lv + + +class TestUpdateLvolNodesForReplicaMove(unittest.TestCase): + + def test_repoints_old_host_to_new_host(self): + cl = _cluster() + db = FakeDB(cl, [], lvols={"p1": [_lvol("p1", ["p1", "old"])]}) + storage_node_ops._update_lvol_nodes_for_replica_move("p1", "old", "new", db) + lvol = db.lvols["p1"][0] + self.assertEqual(lvol.nodes, ["p1", "new"]) + lvol.write_to_db.assert_called_once() + + def test_leaves_unrelated_hosts_untouched(self): + cl = _cluster() + db = FakeDB(cl, [], lvols={"p1": [_lvol("p1", ["p1", "old", "tert"])]}) + storage_node_ops._update_lvol_nodes_for_replica_move("p1", "old", "new", db) + self.assertEqual(db.lvols["p1"][0].nodes, ["p1", "new", "tert"]) + + def test_no_op_when_old_host_not_present(self): + # e.g. a tertiary-only move must not touch an lvol with no tertiary. + cl = _cluster() + lvol = _lvol("p1", ["p1", "sec"]) + db = FakeDB(cl, [], lvols={"p1": [lvol]}) + storage_node_ops._update_lvol_nodes_for_replica_move("p1", "old", "new", db) + self.assertEqual(lvol.nodes, ["p1", "sec"]) + lvol.write_to_db.assert_not_called() + + def test_multiple_lvols_on_the_same_primary_all_updated(self): + cl = _cluster() + lvols = [_lvol("p1", ["p1", "old"]) for _ in range(3)] + db = FakeDB(cl, [], lvols={"p1": lvols}) + storage_node_ops._update_lvol_nodes_for_replica_move("p1", "old", "new", db) + for lvol in lvols: + self.assertEqual(lvol.nodes, ["p1", "new"]) + + def test_redundant_call_is_a_safe_no_op(self): + # Simulates a retry after an earlier attempt already applied it. + cl = _cluster() + lvol = _lvol("p1", ["p1", "new"]) + db = FakeDB(cl, [], lvols={"p1": [lvol]}) + storage_node_ops._update_lvol_nodes_for_replica_move("p1", "old", "new", db) + self.assertEqual(lvol.nodes, ["p1", "new"]) + lvol.write_to_db.assert_not_called() + + +# --------------------------------------------------------------------------- +# _teardown_lvol_subsystems_on_vacated_peer — regression coverage for a real +# bug found live (2026-08-18): _delete_replica_on_peer(destroy_lvstore=False) +# tears down the vacated peer's raid/distrib bdev stack (which cascades to +# remove each hosted lvol's namespace), but never touches the per-lvol NVMe- +# oF subsystem+listener registered separately via add_lvol_thread. Left +# behind, the listener keeps accepting connections in front of a now-empty +# subsystem, and since the peer is no longer in lvol.nodes nothing ever +# tells the CSI/host initiator to drop that connection either -- the volume +# carries a third, live-but-empty path indefinitely alongside its correct +# two. +# --------------------------------------------------------------------------- + +class TestTeardownLvolSubsystemsOnVacatedPeer(unittest.TestCase): + + def test_deletes_subsystem_for_every_lvol_hosted_on_the_primary(self): + cl = _cluster() + primary = _node("p1", lvstore="LVS_1") + peer = _node("peer1") + rpc = peer.rpc_client() + lvols = [ + _lvol("p1", ["p1", "peer1"], lvol_id="lv-a", nqn="nqn:a"), + _lvol("p1", ["p1", "peer1"], lvol_id="lv-b", nqn="nqn:b"), + ] + db = FakeDB(cl, [primary, peer], lvols={"p1": lvols}) + storage_node_ops._teardown_lvol_subsystems_on_vacated_peer(peer, primary, db) + rpc.subsystem_delete.assert_any_call("nqn:a") + rpc.subsystem_delete.assert_any_call("nqn:b") + self.assertEqual(rpc.subsystem_delete.call_count, 2) + + def test_no_op_when_primary_hosts_no_lvols(self): + cl = _cluster() + primary = _node("p1", lvstore="LVS_1") + peer = _node("peer1") + rpc = peer.rpc_client() + db = FakeDB(cl, [primary, peer], lvols={}) + storage_node_ops._teardown_lvol_subsystems_on_vacated_peer(peer, primary, db) + rpc.subsystem_delete.assert_not_called() + + def test_rpc_failure_on_one_lvol_does_not_block_the_others(self): + # Best-effort: an RPC failure here must not propagate and must not + # stop the remaining lvols from being cleaned up. + cl = _cluster() + primary = _node("p1", lvstore="LVS_1") + peer = _node("peer1") + rpc = peer.rpc_client() + rpc.subsystem_delete.side_effect = [RPCConnectionError("connection error"), None] + lvols = [ + _lvol("p1", ["p1", "peer1"], lvol_id="lv-a", nqn="nqn:a"), + _lvol("p1", ["p1", "peer1"], lvol_id="lv-b", nqn="nqn:b"), + ] + db = FakeDB(cl, [primary, peer], lvols={"p1": lvols}) + storage_node_ops._teardown_lvol_subsystems_on_vacated_peer(peer, primary, db) # must not raise + self.assertEqual(rpc.subsystem_delete.call_count, 2) # --------------------------------------------------------------------------- @@ -410,6 +727,419 @@ def test_relocate_missing_primary_just_clears(self): self.assertTrue(ret) self.assertEqual(removed.lvstore_stack_secondary, "") + def test_relocate_repoints_primarys_lvol_nodes_off_the_removed_host(self): + # Regression: the direct (non-splice) relocation path must repoint + # every LVol hosted on the primary from the removed host to the new + # one -- see TestUpdateLvolNodesForReplicaMove's docstring. + cl = _cluster() + removed = _node("n1", stack_secondary="p1") + primary = _node("p1", secondary_id="n1", lvstore="LVS_p1") + new = _node("n3") + lvol = _lvol("p1", ["p1", "n1"]) + db = FakeDB(cl, [removed, primary, new], lvols={"p1": [lvol]}) + with patch.object(storage_node_ops, "DBController", return_value=db), \ + patch.object(storage_node_ops, "_pick_replica_relocation_node", + return_value="n3"), \ + patch.object(storage_node_ops, "recreate_lvstore_on_non_leader", + return_value=True): + ret = storage_node_ops._relocate_one_replica(removed, "p1", "secondary") + self.assertTrue(ret) + self.assertEqual(lvol.nodes, ["p1", "n3"]) + + def test_relocate_failure_does_not_repoint_lvol_nodes(self): + # A failed rebuild must leave the lvol pointed at the still-intact + # old copy -- nothing to repoint to yet. + cl = _cluster() + removed = _node("n1", stack_secondary="p1") + primary = _node("p1", secondary_id="n1", lvstore="LVS_p1") + new = _node("n3") + lvol = _lvol("p1", ["p1", "n1"]) + db = FakeDB(cl, [removed, primary, new], lvols={"p1": [lvol]}) + with patch.object(storage_node_ops, "DBController", return_value=db), \ + patch.object(storage_node_ops, "_pick_replica_relocation_node", + return_value="n3"), \ + patch.object(storage_node_ops, "recreate_lvstore_on_non_leader", + return_value=False): + ret = storage_node_ops._relocate_one_replica(removed, "p1", "secondary") + self.assertFalse(ret) + self.assertEqual(lvol.nodes, ["p1", "n1"]) + + +# --------------------------------------------------------------------------- +# Case B, splice fallback — _pick_replica_relocation_node returned a BUSY +# node (a splice candidate, per _find_splice_target_for_relocation) instead +# of a free one. _relocate_one_replica must evict that node's current +# occupant onto the stranded primary before claiming the slot for itself. +# --------------------------------------------------------------------------- + +class TestRelocateOneReplicaSpliceExecution(unittest.TestCase): + + def test_relocate_via_splice_evicts_occupant_first(self): + 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") # x is busy, not free + 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) as rec, \ + patch.object(storage_node_ops, "_delete_replica_on_peer") as drp: + ret = storage_node_ops._relocate_one_replica(removed, "stranded", "secondary") + + self.assertTrue(ret) + # occupant's old replica torn down off x -- must NOT destroy the + # shared lvstore: occupant survives this relocation, only its host + # moves (2026-08-16: destroying it here corrupted the on-disk + # blobstore for a still-live primary). + drp.assert_called_once_with(x, occupant, cl, destroy_lvstore=False) + self.assertEqual(occupant.secondary_node_id, "stranded") # occupant re-homed onto stranded + self.assertEqual(stranded.secondary_node_id, "x") # stranded takes over x's freed slot + self.assertEqual(x.lvstore_stack_secondary, "stranded") + self.assertEqual(rec.call_count, 2) # occupant's rebuild + stranded's own rebuild + self.assertEqual(removed.lvstore_stack_secondary, "") + + 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 + # every lvol hosted on their respective primaries. See + # TestUpdateLvolNodesForReplicaMove's docstring. + 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") + occupant_lvol = _lvol("occupant", ["occupant", "x"]) + stranded_lvol = _lvol("stranded", ["stranded", "n1"]) + db = FakeDB(cl, [removed, stranded, occupant, x], + lvols={"occupant": [occupant_lvol], "stranded": [stranded_lvol]}) + 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"): + ret = storage_node_ops._relocate_one_replica(removed, "stranded", "secondary") + + self.assertTrue(ret) + self.assertEqual(occupant_lvol.nodes, ["occupant", "stranded"]) + self.assertEqual(stranded_lvol.nodes, ["stranded", "x"]) + + def test_relocate_via_splice_tears_down_lvol_subsystems_on_the_vacated_peer(self): + # Regression: evicting occupant's replica off x must also delete + # occupant's own lvols' NVMe-oF subsystems on x -- see + # TestTeardownLvolSubsystemsOnVacatedPeer's docstring. Must NOT run + # for stranded's own vacate-of-n1 (n1 is the node being removed, + # already shut down, not a surviving peer to clean up on). + 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, + "_teardown_lvol_subsystems_on_vacated_peer") as teardown: + ret = storage_node_ops._relocate_one_replica(removed, "stranded", "secondary") + + self.assertTrue(ret) + teardown.assert_called_once_with(x, occupant, db) + + def test_relocate_via_splice_occupant_rebuild_failure_leaves_old_copy_untouched(self): + # Create-before-destroy: a failed rebuild on the stranded node must + # NOT tear down or repoint the occupant's still-intact old copy on x + # -- that copy is occupant's ONLY surviving replica under FTT1, so a + # failed build must change nothing about it (2026-08-07 incident). + 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=False), \ + patch.object(storage_node_ops, "_delete_replica_on_peer") as drp: + ret = storage_node_ops._relocate_one_replica(removed, "stranded", "secondary") + + self.assertFalse(ret) + drp.assert_not_called() # old copy on x never torn down + self.assertEqual(occupant.secondary_node_id, "x") # unchanged -- still protected + self.assertEqual(x.lvstore_stack_secondary, "occupant") # unchanged + # The outer splice claim (stranded -> x) never got committed either. + self.assertEqual(stranded.secondary_node_id, "n1") + self.assertEqual(removed.lvstore_stack_secondary, "stranded") + + def test_relocate_via_splice_occupant_rebuild_raises_treated_as_failure(self): + # The 2026-08-07 incident's actual failure mode: recreate_lvstore_on_non_leader + # RAISED (a hublvol attach error) instead of returning False. Must be + # caught and handled identically to a returned False -- old copy on x + # stays untouched either way. + 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", + side_effect=Exception("connect_to_hublvol failed for LVS_18")), \ + patch.object(storage_node_ops, "_delete_replica_on_peer") as drp: + ret = storage_node_ops._relocate_one_replica(removed, "stranded", "secondary") + + self.assertFalse(ret) + drp.assert_not_called() + self.assertEqual(occupant.secondary_node_id, "x") + self.assertEqual(x.lvstore_stack_secondary, "occupant") + self.assertEqual(stranded.secondary_node_id, "n1") + + def test_relocate_via_splice_resumes_teardown_after_crash_between_writes(self): + # occupant's move was already built + committed by a PRIOR attempt + # (forward pointer already points at stranded), but the process + # crashed before the old copy on x was torn down -- x's backref is + # still stale. A retry must skip re-building (already done) and go + # straight to finishing the teardown, without erroring. + cl = _cluster() + removed = _node("n1", stack_secondary="stranded") + stranded = _node("stranded", secondary_id="n1", lvstore="LVS_stranded") + occupant = _node("occupant", secondary_id="stranded", lvstore="LVS_occupant") + x = _node("x", stack_secondary="occupant") # stale -- not yet cleared + 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) as rec, \ + patch.object(storage_node_ops, "_delete_replica_on_peer") as drp: + ret = storage_node_ops._relocate_one_replica(removed, "stranded", "secondary") + + self.assertTrue(ret) + drp.assert_called_once() # the deferred teardown finally runs + self.assertEqual(stranded.secondary_node_id, "x") + self.assertEqual(x.lvstore_stack_secondary, "stranded") + # rebuild still runs once for stranded's own claim on x -- the + # occupant's rebuild was already done, so it must NOT run again. + self.assertEqual(rec.call_count, 1) + + def test_relocate_via_splice_own_rebuild_failure_after_occupant_moved(self): + # occupant's move succeeds (evicted + rebuilt on stranded), but + # stranded's own rebuild on the freed slot x fails. + 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", + side_effect=[True, False]), \ + patch.object(storage_node_ops, "_delete_replica_on_peer") as drp: + ret = storage_node_ops._relocate_one_replica(removed, "stranded", "secondary") + + self.assertFalse(ret) + drp.assert_called_once() + self.assertEqual(occupant.secondary_node_id, "stranded") + # Forward pointers for stranded's own claim ARE committed (pre-build, + # same idempotent pattern) even though the rebuild on x failed. + self.assertEqual(stranded.secondary_node_id, "x") + self.assertEqual(x.lvstore_stack_secondary, "stranded") + self.assertEqual(removed.lvstore_stack_secondary, "stranded") + + def test_relocate_via_splice_vacates_strandeds_preexisting_occupant_first(self): + # 2026-08-12 live incident: `stranded` already hosts an unrelated + # occupant `z` via its own back-reference BEFORE this removal starts + # -- every node in a full ring already hosts someone. Splicing + # `occupant` onto `stranded` without first moving `z` elsewhere + # would silently drop z's back-reference (a single-value field + # can't hold both z and occupant): live symptom was `sn list` + # showing two different primaries both claiming the same secondary + # node, and a physically-live replica invisible to lvstore_ports. + # `z` must be relocated first -- onto a genuinely free node -- before + # `occupant` claims the freed slot. + cl = _cluster() + removed = _node("n1", stack_secondary="stranded") + stranded = _node("stranded", secondary_id="n1", lvstore="LVS_stranded", + stack_secondary="z") # already hosting 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") # genuinely unclaimed + db = FakeDB(cl, [removed, stranded, occupant, x, z, free_node]) + + def pick_side_effect(primary, exclude_node, role, db_controller): + if primary.get_id() == "stranded": + self.assertEqual(exclude_node.get_id(), "n1") + return "x" + if primary.get_id() == "z": + self.assertEqual(exclude_node.get_id(), "stranded") + 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) as rec, \ + patch.object(storage_node_ops, "_delete_replica_on_peer") as drp: + ret = storage_node_ops._relocate_one_replica(removed, "stranded", "secondary") + + self.assertTrue(ret) + # z relocated off stranded onto the genuinely free node. + self.assertEqual(z.secondary_node_id, "free") + self.assertEqual(free_node.lvstore_stack_secondary, "z") + # stranded's slot no longer holds the stale "z" -- it now correctly + # reflects occupant, the relationship this splice actually created. + self.assertEqual(stranded.lvstore_stack_secondary, "occupant") + # occupant evicted off x onto the now-vacated stranded. + self.assertEqual(occupant.secondary_node_id, "stranded") + # stranded's own claim on x (the originally-picked splice target). + self.assertEqual(stranded.secondary_node_id, "x") + self.assertEqual(x.lvstore_stack_secondary, "stranded") + self.assertEqual(removed.lvstore_stack_secondary, "") + 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_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. + 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") + db = FakeDB(cl, [removed, stranded, occupant, x, z]) + + def pick_side_effect(primary, exclude_node, role, db_controller): + if primary.get_id() == "stranded": + return "x" + if primary.get_id() == "z": + return None # nothing free anywhere for z + 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) as rec, \ + patch.object(storage_node_ops, "_delete_replica_on_peer") as drp: + ret = storage_node_ops._relocate_one_replica(removed, "stranded", "secondary") + + self.assertFalse(ret) + drp.assert_not_called() + rec.assert_not_called() + # Nothing committed -- z, occupant, x, stranded, removed all untouched. + self.assertEqual(z.secondary_node_id, "stranded") + self.assertEqual(stranded.lvstore_stack_secondary, "z") + self.assertEqual(occupant.secondary_node_id, "x") + self.assertEqual(stranded.secondary_node_id, "n1") + self.assertEqual(removed.lvstore_stack_secondary, "stranded") + + def test_relocate_via_splice_tertiary_role(self): + cl = _cluster() + removed = _node("n1", stack_tertiary="stranded") + stranded = _node("stranded", tertiary_id="n1", lvstore="LVS_stranded") + occupant = _node("occupant", tertiary_id="x", lvstore="LVS_occupant") + x = _node("x", stack_tertiary="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") as drp: + ret = storage_node_ops._relocate_one_replica(removed, "stranded", "tertiary") + + self.assertTrue(ret) + drp.assert_called_once() + self.assertEqual(occupant.tertiary_node_id, "stranded") + self.assertEqual(stranded.tertiary_node_id, "x") + self.assertEqual(x.lvstore_stack_tertiary, "stranded") + self.assertEqual(removed.lvstore_stack_tertiary, "") + + def test_relocate_via_splice_tertiary_vacates_strandeds_preexisting_occupant_first(self): + # FTT2 (dual fault tolerance) variant of + # test_relocate_via_splice_vacates_strandeds_preexisting_occupant_first: + # secondary and tertiary live in separate fields on every node, so a + # node hosting one primary's secondary AND a different primary's + # tertiary at once is fine -- that was never the collision. The + # collision is within a single field, and the cascade fix is + # parameterized by role throughout, so this exercises the same path + # for lvstore_stack_tertiary specifically. + cl = _cluster() + removed = _node("n1", stack_tertiary="stranded") + stranded = _node("stranded", tertiary_id="n1", lvstore="LVS_stranded", + stack_tertiary="z") # already hosting z's tertiary + occupant = _node("occupant", tertiary_id="x", lvstore="LVS_occupant") + x = _node("x", stack_tertiary="occupant") + z = _node("z", tertiary_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): + self.assertEqual(role, "tertiary") + if primary.get_id() == "stranded": + return "x" + if primary.get_id() == "z": + 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) as rec, \ + patch.object(storage_node_ops, "_delete_replica_on_peer") as drp: + ret = storage_node_ops._relocate_one_replica(removed, "stranded", "tertiary") + + self.assertTrue(ret) + self.assertEqual(z.tertiary_node_id, "free") + self.assertEqual(free_node.lvstore_stack_tertiary, "z") + self.assertEqual(stranded.lvstore_stack_tertiary, "occupant") + self.assertEqual(occupant.tertiary_node_id, "stranded") + self.assertEqual(stranded.tertiary_node_id, "x") + self.assertEqual(x.lvstore_stack_tertiary, "stranded") + self.assertEqual(removed.lvstore_stack_tertiary, "") + self.assertEqual(rec.call_count, 3) + self.assertEqual(drp.call_count, 2) + + def test_relocate_free_target_never_triggers_splice_eviction(self): + # Regression guard: when the picked target is genuinely free (no + # backref set), _relocate_one_replica must behave exactly as before + # -- no eviction, no extra recreate_lvstore_on_non_leader call. + cl = _cluster() + removed = _node("n1", stack_secondary="p1") + primary = _node("p1", secondary_id="n1", lvstore="LVS_p1") + free_node = _node("n3") # stack_secondary="" -- genuinely free + db = FakeDB(cl, [removed, primary, free_node]) + with patch.object(storage_node_ops, "DBController", return_value=db), \ + patch.object(storage_node_ops, "_pick_replica_relocation_node", + return_value="n3"), \ + patch.object(storage_node_ops, "recreate_lvstore_on_non_leader", + return_value=True) as rec, \ + patch.object(storage_node_ops, "_delete_replica_on_peer") as drp: + ret = storage_node_ops._relocate_one_replica(removed, "p1", "secondary") + + self.assertTrue(ret) + drp.assert_not_called() + rec.assert_called_once() + self.assertEqual(primary.secondary_node_id, "n3") + self.assertEqual(free_node.lvstore_stack_secondary, "p1") + # --------------------------------------------------------------------------- # Device decommission completion gate @@ -461,6 +1191,655 @@ def test_complete_when_all_migrated(self): self.assertTrue(ret) dc.device_remove.assert_not_called() + def test_skips_already_removed_peer_with_stale_jm_ids(self): + # 2026-08-11 incident: an earlier-removed node can still carry the + # currently-removed node's JM id in its own stale jm_ids (never + # cleared on ITS OWN removal) -- get_storage_nodes_by_cluster_id + # returns every node regardless of status, including removed ones. + # Must be skipped outright, not "fixed" via its own (permanently + # dead) rpc_client. + cl = _cluster() + removed = _node("n1", n_devices=0, with_jm=True) + removed.jm_ids = [] + stale_peer = _node("stale-peer", status=StorageNode.STATUS_REMOVED) + stale_peer.jm_ids = [removed.jm_device.get_id()] + db = FakeDB(cl, [removed, stale_peer]) + dc = MagicMock() + with patch.object(storage_node_ops, "DBController", return_value=db), \ + patch.object(storage_node_ops, "device_controller", dc), \ + patch.object(storage_node_ops, "_connect_to_remote_jm_devs") as connect_mock: + ret = storage_node_ops._decommission_node_devices(removed) + + self.assertTrue(ret) + dc.remove_jm_device.assert_called_once() + connect_mock.assert_not_called() + # The stale peer's own bookkeeping is left alone -- it's dead, not "fixed". + self.assertEqual(stale_peer.jm_ids, [removed.jm_device.get_id()]) + stale_peer.write_to_db.assert_not_called() + + def test_refreshes_peer_holding_dead_jm_only_via_hosted_primary(self): + # 2026-08-14 incident: _connect_to_remote_jm_devs populates + # remote_jm_devices from TWO sources -- a node's own jm_ids (its + # redundancy set for its own JM), AND, separately, whichever + # primary it hosts as secondary/tertiary pulls in THAT primary's + # jm_ids too (lvstore_stack_secondary/_tertiary). A peer reachable + # only through the second path never touches its own jm_ids at + # all, so the jm_ids-only guard above never even looks at it, and + # its remote_jm_devices entry for the dead JM is left stale + # forever. A plain removal that never reshuffles who-hosts-whom + # never surfaces this (peer's remote_jm_devices happens to already + # be right) -- it takes a splice reshuffle to expose it. + cl = _cluster() + removed = _node("n1", n_devices=0, with_jm=True) + removed.jm_ids = [] + peer = _node("peer1", stack_secondary="some-primary") + peer.jm_ids = [] # clean -- the dead JM was never in ITS OWN set + stale_remote = RemoteJMDevice() + stale_remote.uuid = removed.jm_device.get_id() + peer.remote_jm_devices = [stale_remote] + db = FakeDB(cl, [removed, peer]) + dc = MagicMock() + with patch.object(storage_node_ops, "DBController", return_value=db), \ + patch.object(storage_node_ops, "device_controller", dc), \ + patch.object(storage_node_ops, "_connect_to_remote_jm_devs", + return_value=[]) as connect_mock: + ret = storage_node_ops._decommission_node_devices(removed) + + self.assertTrue(ret) + dc.remove_jm_device.assert_called_once() + connect_mock.assert_called_once_with(peer, peer.jm_ids) + self.assertEqual(peer.remote_jm_devices, []) + peer.write_to_db.assert_called_once() + + def test_does_not_refresh_peer_with_no_dead_jm_reference_at_all(self): + # Regression guard for the new elif's condition itself: a peer with + # neither the dead JM in its own jm_ids NOR in remote_jm_devices + # must be left completely untouched. + cl = _cluster() + removed = _node("n1", n_devices=0, with_jm=True) + removed.jm_ids = [] + peer = _node("peer1") + peer.jm_ids = [] + peer.remote_jm_devices = [] + db = FakeDB(cl, [removed, peer]) + dc = MagicMock() + with patch.object(storage_node_ops, "DBController", return_value=db), \ + patch.object(storage_node_ops, "device_controller", dc), \ + patch.object(storage_node_ops, "_connect_to_remote_jm_devs") as connect_mock: + ret = storage_node_ops._decommission_node_devices(removed) + + self.assertTrue(ret) + connect_mock.assert_not_called() + peer.write_to_db.assert_not_called() + + def test_picks_replacement_connects_it_and_calls_jc_replace_jm(self): + # Baseline: the replacement is connected under its OWN natural name + # (no override), and jc_replace_jm is told to swap consumer's live + # JC member from whatever it currently is (name_old, taken from + # consumer's own remote_jm_devices record) to that new name. + cl = _cluster() + removed = _node("n1", n_devices=0, with_jm=True) + removed.jm_ids = [] + consumer = _node("consumer", n_devices=0, with_jm=True) + consumer.jm_ids = [removed.jm_device.get_id()] + live_old = RemoteJMDevice() + live_old.uuid = removed.jm_device.get_id() + live_old.remote_bdev = "remote_jm_n1n1" + consumer.remote_jm_devices = [live_old] + replacement = _node("replacement", n_devices=0, with_jm=True) + replacement.jm_ids = [] + replacement.jm_device.jm_bdev = "jm_replacement" + db = FakeDB(cl, [removed, consumer, replacement]) + db.get_jm_device_by_id = MagicMock(side_effect=lambda jid: { + removed.jm_device.get_id(): removed.jm_device, + replacement.jm_device.get_id(): replacement.jm_device, + }[jid]) + dc = MagicMock() + connected_new = RemoteJMDevice() + connected_new.uuid = replacement.jm_device.get_id() + connected_new.remote_bdev = "remote_jm_replacementn1" + with patch.object(storage_node_ops, "DBController", return_value=db), \ + patch.object(storage_node_ops, "device_controller", dc), \ + patch.object(storage_node_ops, "get_sorted_ha_jms", + return_value=[replacement.jm_device.get_id()]), \ + patch.object(storage_node_ops, "_connect_to_remote_jm_devs", + return_value=[connected_new]) as connect_mock: + ret = storage_node_ops._decommission_node_devices(removed) + + self.assertTrue(ret) + connect_mock.assert_called_once_with( + consumer, jm_ids=[replacement.jm_device.get_id()], + only_node_id=replacement.get_id()) + consumer.rpc_client().jc_replace_jm.assert_called_once_with( + name_old="remote_jm_n1n1", name_new="remote_jm_replacementn1") + self.assertIn(replacement.jm_device.get_id(), consumer.jm_ids) + self.assertNotIn(removed.jm_device.get_id(), consumer.jm_ids) + self.assertEqual(consumer.remote_jm_devices, [connected_new]) + consumer.write_to_db.assert_called() + + def test_name_old_is_whatever_the_consumer_currently_has_live_not_removeds_own_name(self): + # A prior replacement (back when this used the retired + # override_name_on_node trick, or simply a longer chain of + # replacements) can leave consumer's live JC member named something + # that has nothing to do with removed_node's own jm_bdev. name_old + # must reflect reality (consumer's own remote_jm_devices record), + # never removed_node.jm_device.jm_bdev blindly. + cl = _cluster() + removed = _node("n1", n_devices=0, with_jm=True) + removed.jm_ids = [] + removed.jm_device.jm_bdev = "jm_n1" # consumer's JC never actually used this name + consumer = _node("consumer", n_devices=0, with_jm=True) + consumer.jm_ids = [removed.jm_device.get_id()] + live_old = RemoteJMDevice() + live_old.uuid = removed.jm_device.get_id() + live_old.remote_bdev = "jm_A" # the name actually live in consumer's JC + consumer.remote_jm_devices = [live_old] + replacement = _node("replacement", n_devices=0, with_jm=True) + replacement.jm_ids = [] + db = FakeDB(cl, [removed, consumer, replacement]) + db.get_jm_device_by_id = MagicMock(side_effect=lambda jid: { + removed.jm_device.get_id(): removed.jm_device, + replacement.jm_device.get_id(): replacement.jm_device, + }[jid]) + dc = MagicMock() + connected_new = RemoteJMDevice() + connected_new.uuid = replacement.jm_device.get_id() + connected_new.remote_bdev = "remote_jm_replacementn1" + with patch.object(storage_node_ops, "DBController", return_value=db), \ + patch.object(storage_node_ops, "device_controller", dc), \ + patch.object(storage_node_ops, "get_sorted_ha_jms", + return_value=[replacement.jm_device.get_id()]), \ + patch.object(storage_node_ops, "_connect_to_remote_jm_devs", + return_value=[connected_new]): + ret = storage_node_ops._decommission_node_devices(removed) + + self.assertTrue(ret) + consumer.rpc_client().jc_replace_jm.assert_called_once_with( + name_old="jm_A", name_new="remote_jm_replacementn1") + + def test_skips_a_candidate_the_consumer_already_reaches_via_another_path(self): + # SPDK won't attach a second, distinctly-named local controller to a + # target the consumer already has a live connection to under + # another name, and jc_replace_jm itself rejects a name_new already + # used by JC (-14) -- so "colliding" (already in + # consumer.remote_jm_devices, e.g. reached via hosting some OTHER + # primary's secondary copy, even though it was never in + # consumer.jm_ids) must be skipped in favor of "clean", the next + # candidate that isn't already reachable by any path. + cl = _cluster() + removed = _node("n1", n_devices=0, with_jm=True) + removed.jm_ids = [] + consumer = _node("consumer", n_devices=0, with_jm=True) + consumer.jm_ids = [removed.jm_device.get_id()] + colliding = _node("colliding", n_devices=0, with_jm=True) + colliding.jm_ids = [] + already_connected = RemoteJMDevice() + already_connected.uuid = colliding.jm_device.get_id() + already_connected.remote_bdev = "remote_jm_colliding-own-namen1" + live_old = RemoteJMDevice() + live_old.uuid = removed.jm_device.get_id() + live_old.remote_bdev = "remote_jm_n1n1" + consumer.remote_jm_devices = [already_connected, live_old] + clean = _node("clean", n_devices=0, with_jm=True) + clean.jm_ids = [] + db = FakeDB(cl, [removed, consumer, colliding, clean]) + db.get_jm_device_by_id = MagicMock(side_effect=lambda jid: { + removed.jm_device.get_id(): removed.jm_device, + colliding.jm_device.get_id(): colliding.jm_device, + clean.jm_device.get_id(): clean.jm_device, + }[jid]) + dc = MagicMock() + connected_new = RemoteJMDevice() + connected_new.uuid = clean.jm_device.get_id() + connected_new.remote_bdev = "remote_jm_cleann1" + with patch.object(storage_node_ops, "DBController", return_value=db), \ + patch.object(storage_node_ops, "device_controller", dc), \ + patch.object(storage_node_ops, "get_sorted_ha_jms", + return_value=[colliding.jm_device.get_id(), clean.jm_device.get_id()]), \ + patch.object(storage_node_ops, "_connect_to_remote_jm_devs", + return_value=[connected_new]) as connect_mock: + ret = storage_node_ops._decommission_node_devices(removed) + + self.assertTrue(ret) + connect_mock.assert_called_once_with( + consumer, jm_ids=[clean.jm_device.get_id()], only_node_id=clean.get_id()) + consumer.rpc_client().jc_replace_jm.assert_called_once_with( + name_old="remote_jm_n1n1", name_new="remote_jm_cleann1") + self.assertIn(clean.jm_device.get_id(), consumer.jm_ids) + self.assertNotIn(colliding.jm_device.get_id(), consumer.jm_ids) + + def test_no_collision_free_candidate_leaves_slot_honestly_short(self): + # No collision-free candidate anywhere -- unlike the retired + # override mechanism (which used to fake-accept a colliding + # candidate and rely on a later restart to self-heal it), + # jc_replace_jm would just reject a colliding name_new outright + # (-14), so there's no point even attempting it. Leave the + # redundancy slot honestly short instead. + cl = _cluster() + removed = _node("n1", n_devices=0, with_jm=True) + removed.jm_ids = [] + consumer = _node("consumer", n_devices=0, with_jm=True) + consumer.jm_ids = [removed.jm_device.get_id()] + colliding = _node("colliding", n_devices=0, with_jm=True) + colliding.jm_ids = [] + already_connected = RemoteJMDevice() + already_connected.uuid = colliding.jm_device.get_id() + already_connected.remote_bdev = "remote_jm_colliding-own-namen1" + consumer.remote_jm_devices = [already_connected] + db = FakeDB(cl, [removed, consumer, colliding]) + db.get_jm_device_by_id = MagicMock(side_effect=lambda jid: { + removed.jm_device.get_id(): removed.jm_device, + colliding.jm_device.get_id(): colliding.jm_device, + }[jid]) + dc = MagicMock() + with patch.object(storage_node_ops, "DBController", return_value=db), \ + patch.object(storage_node_ops, "device_controller", dc), \ + patch.object(storage_node_ops, "get_sorted_ha_jms", + return_value=[colliding.jm_device.get_id()]), \ + patch.object(storage_node_ops, "_connect_to_remote_jm_devs") as connect_mock: + ret = storage_node_ops._decommission_node_devices(removed) + + self.assertTrue(ret) + connect_mock.assert_not_called() + self.assertNotIn(colliding.jm_device.get_id(), consumer.jm_ids) + self.assertNotIn(removed.jm_device.get_id(), consumer.jm_ids) + colliding.write_to_db.assert_not_called() + consumer.write_to_db.assert_called() + + def test_jc_replace_jm_failure_leaves_slot_short_and_detaches_unused_connection(self): + # The candidate connects fine but the swap itself is rejected (e.g. + # a timeout connecting to the new JM bdev, code -6) -- don't claim + # the replacement, and don't leave the now-unused connection + # dangling: best-effort detach it. + cl = _cluster() + removed = _node("n1", n_devices=0, with_jm=True) + removed.jm_ids = [] + consumer = _node("consumer", n_devices=0, with_jm=True) + consumer.jm_ids = [removed.jm_device.get_id()] + live_old = RemoteJMDevice() + live_old.uuid = removed.jm_device.get_id() + live_old.remote_bdev = "remote_jm_n1n1" + consumer.remote_jm_devices = [live_old] + replacement = _node("replacement", n_devices=0, with_jm=True) + replacement.jm_ids = [] + replacement.jm_device.jm_bdev = "jm_replacement" + db = FakeDB(cl, [removed, consumer, replacement]) + db.get_jm_device_by_id = MagicMock(side_effect=lambda jid: { + removed.jm_device.get_id(): removed.jm_device, + replacement.jm_device.get_id(): replacement.jm_device, + }[jid]) + dc = MagicMock() + connected_new = RemoteJMDevice() + connected_new.uuid = replacement.jm_device.get_id() + connected_new.remote_bdev = "remote_jm_replacementn1" + consumer.rpc_client.return_value.jc_replace_jm.side_effect = RPCRemoteError( + "timed out connecting to the new JM bdev", code=-6) + with patch.object(storage_node_ops, "DBController", return_value=db), \ + patch.object(storage_node_ops, "device_controller", dc), \ + patch.object(storage_node_ops, "get_sorted_ha_jms", + return_value=[replacement.jm_device.get_id()]), \ + patch.object(storage_node_ops, "_connect_to_remote_jm_devs", + return_value=[connected_new]): + ret = storage_node_ops._decommission_node_devices(removed) + + self.assertTrue(ret) + self.assertNotIn(replacement.jm_device.get_id(), consumer.jm_ids) + self.assertNotIn(removed.jm_device.get_id(), consumer.jm_ids) + self.assertEqual(consumer.remote_jm_devices, [live_old]) + consumer.rpc_client.return_value.bdev_nvme_detach_controller.assert_called_once_with( + "remote_jm_replacement") + consumer.write_to_db.assert_called() + + def test_jc_replace_jm_dash14_skips_the_detach_cleanup(self): + # -14 (name_new already used by JC) means the bdev is legitimately + # claimed by JC already -- detaching it would tear down something + # in active use, so the cleanup must be skipped for this one code. + cl = _cluster() + removed = _node("n1", n_devices=0, with_jm=True) + removed.jm_ids = [] + consumer = _node("consumer", n_devices=0, with_jm=True) + consumer.jm_ids = [removed.jm_device.get_id()] + live_old = RemoteJMDevice() + live_old.uuid = removed.jm_device.get_id() + live_old.remote_bdev = "remote_jm_n1n1" + consumer.remote_jm_devices = [live_old] + replacement = _node("replacement", n_devices=0, with_jm=True) + replacement.jm_ids = [] + replacement.jm_device.jm_bdev = "jm_replacement" + db = FakeDB(cl, [removed, consumer, replacement]) + db.get_jm_device_by_id = MagicMock(side_effect=lambda jid: { + removed.jm_device.get_id(): removed.jm_device, + replacement.jm_device.get_id(): replacement.jm_device, + }[jid]) + dc = MagicMock() + connected_new = RemoteJMDevice() + connected_new.uuid = replacement.jm_device.get_id() + connected_new.remote_bdev = "remote_jm_replacementn1" + consumer.rpc_client.return_value.jc_replace_jm.side_effect = RPCRemoteError( + "name_new is already used by JC", code=-14) + with patch.object(storage_node_ops, "DBController", return_value=db), \ + patch.object(storage_node_ops, "device_controller", dc), \ + patch.object(storage_node_ops, "get_sorted_ha_jms", + return_value=[replacement.jm_device.get_id()]), \ + patch.object(storage_node_ops, "_connect_to_remote_jm_devs", + return_value=[connected_new]): + ret = storage_node_ops._decommission_node_devices(removed) + + self.assertTrue(ret) + self.assertNotIn(replacement.jm_device.get_id(), consumer.jm_ids) + consumer.rpc_client.return_value.bdev_nvme_detach_controller.assert_not_called() + + def test_no_candidate_at_all_still_persists_the_removed_jm_id(self): + # Regression guard for a real bug found while explaining this + # branch: the ONLY prior action, "no jm_id found" -> logger.error, + # never called node.write_to_db(). The node.jm_ids.remove() earlier + # in this same code path was therefore silently discarded -- the DB + # kept referencing a JM device that no longer exists, forever, with + # nothing left to show even that removal was attempted. An + # explicitly short jm_ids is a strictly more honest persisted state. + cl = _cluster() + removed = _node("n1", n_devices=0, with_jm=True) + removed.jm_ids = [] + consumer = _node("consumer", n_devices=0, with_jm=True) + consumer.jm_ids = [removed.jm_device.get_id()] + db = FakeDB(cl, [removed, consumer]) + dc = MagicMock() + with patch.object(storage_node_ops, "DBController", return_value=db), \ + patch.object(storage_node_ops, "device_controller", dc), \ + patch.object(storage_node_ops, "get_sorted_ha_jms", return_value=[]), \ + patch.object(storage_node_ops, "_connect_to_remote_jm_devs") as connect_mock: + ret = storage_node_ops._decommission_node_devices(removed) + + self.assertTrue(ret) + self.assertNotIn(removed.jm_device.get_id(), consumer.jm_ids) + consumer.write_to_db.assert_called() + connect_mock.assert_not_called() + + +# --------------------------------------------------------------------------- +# node_removal_orchestrate — phase-5 resume gap +# +# Phase 4 flips node status to REMOVED *before* phase 5 (device/JM +# decommission) runs. "status == REMOVED" therefore means phases 1/3a/3b/4 +# committed, NOT that removal is fully done -- a resumed attempt must still +# (re)run phase 5 rather than short-circuiting to "done" (2026-08-10 +# incident: an RPC error mid phase 5 left a peer's lvstore un-rebuilt while +# the task still reported "Node removed"). +# --------------------------------------------------------------------------- + +class TestNodeRemovalOrchestrateResumesPhase5(unittest.TestCase): + + def _patch_all(self): + return patch.multiple( + storage_node_ops, + DBController=DEFAULT, + cluster_ops=DEFAULT, + shutdown_storage_node=DEFAULT, + _teardown_replicas_of_primary=DEFAULT, + _relocate_replicas_hosted_on=DEFAULT, + _finalize_node_removal=DEFAULT, + set_node_status=DEFAULT, + _decommission_node_devices=DEFAULT, + ) + + def test_already_removed_skips_phases_1_to_4_but_reruns_phase5(self): + cl = _cluster() + node = _node("n1", status=StorageNode.STATUS_REMOVED) + db = FakeDB(cl, [node]) + with self._patch_all() as mocks: + mocks["DBController"].return_value = db + mocks["_decommission_node_devices"].return_value = True + ret = storage_node_ops.node_removal_orchestrate("n1") + + self.assertTrue(ret) + mocks["shutdown_storage_node"].assert_not_called() + mocks["_teardown_replicas_of_primary"].assert_not_called() + mocks["_relocate_replicas_hosted_on"].assert_not_called() + mocks["_finalize_node_removal"].assert_not_called() + mocks["set_node_status"].assert_not_called() + mocks["_decommission_node_devices"].assert_called_once_with(node) + + def test_already_removed_reports_incomplete_if_phase5_fails_again(self): + # The regression this guards: a prior attempt raised mid phase 5 + # after the status flip had already committed. The retry must + # actually retry phase 5, not silently report done because status + # already reads REMOVED. + cl = _cluster() + node = _node("n1", status=StorageNode.STATUS_REMOVED) + db = FakeDB(cl, [node]) + with self._patch_all() as mocks: + mocks["DBController"].return_value = db + mocks["_decommission_node_devices"].return_value = False + ret = storage_node_ops.node_removal_orchestrate("n1") + + self.assertFalse(ret) + mocks["_decommission_node_devices"].assert_called_once_with(node) + + def test_fresh_removal_still_runs_all_phases_then_phase5(self): + # Regression guard the other way: a from-scratch removal (status + # still ONLINE) must not skip phases 1/3a/3b/4. + cl = _cluster() + node = _node("n1", status=StorageNode.STATUS_ONLINE) + db = FakeDB(cl, [node]) + with self._patch_all() as mocks: + mocks["DBController"].return_value = db + mocks["shutdown_storage_node"].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 + ret = storage_node_ops.node_removal_orchestrate("n1") + + self.assertTrue(ret) + mocks["shutdown_storage_node"].assert_called_once() + mocks["_teardown_replicas_of_primary"].assert_called_once() + mocks["_relocate_replicas_hosted_on"].assert_called_once() + mocks["_finalize_node_removal"].assert_called_once() + mocks["set_node_status"].assert_called_once_with( + "n1", StorageNode.STATUS_REMOVED, caused_by="remove") + mocks["_decommission_node_devices"].assert_called_once() + + +# --------------------------------------------------------------------------- +# _finalize_node_removal — clearing the removed node's OWN stale bookkeeping +# +# Case A/B relocation clears every forward/back-reference field as each +# relationship is moved elsewhere, but neither touches lvstore_ports -- it +# isn't part of any relocation, just a port-reuse cache for this node's own +# restarts. Left uncleared, `sn list`'s "LVS Ports" column keeps showing +# entries for a node with no SPDK process left to back them (2026-08-13, +# found live after a removal). +# --------------------------------------------------------------------------- + +class TestFinalizeNodeRemovalClearsLvstorePorts(unittest.TestCase): + + def _run(self, removed, cluster_mode="kubernetes", node_api_up=False): + cl = _cluster(mode=cluster_mode) + db = FakeDB(cl, [removed]) + with patch.object(storage_node_ops, "DBController", return_value=db), \ + patch.object(storage_node_ops.health_controller, "_check_node_api", + return_value=node_api_up): + storage_node_ops._finalize_node_removal(removed) + return removed + + def test_clears_stale_lvstore_ports(self): + removed = _node("n1", lvstore="LVS_1") + removed.lvstore_ports = {"LVS_1": {"lvol_subsys_port": 4440, "hublvol_port": 4441}, + "LVS_9": {"lvol_subsys_port": 4450, "hublvol_port": 4451}} + removed = self._run(removed) + self.assertEqual(removed.lvstore_ports, {}) + removed.write_to_db.assert_called() + + def test_no_op_when_already_empty(self): + # Don't write to the DB at all when there's nothing to clear. + removed = _node("n1", lvstore="LVS_1") + removed.lvstore_ports = {} + removed = self._run(removed) + self.assertEqual(removed.lvstore_ports, {}) + removed.write_to_db.assert_not_called() + + +# --------------------------------------------------------------------------- +# _connect_to_remote_jm_devs — bounded retry + degrade-not-crash on a +# transient RPC/DNS failure during the fallback bdev-existence poll +# +# The primary connect_device() failure already degrades gracefully (logs +# "Failed to connect to ...", sets connect_failed=True). The get_bdevs() +# poll called right after it, against the same rpc_client, hits the +# identical transport and gets a bounded retry (3 attempts, 1s apart) to +# ride out a DNS blip; only once that's exhausted does it degrade to "this +# JM not connected" instead of raising -- 2026-08-10 incident: this exact +# call raised RPCException uncaught and killed a node-removal task mid +# phase 5. +# --------------------------------------------------------------------------- + +class TestConnectToRemoteJmDevsDegradesOnRpcException(unittest.TestCase): + + def _owner_setup(self, this_node_id="this-node"): + jm_dev = JMDevice() + jm_dev.uuid = "jm-owner" + jm_dev.jm_bdev = "jm_owner_bdev" + jm_dev.status = NVMeDevice.STATUS_ONLINE + + owner_node = MagicMock(spec=StorageNode) + owner_node.get_id = MagicMock(return_value="owner-node") + owner_node.status = StorageNode.STATUS_ONLINE + owner_node.jm_device = jm_dev + + this_node = MagicMock(spec=StorageNode) + this_node.get_id = MagicMock(return_value=this_node_id) + this_node.jm_ids = [] + this_node.lvstore_stack_secondary = "" + this_node.lvstore_stack_tertiary = "" + this_node.remote_jm_devices = [] + rpc_client = MagicMock() + this_node.rpc_client = MagicMock(return_value=rpc_client) + + db = MagicMock() + db.get_jm_device_by_id.return_value = jm_dev + db.get_storage_nodes.return_value = [owner_node] + + return this_node, rpc_client, db + + def test_get_bdevs_rpc_exception_exhausts_retries_and_does_not_raise(self): + # Persistent failure (all 3 bounded-retry attempts fail): must + # still degrade, not raise -- this is the exact call chain that + # took down a live node-removal task before the retry was added. + this_node, rpc_client, db = self._owner_setup() + rpc_client.get_bdevs.side_effect = RPCException("connection error") + + with patch.object(storage_node_ops, "DBController", return_value=db), \ + patch.object(storage_node_ops, "connect_device", + side_effect=RPCException("connection error")): + result = storage_node_ops._connect_to_remote_jm_devs( + this_node, jm_ids=["jm-owner"]) + + self.assertEqual(result, []) + # 3 bounded-retry attempts, one get_bdevs call each (remote_bdev + # is empty so the first branch short-circuits without calling). + self.assertEqual(rpc_client.get_bdevs.call_count, 3) + + def test_transient_failure_recovers_on_retry(self): + # A blip that clears within the retry budget must be caught, not + # just tolerated -- the whole point of adding the bounded retry + # instead of degrading on the very first failure. + this_node, rpc_client, db = self._owner_setup() + rpc_client.get_bdevs.side_effect = [ + RPCException("connection error"), + RPCException("connection error"), + {"name": "remote_jm_owner_bdevn1"}, + ] + + with patch.object(storage_node_ops, "DBController", return_value=db), \ + patch.object(storage_node_ops, "connect_device", + side_effect=RPCException("connection error")), \ + patch.object(storage_node_ops.time, "sleep"): + result = storage_node_ops._connect_to_remote_jm_devs( + this_node, jm_ids=["jm-owner"]) + + self.assertEqual(len(result), 1) + self.assertEqual(result[0].remote_bdev, "remote_jm_owner_bdevn1") + self.assertEqual(rpc_client.get_bdevs.call_count, 3) + + def test_transient_failure_does_not_block_a_clean_connect(self): + # Once the blip has cleared, the same code path must still succeed + # normally -- the new guard must not swallow a real success too. + this_node, rpc_client, db = self._owner_setup() + rpc_client.get_bdevs.return_value = {"name": "remote_jm_owner_bdevn1"} + + with patch.object(storage_node_ops, "DBController", return_value=db), \ + patch.object(storage_node_ops, "connect_device", + return_value="remote_jm_owner_bdevn1"): + result = storage_node_ops._connect_to_remote_jm_devs( + this_node, jm_ids=["jm-owner"]) + + self.assertEqual(len(result), 1) + self.assertEqual(result[0].remote_bdev, "remote_jm_owner_bdevn1") + + +# --------------------------------------------------------------------------- +# _connect_to_remote_jm_devs — remote_device.jm_bdev must record the name +# THIS NODE actually connects under +# +# Used to matter most when an override applied (a replacement JM connecting +# under a removed peer's old bdev name, per the now-retired +# override_name_on_node -- see _decommission_node_devices for why +# jc_replace_jm replaced that trick): remote_device.jm_bdev had to record +# the resolved (override) name, not org_dev's own natural name, or +# health_controller's diagnostic controller lookup +# (f'remote_{remote_device.jm_bdev}') queried the wrong, never-connected +# name every cycle. Now there's only ever one name to record -- the owner's +# own -- but the field still has to be right for the same diagnostic lookup. +# --------------------------------------------------------------------------- + +class TestConnectToRemoteJmDevsRecordsResolvedName(unittest.TestCase): + # override_name_on_node (and the drop_stale_overrides parameter that + # existed only to retire a stale entry) is gone now that SPDK's + # jc_replace_jm RPC swaps a live JC member by name directly -- + # _connect_to_remote_jm_devs always connects under the owner's own + # current name. This class is now just a baseline regression guard for + # that natural-name path. + + def _owner_setup(self, this_node_id="this-node"): + jm_dev = JMDevice() + jm_dev.uuid = "jm-owner" + jm_dev.jm_bdev = "jm_owner_bdev" + jm_dev.status = NVMeDevice.STATUS_ONLINE + + owner_node = MagicMock(spec=StorageNode) + owner_node.get_id = MagicMock(return_value="owner-node") + owner_node.status = StorageNode.STATUS_ONLINE + owner_node.jm_device = jm_dev + + this_node = MagicMock(spec=StorageNode) + this_node.get_id = MagicMock(return_value=this_node_id) + this_node.jm_ids = [] + this_node.lvstore_stack_secondary = "" + this_node.lvstore_stack_tertiary = "" + this_node.remote_jm_devices = [] + rpc_client = MagicMock() + this_node.rpc_client = MagicMock(return_value=rpc_client) + + db = MagicMock() + db.get_jm_device_by_id.return_value = jm_dev + db.get_storage_nodes.return_value = [owner_node] + + return this_node, rpc_client, db + + def test_connects_under_owners_own_natural_name(self): + this_node, rpc_client, db = self._owner_setup() + rpc_client.get_bdevs.return_value = {"name": "remote_jm_owner_bdevn1"} + + with patch.object(storage_node_ops, "DBController", return_value=db), \ + patch.object(storage_node_ops, "connect_device", + return_value="remote_jm_owner_bdevn1") as connect_mock: + result = storage_node_ops._connect_to_remote_jm_devs( + this_node, jm_ids=["jm-owner"]) + + self.assertEqual(len(result), 1) + self.assertEqual(result[0].jm_bdev, "jm_owner_bdev") + self.assertEqual(result[0].remote_bdev, "remote_jm_owner_bdevn1") + self.assertEqual(connect_mock.call_args[0][0], "remote_jm_owner_bdev") + class TestShrinkStatusDoesNotDeadlockRemoval(unittest.TestCase): """``node_removal_orchestrate`` holds ``Cluster.STATUS_IN_SHRINK`` for the diff --git a/tests/unit/web/api/v2/test_storage_node_endpoints.py b/tests/unit/web/api/v2/test_storage_node_endpoints.py index 859ca945f3..4e0c8c4ce9 100644 --- a/tests/unit/web/api/v2/test_storage_node_endpoints.py +++ b/tests/unit/web/api/v2/test_storage_node_endpoints.py @@ -112,6 +112,32 @@ def test_force_delete_also_deletes_node(self, client, storage_node, storage_node storage_node_ops.delete_storage_node.assert_called_once_with( STORAGE_NODE_ID, force=True) + def test_refused_removal_returns_400_not_500(self, client, storage_node, storage_node_ops): + # remove_storage_node's precondition gates (FTT, failure-domain + # balance, replica-relocation feasibility, ...) signal refusal via + # `return False`. An unhandled exception with no registered FastAPI + # handler becomes a 500 -- and 500 is on the operator's *retryable* + # list, so a permanently-infeasible removal (e.g. would leave a + # failure domain unbalanced) got retried forever instead of the + # operator resuming the node it had already suspended and failing + # cleanly (2026-08-13 incident). Must be a 400: non-retryable there. + storage_node_ops.remove_storage_node.return_value = False + + response = client.delete(f'{BASE}/{STORAGE_NODE_ID}/') + + assert response.status_code == 400 + storage_node_ops.delete_storage_node.assert_not_called() + + def test_refused_delete_after_successful_remove_returns_400( + self, client, storage_node, storage_node_ops): + storage_node_ops.remove_storage_node.return_value = 'task-uuid-1' + storage_node_ops.delete_storage_node.return_value = False + + response = client.delete( + f'{BASE}/{STORAGE_NODE_ID}/', params={'force_delete': True}) + + assert response.status_code == 400 + class TestStorageNodeLifecycle: