-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
[Feature] Accept image as future instead of a value #106
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 |
---|---|---|
|
@@ -9,12 +9,12 @@ | |
import com.github.dockerjava.api.command.LogContainerCmd; | ||
import com.github.dockerjava.api.model.*; | ||
import com.github.dockerjava.core.async.ResultCallbackTemplate; | ||
import com.github.dockerjava.core.command.PullImageResultCallback; | ||
import com.google.common.base.Preconditions; | ||
import com.google.common.base.Strings; | ||
import lombok.Data; | ||
import lombok.EqualsAndHashCode; | ||
import lombok.NonNull; | ||
import lombok.SneakyThrows; | ||
import org.jetbrains.annotations.Nullable; | ||
import org.junit.runner.Description; | ||
import org.rnorth.ducttape.TimeoutException; | ||
|
@@ -36,6 +36,8 @@ | |
import java.net.URL; | ||
import java.nio.file.Path; | ||
import java.util.*; | ||
import java.util.concurrent.ExecutionException; | ||
import java.util.concurrent.Future; | ||
import java.util.concurrent.TimeUnit; | ||
import java.util.function.Consumer; | ||
import java.util.stream.Collectors; | ||
|
@@ -63,7 +65,7 @@ public class GenericContainer extends FailureDetectingExternalResource implement | |
private List<String> portBindings = new ArrayList<>(); | ||
|
||
@NonNull | ||
private String dockerImageName = "alpine:3.2"; | ||
private Future<String> image; | ||
|
||
@NonNull | ||
private List<String> env = new ArrayList<>(); | ||
|
@@ -99,21 +101,25 @@ public class GenericContainer extends FailureDetectingExternalResource implement | |
.build(); | ||
|
||
public GenericContainer() { | ||
|
||
this("alpine:3.2"); | ||
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. backward compatibility |
||
} | ||
|
||
public GenericContainer(@NonNull final String dockerImageName) { | ||
this.setDockerImageName(dockerImageName); | ||
} | ||
|
||
public GenericContainer(@NonNull final Future<String> image) { | ||
this.image = image; | ||
} | ||
|
||
/** | ||
* Starts the container using docker, pulling an image if necessary. | ||
*/ | ||
public void start() { | ||
int[] attempt = {0}; | ||
Unreliables.retryUntilSuccess(STARTUP_RETRY_COUNT, () -> { | ||
attempt[0]++; | ||
logger().debug("Trying to start container: {} (attempt {}/{})", dockerImageName, attempt[0], STARTUP_RETRY_COUNT); | ||
logger().debug("Trying to start container: {} (attempt {}/{})", image.get(), attempt[0], STARTUP_RETRY_COUNT); | ||
this.tryStart(); | ||
return true; | ||
}); | ||
|
@@ -123,11 +129,9 @@ private void tryStart() { | |
Profiler profiler = new Profiler("Container startup"); | ||
profiler.setLogger(logger()); | ||
|
||
logger().debug("Starting container: {}", dockerImageName); | ||
|
||
try { | ||
|
||
pullImageIfNeeded(dockerImageName, profiler.startNested("Pull image if needed")); | ||
String dockerImageName = image.get(); | ||
logger().debug("Starting container: {}", dockerImageName); | ||
|
||
profiler.start("Prepare container configuration and host configuration"); | ||
configure(); | ||
|
@@ -196,7 +200,7 @@ public void stop() { | |
try { | ||
logger().trace("Stopping container: {}", containerId); | ||
dockerClient.killContainerCmd(containerId).exec(); | ||
logger().info("Stopped container: {}", dockerImageName); | ||
logger().info("Stopped container: {}", containerId); | ||
} catch (DockerException e) { | ||
logger().trace("Error encountered shutting down container (ID: {}) - it may not have been stopped, or may already be stopped: {}", containerId, e.getMessage()); | ||
} | ||
|
@@ -205,9 +209,9 @@ public void stop() { | |
logger().trace("Stopping container: {}", containerId); | ||
try { | ||
dockerClient.removeContainerCmd(containerId).withRemoveVolumes(true).withForce(true).exec(); | ||
logger().info("Removed container and associated volume(s): {}", dockerImageName); | ||
logger().info("Removed container and associated volume(s): {}", containerId); | ||
} catch (InternalServerErrorException e) { | ||
logger().warn("Exception when removing container with associated volume(s): {} (due to {})", dockerImageName, e.getMessage()); | ||
logger().warn("Exception when removing container with associated volume(s): {} (due to {})", containerId, e.getMessage()); | ||
} | ||
} catch (DockerException e) { | ||
logger().trace("Error encountered shutting down container (ID: {}) - it may not have been stopped, or may already be stopped: {}", containerId, e.getMessage()); | ||
|
@@ -221,52 +225,9 @@ public void stop() { | |
*/ | ||
protected Logger logger() { | ||
if ("UTF-8".equals(System.getProperty("file.encoding"))) { | ||
return LoggerFactory.getLogger("\uD83D\uDC33 [" + dockerImageName + "]"); | ||
return LoggerFactory.getLogger("\uD83D\uDC33 [" + this.getDockerImageName() + "]"); | ||
} else { | ||
return LoggerFactory.getLogger("docker[" + dockerImageName + "]"); | ||
} | ||
} | ||
|
||
private void pullImageIfNeeded(final String imageName, Profiler profiler) throws DockerException, InterruptedException { | ||
|
||
// Does our cache already know the image? | ||
if (AVAILABLE_IMAGE_NAME_CACHE.contains(imageName)) { | ||
return; | ||
} | ||
|
||
profiler.start("Check local images"); | ||
// Update the cache | ||
List<Image> images = dockerClient.listImagesCmd().exec(); | ||
for (Image image : images) { | ||
Collections.addAll(AVAILABLE_IMAGE_NAME_CACHE, image.getRepoTags()); | ||
} | ||
|
||
// Check cache again following update | ||
if (AVAILABLE_IMAGE_NAME_CACHE.contains(imageName)) { | ||
return; | ||
} | ||
|
||
logger().info("Pulling docker image: {}. Please be patient; this may take some time but only needs to be done once.", imageName); | ||
profiler.start("Pull image"); | ||
int attempts = 0; | ||
while (!AVAILABLE_IMAGE_NAME_CACHE.contains(imageName)) { | ||
// The image is not available locally - pull it | ||
|
||
if (attempts++ >= 3) { | ||
logger().error("Retry limit reached while trying to pull image: " + imageName + ". Please check output of `docker pull " + imageName + "`"); | ||
throw new ContainerFetchException("Retry limit reached while trying to pull image: " + imageName); | ||
} | ||
|
||
PullImageResultCallback resultCallback = dockerClient.pullImageCmd(imageName).exec(new PullImageResultCallback()); | ||
|
||
resultCallback.awaitCompletion(); | ||
|
||
// Confirm successful pull, otherwise may need to retry... | ||
// see https://github.com/docker/docker/issues/10708 | ||
List<Image> updatedImages = dockerClient.listImagesCmd().exec(); | ||
for (Image image : updatedImages) { | ||
Collections.addAll(AVAILABLE_IMAGE_NAME_CACHE, image.getRepoTags()); | ||
} | ||
return LoggerFactory.getLogger("docker[" + this.getDockerImageName() + "]"); | ||
} | ||
} | ||
|
||
|
@@ -577,21 +538,41 @@ public Integer getMappedPort(final int originalPort) { | |
} | ||
} | ||
|
||
/** | ||
* <b>Resolve</b> Docker image and set it. | ||
* | ||
* @param dockerImageName image name | ||
*/ | ||
@SneakyThrows | ||
public void setDockerImageName(@NonNull String dockerImageName) { | ||
String[] parts = dockerImageName.split(":"); | ||
boolean isTagNamePresent = parts.length == 2; | ||
Preconditions.checkArgument(isTagNamePresent, "No image tag was specified in docker image name (" + dockerImageName + "). Please provide a tag; this may be 'latest' or a specific version"); | ||
|
||
this.dockerImageName = dockerImageName; | ||
RemoteDockerImage remoteDockerImage = new RemoteDockerImage(parts[0], parts[1]); | ||
|
||
boolean isTagNamePresent = dockerImageName.split(":").length == 2; | ||
Preconditions.checkArgument(isTagNamePresent, "No image tag was specified in docker image name (" + dockerImageName + "). Please provide a tag; this may be 'latest' or a specific version"); | ||
try { | ||
// Mimic old behavior where we resolve image once it's set | ||
remoteDockerImage.get(); | ||
} catch (ExecutionException e) { | ||
throw e.getCause(); | ||
} | ||
|
||
Profiler profiler = new Profiler("Rule creation - prefetch image"); | ||
profiler.setLogger(logger()); | ||
this.image = remoteDockerImage; | ||
} | ||
|
||
/** | ||
* Get image name. | ||
* | ||
* @return image name | ||
*/ | ||
@NonNull | ||
@SneakyThrows | ||
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. @SneakyThrows from Lombok here to make it behave as previously - if exception was thrown during pull, it will be thrown here as well. |
||
public String getDockerImageName() { | ||
try { | ||
pullImageIfNeeded(dockerImageName, profiler); | ||
} catch (InterruptedException e) { | ||
throw new ContainerFetchException("Failed to fetch container image for " + dockerImageName, e); | ||
} finally { | ||
profiler.stop().log(); | ||
return image.get(); | ||
} catch (ExecutionException e) { | ||
throw e.getCause(); | ||
} | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,92 @@ | ||
package org.testcontainers.containers; | ||
|
||
import com.github.dockerjava.api.DockerClient; | ||
import com.github.dockerjava.api.model.Image; | ||
import com.github.dockerjava.core.command.PullImageResultCallback; | ||
import com.google.common.util.concurrent.Futures; | ||
import lombok.NonNull; | ||
import lombok.experimental.Delegate; | ||
import org.slf4j.LoggerFactory; | ||
import org.slf4j.profiler.Profiler; | ||
import org.testcontainers.DockerClientFactory; | ||
|
||
import java.io.IOException; | ||
import java.util.Collections; | ||
import java.util.HashSet; | ||
import java.util.List; | ||
import java.util.Set; | ||
import java.util.concurrent.CompletableFuture; | ||
import java.util.concurrent.Future; | ||
|
||
public class RemoteDockerImage implements Future<String> { | ||
|
||
private static final Set<String> AVAILABLE_IMAGE_NAME_CACHE = new HashSet<>(); | ||
|
||
@Delegate | ||
private final Future<String> delegate; | ||
|
||
public RemoteDockerImage(@NonNull String repository, String tag) { | ||
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. We decided to use two arguments instead of one to make it more explicit that it must contain both repository and tag. |
||
delegate = Futures.lazyTransform(CompletableFuture.completedFuture(repository + ":" + tag), dockerImageName -> { | ||
|
||
Profiler profiler = new Profiler("Rule creation - prefetch image"); | ||
|
||
if ("UTF-8".equals(System.getProperty("file.encoding"))) { | ||
profiler.setLogger(LoggerFactory.getLogger("\uD83D\uDC33 [" + dockerImageName + "]")); | ||
} else { | ||
profiler.setLogger(LoggerFactory.getLogger("docker[" + dockerImageName + "]")); | ||
} | ||
|
||
try (DockerClient dockerClient = DockerClientFactory.instance().client()) { | ||
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. Docker clients should be closed to avoid leaks. |
||
|
||
profiler.start("Check local images"); | ||
|
||
int attempts = 0; | ||
while (true) { | ||
// Does our cache already know the image? | ||
if (AVAILABLE_IMAGE_NAME_CACHE.contains(dockerImageName)) { | ||
break; | ||
} | ||
|
||
// Update the cache | ||
List<Image> updatedImages = dockerClient.listImagesCmd().exec(); | ||
for (Image image : updatedImages) { | ||
Collections.addAll(AVAILABLE_IMAGE_NAME_CACHE, image.getRepoTags()); | ||
} | ||
|
||
// And now? | ||
if (AVAILABLE_IMAGE_NAME_CACHE.contains(dockerImageName)) { | ||
break; | ||
} | ||
|
||
// Log only on first attempt | ||
if (attempts == 0) { | ||
profiler.getLogger().info("Pulling docker image: {}. Please be patient; this may take some time but only needs to be done once.", dockerImageName); | ||
profiler.start("Pull image"); | ||
} | ||
|
||
if (attempts++ >= 3) { | ||
profiler.getLogger().error("Retry limit reached while trying to pull image: " + dockerImageName + ". Please check output of `docker pull " + dockerImageName + "`"); | ||
throw new ContainerFetchException("Retry limit reached while trying to pull image: " + dockerImageName); | ||
} | ||
|
||
// The image is not available locally - pull it | ||
try { | ||
dockerClient.pullImageCmd(dockerImageName).exec(new PullImageResultCallback()).awaitCompletion(); | ||
} catch (InterruptedException e) { | ||
throw new ContainerFetchException("Failed to fetch container image for " + dockerImageName, e); | ||
} | ||
|
||
// Do not break here, but step into the next iteration, where it will be verified with listImagesCmd(). | ||
// see https://github.com/docker/docker/issues/10708 | ||
} | ||
|
||
return dockerImageName; | ||
} catch (IOException e) { | ||
throw new ContainerFetchException("Failed to get Docker client for " + dockerImageName, e); | ||
} finally { | ||
profiler.stop().log(); | ||
} | ||
}); | ||
} | ||
|
||
} |
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.
The previous field was private, so it's safe to delete it. However, getters/setters are there and not deleted (to keep backward compatibility)
image
name was selected because it might me both image name ( "owner/repo:tag" ), and image id ( "bde98019a9e2" )