Add DockerHealthcheckWaitStrategy #618
Conversation
|
OMG, some cached Gradle file locks in Travis... |
CHANGELOG.md
Outdated
| - 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). |
There was a problem hiding this comment.
Of course, there was no PR when I added this line 😅
There was a problem hiding this comment.
I have the same problem every time I submit a PR :D
| */ | ||
| default Boolean isHealthy() { | ||
| try { | ||
| return getContainerId() != null && DockerClientFactory.instance().client().inspectContainerCmd(getContainerId()).exec().getState().getHealth().getStatus().equals(STATE_HEALTHY); |
There was a problem hiding this comment.
could you please extract the result of DockerClientFactory.instance().client().inspectContainerCmd(getContainerId()).exec()?
There was a problem hiding this comment.
Gladly, I simply copied this from the isRunning() method and thought this code was leveraging lazy evaluation of && (cargo cult 🛩).
There was a problem hiding this comment.
yeah, but that was an old code and we should improve bit by bit :)
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
| /** | ||
| * @return has the container health state 'healthy'? | ||
| */ | ||
| default Boolean isHealthy() { |
There was a problem hiding this comment.
Same as above, isRunning() did use Boolean, so I wanted to be consistent.
| protected void waitUntilReady() { | ||
|
|
||
| try { | ||
| Unreliables.retryUntilTrue((int) startupTimeout.getSeconds(), TimeUnit.SECONDS, () -> waitStrategyTarget.isHealthy()); |
There was a problem hiding this comment.
waitStrategyTarget::isHealthy ?
|
|
||
| @Before | ||
| public void setUp() { | ||
| // Using a Dockerfile here, since Dockerfile builder DSL doesn't support HEALTHCHECK |
There was a problem hiding this comment.
Maybe we should add HEALTHCHECK support to the DSL?
There was a problem hiding this comment.
Definitly, but another PR I assume?
| @@ -0,0 +1,8 @@ | |||
| FROM alpine:latest | |||
|
|
||
| @Test | ||
| public void containerStartFailsIfContainerIsUnhealthy() { | ||
| container.withCommand("ash"); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
I think I looked into it, but I can check again. But yes, good idea.
rnorth
left a comment
rnorth
left a comment
There was a problem hiding this comment.
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.
| 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))); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
You're right, I'll change it.
|
|
||
| try { | ||
| Boolean running = getCurrentContainerInfo().getState().getRunning(); | ||
| return running != null && running; // avoid NPE when unboxing |
There was a problem hiding this comment.
Boolean.TRUE.equals(running) ;)
There was a problem hiding this comment.
Thx, I think it's obvious that I'm not used to working with boxed primitives :D
|
@rnorth You are right about the documentation, I'll try to come up with something. |
Adds a DockerHealthcheckWaitStrategy that relies on the built-in Docker healthcheck.
TBH I was not sure what's our current template for creating
WaitStrategytests, so I tried to keep it as simple as possible. Thewrite_file_and_loop.shcould surely be included into the DockerfileCMD, but this way it seemed to be a bit easier for debugging purposes.This PR also implicitly adds a
isHealthy()method to theContainerStateinterface.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.