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

Improve cleanup for docker-compose. #394

Merged
merged 5 commits into from
Jul 9, 2017
Merged
Show file tree
Hide file tree
Changes from 3 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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ All notable changes to this project will be documented in this file.
- Fixed leakage of Vibur and Tomcat JDBC test dependencies in `jdbc-test` and `mysql` modules (#382)
- Added timeout and retries for creation of `RemoteWebDriver` (#381, #373, #257)
- Fixed various shading issues
- Improved removal of containers/networks when using Docker Compose, eliminating irrelevant errors during cleanup (#342, #394)

### Changed
- Added support for Docker networks (#372)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
import com.google.common.base.Splitter;
import com.google.common.collect.Maps;
import com.google.common.util.concurrent.Uninterruptibles;
import org.apache.commons.lang.StringUtils;
import org.junit.runner.Description;
import org.rnorth.ducttape.ratelimits.RateLimiter;
import org.rnorth.ducttape.ratelimits.RateLimiterBuilder;
Expand Down Expand Up @@ -46,8 +47,9 @@ public class DockerComposeContainer<SELF extends DockerComposeContainer<SELF>> e
private final String identifier;
private final Map<String, AmbassadorContainer> ambassadorContainers = new HashMap<>();
private final List<File> composeFiles;
private Set<String> spawnedContainerIds = Collections.emptySet();
private Map<String, Integer> scalingPreferences = new HashMap<>();
private final Set<String> spawnedContainerIds = new HashSet<>();
private final Set<String> spawnedNetworkIds = new HashSet<>();
private final Map<String, Integer> scalingPreferences = new HashMap<>();
private DockerClient dockerClient;
private boolean localCompose;
private boolean pull = true;
Expand Down Expand Up @@ -117,14 +119,14 @@ public void starting(Description description) {

private void pullImages() {
getDockerCompose("pull")
.start();
.invoke();
}


private void createServices() {
// Start the docker-compose container, which starts up the services
getDockerCompose("up -d")
.start();
.invoke();
}

private void tailChildContainerLogs() {
Expand Down Expand Up @@ -158,7 +160,7 @@ private void applyScaling() {
}

getDockerCompose(sb.toString())
.start();
.invoke();
}
}

Expand All @@ -173,17 +175,20 @@ private void registerContainersForShutdown() {

// Ensure that the default network for this compose environment, if any, is also cleaned up
ResourceReaper.instance().registerNetworkForCleanup(identifier + "_default");
spawnedNetworkIds.add(identifier + "_default");

// Compose can define their own networks as well; ensure these are cleaned up
dockerClient.listNetworksCmd().exec().forEach(network -> {
if (network.getName().contains(identifier)) {
Copy link
Member

Choose a reason for hiding this comment

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

not related to this PR, but AFAIK we shouldn't use contains here, but startsWith or whatever the rule for Docker Compose, otherwise if identifier if "a", then it will delete all networks with "a" character in name :D

spawnedNetworkIds.add(network.getId());
ResourceReaper.instance().registerNetworkForCleanup(network.getId());
}
});

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

} catch (DockerException e) {
logger().debug("Failed to stop a service container with exception", e);
Expand Down Expand Up @@ -240,15 +245,25 @@ public void finished(Description description) {
ambassadorContainers.forEach((String address, AmbassadorContainer container) -> container.stop());

// Kill the services using docker-compose
getDockerCompose("down -v")
.start();
try {
getDockerCompose("down -v").invoke();

// remove the networks before removing the containers
ResourceReaper.instance().removeNetworks(identifier);
// If we reach here then docker-compose down has cleared networks and containers;
// we can unregister from ResourceReaper
spawnedContainerIds.forEach(id -> ResourceReaper.instance().unregisterContainer(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 is the core of the change - we unregister containers and networks if docker-compose down seems to have worked.

Copy link
Member

Choose a reason for hiding this comment

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

FYI spawnedContainerIds.forEach(ResourceReaper.instance()::unregisterContainer); is also valid here :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah interesting - done :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Not quite sure I follow this:

Since we list networks in Docker Compose's implementation, maybe we can use the IDs there as well?

But I'd interpret this as 'let's only register network IDs, not names'. Is that right? I'll change and make sure we use IDs throughout, then I think we should be OK.

spawnedNetworkIds.forEach(id -> ResourceReaper.instance().unregisterNetwork(identifier));
} catch (ContainerLaunchException e) {
Copy link
Member

Choose a reason for hiding this comment

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

maybe catching just Exception will be better here? Any Exception in invoke will prevent Reaper from being called

Copy link
Member Author

Choose a reason for hiding this comment

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

👍

// docker-compose down failed; use ResourceReaper to ensure cleanup

// kill the spawned service containers
spawnedContainerIds.forEach(id -> ResourceReaper.instance().stopAndRemoveContainer(id));

// remove the networks after removing the containers
spawnedNetworkIds.forEach(id -> ResourceReaper.instance().removeNetworks(identifier));
}

// kill the spawned service containers
spawnedContainerIds.forEach(id -> ResourceReaper.instance().stopAndRemoveContainer(id));
spawnedContainerIds.clear();
spawnedNetworkIds.clear();
}
}

Expand Down Expand Up @@ -372,7 +387,7 @@ interface DockerCompose {

DockerCompose withEnv(Map<String, String> env);

void start();
void invoke();

default void validateFileList(List<File> composeFiles) {
checkNotNull(composeFiles);
Expand Down Expand Up @@ -417,7 +432,7 @@ public ContainerisedDockerCompose(List<File> composeFiles, String identifier) {
}

@Override
public void start() {
public void invoke() {
Copy link
Member Author

Choose a reason for hiding this comment

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

Changed this (internally used) method name, as it's always a blocking operation and invoke seems more accurate.

Copy link
Member

Choose a reason for hiding this comment

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

that's a good change, I was confused by getDockerCompose(...).start(...) constructions :)
P.S. do we use anything else besides invoke() on getDockerCompose's return value? Is not, maybe we can simplify it to something like runWithCompose("down -v") ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good call, will do!

super.start();

this.followOutput(new Slf4jLogConsumer(logger()));
Expand All @@ -431,6 +446,19 @@ public void start() {
logger().info("Docker Compose has finished running");

AuditLogger.doComposeLog(this.getCommandParts(), this.getEnv());

final Integer exitCode = this.dockerClient.inspectContainerCmd(containerId)
.exec()
.getState()
.getExitCode();

if (exitCode == null || exitCode != 0) {
throw new ContainerLaunchException(
Copy link
Member Author

Choose a reason for hiding this comment

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

As with LocalDockerCompose, we now throw an exception if a failure occurred. This allows us to detect a failure of docker-compose down.

"Containerised Docker Compose exited abnormally with code " +
exitCode +
" whilst running command: " +
StringUtils.join(this.getCommandParts(), ' '));
}
}
}

Expand Down Expand Up @@ -468,7 +496,7 @@ public DockerCompose withEnv(Map<String, String> env) {
}

@Override
public void start() {
public void invoke() {
// bail out early
if (!CommandLine.executableExists(COMPOSE_EXECUTABLE)) {
throw new ContainerLaunchException("Local Docker Compose not found. Is " + COMPOSE_EXECUTABLE + " on the PATH?");
Expand Down
20 changes: 12 additions & 8 deletions core/src/main/java/org/testcontainers/utility/ResourceReaper.java
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,10 @@
import org.slf4j.LoggerFactory;
import org.testcontainers.DockerClientFactory;

import java.util.*;
import java.util.Collections;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.concurrent.ConcurrentHashMap;

/**
Expand Down Expand Up @@ -143,13 +146,6 @@ public void removeNetworks(String identifier) {

private void removeNetwork(String networkName) {
try {
try {
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 this block should do the job when parameter is id, and "list with filter" is not required anymore

Copy link
Member Author

Choose a reason for hiding this comment

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

It could be clearer - I'll do that.
I think we still need to keep the list-with-filter operation though: if the network has already been removed for any reason, dockerClient.removeNetworkCmd will log out an error. So we need to do something to check for the existence of the network first that won't log an error.

// First try to remove by name
dockerClient.removeNetworkCmd(networkName).exec();
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 seems to always fail and causes an error log. I'm just running some checks that this doesn't let any excess containers survive cleanup.

Copy link
Member

Choose a reason for hiding this comment

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

@rnorth ouch. It doesn't fail when recently added Docker Networks support is being used (because we know the IDs), so I wouldn't remove it.

Since we list networks in Docker Compose's implementation, maybe we can use the IDs there as well?

} catch (Exception e) {
LOGGER.trace("Error encountered removing network by name ({}) - it may not have been removed", networkName);
}

List<Network> networks;
try {
// Then try to list all networks with the same name
Expand All @@ -172,4 +168,12 @@ private void removeNetwork(String networkName) {
registeredNetworks.remove(networkName);
}
}

public void unregisterNetwork(String identifier) {
registeredNetworks.remove(identifier);
}

public void unregisterContainer(String identifier) {
registeredContainers.remove(identifier);
}
}