contest: hw: mctrl: make reboot handling more robust

SSH reboots can get stuck.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Author: kuba-moo

contest/hw/README.rst

@@ -240,9 +240,10 @@ machine_state
 
 If the machine is not reserved service SSHs to it every 5min and
 checks uptime and kernel version. If SSH fails machine state progresses
-HEALTHY -> MISS_ONE -> MISS_TWO. If SSH fails again, power cycle is
-issued and machine enters REBOOT_ISSUED, where it stays until it
-starts responding to SSH again.
+HEALTHY -> MISS_ONE -> MISS_TWO. After three missed checks the machine
+is power-cycled via BMC (POWER_CYCLE_ISSUED). If the machine is still
+down after power cycle, the miss counter restarts (MISS_ONE) and the
+cycle repeats.
 
 If the machine is RESERVED the machine state is just tracking last
 refresh time to potentially time out the reservation.
@@ -253,7 +254,7 @@ to reserve a machine not in HEALTHY state we respond with try again.
 Refreshed every 5min, and immediately after reservation is released.
 
  - machine ID
- - state, enum: RESERVED, HEALTHY, MISS_ONE, MISS_TWO, REBOOT_ISSUED
+ - state, enum: RESERVED, HEALTHY, MISS_ONE, MISS_TWO, POWER_CYCLE_ISSUED
  - last_reservation: CLOSED, TIMEOUT
  - reservation_last_refresh
  - uptime

contest/hw/lib/health.py

@@ -5,6 +5,7 @@
 import enum
 import subprocess
 import threading
+import time
 
 
 class MachineState(enum.Enum):
@@ -14,7 +15,12 @@ class MachineState(enum.Enum):
     RESERVED = "RESERVED"
     MISS_ONE = "MISS_ONE"
     MISS_TWO = "MISS_TWO"
-    REBOOT_ISSUED = "REBOOT_ISSUED"
+    SSH_REBOOT_ISSUED = "SSH_REBOOT_ISSUED"
+    POWER_CYCLE_ISSUED = "POWER_CYCLE_ISSUED"
+
+
+# How long to wait for an SSH reboot before escalating to power cycle (sec)
+SSH_REBOOT_TIMEOUT = 600
 
 
 class HealthChecker(threading.Thread):
@@ -24,23 +30,18 @@ class HealthChecker(threading.Thread):
     State transitions:
       HEALTHY -> MISS_ONE (SSH fail)
       MISS_ONE -> MISS_TWO (SSH fail again)
-      MISS_TWO -> REBOOT_ISSUED (SSH fail, triggers power cycle)
-      REBOOT_ISSUED -> HEALTHY (SSH succeeds)
+      MISS_TWO -> POWER_CYCLE_ISSUED (SSH fail, BMC power cycle)
+      SSH_REBOOT_ISSUED -> POWER_CYCLE_ISSUED (>10min and still down)
+      SSH_REBOOT_ISSUED -> HEALTHY (SSH succeeds)
+      POWER_CYCLE_ISSUED -> MISS_ONE (SSH fail, restart miss counter)
+      POWER_CYCLE_ISSUED -> HEALTHY (SSH succeeds)
       Any state -> HEALTHY (SSH succeeds)
+
+    SSH_REBOOT_ISSUED is set by the reservation manager when it reboots
+    a machine via SSH after releasing a reservation. The health checker
+    monitors it and escalates to power cycle if SSH doesn't come back.
     """
     def __init__(self, machines, bmc_map, interval=300, lock=None):
-        """
-        Parameters
-        ----------
-        machines : dict
-            machine_id -> {'name': str, 'mgmt_ipaddr': str, 'state': MachineState}
-        bmc_map : dict
-            machine_id -> BMC instance
-        interval : int
-            Seconds between health check rounds
-        lock : threading.Lock, optional
-            External lock for machines dict; creates own if not provided
-        """
         super().__init__(daemon=True)
         self.machines = machines
         self.bmc_map = bmc_map
@@ -77,28 +78,39 @@ def check_machine(self, machine_id, machine):
         alive = self._ssh_check(ipaddr)
 
         with self.lock:
-            # Re-check in case state changed while we were polling
             state = machine['state']
             if state == MachineState.RESERVED:
                 return
 
             if alive:
                 machine['state'] = MachineState.HEALTHY
+                machine.pop('ssh_reboot_at', None)
             elif state == MachineState.HEALTHY:
                 machine['state'] = MachineState.MISS_ONE
                 print(f"Health: {machine['name']} missed one check")
             elif state == MachineState.MISS_ONE:
                 machine['state'] = MachineState.MISS_TWO
                 print(f"Health: {machine['name']} missed two checks")
             elif state == MachineState.MISS_TWO:
-                machine['state'] = MachineState.REBOOT_ISSUED
-                print(f"Health: {machine['name']} missed three checks, rebooting")
                 bmc = self.bmc_map.get(machine_id)
                 if bmc:
                     bmc.power_cycle()
-            elif state == MachineState.REBOOT_ISSUED:
-                # Still waiting for reboot to take effect
-                pass
+                machine['state'] = MachineState.POWER_CYCLE_ISSUED
+                print(f"Health: {machine['name']} missed three checks, "
+                      "power cycling")
+            elif state == MachineState.SSH_REBOOT_ISSUED:
+                elapsed = time.monotonic() - machine.get('ssh_reboot_at', 0)
+                if elapsed >= SSH_REBOOT_TIMEOUT:
+                    bmc = self.bmc_map.get(machine_id)
+                    if bmc:
+                        bmc.power_cycle()
+                    machine['state'] = MachineState.POWER_CYCLE_ISSUED
+                    machine.pop('ssh_reboot_at', None)
+                    print(f"Health: {machine['name']} SSH reboot timed out, "
+                          "power cycling")
+            elif state == MachineState.POWER_CYCLE_ISSUED:
+                machine['state'] = MachineState.MISS_ONE
+                print(f"Health: {machine['name']} still down after power cycle")
 
     def run(self):
         while not self._stop_event.is_set():

contest/hw/lib/reservations.py

@@ -5,6 +5,7 @@
 import datetime
 import subprocess
 import threading
+import time
 
 import psycopg2
 
@@ -156,15 +157,16 @@ def _finish_close(self, reservation_id, machine_ids):
             if ipaddr and self._ssh_reboot(ipaddr):
                 print(f"Reservation: SSH reboot sent to machine {mid} ({ipaddr})")
                 with self.lock:
-                    machine['state'] = MachineState.REBOOT_ISSUED
+                    machine['state'] = MachineState.SSH_REBOOT_ISSUED
+                    machine['ssh_reboot_at'] = time.monotonic()
             else:
                 # Fall back to BMC power cycle
                 bmc = self.bmc_map.get(mid)
                 if bmc:
                     print(f"Reservation: BMC power cycle for machine {mid}")
                     bmc.power_cycle()
                     with self.lock:
-                        machine['state'] = MachineState.REBOOT_ISSUED
+                        machine['state'] = MachineState.POWER_CYCLE_ISSUED
 
     @staticmethod
     def _ssh_reboot(ipaddr):

contest/hw/tests/test_machine_control.py

@@ -13,7 +13,7 @@
 
 from lib.bmc import BMC
 from lib.health import HealthChecker, MachineState
-from lib.sol_listener import SOLListener
+from lib.sol_listener import SOLCollector as SOLListener
 from lib.reservations import ReservationManager
 
 
@@ -169,11 +169,12 @@ def test_miss_two_triggers_reboot(self):
             mock_run.return_value = mock.Mock(returncode=1)
             hc.check_machine(1, machines[1])
 
-        self.assertEqual(machines[1]['state'], MachineState.REBOOT_ISSUED)
+        # SSH unreachable -> power cycle
+        self.assertEqual(machines[1]['state'], MachineState.POWER_CYCLE_ISSUED)
         bmc_map[1].power_cycle.assert_called_once()
 
-    def test_reboot_issued_to_healthy(self):
-        machines = self._make_machines({1: MachineState.REBOOT_ISSUED})
+    def test_power_cycle_issued_to_healthy(self):
+        machines = self._make_machines({1: MachineState.POWER_CYCLE_ISSUED})
         bmc_map = self._make_bmc_map()
         hc = HealthChecker(machines, bmc_map)
 
@@ -183,6 +184,18 @@ def test_reboot_issued_to_healthy(self):
 
         self.assertEqual(machines[1]['state'], MachineState.HEALTHY)
 
+    def test_power_cycle_issued_still_down(self):
+        machines = self._make_machines({1: MachineState.POWER_CYCLE_ISSUED})
+        bmc_map = self._make_bmc_map()
+        hc = HealthChecker(machines, bmc_map)
+
+        with mock.patch('subprocess.run') as mock_run:
+            mock_run.return_value = mock.Mock(returncode=1)
+            hc.check_machine(1, machines[1])
+
+        # Restart miss counter
+        self.assertEqual(machines[1]['state'], MachineState.MISS_ONE)
+
     def test_reserved_skipped(self):
         machines = self._make_machines({1: MachineState.RESERVED})
         bmc_map = self._make_bmc_map()
@@ -339,7 +352,7 @@ def test_timeout(self):
 
         mgr.check_timeouts()
 
-        self.assertEqual(machines[1]['state'], MachineState.HEALTHY)
+        self.assertEqual(machines[1]['state'], MachineState.POWER_CYCLE_ISSUED)
         self.assertNotIn(42, mgr.active)
         bmc_map[1].power_cycle.assert_called_once()
 
@@ -349,8 +362,8 @@ def test_close(self):
 
         ok, err = mgr.close('test-caller', 42)
         self.assertTrue(ok)
-        self.assertEqual(machines[1]['state'], MachineState.HEALTHY)
-        self.assertEqual(machines[2]['state'], MachineState.HEALTHY)
+        self.assertEqual(machines[1]['state'], MachineState.POWER_CYCLE_ISSUED)
+        self.assertEqual(machines[2]['state'], MachineState.POWER_CYCLE_ISSUED)
         bmc_map[1].power_cycle.assert_called_once()
         bmc_map[2].power_cycle.assert_called_once()
 
@@ -491,7 +504,7 @@ def test_reserve_and_close(self):
         self.assertEqual(resp.status_code, 200)
 
         self.assertEqual(
-            self.mc_mod.machines[1]['state'], MachineState.HEALTHY
+            self.mc_mod.machines[1]['state'], MachineState.POWER_CYCLE_ISSUED
         )
 
     def test_reservation_refresh(self):