-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Reusable containers #1781
Reusable containers #1781
Changes from all commits
1176da6
469d475
81021ba
7dba2fb
480b9d0
3b0adb3
66f481e
111ae0f
45f972d
9e5951a
173f66e
2b13c46
44c7ad4
e99d4d5
bf790d2
63b276e
e793eb0
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 |
---|---|---|
@@ -0,0 +1,21 @@ | ||
package org.testcontainers; | ||
|
||
import java.lang.annotation.Documented; | ||
import java.lang.annotation.ElementType; | ||
import java.lang.annotation.Retention; | ||
import java.lang.annotation.RetentionPolicy; | ||
import java.lang.annotation.Target; | ||
|
||
/** | ||
* Marks that the annotated API is a subject to change and SHOULD NOT be considered | ||
* a stable API. | ||
*/ | ||
@Retention(RetentionPolicy.SOURCE) | ||
@Target({ | ||
ElementType.TYPE, | ||
ElementType.METHOD, | ||
ElementType.FIELD, | ||
}) | ||
@Documented | ||
public @interface UnstableAPI { | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,9 @@ | ||
package org.testcontainers.containers; | ||
|
||
import com.fasterxml.jackson.core.JsonProcessingException; | ||
import com.fasterxml.jackson.databind.MapperFeature; | ||
import com.fasterxml.jackson.databind.ObjectMapper; | ||
import com.fasterxml.jackson.databind.SerializationFeature; | ||
import com.github.dockerjava.api.DockerClient; | ||
import com.github.dockerjava.api.command.CreateContainerCmd; | ||
import com.github.dockerjava.api.command.InspectContainerResponse; | ||
|
@@ -12,13 +16,15 @@ | |
import com.github.dockerjava.api.model.PortBinding; | ||
import com.github.dockerjava.api.model.Volume; | ||
import com.github.dockerjava.api.model.VolumesFrom; | ||
import com.google.common.annotations.VisibleForTesting; | ||
import com.google.common.base.Strings; | ||
import com.google.common.collect.ImmutableMap; | ||
import lombok.AccessLevel; | ||
import lombok.Data; | ||
import lombok.EqualsAndHashCode; | ||
import lombok.NonNull; | ||
import lombok.Setter; | ||
import lombok.SneakyThrows; | ||
import org.apache.commons.codec.digest.DigestUtils; | ||
import org.apache.commons.compress.archivers.tar.TarArchiveInputStream; | ||
import org.apache.commons.compress.archivers.tar.TarArchiveOutputStream; | ||
import org.apache.commons.compress.utils.IOUtils; | ||
|
@@ -33,6 +39,7 @@ | |
import org.rnorth.visibleassertions.VisibleAssertions; | ||
import org.slf4j.Logger; | ||
import org.testcontainers.DockerClientFactory; | ||
import org.testcontainers.UnstableAPI; | ||
import org.testcontainers.containers.output.OutputFrame; | ||
import org.testcontainers.containers.startupcheck.IsRunningStartupCheckStrategy; | ||
import org.testcontainers.containers.startupcheck.MinimumDurationRunningStartupCheckStrategy; | ||
|
@@ -62,11 +69,14 @@ | |
import java.io.FileOutputStream; | ||
import java.io.IOException; | ||
import java.io.InputStream; | ||
import java.lang.reflect.Method; | ||
import java.nio.charset.Charset; | ||
import java.nio.file.Path; | ||
import java.time.Duration; | ||
import java.time.Instant; | ||
import java.util.ArrayList; | ||
import java.util.Arrays; | ||
import java.util.Collection; | ||
import java.util.Collections; | ||
import java.util.HashMap; | ||
import java.util.HashSet; | ||
|
@@ -100,6 +110,8 @@ public class GenericContainer<SELF extends GenericContainer<SELF>> | |
|
||
public static final String INTERNAL_HOST_HOSTNAME = "host.testcontainers.internal"; | ||
|
||
static final String HASH_LABEL = "org.testcontainers.hash"; | ||
|
||
/* | ||
* Default settings | ||
*/ | ||
|
@@ -168,11 +180,12 @@ public class GenericContainer<SELF extends GenericContainer<SELF>> | |
|
||
protected final Set<Startable> dependencies = new HashSet<>(); | ||
|
||
/* | ||
/** | ||
* Unique instance of DockerClient for use by this container object. | ||
* We use {@link LazyDockerClient} here to avoid eager client creation | ||
*/ | ||
@Setter(AccessLevel.NONE) | ||
protected DockerClient dockerClient = DockerClientFactory.instance().client(); | ||
protected DockerClient dockerClient = LazyDockerClient.INSTANCE; | ||
|
||
/* | ||
* Info about the Docker server; lazily fetched. | ||
|
@@ -222,6 +235,8 @@ public class GenericContainer<SELF extends GenericContainer<SELF>> | |
@Nullable | ||
private Map<String, String> tmpFsMapping; | ||
|
||
@Setter(AccessLevel.NONE) | ||
private boolean shouldBeReused = false; | ||
|
||
public GenericContainer() { | ||
this(TestcontainersConfiguration.getInstance().getTinyImage()); | ||
|
@@ -276,13 +291,15 @@ protected void doStart() { | |
try { | ||
configure(); | ||
|
||
Instant startedAt = Instant.now(); | ||
|
||
logger().debug("Starting container: {}", getDockerImageName()); | ||
logger().debug("Trying to start container: {}", image.get()); | ||
|
||
AtomicInteger attempt = new AtomicInteger(0); | ||
Unreliables.retryUntilSuccess(startupAttempts, () -> { | ||
logger().debug("Trying to start container: {} (attempt {}/{})", image.get(), attempt.incrementAndGet(), startupAttempts); | ||
tryStart(); | ||
tryStart(startedAt); | ||
return true; | ||
}); | ||
|
||
|
@@ -291,7 +308,25 @@ protected void doStart() { | |
} | ||
} | ||
|
||
private void tryStart() { | ||
@UnstableAPI | ||
@SneakyThrows | ||
protected boolean canBeReused() { | ||
for (Class<?> type = getClass(); type != GenericContainer.class; type = type.getSuperclass()) { | ||
kiview marked this conversation as resolved.
Show resolved
Hide resolved
|
||
try { | ||
Method method = type.getDeclaredMethod("containerIsCreated", String.class); | ||
if (method.getDeclaringClass() != GenericContainer.class) { | ||
logger().warn("{} can't be reused because it overrides {}", getClass(), method.getName()); | ||
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. Very good idea. |
||
return false; | ||
} | ||
} catch (NoSuchMethodException e) { | ||
// ignore | ||
} | ||
} | ||
|
||
return true; | ||
} | ||
|
||
private void tryStart(Instant startedAt) { | ||
try { | ||
String dockerImageName = image.get(); | ||
logger().debug("Starting container: {}", dockerImageName); | ||
|
@@ -300,16 +335,49 @@ private void tryStart() { | |
CreateContainerCmd createCommand = dockerClient.createContainerCmd(dockerImageName); | ||
applyConfiguration(createCommand); | ||
|
||
containerId = createCommand.exec().getId(); | ||
createCommand.getLabels().put(DockerClientFactory.TESTCONTAINERS_LABEL, "true"); | ||
|
||
connectToPortForwardingNetwork(createCommand.getNetworkMode()); | ||
boolean reused = false; | ||
if (shouldBeReused) { | ||
if (!canBeReused()) { | ||
throw new IllegalStateException("This container does not support reuse"); | ||
} | ||
|
||
copyToFileContainerPathMap.forEach(this::copyFileToContainer); | ||
if (TestcontainersConfiguration.getInstance().environmentSupportsReuse()) { | ||
String hash = hash(createCommand); | ||
|
||
containerIsCreated(containerId); | ||
containerId = findContainerForReuse(hash).orElse(null); | ||
bsideup marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
logger().info("Starting container with ID: {}", containerId); | ||
dockerClient.startContainerCmd(containerId).exec(); | ||
if (containerId != null) { | ||
logger().info("Reusing container with ID: {} and hash: {}", containerId, hash); | ||
reused = true; | ||
} else { | ||
logger().debug("Can't find a reusable running container with hash: {}", hash); | ||
|
||
createCommand.getLabels().put(HASH_LABEL, hash); | ||
} | ||
} else { | ||
logger().info("Reuse was requested but the environment does not support the reuse of containers"); | ||
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. IMO we should make this a big/obvious INFO message that includes instructions on how to enable reusable containers. This way users that don't closely follow the new TC features will still hear about it and know how to enable it.
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. This warning will only appear if they use Also, we expect this feature to be pretty well discoverable by anyone who cares about the speed of the tests (will also make sure that the docs explain it well), and I don't feel that we need the banner 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 think for projects with multiple developers it may be less obvious. One developer on a team may know about the feature and add 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 just tried this feature for the first time, and I have to say it's completely non-obvious. I see the INFO log, but there is no hint about what I need to do to fix it. 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. BTW the file name in the banner hint above appears to be wrong as well. I think it needs to be 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. Was just trying to debug this feature... it is very difficult to discover. Additionally, it's setup such that it forces you to use By forcing it to be under 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. The intent of this feature (reusing containers across multiple test runs) was to only have it apply for developer workstations. @bsideup specifically did not want it to be used for CI/build servers because container instances could pile up and bog down remote servers and easily go un-noticed. That being said, I do think a more prominent INFO/WARN message for this situation would help users figure out how to turn this on in their local dev workstations. 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. Thanks @aguibert for providing a good answer :) So yeah, what Andy said :)
Yes, next iteration we will definitely improve the UX around it 👍 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. What about if a user does check this in to the file on their classpath, to ignore it (as you are already doing), but also print out the explanation that @aguibert mentioned and that the setting needs to be set elsewhere. (Perhaps that is what you meant by 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. yes, we will definitely consider reporting environment-specific configuration set in the classpath file! Good suggestion 👍 |
||
} | ||
} else { | ||
createCommand.getLabels().put(DockerClientFactory.TESTCONTAINERS_SESSION_ID_LABEL, DockerClientFactory.SESSION_ID); | ||
} | ||
|
||
if (!reused) { | ||
containerId = createCommand.exec().getId(); | ||
|
||
// TODO add to the hash | ||
copyToFileContainerPathMap.forEach(this::copyFileToContainer); | ||
} | ||
|
||
connectToPortForwardingNetwork(createCommand.getNetworkMode()); | ||
|
||
if (!reused) { | ||
containerIsCreated(containerId); | ||
|
||
logger().info("Starting container with ID: {}", containerId); | ||
dockerClient.startContainerCmd(containerId).exec(); | ||
} | ||
|
||
logger().info("Container {} is starting: {}", dockerImageName, containerId); | ||
|
||
|
@@ -331,7 +399,7 @@ private void tryStart() { | |
// Wait until the process within the container has become ready for use (e.g. listening on network, log message emitted, etc). | ||
waitUntilContainerStarted(); | ||
bsideup marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
logger().info("Container {} started", dockerImageName); | ||
logger().info("Container {} started in {}", dockerImageName, Duration.between(startedAt, Instant.now())); | ||
kiview marked this conversation as resolved.
Show resolved
Hide resolved
|
||
containerIsStarted(containerInfo); | ||
} catch (Exception e) { | ||
logger().error("Could not start container", e); | ||
|
@@ -351,6 +419,31 @@ private void tryStart() { | |
} | ||
} | ||
|
||
@UnstableAPI | ||
@SneakyThrows(JsonProcessingException.class) | ||
final String hash(CreateContainerCmd createCommand) { | ||
// TODO add Testcontainers' version to the hash | ||
String commandJson = new ObjectMapper() | ||
.enable(MapperFeature.SORT_PROPERTIES_ALPHABETICALLY) | ||
.enable(SerializationFeature.ORDER_MAP_ENTRIES_BY_KEYS) | ||
.writeValueAsString(createCommand); | ||
|
||
return DigestUtils.sha1Hex(commandJson); | ||
} | ||
|
||
@VisibleForTesting | ||
Optional<String> findContainerForReuse(String hash) { | ||
kiview marked this conversation as resolved.
Show resolved
Hide resolved
kiview marked this conversation as resolved.
Show resolved
Hide resolved
|
||
// TODO locking | ||
bsideup marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return dockerClient.listContainersCmd() | ||
.withLabelFilter(ImmutableMap.of(HASH_LABEL, hash)) | ||
.withLimit(1) | ||
.withStatusFilter(Arrays.asList("running")) | ||
kiview marked this conversation as resolved.
Show resolved
Hide resolved
|
||
.exec() | ||
.stream() | ||
.findAny() | ||
.map(it -> it.getId()); | ||
} | ||
|
||
/** | ||
* Set any custom settings for the create command such as shared memory size. | ||
*/ | ||
|
@@ -613,7 +706,6 @@ private void applyConfiguration(CreateContainerCmd createCommand) { | |
if (createCommand.getLabels() != null) { | ||
combinedLabels.putAll(createCommand.getLabels()); | ||
} | ||
combinedLabels.putAll(DockerClientFactory.DEFAULT_LABELS); | ||
|
||
createCommand.withLabels(combinedLabels); | ||
} | ||
|
@@ -1214,7 +1306,7 @@ public SELF withStartupAttempts(int attempts) { | |
} | ||
|
||
/** | ||
* Allow low level modifications of {@link CreateContainerCmd} after it was pre-configured in {@link #tryStart()}. | ||
* Allow low level modifications of {@link CreateContainerCmd} after it was pre-configured in {@link #tryStart(Instant)}. | ||
* Invocation happens eagerly on a moment when container is created. | ||
* Warning: this does expose the underlying docker-java API so might change outside of our control. | ||
* | ||
|
@@ -1246,6 +1338,12 @@ public SELF withTmpFs(Map<String, String> mapping) { | |
return self(); | ||
} | ||
|
||
@UnstableAPI | ||
public SELF withReuse(boolean reusable) { | ||
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. Re this bit of the API, I'm afraid I still really would like this to not be a boolean, so that we have room for expansion. e.g. to have an enum or interface to allow (pseudo-code-ish):
but also allow, for the sake of #1713 (and the hilariously old draft implementation):
I think we could get the second style of reuse done very quickly for JDBC-based containers. With the reduced scope of container reuse in this PR, do you think this buys us enough cognitive space to fit this flexibility in? 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. While I understand why we would want to add it, here my 2c why I think we should not do it as part of the first step:
The implementation details of the reuse also make it not easy to have strategies for it, and we may never have more than one (given №3) 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. OK, I'm convinced 😄 I'll add a 5th reason: while I'd love to do it, we may never get around to image reuse. To be pragmatic, I don't want to hold up this almost feature for a someday-maybe feature. |
||
this.shouldBeReused = reusable; | ||
return self(); | ||
} | ||
|
||
@Override | ||
public boolean equals(Object o) { | ||
return this == o; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
package org.testcontainers.containers; | ||
|
||
import com.github.dockerjava.api.DockerClient; | ||
import lombok.experimental.Delegate; | ||
import org.testcontainers.DockerClientFactory; | ||
|
||
enum LazyDockerClient implements DockerClient { | ||
|
||
INSTANCE; | ||
|
||
@Delegate | ||
final DockerClient getDockerClient() { | ||
return DockerClientFactory.instance().client(); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,6 +2,7 @@ | |
|
||
import lombok.*; | ||
import lombok.extern.slf4j.Slf4j; | ||
import org.testcontainers.UnstableAPI; | ||
|
||
import java.io.*; | ||
import java.net.MalformedURLException; | ||
|
@@ -85,12 +86,17 @@ public boolean isDisableChecks() { | |
return Boolean.parseBoolean((String) environmentProperties.getOrDefault("checks.disable", "false")); | ||
} | ||
|
||
@UnstableAPI | ||
public boolean environmentSupportsReuse() { | ||
return Boolean.parseBoolean((String) environmentProperties.getOrDefault("testcontainers.reuse.enable", "false")); | ||
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. Is there a reason this is fetched from the 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. See this thread: TL;DR: it is a parameter of every environment and cannot be shared / committed to the repo. Also, it SHOULD NOT be used on CI environments. |
||
} | ||
|
||
public String getDockerClientStrategyClassName() { | ||
return (String) environmentProperties.get("docker.client.strategy"); | ||
} | ||
|
||
/** | ||
* | ||
* | ||
* @deprecated we no longer have different transport types | ||
*/ | ||
@Deprecated | ||
|
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.
needed this one for unit tests, plus good improvement in general. Was also reported as #1749
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.
awesome, thanks for doing this change @bsideup!