AIO-interface: add Unhealthy container state #5307

docjyJ · 2024-09-21T12:47:23Z

Add Unhealty state
Replace class by backed enum

st3iny

Hey Jean-Yves, Simon asked me to have a look at your changes. I left some feedback regarding the code itself.

I really like you refactoring the container state logic from an interface to an enum. However, I think that both concerns could have been split into separate PRs. One PR to refactor the interface and another one to implement the new healthy states. But the code is already there so why not.

Looks good otherwise but did not test.

php/src/Container/Container.php

php/src/Controller/DockerController.php

php/src/Docker/DockerActionManager.php

php/README.md

php/src/Container/IContainerState.php

docjyJ · 2024-10-01T19:24:31Z

Hi,

Thanks for the answer, as soon as I have time I'll look into it.

I will try to separate and make several PR.

Take care,

docjyJ · 2024-10-02T13:19:25Z

I keep this PR open for the unhealthy state.
See #5372 for enum

szaimen · 2024-10-08T09:03:08Z

Conflicts :/

docjyJ · 2024-10-08T09:17:33Z

Don't worry, i'll handle it.

docjyJ · 2024-10-08T09:24:15Z

Solved and up to date and ready (to be tested anyway...)

docjyJ · 2024-10-08T10:01:49Z

I have podman on my machine and I can't launch the container... I can't test it and I don't have time to debug...

szaimen · 2024-10-08T10:07:09Z

I have podman on my machine and I can't launch the container...

Why may I ask? Do you run into an issue here?

docjyJ · 2024-10-15T13:21:54Z

Another round of reviews and some feedback for you.

Thank, resolved

st3iny

Awesome! Looks good to me now!

docjyJ · 2024-10-15T13:46:23Z

Health checks may need to be adjusted, probably reduce the interval during startup to speed up startup of all containers.

start period provides initialization time for containers that need time to bootstrap. Probe failure during that period will not be counted towards the maximum number of retries. However, if a health check succeeds during the start period, the container is considered started and all consecutive failures will be counted towards the maximum number of retries.

start interval is the time between health checks during the start period. This option requires Docker Engine version 25.0 or later.

See: https://docs.docker.com/reference/dockerfile/#healthcheck

docjyJ · 2024-10-15T13:55:53Z

This should allow for better dependency management.
Can be used for helm chart, I guess.

szaimen · 2024-10-15T14:02:06Z

Thanks for the idea! However our health checks are currently built in a way that they never fail after a specific time. See for example https://github.com/nextcloud/all-in-one/blob/main/Containers/apache/healthcheck.sh. for the rest the defaults are good enough imho

docjyJ · 2024-10-15T14:28:59Z

Thanks for the idea! However our health checks are currently built in a way that they never fail after a specific time. See for example https://github.com/nextcloud/all-in-one/blob/main/Containers/apache/healthcheck.sh. for the rest the defaults are good enough imho

The problem with doing this is that docker considers the container ready... This should immediately take the container out of the starting state.

php/src/Docker/DockerActionManager.php

szaimen

Blocking, see above

szaimen · 2024-10-15T14:33:53Z

This is the only reliable way to check if the container is really up and running.

docjyJ · 2024-10-15T14:34:50Z

A fix would be to fail the container if nextcloud:9000 is not reachable and add a startup period (e.g. 10 minutes)

#!/bin/bash

- nc -z "$NEXTCLOUD_HOST" 9000 || exit 0
+ nc -z "$NEXTCLOUD_HOST" 9000 || exit 0
nc -z 127.0.0.1 8000 || exit 1
nc -z 127.0.0.1 "$APACHE_PORT" || exit 1
if ! nc -z "$NC_DOMAIN" 443; then
    echo "Could not reach $NC_DOMAIN on port 443."
    exit 1
fi

- HEALTHCHECK CMD /healthcheck.sh
+ HEALTHCHECK --start-period=10m CMD /healthcheck.sh

szaimen · 2024-10-15T14:39:22Z

A fix would be to fail the container if nextcloud:9000 is not reachable and add a startup period (e.g. 10 minutes)

This is the problem: we cannot spwcify a time here as is depending on the overall setup like installed apps and features, amount of users, given hardware and else especially during upgrades

docjyJ · 2024-10-15T14:46:29Z

This is the problem: we cannot spwcify a time here as is depending on the overall setup like installed apps and features, amount of users, given hardware and else especially during upgrades

Yes, I see...

Would it be better to manage dependencies like docker compose?

The solution for detecting the ready state of a service is to use the condition attribute with one of the following options:

service_started

service_healthy. This specifies that a dependency is expected to be “healthy”, which is defined with healthcheck, before starting a dependent service.

https://docs.docker.com/compose/how-tos/startup-order/

szaimen · 2024-10-15T15:06:42Z

I fear this is going to have the same problem afaics, no?

szaimen · 2024-10-21T08:32:42Z

@docjyJ LGTM now 😊

Can you please rebase the PR and squash the commits?
Afterwards we can merge this :)

Signed-off-by: Jean-Yves <[email protected]>

docjyJ · 2024-10-21T09:56:49Z

Done

php/src/Container/ContainerState.php

php/src/Docker/DockerActionManager.php

docjyJ · 2024-10-21T16:17:58Z

php/src/Container/ContainerState.php

+    public function isStarting(): bool {
+        return $this == self::Starting;
+    }
+
+    public function isRestarting(): bool {
+        return $this == self::Restarting;
+    }
+
+    public function isHealthy(): bool {
+        return $this == self::Healthy;
+    }
+
+    public function isUnhealthy(): bool {
+        return $this == self::Unhealthy;
+    }
+
+    public function isRunning(): bool {
+        return $this->isHealthy() || $this->isUnhealthy() || $this->isStarting() || $this->isRestarting();
+    }


The Running state corresponds to the old GetRunningContainerState state.
Maybe there is another clearer state. Healthy means that the container is running without any problem detected by the AIO.

docjyJ · 2024-10-21T16:18:56Z

php/src/Controller/DockerController.php

@@ -28,7 +30,7 @@ private function PerformRecursiveContainerStart(string $id, bool $pullImage = tr

        // Don't start if container is already running
        // This is expected to happen if a container is defined in depends_on of multiple containers
-        if ($container->GetRunningState() === ContainerState::Running) {
+        if ($container->GetContainerState()->isRunning()) {


container->GetRunningState does not include Starting and Restarting

To avoid it being hard to understand I went through the isSomething function

docjyJ · 2024-10-21T16:21:16Z

php/src/Docker/DockerActionManager.php

-        if ($responseBody['State']['Running'] === true) {
-            return ContainerState::Running;
-        } else {
-            return ContainerState::Stopped;
-        }


This is true regardless of the Healthy or Starting or Running state.

szaimen

@docjyJ I discussed the latest changes with @st3iny and we agree that the latest changes do not make much sense or at least are not easy enough to merge them. I am very sorry for that.

I would like to ask you if you could restore the changes from b20e2a8 by running git checkout <hash> and then push the old changes to a new branch via so: git checkout -b ench/noid/heathcheck-restored. From that commit hash, I think only such a check like https://github.com/nextcloud/all-in-one/pull/5307/files#diff-502ad3fbc5a2714763795f78cf314d4a76ada0cb3746530f2fb86fa471dd897bR91-R105 is missing. (best in a new commit)

Can you please do the above? I would do it myself but I unfortunately don't have your changes locally available anymore

szaimen added 3. to review Waiting for reviews enhancement New feature or request labels Sep 25, 2024

szaimen added this to the next milestone Sep 25, 2024

szaimen mentioned this pull request Sep 25, 2024

publish new images after recent PRs are merged and tested #5320

Closed

22 tasks

szaimen changed the title ~~Ench/noid/heathcheck~~ AIO: add IContainerState Sep 25, 2024

docjyJ self-assigned this Sep 28, 2024

docjyJ requested a review from szaimen September 28, 2024 18:21

st3iny reviewed Oct 1, 2024

View reviewed changes

docjyJ mentioned this pull request Oct 2, 2024

[PHP] Continuous Integration / Clean code #5368

Open

8 tasks

szaimen mentioned this pull request Oct 2, 2024

Update Configuration management #5328

Draft

docjyJ added 2. developing Work in progress blocked and removed 3. to review Waiting for reviews labels Oct 2, 2024

docjyJ marked this pull request as draft October 2, 2024 13:11

docjyJ force-pushed the ench/noid/heathcheck branch from 1f3f231 to b9ca83a Compare October 7, 2024 07:45

docjyJ changed the title ~~AIO: add IContainerState~~ AIO: add Unhealthy container state Oct 7, 2024

docjyJ mentioned this pull request Oct 7, 2024

AIO-interface: Use enum instead of interface for state #5372

Merged

docjyJ force-pushed the ench/noid/heathcheck branch from b9ca83a to 6a3c340 Compare October 8, 2024 09:23

docjyJ marked this pull request as ready for review October 8, 2024 09:24

docjyJ added 3. to review Waiting for reviews and removed 2. developing Work in progress blocked labels Oct 8, 2024

szaimen changed the title ~~AIO: add Unhealthy container state~~ AIO-interface: add Unhealthy container state Oct 8, 2024

st3iny approved these changes Oct 15, 2024

View reviewed changes

szaimen reviewed Oct 15, 2024

View reviewed changes

php/src/Docker/DockerActionManager.php Show resolved Hide resolved

szaimen requested changes Oct 15, 2024

View reviewed changes

docjyJ requested a review from szaimen October 18, 2024 21:40

szaimen mentioned this pull request Oct 18, 2024

publish new images after recent PRs are merged and tested #5427

Closed

20 tasks

docjyJ force-pushed the ench/noid/heathcheck branch from 730f4f4 to 9482e43 Compare October 21, 2024 09:18

Improuve Container States

4798489

Signed-off-by: Jean-Yves <[email protected]>

docjyJ force-pushed the ench/noid/heathcheck branch from 9482e43 to 4798489 Compare October 21, 2024 09:19

szaimen reviewed Oct 21, 2024

View reviewed changes

php/src/Container/ContainerState.php Show resolved Hide resolved

szaimen reviewed Oct 21, 2024

View reviewed changes

php/src/Docker/DockerActionManager.php Show resolved Hide resolved

docjyJ commented Oct 21, 2024

View reviewed changes

szaimen requested changes Oct 24, 2024

View reviewed changes

szaimen added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Oct 27, 2024

szaimen modified the milestones: v9.8.0, next Oct 30, 2024

szaimen marked this pull request as draft November 4, 2024 09:24

szaimen removed this from the next milestone Nov 4, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AIO-interface: add Unhealthy container state #5307

AIO-interface: add Unhealthy container state #5307

docjyJ commented Sep 21, 2024

st3iny left a comment •

edited

Loading

docjyJ commented Oct 1, 2024

docjyJ commented Oct 2, 2024

szaimen commented Oct 8, 2024

docjyJ commented Oct 8, 2024

docjyJ commented Oct 8, 2024

docjyJ commented Oct 8, 2024

szaimen commented Oct 8, 2024

docjyJ commented Oct 15, 2024

st3iny left a comment

docjyJ commented Oct 15, 2024 •

edited

Loading

docjyJ commented Oct 15, 2024

szaimen commented Oct 15, 2024 •

edited

Loading

docjyJ commented Oct 15, 2024

szaimen left a comment

szaimen commented Oct 15, 2024

docjyJ commented Oct 15, 2024

szaimen commented Oct 15, 2024 •

edited

Loading

docjyJ commented Oct 15, 2024 •

edited

Loading

szaimen commented Oct 15, 2024

szaimen commented Oct 21, 2024

docjyJ commented Oct 21, 2024

docjyJ Oct 21, 2024

docjyJ Oct 21, 2024

docjyJ Oct 21, 2024

docjyJ Oct 21, 2024

szaimen left a comment •

edited

Loading

AIO-interface: add Unhealthy container state #5307

Are you sure you want to change the base?

AIO-interface: add Unhealthy container state #5307

Conversation

docjyJ commented Sep 21, 2024

st3iny left a comment • edited Loading

Choose a reason for hiding this comment

docjyJ commented Oct 1, 2024

docjyJ commented Oct 2, 2024

szaimen commented Oct 8, 2024

docjyJ commented Oct 8, 2024

docjyJ commented Oct 8, 2024

docjyJ commented Oct 8, 2024

szaimen commented Oct 8, 2024

docjyJ commented Oct 15, 2024

st3iny left a comment

Choose a reason for hiding this comment

docjyJ commented Oct 15, 2024 • edited Loading

docjyJ commented Oct 15, 2024

szaimen commented Oct 15, 2024 • edited Loading

docjyJ commented Oct 15, 2024

szaimen left a comment

Choose a reason for hiding this comment

szaimen commented Oct 15, 2024

docjyJ commented Oct 15, 2024

szaimen commented Oct 15, 2024 • edited Loading

docjyJ commented Oct 15, 2024 • edited Loading

szaimen commented Oct 15, 2024

szaimen commented Oct 21, 2024

docjyJ commented Oct 21, 2024

docjyJ Oct 21, 2024

Choose a reason for hiding this comment

docjyJ Oct 21, 2024

Choose a reason for hiding this comment

docjyJ Oct 21, 2024

Choose a reason for hiding this comment

docjyJ Oct 21, 2024

Choose a reason for hiding this comment

szaimen left a comment • edited Loading

Choose a reason for hiding this comment

st3iny left a comment •

edited

Loading

docjyJ commented Oct 15, 2024 •

edited

Loading

szaimen commented Oct 15, 2024 •

edited

Loading

szaimen commented Oct 15, 2024 •

edited

Loading

docjyJ commented Oct 15, 2024 •

edited

Loading

szaimen left a comment •

edited

Loading