#372 : Fix stopping of wrong containers in parallel executions

The ContainerTracker has been refactored to support PomLabels to only stop containerts stopped by a certain reactor project (identified via its Maven coordinates).
+ unit tests for container tracker.

doc/changelog.md

@@ -2,6 +2,7 @@
 
 * **0.13.9-SNAPSHOT**
   - Check also registry stored with an `https` prefix (#367)
+  - Don't stop containers not started by the project during parallel reactor builds (#372)
 
 * **0.13.8**
   - Add option `nocache` to build configuration ([#348](https://github.com/rhuss/docker-maven-plugin/issues/348))

src/main/java/org/jolokia/docker/maven/StartMojo.java

@@ -63,6 +63,7 @@ public synchronized void executeInternal(final ServiceHub hub) throws DockerAcce
         PortMapping.PropertyWriteHelper portMappingPropertyWriteHelper = new PortMapping.PropertyWriteHelper(portPropertyFile);
 
         boolean success = false;
+        PomLabel pomLabel = getPomLabel();
         try {
             for (StartOrderResolver.Resolvable resolvable : runService.getImagesConfigsInOrder(queryService, getImages())) {
                 final ImageConfiguration imageConfig = (ImageConfiguration) resolvable;
@@ -77,7 +78,7 @@ public synchronized void executeInternal(final ServiceHub hub) throws DockerAcce
                 RunImageConfiguration runConfig = imageConfig.getRunConfiguration();
                 PortMapping portMapping = runService.getPortMapping(runConfig, projProperties);
 
-                String containerId = runService.createAndStartContainer(imageConfig, portMapping, getPomLabel(), projProperties);
+                String containerId = runService.createAndStartContainer(imageConfig, portMapping, pomLabel, projProperties);
 
                 if (showLogs(imageConfig)) {
                     dispatcher.trackContainerLog(containerId,
@@ -109,7 +110,7 @@ public synchronized void executeInternal(final ServiceHub hub) throws DockerAcce
         } finally {
             if (!success) {
                 log.error("Error occurred during container startup, shutting down...");
-                runService.stopStartedContainers(keepContainer, removeVolumes);
+                runService.stopStartedContainers(keepContainer, removeVolumes, pomLabel);
             }
         }
     }

src/main/java/org/jolokia/docker/maven/StopMojo.java

@@ -41,12 +41,14 @@ public class StopMojo extends AbstractDockerMojo {
     protected void executeInternal(ServiceHub hub) throws MojoExecutionException, DockerAccessException {
         QueryService queryService = hub.getQueryService();
         RunService runService = hub.getRunService();
+
+        PomLabel pomLabel = getPomLabel();
 
         if (!keepRunning) {
             if (invokedTogetherWithDockerStart()) {
-                runService.stopStartedContainers(keepContainer, removeVolumes);
+                runService.stopStartedContainers(keepContainer, removeVolumes, pomLabel);
             } else {
-                stopContainers(queryService, runService);
+                stopContainers(queryService, runService, pomLabel);
             }
         }
 
@@ -55,9 +57,7 @@ protected void executeInternal(ServiceHub hub) throws MojoExecutionException, Do
         dispatcher.untrackAllContainerLogs();
     }
 
-    private void stopContainers(QueryService queryService, RunService runService) throws DockerAccessException {
-        PomLabel pomLabel = getPomLabel();
-
+    private void stopContainers(QueryService queryService, RunService runService, PomLabel pomLabel) throws DockerAccessException {
         for (ImageConfiguration image : getImages()) {
             for (Container container : getContainersToStop(queryService, image)) {
                 if (shouldStopContainer(container, pomLabel)) {
@@ -88,7 +88,7 @@ private boolean shouldStopContainer(Container container, PomLabel pomLabel) {
         String key = pomLabel.getKey();
         Map<String, String> labels = container.getLabels();
 
-        return labels.containsKey(key) && pomLabel.matches(new PomLabel(labels.get(key)));
+        return labels.containsKey(key) && pomLabel.equals(new PomLabel(labels.get(key)));
     }
 
     private boolean isStopAllContainers() {

src/main/java/org/jolokia/docker/maven/WatchMojo.java

@@ -238,7 +238,7 @@ private void restartContainer(ServiceHub hub, ImageWatcher watcher) throws Docke
         if (optionalPreStop != null) {
             runService.execInContainer(id, optionalPreStop, watcher.getImageConfiguration());
         }
-        runService.stopContainer(id, false, false);
+        runService.stopPreviouslyStartedContainer(id, false, false);
 
         // Start new one
         watcher.setContainerId(runService.createAndStartContainer(imageConfig, mappedPorts, getPomLabel(), project.getProperties()));

src/main/java/org/jolokia/docker/maven/service/ContainerTracker.java

@@ -1,15 +1,14 @@
 package org.jolokia.docker.maven.service;
 
-import java.util.ArrayList;
-import java.util.Collection;
-import java.util.Collections;
-import java.util.HashMap;
-import java.util.LinkedHashMap;
-import java.util.List;
-import java.util.Map;
+import java.util.*;
 
 import org.jolokia.docker.maven.config.*;
+import org.jolokia.docker.maven.util.PomLabel;
 
+/**
+ * Tracker class for tracking started containers so that they can be shut down at the end when
+ * <code>docker:start</code> and <code>docker:stop</code> are used in the same run
+ */
 public class ContainerTracker {
 
     // Map holding associations between started containers and their images via name and aliases
@@ -19,46 +18,124 @@ public class ContainerTracker {
     // Key: Alias, Value: container
     private final Map<String, String> aliasToContainerMap = new HashMap<>();
 
-    // Action to be used when doing a shutdown
-    private final Map<String,ContainerShutdownDescriptor> shutdownDescriptorMap = new LinkedHashMap<>();
+    // Maps holding actions to be used when doing a shutdown
+    private final Map<String, ContainerShutdownDescriptor> shutdownDescriptorPerContainerMap = new LinkedHashMap<>();
+    private final Map<PomLabel,List<ContainerShutdownDescriptor>> shutdownDescriptorPerPomLabelMap = new HashMap<>();
 
     /**
-     * Register a container to this tracker
+     * Register a started container to this tracker
      *
      * @param containerId container id to register
      * @param imageConfig configuration of associated image
+     * @param pomLabel pom label to identifying the reactor project where the container was created
      */
-    public void registerContainer(String containerId, ImageConfiguration imageConfig) {
-        shutdownDescriptorMap.put(containerId, new ContainerShutdownDescriptor(imageConfig, containerId));
+    public synchronized void registerContainer(String containerId,
+                                               ImageConfiguration imageConfig,
+                                               PomLabel pomLabel) {
+        ContainerShutdownDescriptor descriptor = new ContainerShutdownDescriptor(imageConfig, containerId);
+        shutdownDescriptorPerContainerMap.put(containerId, descriptor);
+        updatePomLabelMap(pomLabel, descriptor);
         updateImageToContainerMapping(imageConfig, containerId);
     }
-
-    public ContainerShutdownDescriptor getContainerShutdownDescriptor(String containerId) {
-        return shutdownDescriptorMap.get(containerId);
-    }
-
-    public ContainerShutdownDescriptor removeContainerShutdownDescriptor(String containerId) {
-        return shutdownDescriptorMap.remove(containerId);
+
+    /**
+     * Remove a container from this container (if stored) and return its descriptor
+     *
+     * @param containerId id to remove
+     * @return descriptor of the container removed or <code>null</code>
+     */
+    public synchronized ContainerShutdownDescriptor removeContainer(String containerId) {
+        ContainerShutdownDescriptor descriptor = shutdownDescriptorPerContainerMap.remove(containerId);
+        if (descriptor != null) {
+            removeContainerIdFromLookupMaps(containerId);
+            removeDescriptorFromPomLabelMap(descriptor);
+        }
+        return descriptor;
     }
-
-    public String lookupContainer(String lookup) {
+
+    /**
+     * Lookup a container by name or alias from the tracked containers
+     *
+     * @param lookup name or alias of the container to lookup
+     * @return container id found or <code>null</code>
+     */
+    public synchronized String lookupContainer(String lookup) {
         if (aliasToContainerMap.containsKey(lookup)) {
             return aliasToContainerMap.get(lookup);
         }
         return imageToContainerMap.get(lookup);
     }
 
-    public void resetContainers() {
-        shutdownDescriptorMap.clear();
-    }
-
-    public Collection<ContainerShutdownDescriptor> getAllContainerShutdownDescriptors() {
-        List<ContainerShutdownDescriptor> descriptors = new ArrayList<>(shutdownDescriptorMap.values());
-        Collections.reverse(descriptors);
+    /**
+     * Get all shutdown descriptors for a given pom label and remove it from the tracker. The descriptors
+     * are returned in reverse order of their registration.
+     *
+     * If no pom label is given, then all descriptors are returned.
+     *
+     * @param pomLabel the label for which to get the descriptors or <code>null</code> for all descriptors
+     * @return the descriptors for the given label or an empty collection
+     */
+    public synchronized Collection<ContainerShutdownDescriptor> removeShutdownDescriptors(PomLabel pomLabel) {
+        List<ContainerShutdownDescriptor> descriptors;
+        if (pomLabel != null) {
+            descriptors = removeFromPomLabelMap(pomLabel);
+            removeFromPerContainerMap(descriptors);
+        } else {
+            // All entries are requested
+            descriptors = new ArrayList<>(shutdownDescriptorPerContainerMap.values());
+            clearAllMaps();
+        }
 
+        Collections.reverse(descriptors);
         return descriptors;
     }
-
+
+    // ========================================================
+
+    private void updatePomLabelMap(PomLabel pomLabel, ContainerShutdownDescriptor descriptor) {
+        if (pomLabel != null) {
+            List<ContainerShutdownDescriptor> descList = shutdownDescriptorPerPomLabelMap.get(pomLabel);
+            if (descList == null) {
+                descList = new ArrayList<>();
+                shutdownDescriptorPerPomLabelMap.put(pomLabel,descList);
+            }
+            descList.add(descriptor);
+        }
+    }
+
+    private void removeDescriptorFromPomLabelMap(ContainerShutdownDescriptor descriptor) {
+        Iterator<Map.Entry<PomLabel, List<ContainerShutdownDescriptor>>> mapIt = shutdownDescriptorPerPomLabelMap.entrySet().iterator();
+        while(mapIt.hasNext()) {
+            Map.Entry<PomLabel,List<ContainerShutdownDescriptor>> mapEntry = mapIt.next();
+            List descs = mapEntry.getValue();
+            Iterator<ContainerShutdownDescriptor> it = descs.iterator();
+            while (it.hasNext()) {
+                ContainerShutdownDescriptor desc = it.next();
+                if (descriptor.equals(desc)) {
+                    it.remove();
+                }
+            }
+            if (descs.size() == 0) {
+                mapIt.remove();
+            }
+        }
+    }
+
+    private void removeContainerIdFromLookupMaps(String containerId) {
+        removeValueFromMap(imageToContainerMap,containerId);
+        removeValueFromMap(aliasToContainerMap,containerId);
+    }
+
+    private void removeValueFromMap(Map<String, String> map, String value) {
+        Iterator<Map.Entry<String,String>> it = map.entrySet().iterator();
+        while (it.hasNext()) {
+            Map.Entry<String,String> entry = it.next();
+            if (entry.getValue().equals(value)) {
+                it.remove();
+            }
+        }
+    }
+
     private void updateImageToContainerMapping(ImageConfiguration imageConfig, String id) {
         // Register name -> containerId and alias -> name
         imageToContainerMap.put(imageConfig.getName(), id);
@@ -67,6 +144,32 @@ private void updateImageToContainerMapping(ImageConfiguration imageConfig, Strin
         }
     }
 
+    private void removeFromPerContainerMap(List<ContainerShutdownDescriptor> descriptors) {
+        Iterator<Map.Entry<String, ContainerShutdownDescriptor>> it = shutdownDescriptorPerContainerMap.entrySet().iterator();
+        while (it.hasNext()) {
+            Map.Entry<String, ContainerShutdownDescriptor> entry = it.next();
+            if (descriptors.contains(entry.getValue())) {
+                removeContainerIdFromLookupMaps(entry.getKey());
+                it.remove();
+            }
+        }
+    }
+
+    private List<ContainerShutdownDescriptor> removeFromPomLabelMap(PomLabel pomLabel) {
+        List<ContainerShutdownDescriptor> descriptors;
+        descriptors = shutdownDescriptorPerPomLabelMap.remove(pomLabel);
+        if (descriptors == null) {
+            descriptors = new ArrayList<>();
+        } return descriptors;
+    }
+
+    private void clearAllMaps() {
+        shutdownDescriptorPerContainerMap.clear();
+        shutdownDescriptorPerPomLabelMap.clear();
+        imageToContainerMap.clear();
+        aliasToContainerMap.clear();
+    }
+
     // =======================================================
 
     static class ContainerShutdownDescriptor {
@@ -78,17 +181,18 @@ static class ContainerShutdownDescriptor {
         private final String containerId;
 
         // How long to wait after shutdown (in milliseconds)
-        private int shutdownGracePeriod;
+        private final int shutdownGracePeriod;
 
         // How long to wait after stop to kill container (in seconds)
-        private int killGracePeriod;
+        private final int killGracePeriod;
 
         // Command to call before stopping container
         private String preStop;
 
         ContainerShutdownDescriptor(ImageConfiguration imageConfig, String containerId) {
             this.imageConfig = imageConfig;
             this.containerId = containerId;
+
             RunImageConfiguration runConfig = imageConfig.getRunConfiguration();
             WaitConfiguration waitConfig = runConfig != null ? runConfig.getWaitConfiguration() : null;
             this.shutdownGracePeriod = waitConfig != null ? waitConfig.getShutdown() : 0;

src/main/java/org/jolokia/docker/maven/service/RunService.java

@@ -80,6 +80,7 @@ public String execInContainer(String containerId, String command, ImageConfigura
      * @param imageConfig image configuration holding the run information and the image name
      * @param mappedPorts container port mapping
      * @param mavenProps properties to fill in with dynamically assigned ports
+     * @param pomLabel label to tag the started container with
      *
      * @return the container id
      *
@@ -95,7 +96,7 @@ public String createAndStartContainer(ImageConfiguration imageConfig,
         ContainerCreateConfig config = createContainerConfig(imageName, runConfig, mappedPorts, pomLabel, mavenProps);
 
         String id = docker.createContainer(config, containerName);
-        startContainer(imageConfig, id);
+        startContainer(imageConfig, id, pomLabel);
 
         if (mappedPorts.containsDynamicPorts() || mappedPorts.containsDynamicHostIps()) {
             updateMappedPorts(id, mappedPorts);
@@ -123,22 +124,20 @@ public void stopContainer(String containerId,
 
     /**
      * Lookup up whether a certain has been already started and registered. If so, stop it
+     *
      * @param containerId the container to stop
      * @param keepContainer whether to keep container or to remove them after stoppings
      * @param removeVolumes whether to remove volumes after stopping
      *
      * @throws DockerAccessException
      */
-    public void stopContainer(String containerId,
-                              boolean keepContainer,
-                              boolean removeVolumes)
+    public void stopPreviouslyStartedContainer(String containerId,
+                                               boolean keepContainer,
+                                               boolean removeVolumes)
             throws DockerAccessException {
-        synchronized (tracker) {
-            ContainerTracker.ContainerShutdownDescriptor descriptor = tracker.getContainerShutdownDescriptor(containerId);
-            if (descriptor != null) {
-                shutdown(docker, descriptor, log, keepContainer, removeVolumes);
-                tracker.removeContainerShutdownDescriptor(containerId);
-            }
+        ContainerTracker.ContainerShutdownDescriptor descriptor = tracker.removeContainer(containerId);
+        if (descriptor != null) {
+            shutdown(docker, descriptor, log, keepContainer, removeVolumes);
         }
     }
 
@@ -150,13 +149,11 @@ public void stopContainer(String containerId,
      * @throws DockerAccessException if during stopping of a container sth fails
      */
     public void stopStartedContainers(boolean keepContainer,
-                                      boolean removeVolumes)
+                                      boolean removeVolumes,
+                                      PomLabel pomLabel)
             throws DockerAccessException {
-        synchronized (tracker) {
-            for (ContainerTracker.ContainerShutdownDescriptor descriptor : tracker.getAllContainerShutdownDescriptors()) {
-                shutdown(docker, descriptor, log, keepContainer, removeVolumes);
-            }
-            tracker.resetContainers();
+        for (ContainerTracker.ContainerShutdownDescriptor descriptor : tracker.removeShutdownDescriptors(pomLabel)) {
+            shutdown(docker, descriptor, log, keepContainer, removeVolumes);
         }
     }
 
@@ -202,7 +199,7 @@ public void addShutdownHookForStoppingContainers(final boolean keepContainer, fi
             @Override
             public void run() {
                 try {
-                    stopStartedContainers(keepContainer, removeVolumes);
+                    stopStartedContainers(keepContainer, removeVolumes, null);
                 } catch (DockerAccessException e) {
                     log.error("Error while stopping containers: " + e);
                 }
@@ -338,10 +335,10 @@ private String findContainerId(String imageNameOrAlias, boolean checkAllContaine
         return id;
     }
 
-    private void startContainer(ImageConfiguration imageConfig, String id) throws DockerAccessException {
+    private void startContainer(ImageConfiguration imageConfig, String id, PomLabel pomLabel) throws DockerAccessException {
         log.info(imageConfig.getDescription() + ": Start container " + id);
         docker.startContainer(id);
-        tracker.registerContainer(id, imageConfig);
+        tracker.registerContainer(id, imageConfig, pomLabel);
     }
 
     private void updateMappedPorts(String containerId, PortMapping mappedPorts) throws DockerAccessException {
@@ -387,6 +384,7 @@ private void shutdown(DockerAccess access,
             // Remove the container
             access.removeContainer(containerId, removeVolumes);
         }
+
         log.info(descriptor.getDescription() + ": Stop" + (keepContainer ? "" : " and remove") + " container " +
                  containerId.substring(0, 12));
     }