-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
When starting a container fails, don't wait until the full timeout; #115
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,6 +2,7 @@ | |
|
|
||
| import com.github.dockerjava.api.DockerClient; | ||
| import com.github.dockerjava.api.DockerException; | ||
| import com.github.dockerjava.api.NotFoundException; | ||
| import com.github.dockerjava.api.async.ResultCallback; | ||
| import com.github.dockerjava.api.command.CreateContainerCmd; | ||
| import com.github.dockerjava.api.command.InspectContainerResponse; | ||
|
|
@@ -25,14 +26,19 @@ | |
| import org.testcontainers.containers.output.OutputFrame; | ||
| import org.testcontainers.containers.traits.LinkableContainer; | ||
| import org.testcontainers.images.RemoteDockerImage; | ||
| import org.testcontainers.utility.*; | ||
| import org.testcontainers.utility.ContainerReaper; | ||
| import org.testcontainers.utility.DockerLoggerFactory; | ||
| import org.testcontainers.utility.DockerMachineClient; | ||
| import org.testcontainers.utility.DockerStatus; | ||
| import org.testcontainers.utility.PathOperations; | ||
|
|
||
| import java.io.File; | ||
| import java.io.IOException; | ||
| import java.net.Socket; | ||
| import java.net.URL; | ||
| import java.nio.file.Path; | ||
| import java.time.Duration; | ||
| import java.time.Instant; | ||
| import java.util.*; | ||
| import java.util.concurrent.Future; | ||
| import java.util.concurrent.TimeUnit; | ||
|
|
@@ -52,6 +58,9 @@ | |
| public class GenericContainer extends FailureDetectingExternalResource implements LinkableContainer { | ||
|
|
||
| public static final int STARTUP_RETRY_COUNT = 3; | ||
| public static final int CONTAINER_RUNNING_TIMEOUT_SEC = 30; | ||
|
|
||
|
|
||
| /* | ||
| * Default settings | ||
| */ | ||
|
|
@@ -79,6 +88,9 @@ public class GenericContainer extends FailureDetectingExternalResource implement | |
| @NonNull | ||
| private Duration startupTimeout = Duration.ofSeconds(60); | ||
|
|
||
| @NonNull | ||
| private Duration minimumRunningDuration = null; | ||
|
|
||
| /* | ||
| * Unique instance of DockerClient for use by this container object. | ||
| */ | ||
|
|
@@ -137,7 +149,7 @@ public void start() { | |
| } | ||
| } | ||
|
|
||
| private void tryStart(Profiler profiler) { | ||
| private void tryStart(Profiler profiler) { | ||
| try { | ||
| String dockerImageName = image.get(); | ||
| logger().debug("Starting container: {}", dockerImageName); | ||
|
|
@@ -163,21 +175,40 @@ private void tryStart(Profiler profiler) { | |
| containerIsStarting(containerInfo); | ||
|
|
||
| // Wait until the container is running (may not be fully started) | ||
| profiler.start("Wait until container state=running"); | ||
| Unreliables.retryUntilTrue(30, TimeUnit.SECONDS, () -> { | ||
| profiler.start("Wait until container state=running, or there's evidence it failed to start."); | ||
| final Boolean[] startedOK = {null}; | ||
| Unreliables.retryUntilTrue(CONTAINER_RUNNING_TIMEOUT_SEC, TimeUnit.SECONDS, () -> { | ||
| //noinspection CodeBlock2Expr | ||
| return DOCKER_CLIENT_RATE_LIMITER.getWhenReady(() -> { | ||
| // record "now" before fetching status; otherwise the time to fetch the status | ||
| // will contribute to how long the container has been running. | ||
| Instant now = Instant.now(); | ||
| InspectContainerResponse inspectionResponse = dockerClient.inspectContainerCmd(containerId).exec(); | ||
| return inspectionResponse.getState().isRunning(); | ||
|
|
||
| if (DockerStatus.isContainerRunning(inspectionResponse.getState(), | ||
| minimumRunningDuration, | ||
| now)) { | ||
| startedOK[0] = true; | ||
| return true; | ||
| } else if (DockerStatus.isContainerStopped(inspectionResponse.getState())) { | ||
| startedOK[0] = false; | ||
| return true; | ||
| } | ||
| return false; | ||
| }); | ||
| }); | ||
|
|
||
| if (!startedOK[0]) { | ||
| // Bail out, don't wait for the port to start listening. | ||
| // (Exception thrown here will be caught below and wrapped) | ||
| throw new NotFoundException("Container has already stopped."); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Whatever we throw here will be caught below and wrapped in a ContainerLaunchException. I thought having a ContainerLaunchException wrapped in a ContainerLaunchException would be a bit silly.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I feel slightly funny about using a client lib's exception class - I think I'll amend to a boring-but-neutral |
||
| } | ||
|
|
||
| profiler.start("Wait until container started"); | ||
| waitUntilContainerStarted(); | ||
|
|
||
| logger().info("Container {} started", dockerImageName); | ||
| containerIsStarted(containerInfo); | ||
|
|
||
| } catch (Exception e) { | ||
| logger().error("Could not start container", e); | ||
|
|
||
|
|
@@ -491,6 +522,16 @@ public String getContainerIpAddress() { | |
| return DockerClientFactory.instance().dockerHostIpAddress(); | ||
| } | ||
|
|
||
| /** | ||
| * Only consider a container to have successfully started if it has been running for this duration. The default | ||
| * value is null; if that's the value, ignore this check. | ||
| */ | ||
|
|
||
| public GenericContainer withMinimumRunningDuration(Duration minimumRunningDuration) { | ||
| this.setMinimumRunningDuration(minimumRunningDuration); | ||
| return this; | ||
| } | ||
|
|
||
| /** | ||
| * Get the IP address that this container may be reached on (may not be the local machine). | ||
| * | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,82 @@ | ||
| package org.testcontainers.utility; | ||
|
|
||
| import com.github.dockerjava.api.command.InspectContainerResponse; | ||
|
|
||
| import java.time.Duration; | ||
| import java.time.Instant; | ||
| import java.time.format.DateTimeFormatter; | ||
|
|
||
| /** | ||
| * Utility functions for dealing with docker status based on the information available to us, and trying to be | ||
| * defensive. | ||
| * <p> | ||
| * <p>In docker-java version 2.2.0, which we're using, only these | ||
| * fields are available in the container state returned from Docker Inspect: "isRunning", "isPaused", "startedAt", and | ||
| * "finishedAt". There are states that can occur (including "created", "OOMkilled" and "dead") that aren't directly | ||
| * shown through this result. | ||
| * <p> | ||
| * <p>Docker also doesn't seem to use null values for timestamps; see DOCKER_TIMESTAMP_ZERO, below. | ||
| */ | ||
| public class DockerStatus { | ||
|
|
||
| /** | ||
| * When the docker client has an "empty" timestamp, it returns this special value, rather than | ||
| * null or an empty string. | ||
| */ | ||
| static final String DOCKER_TIMESTAMP_ZERO = "0001-01-01T00:00:00Z"; | ||
|
|
||
| /** | ||
| * Based on this status, is this container running, and has it been doing so for the specified amount of time? | ||
| * | ||
| * @param state the state provided by InspectContainer | ||
| * @param minimumRunningDuration minimum duration to consider this as "solidly" running, or null | ||
| * @param now the time to consider as the current time | ||
| * @return true if we can conclude that the container is running, false otherwise | ||
| */ | ||
| public static boolean isContainerRunning(InspectContainerResponse.ContainerState state, | ||
| Duration minimumRunningDuration, | ||
| Instant now) { | ||
| if (state.isRunning()) { | ||
| if (minimumRunningDuration == null) { | ||
| return true; | ||
| } | ||
| Instant startedAt = DateTimeFormatter.ISO_INSTANT.parse( | ||
| state.getStartedAt(), Instant::from); | ||
|
|
||
| if (startedAt.isBefore(now.minus(minimumRunningDuration))) { | ||
| return true; | ||
| } | ||
| } | ||
| return false; | ||
| } | ||
|
|
||
| /** | ||
| * Based on this status, has the container halted? | ||
| * | ||
| * @param state the state provided by InspectContainer | ||
| * @return true if we can conclude that the container has started but is now stopped, false otherwise. | ||
| */ | ||
| public static boolean isContainerStopped(InspectContainerResponse.ContainerState state) { | ||
|
|
||
| // get some preconditions out of the way | ||
| if (state.isRunning() || state.isPaused()) { | ||
| return false; | ||
| } | ||
|
|
||
| // if the finished timestamp is non-empty, that means the container started and finished. | ||
| if (!isDockerTimestampEmpty(state.getStartedAt()) && !isDockerTimestampEmpty(state.getFinishedAt())) { | ||
| return true; | ||
| } | ||
| return false; | ||
| } | ||
|
|
||
| public static boolean isDockerTimestampEmpty(String dockerTimestamp) { | ||
| // This is a defensive approach. Current versions of Docker use the DOCKER_TIMESTAMP_ZERO value, but | ||
| // that could change. | ||
| return dockerTimestamp == null | ||
| || dockerTimestamp.isEmpty() | ||
| || dockerTimestamp.equals(DOCKER_TIMESTAMP_ZERO) | ||
| || DateTimeFormatter.ISO_INSTANT.parse(dockerTimestamp, Instant::from).getEpochSecond() < 0L; | ||
| } | ||
|
|
||
| } |
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,84 @@ | ||
| package org.testcontainers.utility; | ||
|
|
||
| import static org.junit.Assert.assertFalse; | ||
| import static org.junit.Assert.assertTrue; | ||
| import static org.mockito.Mockito.when; | ||
|
|
||
| import com.github.dockerjava.api.command.InspectContainerResponse; | ||
| import org.junit.Test; | ||
| import org.mockito.Mockito; | ||
|
|
||
| import java.time.Duration; | ||
| import java.time.Instant; | ||
| import java.time.format.DateTimeFormatter; | ||
|
|
||
| /** | ||
| * | ||
| */ | ||
| public class DockerStatusTest { | ||
|
|
||
| private static Instant now = Instant.now(); | ||
|
|
||
| private static InspectContainerResponse.ContainerState running = | ||
| buildState(true, false, buildTimestamp(now.minusMillis(30)), DockerStatus.DOCKER_TIMESTAMP_ZERO); | ||
|
|
||
| private static InspectContainerResponse.ContainerState runningVariant = | ||
| buildState(true, false, buildTimestamp(now.minusMillis(30)), ""); | ||
|
|
||
| private static InspectContainerResponse.ContainerState shortRunning = | ||
| buildState(true, false, buildTimestamp(now.minusMillis(10)), DockerStatus.DOCKER_TIMESTAMP_ZERO); | ||
|
|
||
| private static InspectContainerResponse.ContainerState created = | ||
| buildState(false, false, DockerStatus.DOCKER_TIMESTAMP_ZERO, DockerStatus.DOCKER_TIMESTAMP_ZERO); | ||
|
|
||
| // a container in the "created" state is not running, and has both startedAt and finishedAt empty. | ||
| private static InspectContainerResponse.ContainerState createdVariant = | ||
| buildState(false, false, null, null); | ||
|
|
||
| private static InspectContainerResponse.ContainerState exited = | ||
| buildState(false, false, buildTimestamp(now.minusMillis(100)), buildTimestamp(now.minusMillis(90))); | ||
|
|
||
| private static InspectContainerResponse.ContainerState paused = | ||
| buildState(false, true, buildTimestamp(now.minusMillis(100)), DockerStatus.DOCKER_TIMESTAMP_ZERO); | ||
|
|
||
| private static Duration minimumDuration = Duration.ofMillis(20); | ||
|
|
||
| @Test | ||
| public void testRunning() throws Exception { | ||
| assertTrue(DockerStatus.isContainerRunning(running, minimumDuration, now)); | ||
| assertTrue(DockerStatus.isContainerRunning(runningVariant, minimumDuration, now)); | ||
| assertFalse(DockerStatus.isContainerRunning(shortRunning, minimumDuration, now)); | ||
| assertFalse(DockerStatus.isContainerRunning(created, minimumDuration, now)); | ||
| assertFalse(DockerStatus.isContainerRunning(createdVariant, minimumDuration, now)); | ||
| assertFalse(DockerStatus.isContainerRunning(exited, minimumDuration, now)); | ||
| assertFalse(DockerStatus.isContainerRunning(paused, minimumDuration, now)); | ||
| } | ||
|
|
||
| @Test | ||
| public void testStopped() throws Exception { | ||
| assertFalse(DockerStatus.isContainerStopped(running)); | ||
| assertFalse(DockerStatus.isContainerStopped(runningVariant)); | ||
| assertFalse(DockerStatus.isContainerStopped(shortRunning)); | ||
| assertFalse(DockerStatus.isContainerStopped(created)); | ||
| assertFalse(DockerStatus.isContainerStopped(createdVariant)); | ||
| assertTrue(DockerStatus.isContainerStopped(exited)); | ||
| assertFalse(DockerStatus.isContainerStopped(paused)); | ||
| } | ||
|
|
||
| private static String buildTimestamp(Instant instant) { | ||
| return DateTimeFormatter.ISO_INSTANT.format(instant); | ||
| } | ||
|
|
||
| // ContainerState is a non-static inner class, with private member variables, in a different package. | ||
| // It's simpler to mock it that to try to create one. | ||
| private static InspectContainerResponse.ContainerState buildState(boolean running, boolean paused, | ||
| String startedAt, String finishedAt) { | ||
|
|
||
| InspectContainerResponse.ContainerState state = Mockito.mock(InspectContainerResponse.ContainerState.class); | ||
| when(state.isRunning()).thenReturn(running); | ||
| when(state.isPaused()).thenReturn(paused); | ||
| when(state.getStartedAt()).thenReturn(startedAt); | ||
| when(state.getFinishedAt()).thenReturn(finishedAt); | ||
| return state; | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead of a boolean you can define some non-Exception throwable (i.e.
ContainerStartError extends Throwable) and catch it.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While that would work, I'd rather not. Recall that the javadoc for Error says:
"An Error is a subclass of Throwable that indicates serious problems that a reasonable application should not try to catch."
With that in mind, what you're suggesting doesn't feel (to me at least) to pass a "principle of least surprise" check. The boolean[] approach is consistent with what we're doing with the int[] in start().