Move health checks into Docker #219
Merged
Conversation
This file contains 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
kevinmcconnell
commented
Apr 13, 2023
Comment on lines
+18
to
+19
| app = MRSK.app(role: role) | ||
| auditor = MRSK.auditor(role: role) |
There was a problem hiding this comment.
We were starting to say MRSK.app(role: role) etc a lot in this method.
kevinmcconnell
commented
Apr 13, 2023
Comment on lines
+21
to
23
| def status | ||
| pipe container_id, xargs(docker(:inspect, "--format", DOCKER_HEALTH_STATUS_FORMAT)) | ||
| end |
There was a problem hiding this comment.
Healthcheck and App both define a status command to fetch container status. It's a bit same-y, but the container name/id differs in each, and letting them figure that out themselves ended up looking clearer.
kevinmcconnell
commented
Apr 13, 2023
Comment on lines
-78
to
-79
| "traefik.http.services.#{traefik_service}.loadbalancer.healthcheck.path" => config.healthcheck["path"], | ||
| "traefik.http.services.#{traefik_service}.loadbalancer.healthcheck.interval" => "1s", |
There was a problem hiding this comment.
We don't need Traefik to healthcheck any more, since Docker is doing it. Also this wouldn't work for any custom cmd anyway.
kevinmcconnell
commented
Apr 13, 2023
| @@ -0,0 +1,39 @@ | |||
| class Mrsk::Utils::HealthcheckPoller | |||
There was a problem hiding this comment.
Named this HealthcheckPoller rather than something like Healthcheck, since the actual healthcheck is provided for us by Docker. This class just needs to know how to wait for the status to reach something runnable.
Replaces our current host-based HTTP healthchecks with Docker healthchecks, and adds a new `healthcheck.cmd` config option that can be used to define a custom health check command. Also removes Traefik's healthchecks, since they are no longer necessary. When deploying a container that has a healthcheck defined, we wait for it to report a healthy status before stopping the old container that it replaces. Containers that don't have a healthcheck defined continue to wait for `MRSK.config.readiness_delay`. There are some pros and cons to using Docker healthchecks rather than checking from the host. The main advantages are: - Supports non-HTTP checks, and app-specific check scripts provided by a container. - When booting a container, allows MRSK to wait for a container to be healthy before shutting down the old container it replaces. This should be safer than relying on a timeout. - Containers with healthchecks won't be active in Traefik until they reach a healthy state, which prevents any traffic from being routed to them before they are ready. The main _disadvantage_ is that containers are now required to provide some way to check their health. Our default check assumes that `curl` is available in the container which, while common, won't always be the case.
|
Oh this seems nice. |
|
Excellent change!!! |
duyanton
added a commit
to duyanton/rails
that referenced
this pull request
May 12, 2023
So the container can work with mrsk healthcheck. See: basecamp/kamal#219
4 tasks
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.
Replaces our current host-based HTTP healthchecks with Docker healthchecks, and adds a new
healthcheck.cmdconfig option that can be used to define a custom health check command. Also removes Traefik's healthchecks, since they are no longer necessary (Traefik watches the Docker health change events).When deploying a container that has a healthcheck defined, we wait for it to report a healthy status before stopping the old container that it replaces. Containers that don't have a healthcheck defined continue to wait for
MRSK.config.readiness_delay.There are some pros and cons to using Docker healthchecks rather than checking from the host. The main advantages are:
docker psoutput,mrsk app details, etc.The main disadvantage is that containers are now required to provide some way to check their health. MRSK's default check assumes that
curlis available in the container which, while common, won't always be the case.