Treat unhealthy docker container as down

[vfosnar]

• I have read and understand the pull request rules.

Description

Fixes #4369

Type of change

Please delete any options that are not relevant.

• Bug fix (non-breaking change which fixes an issue)
  or
• Breaking change (a fix or feature that would cause existing functionality to not work as expected)

Checklist

• My code follows the style guidelines of this project
• I ran ESLint and other linters for modified files
• I have performed a self-review of my own code and tested it
• I have commented my code, particularly in hard-to-understand areas (including JSDoc for methods)
• My changes generates no new warnings
• My code needed automated testing. I have added them (this is optional task)

[nougad]

Thank you @vfosnar for merging my commit. The change looks good to me. I'm surprised I completely missed that bug when I made my change. 🙈

My only suggestion would be to make DOWN the default (in the else branch):

• starting -> PENDING (starting is according to 3 the third possible state: https://docs.docker.com/engine/reference/builder/#healthcheck)
• healthy or empty string for podman -> UP
• unhealthy and everything else -> DOWN

That way any unknown input would result in DOWN instead of showing it as UP. But that's just a suggestion and not a blocker.

[chakflying]

We may also need to increment retries in case it's stuck in starting.

[vfosnar]

This should (don't count on that) be okay because if you have a healthcheck defined the daemon will respond with "unhealthy"

[chakflying]

I haven't tested this, I'm just wondering if you set a container to automatically restart, but it keeps failing, would it be shown as always "starting"

[vfosnar]

nvm you're right, it will get stuck even with a healthcheck

[vfosnar]

But I'm not sure if simply incrementing retries every time is the best way to handle this. We should respect the start period in which we won't increment them. No idea how to get data on that from the daemon tho.

I propose we get this pr merged and start a seperate issue.

[vfosnar]

or nah, I will just add the counting, better false positive than false negative

[CommanderStorm]

Should be possible via listing containers with filters

[vfosnar]

I think the best way to handle this is to treat the monitor as DOWN if the container is in the "starting" state, as it basically is down for the end user.

This PR is ready to be reviewed.

[CommanderStorm]

I have not tested the changes, but they seem way cleaner than the code before. 👍🏻

Could you rebase the changes onto the 1.23.X-branch instead?
This would reduce our effort involved in making this part of v1.23.12

[louislam]

Making starting as DOWN is questionable.

I take Uptime Kuma's Docker image as an example:

uptime-kuma/docker/dockerfile

Line 85 in 36196f6

HEALTHCHECK --interval=60s --timeout=30s --start-period=180s --retries=5 CMD extra/healthcheck

I set the starting period to 180s to make sure the server is fully started, in order to avoid "bootloop".

[vfosnar]

Yeah but even if it's starting it is not responding to user's request and that's in my opinion something we should care and notify about

[louislam]

As it is changing the behaviour of this monitor type, I think it won't be merged into 1.23.X.

And I think it needs an alternative method to deal the issue. Making starting as DOWN is not necessarily correct in my cases, because in the running state, the service could be ready or not ready.

Maybe Uptime Kuma should also add something like --start-period to avoid the issue.

[up2-date]

The problem still exists in current versions (even in the 2.0 beta). Is there a plan to fix this problem?

[rmatte]

Still a problem in the current version. Saw this today and missed a major issue for longer than I should have because of it (only eventually noticed because it was my DNS service that was down, but would have been useful to know right away).