diff --git a/docs/plugins/host-agent-design.md b/docs/plugins/host-agent-design.md index 7bb324e..6693995 100644 --- a/docs/plugins/host-agent-design.md +++ b/docs/plugins/host-agent-design.md @@ -266,7 +266,7 @@ The figures are recorded at most once every 30 s, whichever manager sent them, s A maintenance job on a swarm node opens a second window over `docker` / `swarm` beside its host's (`maintenance._swarm_window`). An alert the drain or reboot causes then waits for the job, and one still firing when the window ends is sent then. -### Drive health (1.20.0, #5789) +### Drive health (1.20.0, #5789; 1.20.1, #5806) Reading SMART needs root, so the root helper does it, as the one action on its fixed list that only reads: `drives`. Hourly, and at start, the agent leaves a `drives` request. The helper runs `smartctl --scan-open` to find the drives itself (the request names none), reads each with `smartctl --json -a -d `, and writes a summary to `/var/lib/steward-maint/drives.json` (0644). It writes no job result, because a drive read is not a maintenance job. The agent's newest copy rides every sample, as updates do. @@ -277,7 +277,7 @@ smartctl ≥ 7.0 answers in JSON, the same on every distribution and for SATA, S - reallocated / pending / offline-uncorrectable sectors for ATA, and life used where the drive reports it; - grown defects for SCSI. -A drive with no SMART (a virtual disk, a bridge that hides it) reads `available: false`, and one smartctl couldn't open carries `error`. +A drive with no SMART (a virtual disk, a bridge that hides it) reads `available: false`, and one smartctl couldn't open carries `error`. Disks behind a RAID controller share one path (`/dev/bus/0`) and differ by the slot in their type (`megaraid,3`), so the slot is part of the name: `bus0-megaraid3` (1.20.1, #5806). Steward also gives a repeated name an ordinal (`0#2`), so two drives never share a series. Steward judges the figures in `drives.py`, so the judgment can change without an agent update: diff --git a/plugins/host_agent/agent.py b/plugins/host_agent/agent.py index 559ec4e..0623bb0 100644 --- a/plugins/host_agent/agent.py +++ b/plugins/host_agent/agent.py @@ -22,7 +22,7 @@ from collections import deque from concurrent.futures import ThreadPoolExecutor, wait as wait_futures from datetime import datetime, timezone -AGENT_VERSION = "1.20.0" +AGENT_VERSION = "1.20.1" # Default path to the local Docker Engine socket. Overridable via the # `docker_socket` config key; collection is silently skipped if it's absent or diff --git a/plugins/host_agent/drives.py b/plugins/host_agent/drives.py index 521fbc4..8bfb5d0 100644 --- a/plugins/host_agent/drives.py +++ b/plugins/host_agent/drives.py @@ -69,9 +69,20 @@ def judge(d: dict) -> dict: def drives_of(report) -> list[dict]: - """The report's drives, judged, what needs attention first.""" + """The report's drives, judged, what needs attention first. A name seen + twice gets an ordinal, so two drives never share one series: agent + 1.20.0 named every disk behind a RAID controller "0" (#5806).""" drives = report.get("drives") if isinstance(report, dict) else None - judged = [judge(d) for d in drives or [] if isinstance(d, dict) and d.get("device")] + judged, seen = [], {} + for d in drives or []: + if not isinstance(d, dict) or not d.get("device"): + continue + d = judge(d) + name = str(d["device"]) + seen[name] = seen.get(name, 0) + 1 + if seen[name] > 1: + d["device"] = f"{name}#{seen[name]}" + judged.append(d) return sorted(judged, key=lambda d: (_ORDER[d["state"]], str(d["device"]))) diff --git a/plugins/host_agent/files/maint.py b/plugins/host_agent/files/maint.py index 07a1e11..8a390f2 100644 --- a/plugins/host_agent/files/maint.py +++ b/plugins/host_agent/files/maint.py @@ -199,9 +199,21 @@ def _int(v): return v if isinstance(v, int) and not isinstance(v, bool) else None -def drive_summary(name: str, status, data) -> dict: +def drive_name(name: str, kind) -> str: + """A drive's name, unique on its host. Disks behind a RAID controller + share one path (/dev/bus/0) and differ only by the slot in their type + (megaraid,3; cciss,0), so the slot is part of the name (#5806).""" + base = os.path.basename(name) + if name.startswith("/dev/bus/"): + base = "bus" + base + if isinstance(kind, str) and "," in kind: + base += "-" + re.sub(r"[^A-Za-z0-9]", "", kind.split("+")[-1]) + return base + + +def drive_summary(name: str, status, data, kind=None) -> dict: """The few figures Steward reads from one drive's `smartctl -a` JSON.""" - out = {"device": os.path.basename(name)} + out = {"device": drive_name(name, kind)} if not isinstance(data, dict) or status is None or status & _SMARTCTL_UNREAD: messages = ((data.get("smartctl") or {}).get("messages") or []) if isinstance(data, dict) else [] text = "; ".join(str(m.get("string", "")) for m in messages if isinstance(m, dict)) @@ -267,7 +279,7 @@ def do_drives() -> dict: continue args = ["-a", name] if not isinstance(kind, str) else ["-a", "-d", kind, name] status, data = smartctl_json(args) - drives.append(drive_summary(name, status, data)) + drives.append(drive_summary(name, status, data, kind)) return {**base, "drives": drives} diff --git a/steward/ansible/executor.py b/steward/ansible/executor.py index 4fbdbff..23034db 100644 --- a/steward/ansible/executor.py +++ b/steward/ansible/executor.py @@ -182,14 +182,17 @@ def deregister_listener(run_id: str, q: asyncio.Queue) -> None: listeners.remove(q) +def _drop_run_state(run_id: str) -> None: + _run_lines.pop(run_id, None) + _run_done.pop(run_id, None) + + def _schedule_cleanup(run_id: str) -> None: - """Drop a finished run's in-memory state after a grace window.""" - async def _cleanup() -> None: - await asyncio.sleep(_MEM_TTL_SECS) - _run_lines.pop(run_id, None) - _run_done.pop(run_id, None) + """Drop a finished run's in-memory state after a grace window. A timer, + not a task (#5749): a run cut short by shutdown schedules this as the + loop closes, and a sleeping task would be destroyed while pending.""" try: - asyncio.create_task(_cleanup()) + asyncio.get_running_loop().call_later(_MEM_TTL_SECS, _drop_run_state, run_id) except RuntimeError: pass # no running loop (e.g. tests) — fine to skip diff --git a/steward/ansible/runner.py b/steward/ansible/runner.py index 235b053..3476d61 100644 --- a/steward/ansible/runner.py +++ b/steward/ansible/runner.py @@ -109,9 +109,18 @@ async def trigger_run( connection=connection, secret_vars=secret_vars, redact=redact, ) ) - task.add_done_callback( - lambda t: t.exception() and app.logger.error( - "Ansible run %s raised: %s", run_id, t.exception() - ) - ) + task.add_done_callback(run_ended(app, run_id)) return run, source, None + + +def run_ended(app, run_id: str): + """A run task's done-callback: an error is logged as one. A run cut short + by Steward stopping is not an error (#5749): `exception()` would raise + CancelledError on it, and the next start marks the run interrupted.""" + def callback(t: asyncio.Task) -> None: + if t.cancelled(): + app.logger.info("Ansible run %s interrupted: Steward is stopping; " + "it is marked interrupted at the next start", run_id) + elif t.exception() is not None: + app.logger.error("Ansible run %s raised: %s", run_id, t.exception()) + return callback diff --git a/tests/plugins/host_agent/test_drive_health.py b/tests/plugins/host_agent/test_drive_health.py index 7c31d22..3741178 100644 --- a/tests/plugins/host_agent/test_drive_health.py +++ b/tests/plugins/host_agent/test_drive_health.py @@ -80,6 +80,38 @@ def test_a_drive_with_no_smart_and_one_that_can_t_be_opened_say_so(maint): assert maint.drive_summary("/dev/sdd", None, None)["error"] == "smartctl couldn't read it" +@pytest.mark.parametrize("name, kind, device", [ + ("/dev/sda", "sat", "sda"), + ("/dev/nvme0", "nvme", "nvme0"), + ("/dev/bus/0", "megaraid,3", "bus0-megaraid3"), # VDNT-Docker02's PERC (#5806) + ("/dev/bus/0", "sat+megaraid,12", "bus0-megaraid12"), + ("/dev/sg1", "cciss,0", "sg1-cciss0"), + ("/dev/sdc", None, "sdc"), +]) +def test_each_drive_has_a_name_of_its_own(maint, name, kind, device): + assert maint.drive_name(name, kind) == device + + +def test_disks_behind_one_controller_stay_apart(maint, monkeypatch): + def fake(args): + if args == ["--scan-open"]: + return 0, {"devices": [{"name": "/dev/bus/0", "type": "megaraid,0"}, + {"name": "/dev/bus/0", "type": "megaraid,1"}]} + return 0, ATA + monkeypatch.setattr(maint, "smartctl_version", lambda: (7, 4)) + monkeypatch.setattr(maint, "smartctl_json", fake) + assert [d["device"] for d in maint.do_drives()["drives"]] == ["bus0-megaraid0", "bus0-megaraid1"] + + +def test_a_name_seen_twice_is_still_two_drives(): + """What agent 1.20.0 sent from a PERC: every disk named "0".""" + report = {"drives": [{"device": "0", "passed": True}, {"device": "0", "passed": False}, + {"device": "0", "passed": True}]} + assert sorted(d["device"] for d in dh.drives_of(report)) == ["0", "0#2", "0#3"] + rows = {(m, r) for m, r, _ in dh.metric_rows(report, "box") if m == "drive_failing"} + assert len(rows) == 3 + + def test_a_failing_disk_s_figures_still_count(maint): """Exit bit 3 says the disk is failing; its JSON is the point, not an error.""" failing = {**ATA, "smart_status": {"passed": False}} diff --git a/tests/test_ansible_run_end.py b/tests/test_ansible_run_end.py new file mode 100644 index 0000000..be45b71 --- /dev/null +++ b/tests/test_ansible_run_end.py @@ -0,0 +1,52 @@ +"""#5749: a redeploy during an Ansible run logged two CancelledError tracebacks +from the run's done-callback and two "Task was destroyed but it is pending!" +lines for its in-memory cleanup. Steward stopping mid-run is not an error: the +next start marks the run interrupted.""" +import asyncio +import logging + +from steward.ansible import executor +from steward.ansible.runner import run_ended + + +class _App: + logger = logging.getLogger("test.ansible.run_end") + + +async def test_a_run_cut_short_by_shutdown_logs_no_error(caplog): + async def run(): + await asyncio.sleep(60) + + task = asyncio.create_task(run()) + task.add_done_callback(run_ended(_App, "r1")) + await asyncio.sleep(0) + task.cancel() + with caplog.at_level(logging.INFO, logger="test.ansible.run_end"): + await asyncio.gather(task, return_exceptions=True) + await asyncio.sleep(0) # done-callbacks run on the next turn + assert [r.levelname for r in caplog.records] == ["INFO"] + assert "Ansible run r1 interrupted" in caplog.records[0].getMessage() + + +async def test_a_run_that_raised_is_still_an_error(caplog): + async def run(): + raise RuntimeError("ansible-playbook vanished") + + task = asyncio.create_task(run()) + task.add_done_callback(run_ended(_App, "r2")) + with caplog.at_level(logging.INFO, logger="test.ansible.run_end"): + await asyncio.gather(task, return_exceptions=True) + await asyncio.sleep(0) + assert [(r.levelname, r.getMessage()) for r in caplog.records] == [ + ("ERROR", "Ansible run r2 raised: ansible-playbook vanished")] + + +async def test_a_finished_run_s_state_is_dropped_by_a_timer_not_a_task(monkeypatch): + """A task left sleeping is what shutdown found pending; a timer isn't one.""" + monkeypatch.setattr(executor, "_MEM_TTL_SECS", 0.01) + executor._run_lines["r3"] = ["ok"] + before = asyncio.all_tasks() + executor._schedule_cleanup("r3") + assert asyncio.all_tasks() == before + await asyncio.sleep(0.05) + assert "r3" not in executor._run_lines