Add healthy wait condition that waits until a container becomes healthy #719

danielwegener · 2017-02-17T10:41:46Z

according to its configured HEALTHCHECK (fixes #709).

Signed-off-by: Daniel Wegener daniel@wegener.me

danielwegener · 2017-02-17T10:47:43Z

The actual healtcheck test is extracted from the docker inspect container configuration and displayed in the log file.

Example output:

[INFO] ------------------------------------------------------------------------
[INFO] Building Fubibabi 7.0.0-SNAPSHOT
[INFO] ------------------------------------------------------------------------
[INFO] 
[INFO] --- docker-maven-plugin:0.19:start (default-cli) @ com.fubibabi ---
[INFO] DOCKER> [my-docker-snapshot.com/core:7.0.0-SNAPSHOT] "core": Start container fbcbb1469a6b
[INFO] DOCKER> [my-docker-snapshot.com/core:7.0.0-SNAPSHOT] "core": Waiting on healthcheck: '"CMD-SHELL", "curl -sf http://localhost:8080/core/status || exit 1"'
[INFO] DOCKER> [my-docker-snapshot.com/core:7.0.0-SNAPSHOT] "core": Waited on healthcheck '"CMD-SHELL", "curl -sf http://localhost:8080/core/status || exit 1"' 30337 ms
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 41.720 s
[INFO] Finished at: 2017-02-17T11:39:36+01:00
[INFO] Final Memory: 43M/495M
[INFO] ------------------------------------------------------------------------

or (if healthy wait is configured for a container that does not have a HEALTHCHECK configured)

INFO] ------------------------------------------------------------------------
[INFO] Building Fubibabi 7.0.0-SNAPSHOT
[INFO] ------------------------------------------------------------------------
[INFO] 
[INFO] --- docker-maven-plugin:0.19:start (default-cli) @ com.fubibabi ---

[INFO] DOCKER> [my-docker-snapshot.com/noHealthcheck:7.0.0-SNAPSHOT] "has-no-healthcheck": Start container 1364337af9b9
[ERROR] DOCKER> Error occurred during container startup, shutting down...
[INFO] DOCKER> [my-docker-snapshot.com/noHealthcheck:7.0.0-SNAPSHOT] "has-no-healthcheck": Stop and removed container 1364337af9b9 after 0 ms
[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 43.318 s
[INFO] Finished at: 2017-02-17T11:52:10+01:00
[INFO] Final Memory: 46M/501M
[INFO] ------------------------------------------------------------------------
[ERROR] Failed to execute goal io.fabric8:docker-maven-plugin:0.19:start (default-cli) on project com.fubibabi: Execution default-cli of goal io.fabric8:docker-maven-plugin:0.19-dwe:start failed: Can not wait for healthy state of [my-docker-snapshot.com/noHealthcheck:7.0.0-SNAPSHOT] "has-no-healthcheck". No HEALTHCHECK configured. -> [Help 1]
[ERROR] 
[ERROR] To see the full stack trace of the errors, re-run Maven with the -e switch.
[ERROR] Re-run Maven using the -X switch to enable full debug logging.
[ERROR] 
[ERROR] For more information about the errors and possible solutions, please read the following articles:
[ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/PluginExecutionException

codecov · 2017-02-17T10:48:54Z

Codecov Report

Merging #719 into master will decrease coverage by 0.25%.
The diff coverage is 5.12%.

@@             Coverage Diff              @@
##             master     #719      +/-   ##
============================================
- Coverage     47.92%   47.67%   -0.26%     
  Complexity     1043     1043              
============================================
  Files           130      131       +1     
  Lines          6714     6752      +38     
  Branches        872      879       +7     
============================================
+ Hits           3218     3219       +1     
- Misses         3222     3259      +37     
  Partials        274      274

Impacted Files	Coverage Δ	Complexity Δ
...ven/docker/access/hc/DockerAccessWithHcClient.java	`14.04% <ø> (ø)`	`12 <0> (ø)`	⬇️
.../fabric8/maven/docker/wait/HealthCheckChecker.java	`0% <0%> (ø)`	`0 <0> (?)`
...c/main/java/io/fabric8/maven/docker/StartMojo.java	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
...o/fabric8/maven/docker/model/ContainerDetails.java	`69.44% <0%> (-6.32%)`	`18 <0> (ø)`
...fabric8/maven/docker/config/WaitConfiguration.java	`67.07% <40%> (-2.16%)`	`8 <1> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0b20943...b9a5c9d. Read the comment docs.

danielwegener · 2017-02-17T10:57:42Z

@rhuss WDYT? Too verbose? should we really log the internally configured (and possibly longish) Healthcheck command?

…hy according to its configured HEALTHCHECK (fixes fabric8io#709). Signed-off-by: Daniel Wegener <daniel@wegener.me>

rhuss · 2017-02-17T11:23:52Z

thanks for the PR ! let me check this later, still busy a bit ...

Signed-off-by: Daniel Wegener <daniel@wegener.me>

rhuss · 2017-03-15T08:25:59Z

the PR is not forgotten, plan some integration session on the weekend ;-) ....

danielwegener · 2017-03-16T20:56:01Z

src/main/asciidoc/inc/start/_wait.adoc

+| *healthy*
+a| Check that waits until the container health state becomes `healthy`. A container is considered healthy when its <<build-healthcheck,  configured healtcheck>> succeeds.
+
+This behaviour mirros the docker compose dependsOn `condition: service_healthy`.


... or maybe better: mimics

yeah, sounds better ;-)

rhuss

looks mainly good to me, thanks a lot ! 'have some minor comments and questions, though. but no biggies, think we can resolve this soon, too. If you don't have time, I can tackle that latter this week (or the week after. still busy time ;)

rhuss · 2017-03-19T18:38:19Z

src/main/asciidoc/inc/start/_wait.adoc

+| *healthy*
+a| Check that waits until the container health state becomes `healthy`. A container is considered healthy when its <<build-healthcheck,  configured healtcheck>> succeeds.
+
+This behaviour mirros the docker compose dependsOn `condition: service_healthy`.


yeah, sounds better ;-)

rhuss · 2017-03-19T18:39:53Z

samples/healthcheck/pom.xml

@@ -39,6 +39,11 @@
                  <cmd>curl -f http://localhost/ || exit 1</cmd>
                </healthCheck>
              </build>
+              <run>
+                <wait>
+                  <healthy>true</healthy>


I wonder what <healthy>false</healthy> would imply ;-)

I know, Maven doesn't allow empty tags, so that's probably fine.

To be honest, I have no clue what particularly maven does to merge those plugin configs into mojo parameters :) I am open to any other suggestion (maybe as attribute on wait?)

It's more a complaint against Maven ;-). No, attributes are not possible at all for Maven configurations, no empty tags, too.

So its fine for me as it is now, except when we would find something 'useful' to put as value (like a timeout, but thats also available otherwise). But no worries here ...

rhuss · 2017-03-19T18:41:13Z

src/main/java/io/fabric8/maven/docker/model/InspectedContainer.java

+ * @author daniel
+ * @since 17/02/15
+ */
+public interface InspectedContainer extends Container {


What is the benefit to extend Container over simplify adding the method directly to Container ?

Please add also some javadocs to public interface methods.

ContainersListElement can not provide these properties since they are only available on the docker inspect endpoint (single container) and not the list container endpoint. To be honest, right now i can not even find the healthy field documented it in the official docker api docs, I took the path directly from the docker source. I'll try to provide the original reference asap.

Ok, understand.

rhuss · 2017-03-19T18:42:26Z

src/main/java/io/fabric8/maven/docker/model/ContainerDetails.java

+    }
+
+    @Override
+    public String healthcheckTests() {


please use standard getter naming convention (getHealthcheck())

rhuss · 2017-03-19T18:44:23Z

src/main/java/io/fabric8/maven/docker/model/ContainerDetails.java

+        if (!json.getJSONObject(CONFIG).has(HEALTHCHECK)) {
+            return null;
+        }
+        return json.getJSONObject(CONFIG).getJSONObject(HEALTHCHECK).getJSONArray(TEST).join(", ");


is CONFIG and TEST always set or might it make sense to add null checks here, too ?

Actually while looking at the code I wonder about the value to have constants and not using it literally (because they are probably only once anyway). I know, there are already constants, but that's maybe a dead end ? wdyt ?

Is there a way to detect whether the Docke daemon is too old (<1.12) so that we could prepare a proper error message ?

Good catch, docker api might leave the test field undefined if the array is empty - however an empty test does not really make sense. I'll add a precondition.

We could ask DockerAccess.getServerApiVersion before each getHealthyWaitChecker and ensure a minimal api version (happens on other places too). I wasn't sure if it would be worth the overhead (+1 roundtrip for each wait) since we are backward-compatible anyway (by not waiting if there is no health status).

I think its worth the extra round trip (maybe it could be even cached somewhere else ?), because although the plugin won't break if there are no health-check support I think its important to inform the user that the health-check doesn't work because he is running against an too-old Docker daemon.

So the user configuration can fail for two case: Either no health-check has been provided or the Docker daemon doesn't support health-check. We should report that differently. In the first case its a configuration error (doesn't make sense to wait in this case) in the later it's because a potentially supported configuration is run in an non-compatible environment.

Actually while looking at the code I wonder about the value to have constants and not using it literally (because they are probably only once anyway). I know, there are already constants, but that's maybe a dead end ? wdyt ?

There is an ancient tale about magic numbers and strings and code conventions that argue against them :) TBH. I'd be fine with explicit Strings. One level less mental indirection.

So the user configuration can fail for two case: Either no health-check has been provided or the Docker daemon doesn't support health-check. We should report that differently. In the first case its a configuration error (doesn't make sense to wait in this case) in the later it's because a potentially supported configuration is run in an non-compatible environment.

Okay. Since we require a certain server api version for this feature anyway, we could also simply check if the DockerAccessContext.getMinimalApiVersion() is configured to >=1.25.0 and advice the user to configure it explicitly otherwise (+ add this requirement to the docs). What do you think?

I just saw we could add this check simply to a WatchConfiguration.initAndValidate() much like we do for BuildImageConfiguration.initAndValidate(). This method is supposed to return a minimal API version to operate. This would result to refuse to work when a too old Docker daemon is used.

The implementation should be simple and straight forward.

BTW, I think HEALTHCHECK is supported since 1.24.

There is an ancient tale about magic numbers and strings and code conventions that argue against them :) TBH. I'd be fine with explicit Strings. One level less mental indirection.

;-) maybe we just leave it and then change it when we extract the health check off from StartMojo. Not a biggie anyway.

rhuss · 2017-03-19T18:46:41Z

src/main/java/io/fabric8/maven/docker/StartMojo.java

@@ -483,6 +488,45 @@ private String getTcpHost(WaitConfiguration.TcpConfiguration tcpConfig, Properti
        return host;
    }

+    private WaitUtil.WaitChecker getHealthyWaitChecker(final String imageConfigDesc, final DockerAccess docker, final String containerId, final List<String> logOut) {


More a note to myself: We should extract functionality out of StartMojo as it already grows quite a bit. The wait stuff could go into an extra package (without normal instead of inner classes).

Also easier to test then.

rhuss · 2017-03-19T18:52:13Z

src/main/java/io/fabric8/maven/docker/StartMojo.java

+                        return false;
+                    }
+
+                    if (first) {


Maybe its better to log out the health check itself in the constructor of the checker, so we don't need this boolean flag dance.

Can we be absolutely sure that the container already exists within the docker api when waitIfRequested is called? I am not sure if the docker create container-api gives us such guarantees (iirc i moved the healthcheck resolution into the check during development because the container could not be fetched once)

Yes, I hope we can be sure since e.g. the TcpWaitChecker also relies on it when it is created. Haven't got any bug report for this yet ;-) so it should work.

rhuss · 2017-03-19T18:53:06Z

src/main/java/io/fabric8/maven/docker/StartMojo.java

+                        if (healthcheckTests == null) {
+                            throw new IllegalArgumentException("Can not wait for healthy state of "+imageConfigDesc+". No HEALTHCHECK configured.");
+                        }
+                        log.info("%s: Waiting on healthcheck: '%s'", imageConfigDesc, healthcheckTests);


We could use log.verbose() for the full health check test adn log.info() for a summary line (and keep it short)

Yeah I wasn't sure about this one. Other checkers report their "content" (as url waits log their url). I think some context would be useful but I agree that the plain printing of the docker HEALTHCHECK line is not that beautiful.

I took log.debug (consistent with other waitCheckers). what do you think about the logOut then?

I would only use a short, fixed message for logOut and post the full health-check with log.verbose (once). That is in line with how Dockerfile instruction are only printed with verbose level when an image is build.

Signed-off-by: Daniel Wegener <daniel@wegener.me>

…en-plugin into pr/danielwegener-wip/709

danielwegener · 2017-03-27T06:45:43Z

Whoops, I guess i'll have too do a rebase asap :)

…o pr/danielwegener-wip/709

rhuss · 2017-03-29T10:31:47Z

@danielwegener np ;-) just merged in (actually I refactored the healtchecks out into dedicated classes)

rhuss · 2017-03-29T21:38:24Z

[merge]

danielwegener · 2017-03-30T10:13:14Z

Lovely! :) Thanks

rhuss · 2017-03-30T10:16:26Z

I thank you :). d-m-p would be nothing without contributions like this ;)

thanks ...

PR merged! Thanks!

Add healthy wait condition that waits until a container becomes healt…

9877a5e

…hy according to its configured HEALTHCHECK (fixes fabric8io#709). Signed-off-by: Daniel Wegener <daniel@wegener.me>

danielwegener force-pushed the wip/709 branch from b8cdfc2 to 9877a5e Compare February 17, 2017 10:59

fixup: fix healthy wait configuration in documentation example.

58cfd14

Signed-off-by: Daniel Wegener <daniel@wegener.me>

danielwegener commented Mar 16, 2017

View reviewed changes

rhuss requested changes Mar 19, 2017

View reviewed changes

rhuss added changes requested feature update changelog labels Mar 19, 2017

danielwegener and others added 2 commits March 20, 2017 00:23

fixup: code review 1

3f27fcb

Signed-off-by: Daniel Wegener <daniel@wegener.me>

Merge branch 'wip/709' of https://github.com/danielwegener/docker-mav…

0ebc408

…en-plugin into pr/danielwegener-wip/709

rhuss added 2 commits March 29, 2017 12:00

Merge branch 'master' of github.com:fabric8io/docker-maven-plugin int…

fb766ca

…o pr/danielwegener-wip/709

Re-add accidentally deleted line

79cba59

rhuss approved these changes Mar 29, 2017

View reviewed changes

rhuss added 2 commits March 29, 2017 12:33

Update changelog

75fa940

Merge branch 'master' into wip/709

b9a5c9d

fusesource-ci merged commit b38adfd into fabric8io:master Mar 29, 2017

danielwegener deleted the wip/709 branch March 30, 2017 10:04

rgbj pushed a commit to rgbj/docker-maven-plugin that referenced this pull request Jun 21, 2017

Merge pull request fabric8io#719 from danielwegener/wip/709

3d37a08

PR merged! Thanks!

Add healthy wait condition that waits until a container becomes healthy #719

Add healthy wait condition that waits until a container becomes healthy #719

Conversation

danielwegener commented Feb 17, 2017

danielwegener commented Feb 17, 2017 • edited Loading

codecov bot commented Feb 17, 2017 • edited Loading

Codecov Report

danielwegener commented Feb 17, 2017

rhuss commented Feb 17, 2017

rhuss commented Mar 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhuss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielwegener Mar 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielwegener Mar 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielwegener Mar 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielwegener commented Mar 27, 2017

rhuss commented Mar 29, 2017

rhuss commented Mar 29, 2017

danielwegener commented Mar 30, 2017

rhuss commented Mar 30, 2017

danielwegener commented Feb 17, 2017 •

edited

Loading

codecov bot commented Feb 17, 2017 •

edited

Loading

danielwegener Mar 19, 2017 •

edited

Loading

danielwegener Mar 20, 2017 •

edited

Loading

danielwegener Mar 19, 2017 •

edited

Loading