Skip to content
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

Merged
merged 17 commits into from
Oct 26, 2019
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()) {
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());
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);
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");
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();
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);
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) {
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.
*/
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