diff --git a/docs/core/ansible.md b/docs/core/ansible.md index f1a4fd8..bdde28e 100644 --- a/docs/core/ansible.md +++ b/docs/core/ansible.md @@ -101,12 +101,20 @@ If a client connects after the run has already completed, the endpoint immediate | Status | Description | |---|---| +| `queued` | Waiting for a host another run holds, or for a run slot | | `running` | Execution in progress | | `success` | Playbook exited 0 | | `failed` | Playbook exited non-zero | | `interrupted` | App restarted while run was in progress | +| `cancelled` | Stopped by an operator, or by Steward shutting down mid-run | -On startup, the app marks any runs still in `running` state as `interrupted` (see `_mark_interrupted_runs()` in `app.py`). +On startup, the app marks any runs still `running` or `queued` as `interrupted` (see `_mark_interrupted_runs()` in `app.py`). + +### Overlapping runs + +- **One run per host.** Before it takes a run slot, a run claims every host it touches. Hosts come from the inventory Steward writes, keyed by address. A run on a source's own inventory file claims that file, because Steward doesn't read its hosts. While another run holds one of its hosts, the run shows `queued` and says which run it waits for. After an hour of waiting it fails and says so. +- **One run at a time per schedule.** A schedule that falls due while its previous run is still `queued` or `running` skips that firing. The Schedules page shows "Skipped: the previous run was still going" beside that run. The next firing comes an interval later. +- **One action run at a time per alert rule.** When a rule fires again while the action run from an earlier firing is still going, no new run starts. The firing's event says it was skipped and names the run. Output stored in the DB is capped at 1 MB. If truncated, `[output truncated]` is appended to the DB column, but the live SSE stream continues unaffected. diff --git a/steward/ansible/executor.py b/steward/ansible/executor.py index 2786b0c..3c6415e 100644 --- a/steward/ansible/executor.py +++ b/steward/ansible/executor.py @@ -38,6 +38,111 @@ def artifact_path(run_id: str) -> str: return os.path.join(ARTIFACT_DIR, f"{run_id}.log") +# ── One run per host (#5158) ────────────────────────────────────────────────── +# Two runs against the same host would race each other's changes, so a run +# claims every host it touches before it takes a run slot, and waits (queued) +# while another run holds one. The claim is all-or-nothing, so two runs over +# overlapping hosts can't each hold half and wait on the other. + +HOST_WAIT_SECONDS = 3600.0 # a run waiting longer than this for a host fails + + +class HostBusy(Exception): + """A run gave up waiting for a host another run held.""" + + +def run_hosts(inventory_content: str | None, inventory_path: str | None) -> set[str]: + """What a run touches, as claim keys: the address of each host in the YAML + inventory Steward wrote for it, or, for a source's own inventory file + (whose hosts Steward doesn't read), that file.""" + if not inventory_content: + return {f"inventory:{inventory_path}"} if inventory_path else set() + import yaml + try: + data = yaml.safe_load(inventory_content) + except yaml.YAMLError: + return set() # ansible-playbook will refuse it and say why + # A host listed again under a group carries no vars there, so its address + # is whichever listing gives one. + addresses: dict[str, str | None] = {} + + def walk(group) -> None: + if not isinstance(group, dict): + return + members = group.get("hosts") + if isinstance(members, dict): + for name, hostvars in members.items(): + addr = hostvars.get("ansible_host") if isinstance(hostvars, dict) else None + addresses[str(name)] = addr or addresses.get(str(name)) + children = group.get("children") + if isinstance(children, dict): + for child in children.values(): + walk(child) + + if isinstance(data, dict): + for group in data.values(): + walk(group) + return {str(addr or name).strip().lower() for name, addr in addresses.items()} + + +def _host_gate(app: "Quart") -> dict: + """The claims and the wake-up event, bound to the running loop like the + run-slot semaphore (one loop in prod, one per test).""" + loop = asyncio.get_running_loop() + gate = getattr(app, "_ansible_host_gate", None) + if gate is None or gate["loop"] is not loop: + gate = {"loop": loop, "held": {}, "changed": asyncio.Event()} + app._ansible_host_gate = gate + return gate + + +async def _claim_hosts(app: "Quart", run_id: str, hosts: set[str]) -> bool: + """Hold every host in hosts for run_id, waiting while another run holds + any of them. Returns whether it had to wait. Raises _Aborted when the run + is cancelled while waiting, HostBusy after HOST_WAIT_SECONDS.""" + from steward.models.ansible import AnsibleRunStatus + + gate = _host_gate(app) + held: dict[str, str] = gate["held"] + give_up = time.monotonic() + HOST_WAIT_SECONDS + waited = False + while True: + busy = sorted({held[h] for h in hosts if h in held and held[h] != run_id}) + if not busy: + for h in hosts: + held[h] = run_id + return waited + if run_id in _cancelled: + raise _Aborted + remaining = give_up - time.monotonic() + if remaining <= 0: + names = ", ".join(sorted(h for h in hosts if h in held)) + raise HostBusy(f"gave up after {HOST_WAIT_SECONDS:.0f} s waiting for " + f"{names}, held by run {', '.join(busy)}") + if not waited: + waited = True + await _set_status(app, run_id, AnsibleRunStatus.queued) + _broadcast(run_id, f"[queued — waiting for run {', '.join(busy)} " + "to finish on the same host]") + changed = gate["changed"] + try: + # Woken by a release; the short timeout also notices a cancel. + await asyncio.wait_for(changed.wait(), timeout=min(remaining, 2.0)) + except asyncio.TimeoutError: + pass + + +def _release_hosts(app: "Quart", run_id: str, hosts: set[str]) -> None: + gate = getattr(app, "_ansible_host_gate", None) + if gate is None: + return + for h in hosts: + if gate["held"].get(h) == run_id: + del gate["held"][h] + changed, gate["changed"] = gate["changed"], asyncio.Event() + changed.set() + + @dataclass class _Done: status: str @@ -392,8 +497,9 @@ async def start_run( ) -> None: """Execute ansible-playbook as a subprocess and update the DB run row. - Respects a global concurrency cap (app._ansible_semaphore): if no slot is - free the run shows as 'queued' until one frees. Supports cancellation, a + Waits (as 'queued') while another run holds one of its hosts (#5158), then + for a slot under the global concurrency cap (app._ansible_semaphore). + Supports cancellation, a persistent full-log artifact, and a parsed per-host result summary. connection is an optional per-run SSH override (user/password) for @@ -442,17 +548,23 @@ async def start_run( sem = asyncio.Semaphore(max(1, cap)) app._ansible_semaphore = sem app._ansible_sem_loop = loop - queued = sem.locked() - if queued: - await _set_status(app, run_id, AnsibleRunStatus.queued) - _broadcast(run_id, "[queued — waiting for an Ansible run slot]") - await sem.acquire() + hosts = run_hosts(inventory_content, inventory_path) final_status = "failed" logf = None proc = None interrupted = False + slot_held = False try: + # The hosts first, so a run waiting on a host doesn't sit on a run slot. + queued = await _claim_hosts(app, run_id, hosts) + if sem.locked(): + if not queued: + await _set_status(app, run_id, AnsibleRunStatus.queued) + queued = True + _broadcast(run_id, "[queued — waiting for an Ansible run slot]") + await sem.acquire() + slot_held = True if run_id in _cancelled: # cancelled while queued — abort before launching raise _Aborted if queued: @@ -592,7 +704,9 @@ async def start_run( logf.close() _run_procs.pop(run_id, None) _cancelled.discard(run_id) - sem.release() + if slot_held: + sem.release() + _release_hosts(app, run_id, hosts) results = {"hosts": parse_recap(_run_lines.get(run_id, [])), "failures": failures} try: diff --git a/steward/ansible/runner.py b/steward/ansible/runner.py index 0d1eacc..8f7ca6a 100644 --- a/steward/ansible/runner.py +++ b/steward/ansible/runner.py @@ -16,6 +16,19 @@ from steward.ansible.inventory_gen import ( ) from steward.models.ansible import AnsibleRun, AnsibleRunStatus +# A run in one of these hasn't finished: a schedule or an alert action whose +# previous run is still in one skips its next firing (#5158). +IN_FLIGHT = (AnsibleRunStatus.queued, AnsibleRunStatus.running) + + +async def run_in_flight(db, run_id: str | None) -> bool: + """Whether run_id names a run that hasn't finished. Runs left running by a + restart are marked interrupted at startup, so this can't stick.""" + if not run_id: + return False + run = await db.get(AnsibleRun, run_id) + return run is not None and run.status in IN_FLIGHT + async def trigger_run( app, diff --git a/steward/ansible/scheduler.py b/steward/ansible/scheduler.py index f4efaff..17a67c1 100644 --- a/steward/ansible/scheduler.py +++ b/steward/ansible/scheduler.py @@ -7,7 +7,7 @@ from datetime import datetime, timezone from sqlalchemy import select -from steward.ansible.runner import trigger_run +from steward.ansible.runner import run_in_flight, trigger_run from steward.models.ansible_schedule import AnsibleSchedule logger = logging.getLogger(__name__) @@ -27,7 +27,8 @@ def is_due(schedule: AnsibleSchedule, now: datetime) -> bool: async def run_due_schedules(app) -> int: - """Trigger every due schedule once. Returns the number launched.""" + """Trigger every due schedule once. Returns the number launched. A due + schedule whose previous run hasn't finished skips this firing.""" now = datetime.now(timezone.utc) async with app.db_sessionmaker() as db: schedules = (await db.execute( @@ -38,6 +39,21 @@ async def run_due_schedules(app) -> int: for s in schedules: if not is_due(s, now): continue + async with app.db_sessionmaker() as db: + busy = await run_in_flight(db, s.last_run_id) + if busy: + # One run at a time per schedule (#5158). This firing is skipped, + # not deferred, so the cadence holds; last_run_id still names the + # run that's going. + async with app.db_sessionmaker() as db: + async with db.begin(): + fresh = await db.get(AnsibleSchedule, s.id) + if fresh: + fresh.last_run_at = now + fresh.last_error = "Skipped: the previous run was still going" + logger.info("Schedule %s (%s) skipped: run %s still going", + s.id, s.name, s.last_run_id) + continue run = None err: str | None = None try: diff --git a/steward/core/alerts.py b/steward/core/alerts.py index 68d422b..c40f753 100644 --- a/steward/core/alerts.py +++ b/steward/core/alerts.py @@ -497,8 +497,47 @@ async def _run_ansible_action( inventory must be ones the source lists (ansible/guards.py). The firing event links to the run, or says why the action did not start. """ - from steward.ansible.runner import trigger_run + from steward.ansible.runner import IN_FLIGHT, trigger_run + from steward.models.ansible import AnsibleRun + if rule.id in _actions_starting: + await _note_event(app, event_id, "Ansible action skipped: an earlier " + "firing's action was still starting") + return + async with app.db_sessionmaker() as db: + busy = (await db.execute( + select(AnsibleRun.id) + .join(AlertEvent, AlertEvent.ansible_run_id == AnsibleRun.id) + .where(AlertEvent.rule_id == rule.id, AnsibleRun.status.in_(IN_FLIGHT)) + .limit(1))).scalar_one_or_none() + if busy: + # One action run at a time per rule (#5158): a re-firing while the + # last one is still going would run the same fix over it. + await _note_event(app, event_id, f"Ansible action skipped: run {busy} " + "from an earlier firing was still going") + return + _actions_starting.add(rule.id) + try: + await _start_ansible_action(app, rule, value, event_id, trigger_run) + finally: + _actions_starting.discard(rule.id) + + +# Rules whose action is between the in-flight check and its run being linked +# to the event, so two firings that close together can't both start one. +_actions_starting: set[str] = set() + + +async def _note_event(app: "Quart", event_id: str, why: str) -> None: + async with app.db_sessionmaker() as db: + async with db.begin(): + ev = (await db.execute( + select(AlertEvent).where(AlertEvent.id == event_id))).scalar_one_or_none() + if ev is not None: + ev.note = f"{ev.note}; {why}" if ev.note else why + + +async def _start_ansible_action(app, rule, value, event_id, trigger_run) -> None: action = rule.ansible_action or {} playbook = action.get("playbook") or "" inventory = action.get("inventory") or "" @@ -529,17 +568,15 @@ async def _run_ansible_action( if err: logger.error("Ansible action for rule %r not started: %s", rule.name, err) + if run is None: + await _note_event(app, event_id, f"Ansible action not started: {err}") + return async with app.db_sessionmaker() as db: async with db.begin(): ev = (await db.execute( select(AlertEvent).where(AlertEvent.id == event_id))).scalar_one_or_none() - if ev is None: - return - if run is not None: + if ev is not None: ev.ansible_run_id = run.id - else: - why = f"Ansible action not started: {err}" - ev.note = f"{ev.note}; {why}" if ev.note else why async def check_silence(app: "Quart") -> None: diff --git a/steward/mcp/tools/automation.py b/steward/mcp/tools/automation.py index 1e84b94..6883af2 100644 --- a/steward/mcp/tools/automation.py +++ b/steward/mcp/tools/automation.py @@ -527,8 +527,10 @@ async def list_schedules() -> dict: `check: true` schedules are dry runs that never change anything. `last_error` is set when a firing could not start at all (a source or - playbook that went away); a run that started and failed shows in - `last_run_status`, and get_ansible_run(last_run_id) has its log. + playbook that went away), or was skipped because the schedule's previous + run was still going (then `last_run_id` still names that run). A run that + started and failed shows in `last_run_status`, and + get_ansible_run(last_run_id) has its log. """ async with db_session() as db: scheds = await _schedules(db) diff --git a/steward/templates/ansible/schedules.html b/steward/templates/ansible/schedules.html index e4c8ea8..32c756a 100644 --- a/steward/templates/ansible/schedules.html +++ b/steward/templates/ansible/schedules.html @@ -38,13 +38,18 @@