fix(slurm-controller): add HA single-active guard and defer scontrol reconfigure

co-authored-by: Wouter de Geus <wouter.x.degeus@gsk.com>
co-authored-by: Duncan Lyall <duncan.x.lyall@gsk.com>

Author: 3 people

community/modules/scheduler/schedmd-slurm-gcp-v6-controller/modules/slurm_files/scripts/setup.py

@@ -459,6 +459,26 @@ def self_report_controller_address(lkp: util.Lookup) -> None:
     with blob.open('w') as f:
         f.write(yaml.dump(data))
 
+def check_slurmdbd_ready(maxtries: int = 30, interval: float = 15.0) -> None:
+    """Poll sacctmgr until slurmdbd is ready, with timeout safety and backoff."""
+    sacctmgr = slurmdirs.prefix / "bin" / "sacctmgr"
+    wait = 10.0
+    for try_cnt in range(1, maxtries + 1):
+        try:
+            res = run(f"{sacctmgr} -n -i show cluster", check=False, timeout=10)
+            if res.returncode == 0:
+                log.info(f"slurmdbd is ready (try {try_cnt}).")
+                return
+        except Exception as e:
+            log.debug(f"slurmdbd check error: {e}")
+
+        log.info(f"Waiting for slurmdbd to be ready... (try {try_cnt}/{maxtries}, retrying in {int(wait)}s)")
+        time.sleep(wait)
+        wait = min(wait * 1.5, interval)
+
+    log.error(f"Slurmdbd is still not ready after {maxtries} tries. Expecting issues!")
+    raise Exception("Slurmdbd is not ready")
+
 def setup_slurm_health_check_service(lkp: util.Lookup) -> None:
     if not lkp.cfg.get("slurm_backup_controller_name") or not lkp.cfg.get("enable_controller_load_balancer"):
         return
@@ -546,8 +566,9 @@ def setup_controller(is_primary: bool):
     run("systemctl enable slurmdbd", timeout=30)
     run("systemctl restart slurmdbd", timeout=30)
 
-    # Wait for slurmdbd to come up
-    time.sleep(5)
+    # Wait for slurmdbd to come up.
+    # This can take a while in case of database migration after upgrades (up to an hour).
+    check_slurmdbd_ready(maxtries=30, interval=240)
 
     sacctmgr = f"{slurmdirs.prefix}/bin/sacctmgr -i"
     result = run(

community/modules/scheduler/schedmd-slurm-gcp-v6-controller/modules/slurm_files/scripts/slurmsync.py

@@ -451,32 +451,40 @@ def sync_instances():
         action.apply(list(nodes))
 
 
-def reconfigure_slurm():
+def reconfigure_slurm() -> bool:
+    """Fetch new config and regenerate Slurm config files.
+
+    Returns True if config was updated and a scontrol reconfigure is needed.
+    On the controller, the reconfigure is deferred until after topology
+    regeneration to avoid issuing two sequential scontrol reconfigure calls
+    in the same sync cycle (which can cause a socket timeout).
+    """
     update_msg = "*** slurm configuration was updated ***"
 
     if lookup().cfg.hybrid:
         # terraform handles generating the config.yaml, don't do it here
-        return
+        return False
 
     upd, cfg_new = util.fetch_config()
     if not upd:
         log.debug("No changes in config detected.")
-        return
+        return False
     log.debug("Changes in config detected. Reconfiguring Slurm now.")
     util.update_config(cfg_new)
 
     if lookup().is_controller:
         conf.get_generator(lookup()).generate_configs()
 
-        log.info("Restarting slurmctld to make changes take effect.")
+        # Defer slurmctld restart and scontrol reconfigure:
+        # Running them now would cause slurmctld to temporarily load new node defs with old topology.
+        # This causes slurmctld to hang/timeout when update_topology tries to reconfigure it subsequently.
+        log.info("Config files regenerated. Deferring scontrol reconfigure until after topology update.")
         try:
-            # TODO: consider removing "restart" since "reconfigure" should restart slurmctld as well
-            run("sudo systemctl restart slurmctld.service", check=False)
-            util.scontrol_reconfigure(lookup())
+            util.run(f"wall '{update_msg}'", timeout=30)
         except Exception:
-            log.exception("failed to reconfigure slurmctld")
-        util.run(f"wall '{update_msg}'", timeout=30)
+            log.warning("failed to broadcast wall message")
         log.debug("Done.")
+        return True
     elif lookup().instance_role_safe == "compute":
         log.info("Restarting slurmd to make changes take effect.")
         run("systemctl restart slurmd")
@@ -487,19 +495,23 @@ def reconfigure_slurm():
         run("systemctl restart sackd")
         util.run(f"wall '{update_msg}'", timeout=30)
         log.debug("Done.")
+    return False
 
 
 def _generate_topology(lkp: util.Lookup) -> Tuple[bool, Any]:
     return conf.get_generator(lkp).generate_topology_data()
 
-def update_topology(lkp: util.Lookup) -> None:
+def update_topology(lkp: util.Lookup) -> Tuple[bool, Any]:
+    """Regenerate topology files.
+
+    Returns (updated, summary) so the caller can decide when to
+    issue the scontrol reconfigure and persist the summary.
+    """
     updated, summary = _generate_topology(lkp) # type: ignore[attr-defined]
 
     if updated:
-        log.info("Topology configuration updated. Reconfiguring Slurm.")
-        util.scontrol_reconfigure(lkp)
-        # Safe summary only after Slurm got reconfigured, so summary reflects Slurm POV
-        summary.dump(lkp)
+        log.info("Topology configuration updated.")
+    return updated, summary
 
 
 def delete_reservation(lkp: util.Lookup, reservation_name: str) -> None:
@@ -662,11 +674,19 @@ def main():
     lkp = lookup()
     if util.should_mount_slurm_bucket() and not lkp.is_controller:
         return
+
+    # Track if reconfigure_slurm changed cloud.conf so we can trigger
+    # a deferred scontrol reconfigure inside update_topology
+    config_changed = False
     try:
-        reconfigure_slurm()
+        config_changed = reconfigure_slurm()
     except Exception:
         log.exception("failed to reconfigure slurm")
     if lkp.is_controller:
+        if not util.is_active_controller(lkp):
+            log.info("Local controller is in STANDBY mode. Skipping instance sync, repair, and reconfigure.")
+            return
+
         try:
             process_messages(lkp)
         except:
@@ -692,11 +712,30 @@ def main():
         except Exception:
             log.exception("failed to sync placement groups")
 
+        topology_changed = False
+        topology_summary = None
         try:
-            update_topology(lkp)
+            topology_changed, topology_summary = update_topology(lkp)
         except Exception:
             log.exception("failed to update topology")
 
+        # Single deferred reconfigure for both config and topology changes.
+        # Placed here so it runs even if topology generation failed above.
+        if config_changed or topology_changed:
+            reasons = []
+            if config_changed:
+                reasons.append("config updated")
+            if topology_changed:
+                reasons.append("topology changed")
+            try:
+                log.info(f"Reconfiguring Slurm ({', '.join(reasons)}).")
+                util.scontrol_reconfigure(lkp)
+                # Only dump summary after successful reconfigure so it reflects Slurm's view
+                if topology_changed and topology_summary is not None:
+                    topology_summary.dump(lkp)
+            except Exception:
+                log.exception("failed to reconfigure slurmctld")
+
         try:
             sync_maintenance_reservation(lkp)
         except Exception:

community/modules/scheduler/schedmd-slurm-gcp-v6-controller/modules/slurm_files/scripts/util.py

@@ -2381,12 +2381,74 @@ def update_config(cfg: NSDict) -> None:
     global _lkp
     _lkp = Lookup(cfg)
 
+def wait_slurmctld_up(lkp: Lookup, timeout: float = 60) -> None:
+    """Wait for slurmctld to respond to scontrol ping and report UP status.
+    This ensure scontrol reconfigure does not fail due to slurmctld not being ready
+    after a service restart.
+    """
+    log.info("Waiting for slurmctld to be fully up...")
+    for wait in backoff_delay(0.5, timeout=timeout):
+        try:
+            res = run(f"{lkp.scontrol} ping", check=False, timeout=5)
+            output = (res.stdout + res.stderr).lower()
+            if res.returncode == 0 and any(
+                "up" in line for line in output.splitlines() if "down" not in line
+            ):
+                log.info("slurmctld is fully up.")
+                return
+        except Exception as e:
+            log.debug(f"scontrol ping check failed: {e}")
+        if wait > 0:
+            sleep(wait)
+    raise TimeoutError(f"slurmctld is not fully up after {timeout} seconds")
+
 def scontrol_reconfigure(lkp: Lookup) -> None:
     log.info("Running systemctl restart slurmctld.service")
     run("sudo systemctl restart slurmctld.service", timeout=30)
+    wait_slurmctld_up(lkp)
     log.info("Running scontrol reconfigure")
     run(f"{lkp.scontrol} reconfigure")
 
+def is_active_controller(lkp: Lookup) -> bool:
+    """Returns True if the local node is the currently active Slurm controller.
+    In non-HA setups, always returns True for the controller.
+    In HA setups, queries scontrol ping to check if local node is active primary or active takeover backup.
+    """
+    if not lkp.is_controller:
+        return False
+
+    backup_name = lkp.cfg.get("slurm_backup_controller_name")
+    if not backup_name:
+        return True
+
+    hostname = socket.gethostname().split(".")[0]
+    is_backup_instance = hostname.endswith("-1")
+
+    try:
+        res = run(f"{lkp.scontrol} ping", check=False, timeout=5)
+        output = (res.stdout + res.stderr).lower()
+
+        primary_up = any(
+            "primary controller: up" in line
+            or "primary controller(up)" in line
+            or ("slurmctld(primary) at" in line and "is up" in line)
+            for line in output.splitlines()
+        )
+        backup_up = any(
+            "backup controller: up" in line
+            or "backup controller(up)" in line
+            or ("slurmctld(backup) at" in line and "is up" in line)
+            for line in output.splitlines()
+        )
+
+        if not is_backup_instance:
+            return primary_up
+        else:
+            return (not primary_up) and backup_up
+    except Exception as e:
+        log.warning(f"Failed to query scontrol ping for HA active check: {e}")
+        return not is_backup_instance
+
 def slurm_version_gte(v1: str, v2: str) -> bool:
     """Returns true if v1 >= v2, expects YY.MM format"""
     try: