Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add DockerHealthcheckWaitStrategy #618

Merged
merged 9 commits into from
Apr 3, 2018
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR link? :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


## [1.6.0] - 2018-01-28

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@

public interface ContainerState {

String STATE_HEALTHY = "healthy";

/**
* Get the IP address that this container may be reached on (may not be the local machine).
*
Expand All @@ -35,6 +37,17 @@ default Boolean isRunning() {
}
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unwrap to boolean

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

try {
return getContainerId() != null && DockerClientFactory.instance().client().inspectContainerCmd(getContainerId()).exec().getState().getHealth().getStatus().equals(STATE_HEALTHY);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

} catch (DockerException e) {
return false;
}
}

/**
* Get the actual mapped port for a first port exposed by the container.
*
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
package org.testcontainers.containers.wait.strategy;

import org.rnorth.ducttape.TimeoutException;
import org.rnorth.ducttape.unreliables.Unreliables;
import org.testcontainers.containers.ContainerLaunchException;

import java.util.concurrent.TimeUnit;

/**
* Wait strategy leveraging Docker's built-in healthcheck mechanism.
*
* @see <a href="https://docs.docker.com/engine/reference/builder/#healthcheck">https://docs.docker.com/engine/reference/builder/#healthcheck</a>
*/
public class DockerHealthcheckWaitStrategy extends AbstractWaitStrategy {

@Override
protected void waitUntilReady() {

try {
Unreliables.retryUntilTrue((int) startupTimeout.getSeconds(), TimeUnit.SECONDS, () -> waitStrategyTarget.isHealthy());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

waitStrategyTarget::isHealthy ?

} catch (TimeoutException e) {
throw new ContainerLaunchException("Timed out waiting for container to become healthy");
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -61,4 +61,13 @@ public static HttpWaitStrategy forHttps(String path) {
public static LogMessageWaitStrategy forLogMessage(String regex, int times) {
return new LogMessageWaitStrategy().withRegEx(regex).withTimes(times);
}

/**
* Convenience method to return a WaitStrategy leveraging Docker's built-in healthcheck.
*
* @return DockerHealthcheckWaitStrategy
*/
public static DockerHealthcheckWaitStrategy forHealthcheck() {
return new DockerHealthcheckWaitStrategy();
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
package org.testcontainers.containers.wait.strategy;

import org.junit.Before;
import org.junit.Test;
import org.testcontainers.containers.ContainerLaunchException;
import org.testcontainers.containers.GenericContainer;
import org.testcontainers.images.builder.ImageFromDockerfile;

import java.time.Duration;

import static org.rnorth.visibleassertions.VisibleAssertions.assertThrows;

public class DockerHealthcheckWaitStrategyTest {

private GenericContainer container;

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should add HEALTHCHECK support to the DSL?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitly, but another PR I assume?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure, up to you

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)));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, I'll change it.

}

@Test
public void startsOnceHealthy() {
container.start();
}

@Test
public void containerStartFailsIfContainerIsUnhealthy() {
container.withCommand("ash");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

assertThrows("Container launch fails when unhealthy", ContainerLaunchException.class, container::start);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
FROM alpine:latest
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please pin the version


HEALTHCHECK --interval=1s CMD test -e /testfile

ADD write_file_and_loop.sh write_file_and_loop.sh
RUN chmod +x write_file_and_loop.sh

CMD ["/write_file_and_loop.sh"]
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
#!/bin/ash

echo sleeping
sleep 2
echo writing file
touch /testfile

while true; do sleep 1; done