Merge pull request #600 from lmiccini/instanceha-interruptible-sleep-and-docs

Make fencing sleeps interruptible, add dual-key HMAC tests, document small clusters

Author: openshift-merge-bot[bot]

docs/instanceha_architecture.md

@@ -2122,7 +2122,7 @@ spec:
       serviceAccountName: instanceha-instanceha
       securityContext:
         fsGroup: 42401
-      terminationGracePeriodSeconds: 30
+      terminationGracePeriodSeconds: 45
       containers:
       - name: instanceha
         image: <resolved from infra-instanceha-config ConfigMap or RELATED_IMAGE env var>
@@ -2218,6 +2218,52 @@ spec:
 
 ---
 
+## Small Cluster Tuning
+
+InstanceHA works on clusters of any size, but the default safety thresholds are tuned for medium-to-large deployments. On clusters with 3–5 compute nodes, the percentage-based safety gates can block evacuation during legitimate multi-node failures because a small absolute number of failures translates to a large percentage.
+
+### Affected Safety Gates
+
+| Gate | Default | 3-node impact | 4-node impact |
+|------|---------|---------------|---------------|
+| **THRESHOLD** (global, `>`) | 50% | 2 failures = 66.7% → **blocked** | 2 failures = 50% → allowed (check is `>`, not `>=`) |
+| **HEARTBEAT_CLIFF_THRESHOLD** (`>=`) | 50% | 2 heartbeat losses = 66.7% → **cliff triggered** | 2 losses = 50% → **cliff triggered** (check is `>=`) |
+| **Per-aggregate threshold** | N/A (absolute count) | Not affected (uses absolute `instanceha:max_failures`) | Not affected |
+
+Single-node failures are handled correctly at any cluster size.
+
+### Recommended Configuration
+
+For clusters with 3–5 compute nodes where simultaneous multi-node failures are a realistic scenario (not just network partitions):
+
+```yaml
+config:
+  THRESHOLD: 100              # Disable global threshold — trust fencing to proceed
+  CHECK_HEARTBEAT: true       # Enable heartbeat detection to distinguish host vs nova-compute failures
+  HEARTBEAT_CLIFF_THRESHOLD: 100  # Disable cliff detection — trust individual heartbeat status
+```
+
+Setting `THRESHOLD: 100` disables the global percentage check entirely. This is safe because:
+1. Heartbeat detection (`CHECK_HEARTBEAT`) provides the primary protection against fencing during network partitions
+2. The K8s API reachability gate (gate 1) suppresses fencing if the pod itself loses network connectivity
+3. Per-aggregate thresholds (if configured) still apply as an absolute count, which is better suited to small clusters
+
+If heartbeats are not deployed, a more conservative approach:
+
+```yaml
+config:
+  THRESHOLD: 67               # Allow up to 2/3 failure ratio
+```
+
+### Why the Defaults Are Conservative
+
+The default `THRESHOLD: 50` and `HEARTBEAT_CLIFF_THRESHOLD: 50` are deliberately conservative because:
+- False positives (unnecessary fencing) are more dangerous than false negatives (delayed evacuation)
+- On larger clusters, losing >50% of nodes simultaneously almost always indicates a network partition, not real failures
+- Small clusters are the exception, not the rule, in production OpenStack deployments
+
+---
+
 ## Configuration Options Reference
 
 This section describes each configuration option in the `_config_map`, including its purpose, validation rules, usage in the codebase, and testing coverage.
@@ -2345,6 +2391,7 @@ config:
 - Value of `0` blocks all evacuations (any failure percentage exceeds 0%)
 - Value of `100` disables threshold checking (nothing can exceed 100%)
 - Aggregate-aware calculation uses only evacuable hosts as denominator
+- **Small clusters**: On a 3-node cluster, 2 simultaneous failures = 66.7%, which exceeds the default 50%. On a 4-node cluster, 2 failures = 50% (not blocked, since the check is `>`). Operators of small clusters who expect multi-node failures should raise this value. See [Small Cluster Tuning](#small-cluster-tuning).
 
 ---
 
@@ -3008,6 +3055,34 @@ config:
 
 ---
 
+#### HEARTBEAT_CLIFF_THRESHOLD
+**Type**: Integer
+**Default**: `50`
+**Range**: `10-100` (percentage)
+
+**Description**: Maximum percentage of heartbeat-active hosts that can disappear in a single poll cycle before cliff detection triggers. When the drop exceeds this threshold, InstanceHA assumes a listener-side network partition rather than simultaneous host failures and skips all fencing for that cycle.
+
+**Usage**:
+- Evaluated in `_filter_reachable_hosts()` each poll cycle when `CHECK_HEARTBEAT` is enabled
+- Compares `heartbeat_previous_active_count` (previous cycle) against current active count
+- Only evaluated when the previous active count was >= 3 (avoids false triggers during startup ramp-up)
+
+**Behavior**:
+- If drop percentage >= `HEARTBEAT_CLIFF_THRESHOLD`: skip all fencing this cycle, emit `HeartbeatCliff` K8s event (Warning)
+- Baseline count is preserved during a cliff event, so the cliff re-triggers on subsequent cycles until heartbeats recover
+- When no cliff is detected, update `heartbeat_previous_active_count` for the next cycle
+
+**Small Cluster Considerations**: On a 3-node cluster with all nodes sending heartbeats, losing 2 nodes simultaneously causes a 66.7% drop — exceeding the default 50% threshold. This blocks fencing for that cycle. If simultaneous multi-node failures are expected (not partitions), raise this value (e.g., `100` to disable cliff detection entirely). See [Small Cluster Tuning](#small-cluster-tuning).
+
+**Example**:
+```yaml
+config:
+  CHECK_HEARTBEAT: true
+  HEARTBEAT_CLIFF_THRESHOLD: 100  # Disable cliff detection (trust individual heartbeat status)
+```
+
+---
+
 ### Service Control Options
 
 #### DISABLED

internal/instanceha/funcs.go

@@ -177,7 +177,7 @@ func Deployment(
 						FSGroup: ptr.To(instanceHaUID),
 					},
 					Volumes:                       volumes,
-					TerminationGracePeriodSeconds: ptr.To[int64](30),
+					TerminationGracePeriodSeconds: ptr.To[int64](45),
 					Containers: []corev1.Container{{
 						Name:    "instanceha",
 						Image:   containerImage,

templates/instanceha/bin/instanceha.py

@@ -405,11 +405,18 @@ def _make_ssl_request(method: str, url: str, auth: tuple, timeout: int,
 
 
 def _wait_for_power_off(check_func: Callable[[], Optional[str]], timeout: int,
-                       host_identifier: str, expected_state: str, agent_type: str) -> bool:
+                       host_identifier: str, expected_state: str, agent_type: str,
+                       shutdown_event=None) -> bool:
     """Wait for host to power off by polling power state."""
     deadline = time.monotonic() + timeout
     while time.monotonic() < deadline:
-        time.sleep(1)
+        if shutdown_event and shutdown_event.is_set():
+            logging.warning("Shutdown requested, aborting power-off wait for %s", host_identifier)
+            return False
+        if shutdown_event:
+            shutdown_event.wait(1)
+        else:
+            time.sleep(1)
         power_state = check_func()
         if power_state == expected_state:
             logging.info("%s power off successful for %s", agent_type, host_identifier)
@@ -1720,7 +1727,7 @@ def _host_evacuate(connection, failed_service, service, target_host=None) -> boo
         logging.info("Nothing to evacuate")
         return True
 
-    time.sleep(service.config.get_config_value('DELAY'))
+    _interruptible_sleep(service.shutdown_event, service.config.get_config_value('DELAY'))
 
     if service.config.get_config_value('ORCHESTRATED_RESTART') is True or service.config.get_config_value('SMART_EVACUATION'):
         return _concurrent_evacuate(connection, evacuables, service, host, failed_service.id, target_host=target_host)
@@ -1929,7 +1936,7 @@ def _server_evacuate_future(connection, server, target_host=None,
             return False
 
         logging.debug("Starting evacuation of %s", response.uuid)
-        time.sleep(INITIAL_EVACUATION_WAIT_SECONDS)
+        _interruptible_sleep(shutdown_event, INITIAL_EVACUATION_WAIT_SECONDS)
 
         result = _monitor_evacuation(connection, server.id, response.uuid, start_time,
                                      max_retries=max_retries,
@@ -2194,7 +2201,7 @@ def _get_power_state(agent_type: str, session: requests.Session = None, **kwargs
 
     return None
 
-def _redfish_reset(url, user, passwd, timeout, action, config_mgr):
+def _redfish_reset(url, user, passwd, timeout, action, config_mgr, shutdown_event=None):
     """Perform a Redfish reset operation on a computer system."""
     if not all([url, user, passwd, action]):
         logging.error("Redfish reset failed: missing required parameters")
@@ -2225,7 +2232,8 @@ def _redfish_reset(url, user, passwd, timeout, action, config_mgr):
                     try:
                         return _wait_for_power_off(
                             lambda: _get_power_state('redfish', session=poll_session, url=url, user=user, passwd=passwd, timeout=timeout, config_mgr=config_mgr),
-                            timeout, safe_url, 'OFF', 'Redfish')
+                            timeout, safe_url, 'OFF', 'Redfish',
+                            shutdown_event=shutdown_event)
                     finally:
                         poll_session.close()
                 else:
@@ -2345,7 +2353,13 @@ def _bmh_wait_for_power_off(get_url, headers, cacert, host, timeout, poll_interv
 
     with requests.Session() as session:
         while time.monotonic() < timeout_end:
-            time.sleep(poll_interval)
+            if service.shutdown_event and service.shutdown_event.is_set():
+                logging.warning("Shutdown requested, aborting power-off wait for %s", host)
+                return False
+            if service.shutdown_event:
+                service.shutdown_event.wait(poll_interval)
+            else:
+                time.sleep(poll_interval)
 
             try:
                 response = session.get(get_url, headers=headers, verify=cacert, timeout=request_timeout)
@@ -2393,7 +2407,7 @@ def _validate_fencing_inputs(fencing_data, agent_name) -> bool:
         validations['username'] = fencing_data["login"]
     return validate_inputs(validations, agent_name)
 
-def _execute_ipmi_fence(host, action, fencing_data, timeout) -> bool:
+def _execute_ipmi_fence(host, action, fencing_data, timeout, shutdown_event=None) -> bool:
     """Execute IPMI fencing operation with retry logic."""
     timeout = max(5, min(300, int(timeout or 30)))
     ipaddr, ipport, login, passwd = fencing_data["ipaddr"], fencing_data["ipport"], \
@@ -2415,7 +2429,8 @@ def _execute_ipmi_fence(host, action, fencing_data, timeout) -> bool:
     if action == 'off':
         return _wait_for_power_off(
             lambda: _get_power_state('ipmi', ip=ipaddr, port=ipport, user=login, passwd=passwd, timeout=timeout),
-            timeout, host, 'CHASSIS POWER IS OFF', 'IPMI')
+            timeout, host, 'CHASSIS POWER IS OFF', 'IPMI',
+            shutdown_event=shutdown_event)
 
     logging.info("IPMI %s successful for %s", action, host)
     return True
@@ -2433,7 +2448,8 @@ def _fence_ipmi(host, action, fencing_data, service):
     if not _validate_fencing_inputs(fencing_data, "IPMI"):
         return False
     timeout = fencing_data.get("timeout", service.config.get_config_value('FENCING_TIMEOUT'))
-    return _execute_ipmi_fence(host, action, fencing_data, timeout)
+    return _execute_ipmi_fence(host, action, fencing_data, timeout,
+                              shutdown_event=service.shutdown_event)
 
 
 def _fence_redfish(host, action, fencing_data, service):
@@ -2448,7 +2464,8 @@ def _fence_redfish(host, action, fencing_data, service):
     redfish_action = "ForceOff" if action == "off" else "On"
     timeout = fencing_data.get("timeout", service.config.get_config_value('FENCING_TIMEOUT'))
     return _redfish_reset(url, fencing_data["login"], fencing_data["passwd"],
-                        timeout, redfish_action, service.config)
+                        timeout, redfish_action, service.config,
+                        shutdown_event=service.shutdown_event)
 
 
 def _fence_bmh(host, action, fencing_data, service):
@@ -3545,6 +3562,7 @@ def _sigterm_handler(signum, frame):
 
             services = conn.services.list(binary="nova-compute")
             if not services:
+                logging.info("Poll cycle completed: no compute services found, next poll in %ds", poll_interval)
                 service.shutdown_event.wait(poll_interval)
                 continue
 
@@ -3556,9 +3574,12 @@ def _sigterm_handler(signum, frame):
             _process_stale_services(conn, service, services, compute_nodes_list, to_resume)
             _process_reenabling(conn, service, to_reenable)
 
+            stale_count = len(compute_nodes_list)
             if not service.ready:
                 service.ready = True
                 logging.info("Readiness probe active — first poll cycle completed")
+            logging.info("Poll cycle completed: %d compute services, %d stale, next poll in %ds",
+                         len(services), stale_count, poll_interval)
             consecutive_failures = 0
             POLL_CONSECUTIVE_FAILURES.set(0)
             POLL_CYCLE_TOTAL.labels(result='success').inc()

test/instanceha/test_config_features.py

@@ -495,7 +495,7 @@ def test_delay_zero_skips_sleep(self):
         mock_server.id = 'server-123'
 
         # Mock the evacuation functions to avoid actual evacuation
-        with patch('time.sleep') as mock_sleep:
+        with patch('instanceha._interruptible_sleep') as mock_sleep:
             with patch('instanceha._get_evacuable_servers', return_value=[mock_server]):
                 with patch('instanceha._traditional_evacuate', return_value=True):
                     failed_service = Mock()
@@ -505,8 +505,8 @@ def test_delay_zero_skips_sleep(self):
                     # Call _host_evacuate which contains the DELAY sleep
                     result = instanceha._host_evacuate(mock_conn, failed_service, service)
 
-                    # Verify sleep was called with 0 (DELAY=0)
-                    mock_sleep.assert_called_once_with(0)
+                    # Verify interruptible sleep was called with shutdown_event and 0 (DELAY=0)
+                    mock_sleep.assert_called_once_with(service.shutdown_event, 0)
 
     def test_delay_non_zero_delays_evacuation(self):
         """Test that DELAY>0 delays evacuation by specified seconds."""
@@ -524,7 +524,7 @@ def test_delay_non_zero_delays_evacuation(self):
         mock_server = Mock()
         mock_server.id = 'server-123'
 
-        with patch('time.sleep') as mock_sleep:
+        with patch('instanceha._interruptible_sleep') as mock_sleep:
             with patch('instanceha._get_evacuable_servers', return_value=[mock_server]):
                 with patch('instanceha._traditional_evacuate', return_value=True):
                     failed_service = Mock()
@@ -533,8 +533,8 @@ def test_delay_non_zero_delays_evacuation(self):
 
                     result = instanceha._host_evacuate(mock_conn, failed_service, service)
 
-                    # Verify sleep was called with configured DELAY value
-                    mock_sleep.assert_called_once_with(5)
+                    # Verify interruptible sleep was called with shutdown_event and configured DELAY value
+                    mock_sleep.assert_called_once_with(service.shutdown_event, 5)
 
     def test_delay_with_smart_evacuation(self):
         """Test that DELAY works with SMART_EVACUATION enabled."""
@@ -552,7 +552,7 @@ def test_delay_with_smart_evacuation(self):
         mock_server = Mock()
         mock_server.id = 'server-123'
 
-        with patch('time.sleep') as mock_sleep:
+        with patch('instanceha._interruptible_sleep') as mock_sleep:
             with patch('instanceha._get_evacuable_servers', return_value=[mock_server]):
                 with patch('instanceha._concurrent_evacuate', return_value=True):
                     failed_service = Mock()
@@ -561,8 +561,8 @@ def test_delay_with_smart_evacuation(self):
 
                     result = instanceha._host_evacuate(mock_conn, failed_service, service)
 
-                    # Verify sleep was called with DELAY before smart evacuation
-                    mock_sleep.assert_called_once_with(3)
+                    # Verify interruptible sleep was called with shutdown_event and DELAY before smart evacuation
+                    mock_sleep.assert_called_once_with(service.shutdown_event, 3)
 
     def test_delay_boundary_values(self):
         """Test DELAY boundary values (min=0, max=300)."""

test/instanceha/test_coverage_gaps.py

@@ -274,16 +274,20 @@ def test_invalid_ip_returns_false(self):
     def test_valid_params_calls_execute(self, mock_exec):
         """Valid params delegates to _execute_ipmi_fence."""
         data = {"ipaddr": "192.168.1.10", "ipport": "623", "login": "admin", "passwd": "secret"}
-        result = instanceha._fence_ipmi("host1", "off", data, self._make_service())
+        svc = self._make_service()
+        result = instanceha._fence_ipmi("host1", "off", data, svc)
         self.assertTrue(result)
-        mock_exec.assert_called_once_with("host1", "off", data, 30)
+        mock_exec.assert_called_once_with("host1", "off", data, 30,
+                                          shutdown_event=svc.shutdown_event)
 
     @patch('instanceha._execute_ipmi_fence', return_value=True)
     def test_uses_custom_timeout_from_fencing_data(self, mock_exec):
         """Uses timeout from fencing_data if provided."""
         data = {"ipaddr": "192.168.1.10", "ipport": "623", "login": "admin", "passwd": "secret", "timeout": 60}
-        instanceha._fence_ipmi("host1", "off", data, self._make_service())
-        mock_exec.assert_called_once_with("host1", "off", data, 60)
+        svc = self._make_service()
+        instanceha._fence_ipmi("host1", "off", data, svc)
+        mock_exec.assert_called_once_with("host1", "off", data, 60,
+                                          shutdown_event=svc.shutdown_event)
 
     @patch('instanceha._execute_ipmi_fence', return_value=False)
     def test_returns_false_when_execute_fails(self, mock_exec):