feat: add Docker HEALTHCHECK - #1273
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a Docker image HEALTHCHECK for acme-companion so orchestrators / docker compose up --wait can reliably gate on the companion being “up” (i.e., both letsencrypt_service and the bundled docker-gen are running), addressing issue #709.
Changes:
- Record
letsencrypt_serviceanddocker-genPIDs in/var/runfromapp/start.shand introduceapp/healthcheck.shto validate both are alive. - Add a Dockerfile
HEALTHCHECKthat runs the new healthcheck script. - Add an integration test (
container_health), wire it into the test list and CI matrix, and document Compose usage.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
app/start.sh |
Writes PID files for the two background services for later health verification. |
app/healthcheck.sh |
New script used by Docker healthcheck to validate both services are alive. |
Dockerfile |
Adds HEALTHCHECK instruction to the image. |
test/tests/container_health/run.sh |
New integration test waiting for container health status to become healthy. |
test/config.sh |
Registers the new container_health test in the suite. |
.github/workflows/test.yml |
Adds container_health to the CI test matrix. |
docs/Docker-Compose.md |
Documents how to use the image healthcheck with Compose (--wait / service_healthy). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Address Copilot review on nginx-proxy#1273: - app/healthcheck.sh: read a single line and require a numeric PID instead of using `cat`, so a malformed/empty/missing PID file is reported unambiguously. - container_health test: call /app/cleanup_test_artifacts in the EXIT trap, matching the other integration tests, so it does not leave state behind in the shared volumes when the suite is run locally/sequentially.
a89001b to
4462e57
Compare
4462e57 to
af74d30
Compare
2fa8977 to
236a159
Compare
|
@JamBalaya56562 please flag AI-generated PR explicitly in the PR description (ie "🤖 Generated with Claude Code" like in this PR) and add the AI agent as co-author of the commits. |
236a159 to
a9e4f44
Compare
|
@buchdag done, added Claude Code as co-author on the commits. The AI-generated flag is already in the PR description. |
463e0c3 to
bfb433a
Compare
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
bfb433a to
b8446eb
Compare
What
Add a Docker
HEALTHCHECKto the acme-companion image so orchestrators (anddocker compose up --wait/depends_on: condition: service_healthy) can gate on the companion being up. Closes #709.Why
app/start.shlaunches two background services —letsencrypt_serviceand the bundleddocker-gen— but its wait loop only monitors the docker-gen PID. Ifletsencrypt_servicedies, PID 1 (start.sh) and docker-gen stay alive, so the container keeps reporting up while certificate issuance/renewal has silently stopped. There is currently no signal for this.How
app/start.shrecords each background PID under/var/run(letsencrypt_service.pid,docker-gen.pid).letsencrypt_servicere-execs itself via itsEXITtrap (exec $0), which preserves the PID, so the recorded value stays valid across renewal loops.app/healthcheck.shverifies both PIDs are alive withkill -0and exits non-zero (with a message) otherwise.DockerfileaddsHEALTHCHECK --interval=30s --timeout=5s --start-period=30s --retries=3 CMD [ "/bin/bash", "/app/healthcheck.sh" ].I used PID files rather than
pgrepon purpose: docker-gen is launched with/app/letsencrypt_service_data.tmplin its arguments, sopgrep -f letsencrypt_servicematches docker-gen too and would mask a deadletsencrypt_service. PID +kill -0is unambiguous and matches the "verify both background PIDs are alive" direction from the issue.Tests / docs
container_healthintegration test (added totest/config.shand the CI matrix, runs under both 2containers and 3containers) waits for the container to report ahealthystatus.docs/Docker-Compose.mdgains a short "Health check" section coveringcompose up --waitandservice_healthy.Verified locally
Built the image and ran a 2-container setup: the container reaches
healthy; afterkill -9of theletsencrypt_servicePID,healthcheck.shreturns non-zero and Docker flips the container tounhealthyafter the retry budget — while the container itself stays "up", which is exactly the blind spot this closes.Out of scope (deliberately, can add later)
--start-interval(faster startup gating) needs Docker Engine 25+; omitted for build portability./proc/<pid>/cmdline) — negligible risk in the companion'''s small PID namespace, kept simple.🤖 Generated with Claude Code