Add DockerHealthcheckWaitStrategy #618

kiview · 2018-03-22T16:26:44Z

Adds a DockerHealthcheckWaitStrategy that relies on the built-in Docker healthcheck.

TBH I was not sure what's our current template for creating WaitStrategy tests, so I tried to keep it as simple as possible. The write_file_and_loop.sh could surely be included into the Dockerfile CMD, but this way it seemed to be a bit easier for debugging purposes.

This PR also implicitly adds a isHealthy() method to the ContainerState interface.

Regarding backwards compatibility, I'm not sure how docker-java behaves if the health state is missing. docker-java itself documents to support it since Docker version 1.12.

kiview · 2018-03-22T16:54:21Z

OMG, some cached Gradle file locks in Travis...

bsideup · 2018-03-25T09:41:19Z

CHANGELOG.md

@@ -14,6 +14,7 @@ All notable changes to this project will be documented in this file.
 - Deprecated `WaitStrategy` and implementations in favour of classes with same names in `org.testcontainers.containers.strategy` ([\#600](https://github.com/testcontainers/testcontainers-java/pull/600))
 - Added `ContainerState` interface representing the state of a started container ([\#600](https://github.com/testcontainers/testcontainers-java/pull/600))
 - Added `WaitStrategyTarget` interface which is the target of the new `WaitStrategy` ([\#600](https://github.com/testcontainers/testcontainers-java/pull/600))
+- Added `DockerHealthcheckWaitStrategy` that is based on Docker's built-in [healthcheck](https://docs.docker.com/engine/reference/builder/#healthcheck).


PR link? :)

Of course, there was no PR when I added this line 😅

I have the same problem every time I submit a PR :D

bsideup · 2018-03-25T09:42:00Z

core/src/main/java/org/testcontainers/containers/ContainerState.java

+     */
+    default Boolean isHealthy() {
+        try {
+            return getContainerId() != null && DockerClientFactory.instance().client().inspectContainerCmd(getContainerId()).exec().getState().getHealth().getStatus().equals(STATE_HEALTHY);


could you please extract the result of DockerClientFactory.instance().client().inspectContainerCmd(getContainerId()).exec()?

Gladly, I simply copied this from the isRunning() method and thought this code was leveraging lazy evaluation of && (cargo cult 🛩).

yeah, but that was an old code and we should improve bit by bit :)

I tried to do a little refactoring here and wanted to simply use getContainerInfo(), but this seems to be implemented by the getter of the GenericContainer, which is only initially set during start in the tryStart() method. So my question, is it intended for this public API to basically return a cached version of InspectContainerResponse or am I missing something?

yes, of course. Most of getters (i.e. exposed ports) use InspectContainerResponse and if we would query Docker every time we call them that will add a significant delay

Thanks for the clarification. I've thus added the getCurrentContainerInfo() default method to the interface. I would have been fine with a private method for this, since I was mostly for avoiding code duplication, but I think it's actually useful to also have this as a public API?

bsideup · 2018-03-25T09:42:10Z

core/src/main/java/org/testcontainers/containers/ContainerState.java

+    /**
+     * @return has the container health state 'healthy'?
+     */
+    default Boolean isHealthy() {


unwrap to boolean

Same as above, isRunning() did use Boolean, so I wanted to be consistent.

bsideup · 2018-03-25T09:42:40Z

...src/main/java/org/testcontainers/containers/wait/strategy/DockerHealthcheckWaitStrategy.java

+    protected void waitUntilReady() {
+
+        try {
+            Unreliables.retryUntilTrue((int) startupTimeout.getSeconds(), TimeUnit.SECONDS, () -> waitStrategyTarget.isHealthy());


waitStrategyTarget::isHealthy ?

bsideup · 2018-03-25T09:43:33Z

...test/java/org/testcontainers/containers/wait/strategy/DockerHealthcheckWaitStrategyTest.java

+
+    @Before
+    public void setUp() {
+        // Using a Dockerfile here, since Dockerfile builder DSL doesn't support HEALTHCHECK


Maybe we should add HEALTHCHECK support to the DSL?

Definitly, but another PR I assume?

sure, up to you

bsideup · 2018-03-25T09:45:41Z

core/src/test/resources/health-wait-strategy-dockerfile/Dockerfile

@@ -0,0 +1,8 @@
+FROM alpine:latest


please pin the version

bsideup · 2018-03-25T09:52:31Z

...test/java/org/testcontainers/containers/wait/strategy/DockerHealthcheckWaitStrategyTest.java

+
+    @Test
+    public void containerStartFailsIfContainerIsUnhealthy() {
+        container.withCommand("ash");


I think this test fails because container exits immediately and not because of the healthcheck.
We should use some long-running command here and wait until healthcheck fails with "unhealthy" status

I think I looked into it, but I can check again. But yes, good idea.

rnorth

This looks like a good change to me.

Aside from a tiny comment on the test, there's one more thing I can think of: documentation. Happy to defer that until we do a general (much needed) tidy of of docs in this area, though.

rnorth · 2018-03-26T19:28:44Z

...test/java/org/testcontainers/containers/wait/strategy/DockerHealthcheckWaitStrategyTest.java

+        container = new GenericContainer(new ImageFromDockerfile()
+            .withFileFromClasspath("write_file_and_loop.sh", "health-wait-strategy-dockerfile/write_file_and_loop.sh")
+            .withFileFromClasspath("Dockerfile", "health-wait-strategy-dockerfile/Dockerfile"))
+            .waitingFor(new DockerHealthcheckWaitStrategy().withStartupTimeout(Duration.ofSeconds(3)));


Any reason not to use Wait.forHealthcheck() here? It seems like the preferable style, so might be good to use it anywhere people might look for examples.

You're right, I'll change it.

bsideup · 2018-03-26T19:43:04Z

core/src/main/java/org/testcontainers/containers/ContainerState.java

+
+        try {
+            Boolean running = getCurrentContainerInfo().getState().getRunning();
+            return running != null && running; // avoid NPE when unboxing


Boolean.TRUE.equals(running) ;)

Thx, I think it's obvious that I'm not used to working with boxed primitives :D

bsideup

@kiview approved with a comment about Boolean.TRUE

kiview · 2018-03-26T20:10:07Z

@rnorth You are right about the documentation, I'll try to come up with something.

Add DockerHealthcheckWaitStrategy and isHealthy method to ContainerState

184ca99

kiview requested review from rnorth and bsideup March 22, 2018 16:26

kiview added the type/feature label Mar 23, 2018

bsideup requested changes Mar 25, 2018

View reviewed changes

kiview added 6 commits March 26, 2018 20:25

Add PR link to changelog

5dd6229

Use COPY instead of ADD in Dockerfile for healthstrategy check

98ef3dd

Use method reference in DockerHealthcheckWaitStrategy

5dc9bd1

Use long running command in DockerHealthcheckWaitStrategyTest

e92538f

Pin Alpine image for healthcheck test to 3.7

f03659a

Refactor ContainerState

c8fcbac

rnorth requested changes Mar 26, 2018

View reviewed changes

bsideup reviewed Mar 26, 2018

View reviewed changes

bsideup approved these changes Mar 26, 2018

View reviewed changes

kiview added 2 commits March 26, 2018 22:32

Small refactorings after review

4de4253

Add documentation and example for healthcheck strategy

79cc554

rnorth approved these changes Apr 3, 2018

View reviewed changes

kiview merged commit ac1d4af into master Apr 3, 2018

kiview deleted the docker-health-wait-strategy branch April 3, 2018 11:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add DockerHealthcheckWaitStrategy #618

Add DockerHealthcheckWaitStrategy #618

kiview commented Mar 22, 2018 •

edited

Loading

kiview commented Mar 22, 2018

bsideup Mar 25, 2018

kiview Mar 25, 2018

bsideup Mar 25, 2018

bsideup Mar 25, 2018

kiview Mar 25, 2018

bsideup Mar 25, 2018

kiview Mar 26, 2018

bsideup Mar 26, 2018

kiview Mar 26, 2018

bsideup Mar 25, 2018

kiview Mar 25, 2018

bsideup Mar 25, 2018

bsideup Mar 25, 2018

kiview Mar 25, 2018

bsideup Mar 25, 2018

bsideup Mar 25, 2018

bsideup Mar 25, 2018

kiview Mar 25, 2018

rnorth left a comment

rnorth Mar 26, 2018

kiview Mar 26, 2018

bsideup Mar 26, 2018

kiview Mar 26, 2018

bsideup left a comment

kiview commented Mar 26, 2018

Add DockerHealthcheckWaitStrategy #618

Add DockerHealthcheckWaitStrategy #618

Conversation

kiview commented Mar 22, 2018 • edited Loading

kiview commented Mar 22, 2018

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

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

rnorth 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

bsideup left a comment

Choose a reason for hiding this comment

kiview commented Mar 26, 2018

kiview commented Mar 22, 2018 •

edited

Loading