diff --git a/AppImage/scripts/health_monitor.py b/AppImage/scripts/health_monitor.py index 5bbfbc1e..258b3824 100644 --- a/AppImage/scripts/health_monitor.py +++ b/AppImage/scripts/health_monitor.py @@ -385,6 +385,10 @@ class HealthMonitor: # PVE Critical Services PVE_SERVICES = ['pveproxy', 'pvedaemon', 'pvestatd', 'pve-cluster'] + # A restart passes through these for a few seconds; a service that stays + # in one of them longer is not restarting, it is failing to. + SERVICE_TRANSIENT_STATES = ('activating', 'deactivating') + SERVICE_SETTLE_SECONDS = 8 # P2 fix: Pre-compiled regex patterns for performance (avoid re-compiling on every line) _BENIGN_RE = None @@ -3893,6 +3897,28 @@ class HealthMonitor: print(f"[HealthMonitor] VMs/CTs persistence check failed: {e}") return {'status': 'UNKNOWN', 'reason': f'VM/CT check unavailable: {str(e)}', 'checks': {}, 'dismissable': True} + def _service_state(self, service: str) -> str: + """The state of a service, once a restart in progress has finished.""" + deadline = time.time() + self.SERVICE_SETTLE_SECONDS + while True: + state = subprocess.run( + ['systemctl', 'is-active', service], + capture_output=True, + text=True, + timeout=2 + ).stdout.strip() + if state not in self.SERVICE_TRANSIENT_STATES or time.time() >= deadline: + return state + time.sleep(1) + + def _package_update_running(self) -> bool: + """An update stops and restarts the services it replaces.""" + try: + from notification_events import is_apt_active_on_host + return is_apt_active_on_host() + except Exception: + return False + def _check_pve_services(self) -> Dict[str, Any]: """ Check critical Proxmox services with persistence tracking. @@ -3923,21 +3949,21 @@ class HealthMonitor: for service in services_to_check: try: - result = subprocess.run( - ['systemctl', 'is-active', service], - capture_output=True, - text=True, - timeout=2 - ) - - status = result.stdout.strip() - if result.returncode != 0 or status != 'active': + status = self._service_state(service) + # A service reloading its configuration keeps running. + if status not in ('active', 'reloading'): failed_services.append(service) service_details[service] = status or 'inactive' except Exception: failed_services.append(service) service_details[service] = 'error' + # During a package update nothing is concluded about these services + # in either direction: the next check after it does that. + in_update = [] + if failed_services and self._package_update_running(): + in_update, failed_services = failed_services, [] + # Auto-clear errors for services that are healthy this cycle. # Lifted out of the `if failed_services:` branch so it runs # unconditionally — otherwise a service that recovered while @@ -3945,7 +3971,7 @@ class HealthMonitor: # active forever (the dismissed-only path returned before the # cleanup loop). Audit Tier 5 — Health stack. for svc in services_to_check: - if svc not in failed_services: + if svc not in failed_services and svc not in in_update: error_key = f'{service_prefix}_service_{svc}' if health_persistence.is_error_active(error_key): health_persistence.clear_error(error_key) @@ -3962,6 +3988,12 @@ class HealthMonitor: 'error_key': error_key, 'dismissable': True, } + elif svc in in_update: + checks[svc] = { + 'status': 'OK', + 'detail': f"Service is {service_details.get(svc, 'inactive')}", + 'error_key': error_key, + } else: checks[svc] = { 'status': 'OK', diff --git a/AppImage/scripts/tests/test_pbs.py b/AppImage/scripts/tests/test_pbs.py index 67eccfbd..43510a13 100644 --- a/AppImage/scripts/tests/test_pbs.py +++ b/AppImage/scripts/tests/test_pbs.py @@ -274,14 +274,17 @@ class PBSManagerTests(unittest.TestCase): names = ("_check_pve_services", "_check_vms_cts_optimized", "_check_vms_cts_with_persistence", "_check_proxmox_storages", "_check_proxmox_storage", "_check_lxc_disk_usage", "_check_vm_disk_usage", "_check_lxc_mount_capacity", "_check_pve_storage_capacity") - functions = [node for node in ast.walk(source) if isinstance(node, ast.FunctionDef) and node.name in names] + functions = [node for node in ast.walk(source) if isinstance(node, ast.FunctionDef) + and node.name in (*names, "_service_state")] persistence = mock.Mock(); persistence.is_error_active.return_value = False run = mock.Mock(return_value=SimpleNamespace(returncode=0, stdout="active\n")) namespace = {"Dict": dict, "Any": object, "Optional": __import__('typing').Optional, "PBS_SERVICES": pbs.PBS_SERVICES, "health_persistence": persistence, - "subprocess": SimpleNamespace(run=run), "os": __import__('os')} + "subprocess": SimpleNamespace(run=run), "os": __import__('os'), "time": __import__('time')} exec(compile(ast.Module(body=functions, type_ignores=[]), '', 'exec'), namespace) instance = SimpleNamespace(platform_type="pbs", PVE_SERVICES=["pveproxy", "pvedaemon", "pvestatd", "pve-cluster"]) + instance.SERVICE_TRANSIENT_STATES, instance.SERVICE_SETTLE_SECONDS = (), 0 + instance._service_state = lambda service: namespace["_service_state"](instance, service) services = namespace["_check_pve_services"](instance) self.assertEqual(services["status"], "OK") self.assertEqual([call.args[0][2] for call in run.call_args_list], list(pbs.PBS_SERVICES)) diff --git a/AppImage/scripts/tests/test_pve_service_states.py b/AppImage/scripts/tests/test_pve_service_states.py new file mode 100644 index 00000000..3c85dae1 --- /dev/null +++ b/AppImage/scripts/tests/test_pve_service_states.py @@ -0,0 +1,84 @@ +"""A service reloading or restarting is working; a failed one is still reported.""" +import sys +import tempfile +import unittest +from pathlib import Path +from types import SimpleNamespace +from unittest.mock import patch + +SCRIPTS_DIR = Path(__file__).resolve().parents[1] +if str(SCRIPTS_DIR) not in sys.path: + sys.path.insert(0, str(SCRIPTS_DIR)) +TESTS_DIR = str(Path(__file__).resolve().parent) +if TESTS_DIR not in sys.path: + sys.path.insert(0, TESTS_DIR) + +from health_fixture_modules import load_health_modules + + +class ServiceStates(unittest.TestCase): + def setUp(self): + folder = tempfile.TemporaryDirectory() + self.addCleanup(folder.cleanup) + self.module, _, _ = load_health_modules(folder.name) + self.monitor = self.module.HealthMonitor.__new__(self.module.HealthMonitor) + self.persistence = self.module.health_persistence + + def check(self, states, updating=False): + """`states` gives each service its answers to is-active, one per call.""" + answers = {service: list(values) for service, values in states.items()} + + def is_active(command, **_kwargs): + pending = answers.get(command[-1], ['active']) + return SimpleNamespace(stdout=(pending.pop(0) if len(pending) > 1 else pending[0]) + '\n') + + with patch.object(self.module.subprocess, 'run', side_effect=is_active), \ + patch.object(self.module.time, 'sleep'), \ + patch.object(self.monitor, '_package_update_running', return_value=updating): + return self.monitor._check_pve_services() + + def active(self, service): + return self.persistence.is_error_active(f'pve_service_{service}') + + def test_a_service_reloading_is_not_a_failure(self): + result = self.check({'pveproxy': ['reloading']}) + self.assertEqual(result['status'], 'OK') + self.assertFalse(self.active('pveproxy')) + + def test_a_restart_that_ends_is_not_a_failure(self): + result = self.check({'pveproxy': ['deactivating', 'activating', 'active']}) + self.assertEqual(result['status'], 'OK') + self.assertFalse(self.active('pveproxy')) + + def test_a_service_that_never_finishes_starting_is_reported(self): + with patch.object(self.module.HealthMonitor, 'SERVICE_SETTLE_SECONDS', 0): + result = self.check({'pveproxy': ['activating']}) + self.assertEqual(result['status'], 'CRITICAL') + self.assertTrue(self.active('pveproxy')) + + def test_a_failed_service_is_reported_at_once(self): + for state in ('failed', 'inactive'): + with self.subTest(state=state): + result = self.check({'pvedaemon': [state]}) + self.assertEqual(result['status'], 'CRITICAL') + self.assertEqual(result['failed'], ['pvedaemon']) + + def test_a_package_update_stops_nothing_being_reported(self): + result = self.check({'pveproxy': ['inactive']}, updating=True) + self.assertEqual(result['status'], 'OK') + self.assertEqual(result['checks']['pveproxy']['detail'], 'Service is inactive') + self.assertFalse(self.active('pveproxy')) + + def test_a_package_update_does_not_clear_a_real_failure(self): + self.check({'pveproxy': ['failed']}) + self.check({'pveproxy': ['inactive']}, updating=True) + self.assertTrue(self.active('pveproxy')) + + def test_the_failure_shows_once_the_update_has_finished(self): + self.check({'pveproxy': ['inactive']}, updating=True) + result = self.check({'pveproxy': ['inactive']}) + self.assertEqual(result['status'], 'CRITICAL') + + +if __name__ == '__main__': + unittest.main()