-
Notifications
You must be signed in to change notification settings - Fork 66
added healthcheck in Dockerfile #1192
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.
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
base: main
Are you sure you want to change the base?
Conversation
curl-minimal
Dockerfile
Outdated
|
||
RUN yum update -y && \ | ||
yum install -y tar xz openssl && \ | ||
yum install -y tar xz openssl curl-minimal && \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You'll need to double check me on this, but I believe the amazonlinux:2023 image contains curl
already.
# - GRAPH_EXP_ENV_ROOT_FOLDER = /explorer | ||
# - PROXY_SERVER_HTTP_PORT = 80 | ||
# - LOG_STYLE = default | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't remove the documentation
Dockerfile
Outdated
EXPOSE 9250 | ||
|
||
HEALTHCHECK --interval=30s --timeout=5s --start-period=10s --retries=3 \ | ||
CMD curl -f http://localhost:${PROXY_SERVER_HTTP_PORT}/ || exit 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The /status
endpoint is what we should hit to test the server is up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, it seems the server url may be a complicated endeavor. We have multiple possible endpoints:
Config | URL |
---|---|
HTTP | http://localhost:${PROXY_SERVER_HTTP_PORT}/status |
HTTPS | https://localhost:${PROXY_SERVER_HTTPS_PORT}/status |
Neptune Notebook | http://localhost:9250/status |
hi @kmcginnes , Dockerfile is updated |
yum install -y tar xz openssl && \ | ||
# Amazon Linux 2023 base image is minimal and does not include curl by default, | ||
# so we explicitly install curl here to ensure availability. | ||
yum install -y tar xz openssl curl && \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did a quick test to see if curl was available in the base image. It seems like it is.
FROM amazonlinux:2023
CMD ["curl", "--version"]
Tested it with:
docker build -t curl-test .
docker run curl-test
Which produces the output:
curl 8.11.1 (aarch64-amazon-linux-gnu) libcurl/8.11.1 OpenSSL/3.2.2 zlib/1.2.11 libidn2/2.3.2 libpsl/0.21.5 nghttp2/1.59.0
Release-Date: 2024-12-11
Protocols: file ftp ftps http https ipfs ipns
Features: alt-svc AsynchDNS GSS-API HSTS HTTP2 HTTPS-proxy IDN IPv6 Kerberos Largefile libz PSL SPNEGO SSL threadsafe UnixSockets
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't need to install curl manually anymore
|
||
ENV PROXY_SERVER_HTTP_PORT=${NEPTUNE_NOTEBOOK:+9250} | ||
ENV PROXY_SERVER_HTTP_PORT=${PROXY_SERVER_HTTP_PORT:-80} | ||
ENV PROXY_SERVER_HTTPS_PORT=443 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to define the HTTPS default port here. It will be defaulted to 443 if not set explicitly
EXPOSE 9250 | ||
|
||
HEALTHCHECK --interval=30s --timeout=5s --start-period=10s --retries=3 \ | ||
CMD curl -fsSL "http://localhost:${PROXY_SERVER_HTTP_PORT}/status" || exit 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This still does not take in to account the different deployment scenarios I pointed out last time. Perhaps I did not give enough information about the issue.
Administrators of Graph Explorer have flexible deployment options. They can:
- Choose whether to host over HTTP or HTTPS with a self signed certificate
- Choose to override the default HTTP port (when using HTTP)
- Choose to override the default HTTPS port (when using HTTPS)
- Deploy to AWS Neptune Notebooks, which use Jupyter proxy to serve the site which requires HTTP, port 9250, and a base path of
/proxy/9250
This means the curl command you have above will only work in the HTTP scenario. All the other scenarios will break and report unhealthy to Docker.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the healthcheck, shall we mention the URL in the Dockerfile and also add it to .env so it can be configured?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The user can already configure it, but it is a collection of options mapped together.
I tried seeing what the LLMs would recommend, and they offered a decent approach. Please don't take this as the solution. You'll need to test this out in all the different scenarios to prove it works properly.
I'm already a bit suspect of the check against "true"
. I'm not sure if that is case insensitive, but we would need it to be.
# Dynamic healthcheck URL construction
HEALTHCHECK --interval=30s --timeout=10s --start-period=5s --retries=3 \
CMD protocol=$([ "$PROXY_SERVER_HTTPS_CONNECTION" = "true" ] && echo "https" || echo "http"); \
port=$([ "$PROXY_SERVER_HTTPS_CONNECTION" = "true" ] && echo "$PROXY_SERVER_HTTPS_PORT" || echo "$PROXY_SERVER_HTTP_PORT"); \
base_path=$([ "$NEPTUNE_NOTEBOOK" = "true" ] && echo "/proxy/9250" || echo ""); \
curl -f ${protocol}://localhost:${port}${base_path}/status || exit 1
@aravindan888 I'm sorry this particular feature ended up being so complex. It sure seemed simple on its face. I'll be on vacation for about a week, so you won't hear from me until I get back. |
Fixes #1187
Added a healthcheck to the Dockerfile by installing curl-minimal and configuring Docker to periodically check the service endpoint, ensuring the container is marked healthy only when it’s responsive, also github action wokflow for the build docker file ran successfully so its fine to merge