Skip to content

feat(ops): recreate VM containers that are persistently unhealthy - #15

Open
Marinski wants to merge 2 commits into
psyb0t:masterfrom
Marinski:feat/vm-health-watchdog
Open

feat(ops): recreate VM containers that are persistently unhealthy#15
Marinski wants to merge 2 commits into
psyb0t:masterfrom
Marinski:feat/vm-health-watchdog

Conversation

@Marinski

@Marinski Marinski commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

dockurr/windows keeps the container "up" while the Windows guest may have crashed internally — an unexpected shutdown (Event 6008), a wedged terminal, an OOM. restart: unless-stopped never fires, because from Docker's point of view nothing died, so every terminal API in that VM stays dead until a human notices.

This adds a Compose-managed vm-watchdog sidecar that restarts those containers automatically.

What it is

A small python:3.12-alpine service that polls the Docker API over a mounted /var/run/docker.sock and docker restarts VM containers whose health has stayed unhealthy for a sustained streak.

vm-watchdog:
  image: python:3.12-alpine
  restart: unless-stopped
  command: ["python", "-u", "/vm-watchdog.py"]
  volumes:
    - /var/run/docker.sock:/var/run/docker.sock
    - ./scripts/vm-watchdog.py:/vm-watchdog.py:ro
    - vm-watchdog-state:/state

The footprint worth reviewing is the Docker socket mount. That is root-equivalent access to the host daemon, so the script is deliberately small and boring: stdlib only, no third-party Docker client, no shell, no eval, no user-supplied strings reaching a command. It speaks HTTP over the unix socket directly (~300 lines) and the only mutating call it ever makes is POST /containers/<id>/restart.

What it will touch

A container is only ever restarted when all of these hold:

  • health status is unhealthy (never healthy, never starting)
  • FailingStreakWATCHDOG_MIN_FAILING_STREAK (default 10, ≈5 min at the 30s healthcheck interval)
  • the image matches WATCHDOG_IMAGE_FILTER (default dockurr/windows)
  • it belongs to this Compose project
  • it is not the watchdog itself

So nginx, wickworks, the log rotator and the watchdog are structurally out of scope, and a healthy VM is never interrupted — a long backtest keeps the healthcheck green the whole time it runs.

Restarts are rate-limited by exponential backoff (5m → 15m → 1h) and capped at WATCHDOG_MAX_ATTEMPTS (default 3) before the watchdog gives up and leaves the container alone, so it cannot restart-loop. State survives its own restart via the vm-watchdog-state volume. WATCHDOG_DRY_RUN logs decisions without acting.

docker restart is used rather than recreate, deliberately: recreating a VM orphans a network_mode: service:<vm> sidecar, which is the failure mode documented in "VM recreate and the wickworks sidecar".

Files

  • scripts/vm-watchdog.py — the sidecar (stdlib-only Docker client + recovery loop)
  • docker-compose.yml.j2 / .example — the service and its state volume
  • tests/test_vm_watchdog.py — 293 lines covering the gates above: image filter, project scoping, self-exclusion, streak threshold, backoff, attempt cap, state persistence
  • Dockerfile.test — copies the script so the tests can import it
  • docs/operations.md — "Auto-recovery" section

Notes

Supersedes the host-cron scripts/watchdog.sh this PR originally proposed. A cron line needed a machine-specific checkout path and a systemd/cron unit outside Compose, so the recovery mechanism lived somewhere the stack could not ship or version. As a Compose service it deploys with everything else and is covered by the test suite.

@psyb0t

psyb0t commented Aug 16, 2026

Copy link
Copy Markdown
Owner

Thanks — the underlying failure mode is real, but I do not want operators installing a host cron job for this.

Could we make this a Compose-managed vm-watchdog sidecar instead? It can poll Docker health through the Docker socket, restrict itself to this Compose project plus the dockurr/windows VM image, and use Docker's existing State.Health.FailingStreak as the source of truth. That keeps recovery in docker compose up -d, with no machine-specific checkout path, cron setup, or systemd unit.

The sidecar should keep a tiny named-volume state record per container for restart policy: last restart, attempt count, and healthy-since. Suggested behaviour: restart after the sustained unhealthy threshold; exponential retry spacing (for example 5m → 15m → 1h); stop after a bounded number of failed recoveries and log loudly; reset attempts only after the VM has stayed healthy for a meaningful period.

There are also two correctness issues in the current script:

  1. watch_once returns 1 after a successful restart. With set -e and main calling it directly, the script exits as an error precisely when it recovered something.
  2. The current cooldown is only a fixed delay. A persistently broken VM will be restarted again every cooldown interval, so it does not actually prevent a restart loop as the docs claim.

Please add behavioural coverage for healthy/starting exclusion, sustained unhealthy restart, image/label scoping, cooldown/backoff, bounded retries, and reset after stable health. Happy to review a follow-up.

@Marinski
Marinski force-pushed the feat/vm-health-watchdog branch from 010327e to 5ef74cf Compare August 18, 2026 14:48
@Marinski

Copy link
Copy Markdown
Contributor Author

Thanks — agreed on the cron, and this branch now delivers exactly the Compose-managed sidecar you described. scripts/watchdog.sh is gone; vm-watchdog is a project-level Compose service (ships in docker compose up -d). Implemented in 5ef74cf:

scripts/vm-watchdog.py (Python 3, stdlib-only — no image deps):

  • Polls Docker health through the mounted unix socket; uses Docker's own State.Health.FailingStreak as the source of truth.
  • Scoped to this Compose project (self-discovered from its own container labels) plus the dockurr/windows image — never touches nginx/wickworks/log-rotator or itself.
  • Named-volume state record per container (/state/<id>.json): last restart, attempt count, healthy-since.
  • Restarts only after the sustained unhealthy threshold; exponential backoff 5m → 15m → 1h between attempts; stops after WATCHDOG_MAX_ATTEMPTS (3) failed recoveries and logs loudly; resets the budget only after the VM has stayed healthy for WATCHDOG_RESET_SECONDS (30m).
  • Uses docker restart (not recreate), preserving the owner container ID and therefore the wickworks sidecar's netns attachment.
  • Both correctness issues fixed by construction: the loop never exits with a "recovered" error (it's a daemon, not a one-shot), and backoff is exponential, not a fixed cooldown — so a persistently broken VM is not restarted every cooldown interval.

Behavioural coverage (tests/test_vm_watchdog.py, 11 tests, run against a fake Docker transport in the offline suite): healthy/starting exclusion, sustained-unhealthy restart, image/label scoping, exponential backoff timing, bounded retries + loud give-up, reset after stable health, dry-run, project self-discovery, and restart-failure state retention for backoff.

Compose wiring: vm-watchdog service in docker-compose.yml.j2 and .example (socket mount, script mount, named vm-watchdog-state volume, python -u for unbuffered logs). docs/operations.md rewritten — cron/systemd instructions removed.

make test-unit passes on this branch (390 passed, coverage 74.55% ≥ 62% floor). Live on our farm already: docker compose up -d vm-watchdog, self-discovered project, both VMs healthy, restart: unless-stopped; the host cron has been removed.

@psyb0t

psyb0t commented Aug 19, 2026

Copy link
Copy Markdown
Owner

This is solid — the watchdog is well-scoped and safe. It only touches containers that are actually `unhealthy`, running the `dockurr/windows` image, in this Compose project (self-excluded), and only after a sustained `FailingStreak`, with a capped attempt budget and exponential backoff. So healthy VMs — and running backtests on them — are never interrupted, and it can't restart-loop. stdlib-only Docker client over the socket, no shell/eval, minimal surface. I like it.

Two things before I merge:

  1. Rebase on master. fix(backtest): scope job sweep to this terminal, stop leaking tester processes, tail the real log #13 just landed and touched the same files (`docker-compose.yml.j2`/`.example`, `Dockerfile.test`, `docs/operations.md`), so this is showing conflicts now.
  2. Update the PR description to match what shipped. The body still describes the earlier host-cron `scripts/watchdog.sh` / "single cron line" approach, but the actual change is a Compose service (`scripts/vm-watchdog.py`) that mounts the Docker socket. Worth making the description reflect the real footprint (a docker.sock-mounted sidecar) so the history is accurate.

Resolve the conflict and I'll merge. Nice work.

@Marinski
Marinski force-pushed the feat/vm-health-watchdog branch from 5ef74cf to 1d586d1 Compare August 19, 2026 19:13
@Marinski

Marinski commented Aug 19, 2026

Copy link
Copy Markdown
Contributor Author

Both done, plus one fix this branch needed that I found afterwards.

1. Rebased on master. The two commits now sit on 986a3be (post-#13). Two conflicts, both from #13 touching the same files:

docker-compose.yml.j2 auto-merged.

Worth noting the Dockerfile.test resolution is load-bearing in both directions: dropping either script from that COPY makes the corresponding test module fail at import with a FileNotFoundError from spec.loader.exec_module, not a readable assertion — so it fails as a collection error rather than a test failure.

2. PR description rewritten to match what actually ships. You were right that it was describing a different change: the body still had the host-cron scripts/watchdog.sh and "single cron line" framing from the first pass, while the branch ships the Compose sidecar.

The new description leads with the Docker socket mount, since that is the part of this that deserves review attention rather than the recovery logic — it is root-equivalent access to the host daemon, which is why the script stays stdlib-only with no shell, no eval, and exactly one mutating call. It also spells out the five conditions that all have to hold before anything is restarted, the backoff and attempt cap, and why it uses docker restart rather than recreate (recreate orphans a network_mode: service:<vm> sidecar — the failure mode #13 documented).

I left a short note at the bottom recording that the Compose service supersedes the original cron approach and why, so the history explains itself rather than looking like an unexplained pivot.

3. A busy VM is not a dead VM — this branch was treating it as one.

Running the watchdog in anger surfaced a real problem with it, so there is a third commit here now.

healthcheck.sh probes each API port with a 3s HTTP timeout and reports the whole VM DOWN if any port misses that window. That conflates two different failures: nothing listening, and something listening that is too busy to answer.

The second is routine — a compile, a Strategy Tester run, or a backtest saturates the guest CPU and /ping misses the window while every API process is alive and working. On its own that was a cosmetic red healthcheck. With this branch's watchdog on the other end of the signal it stops being cosmetic: the supervisor restarts a VM because it was busy, so a slow batch becomes a multi-minute outage and whatever was running is lost. I hit exactly that on my own host — five concurrent compiles were enough to trigger a restart, mid-run.

The fix uses time_connect to tell the cases apart:

  • Handshake completed, no HTTP response in the window → a process is listening and accepting. Report healthy, and name the slow ports in the verdict so it stays visible.
  • Nothing listening → connection refused, time_connect stays 0.000000 → still DOWN. That is the outage this check exists to catch and it is unaffected.
  • Empty probe result (no curl, crash) → DOWN. Fails closed.

Raising the timeout instead does not work: Docker allows the script 30s total and it probes every port on the VM.

I also made the three fixed paths (CONFIG, VM_GROUP, DNSMASQ_LEASES) overridable so the behavioral tests can run the real script against fixtures with a stub curl. The container sets none of them and gets the paths it always had.

Four new tests in tests/test_healthcheck_behavior.py cover answers / refused / slow-but-listening / missing-curl. The slow-but-listening one fails against the previous script, which is the point of it.

One limitation I did not fix, flagging rather than leaving it to be discovered: the healthy path measures 2.9s for 20 ports, but if many ports are simultaneously slow the script can approach Docker's 30s ceiling (20 × 3s worst case) and be killed — which counts as a failure again. Bounding total runtime, probably by probing in parallel, is a separate change and I did not want to fold it into this one. In practice the retries: 10 on the healthcheck plus the watchdog's own streak gate mean it takes sustained saturation to matter.

CI is green (lint + 430 tests) and the PR shows mergeable.

@psyb0t

psyb0t commented Aug 20, 2026

Copy link
Copy Markdown
Owner

One small correctness fix before merge: WATCHDOG_DRY_RUN=1 currently persists retry state.

sweep_once() calls decide(), which increments attempts and sets last_restart, then immediately calls save_state() before branching on dry_run. That means a dry-run can consume the real backoff / attempt budget without ever restarting anything. After enough dry-run passes, turning it off can leave the VM at GIVING UP.

I reproduced this against the exact PR-head script with a fake Docker client returning one dockurr/windows container in this Compose project, Status=unhealthy, FailingStreak=10, dry_run=True, and now=1000000. It logged DRY-RUN: would restart /mt5 ... (attempt 1) and persisted {"attempts": 1, "last_restart": 1000000, "healthy_since": 0}. No restart was called.

Could dry-run evaluate against a copy of the state and skip persistence, then add a test that asserts the state file remains unchanged? Everything else looks good.

@Marinski

Copy link
Copy Markdown
Contributor Author

Good catch, and thank you for reproducing it against the PR head rather than describing it — that made it unambiguous.

Fixed in d3a3bdb. sweep_once() now evaluates against a copy under dry-run and persists nothing; a real run is unchanged.

working = copy.deepcopy(state) if dry_run else state
action, reason = decide(working, status, streak, now)
if not dry_run:
    save_state(cid, working)

The GIVING UP log line reads from working too, so it still reports the attempt count it actually decided on.

Three tests, in tests/test_vm_watchdog.py:

  • test_dry_run_writes_no_state — your scenario. Asserts the state directory is still empty after a dry pass, so this fails on any persistence rather than only on the fields we happen to check today.
  • test_repeated_dry_runs_do_not_exhaust_the_attempt_budget — the consequence end to end: MAX_ATTEMPTS + 2 dry passes, then a real one, which must still restart. Asserted on the restart itself rather than on a number, so it stays honest if the cap ever moves.
  • test_a_real_run_still_persists_state — the other direction, since skipping persistence has to be dry-run only. A real run still records attempts and last_restart.

The first two fail against the previous script; I checked rather than assumed.

Worth stating what the bug actually cost, because it is nastier than a stale counter: the failure only appears after dry-run is turned off. You would run dry to satisfy yourself the thing was safe, enable it, and find the watchdog had already decided to give up on that VM — refusing to act at precisely the moment it was finally allowed to. A supervisor that is silently disarmed by its own rehearsal is worse than one that never ran.

Full suite green: 433 passed, lint clean.

@psyb0t

psyb0t commented Aug 20, 2026

Copy link
Copy Markdown
Owner

Follow-up from the dry-run review. That fix looks correct. I found two remaining issues from a full pass:

  1. Invalid watchdog configuration crashes the daemon instead of failing fast. The original sidecar parses WATCHDOG_BACKOFF_ATTEMPTS= as an empty list. After the first recorded restart, the next unhealthy pass indexes that list and raises IndexError: list index out of range. I reproduced this against the current PR head using the real module with the environment variable set to an empty string. Please validate the full watchdog configuration at startup, especially a non-empty positive backoff list, and add environment-parsing coverage. The other numeric settings should reject empty, zero, and negative values where those do not make sense too.

  2. vm-watchdog mounts the root-equivalent Docker socket but runs a mutable python:3.12-alpine tag. Please pin that image to a digest before merge. A moving image tag is not an acceptable trust boundary for a container that can control the host daemon.

The diff is otherwise clean from a supply-chain perspective: no opaque files, dependency or workflow changes, downloaders, shell execution, or dynamic execution primitives.

@Marinski

Copy link
Copy Markdown
Contributor Author

Both fixed in 1d8850f.

1. Configuration is validated at startup

Every setting now goes through a checked parser, and validate_config() refuses to start on any problem — exit 2, listing all of them rather than making you fix one per traceback:

[vm-watchdog] invalid configuration (1 problem(s)); refusing to start
[vm-watchdog]   WATCHDOG_BACKOFF_ATTEMPTS='' parsed to an empty list; expected at least one integer >= 1

Bad values fall back to the default so the module still imports (the tests load it directly), but main() will not run on a value nobody chose. That distinction seemed worth keeping: this thing holds the Docker socket, and silently substituting configuration is the wrong failure mode for it.

Rejected across the numeric settings: empty, non-integer, zero and negative where those make no sense. Two worth calling out:

  • WATCHDOG_RESET_SECONDS=0 is refused. The attempt budget would reset on the first healthy poll after a restart, which makes MAX_ATTEMPTS unreachable and the give-up path dead code — it disables the cap while looking like a tuning value.
  • WATCHDOG_IMAGE_FILTER= is refused. Not in your report, and worse than the crash: "".startswith() matches every image, so a blank filter made every container in the project a restart candidate — this watchdog included, and anything else sharing the project. It fails closed now rather than widening scope.

17 new tests in tests/test_vm_watchdog.py, all failing against the previous script: your exact case, the parametrised zero/negative/garbage matrix per variable, all-problems-at-once, the empty image filter, and — because a guard is only as good as its bypass — that the fallback value is non-empty so decide() cannot reach the indexing that raised IndexError even if validation were skipped.

2. Sidecar pinned by digest

image: python:3.12-alpine@sha256:d09d15e60962ca365d1cd544a48773bac9d33f2fb1b00f2aa0deec78ade7dc31

That is the multi-arch OCI index (16 platforms), not a single-arch manifest, so it still resolves per-platform while being immutable. Pinned in both docker-compose.yml.j2 and docker-compose.yml.example, with the refresh command in a comment above it so the next person updating it does not have to guess how it was produced.

You are right that a moving tag is not an acceptable trust boundary for a socket-mounting container. Worth noting the same argument applies to dockurr/windows, which this repo also runs untagged-by-digest — out of scope here, and I did not want to widen a review you had already scoped.

Knock-on: PR #10

Your finding is a class, not an instance, so I checked the other two open PRs for it.

I clamped there rather than refusing, on purpose. mt5api/config.py is imported by the whole API, so raising on an optional feature's tuning value would stop trading and backtesting too. The watchdog makes the opposite call for the opposite reason, and both files say so in a comment.

Full suite green: 450 tests here, lint clean.

@psyb0t

psyb0t commented Aug 21, 2026

Copy link
Copy Markdown
Owner

Thanks for the follow-up fixes. I found one blocking lifecycle conflict and two smaller scope bugs in the current head.

The blocker is the recovery operation itself. The watchdog calls Docker restart on the VM alone (client.restart(cid)). This repository already has a real Compose lifecycle regression proving that restarting an owner with a network_mode: service:<owner> sidecar leaves the sidecar detached from the owner network namespace. The owner ID remains unchanged, but the sidecar loses eth0 and its health check fails. Only recreating the owner and sidecar together restores it.

I ran that exact integration regression against this PR head in a disposable Compose project:

python3 -m pytest -v tests/integration/test_wickworks_lifecycle.py
4 passed in 107.10s

That directly contradicts the new watchdog documentation and module docstring, which say a plain Docker restart preserves Wickworks attachment. In production, every successful watchdog recovery would therefore leave Wickworks unable to reach the VM. Please change the recovery mechanism to coordinate the VM and its Wickworks sidecar as one lifecycle operation, and add a regression that exercises the watchdog path against the real topology.

Two scope issues also need tightening because this service holds the Docker socket:

  1. Container matching uses startswith(IMAGE_FILTER), so the default dockurr/windows filter also matches dockurr/windows-not-the-vm:latest. Match the image repository exactly, allowing only tag or digest separators.
  2. The code never excludes SELF_ID. With a valid operator override such as WATCHDOG_IMAGE_FILTER=python, the watchdog selects itself. Exclude its own container unconditionally, independent of image filtering.

The configuration validation fixes look good, and the current unit suite and lint are green. The lifecycle issue is still a merge blocker.

@psyb0t

psyb0t commented Aug 21, 2026

Copy link
Copy Markdown
Owner

One useful correction to my review: this repository already has the proven production recovery path from the merged Wickworks lifecycle fix.

Please rebase onto current master and build on scripts/recreate-vm.sh, rather than using docker restart from the watchdog. That helper discovers every network_mode: service:<vm> sidecar from the generated Compose file and recreates the VM together with those sidecars:

./scripts/recreate-vm.sh mt5
# docker compose up -d --force-recreate --no-deps mt5 wickworks

The real lifecycle regression covers this exact operation. The watchdog implementation needs to trigger the equivalent coordinated lifecycle action, not restart the owner alone.

@Marinski
Marinski force-pushed the feat/vm-health-watchdog branch from 1d8850f to 21e7619 Compare August 21, 2026 09:34
@Marinski

Copy link
Copy Markdown
Contributor Author

You're right, and the branch's own reasoning was wrong. It claimed docker restart was safe because the owner keeps its container ID — but
tests/integration/test_wickworks_lifecycle.py already asserts the opposite:
Docker tears the netns down on stop and builds a fresh one on start, so
restarting the owner alone strands the sidecar exactly as recreating it alone
does. I had that test in front of me and still shipped the wrong claim.

Rebased onto current master and rewritten to call scripts/recreate-vm.sh
rather than reimplement sidecar discovery. Now one commit.

Using compose from inside a container has three consequences worth flagging,
since they change the sidecar's shape:

  • The image now carries the docker CLI + compose plugin, bash and PyYAML
    (Dockerfile.watchdog). Base is still digest-pinned — this container mounts
    the root-equivalent Docker socket, so a moving tag would hand that to
    whatever the registry serves tomorrow.
  • The project has to be mounted at the same absolute path the host uses.
    Compose resolves relative bind mounts client-side, so ./scripts/x in the
    compose file has to land on the host's <project>/scripts/x, not on a path
    inside the watchdog. run.sh exports MT5_PROJECT_DIR so a normal install
    works with no extra configuration; validate_config() reports it at startup
    when it is missing and the watchdog then refuses to act, rather than falling
    back to a restart that looks like recovery and is not.
  • COMPOSE_PROJECT_NAME is passed explicitly. Compose otherwise derives
    the project from the directory name, and a mismatch there does not fail — it
    quietly creates a second set of containers beside the running ones.

Recovery also names the compose service now (from
com.docker.compose.service), not the container id, since that is what
recreate-vm.sh takes. A VM without that label is skipped rather than guessed
at.

Seven new tests cover the recreate path specifically: that it passes the
service name rather than the id, that the project name is passed through, that
a missing WATCHDOG_PROJECT_DIR refuses loudly, that a script failure
surfaces, and that both misconfigurations are caught at startup instead of when
a VM has already crashed. Dockerfile.test now copies recreate-vm.sh so the
offline suite can exercise it.

Full suite in the container test image: 458 passed, 2 skipped. The
watchdog image builds and has docker, docker compose, bash and PyYAML.

Not verified end to end: I have not run a real recreate against a live VM from
inside the sidecar, so the compose-in-container path is reasoned and unit
tested rather than demonstrated. If you would rather that be proven first, the
integration test would be the place.

@Marinski Marinski changed the title feat(ops): restart VM containers that are persistently unhealthy feat(ops): recreate VM containers that are persistently unhealthy Aug 21, 2026
@Marinski

Copy link
Copy Markdown
Contributor Author

Follow-up to my "not verified end to end" caveat above — I can now narrow it.

Deployed this to our own two-VM host and exercised the recreate path from
inside the running sidecar. The compose-in-container wiring works:

image tooling:
  Docker version 29.5.3, build d1c06ef6b41d88d76866aea43c246cd7c63d04fa
  Docker Compose version v5.1.4

wiring:
  project dir      present
  recreate script  executable
  compose file     present

[recreate-vm] DRY-RUN: would run 'docker compose up -d --force-recreate --no-deps mt5 wickworks'
[recreate-vm] DRY-RUN: would run 'docker compose up -d --force-recreate --no-deps mt5-b wickworks-b'

That is the part I most wanted evidence for: sidecar discovery reads the
generated compose file from inside the container and pairs each VM with its
own sidecar — mt5 → wickworks, mt5-b → wickworks-b — rather than lumping
them together or missing the second pair. This host also carries hand-added
services (mt5-b, wickworks-b, a log pruner) that are not in the template,
so the discovery is being read from real deployed state, not from the shipped
example.

Also confirmed on that deployment:

  • The container starts clean against the real wiring, i.e. validate_config()
    passes rather than exiting 2.
  • ${MT5_PROJECT_DIR} resolves to the same absolute path inside and out, which
    is what keeps compose from rewriting the project's relative bind mounts.

Still not demonstrated: a real, non-dry-run recreate. Nothing has gone
unhealthy since the deploy, so the branch that actually calls
recreate_vm() has not fired in anger — only its dry-run twin and the unit
tests. I would rather say that plainly than call this proven.

@Marinski
Marinski force-pushed the feat/vm-health-watchdog branch from 21e7619 to 1c1b830 Compare August 21, 2026 10:20
psyb0t pushed a commit that referenced this pull request Aug 25, 2026
recreate-vm.sh recreates with `docker compose up -d --force-recreate`, whose
implicit stop uses compose's own --timeout -- 10 SECONDS by default -- rather
than the service's declared stop_grace_period. A dockurr/windows guest cannot
shut down in ten seconds, so compose stops waiting and goes straight to
removing a container that is still running:

  Error response from daemon: cannot remove container "1d5e3c2f...":
  container is running: stop the container before removing or force remove

The script then exits 1 and the VM is left unhealthy with its network_mode
sidecar stranded on a dead netns -- the exact outcome recreate-vm.sh exists to
prevent.

It is timing-dependent, which is why it can look fine for a while. On the
deployment where this was found the script recreated two VMs successfully three
times inside one hour, then failed on the fourth attempt when the guest took
longer than ten seconds to go down.

The targets are now stopped explicitly first with a timeout that matches the
grace period, and the same value is passed to `up` so its implicit stop cannot
fall back to 10s. RECREATE_STOP_TIMEOUT overrides the 120s default; anything
calling this script on a timeout of its own should stay above it.

The script had no direct test coverage. tests/test_recreate_vm_script.py covers
it through --dry-run, so it needs no Docker daemon: stop-before-up ordering, the
timeout default and its override, and the sidecar expansion that is the reason
the script exists. Four of the seven fail against the current version.

Worth noting for #15: that watchdog delegates recovery to this script, so
merging it without this fix ships an automated recovery path that hits the
failure above.

Dockerfile.test copies a named subset of scripts/ and recreate-vm.sh was not in
it, so the new tests could not see the script. It is added to that COPY line;
nothing else about the image changes.
@psyb0t

psyb0t commented Aug 25, 2026

Copy link
Copy Markdown
Owner

I tested the current head and found that the recovery cap and backoff do not survive the recovery they trigger.

vm-watchdog.py persists state as <container-id>.json. recreate-vm.sh force-recreates the VM and its network_mode: service:<vm> sidecars, so the next poll sees a new container ID and loads a fresh state with attempts = 0 and last_restart = 0.

I reproduced this with max_recovery_attempts = 1 and a deliberately huge backoff:

  1. An unhealthy old-container-id was recovered once.
  2. The same Compose service returned as new-container-id.
  3. The next unhealthy sweep recovered it again immediately, also as attempt 1.

Both old-container-id.json and new-container-id.json were present. The existing backoff tests keep one fixed container ID, so they do not exercise the real recreate path.

Please persist watchdog state by stable service identity, for example Compose project plus com.docker.compose.service, rather than Docker container ID. Add a test that simulates the same service returning with a replacement ID and proves it cannot bypass the attempt cap or backoff.

Separately, Dockerfile.watchdog installs unpinned pyyaml at image-build time while the service has direct access to /var/run/docker.sock. Please pin the package version and verify its hash or otherwise make that dependency reproducible before giving this privileged component access to the host Docker API.

dockurr/windows keeps its container up while the Windows guest inside may
have crashed, so `restart: unless-stopped` never fires and every terminal
API in that VM stays dead until a human intervenes. This adds a
compose-managed sidecar that watches Docker health and recovers a VM on
its own.

Recovery is a COORDINATED RECREATE, not a restart
-------------------------------------------------
An earlier revision of this branch used `docker restart` through the
Docker API, on the reasoning that keeping the owner's container ID keeps
a wickworks sidecar's netns attachment intact. That reasoning is wrong,
and tests/integration/test_wickworks_lifecycle.py already proves it:
Docker tears the netns down on stop and builds a fresh one on start, so
restarting the owner alone strands the sidecar exactly as recreating the
owner alone does. Only recreating the owner together with its sidecars
repairs the binding.

So the watchdog shells out to scripts/recreate-vm.sh -- the helper an
operator runs by hand, and the one that lifecycle test covers -- rather
than reimplementing sidecar discovery. Two recovery paths that could
drift apart is precisely what this avoids.

Consequences of using compose from inside a container:
- The sidecar image now carries the docker CLI, the compose plugin, bash
  and PyYAML (Dockerfile.watchdog, base still digest-pinned because this
  container mounts the root-equivalent Docker socket).
- Compose resolves this project's relative bind mounts client-side, so
  the project has to be mounted through at the SAME absolute path the
  host uses. run.sh exports MT5_PROJECT_DIR; validate_config() reports it
  at startup when it is missing and the watchdog refuses to act, rather
  than falling back to a restart that looks like recovery and is not.
- COMPOSE_PROJECT_NAME is passed explicitly. Compose otherwise derives
  the project from the directory name, and a mismatch would not fail --
  it would quietly create a second set of containers beside the running
  ones.
- Recovery names the compose SERVICE, taken from the container's
  com.docker.compose.service label; a container id means nothing to
  compose. A VM without that label is skipped rather than guessed at.

Watchdog behaviour
------------------
- Scoped to this compose project and the dockurr/windows image, so nginx,
  wickworks, the log rotator and the watchdog itself are never touched.
- Acts only after health has stayed unhealthy for a sustained
  FailingStreak, so a busy VM mid-backtest is never interrupted.
- Per-container state on a named volume, exponential backoff between
  attempts, a bounded attempt budget, and a reset only after sustained
  health -- so a VM that crashes again immediately is not thrashed.
- --dry-run evaluates against a copy of the state, so dry passes cannot
  consume the real backoff and attempt budget.

Full suite passes in the container test image: 458 passed, 2 skipped.
…d; pin pyyaml by hash

Recovery is a recreate, which replaces the container - so state keyed by
container id was orphaned by the very recovery that wrote it. The next
poll saw a fresh id, loaded a fresh record at attempts=0, and the attempt
cap and backoff reset themselves on every recovery they were meant to
bound: a persistently broken VM was recovered forever, always at
'attempt 1'.

State is now keyed by stable compose identity (project + service label),
which survives the recreate. The service label is resolved before state
is touched; a container without one is skipped up front, since it can
neither be recreated nor tracked. Labels are sanitized before becoming a
file name.

Two regression tests drive the exact replacement-id scenario from review:
the attempt cap and the backoff window must both survive the recreate
they triggered, with the same service returning under a new container id
each pass. Both fail against the previous script.

Also from review: Dockerfile.watchdog installed unpinned pyyaml at build
time in an image that mounts the root-equivalent Docker socket. The
dependency is now pinned by version and hash (requirements-watchdog.txt,
pip --require-hashes: musllinux cp312 wheels for x86_64/aarch64 plus the
sdist), same trust argument as the digest-pinned base image.
@Marinski
Marinski force-pushed the feat/vm-health-watchdog branch from ae4352c to c29b289 Compare August 27, 2026 06:42
@Marinski

Copy link
Copy Markdown
Contributor Author

Both fixed in c29b289.

1. State keyed by stable compose identity

You found the contradiction at the heart of it: recovery is a recreate, which replaces the container — so state keyed by container id was orphaned by the very action that wrote it. The replacement arrived with a fresh id, loaded a fresh record at attempts = 0, and the cap and backoff reset themselves on every recovery they were meant to bound.

State now lives at /state/<project>.<service>.json, from the compose project plus com.docker.compose.service — the identity that survives the recreate. The service label is resolved before state is touched (it was already required for the recreate itself), and a container without one is skipped up front: it can neither be recreated nor tracked. Both label values are sanitized to a single path component before becoming a file name, since that is the only place outside text touches the filesystem.

Your scenario is now a test, twice over, in tests/test_vm_watchdog.py:

  • test_the_attempt_cap_survives_the_recreate_it_triggeredMAX_ATTEMPTS=1, huge backoff: old-container-id is recovered once, the same compose service returns as new-container-id still unhealthy, and the second sweep must recover nothing.
  • test_backoff_survives_the_recreate_it_triggered — the same replacement-id handover asserted on the backoff window instead: inside the window the replacement waits, past it the recovery happens and attempts reads 2 — accumulated across three different container ids.

Both fail against the previous script; I checked rather than assumed. The pre-existing backoff tests keep their single fixed id, which is exactly why they never caught this — these two are the ones that pin the boundary.

2. pyyaml pinned by version and hash

Dockerfile.watchdog now installs from requirements-watchdog.txt with pip --require-hashes: pyyaml==6.0.2 locked to the sha256 of the two musllinux cp312 wheels (x86_64 / aarch64 for python:3.12-alpine) plus the sdist as the fallback for any other platform. pip fails closed on anything not in that file, including a re-upload under the same version number. The refresh procedure is in a comment above the hashes, same as the base-image digest.

Merge order

For all four open PRs: #15#16#18#10. This one first — it is compose/ops-only and overlaps the others in nothing but a Dockerfile.test COPY line. #16/#18/#10 all touch mt5api/config.py, scripts/config_helper.py, config.yaml.example and the [Unreleased] changelog section, and were built in that sequence. I will rebase each successor promptly as its predecessor lands.

Also rebased onto current master: #17 landed on Dockerfile.test and scripts/recreate-vm.sh after my push, which left this PR conflicting and CI unable to build the merge commit. The one conflict (the Dockerfile.test COPY line) resolved as the union; the watchdog's calls into the now-slower-stopping recreate-vm.sh are interface-compatible, and #17's own tests/test_recreate_vm_script.py passes on this branch unmodified.

Full suite green (41 watchdog tests plus #17's 7 recreate-script tests, 463 total offline), lint clean.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants