-
-
Notifications
You must be signed in to change notification settings - Fork 2k
chore: Simplify compose.yaml
healthcheck
#4498
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
Conversation
Curl uses a lot of different return codes. The I don't like the new approach, because it pollutes the
This might also introduce problems with fail2ban. IIRC using mode Edit: To achieve a cleaner output, we could use: |
Where is curl involved here? Isn't the exit status coming from the
I was not aware of that and you raise very valid points, thanks for pointing that out! 🙏
Is there value in that output being stored repeated in the healthcheck log? Shouldn't we just use
|
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.
Verified this check adjusted to interval: 1s
:
$ docker inspect --format='{{json .State.Health}}' dms | jq
{
"Status": "healthy",
"FailingStreak": 0,
"Log": [
{
"Start": "2025-06-01T21:57:33.747428151Z",
"End": "2025-06-01T21:57:33.827584046Z",
"ExitCode": 1,
"Output": ""
},
{
"Start": "2025-06-01T21:57:34.828241261Z",
"End": "2025-06-01T21:57:34.919379591Z",
"ExitCode": 1,
"Output": ""
},
{
"Start": "2025-06-01T21:57:35.919784141Z",
"End": "2025-06-01T21:57:35.965090221Z",
"ExitCode": 0,
"Output": ""
},
{
"Start": "2025-06-01T21:57:36.965467119Z",
"End": "2025-06-01T21:57:37.012927453Z",
"ExitCode": 0,
"Output": ""
},
{
"Start": "2025-06-01T21:57:38.013727139Z",
"End": "2025-06-01T21:57:38.059783409Z",
"ExitCode": 0,
"Output": ""
}
]
}
NOTE: Only the last 5 checks are stored in the log output, hence the reduced interval to confirm.
My bad. I was looking at another healthcheck example while writing. It was however just to explain the
That will work. Only disadvantage: when grep fails (return code 2) for whatever reason, it will happen silently. |
compose.yaml
Outdated
# - NET_ADMIN | ||
healthcheck: | ||
test: "ss --listening --tcp | grep -P 'LISTEN.+:smtp' || exit 1" | ||
test: "ss --listening --tcp | grep --silent ':smtp'" |
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.
Afaik our postfix listens only on IPv4. Therefore we could make it more explicit with:
test: "ss --listening --tcp | grep --silent ':smtp'" | |
test: "ss --listening --ipv4 --tcp | grep --silent ':smtp'" |
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'm not sure if we should? If you had an IPv6 only container (I think this is possible in Docker now at least), the healthcheck could be agnostic to that.
DMS itself though does have a fair bit of 127.0.0.1
hard-coded I think instead of localhost
, so it may not support IPv6 only out of the box 😅
If there is a good reason to specifically filter to IPv4 only though, we could do that. But I don't think there's any issues with also detecting listening on IPv6?
I assume the exit status of If grep is given an invalid arg or invalid filepath to search you still get an exit status of I suppose we could keep https://docs.docker.com/reference/dockerfile/#healthcheck
|
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.
Adding --ipv4
as suggested by @casperklein , should someone bring up IPv6 only DMS, we can discuss dropping it then 🤔
Adding back || exit 1
for broader compatibility. Some errors encountered by grep
still return a 2
despite --silent
/--quiet
option being set.
Description
The current example healthcheck
ss --listening --tcp | grep -P 'LISTEN.+:smtp' || exit 1
can be simplified tonc -z localhost 25
(our tests use this via a helper to wait on Postfix being ready), which effectively does the same check (that a service is listening on the port), but without the extra output or need to set an exit status (both of which could be resolved with grep-q
).The
nc
command is a bit more terser and direct though. The port25
can be alternatively substituted for the service portsmtp
, (nc -z localhost smtp
) just like thess
output displays (resolved viagrep smtp /etc/services
), but I figured most would be more familiar with the port number itself.This has a benefit of less noisy healthcheck logs, which currently looks like this every 30 secs:
With
nc
instead:Type of change