From 4bfd6c44295bc05d3e24df379f8dbafd4acc9f94 Mon Sep 17 00:00:00 2001 From: oskar Date: Tue, 14 Jul 2026 20:21:03 +0200 Subject: [PATCH] =?UTF-8?q?fix(observer):=20map=20containers=5Fnot=5Frunni?= =?UTF-8?q?ng=20to=20unhealthy=20status=20+=20incident=20=E2=80=94=20dead/?= =?UTF-8?q?crash-looping=20containers=20created=20no=20incident,=20so=20su?= =?UTF-8?q?pervisor=20never=20alerted?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit node-agent (and stability-agent) emit containers_not_running for exited/dead and crash-looping containers, but the observer's event->status/incident map only handled service_recovered/service_healthy/service_unhealthy/ healthcheck_failed. containers_not_running fell through: status stayed at its last "healthy" value and no incident was opened, so the supervisor saw no drift and generated no action — a dead container produced ZERO operator alerts (matches the "action queue empty despite failures" symptom). - containers_not_running now sets status=unhealthy and opens an incident whose trigger_type ("containers_not_running") is already in the supervisor's CONTAINER_RESTART_TRIGGERS, so remediation (container_restart) fires with no supervisor change. Recovery is unchanged: service_healthy resolves the incident via the existing svc_key->incident_id link. - container_restarting / container_state_unexpected (added in 4746ebe) are kept intentionally observational — no incident, status not flipped to unhealthy (would cause a false redeploy for a transient blip) — but leave a last_observation trace so they don't vanish. A real crash-loop still escalates via node-agent re-emitting containers_not_running. Tests: services/control-plane/tests/test_observer_container_events.py — status + incident + trigger_type, end-to-end reconcile -> container_restart, recovery auto-resolve, observational no-incident/trace, idempotent (no incident multiplication). Full control-plane suite: 117 passed. Co-Authored-By: Claude Opus 4.8 (1M context) --- scripts/observer/observer.py | 43 ++- .../tests/test_observer_container_events.py | 253 ++++++++++++++++++ 2 files changed, 295 insertions(+), 1 deletion(-) create mode 100644 services/control-plane/tests/test_observer_container_events.py diff --git a/scripts/observer/observer.py b/scripts/observer/observer.py index 3f07323..86ebff7 100644 --- a/scripts/observer/observer.py +++ b/scripts/observer/observer.py @@ -750,9 +750,50 @@ class Observer: # unhealthy/crashing is now confirmed healthy, the incident is over. self.world_state["services"][svc_key]["status"] = "healthy" self._resolve_incident(svc_key, timestamp) - elif etype in ["service_unhealthy", "healthcheck_failed"]: + elif etype in ["service_unhealthy", "healthcheck_failed", "containers_not_running"]: + # containers_not_running: node-agent (node_agent.py) — and the + # stability-agent — report a *managed* container that has exited / + # dead or is crash-looping (restarting past RestartCount threshold). + # Treat it exactly like the other hard-failure signals: mark the + # service unhealthy and open an incident. The incident's + # trigger_type is the event type ("containers_not_running"), which + # the supervisor already recognises in CONTAINER_RESTART_TRIGGERS and + # remediates with a low-risk container_restart (vs. a full redeploy). + # + # Before this branch existed, containers_not_running fell through + # this if/elif chain entirely: node-agent saw the dead container and + # emitted the event, the observer bumped last_check but left status at + # its last "healthy" value and created NO incident. The supervisor + # then saw no drift and generated no action — so a dead / crash-looping + # container produced ZERO operator alerts. The monitoring loop was + # silently broken (matches the "action queue empty despite failures" + # symptom). (node_agent.py's own comment claims this event "rides the + # existing, supervisor-wired remediation path" — that path only exists + # once the observer opens the incident here.) self.world_state["services"][svc_key]["status"] = "unhealthy" self._handle_incident(svc_key, event) + elif etype in ["container_restarting", "container_state_unexpected"]: + # Intentionally observational — parity with node_agent.check_containers, + # which emits these as low/medium severity and explicitly does NOT wire + # them to any supervisor trigger. Causes: a transient post-deploy + # restart still below the crash-loop threshold, an operator `docker + # pause`, or an unhandled-but-not-fault docker state. + # + # We deliberately do NOT set status=unhealthy (that would make the + # supervisor generate remediation for a transient blip — a redeploy, + # since there is no CONTAINER_RESTART_TRIGGERS incident) and do NOT + # open an incident (noise). But the event must not vanish, so we + # record a lightweight observational trace on the service entry; the + # raw event also stays visible in the /events feed. If a restart is a + # real crash-loop it escalates on its own: node-agent re-emits it as + # containers_not_running once RestartCount crosses the threshold, and + # the branch above then alarms. + self.world_state["services"][svc_key]["last_observation"] = { + "type": etype, + "severity": severity, + "timestamp": timestamp, + "message": event.get("message"), + } # 3. Update Deployment State if etype.startswith("deployment_") and cid: diff --git a/services/control-plane/tests/test_observer_container_events.py b/services/control-plane/tests/test_observer_container_events.py new file mode 100644 index 0000000..9329b6f --- /dev/null +++ b/services/control-plane/tests/test_observer_container_events.py @@ -0,0 +1,253 @@ +"""Tests for observer handling of node-agent container-state events. + +Regression suite for the "dead/crash-looping container = silent" bug: the +observer's process_event event→status/incident map handled service_recovered, +service_healthy, service_unhealthy and healthcheck_failed but NOT +containers_not_running (emitted by node-agent for exited/dead/crash-loop +containers, and by stability-agent). A dead container therefore left the +service at its last "healthy" status with no incident, so the supervisor saw no +drift and produced no action — the operator was never alerted. + +Covers: + 1. containers_not_running → status=unhealthy + incident with the right trigger_type + 2. the incident's trigger_type is one the supervisor already routes to a + container_restart (end-to-end reconcile, no supervisor change required) + 3. service_healthy after containers_not_running → incident auto-resolved + 4. container_restarting / container_state_unexpected → observational: no + incident, status not flipped to unhealthy, but a trace is left behind + 5. idempotency: reprocessing containers_not_running does not multiply incidents +""" +from __future__ import annotations + +import json +import sys +import time +from pathlib import Path + +import pytest + +# Observer lives outside the control-plane package; add scripts/ to path. +sys.path.insert(0, str(Path(__file__).parent.parent.parent.parent / "scripts")) +sys.path.insert(0, str(Path(__file__).parent.parent / "src")) + +from observer.observer import Observer # noqa: E402 +import supervisor as supervisor_module # noqa: E402 +from supervisor import Supervisor, CONTAINER_RESTART_TRIGGERS # noqa: E402 + + +# --------------------------------------------------------------------------- +# Observer fixture — redirect every runtime path into a per-test tmp_path. +# Mirrors the fixture in test_incident_lifecycle.py (incl. OBSERVER_STATE_FILE, +# which is derived from STATE_DIR at import time). +# --------------------------------------------------------------------------- + +@pytest.fixture +def observer(tmp_path, monkeypatch) -> Observer: + import observer.observer as obs_mod + + world = tmp_path / "world" + state = tmp_path / "state" + events = tmp_path / "events" + logs = tmp_path / "logs" + repo = tmp_path / "repo" + for d in (world, state, events, logs, repo / "inventory", repo / "hosts"): + d.mkdir(parents=True, exist_ok=True) + + (repo / "inventory" / "topology.yaml").write_text( + "nodes:\n piha:\n roles: [infra]\n connectivity: {}\n" + ) + + monkeypatch.setattr(obs_mod, "WORLD_DIR", world) + monkeypatch.setattr(obs_mod, "STATE_DIR", state) + monkeypatch.setattr(obs_mod, "EVENTS_DIR", events) + monkeypatch.setattr(obs_mod, "LOGS_DIR", logs) + monkeypatch.setattr(obs_mod, "INVENTORY_TOPOLOGY", repo / "inventory" / "topology.yaml") + monkeypatch.setattr(obs_mod, "REPO_ROOT", repo) + monkeypatch.setattr(obs_mod, "FAILED_EVENTS_DIR", state / "observer_failed_events") + monkeypatch.setattr(obs_mod, "OBSERVER_STATE_FILE", state / "observer_checkpoint.json") + return Observer() + + +def _container_event(etype, *, node="piha", service="paperless", + severity="high", ts=None, payload=None): + return { + "id": f"evt-{node}-{ts or int(time.time())}-{etype}-{service}", + "type": etype, + "node": node, + "service": service, + "severity": severity, + "timestamp": ts or int(time.time()), + "message": f"Container '{service}' {etype}", + "payload": payload or {"container": service, "status": "exited"}, + } + + +# --------------------------------------------------------------------------- +# 1. containers_not_running → unhealthy + incident +# --------------------------------------------------------------------------- + +def test_containers_not_running_sets_unhealthy(observer): + observer.process_event(_container_event("containers_not_running")) + svc = observer.world_state["services"]["piha/paperless"] + assert svc["status"] == "unhealthy" + assert svc["incident_id"] is not None + + +def test_containers_not_running_creates_incident_with_trigger_type(observer): + ev = _container_event("containers_not_running") + observer.process_event(ev) + + svc = observer.world_state["services"]["piha/paperless"] + inc = observer.world_state["incidents"][svc["incident_id"]] + assert inc["status"] == "active" + assert inc["node"] == "piha" + assert inc["service"] == "paperless" + # trigger_type is the event type, which the supervisor routes to container_restart. + assert inc["trigger_type"] == "containers_not_running" + + +def test_incident_trigger_type_is_recognized_by_supervisor(observer): + """The trigger_type the observer stamps MUST be one the supervisor routes to + a container_restart — otherwise remediation silently never fires.""" + observer.process_event(_container_event("containers_not_running")) + svc = observer.world_state["services"]["piha/paperless"] + inc = observer.world_state["incidents"][svc["incident_id"]] + assert inc["trigger_type"] in CONTAINER_RESTART_TRIGGERS + + +def test_containers_not_running_after_healthy_transitions_to_unhealthy(observer): + """A container that was healthy and then dies must flip to unhealthy — the + exact real-world sequence that used to stay stuck at 'healthy'.""" + observer.process_event(_container_event("service_healthy", severity="info")) + assert observer.world_state["services"]["piha/paperless"]["status"] == "healthy" + + observer.process_event(_container_event("containers_not_running")) + svc = observer.world_state["services"]["piha/paperless"] + assert svc["status"] == "unhealthy" + assert svc["incident_id"] is not None + + +# --------------------------------------------------------------------------- +# 2. End-to-end: observer output → supervisor generates container_restart +# --------------------------------------------------------------------------- + +def test_supervisor_generates_container_restart_from_observer_output( + observer, tmp_path, monkeypatch +): + """Integration: run the real observer to produce world state for a dead + container, then run the real supervisor.reconcile() against it and assert a + container_restart action lands in pending/. Proves the whole loop works with + NO change to the supervisor.""" + # 1. Observer ingests the dead-container event and writes world state to disk. + observer.process_event(_container_event("containers_not_running")) + observer._save_world() + + # 2. Point the supervisor at a fresh tmp tree; copy observer world output in. + actions = tmp_path / "sup_actions" + events = tmp_path / "sup_events" + world = tmp_path / "sup_world" + repo = tmp_path / "sup_repo" + for d in (actions, events, world, repo / "hosts" / "piha"): + d.mkdir(parents=True, exist_ok=True) + + import observer.observer as obs_mod + for name in ("services.json", "incidents.json", "nodes.json"): + src = obs_mod.WORLD_DIR / name + if src.exists(): + (world / name).write_text(src.read_text()) + + # Declare paperless as a desired service on piha so the drift loop considers it. + (repo / "hosts" / "piha" / "services.yaml").write_text( + "host: piha\nservices:\n paperless:\n role: kb\n exposure: lan\n" + ) + + monkeypatch.setattr(supervisor_module, "ACTIONS_DIR", actions) + monkeypatch.setattr(supervisor_module, "EVENTS_DIR", events) + monkeypatch.setattr(supervisor_module, "WORLD_DIR", world) + monkeypatch.setattr(supervisor_module, "REPO_ROOT", repo) + + sup = Supervisor() + sup.reconcile() + + action_id = "container-restart-piha-paperless" + action_path = actions / "pending" / f"{action_id}.json" + assert action_path.exists(), "supervisor did not generate a container_restart" + action = json.loads(action_path.read_text()) + assert action["type"] == "container_restart" + assert action["node"] == "piha" + assert action["service"] == "paperless" + assert action["payload"]["reason"] == "containers_not_running" + + +# --------------------------------------------------------------------------- +# 3. Recovery: service_healthy after containers_not_running auto-resolves +# --------------------------------------------------------------------------- + +def test_service_healthy_resolves_containers_not_running_incident(observer): + observer.process_event(_container_event("containers_not_running")) + svc = observer.world_state["services"]["piha/paperless"] + inc_id = svc["incident_id"] + assert observer.world_state["incidents"][inc_id]["status"] == "active" + + observer.process_event(_container_event("service_healthy", severity="info")) + svc = observer.world_state["services"]["piha/paperless"] + assert svc["status"] == "healthy" + assert svc["incident_id"] is None + assert observer.world_state["incidents"][inc_id]["status"] == "resolved" + + +# --------------------------------------------------------------------------- +# 4. Observational events — no incident, no false remediation, but a trace +# --------------------------------------------------------------------------- + +@pytest.mark.parametrize("etype", ["container_restarting", "container_state_unexpected"]) +def test_observational_events_do_not_create_incident(observer, etype): + observer.process_event(_container_event(etype, severity="low")) + svc = observer.world_state["services"]["piha/paperless"] + # No incident, and status is NOT flipped to unhealthy (would trigger a redeploy). + assert svc.get("incident_id") is None + assert svc["status"] != "unhealthy" + assert not observer.world_state["incidents"] + + +@pytest.mark.parametrize("etype", ["container_restarting", "container_state_unexpected"]) +def test_observational_events_leave_a_trace(observer, etype): + """They must be visible in world state, not vanish into the void.""" + observer.process_event(_container_event(etype, severity="medium")) + svc = observer.world_state["services"]["piha/paperless"] + obs_note = svc.get("last_observation") + assert obs_note is not None + assert obs_note["type"] == etype + + +def test_container_restarting_does_not_clear_existing_unhealthy(observer): + """A container that is already unhealthy (dead) and then reports a transient + 'restarting' must NOT be downgraded to healthy — the incident stands until a + real service_healthy arrives.""" + observer.process_event(_container_event("containers_not_running")) + svc = observer.world_state["services"]["piha/paperless"] + assert svc["status"] == "unhealthy" + inc_id = svc["incident_id"] + + observer.process_event(_container_event("container_restarting", severity="low")) + svc = observer.world_state["services"]["piha/paperless"] + assert svc["status"] == "unhealthy" + assert svc["incident_id"] == inc_id + assert observer.world_state["incidents"][inc_id]["status"] == "active" + + +# --------------------------------------------------------------------------- +# 5. Idempotency — reprocessing must not multiply incidents +# --------------------------------------------------------------------------- + +def test_repeated_containers_not_running_does_not_multiply_incidents(observer): + ev = _container_event("containers_not_running", ts=1784000000) + observer.process_event(ev) + observer.process_event(ev) + observer.process_event(ev) + + active = [i for i in observer.world_state["incidents"].values() + if i["status"] == "active"] + assert len(active) == 1 + # Correlation bumps occurrence_count rather than opening new incidents. + assert active[0]["occurrence_count"] == 3