feat: add dms-healthcheck script for container health checks - #4706

improved per suggestion

The line break seems unnecessary to me.

Do we really want/need 3 retries?

@casperklein

The line break seems unnecessary to me.

Personally I prefer the distinction/separation if there's all those HEALTHCHECK options defined, and that's similar to what is shown in the HEALTHCHECK docs example.

Given feedback with suggestion below, I do agree that it's not worthwhile and we can collapse it back to a single line 👍

Do we really want/need 3 retries?

The default retries is already 3, as is the interval of 30s.

If anything I'd raise more concern with --interval=30s as that is how long we're delaying the first health check which affects any containers using depends_on with the health status of DMS.

Retries being more than 1 is absolutely valid, if the health check is successful at the first attempt, the container is marked healthy, but if it failed, you may still want to permit checking again after a moment in the event the healthcheck was run when a service was restarted (and recovered), not immediately assume failure.

The --timeout shouldn't need to be lowered (default 30s), on the basis that we don't expect any part of the script to hang, that SIGTERM should be sent any earlier. I'd rather introduce a lower timeout as a response to a real report where investigation is unable to resolve what causes the hang to occur.

DMS is up and ready by default in less than 5s on my system, a bit longer on a budget VPS instance, and some other services when enabled (like Rspamd IIRC which is intended to become default in future) can slow that down (Amavis adds some delay too IIRC).

If we want to assume that DMS should be up within 10-15s we could use an interval of 5s, or with other services enabled, give some additional slack. As this is the Dockerfile healthcheck (also applies for Docker Compose), we have additional options to configure (defaults: --start-period=0s + --start-interval=5s) so that the polling interval is shorter only during the initial container startup (duration window defined by --start-period). Retry failures do not count during the start period, but that duration is cut short as soon as as a healthcheck is successful (such that if a failure then occurs and subsequent healthchecks fail, even if they were within the declared start period window, they'd no longer be ignored).

The 5s --start-interval default is sufficient for DMS, so just raise --start-period

I couldn't imagine a case, where the health check only succeed on the 3rd attempt. That's why I questioned the retry amount. We can leave it as it is..

--start-period=30 seems like a good addition 👍

Edit:

HEALTHCHECK --interval=30s --timeout=5s --retries=3

This gives the container 1:30min to start, before becoming unhealthy.

I couldn't imagine a case, where the health check only succeed on the 3rd attempt.

First time starting the container and setting up a user? For someone new to it, that can take more than 30s depending how they go about it.

For me it was when I was setting up automated test examples with multiple containers, setting a lower interval that is too aggressive might fail the first try depending on deployment environment. Thankfully I came across --start-period which resolved that concern completely.

HEALTHCHECK --interval=30s --timeout=5s --retries=3

This gives the container 1:30min to start, before becoming unhealthy.

Technically during a container startup the status would be starting (which we can still consider as unhealthy, at least a depends_on would), and if no check passes, then we stay in that unhealthy state until officially switching to an unhealthy status at 90sec uptime, yes.

We resolved that concern for startup healthchecks via a start period and shorter start interval (which is immune to --retries within the start period) 👍

Thus the starting status can switch to a healthy status much sooner.

• While a unhealthy status still takes 90s (30s start period + 30s x2 interval checks. Exhausting the start period seems to count as 1 failure, we then wait --interval until the next check is performed which fails and so on).
• If you need to assume failure earlier, you can override these healthcheck params, or have your own logic in place when healthy has not been reported after X time.

For actual runtime, the first healthcheck could fail for various reasons (a single check on a port being bound, not so much, but this PR covers a variety of services), such as:

• A service may not report a RUNNING status if it just crashed for example but it could recover shortly after.
• Intermittent connectivity (perhaps using Fetchmail/Getmail and that fails to connect to an external mail server? It'll try again and the connection may succeed).

30s or 90s before your monitoring tools can signal the unhealthy status isn't that much of a difference for your ability to respond, but it does help ensure we avoid false positives wasting your time to respond. Retries are important, and given the types of failures, it's also important that the retries are given enough padding.

Container startup time is a totally different scenario and generally more predictable (but as mentioned depending on context, there can be some where this will take longer).

To have all health checks in one place, we could also add the port check here.

The port 25 check was left in a comment as a TODO, should we actually find a need/benefit in having that check. Presently AFAIK it's just an assumption, and it'd be nicer to know a scenario where DMS has Postfix reporting RUNNING but isn't listening on port 25 when it's expected to.

In that same vein we could also query other ports on other services, but even then a bound port for a service to listen on doesn't equate to a working service port 😅

If you do insist on including it, I won't challenge that but we could express the conditions in a shell group { ... } with a single || exit 1 instead? (to ensure any non-zero status is coalesced into 1):

For me at least, stacking a bunch of if statements and inverting the condition to trigger exit 1 in each seemed like a code smell 🤔

IMO: A service check (e.g. sending a mail) > port check > supervisor status

Replacing the port check with supervisor status seems like a little regression to me.

In that same vein we could also query other ports on other services

Absolutely. But my take on this PR was to improve things a bit and not to make them 100% bullet proof and give OP additional homework 😉

For me at least, stacking a bunch of if statements and inverting the condition to trigger exit 1 in each seemed like a code smell 🤔

I agree that we disagree 😆
Compared to multiple conditions spread across multiple lines, that are glued together with && \ and using an additional sub-shell, I find my proposal in that case much more readable and explicit.

That said, I am fine with the current state of this PR, weather the port check is included or not.
And if it's included, I am fine with both variants 👍

IMO: A service check (e.g. sending a mail) > port check > supervisor status

Replacing the port check with supervisor status seems like a little regression to me.

I think that depends on perspective?

For example your checking port 25 as your healthcheck, but sending a mail from DMS would use an outbound port 25, which has nothing to do with that, whilst the listening port for mail submission is also 587 or 465.

You may also just want to use DMS with Fetchmail/Getmail and Dovecot for a test, where mail can be retrieved from a remote mail server, and then delivered via LMTP, or other means that do not depend on Postfix listening to port 25 (or Postfix at all for that matter). A scenario like has different expectations for what a healthy/ready DMS container means.

If Postfix is healthy, there's a good chance it's already listening on port 25. I've asked for what scenarios that is not the case, but haven't yet had a clear example cited?

NOTE: Presently, you would be correct however that supervisor status here is a regression. Until a follow-up PR addresses the startsecs=0 concern I raised with Supervisord (services instantly are treated as RUNNING state due to this, even when they're in a failure loop, it also prevents a service being marked as unhealthy as retry limits only apply to failure transitions from states that are not RUNNING which resets the retry count).

In that same vein we could also query other ports on other services

Absolutely. But my take on this PR was to improve things a bit and not to make them 100% bullet proof

I think there's a misunderstanding here. I was trying to highlight that it'd be adding complexity for little to no gain for our project and users.

I would rather we add port checks only when we have evidence of scenarios that cause that, so that we don't just carry redundant checks for the sake of it.

If we carry the port checks, extra work should be done for communicating that kind of failure, rather than just reporting an unhealthy status, as that may not lead to maintainers being made aware of a problem causing it, especially if the fix is just to have a restart policy when the container is unhealthy?

If instead it were marked healthy but had a problem with a port, it's far more likely a user would report this to us, or PR a port check into the healthcheck script, which comes with valuable context that it's a known issue, and presumably one we've not been able to troubleshoot/resolve the cause of it.

and give OP additional homework 😉

I don't think that is necessary 🤨

For me at least, stacking a bunch of if statements and inverting the condition to trigger exit 1 in each seemed like a code smell 🤔

I agree that we disagree 😆
Compared to multiple conditions spread across multiple lines, that are glued together with && \ and using an additional sub-shell, I find my proposal in that case much more readable and explicit.

That's fair, but the proposal I gave still resulted in the same conditions that you'd have wrapped with if [[ ... ]];then + exit 1 + fi. If that's all that is done, it's just extra noise to communicate "N conditions (any true) => exit 1", which can really add up as more conditions are accumulated.

You're still going to see those same condition regardless if they're wrapped in several lines of syntax noise, or if each check is co-located right next to each other and scoped to individual lines (which the subshell was intended to help convey, pragmatically speaking in this context there's no notably overhead in doing so?).

You know me, I tend to prefer verbosity (long options, detailed commentary inline docs for context, etc), but in this case my mental model (monotropic / gestalt thinking) is biased towards expressing the logic with a clear encapsulated pattern 😅 (which I appreciate as an explicit flow to grok for readability)

That said, abstractions can add complexity, so as the author perhaps I'm blinded by bias 😓 (I did spend some time trying to see it your way too... but if the conditions themselves were replaced with _all_enabled_services_are_running && _is_port_25_listening, I don't see an advantage with not being DRY here?)

That said, I am fine with the current state of this PR, weather the port check is included or not.
And if it's included, I am fine with both variants 👍

Sweet!

I'd prefer to avoid bike shedding and just defer implementation to the existing TODO comment which requests that we first have proper justification for the port 25 health check (or any other service ports).

If Postfix is healthy, there's a good chance it's already listening on port 25.
I've asked for what scenarios that is not the case, but haven't yet had a clear example cited?

I can't imagine a scenario too. This was just to be safe in the future, when something unexpected happens.

The line break seems unnecessary to me.

Do we really want/need 3 retries?