Conversation
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.
…sy tolerance; continuous healthy clock
The container was given WATCHDOG_PROJECT_DIR but never MT5_PROJECT_DIR, while
docker-compose.yml requires ${MT5_PROJECT_DIR:?} on every compose command. So
recreate-vm.sh's `docker compose` failed at interpolation before it could stop
anything, and no real (non-dry-run) recovery could complete. recreate_env()
now builds the helper's environment explicitly - COMPOSE_PROJECT_NAME and
MT5_PROJECT_DIR from the watchdog's own host path - and run.sh persists the
value to .env (first line, single-quoted) so make down/logs and manual compose
keep working after it exits; run.sh also refuses a stale export from another
checkout, which would otherwise be persisted and then acted on. The compose
files pass the variable through as well.
healthcheck.sh: a port that accepts TCP but never answers HTTP is no longer
healthy forever. The busy tolerance is bounded at HEALTHCHECK_SLOW_GRACE
consecutive checks (default 10), after which the port is reported hung and
DOWN; an answer or a refused connection resets it. An unwritable state dir
degrades to the old tolerance and says so in the verdict.
vm-watchdog.py: healthy_since means continuously healthy - starting, a
sub-threshold unhealthy poll, or no healthcheck all restart the reset clock.
Image filter is an exact repository match, and the watchdog resolves its own
full container id at startup and never selects itself.
Tests: the real recreate-vm.sh runs under the watchdog's exact child
environment with the host variable scrubbed (plus a control proving the
pre-fix environment fails at interpolation); run.sh's actual .env block is
executed; a host integration test drives a real recovery through the built
sidecar on a disposable Compose project. The `assert ... or True` no-op and
the COMPOSE_PROJECT_NAME-only assertion are replaced.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…R too, and must not be silent `docker compose down` interpolates the compose file like every other compose command, so without MT5_PROJECT_DIR it failed at the required-variable check - the very finding this PR round fixes, reproduced by its own harness - and check=False hid that, leaving the disposable project (a socket-mounted watchdog included) running after the suite. The teardown now passes the variable the way an operator's shell does and raises if it fails. The in-container reproduction test also echoes the inner exit code, so its failure is provably the compose run inside the sidecar and not the outer exec. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…exit status
The check's wall clock was the PRODUCT of the probe timeout and the terminal
count. At PROBE_TIMEOUT_SECONDS=3, a 24-terminal VM whose ports are all silent
took 72s against the compose `timeout: 30s`, so Docker killed the check and
recorded "Health check exceeded timeout (30s)" instead of the verdict. A
supervisor cannot tell that from a dead VM, and it happens exactly when every
port is slow - while the VM is still booting its terminals - so a VM that was
merely starting looked identical to one that had crashed.
One background job per port. The bound is now one port's HOST WALK,
PROBE_TIMEOUT_SECONDS x 3 fallback hosts = 9s (measured at 9s), regardless of
terminal count. Not one probe: each job still tries the leased IP then the two
fallbacks, so raising the timeout to 10 would put us back at 30s. Said so in
both the code and the docs.
The verdicts travel in EXIT STATUSES, not in files, and that is the load-
bearing part. The obvious implementation gives each job a verdict file under a
`mktemp -d`; counter-review reproduced what that does on a bad day, with six
ports all answering HTTP 200 and /tmp full:
scratch-file version: DOWN ports: 6600 6601 6602 6603 6604 6605
sequential baseline: ok all ports up: 6600 ... 6605
Ten of those and the watchdog recreates a healthy VM, destroying two dozen
running backtests, because /tmp filled up. `/` on this host is at 91%. The
sequential loop needed no disk and neither does this: 0 up, 1 busy, 2 hung,
3 dead, 4 busy-but-counter-unwritable, read back with `wait` in port order.
Anything else - a job killed by a signal - is unknown and fails closed as down.
It also removes the trap (which used to delete the workdir and then let the
aggregation fabricate a total outage), and the temp dirs SIGKILLed checks leaked.
The slow/hung counters still use the disk, deliberately off the liveness path:
if they cannot be written the bound is off for that check and the verdict says
so, while every port's up/down is still what its probe found. Verified live.
Also fixed here, both found by auditing this change:
- A port configured twice was probed twice. Two jobs on one counter file is a
race; sequentially it was quieter and worse, incrementing that counter twice
per check so the hung bound fired at HALF the configured grace. Each port is
emitted once now.
- The header said the container is alpine/ash. It is dash.
Test defects fixed: `_run_with_slow_curl` omitted HEALTHCHECK_STATE_DIR and so
read and wrote the real /tmp/healthcheck-slow; the dead-port test counted digit
tokens rather than asserting the set. New coverage for per-port verdict routing
under mixed replies, fail-closed on a job killed before it reports, the disk
failure above, and - after live smoke caught a mangled `-w` format that every
existing test sailed past, because the stub curl ignores its arguments - the
real curl argument vector.
518 passed, lint clean, and both live VMs report `ok all ports up`.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
Author
|
Folding this into #15 rather than asking you to review it separately, and closing it here. Seven open PRs from one person is a lot to put in front of you, and three of them were the same subject: #15 changes The commit is now on If you would rather review it on its own after all, say so and I will reopen it — the branch is untouched. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The problem
healthcheck.shprobed the ports one after another, so the check's wall clockwas the product of the probe timeout and the terminal count.
PROBE_TIMEOUT_SECONDSis 3, and our bulk VM carries 24 terminals: 72s againstthe compose
timeout: 30s.Docker kills a check that overruns and records
instead of the verdict. That output is worse than useless, because a supervisor
cannot tell it apart from a dead VM. It is also most likely exactly when it
hurts most: while a VM is booting, every port is slow, so a VM that was merely
starting looked identical to one that had crashed, and the watchdog recreated
VMs that were doing nothing wrong.
The change
One background job per port. The worst case is now one probe timeout regardless
of how many terminals the VM carries.
parent aggregates after
wait. No shared state, so nothing to race on.status whitelist already in the file: an unexplained probe is never a silent
pass.
counter file is that port's alone. The one thing a background job cannot do is
set a variable the parent will see, so a job that cannot write its counter
leaves a marker file and the parent still prints
[slow-state unwritable: hung detection off].The worst case is one port's host walk, not one probe: each job still
tries the leased VM IP and then the two fallbacks in turn, so the bound is
PROBE_TIMEOUT_SECONDS × 3= 9s. Measured at exactly 9s. Raising the timeoutto 10 would put it back at 30s, so the code and the docs both say so.
Measured under busybox ash and under the container's own dash:
Why the verdicts are exit statuses and not files
This is the part worth your attention, because I had it wrong first.
The obvious implementation gives each job a verdict file under a
mktemp -d.Counter-review reproduced what that does on a bad day. Six ports, all
answering HTTP 200, with
/tmpfull:The write fails, the parent finds no verdict, and its fail-closed rule turns
"I cannot write my own scratch file" into "every terminal on this VM is down".
Ten of those in a row and the watchdog recreates a healthy VM, destroying two
dozen running backtests, because
/tmpfilled up. Our/is at 91%. Thesequential loop it replaces had no disk dependency and neither may this.
So each job returns
0up,1busy,2hung,3dead,4busy-but-the-counter-could-not-be-written, and the parent reads them back with
waitinport order. Anything else, such as a job killed by a signal, is unknown and
fails closed as down. No scratch, no cleanup, no signal handler.
That also removed two things the scratch version needed and got wrong: the
INT/TERMtrap deleted the workdir and returned, so the aggregation thenfound no verdicts and printed an authoritative total outage; and five
SIGKILLed checks leaked five temp dirs, two of them still being written to by
jobs that outlived the parent.
The slow/hung counters still touch the disk, deliberately off the liveness
path: if they cannot be written the bound is off for that check and the
verdict says so, while every port's up/down is still what its probe found.
Verified on a live VM with the counter path pointed at something unwritable.
Three more defects the audit turned up, all fixed
A port configured twice was probed twice. Fanned out that is two jobs on
one counter file. Sequentially it was quieter and worse: the counter was
incremented twice per check, so the hung bound fired at half the
configured grace. With
HEALTHCHECK_SLOW_GRACE=3the base branch reportsDOWN hungon the second check. Each port is emitted once now.The
-wformat reached curl with literal quotes around it. Every port onboth live VMs reported
slow but listeningwhile all 24 terminals wereanswering 401. Every test in the file passed anyway, because the stub curl
ignores its arguments. Fixed, and the gap closed: a test now captures the real
argument vector and pins the format and
--max-time. Live smoke is the onlything that caught this.
The file's header said the container is alpine/ash. It is dash.
What was checked
Five phases, receipt kept locally under a gitignored directory, run against
exactly the three files this PR touches and nothing else.
Ten verification passes, an independent counter-review by someone with no
context on the change, live smoke on both production VMs, ten final passes.
Eight findings, six of them from the counter-review, two of which changed the
design. All fixed. One deferral: the dedupe silences a misconfiguration rather
than reporting it, and I have noted in the docs that
config_helper.pyiswhere a duplicated port belongs, because a healthcheck's verdict is about
liveness.
Test defects found and fixed along the way:
_run_with_slow_curlomittedHEALTHCHECK_STATE_DIRand so read and wrote the real/tmp/healthcheck-slow,and the dead-port test asserted a count of digit tokens where a duplicate plus
a drop would have passed. Added coverage for per-port verdict routing under
mixed replies, fail-closed on a job killed before it reports, the disk failure
above, and the curl argument vector.
make test-unit(518) andmake lintare green. The suite runs the realscript under
sh. The disk-failure test fails against both scratch-filedesigns I tried, which is the point of it.
Merge order
This is stacked on #15, because that PR is what put the slow/hung bound into
the same loop; branching from
masterinstead would have meant hand-resolvingthe same file twice. It shows #15's commits until #15 lands, after which this
collapses to
scripts/healthcheck.sh, its test file, and one docs section.#15 → this → #21 → #16 → #18 → #19 → #10, and I will rebasepromptly as each predecessor merges. I rehearsed it: after #15, this and #21 both merge with zero conflicts, in either order, and the
combined tree is green. Happy to hold this until #15 is merged if you would
rather review it as a plain diff.
No CHANGELOG entry: the watchdog and this healthcheck work are all unreleased
and #15 carries no entry either, so a single entry covering the lot belongs on
whichever of these lands last. Say the word and I will add it there.
🤖 Generated with Claude Code