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

Death Note :) #545

Merged
merged 9 commits into from
Jan 27, 2018
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ All notable changes to this project will be documented in this file.

### Changed
- Added Kafka module ([\#546](https://github.com/testcontainers/testcontainers-java/pull/546))
- Added "Death Note" to track & kill spawned containers even if the JVM was "kill -9"ed ([\#545](https://github.com/testcontainers/testcontainers-java/pull/545))
- Environment variables are now stored as Map instead of List ([\#550](https://github.com/testcontainers/testcontainers-java/pull/550))
- Added `withEnv(String name, Function<Optional<String>, String> mapper)` with optional previous value ([\#550](https://github.com/testcontainers/testcontainers-java/pull/550))
- Added `withFileSystemBind` overloaded method with `READ_WRITE` file mode by default ([\#550](https://github.com/testcontainers/testcontainers-java/pull/550))
Expand Down
83 changes: 32 additions & 51 deletions core/src/main/java/org/testcontainers/DockerClientFactory.java
Original file line number Diff line number Diff line change
Expand Up @@ -2,35 +2,32 @@

import com.github.dockerjava.api.DockerClient;
import com.github.dockerjava.api.command.CreateContainerCmd;
import com.github.dockerjava.api.command.InspectContainerResponse;
import com.github.dockerjava.api.exception.InternalServerErrorException;
import com.github.dockerjava.api.exception.NotFoundException;
import com.github.dockerjava.api.model.*;
import com.github.dockerjava.core.command.ExecStartResultCallback;
import com.github.dockerjava.core.command.PullImageResultCallback;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.collect.ImmutableMap;
import lombok.Synchronized;
import lombok.extern.slf4j.Slf4j;
import org.apache.commons.io.IOUtils;
import org.hamcrest.BaseMatcher;
import org.hamcrest.Description;
import org.rnorth.ducttape.unreliables.Unreliables;
import org.rnorth.visibleassertions.VisibleAssertions;
import org.testcontainers.dockerclient.*;
import org.testcontainers.dockerclient.DockerClientProviderStrategy;
import org.testcontainers.dockerclient.DockerMachineClientProviderStrategy;
import org.testcontainers.utility.ComparableVersion;
import org.testcontainers.utility.MountableFile;
import org.testcontainers.utility.ResourceReaper;
import org.testcontainers.utility.TestcontainersConfiguration;

import java.io.ByteArrayOutputStream;
import java.io.IOException;
import java.io.InputStream;
import java.net.Socket;
import java.nio.charset.Charset;
import java.util.ArrayList;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.ServiceLoader;
import java.util.concurrent.TimeUnit;
import java.util.UUID;
import java.util.function.BiFunction;
import java.util.function.Consumer;

Expand All @@ -42,12 +39,22 @@
@Slf4j
public class DockerClientFactory {

public static final String TESTCONTAINERS_LABEL = DockerClientFactory.class.getPackage().getName();
public static final String TESTCONTAINERS_SESSION_ID_LABEL = TESTCONTAINERS_LABEL + ".sessionId";

public static final String SESSION_ID = UUID.randomUUID().toString();

public static final Map<String, String> DEFAULT_LABELS = ImmutableMap.of(
TESTCONTAINERS_LABEL, "true",
TESTCONTAINERS_SESSION_ID_LABEL, SESSION_ID
);

private static final String TINY_IMAGE = TestcontainersConfiguration.getInstance().getTinyImage();
private static DockerClientFactory instance;

// Cached client configuration
private DockerClientProviderStrategy strategy;
private boolean preconditionsChecked = false;
private boolean initialized = false;
Copy link
Member

Choose a reason for hiding this comment

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

This boolean is of course false by default, or did you want to make it explicitly?

Copy link
Member Author

Choose a reason for hiding this comment

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

I just renamed the variable :) But yes, explicit false helps to understand the logic

private String activeApiVersion;
private String activeExecutionDriver;

Expand Down Expand Up @@ -95,7 +102,7 @@ public DockerClient client() {
log.info("Docker host IP address is {}", hostIpAddress);
DockerClient client = strategy.getClient();

if (!preconditionsChecked) {
Copy link
Member Author

Choose a reason for hiding this comment

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

had to change the name because it's not just preconditionsChecked anymore

if (!initialized) {
Copy link
Member

Choose a reason for hiding this comment

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

I think we can encapsulate the following block like:

if (!initialized) {
  initialize();
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Let's not do it in this PR.

Copy link
Member

Choose a reason for hiding this comment

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

No problem, but why not? 😉

Copy link
Member Author

Choose a reason for hiding this comment

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

because it's out of scope, and if somebody submits another PR where they change DockerClientFactory, but we didn't merge "Death Note" PR yet, it will create more conflicts.
I'm happy to extract method as a follow up, or before, or in parallel, but don't want to increase the scope of this change :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Also think about reverts in case we discover some major bug :)

Copy link
Member

Choose a reason for hiding this comment

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

Yes okay, makes sense 🙂

Info dockerInfo = client.infoCmd().exec();
Version version = client.versionCmd().exec();
activeApiVersion = version.getApiVersion();
Expand All @@ -106,30 +113,19 @@ public DockerClient client() {
" Operating System: " + dockerInfo.getOperatingSystem() + "\n" +
" Total Memory: " + dockerInfo.getMemTotal() / (1024 * 1024) + " MB");

if (!TestcontainersConfiguration.getInstance().isDisableChecks()) {
VisibleAssertions.info("Checking the system...");

checkDockerVersion(version.getVersion());

MountableFile mountableFile = MountableFile.forClasspathResource(this.getClass().getName().replace(".", "/") + ".class");
String ryukContainerId = ResourceReaper.start(hostIpAddress, client);
log.info("Ryuk started - will monitor and terminate Testcontainers containers on JVM exit");

runInsideDocker(
client,
cmd -> cmd
.withCmd("/bin/sh", "-c", "while true; do printf 'hello' | nc -l -p 80; done")
.withBinds(new Bind(mountableFile.getResolvedPath(), new Volume("/dummy"), AccessMode.ro))
.withExposedPorts(new ExposedPort(80))
.withPublishAllPorts(true),
(dockerClient, id) -> {
VisibleAssertions.info("Checking the system...");

checkDiskSpace(dockerClient, id);
checkMountableFile(dockerClient, id);
checkExposedPort(hostIpAddress, dockerClient, id);
checkDockerVersion(version.getVersion());

return null;
});
if (!TestcontainersConfiguration.getInstance().isDisableChecks()) {
checkDiskSpace(client, ryukContainerId);
checkMountableFile(client, ryukContainerId);
}
preconditionsChecked = true;

initialized = true;
}

return client;
Expand Down Expand Up @@ -178,26 +174,10 @@ private void checkMountableFile(DockerClient dockerClient, String id) {
}
}

private void checkExposedPort(String hostIpAddress, DockerClient dockerClient, String id) {
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 check is not needed anymore, Ryuk demands a connectivity hence implicitly checks for the exposed port

String response = Unreliables.retryUntilSuccess(3, TimeUnit.SECONDS, () -> {
InspectContainerResponse inspectedContainer = dockerClient.inspectContainerCmd(id).exec();

String portSpec = inspectedContainer.getNetworkSettings().getPorts().getBindings().values().iterator().next()[0].getHostPortSpec();

try (Socket socket = new Socket(hostIpAddress, Integer.parseInt(portSpec))) {
return IOUtils.toString(socket.getInputStream(), Charset.defaultCharset());
} catch (IOException e) {
return e.getMessage();
}
});

VisibleAssertions.assertEquals("A port exposed by a docker container should be accessible", "hello", response);
}

/**
* Check whether the image is available locally and pull it otherwise
*/
private void checkAndPullImage(DockerClient client, String image) {
public void checkAndPullImage(DockerClient client, String image) {
List<Image> images = client.listImagesCmd().withImageNameFilter(image).exec();
if (images.isEmpty()) {
client.pullImageCmd(image).exec(new PullImageResultCallback()).awaitSuccess();
Expand All @@ -221,7 +201,8 @@ public <T> T runInsideDocker(Consumer<CreateContainerCmd> createContainerCmdCons

private <T> T runInsideDocker(DockerClient client, Consumer<CreateContainerCmd> createContainerCmdConsumer, BiFunction<DockerClient, String, T> block) {
checkAndPullImage(client, TINY_IMAGE);
CreateContainerCmd createContainerCmd = client.createContainerCmd(TINY_IMAGE);
CreateContainerCmd createContainerCmd = client.createContainerCmd(TINY_IMAGE)
.withLabels(DEFAULT_LABELS);
createContainerCmdConsumer.accept(createContainerCmd);
String id = createContainerCmd.exec().getId();

Expand Down Expand Up @@ -263,7 +244,7 @@ DiskSpaceUsage parseAvailableDiskSpace(String dfOutput) {
* @return the docker API version of the daemon that we have connected to
*/
public String getActiveApiVersion() {
if (!preconditionsChecked) {
if (!initialized) {
client();
}
return activeApiVersion;
Expand All @@ -273,7 +254,7 @@ public String getActiveApiVersion() {
* @return the docker execution driver of the daemon that we have connected to
*/
public String getActiveExecutionDriver() {
if (!preconditionsChecked) {
if (!initialized) {
client();
}
return activeExecutionDriver;
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package org.testcontainers.containers;

import com.github.dockerjava.api.DockerClient;
import com.github.dockerjava.api.exception.DockerException;
import com.github.dockerjava.api.model.Container;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Joiner;
Expand All @@ -24,16 +23,11 @@
import org.zeroturnaround.exec.stream.slf4j.Slf4jStream;

import java.io.File;
import java.util.Arrays;
import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.*;
import java.util.AbstractMap.SimpleEntry;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.atomic.AtomicInteger;
import java.util.stream.Collectors;

import static com.google.common.base.Preconditions.checkArgument;
import static com.google.common.base.Preconditions.checkNotNull;
Expand All @@ -59,6 +53,8 @@ public class DockerComposeContainer<SELF extends DockerComposeContainer<SELF>> e
private boolean pull = true;
private boolean tailChildContainers;

private String project;

private final AtomicInteger nextAmbassadorPort = new AtomicInteger(2000);
private final Map<String, Map<Integer, Integer>> ambassadorPortMappings = new ConcurrentHashMap<>();
private final SocatContainer ambassadorContainer = new SocatContainer();
Expand Down Expand Up @@ -94,6 +90,7 @@ public DockerComposeContainer(String identifier, List<File> composeFiles) {

// Use a unique identifier so that containers created for this compose environment can be identified
this.identifier = identifier;
project = randomProjectId();

this.dockerClient = DockerClientFactory.instance().client();
}
Expand All @@ -106,6 +103,7 @@ public void starting(Description description) {
profiler.start("Docker Compose container startup");

synchronized (MUTEX) {
registerContainersForShutdown();
if (pull) {
pullImages();
}
Expand All @@ -114,7 +112,6 @@ public void starting(Description description) {
if (tailChildContainers) {
tailChildContainerLogs();
}
registerContainersForShutdown();
startAmbassadorContainers(profiler);
}
}
Expand Down Expand Up @@ -142,9 +139,9 @@ private void tailChildContainerLogs() {
private void runWithCompose(String cmd) {
final DockerCompose dockerCompose;
if (localCompose) {
dockerCompose = new LocalDockerCompose(composeFiles, identifier);
dockerCompose = new LocalDockerCompose(composeFiles, project);
} else {
dockerCompose = new ContainerisedDockerCompose(composeFiles, identifier);
dockerCompose = new ContainerisedDockerCompose(composeFiles, project);
}

dockerCompose
Expand All @@ -166,38 +163,17 @@ private void applyScaling() {
}

private void registerContainersForShutdown() {
// Ensure that all service containers that were launched by compose will be killed at shutdown
Copy link
Member Author

Choose a reason for hiding this comment

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

😍

Copy link
Member

Choose a reason for hiding this comment

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

Awesome 🗡️

Copy link
Member

Choose a reason for hiding this comment

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

✂️ 😄

try {
final List<Container> containers = listChildContainers();

// register with ResourceReaper to ensure final shutdown with JVM
containers.forEach(container ->
ResourceReaper.instance().registerContainerForCleanup(container.getId(), container.getNames()[0]));

// Compose can define their own networks as well; ensure these are cleaned up
dockerClient.listNetworksCmd().exec().forEach(network -> {
if (network.getName().contains(identifier)) {
spawnedNetworkIds.add(network.getId());
ResourceReaper.instance().registerNetworkIdForCleanup(network.getId());
}
});

// remember the IDs to allow containers to be killed as soon as we reach stop()
spawnedContainerIds.addAll(containers.stream()
.map(Container::getId)
.collect(Collectors.toSet()));

} catch (DockerException e) {
logger().debug("Failed to stop a service container with exception", e);
}
ResourceReaper.instance().registerFilterForCleanup(Arrays.asList(
new SimpleEntry<>("label", "com.docker.compose.project=" + project)
));
}

private List<Container> listChildContainers() {
return dockerClient.listContainersCmd()
.withShowAll(true)
.exec().stream()
.filter(container -> Arrays.stream(container.getNames()).anyMatch(name ->
name.startsWith("/" + identifier)))
name.startsWith("/" + project)))
.collect(toList());
}

Expand All @@ -216,29 +192,33 @@ public void finished(Description description) {


synchronized (MUTEX) {
// shut down the ambassador container
ambassadorContainer.stop();

// Kill the services using docker-compose
try {
runWithCompose("down -v");
// shut down the ambassador container
ambassadorContainer.stop();

// If we reach here then docker-compose down has cleared networks and containers;
// we can unregister from ResourceReaper
spawnedContainerIds.forEach(ResourceReaper.instance()::unregisterContainer);
spawnedNetworkIds.forEach(ResourceReaper.instance()::unregisterNetwork);
} catch (Exception e) {
// docker-compose down failed; use ResourceReaper to ensure cleanup
// Kill the services using docker-compose
try {
runWithCompose("down -v");

// kill the spawned service containers
spawnedContainerIds.forEach(ResourceReaper.instance()::stopAndRemoveContainer);
// If we reach here then docker-compose down has cleared networks and containers;
// we can unregister from ResourceReaper
spawnedContainerIds.forEach(ResourceReaper.instance()::unregisterContainer);
spawnedNetworkIds.forEach(ResourceReaper.instance()::unregisterNetwork);
} catch (Exception e) {
// docker-compose down failed; use ResourceReaper to ensure cleanup

// remove the networks after removing the containers
spawnedNetworkIds.forEach(ResourceReaper.instance()::removeNetworkById);
}
// kill the spawned service containers
spawnedContainerIds.forEach(ResourceReaper.instance()::stopAndRemoveContainer);

// remove the networks after removing the containers
spawnedNetworkIds.forEach(ResourceReaper.instance()::removeNetworkById);
}

spawnedContainerIds.clear();
spawnedNetworkIds.clear();
spawnedContainerIds.clear();
spawnedNetworkIds.clear();
} finally {
project = randomProjectId();
}
}
}

Expand Down Expand Up @@ -266,7 +246,7 @@ public SELF withExposedService(String serviceName, int servicePort) {
int ambassadorPort = nextAmbassadorPort.getAndIncrement();
ambassadorPortMappings.computeIfAbsent(serviceName, __ -> new ConcurrentHashMap<>()).put(servicePort, ambassadorPort);
ambassadorContainer.withTarget(ambassadorPort, serviceName, servicePort);
ambassadorContainer.addLink(new FutureContainer(this.identifier + "_" + serviceName), serviceName);
ambassadorContainer.addLink(new FutureContainer(this.project + "_" + serviceName), serviceName);
return self();
}

Expand Down Expand Up @@ -351,6 +331,10 @@ public SELF withTailChildContainers(boolean tailChildContainers) {
private SELF self() {
return (SELF) this;
}

private String randomProjectId() {
return identifier + Base58.randomString(6).toLowerCase();
}
}

interface DockerCompose {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -211,10 +211,8 @@ private void tryStart(Profiler profiler) {
profiler.start("Create container");
CreateContainerCmd createCommand = dockerClient.createContainerCmd(dockerImageName);
applyConfiguration(createCommand);
createContainerCmdModifiers.forEach(hook -> hook.accept(createCommand));

containerId = createCommand.exec().getId();
ResourceReaper.instance().registerContainerForCleanup(containerId, dockerImageName);

logger().info("Starting container with ID: {}", containerId);
profiler.start("Start container");
Expand Down Expand Up @@ -465,7 +463,12 @@ private void applyConfiguration(CreateContainerCmd createCommand) {
createCommand.withPrivileged(privilegedMode);
}

createCommand.withLabels(Collections.singletonMap("org.testcontainers", "true"));
createContainerCmdModifiers.forEach(hook -> hook.accept(createCommand));
Copy link
Member

Choose a reason for hiding this comment

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

Why do you have to do this now?

Copy link
Member Author

Choose a reason for hiding this comment

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

just moved the call from another place because we must add our labels after user sets his own


Map<String, String> labels = createCommand.getLabels();
labels = new HashMap<>(labels != null ? labels : Collections.emptyMap());
labels.putAll(DockerClientFactory.DEFAULT_LABELS);
createCommand.withLabels(labels);
}

private Set<Link> findLinksFromThisContainer(String alias, LinkableContainer linkableContainer) {
Expand Down
Loading