Skip to content
This repository has been archived by the owner on Sep 6, 2024. It is now read-only.

Closes issue #19. Support multiple Docker annotations with networks. #105

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
26 changes: 26 additions & 0 deletions src/main/java/com/github/junit5docker/ContainerInfo.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
package com.github.junit5docker;

import java.util.Collection;

import static java.util.Collections.emptySet;
import static java.util.Collections.unmodifiableCollection;

class ContainerInfo {
Copy link
Owner

Choose a reason for hiding this comment

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

Thanks for fixing my primitive obsession on the container ID :) 👍

Copy link
Author

Choose a reason for hiding this comment

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

:)


private final String containerId;

private final Collection<String> networkIds;

ContainerInfo(String containerId, Collection<String> networkIds) {
this.containerId = containerId;
this.networkIds = networkIds == null ? emptySet() : unmodifiableCollection(networkIds);
}

String getContainerId() {
Copy link
Owner

Choose a reason for hiding this comment

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

oh, i forgot it last time :) Since this is just a immutable data structure, public fields or fine for me. I don't see any value in getters.
What's your opinion on this ?

Copy link
Author

@hanleyt hanleyt Jan 29, 2018

Choose a reason for hiding this comment

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

Hmm I've always done this out of habit after reading effective java. But on further inspection its fine to access fields directly if the class isn't public, and even more so if the fields are immutable.
So sure, I'll make that change. Thanks! :)

return containerId;
}

Collection<String> getNetworkIds() {
return networkIds;
}
}
64 changes: 62 additions & 2 deletions src/main/java/com/github/junit5docker/DefaultDockerClient.java
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,17 @@
import com.github.dockerjava.api.DockerClient;
import com.github.dockerjava.api.exception.NotFoundException;
import com.github.dockerjava.api.model.ExposedPort;
import com.github.dockerjava.api.model.Network;
import com.github.dockerjava.api.model.Ports;
import com.github.dockerjava.core.DockerClientBuilder;
import com.github.dockerjava.core.command.PullImageResultCallback;

import java.util.Collection;
import java.util.Collections;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.stream.Stream;

import static com.github.dockerjava.api.model.ExposedPort.tcp;
Expand All @@ -25,12 +30,67 @@ class DefaultDockerClient implements DockerClientAdapter {
}

@Override
public String startContainer(String wantedImage, Map<String, String> environment, PortBinding... portBinding) {
public ContainerInfo startContainer(String wantedImage,
Map<String, String> environment,
String[] networkNames,
PortBinding... portBinding) {
Ports bindings = createPortBindings(portBinding);
List<String> environmentStrings = createEnvironmentList(environment);
String containerId = createContainer(wantedImage, bindings, environmentStrings);
dockerClient.startContainerCmd(containerId).exec();
return containerId;
Collection<String> networkIds = joinNetworks(networkNames, containerId);
return new ContainerInfo(containerId, networkIds);
}

private Collection<String> joinNetworks(String[] networkNames, String containerId) {
if (networkNames == null || networkNames.length == 0) {
return Collections.emptySet();
}
Collection<String> networkIds = new HashSet<>();

for (String network : networkNames) {
String networkId = getNetworkId(network);
dockerClient.connectToNetworkCmd()
.withNetworkId(networkId)
.withContainerId(containerId)
.exec();
networkIds.add(networkId);
}
return networkIds;
}

private String getNetworkId(String networkName) {
return getExistingNetworkId(networkName)
.orElseGet(() -> dockerClient.createNetworkCmd().withName(networkName).exec().getId());
}

private Optional<String> getExistingNetworkId(String networkName) {
return dockerClient.listNetworksCmd()
.exec()
.stream()
.filter(name -> name.getName().equals(networkName))
.reduce((name1, name2) -> {
String msg = String.format("Multiple networks found with the same name: %s and %s",
name1,
name2);
throw new IllegalStateException(msg);
Copy link
Owner

Choose a reason for hiding this comment

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

Is it really something that can happen or is it a docker bug ?
If it's a docker bug, I think we could just get the first network having the right name. What do you think ?

Copy link
Author

Choose a reason for hiding this comment

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

Hmm for some reason i thought i saw this happening during development when the networks weren't being cleaned up correctly so I added this guard.
But testing it again now it seems you're right, that you can't have multiple networks with the same name. So i'll change it back to find first :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

@hanleyt @FaustXVI I think there is a case you can if they don't have the same scope — i.e. one is local (bridge) and the other would be swarm (overaly — created elsewhere on the cluster). That said, I think it's an edge case and shouldn't really happen that much 😓

Copy link
Owner

Choose a reason for hiding this comment

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

@vdemeester well, I think it's the kind of edge case we actually don't want to handle. I think that when you start to have this kind of configuration, you're not really on a development machine (dev or ci). So maybe we should actually crash at the network creation time if the network already exists. What do you think ? or we can avoid the crash by appending a random value to it ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Both would work (i.e. let the thing crash at network creation, or adding a random value to make sure it's unique)

})
.map(Network::getId);
}

@Override
public void disconnectFromNetwork(String containerId, String networkId) {
dockerClient.disconnectFromNetworkCmd()
.withContainerId(containerId)
.withNetworkId(networkId)
.exec();
}

@Override
public void maybeRemoveNetwork(String networkId) {
if (dockerClient.inspectNetworkCmd().withNetworkId(networkId).exec().getContainers().isEmpty()) {
dockerClient.removeNetworkCmd(networkId).exec();
}
}

@Override
Expand Down
11 changes: 10 additions & 1 deletion src/main/java/com/github/junit5docker/Docker.java
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,15 @@
import org.junit.jupiter.api.extension.ExtendWith;

import java.lang.annotation.ElementType;
import java.lang.annotation.Repeatable;
import java.lang.annotation.Retention;
import java.lang.annotation.RetentionPolicy;
import java.lang.annotation.Target;

import static com.github.junit5docker.WaitFor.NOTHING;

/**
* <p>This annotation is the junit-docker entry point.</p>
* <p>This annotation is a junit-docker entry point.</p>
*
* <p>Adding this annotation to a test's class will start a docker container before running the tests and will be stop
* at the end of the tests. This is done once per class.</p>
Expand All @@ -20,6 +21,7 @@
@Target({ElementType.TYPE})
@Retention(RetentionPolicy.RUNTIME)
@ExtendWith(DockerExtension.class)
@Repeatable(value = Dockers.class)
Copy link
Owner

Choose a reason for hiding this comment

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

If reapeted, don't we risk to call the DockerExtension several times ? Would removing the ExtendWith fix the problem (since you added it to the Dockers annotation)?

Copy link
Author

Choose a reason for hiding this comment

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

From the docs it seems that registering an extension a second time is fine, that the second attempt will be ignored.
http://junit.org/junit5/docs/current/user-guide/#extensions-registration-inheritance

By keeping it this way we allow users to still use the single Docker annotation by itself, without always having to wrap it in the Dockers annotation.

Copy link
Owner

Choose a reason for hiding this comment

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

Great ! Let's go for it then :)

public @interface Docker {

/**
Expand Down Expand Up @@ -51,4 +53,11 @@
* False if it should be created only once for the test class.
*/
boolean newForEachCase() default true;

/**
* @return the names of the networks for the container to join.
* Empty if the container has no networks to join.
*/
String[] networks() default {};

}
15 changes: 14 additions & 1 deletion src/main/java/com/github/junit5docker/DockerClientAdapter.java
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,20 @@
import java.util.stream.Stream;

interface DockerClientAdapter {
String startContainer(String wantedImage, Map<String, String> environment, PortBinding... portBinding);

ContainerInfo startContainer(String wantedImage,
Map<String, String> environment,
String[] networkNames,
PortBinding... portBinding);

void disconnectFromNetwork(String containerId, String networkId);

/**
* Remove the specified network if no containers are connected to it.
*
* @param networkId The id of the network to try remove
*/
void maybeRemoveNetwork(String networkId);

void stopAndRemoveContainer(String containerId);

Expand Down
104 changes: 49 additions & 55 deletions src/main/java/com/github/junit5docker/DockerExtension.java
Original file line number Diff line number Diff line change
Expand Up @@ -6,90 +6,69 @@
import org.junit.jupiter.api.extension.BeforeEachCallback;
import org.junit.jupiter.api.extension.ExtensionContext;

import java.util.Arrays;
import java.util.HashMap;
import java.util.Map;
import java.util.concurrent.CompletableFuture;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
import java.util.concurrent.TimeUnit;
import java.util.function.Supplier;
import java.util.stream.Stream;

import static java.util.concurrent.CompletableFuture.supplyAsync;
import java.util.function.BiConsumer;
import java.util.function.Predicate;

class DockerExtension implements BeforeAllCallback, AfterAllCallback, BeforeEachCallback, AfterEachCallback {

private final DockerClientAdapter dockerClient;

private String containerId;
private final DockerLogChecker dockerLogChecker;
Copy link
Owner

Choose a reason for hiding this comment

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

Awesome refactoring ! Makes the code way simpler :) 👍

Copy link
Author

Choose a reason for hiding this comment

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

thanks! it was motivated by checkstyle/pmd saying the class had too many dependencies :)


DockerExtension() {
this(new DefaultDockerClient());
}

DockerExtension(DockerClientAdapter dockerClient) {
this.dockerClient = dockerClient;
this.dockerLogChecker = new DockerLogChecker(dockerClient);
}

@Override
public void beforeAll(ExtensionContext containerExtensionContext) {
Docker dockerAnnotation = findDockerAnnotation(containerExtensionContext);
if (!dockerAnnotation.newForEachCase()) startContainer(dockerAnnotation);
public void beforeAll(ExtensionContext context) {
forEachDocker(context, docker -> !docker.newForEachCase(), this::startContainer);
}

@Override
public void beforeEach(ExtensionContext context) {
Docker dockerAnnotation = findDockerAnnotation(context);
if (dockerAnnotation.newForEachCase()) startContainer(dockerAnnotation);
forEachDocker(context, Docker::newForEachCase, this::startContainer);
}

private static void forEachDocker(ExtensionContext context,
Predicate<Docker> predicate,
BiConsumer<ExtensionContext, Docker> action) {
Arrays.stream(findDockerAnnotations(context))
.filter(predicate)
.forEach(docker -> action.accept(context, docker));
}

private void startContainer(Docker dockerAnnotation) {
private void startContainer(ExtensionContext context, Docker dockerAnnotation) {
PortBinding[] portBindings = createPortBindings(dockerAnnotation);
Map<String, String> environmentMap = createEnvironmentMap(dockerAnnotation);
String imageReference = findImageName(dockerAnnotation);
WaitFor waitFor = dockerAnnotation.waitFor();
containerId = dockerClient.startContainer(imageReference, environmentMap, portBindings);
waitForLogAccordingTo(waitFor);
}

private void waitForLogAccordingTo(WaitFor waitFor) {
String expectedLog = waitFor.value();
if (!WaitFor.NOTHING.equals(expectedLog)) {
ExecutorService executor = Executors.newSingleThreadExecutor();
CompletableFuture<Boolean> logFound = supplyAsync(findFirstLogContaining(expectedLog), executor);
executor.shutdown();
try {
boolean termination = executor.awaitTermination(waitFor.timeoutInMillis(), TimeUnit.MILLISECONDS);
if (!termination) {
throw new AssertionError("Timeout while waiting for log : \"" + expectedLog + "\"");
}
if (!logFound.getNow(false)) {
throw new AssertionError("\"" + expectedLog + "\" not found in logs and container stopped");
}
} catch (InterruptedException e) {
Thread.currentThread().interrupt();
}
}
}

private Supplier<Boolean> findFirstLogContaining(String logToFind) {
return () -> {
try (Stream<String> logs = dockerClient.logs(containerId)) {
return logs.anyMatch(log -> log.contains(logToFind));
}
};
String[] networkNames = dockerAnnotation.networks();
ContainerInfo containerInfo = dockerClient.startContainer(imageReference,
environmentMap,
networkNames,
portBindings);
dockerLogChecker.waitForLogAccordingTo(waitFor, containerInfo.getContainerId());
getStore(context).put(dockerAnnotation, containerInfo);
}

private Docker findDockerAnnotation(ExtensionContext extensionContext) {
private static Docker[] findDockerAnnotations(ExtensionContext extensionContext) {
Class<?> testClass = extensionContext.getTestClass().get();
return testClass.getAnnotation(Docker.class);
return testClass.getAnnotationsByType(Docker.class);
}

private String findImageName(Docker dockerAnnotation) {
private static String findImageName(Docker dockerAnnotation) {
return dockerAnnotation.image();
}

private Map<String, String> createEnvironmentMap(Docker dockerAnnotation) {
private static Map<String, String> createEnvironmentMap(Docker dockerAnnotation) {
Map<String, String> environmentMap = new HashMap<>();
Environment[] environments = dockerAnnotation.environments();
for (Environment environment : environments) {
Expand All @@ -98,7 +77,7 @@ private Map<String, String> createEnvironmentMap(Docker dockerAnnotation) {
return environmentMap;
}

private PortBinding[] createPortBindings(Docker dockerAnnotation) {
private static PortBinding[] createPortBindings(Docker dockerAnnotation) {
Port[] ports = dockerAnnotation.ports();
PortBinding[] portBindings = new PortBinding[ports.length];
for (int i = 0; i < ports.length; i++) {
Expand All @@ -109,14 +88,29 @@ private PortBinding[] createPortBindings(Docker dockerAnnotation) {
}

@Override
public void afterAll(ExtensionContext containerExtensionContext) {
Docker dockerAnnotation = findDockerAnnotation(containerExtensionContext);
if (!dockerAnnotation.newForEachCase()) dockerClient.stopAndRemoveContainer(containerId);
public void afterAll(ExtensionContext context) {
forEachDocker(context, docker -> !docker.newForEachCase(), this::stopAndRemove);
}

@Override
public void afterEach(ExtensionContext context) {
Docker dockerAnnotation = findDockerAnnotation(context);
if (dockerAnnotation.newForEachCase()) dockerClient.stopAndRemoveContainer(containerId);
forEachDocker(context, Docker::newForEachCase, this::stopAndRemove);
}

private void stopAndRemove(ExtensionContext context, Docker docker) {
ContainerInfo containerInfo = getStore(context).remove(docker, ContainerInfo.class);

String containerId = containerInfo.getContainerId();

containerInfo.getNetworkIds().forEach(networkId -> {
dockerClient.disconnectFromNetwork(containerId, networkId);
dockerClient.maybeRemoveNetwork(networkId);
Copy link
Owner

Choose a reason for hiding this comment

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

The goal of the dockerClient interface is to define the interface we'd like to have with docker.

Since we want to clean up behind us, I would argue that we want to disconnect from all networks and (always?) remove them. If so, the Docker extension doesn't need to know the networkIds. The DockerClient can do it for us.

I think that stopAndRemoveContainer is the perfect place to do so. Maybe we should rename it to stopAndCleanContainer ?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, good idea! that makes sense and would be much cleaner

});
dockerClient.stopAndRemoveContainer(containerId);
}

private static ExtensionContext.Store getStore(ExtensionContext context) {
return context.getStore(ExtensionContext.Namespace.GLOBAL);
Copy link
Owner

Choose a reason for hiding this comment

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

I didn't know about the store. Looks ok to me but I don't understand why you prefered this vs having a field in the class ?

Copy link
Author

Choose a reason for hiding this comment

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

From what I understand its the recommended way to store state in extensions, and that extension classes should be stateless. It is essentially just a namespaced, hierarchical map, and having a map as a class field I imagine would perform in the exact same way.
But apparently there are no guarantees about the exension lifecycle, so its better to use the store to store state. Plus it could be used by other extensions if they wanted to access the test state.

https://blog.codefx.org/design/architecture/junit-5-extension-model/#Stateless

Copy link
Owner

Choose a reason for hiding this comment

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

Awesome ! Good catch !

}

}
48 changes: 48 additions & 0 deletions src/main/java/com/github/junit5docker/DockerLogChecker.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
package com.github.junit5docker;

import java.util.concurrent.CompletableFuture;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
import java.util.concurrent.TimeUnit;
import java.util.function.Supplier;
import java.util.stream.Stream;

import static java.util.concurrent.CompletableFuture.supplyAsync;

class DockerLogChecker {

private final DockerClientAdapter dockerClient;

DockerLogChecker(DockerClientAdapter dockerClient) {
this.dockerClient = dockerClient;
}

void waitForLogAccordingTo(WaitFor waitFor, String containerId) {
String expectedLog = waitFor.value();
if (!WaitFor.NOTHING.equals(expectedLog)) {
ExecutorService executor = Executors.newSingleThreadExecutor();
CompletableFuture<Boolean> logFound = supplyAsync(findFirstLogContaining(expectedLog, containerId),
executor);
executor.shutdown();
try {
boolean termination = executor.awaitTermination(waitFor.timeoutInMillis(), TimeUnit.MILLISECONDS);
if (!termination) {
throw new AssertionError("Timeout while waiting for log : \"" + expectedLog + "\"");
}
if (!logFound.getNow(false)) {
throw new AssertionError("\"" + expectedLog + "\" not found in logs and container stopped");
}
} catch (InterruptedException e) {
Thread.currentThread().interrupt();
}
}
}

private Supplier<Boolean> findFirstLogContaining(String logToFind, String containerId) {
return () -> {
try (Stream<String> logs = dockerClient.logs(containerId)) {
return logs.anyMatch(log -> log.contains(logToFind));
}
};
}
}
Loading