Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

docker: add healthcheck #1559

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

docker: add healthcheck #1559

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
14a06d4
docker: add healthchecks
MNThomson Jul 30, 2024
ff6270e
delete extra newline
MNThomson Jul 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,14 +80,14 @@ RUN adduser --system --home /var/lib/sqld --uid 666 --gid 666 sqld
WORKDIR /var/lib/sqld
USER sqld

COPY docker-entrypoint.sh /usr/local/bin
COPY docker-wrapper.sh /usr/local/bin
COPY docker-entrypoint.sh docker-wrapper.sh docker-healthcheck.sh /usr/local/bin

COPY --from=gosu /usr/local/bin/gosu /usr/local/bin/gosu
COPY --from=builder /etc/ssl/certs/ca-certificates.crt /etc/ssl/certs/ca-certificates.crt
COPY --from=builder /target/release/sqld /bin/sqld

USER root

HEALTHCHECK --interval=2s CMD /usr/local/bin/docker-healthcheck.sh
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@athoscouto I would like to merge this PR. Just want to make sure this won't cause us any troubles on Fly. Please confirm.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MNThomson, sorry for late response - let's make HEALTHCHECK parameters configurable to simplify build of image with tuned parameters like this:

ARG HEALTHCHECK_INTERVAL=2s
ARG HEALTHCHECK_TIMEOUT=5s
ARG HEALTHCHECK_START_PERIOD=0s
ARG HEALTHCHECK_RETRIES=3
...
HEALTHCHECK --interval=$HEALTHCHECK_INTERVAL --timeout=$HEALTHCHECK_TIMEOUT --start-period=$HEALTHCHECK_START_PERIOD --retries=$HEALTHCHECK_RETRIES CMD ...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be even better to have a conditional check and add healthcheck only if those parameters are set

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A healthcheck should always run, please do not provide a footgun.

However this could be best managed by tuning it correctly by default.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps worth a mention that a user can easily disable a healthcheck using docker CLI or docker compose or any other container execution framework. But if that one is correct in the first place, it is highly unlikely and provides a lot of value. :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not every execution framework gives that option so it would be great to be able to control it with env vars

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TLDR; ship it without the healthcheck enabled by default, and update the dockerhub README with an example on how to enable it using docker compose.

Agreed with @haaawk on conditionally enabling, either via cmd and/or env. I'm fighting github actions right now at 2AM to get the go-gorm db wrapper shipped, and it was all because there was no way to configure a healthcheck, and the service was taking too long to come up.

For now I'm putting the pipeline to sleep to give the service enough time to come up, but it would be a nightmare if the health check always killed the container before the server fully started only because the host was slow. While I understand @Everspace's concern, neither to mysql nor postgres docker images have default healthcheck; but both images do offer facilities/cli commands to enable them if needed. I suspect this is to prevent containers from spontaneously ejecting or self-destructing even though the user hasn't set a health check.

ENTRYPOINT ["/usr/local/bin/docker-wrapper.sh"]
CMD ["/bin/sqld"]
4 changes: 2 additions & 2 deletions Dockerfile.dev
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,14 +59,14 @@ RUN adduser --system --home /var/lib/sqld --uid 666 --gid 666 sqld
WORKDIR /var/lib/sqld
USER sqld

COPY docker-entrypoint.sh /usr/local/bin
COPY docker-wrapper.sh /usr/local/bin
COPY docker-entrypoint.sh docker-wrapper.sh docker-healthcheck.sh /usr/local/bin

COPY --from=gosu /usr/local/bin/gosu /usr/local/bin/gosu
COPY --from=builder /etc/ssl/certs/ca-certificates.crt /etc/ssl/certs/ca-certificates.crt
COPY --from=builder /sqld/bin /bin/sqld

USER root

HEALTHCHECK --interval=2s CMD /usr/local/bin/docker-healthcheck.sh
ENTRYPOINT ["/usr/local/bin/docker-wrapper.sh"]
CMD ["/bin/sqld"]
18 changes: 18 additions & 0 deletions docker-healthcheck.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
#!/usr/bin/env bash
set -euo pipefail

SQLD_HTTP_LISTEN_ADDR="${SQLD_HTTP_LISTEN_ADDR:-"0.0.0.0:8080"}"
SQLD_HTTP_LISTEN_ADDR="${SQLD_HTTP_LISTEN_ADDR//:/\/}"

exec 3<>"/dev/tcp/$SQLD_HTTP_LISTEN_ADDR"
echo -e "GET /health HTTP/1.1\r\nConnection: close\r\n\r\n" >&3
RESPONSE=$(cat <&3)
exec 3<&- && exec 3>&-

if echo "$RESPONSE" | grep -q "HTTP/1.1 200 OK"; then
exit 0
else
echo "Did not receive HTTP 200 response"
echo "$RESPONSE"
exit 1
fi
Loading