From 4746ebe0fbba937682447569c76b809fd05320d0 Mon Sep 17 00:00:00 2001 From: oskar Date: Tue, 14 Jul 2026 19:41:05 +0200 Subject: [PATCH] =?UTF-8?q?fix(node-agent):=20detect=20restarting/crash-lo?= =?UTF-8?q?oping=20containers=20=E2=80=94=20state=20fell=20through=20class?= =?UTF-8?q?ification,=20crash-loops=20were=20invisible=20to=20monitoring?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit check_containers() classified only exited/dead, running+unhealthy and running. Docker's "restarting", "paused", "removing" and any future state matched no branch, so a crash-looping container (restart policy + continuous crash) emitted ZERO events and world-state kept showing it "healthy" (observed live: lustro/pi-watchtower-1). Every Docker state is now handled: - restarting + RestartCount >= CRASH_LOOP_RESTART_THRESHOLD (default 3): reuse containers_not_running (high, crash_loop=true) — parity with exited/dead, rides the existing supervisor-wired remediation path. - restarting below threshold: new observational container_restarting (low), visible but non-actionable so benign post-deploy restarts don't alarm. - paused / unknown-or-future state: new diagnostic container_state_unexpected (medium) — no more silent fall-through; new Docker states become visible. - removing: conscious documented skip (ephemeral teardown). - created: unchanged skip (compose tracking artifact). RestartCount (top-level inspect field) distinguishes a crash-loop from a one-off restart. Adds services/node-agent/tests/test_check_containers.py pinning the full state table. Co-Authored-By: Claude Opus 4.8 (1M context) --- services/node-agent/src/node_agent.py | 113 ++++++++++- .../node-agent/tests/test_check_containers.py | 187 ++++++++++++++++++ 2 files changed, 295 insertions(+), 5 deletions(-) create mode 100644 services/node-agent/tests/test_check_containers.py diff --git a/services/node-agent/src/node_agent.py b/services/node-agent/src/node_agent.py index 5ea48cf..337290f 100644 --- a/services/node-agent/src/node_agent.py +++ b/services/node-agent/src/node_agent.py @@ -87,6 +87,19 @@ DISK_CRIT_PCT = 85 MEM_WARN_PCT = 85 MEM_CRIT_PCT = 95 +# A container Docker reports as "restarting" that has accumulated at least this +# many restarts is treated as a genuine crash-loop (a real fault) rather than a +# benign post-deploy / one-off restart. +# +# Rationale for the threshold: Docker's restart backoff keeps a *genuinely* +# crash-looping container cycling through the "restarting" state, so a 60 s +# health cycle is very likely to catch it there. A container that merely +# restarted once or twice (fresh deploy, a single transient failure that then +# recovers) is almost always back in "running" by the next cycle. Requiring +# RestartCount >= 3 clears normal deploy churn while still catching a container +# that is actually stuck flapping. Configurable via env for tuning per fleet. +CRASH_LOOP_RESTART_THRESHOLD = int(os.getenv("CRASH_LOOP_RESTART_THRESHOLD", "3")) + # SD-card nodes: enforce 24-hour gap between Docker cleanup runs CLEANUP_INTERVAL_SECS = 86_400 LAST_CLEANUP_FILE = STATE_DIR / "last-docker-cleanup" @@ -320,10 +333,17 @@ class NodeAgent: health_status = (c.attrs.get("State", {}) .get("Health", {}) .get("Status", "")) + # RestartCount is a top-level inspect field (sibling of State), + # not nested under State. It monotonically counts how many times + # Docker has auto-restarted this container — the signal that + # distinguishes a crash-loop from a one-off restart. + restart_count = c.attrs.get("RestartCount", 0) # Skip containers in "created" state — these are Docker Compose # internal tracking artifacts (never started, often hash-prefixed) # that appear when a container is rebuilt outside of compose. + # This is a conscious skip, not a blind spot: a "created" container + # is not a running service and has no fault to report. if status == "created": continue @@ -332,23 +352,73 @@ class NodeAgent: if not is_managed: continue + base_payload = { + "container": name, + "status": status, + "restart_policy": restart_policy, + "restart_count": restart_count, + } + # Exited container that carries an auto-restart policy if status in ("exited", "dead"): logger.warning(f"Container exited: {name} (restart={restart_policy})") self.emit_event( "containers_not_running", "high", name, f"Container '{name}' has exited (restart={restart_policy})", - {"container": name, "status": status, - "restart_policy": restart_policy}, + base_payload, ) + # Container stuck in Docker's "restarting" state. This is the gap + # that previously made crash-loops invisible: "restarting" matched + # neither the exited/dead branch nor the running branch, so a + # container flapping under its restart policy emitted ZERO events + # and the observer kept showing its last (stale) "healthy" state. + # + # We split on RestartCount so a benign post-deploy restart does not + # alarm, while a real crash-loop escalates to the same actionable + # signal as an exited container. + elif status == "restarting": + if restart_count >= CRASH_LOOP_RESTART_THRESHOLD: + # Genuine crash-loop: reuse containers_not_running so it + # rides the existing, supervisor-wired remediation path + # (parity with exited/dead — the container is, by + # definition, not staying running). + logger.warning( + f"Container crash-looping: {name} " + f"(restarting, {restart_count} restarts, restart={restart_policy})" + ) + self.emit_event( + "containers_not_running", "high", name, + f"Container '{name}' is crash-looping " + f"(restarting, {restart_count} restarts, restart={restart_policy})", + {**base_payload, "crash_loop": True}, + ) + else: + # Fresh / transient restart. Make it VISIBLE (so the state + # is no longer a silent hole) but do NOT trigger remediation + # — a restart action here would be premature and noisy. If + # it keeps flapping, RestartCount crosses the threshold on a + # later cycle and escalates to containers_not_running above. + # container_restarting is intentionally observational: it is + # not wired to any supervisor trigger. + logger.info( + f"Container restarting: {name} " + f"({restart_count} restarts) — watching for crash-loop" + ) + self.emit_event( + "container_restarting", "low", name, + f"Container '{name}' is restarting " + f"({restart_count} restarts) — watching for crash-loop", + {**base_payload, "crash_loop": False}, + ) + # Running container with a failing health check elif status == "running" and health_status == "unhealthy": logger.warning(f"Container unhealthy: {name}") self.emit_event( "healthcheck_failed", "high", name, f"Container '{name}' is running but its health check is failing", - {"container": name, "health_status": health_status}, + {**base_payload, "health_status": health_status}, ) # Running container that is healthy — confirm to observer so that @@ -359,8 +429,41 @@ class NodeAgent: self.emit_event( "service_healthy", "info", name, f"Container '{name}' is running", - {"container": name, "status": status, - "health_status": health_status or "none"}, + {**base_payload, "health_status": health_status or "none"}, + ) + + # Deliberately paused. docker pause is always an operator/tool + # action (Docker never auto-pauses), so it is not a fault — but a + # managed service should not normally sit paused, so surface it + # observationally rather than swallowing it. Non-actionable. + elif status == "paused": + logger.info(f"Container paused: {name}") + self.emit_event( + "container_state_unexpected", "medium", name, + f"Container '{name}' is paused (restart={restart_policy})", + base_payload, + ) + + # Ephemeral teardown state: the container is being removed and will + # be gone (or replaced) by the next cycle. Emitting a fault here + # would be a guaranteed false positive on every recreate/deploy, so + # this is a conscious, documented skip — not a silent hole. + elif status == "removing": + logger.debug(f"Container removing (transient teardown): {name}") + + # FALLBACK — any Docker state we do not explicitly handle (including + # states a future Docker engine might introduce). Never fall through + # silently: emit a diagnostic so a new/unknown state becomes visible + # instead of quietly recreating the very blind spot this fix closes. + else: + logger.warning( + f"Container in unhandled Docker state: {name} (state='{status}')" + ) + self.emit_event( + "container_state_unexpected", "medium", name, + f"Container '{name}' is in unhandled Docker state '{status}' " + f"(restart={restart_policy})", + base_payload, ) except Exception as exc: diff --git a/services/node-agent/tests/test_check_containers.py b/services/node-agent/tests/test_check_containers.py new file mode 100644 index 0000000..e6274dc --- /dev/null +++ b/services/node-agent/tests/test_check_containers.py @@ -0,0 +1,187 @@ +"""Tests for NodeAgent.check_containers() Docker-state classification. + +Regression coverage for the crash-loop blind spot: a container Docker reports +as "restarting" previously matched neither the exited/dead branch nor the +running branch, so a crash-looping container emitted ZERO events and stayed +"healthy" forever in world-state. These tests pin down the full state table so +no Docker state can silently fall through classification again. +""" +from __future__ import annotations + +from unittest.mock import MagicMock + +import node_agent + + +# --------------------------------------------------------------------------- +# Fake Docker container helper +# --------------------------------------------------------------------------- + +def make_container( + status, + *, + restart_policy="unless-stopped", + restart_count=0, + health="", + name="svc", + compose_service="svc", +): + """Build a stand-in for a docker-py Container with the attrs check_containers reads.""" + c = MagicMock() + c.status = status + c.name = name + attrs = { + "RestartCount": restart_count, + "HostConfig": {"RestartPolicy": {"Name": restart_policy}}, + "State": {"Health": {"Status": health}} if health else {"Health": {}}, + "Config": {"Labels": {"com.docker.compose.service": compose_service}}, + } + c.attrs = attrs + return c + + +def run_check(agent, containers, monkeypatch): + """Run check_containers() against a fixed container list, capturing emitted events. + + Returns the list of (event_type, severity, service, message, payload) tuples. + """ + emitted = [] + + def fake_emit(event_type, severity, service, message, payload=None): + emitted.append((event_type, severity, service, message, payload or {})) + + monkeypatch.setattr(agent, "emit_event", fake_emit) + + client = MagicMock() + client.containers.list.return_value = containers + agent.docker_client = client + + agent.check_containers() + return emitted + + +# --------------------------------------------------------------------------- +# The regression: "restarting" must not be silent +# --------------------------------------------------------------------------- + +def test_restarting_crash_loop_emits_high_containers_not_running(agent, monkeypatch): + """restarting + high RestartCount → containers_not_running, high, crash_loop=True.""" + c = make_container("restarting", restart_count=7, name="pi-watchtower-1", + compose_service="watchtower") + events = run_check(agent, [c], monkeypatch) + + assert len(events) == 1 + etype, severity, service, _msg, payload = events[0] + assert etype == "containers_not_running" + assert severity == "high" + assert service == "watchtower" + assert payload["crash_loop"] is True + assert payload["restart_count"] == 7 + assert payload["status"] == "restarting" + + +def test_restarting_fresh_emits_low_container_restarting_no_alarm(agent, monkeypatch): + """restarting + low RestartCount → observational container_restarting, low, not actionable.""" + c = make_container("restarting", restart_count=1, name="svc") + events = run_check(agent, [c], monkeypatch) + + assert len(events) == 1 + etype, severity, _service, _msg, payload = events[0] + assert etype == "container_restarting" + assert severity == "low" + assert payload["crash_loop"] is False + assert payload["restart_count"] == 1 + # It must NOT masquerade as an actionable "not running" or "healthy" signal. + assert etype not in ("containers_not_running", "service_healthy", "healthcheck_failed") + + +def test_restarting_at_threshold_is_crash_loop(agent, monkeypatch): + """RestartCount exactly at the threshold counts as a crash-loop (>=).""" + c = make_container("restarting", restart_count=node_agent.CRASH_LOOP_RESTART_THRESHOLD) + events = run_check(agent, [c], monkeypatch) + assert events[0][0] == "containers_not_running" + assert events[0][1] == "high" + + +# --------------------------------------------------------------------------- +# Existing behaviour must be preserved +# --------------------------------------------------------------------------- + +def test_exited_still_emits_containers_not_running(agent, monkeypatch): + c = make_container("exited") + events = run_check(agent, [c], monkeypatch) + assert events[0][0] == "containers_not_running" + assert events[0][1] == "high" + + +def test_dead_still_emits_containers_not_running(agent, monkeypatch): + c = make_container("dead") + events = run_check(agent, [c], monkeypatch) + assert events[0][0] == "containers_not_running" + + +def test_running_healthy_emits_service_healthy(agent, monkeypatch): + c = make_container("running") + events = run_check(agent, [c], monkeypatch) + assert events[0][0] == "service_healthy" + assert events[0][1] == "info" + + +def test_running_unhealthy_emits_healthcheck_failed(agent, monkeypatch): + c = make_container("running", health="unhealthy") + events = run_check(agent, [c], monkeypatch) + assert events[0][0] == "healthcheck_failed" + assert events[0][1] == "high" + + +# --------------------------------------------------------------------------- +# The other previously-silent states +# --------------------------------------------------------------------------- + +def test_paused_emits_state_unexpected(agent, monkeypatch): + c = make_container("paused") + events = run_check(agent, [c], monkeypatch) + assert len(events) == 1 + assert events[0][0] == "container_state_unexpected" + assert events[0][1] == "medium" + + +def test_unknown_state_emits_fallback_diagnostic(agent, monkeypatch): + """A Docker state we do not model must never be silent — fallback diagnostic.""" + c = make_container("some-future-state") + events = run_check(agent, [c], monkeypatch) + assert len(events) == 1 + etype, severity, _service, _msg, payload = events[0] + assert etype == "container_state_unexpected" + assert severity == "medium" + assert payload["status"] == "some-future-state" + + +def test_removing_is_conscious_silent_skip(agent, monkeypatch): + """removing is an ephemeral teardown state — deliberately no event (documented).""" + c = make_container("removing") + events = run_check(agent, [c], monkeypatch) + assert events == [] + + +def test_created_is_skipped(agent, monkeypatch): + c = make_container("created") + events = run_check(agent, [c], monkeypatch) + assert events == [] + + +# --------------------------------------------------------------------------- +# Managed-container filter still applies +# --------------------------------------------------------------------------- + +def test_unmanaged_container_is_ignored(agent, monkeypatch): + """No restart policy → not a long-running managed service → no events, even if restarting.""" + c = make_container("restarting", restart_policy="no", restart_count=9) + events = run_check(agent, [c], monkeypatch) + assert events == [] + + +def test_no_docker_client_is_noop(agent, monkeypatch): + agent.docker_client = None + # Should simply return without raising. + agent.check_containers()