Fixed docker container managed labeling

4 files changed, 57 insertions, 71 deletions

diff --git a/docker-api/src/main/java/com/yahoo/vespa/hosted/dockerapi/DockerImpl.java b/docker-api/src/main/java/com/yahoo/vespa/hosted/dockerapi/DockerImpl.java
index 52de3bf91eb..171426da60f 100644
--- a/docker-api/src/main/java/com/yahoo/vespa/hosted/dockerapi/DockerImpl.java
+++ b/docker-api/src/main/java/com/yahoo/vespa/hosted/dockerapi/DockerImpl.java
@@ -6,10 +6,8 @@ import com.github.dockerjava.api.command.ExecCreateCmdResponse;
 import com.github.dockerjava.api.command.ExecStartCmd;
 import com.github.dockerjava.api.command.InspectContainerResponse;
 import com.github.dockerjava.api.command.InspectExecResponse;
-import com.github.dockerjava.api.command.InspectImageResponse;
 import com.github.dockerjava.api.exception.DockerClientException;
 import com.github.dockerjava.api.exception.DockerException;
-import com.github.dockerjava.api.exception.NotFoundException;
 import com.github.dockerjava.api.model.Image;
 import com.github.dockerjava.api.model.Network;
 import com.github.dockerjava.api.model.Statistics;
@@ -48,15 +46,14 @@ import java.util.concurrent.CompletableFuture;
 import java.util.concurrent.TimeUnit;
 import java.util.logging.Logger;
 import java.util.stream.Collectors;
-import java.util.stream.Stream;
 import static com.yahoo.vespa.hosted.dockerapi.DockerNetworkCreator.NetworkAddressInterface;
 
 
 public class DockerImpl implements Docker {
     private static final Logger logger = Logger.getLogger(DockerImpl.class.getName());
 
-    private static final String LABEL_NAME_MANAGEDBY = "com.yahoo.vespa.managedby";
-    private static final String LABEL_VALUE_MANAGEDBY = "node-admin";
+    public static final String LABEL_NAME_MANAGEDBY = "com.yahoo.vespa.managedby";
+    public static final String LABEL_VALUE_MANAGEDBY = "node-admin";
 
     private final int SECONDS_TO_WAIT_BEFORE_KILLING;
     private static final String FRAMEWORK_CONTAINER_PREFIX = "/";
@@ -192,11 +189,10 @@ public class DockerImpl implements Docker {
         return inspectImage(dockerImage).isPresent();
     }
 
-    private Optional<InspectImageResponse> inspectImage(DockerImage dockerImage) {
+    private Optional<Image> inspectImage(DockerImage dockerImage) {
         try {
-            return Optional.of(dockerClient.inspectImageCmd(dockerImage.asString()).exec());
-        } catch (NotFoundException e) {
-            return Optional.empty();
+            return dockerClient.listImagesCmd().withShowAll(true)
+                    .withImageNameFilter(dockerImage.asString()).exec().stream().findFirst();
         } catch (DockerException e) {
             numberOfDockerDaemonFails.add();
             throw new RuntimeException("Failed to list image name: '" + dockerImage + "'", e);
@@ -205,8 +201,7 @@ public class DockerImpl implements Docker {
 
     @Override
     public CreateContainerCommand createContainerCommand(DockerImage image, ContainerName name, String hostName) {
-        return new CreateContainerCommandImpl(dockerClient, image, name, hostName)
-                .withLabel(LABEL_NAME_MANAGEDBY, LABEL_VALUE_MANAGEDBY);
+        return new CreateContainerCommandImpl(dockerClient, image, name, hostName);
     }
 
     @Override
@@ -275,7 +270,7 @@ public class DockerImpl implements Docker {
 
     @Override
     public void startContainer(ContainerName containerName) {
-        Optional<com.github.dockerjava.api.model.Container> dockerContainer = getContainerFromName(containerName, true);
+        Optional<com.github.dockerjava.api.model.Container> dockerContainer = getContainerFromName(containerName);
         if (dockerContainer.isPresent()) {
             try {
                 dockerClient.startContainerCmd(dockerContainer.get().getId()).exec();
@@ -289,7 +284,7 @@ public class DockerImpl implements Docker {
 
     @Override
     public void stopContainer(final ContainerName containerName) {
-        Optional<com.github.dockerjava.api.model.Container> dockerContainer = getContainerFromName(containerName, true);
+        Optional<com.github.dockerjava.api.model.Container> dockerContainer = getContainerFromName(containerName);
         if (dockerContainer.isPresent()) {
             try {
                 dockerClient.stopContainerCmd(dockerContainer.get().getId()).withTimeout(SECONDS_TO_WAIT_BEFORE_KILLING).exec();
@@ -302,7 +297,7 @@ public class DockerImpl implements Docker {
 
     @Override
     public void deleteContainer(ContainerName containerName) {
-        Optional<com.github.dockerjava.api.model.Container> dockerContainer = getContainerFromName(containerName, true);
+        Optional<com.github.dockerjava.api.model.Container> dockerContainer = getContainerFromName(containerName);
         if (dockerContainer.isPresent()) {
             dockerImageGC.ifPresent(imageGC -> imageGC.updateLastUsedTimeFor(dockerContainer.get().getImageId()));
 
@@ -316,35 +311,38 @@ public class DockerImpl implements Docker {
         }
     }
 
-    @Override
-    public List<Container> getAllManagedContainers() {
+    List<Container> getAllContainers(boolean managedOnly) {
         try {
             return dockerClient.listContainersCmd().withShowAll(true).exec().stream()
-                    .filter(this::isManaged)
-                    .flatMap(this::asContainer)
+                    .filter(container -> !managedOnly || isManaged(container))
+                    .map(this::asContainer)
                     .collect(Collectors.toList());
         } catch (DockerException e) {
             numberOfDockerDaemonFails.add();
-            throw new RuntimeException("Could not retrieve all container", e);
+            throw new RuntimeException("Could not retrieve all containers", e);
         }
     }
 
     @Override
+    public List<Container> getAllManagedContainers() {
+        return getAllContainers(true);
+    }
+
+    @Override
     public Optional<Container> getContainer(String hostname) {
-        // TODO Don't rely on getAllManagedContainers
-        return getAllManagedContainers().stream()
+        return getAllContainers(false).stream()
                 .filter(c -> Objects.equals(hostname, c.hostname))
                 .findFirst();
     }
 
-    private Stream<Container> asContainer(com.github.dockerjava.api.model.Container dockerClientContainer) {
+    private Container asContainer(com.github.dockerjava.api.model.Container dockerClientContainer) {
         try {
             final InspectContainerResponse response = dockerClient.inspectContainerCmd(dockerClientContainer.getId()).exec();
-            return Stream.of(new Container(
+            return new Container(
                     response.getConfig().getHostName(),
                     new DockerImage(dockerClientContainer.getImage()),
                     new ContainerName(decode(response.getName())),
-                    response.getState().getRunning()));
+                    response.getState().getRunning());
         } catch (DockerException e) {
             numberOfDockerDaemonFails.add();
             //TODO: do proper exception handling
@@ -353,11 +351,9 @@ public class DockerImpl implements Docker {
     }
 
 
-    private Optional<com.github.dockerjava.api.model.Container> getContainerFromName(
-            final ContainerName containerName, final boolean alsoGetStoppedContainers) {
+    private Optional<com.github.dockerjava.api.model.Container> getContainerFromName(final ContainerName containerName) {
         try {
-            return dockerClient.listContainersCmd().withShowAll(alsoGetStoppedContainers).exec().stream()
-                    .filter(this::isManaged)
+            return dockerClient.listContainersCmd().withShowAll(true).exec().stream()
                     .filter(container -> matchName(container, containerName.asString()))
                     .findFirst();
         } catch (DockerException e) {
@@ -396,8 +392,8 @@ public class DockerImpl implements Docker {
     public void buildImage(File dockerfile, DockerImage image) {
         try {
             dockerClient.buildImageCmd(dockerfile).withTag(image.asString())
-                    .exec(new BuildImageResultCallback()).awaitCompletion();
-        } catch (DockerException | InterruptedException e) {
+                    .exec(new BuildImageResultCallback()).awaitImageId();
+        } catch (DockerException e) {
             numberOfDockerDaemonFails.add();
             throw new RuntimeException("Failed to build image " + image.asString(), e);
         }
@@ -428,9 +424,9 @@ public class DockerImpl implements Docker {
 
         @Override
         public void onComplete() {
-            Optional<InspectImageResponse> inspectImage = inspectImage(dockerImage);
-            if (inspectImage.isPresent()) { // Download successful, update image GC with the newly downloaded image
-                dockerImageGC.ifPresent(imageGC -> imageGC.updateLastUsedTimeFor(inspectImage.get().getId()));
+            Optional<Image> image = inspectImage(dockerImage);
+            if (image.isPresent()) { // Download successful, update image GC with the newly downloaded image
+                dockerImageGC.ifPresent(imageGC -> imageGC.updateLastUsedTimeFor(image.get().getId()));
                 removeScheduledPoll(dockerImage).complete(dockerImage);
             } else {
                 removeScheduledPoll(dockerImage).completeExceptionally(
diff --git a/docker-api/src/main/java/com/yahoo/vespa/hosted/dockerapi/DockerTestUtils.java b/docker-api/src/main/java/com/yahoo/vespa/hosted/dockerapi/DockerTestUtils.java
index 30766392e01..ecee17f1b3b 100644
--- a/docker-api/src/main/java/com/yahoo/vespa/hosted/dockerapi/DockerTestUtils.java
+++ b/docker-api/src/main/java/com/yahoo/vespa/hosted/dockerapi/DockerTestUtils.java
@@ -10,6 +10,17 @@ import java.io.IOException;
 import java.util.concurrent.ExecutionException;
 
 /**
+ * Helper class for testing full integration with docker daemon, requires running daemon. To run these tests:
+ *
+ * MAC:
+ *   1. Install Docker Toolbox, and start it (Docker Quickstart Terminal) (you can close terminal window afterwards)
+ *   2. For network test, we need to make docker containers visible for Mac: sudo route add 172.18.0.0/16 192.168.99.100
+ *
+ * GENERAL TIPS:
+ *   For cleaning up your local docker machine (DON'T DO THIS ON PROD)
+ *     docker stop $(docker ps -a -q)
+ *     docker rm $(docker ps -a -q)
+ *
  * @author freva
  */
 public class DockerTestUtils {
@@ -28,7 +39,7 @@ public class DockerTestUtils {
     public static boolean dockerDaemonIsPresent() {
         if (docker != null) return true;
         if (operatingSystem == OS.Unsupported) {
-            System.out.println("This test does not support " + System.getProperty("os.name") + " yet, ignoring test.");
+            System.err.println("This test does not support " + System.getProperty("os.name") + " yet, ignoring test.");
             return false;
         }
 
diff --git a/docker-api/src/test/java/com/yahoo/vespa/hosted/dockerapi/DockerImageGarbageCollectionTest.java b/docker-api/src/test/java/com/yahoo/vespa/hosted/dockerapi/DockerImageGarbageCollectionTest.java
index fd2ab6f8b5f..72fe753c82b 100644
--- a/docker-api/src/test/java/com/yahoo/vespa/hosted/dockerapi/DockerImageGarbageCollectionTest.java
+++ b/docker-api/src/test/java/com/yahoo/vespa/hosted/dockerapi/DockerImageGarbageCollectionTest.java
@@ -45,15 +45,6 @@ public class DockerImageGarbageCollectionTest {
     }
 
     @Test
-    public void onlyLeafImageIsUnused() throws Exception {
-        new ImageGcTester(0)
-                .withExistingImages(
-                        ImageBuilder.forId("parent-image"),
-                        ImageBuilder.forId("leaf-image").withParentId("parent-image"))
-                .expectUnusedImages("leaf-image", "parent-image");
-    }
-
-    @Test
     public void multipleUnusedImagesAreIdentified() throws Exception {
         new ImageGcTester(0)
                 .withExistingImages(
diff --git a/docker-api/src/test/java/com/yahoo/vespa/hosted/dockerapi/DockerTest.java b/docker-api/src/test/java/com/yahoo/vespa/hosted/dockerapi/DockerTest.java
index 473de689a13..df911c334d7 100644
--- a/docker-api/src/test/java/com/yahoo/vespa/hosted/dockerapi/DockerTest.java
+++ b/docker-api/src/test/java/com/yahoo/vespa/hosted/dockerapi/DockerTest.java
@@ -9,7 +9,7 @@ import org.junit.Test;
 import java.io.IOException;
 import java.net.InetAddress;
 import java.net.URL;
-import java.util.List;
+import java.util.Optional;
 import java.util.concurrent.ExecutionException;
 
 import static org.hamcrest.CoreMatchers.is;
@@ -19,18 +19,7 @@ import static org.junit.Assert.assertTrue;
 import static org.junit.Assume.assumeTrue;
 
 /**
- * Class for testing full integration with docker daemon, requires running daemon. To run these tests:
- *
- * MAC:
- *   1. Install Docker Toolbox, and start it (Docker Quickstart Terminal) (you can close terminal window afterwards)
- *   2. For network test, we need to make docker containers visible for Mac: sudo route add 172.18.0.0/16 192.168.99.100
- *   2. Run tests from IDE/mvn.
- *
- *
- * TIPS:
- *   For cleaning up your local docker machine (DON'T DO THIS ON PROD)
- *     docker stop $(docker ps -a -q)
- *     docker rm $(docker ps -a -q)
+ * Requires docker daemon, see {@link com.yahoo.vespa.hosted.dockerapi.DockerTestUtils} for more details.
  *
  * @author freva
  * @author dybdahl
@@ -95,24 +84,23 @@ public class DockerTest {
 
     @Test
     public void testContainerCycle() throws IOException, InterruptedException, ExecutionException {
-        ContainerName containerName = new ContainerName(CONTAINER_NAME_DOCKER_TEST_PREFIX + "foo");
-        docker.createContainerCommand(dockerImage, containerName, "hostName1").create();
-        List<Container> managedContainers = docker.getAllManagedContainers();
-        assertThat(managedContainers.size(), is(1));
-        assertThat(managedContainers.get(0).name, is(containerName));
-        assertThat(managedContainers.get(0).isRunning, is(false));
+        final ContainerName containerName = new ContainerName(CONTAINER_NAME_DOCKER_TEST_PREFIX + "foo");
+        final String containerHostname = "hostName1";
+
+        docker.createContainerCommand(dockerImage, containerName, containerHostname).create();
+        Optional<Container> container = docker.getContainer(containerHostname);
+        assertTrue(container.isPresent());
+        assertFalse(container.get().isRunning);
 
         docker.startContainer(containerName);
-        managedContainers = docker.getAllManagedContainers();
-        assertThat(managedContainers.size(), is(1));
-        assertThat(managedContainers.get(0).name, is(containerName));
-        assertThat(managedContainers.get(0).isRunning, is(true));
+        container = docker.getContainer(containerHostname);
+        assertTrue(container.isPresent());
+        assertTrue(container.get().isRunning);
 
         docker.stopContainer(containerName);
-        managedContainers = docker.getAllManagedContainers();
-        assertThat(managedContainers.size(), is(1));
-        assertThat(managedContainers.get(0).name, is(containerName));
-        assertThat(managedContainers.get(0).isRunning, is(false));
+        container = docker.getContainer(containerHostname);
+        assertTrue(container.isPresent());
+        assertFalse(container.get().isRunning);
 
         docker.deleteContainer(containerName);
         assertThat(docker.getAllManagedContainers().isEmpty(), is(true));
@@ -160,7 +148,7 @@ public class DockerTest {
         }
 
         // Clean up any non deleted containers from previous tests
-        docker.getAllManagedContainers().forEach(container -> {
+        docker.getAllContainers(false).forEach(container -> {
             if (container.name.asString().startsWith(CONTAINER_NAME_DOCKER_TEST_PREFIX)) {
                 if (container.isRunning) docker.stopContainer(container.name);
                 docker.deleteContainer(container.name);