fix(observability): make make up actually apply rendered config - #19
Merged
Conversation
…oad` `docker compose up -d` recreates a container only when its *service definition* changes — image, command, mounts, environment. The contents of a bind-mounted file are invisible to it. snmp-exporter parses its config once at startup, so `scripts/render-config.sh` could write a brand-new snmp.yaml, `make up` report success, and the exporter go on serving the config it parsed minutes earlier. That is not hypothetical. In PR #18 the corrected pfSense walk was rendered to disk, `make up` ran clean, and the exporter kept timing out at 45s until someone ran `docker compose restart snmp-exporter` by hand. The failure mode is the bad one: the tool says done, and only the target disagrees. The mechanism to fix it already existed. `make reload` has been POSTing /-/reload to all three config-reading services since the rotation tooling landed; `make up` simply never called it. So this is wiring, not a new capability — no unconditional `docker compose restart`, which would drop the TSDB head on Prometheus and is heavier than the reload each service already serves. Extracted into scripts/reload-config.sh because the retry does not fit in Makefile recipe lines, and because `up` and `reload` now need the same logic. Three outcomes, deliberately distinguished: wget exit 4 running but not yet listening — retry 1s, up to RELOAD_TIMEOUT (60s). `up -d` returns when containers are started, not when they are ready. wget exit 8 the service answered and refused: the config on disk does not parse. Waiting cannot fix that, so fail immediately and say it is still serving the previous config. not running stopped or crash-looping (inspect reports `restarting`) — fail immediately with `make ps` advice rather than burning the timeout rediscovering it. Failing is the point. The obvious way to stop a slow box breaking `make up` is to swallow reload errors, which rebuilds the exact defect this closes: `make up` reporting success over a stale config. A reload that never lands is a failed deploy and now says so. `compose ps -q` plus `docker inspect -f`, not `compose ps --format '{{.State}}'`: custom Go templates only reached `compose ps` in a later Compose v2, and on an older one the template is read as a literal format name, so every service looks stopped and `make up` fails on a healthy stack. Widened to prometheus and alertmanager as well. Their configs are bind mounts with the identical staleness problem, `make reload` already treated the three as a unit, and alertmanager.yaml changes do need a reload even though the rendered webhook_url does not (url_file is read at notify time). Docs carried the inverse of the truth. rotate-snmp-community.md said `make up` "also works and is not wrong, it just does more than is needed" — it did less than was needed, and would have walked the next operator into PR #18. deploy-stack.md's "recreates only what changed" was the misconception itself. Co-Authored-By: Claude Opus 5 <[email protected]>
The success line printed the literal text `${GRAFANA_PORT:-3000}`:
up — Grafana: http://localhost:${GRAFANA_PORT:-3000}
The recipe was `@printf '...$${GRAFANA_PORT:-3000}...'`. Make turns `$$` into
`$`, but the format string is single-quoted, so the shell never expanded it.
Switching to double quotes is the obvious fix and the wrong one. GRAFANA_PORT
lives in stacks/<stack>/.env, which docker compose reads and make does not, so
the recipe shell has no such variable and `${GRAFANA_PORT:-3000}` would always
take the default. An operator running on 3001 would get a confidently printed
link to 3000 — a wrong URL that looks right, which is worse than the visibly
broken text it replaced.
Read the value back out of the .env that `make up` just rendered instead, with
3000 as the fallback for a fresh clone. Passed as a %s argument rather than
interpolated into the format, so a stray % in the value cannot be taken as a
format spec.
The other printfs in this file were already correct — they pass values as %s
arguments and only ever single-quote the format itself.
Co-Authored-By: Claude Opus 5 <[email protected]>
…xit status 252f54e claimed wget exit 8 ("server issued an error response") distinguishes a service that answered and refused from one that is not listening yet. That is GNU wget's contract. All three images ship BusyBox wget, which exits 1 for everything: $ docker compose exec -T snmp-exporter wget -q -O- http://localhost:9116/nope wget: server returned error: HTTP/1.1 404 Not Found exit=1 $ docker compose exec -T snmp-exporter wget -q -O- http://localhost:19999/x wget: can't connect to remote host (127.0.0.1): Connection refused exit=1 So the fast-fail branch could never fire. An unparseable snmp.yaml — the case the branch exists for — waited out the full 60s and then reported "did not accept a reload within 60s", which is the wrong diagnosis for a service that answered in milliseconds and said exactly what was wrong. Match on the message instead. The exit 8 test is kept for an image that ever ships GNU wget, but on these three it is the string that fires. Worth recording how this survived review: the original was tested against a stubbed `docker` covering all six branches, and it passed, because the stub returned the exit codes the script expected rather than the ones BusyBox actually returns. A test harness written from the same assumption as the code cannot falsify that assumption. It only failed when run against the real pinned image, where the bad-config case took 61s instead of 1s. Verified against prom/snmp-exporter:v0.30.1 with the real compose file: unparseable config 61s, wrong message -> 1s, "refused the reload: its config on disk does not parse", container confirmed still serving the previous one stopped container 1s, points at `make ps` crash-looping docker reports `restarting`; refused in 1s cold-start race retried until the listener bound, 1s Co-Authored-By: Claude Opus 5 <[email protected]>
snmp_exporter v0.30.1 serves no /health. The healthcheck asked for one anyway,
so every probe since the service was added returned 404 and the container was
permanently unhealthy while polling all four devices perfectly:
exit=1 wget: server returned error: HTTP/1.1 404 Not Found
It stayed invisible because nothing depends_on snmp-exporter and Docker does not
restart a container for being unhealthy. The only symptom was a status in
`make ps` that had been there since day one, which reads as normal.
That is the harm. A healthcheck that cannot pass is worse than no healthcheck:
it reports a permanent fault the operator learns to scroll past, so the one time
the service really is sick it says exactly what it always said. And it is a trap
for whoever first writes `depends_on: {snmp-exporter: {condition:
service_healthy}}` — that never becomes satisfiable, and compose hangs.
snmp-exporter was the only one of the four wrong; prometheus and alertmanager
already probe /-/healthy and grafana /api/health. Verified against the pinned
images by running each healthcheck's exact probe inside its own container:
prometheus /-/healthy OK
alertmanager /-/healthy OK
snmp-exporter /-/healthy OK
snmp-exporter /health FAILS <- what was configured
grafana /api/health OK
and by confirming Docker now reports snmp-exporter `healthy` with exit=0, where
it previously logged the 404 above.
Found while verifying the reload fix in this branch against a local copy of the
stack, not by looking for it.
Co-Authored-By: Claude Opus 5 <[email protected]>
Gerrrt
added a commit
that referenced
this pull request
Aug 19, 2026
fix(observability): make `make up` actually apply rendered config
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
1 participant
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.
docker compose up -drecreates a container only when its service definitionchanges. The contents of a bind-mounted file are invisible to it. snmp_exporter
parses its config once at startup, so
render-config.shcould write a brand-newsnmp.yaml,make upreport success, and the exporter go on serving the configit parsed minutes earlier.
This is what happened in #18: the corrected pfSense walk was rendered to disk,
make upran clean, and the exporter kept timing out at 45s until someone randocker compose restart snmp-exporterby hand.The mechanism to fix it already existed —
make reloadhas POSTed/-/reloadto all three config-reading services since the rotation tooling landed.
make upjust never called it. So this is wiring, not a new capability. No unconditional
docker compose restart, which would drop Prometheus's TSDB head and is heavierthan the reload each service already serves.
Commits
252f54emake upreloads config; logic extracted toscripts/reload-config.sh; three docs correctedc3c7765${GRAFANA_PORT:-3000}684ad0ab4da221/health, which 404s — found while verifying the aboveBehaviour change
make upcan now fail where it always succeeded. If a service will not takethe config on disk, that is a failed deploy and it says so. The obvious way to
stop a slow box breaking
make upis to swallow reload errors, which rebuildsthe exact defect this closes.
Three outcomes are distinguished:
RELOAD_TIMEOUT(60s).up -dreturns when containers are started, not when they are ready.
waiting cannot help.
make psadvice ratherthan burning the timeout rediscovering it.
Verification
Against a local copy of the stack using the pinned images, replaying #18 exactly
(
main's broad pfSense walk as the old config, #18's corrected walk as the new):>, inode unchanged)docker compose up -d→ reportsRunningscripts/reload-config.sh(1.4s)Failure paths, real containers: unparseable config → 1s, container confirmed
still serving the previous config; stopped → 1s; genuine crash-loop
(
restarting restarts=7) → 1s; cold-start race → retried until the listenerbound.
684ad0aexists because the first version of this passed a stubbed-dockertest suite covering all six branches and was still wrong — the stub returned the
exit codes the script expected rather than the ones BusyBox returns. It only
failed against the real image, where the bad-config case took 61s instead of 1s.
Not covered: real SNMP round-trips to the devices, and
make render'ssops/age path. Neither is touched by these changes, but the reload has not been
exercised on
prometheus(10.0.99.20) itself.🤖 Generated with Claude Code