Improve cleanup for docker-compose. (#394)

* Improve cleanup for docker-compose.
Reduce unnecessary cleanup attempts which cause errors to be logged.
docker-compose down is now trusted to have cleanup up properly if it
exits with a 0 status code.

* Swap order of removal of containers vs networks if docker-compose down failed

* Update changelog

* Further improvements following code review

* Reinstate semi-public methods and mark as deprecated
Improve comments to aid clarity in removeNetwork method

Author: rnorth

CHANGELOG.md

@@ -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)

core/src/main/java/org/testcontainers/containers/DockerComposeContainer.java

@@ -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;
@@ -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;
@@ -116,15 +118,13 @@ public void starting(Description description) {
     }
 
     private void pullImages() {
-        getDockerCompose("pull")
-                .start();
+        runWithCompose("pull");
     }
 
 
     private void createServices() {
-        // Start the docker-compose container, which starts up the services
-        getDockerCompose("up -d")
-                .start();
+        // Run the docker-compose container, which starts up the services
+        runWithCompose("up -d");
     }
 
     private void tailChildContainerLogs() {
@@ -137,16 +137,18 @@ private void tailChildContainerLogs() {
         );
     }
 
-    private DockerCompose getDockerCompose(String cmd) {
+    private void runWithCompose(String cmd) {
         final DockerCompose dockerCompose;
         if (localCompose) {
             dockerCompose = new LocalDockerCompose(composeFiles, identifier);
         } else {
             dockerCompose = new ContainerisedDockerCompose(composeFiles, identifier);
         }
-        return dockerCompose
+
+        dockerCompose
                 .withCommand(cmd)
-                .withEnv(env);
+                .withEnv(env)
+                .invoke();
     }
 
     private void applyScaling() {
@@ -157,8 +159,7 @@ private void applyScaling() {
                 sb.append(" ").append(scale.getKey()).append("=").append(scale.getValue());
             }
 
-            getDockerCompose(sb.toString())
-                    .start();
+            runWithCompose(sb.toString());
         }
     }
 
@@ -171,19 +172,18 @@ private void registerContainersForShutdown() {
             containers.forEach(container ->
                     ResourceReaper.instance().registerContainerForCleanup(container.getId(), container.getNames()[0]));
 
-            // Ensure that the default network for this compose environment, if any, is also cleaned up
-            ResourceReaper.instance().registerNetworkForCleanup(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)) {
-                    ResourceReaper.instance().registerNetworkForCleanup(network.getId());
+                    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 = 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);
@@ -240,15 +240,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 {
+                runWithCompose("down -v");
+
+                // 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 before removing the containers
-            ResourceReaper.instance().removeNetworks(identifier);
+                // kill the spawned service containers
+                spawnedContainerIds.forEach(ResourceReaper.instance()::stopAndRemoveContainer);
+
+                // remove the networks after removing the containers
+                spawnedNetworkIds.forEach(ResourceReaper.instance()::removeNetworkById);
+            }
 
-            // kill the spawned service containers
-            spawnedContainerIds.forEach(id -> ResourceReaper.instance().stopAndRemoveContainer(id));
             spawnedContainerIds.clear();
+            spawnedNetworkIds.clear();
         }
     }
 
@@ -372,7 +382,7 @@ interface DockerCompose {
 
     DockerCompose withEnv(Map<String, String> env);
 
-    void start();
+    void invoke();
 
     default void validateFileList(List<File> composeFiles) {
         checkNotNull(composeFiles);
@@ -417,7 +427,7 @@ public ContainerisedDockerCompose(List<File> composeFiles, String identifier) {
     }
 
     @Override
-    public void start() {
+    public void invoke() {
         super.start();
 
         this.followOutput(new Slf4jLogConsumer(logger()));
@@ -431,6 +441,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(
+                    "Containerised Docker Compose exited abnormally with code " +
+                            exitCode +
+                            " whilst running command: " +
+                            StringUtils.join(this.getCommandParts(), ' '));
+        }
     }
 }
 
@@ -468,7 +491,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?");

core/src/main/java/org/testcontainers/containers/Network.java

@@ -20,7 +20,7 @@ public interface Network extends AutoCloseable, TestRule {
 
     @Override
     default void close() {
-        ResourceReaper.instance().removeNetworks(getId());
+        ResourceReaper.instance().removeNetworkById(getId());
     }
 
     static Network newNetwork() {
@@ -66,7 +66,7 @@ private String create() {
             }
 
             String id = createNetworkCmd.exec().getId();
-            ResourceReaper.instance().registerNetworkForCleanup(id);
+            ResourceReaper.instance().registerNetworkIdForCleanup(id);
             return id;
         }
 

core/src/main/java/org/testcontainers/utility/ResourceReaper.java

@@ -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;
 
 /**
@@ -127,49 +130,80 @@ private void stopContainer(String containerId, String imageName) {
     /**
      * Register a network to be cleaned up at JVM shutdown.
      *
-     * @param networkName   the image name of the network
+     * @param id   the ID of the network
      */
+    public void registerNetworkIdForCleanup(String id) {
+        registeredNetworks.add(id);
+    }
+
+    /**
+     * @param networkName   the name of the network
+     * @deprecated see {@link ResourceReaper#registerNetworkIdForCleanup(String)}
+     */
+    @Deprecated
     public void registerNetworkForCleanup(String networkName) {
-        registeredNetworks.add(networkName);
+        try {
+            // Try to find the network by name, so that we can register its ID for later deletion
+            dockerClient.listNetworksCmd()
+                    .withNameFilter(networkName)
+                    .exec()
+            .forEach(network -> registerNetworkIdForCleanup(network.getId()));
+        } catch (Exception e) {
+            LOGGER.trace("Error encountered when looking up network (name: {})", networkName);
+        }
+    }
+
+    /**
+     * Removes a network by ID.
+     * @param id
+     */
+    public void removeNetworkById(String id) {
+      removeNetwork(id);
     }
 
     /**
-     * Removes any networks that contain the identifier.
+     * Removes a network by ID.
      * @param identifier
+     * @deprecated see {@link ResourceReaper#removeNetworkById(String)}
      */
+    @Deprecated
     public void removeNetworks(String identifier) {
-      removeNetwork(identifier);
+        removeNetworkById(identifier);
     }
 
-    private void removeNetwork(String networkName) {
+    private void removeNetwork(String id) {
         try {
-            try {
-                // First try to remove by name
-                dockerClient.removeNetworkCmd(networkName).exec();
-            } 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
-                networks = dockerClient.listNetworksCmd().withNameFilter(networkName).exec();
+                // Try to find the network if it still exists
+                // Listing by ID first prevents docker-java logging an error if we just go blindly into removeNetworkCmd
+                networks = dockerClient.listNetworksCmd().withIdFilter(id).exec();
             } catch (Exception e) {
-                LOGGER.trace("Error encountered when looking up network for removal (name: {}) - it may not have been removed", networkName);
+                LOGGER.trace("Error encountered when looking up network for removal (name: {}) - it may not have been removed", id);
                 return;
             }
 
+            // at this point networks should contain either 0 or 1 entries, depending on whether the network exists
+            // using a for loop we essentially treat the network like an optional, only applying the removal if it exists
             for (Network network : networks) {
                 try {
                     dockerClient.removeNetworkCmd(network.getId()).exec();
                     registeredNetworks.remove(network.getId());
-                    LOGGER.debug("Removed network: {}", networkName);
+                    LOGGER.debug("Removed network: {}", id);
                 } catch (Exception e) {
                     LOGGER.trace("Error encountered removing network (name: {}) - it may not have been removed", network.getName());
                 }
             }
         } finally {
-            registeredNetworks.remove(networkName);
+            registeredNetworks.remove(id);
         }
     }
+
+    public void unregisterNetwork(String identifier) {
+        registeredNetworks.remove(identifier);
+    }
+
+    public void unregisterContainer(String identifier) {
+        registeredContainers.remove(identifier);
+    }
 }