feat(ops): recreate VM containers that are persistently unhealthy - #15
feat(ops): recreate VM containers that are persistently unhealthy#15Marinski wants to merge 2 commits into
Conversation
|
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 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:
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. |
010327e to
5ef74cf
Compare
|
Thanks — agreed on the cron, and this branch now delivers exactly the Compose-managed sidecar you described.
Behavioural coverage ( Compose wiring:
|
|
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:
Resolve the conflict and I'll merge. Nice work. |
5ef74cf to
1d586d1
Compare
|
Both done, plus one fix this branch needed that I found afterwards. 1. Rebased on master. The two commits now sit on
Worth noting the 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 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 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.
The second is routine — a compile, a Strategy Tester run, or a backtest saturates the guest CPU and The fix uses
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 ( Four new tests in 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 CI is green (lint + 430 tests) and the PR shows mergeable. |
|
One small correctness fix before merge:
I reproduced this against the exact PR-head script with a fake Docker client returning one 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. |
|
Good catch, and thank you for reproducing it against the PR head rather than describing it — that made it unambiguous. Fixed in 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 Three tests, in
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. |
|
Follow-up from the dry-run review. That fix looks correct. I found two remaining issues from a full pass:
The diff is otherwise clean from a supply-chain perspective: no opaque files, dependency or workflow changes, downloaders, shell execution, or dynamic execution primitives. |
|
Both fixed in 1. Configuration is validated at startupEvery setting now goes through a checked parser, and Bad values fall back to the default so the module still imports (the tests load it directly), but Rejected across the numeric settings: empty, non-integer, zero and negative where those make no sense. Two worth calling out:
17 new tests in 2. Sidecar pinned by digestimage: python:3.12-alpine@sha256:d09d15e60962ca365d1cd544a48773bac9d33f2fb1b00f2aa0deec78ade7dc31That 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 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 Knock-on: PR #10Your 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. Full suite green: 450 tests here, lint clean. |
|
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 ( I ran that exact integration regression against this PR head in a disposable Compose project: 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:
The configuration validation fixes look good, and the current unit suite and lint are green. The lifecycle issue is still a merge blocker. |
|
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 ./scripts/recreate-vm.sh mt5
# docker compose up -d --force-recreate --no-deps mt5 wickworksThe real lifecycle regression covers this exact operation. The watchdog implementation needs to trigger the equivalent coordinated lifecycle action, not restart the owner alone. |
1d8850f to
21e7619
Compare
|
You're right, and the branch's own reasoning was wrong. It claimed Rebased onto current Using compose from inside a container has three consequences worth flagging,
Recovery also names the compose service now (from Seven new tests cover the recreate path specifically: that it passes the Full suite in the container test image: 458 passed, 2 skipped. The Not verified end to end: I have not run a real recreate against a live VM from |
|
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 That is the part I most wanted evidence for: sidecar discovery reads the Also confirmed on that deployment:
Still not demonstrated: a real, non-dry-run recreate. Nothing has gone |
21e7619 to
1c1b830
Compare
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.
|
I tested the current head and found that the recovery cap and backoff do not survive the recovery they trigger.
I reproduced this with
Both Please persist watchdog state by stable service identity, for example Compose project plus Separately, |
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.
ae4352c to
c29b289
Compare
|
Both fixed in 1. State keyed by stable compose identityYou 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 State now lives at Your scenario is now a test, twice over, in
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
Merge orderFor all four open PRs: #15 → #16 → #18 → #10. This one first — it is compose/ops-only and overlaps the others in nothing but a Also rebased onto current master: #17 landed on Full suite green (41 watchdog tests plus #17's 7 recreate-script tests, 463 total offline), lint clean. |
dockurr/windowskeeps the container "up" while the Windows guest may have crashed internally — an unexpected shutdown (Event 6008), a wedged terminal, an OOM.restart: unless-stoppednever 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-watchdogsidecar that restarts those containers automatically.What it is
A small
python:3.12-alpineservice that polls the Docker API over a mounted/var/run/docker.sockanddocker restarts VM containers whose health has stayedunhealthyfor a sustained streak.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 isPOST /containers/<id>/restart.What it will touch
A container is only ever restarted when all of these hold:
unhealthy(neverhealthy, neverstarting)FailingStreak≥WATCHDOG_MIN_FAILING_STREAK(default 10, ≈5 min at the 30s healthcheck interval)WATCHDOG_IMAGE_FILTER(defaultdockurr/windows)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 thevm-watchdog-statevolume.WATCHDOG_DRY_RUNlogs decisions without acting.docker restartis used rather than recreate, deliberately: recreating a VM orphans anetwork_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 volumetests/test_vm_watchdog.py— 293 lines covering the gates above: image filter, project scoping, self-exclusion, streak threshold, backoff, attempt cap, state persistenceDockerfile.test— copies the script so the tests can import itdocs/operations.md— "Auto-recovery" sectionNotes
MT5AutoRebootscheduled task, which reboots on a fixed timer and can interrupt long backtests. Operators who disable that task to protect backtests still get crash recovery here.masterafter fix(backtest): scope job sweep to this terminal, stop leaking tester processes, tail the real log #13.