Fix/cluster activate stranded secondary node

simplyblock_core/cluster_ops.py

@@ -966,14 +966,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 "
@@ -1013,9 +1015,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 "
@@ -1052,6 +1060,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
@@ -1071,16 +1091,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
@@ -1099,15 +1124,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.

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``.
 #