Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions core/src/main/java/org/testcontainers/UnstableAPI.java
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 {
}
126 changes: 112 additions & 14 deletions core/src/main/java/org/testcontainers/containers/GenericContainer.java
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;
Expand All @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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
*/
Expand Down Expand Up @@ -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;
Copy link
Member Author

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

Copy link
Contributor

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!


/*
* Info about the Docker server; lazily fetched.
Expand Down Expand Up @@ -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());
Expand Down Expand Up @@ -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;
});

Expand All @@ -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()) {
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());
Copy link
Member

Choose a reason for hiding this comment

The 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);
Expand All @@ -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);

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

@aguibert aguibert Aug 28, 2019

Choose a reason for hiding this comment

The 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.
I'm thinking something like:

################################################################
Reuse was requested but the environment does not support the reuse of containers. 
To enable container reuse, add the property 'testcontainers.reuse.enable=true' 
to a file at ~/.testcontainers.properties (you may need to create it).
################################################################

Copy link
Member Author

Choose a reason for hiding this comment

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

This warning will only appear if they use withReuse(true), which means that they are aware of the feature already :)

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

Copy link
Contributor

Choose a reason for hiding this comment

The 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 withReuse(true) but other developers may not

Copy link

Choose a reason for hiding this comment

The 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.

Copy link

Choose a reason for hiding this comment

The 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 ~/.testcontainers.properties. Windows users are probably going to be confused by that as well. Is it the only way to set that property?

Copy link
Contributor

Choose a reason for hiding this comment

The 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 ~/.testcontainers.properties to configure it... why not allow it to also be set from the classpath testcontainers.properties?

By forcing it to be under $HOME, you're forcing end-users to setup all build servers, every developer workstation, etc to configure this file. I don't see why you can't let the end-user opt-in to this feature with the classpath config file, this makes the most sense for enterprise development where consistency is king

Copy link
Contributor

@aguibert aguibert Jan 21, 2020

Choose a reason for hiding this comment

The 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.

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 @aguibert for providing a good answer :)

So yeah, what Andy 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.

Yes, next iteration we will definitely improve the UX around it 👍

Choose a reason for hiding this comment

The 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 improve the UX)

Copy link
Member Author

Choose a reason for hiding this comment

The 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);

Expand All @@ -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();

logger().info("Container {} started", dockerImageName);
logger().info("Container {} started in {}", dockerImageName, Duration.between(startedAt, Instant.now()));
containerIsStarted(containerInfo);
} catch (Exception e) {
logger().error("Could not start container", e);
Expand All @@ -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) {
// TODO locking
return dockerClient.listContainersCmd()
.withLabelFilter(ImmutableMap.of(HASH_LABEL, hash))
.withLimit(1)
.withStatusFilter(Arrays.asList("running"))
.exec()
.stream()
.findAny()
.map(it -> it.getId());
}

/**
* Set any custom settings for the create command such as shared memory size.
*/
Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -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.
*
Expand Down Expand Up @@ -1246,6 +1338,12 @@ public SELF withTmpFs(Map<String, String> mapping) {
return self();
}

@UnstableAPI
public SELF withReuse(boolean reusable) {
Copy link
Member

Choose a reason for hiding this comment

The 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):

.withReuse( Reuse.reuseContainers() )

but also allow, for the sake of #1713 (and the hilariously old draft implementation):

.withReuse ( Reuse.reuseImagesAfterInitialization() ) // definitely needs a better name

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?

Copy link
Member Author

Choose a reason for hiding this comment

The 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:

  1. JDBC URL can only have boolean (or lambda reference, but that's tricky)
  2. withReuse(Boolean) is easy to extend later with an overloaded withReuse(ReuseStrategy)
  3. The image caching is another great optimization, but I don't feel that it is the same as the container reusing - we will need to start a new container every time (which is also CI friendly, unlike this PR), which kinda makes it a sugar for the image + capturing an image after the start of container.
  4. The reuse is currently has to be enabled with the property. Having different strategies may make it harder because I assume it will be per-strategy enablement

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)

Copy link
Member

Choose a reason for hiding this comment

The 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;
Expand Down
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
Expand Up @@ -2,6 +2,7 @@

import lombok.*;
import lombok.extern.slf4j.Slf4j;
import org.testcontainers.UnstableAPI;

import java.io.*;
import java.net.MalformedURLException;
Expand Down Expand Up @@ -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"));
Copy link

Choose a reason for hiding this comment

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

Is there a reason this is fetched from the environmentProperties and not just the general properties? I just added this property to my testcontainers.properties file on my classpath and was wondering for a long time why this wasn't working until I saw this line.

Copy link
Member Author

Choose a reason for hiding this comment

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

See this thread:
#1781 (comment)

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.
The PR's description states it pretty clearly and does not mention the classpath file anywhere :)

}

public String getDockerClientStrategyClassName() {
return (String) environmentProperties.get("docker.client.strategy");
}

/**
*
*
* @deprecated we no longer have different transport types
*/
@Deprecated
Expand Down
Loading