fix(docker): make healthcheck role-aware for Sidekiq#20
Open
andersonlemesc wants to merge 1 commit into
Open
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideMakes the container healthcheck script role-aware by introducing a dedicated healthcheck executable and wiring Docker’s HEALTHCHECK to use it instead of a hardcoded HTTP probe, preparing support for non-web roles like Sidekiq. Sequence diagram for Docker healthcheck using role-aware scriptsequenceDiagram
participant DockerEngine
participant Container
participant HealthcheckScript as evo_auth_healthcheck
participant WebProcess
participant SidekiqProcess
DockerEngine->>Container: Execute HEALTHCHECK
Container->>HealthcheckScript: /usr/local/bin/evo-auth-healthcheck
HealthcheckScript->>Container: Read ROLE env
alt ROLE = web
HealthcheckScript->>WebProcess: HTTP GET /health on port 3001
WebProcess-->>HealthcheckScript: 200 OK or error
alt status OK
HealthcheckScript-->>DockerEngine: exit 0 (healthy)
else status not OK
HealthcheckScript-->>DockerEngine: exit 1 (unhealthy)
end
else ROLE = sidekiq
HealthcheckScript->>SidekiqProcess: Perform sidekiq-specific checks
SidekiqProcess-->>HealthcheckScript: status result
alt status OK
HealthcheckScript-->>DockerEngine: exit 0 (healthy)
else status not OK
HealthcheckScript-->>DockerEngine: exit 1 (unhealthy)
end
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Relying on
--chown=1000:1000for the healthcheck script couples it to a hard-coded UID/GID; consider either copying as root and leaving it world-readable/executable or referencing therailsuser/group symbols to avoid breakage if these IDs change.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Relying on `--chown=1000:1000` for the healthcheck script couples it to a hard-coded UID/GID; consider either copying as root and leaving it world-readable/executable or referencing the `rails` user/group symbols to avoid breakage if these IDs change.
## Individual Comments
### Comment 1
<location path="Dockerfile" line_range="55-56" />
<code_context>
EXPOSE 3001
-# Health check
HEALTHCHECK --interval=30s --timeout=10s --start-period=60s --retries=3 \
- CMD curl -f http://localhost:3001/health || exit 1
+ CMD /usr/local/bin/evo-auth-healthcheck
</code_context>
<issue_to_address>
**suggestion:** Prefer exec-form HEALTHCHECK CMD to avoid an extra shell and improve signal handling.
In this case you can write it as `CMD ["/usr/local/bin/evo-auth-healthcheck"]`. This removes the implicit `/bin/sh -c` wrapper, keeps argument handling explicit, and aligns with Docker’s guidance for CMD/ENTRYPOINT.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
2c30bc8 to
c6be346
Compare
Author
|
Applied the suggestion and updated the Dockerfile to use the exec form for the healthcheck: HEALTHCHECK --interval=30s --timeout=10s --start-period=60s --retries=3 \
CMD ["/usr/local/bin/evo-auth-healthcheck"]
This avoids Docker running the healthcheck through an implicit /bin/sh -c wrapper. Since the healthcheck target is an executable script with a shebang, the exec form is a better fit and should inspect as CMD
instead of CMD-SHELL. |
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.
Problem
The auth Docker image is reused by two different runtime roles:
3001Today the image-level Docker
HEALTHCHECKalways runs:That is correct for the web container, but it is not valid for the Sidekiq container. When the same image is used to start Sidekiq, the worker can boot successfully while Docker still marks it as unhealthy because nothing is listening on
/healthinside that container.This is especially problematic on deployment platforms that do not expose Docker Compose's
healthcheck.disable: trueoverride in their UI/schema. Easypanel is one concrete example: operators can configure environment variables and commands, but cannot reliably disable the inherited image healthcheck for only the worker service. The result is a healthy Sidekiq process reported as an unhealthy container.Why not remove the healthcheck?
Removing the image healthcheck would avoid the Sidekiq false negative, but it would also remove a useful default for the Rails web service. Since this image intentionally supports multiple roles, the image healthcheck should understand the active role instead of assuming every container is an HTTP server.
This follows the common split used by containerized applications:
Changes
bin/healthcheckas the image healthcheck entrypoint.Dockerfileso DockerHEALTHCHECKinvokes/usr/local/bin/evo-auth-healthcheck.SERVICE_ROLE=webprobeshttp://127.0.0.1:${PORT:-3001}/health.SERVICE_ROLE=sidekiqsupport, which validates that a Sidekiq process is running instead of probing HTTP.DISABLE_HEALTHCHECK=trueas an explicit bypass for constrained platforms.SERVICE_ROLEandDISABLE_HEALTHCHECKin.env.example.Operational usage
For the Rails web container, no change is required. The default remains:
For the Sidekiq container, set:
If a deployment platform needs to bypass the image healthcheck entirely, set:
References / rationale
HEALTHCHECK; when the same image runs multiple roles, that command must be role-aware or overridden by the runtime./healthor/upare intended for HTTP-serving app processes./health.Validation
Built and tested the image locally:
Verified Docker image healthcheck config:
docker inspect evo-auth-healthcheck-test --format "{{json .Config.Healthcheck}}"Verified bypass mode:
Verified Sidekiq role mode: