Merge pull request #412 from yahoo/freva/docker-image-pull-fix

Fixed docker image pull

2 files changed, 60 insertions, 40 deletions

diff --git a/node-admin/pom.xml b/node-admin/pom.xml
index 5a0083d82a3..84aff0cc4b7 100644
--- a/node-admin/pom.xml
+++ b/node-admin/pom.xml
@@ -152,6 +152,27 @@
             <artifactId>mockito-core</artifactId>
             <scope>test</scope>
         </dependency>
+        <dependency>
+            <groupId>com.fasterxml.jackson.core</groupId>
+            <artifactId>jackson-core</artifactId>
+            <version>2.6.4</version>
+        </dependency>
+        <dependency>
+            <groupId>com.fasterxml.jackson.core</groupId>
+            <artifactId>jackson-annotations</artifactId>
+            <version>2.6.4</version>
+        </dependency>
+        <dependency>
+            <groupId>com.fasterxml.jackson.core</groupId>
+            <artifactId>jackson-databind</artifactId>
+            <version>2.6.4</version>
+            <exclusions>
+                <exclusion>
+                    <artifactId>com.fasterxml.jackson.core</artifactId>
+                    <groupId>jackson-core</groupId>
+                </exclusion>
+            </exclusions>
+        </dependency>
     </dependencies>
 
     <build>
diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/docker/DockerImpl.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/docker/DockerImpl.java
index b454b24db3b..765abe2db06 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/docker/DockerImpl.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/docker/DockerImpl.java
@@ -8,12 +8,14 @@ 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.exception.DockerClientException;
 import com.github.dockerjava.api.exception.DockerException;
 import com.github.dockerjava.api.model.Bind;
 import com.github.dockerjava.api.model.Image;
 import com.github.dockerjava.core.DefaultDockerClientConfig;
 import com.github.dockerjava.core.DockerClientImpl;
 import com.github.dockerjava.core.command.ExecStartResultCallback;
+import com.github.dockerjava.core.command.PullImageResultCallback;
 import com.github.dockerjava.jaxrs.JerseyDockerCmdExecFactory;
 import com.google.common.base.Joiner;
 import com.google.common.io.CharStreams;
@@ -44,8 +46,6 @@ import java.util.Objects;
 import java.util.Optional;
 import java.util.Set;
 import java.util.concurrent.CompletableFuture;
-import java.util.concurrent.ExecutorService;
-import java.util.concurrent.Executors;
 import java.util.concurrent.TimeUnit;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
@@ -65,7 +65,7 @@ public class DockerImpl implements Docker {
 
     private static final String LABEL_NAME_MANAGEDBY = "com.yahoo.vespa.managedby";
     private static final String LABEL_VALUE_MANAGEDBY = "node-admin";
-    private static final Map<String,String> CONTAINER_LABELS = new HashMap<>();
+    private static final Map<String, String> CONTAINER_LABELS = new HashMap<>();
 
     private static final int DOCKER_MAX_PER_ROUTE_CONNECTIONS = 10;
     private static final int DOCKER_MAX_TOTAL_CONNECTIONS = 100;
@@ -96,9 +96,7 @@ public class DockerImpl implements Docker {
             getDefaults().underVespaHome("var/ycore++"),
             getDefaults().underVespaHome("var/zookeeper"));
 
-    private final DockerClient docker;
-
-    private final ExecutorService executor = Executors.newSingleThreadExecutor();
+    final DockerClient docker;
 
     private final Object monitor = new Object();
 
@@ -118,39 +116,17 @@ public class DockerImpl implements Docker {
                 .withCustomSslConfig(new VespaSSLConfig(config))
                 .withApiVersion("1.23")
                 .build())
-            .withDockerCmdExecFactory(
-                    new JerseyDockerCmdExecFactory()
-                    .withMaxPerRouteConnections(DOCKER_MAX_PER_ROUTE_CONNECTIONS)
-                    .withMaxTotalConnections(DOCKER_MAX_TOTAL_CONNECTIONS)
-                    .withConnectTimeout(DOCKER_CONNECT_TIMEOUT_MILLIS)
-                    .withReadTimeout(DOCKER_READ_TIMEOUT_MILLIS)
-            ));
+                .withDockerCmdExecFactory(
+                        new JerseyDockerCmdExecFactory()
+                                .withMaxPerRouteConnections(DOCKER_MAX_PER_ROUTE_CONNECTIONS)
+                                .withMaxTotalConnections(DOCKER_MAX_TOTAL_CONNECTIONS)
+                                .withConnectTimeout(DOCKER_CONNECT_TIMEOUT_MILLIS)
+                                .withReadTimeout(DOCKER_READ_TIMEOUT_MILLIS)
+                ));
     }
 
     @Override
     public CompletableFuture<DockerImage> pullImageAsync(final DockerImage image) {
-        // We define the task before we create the CompletableFuture, to ensure that the local future variable cannot
-        // be accessed by the task, forcing it to always go through removeScheduledPoll() before completing the task.
-        final Runnable task = () -> {
-            try {
-                docker.pullImageCmd(image.asString());
-                removeScheduledPoll(image).complete(image);
-            } catch (DockerException e) {
-                if (imageIsDownloaded(image)) {
-                    /* TODO: the docker client is not in sync with the server protocol causing it to throw
-                     * "java.io.IOException: Stream closed", even if the pull succeeded; thus ignoring here
-                     * MAY NO LONGER APPLY, was written when spotify/docker-client API was used.
-                     */
-                    removeScheduledPoll(image).complete(image);
-                } else {
-                    removeScheduledPoll(image).completeExceptionally(e);
-                }
-            } catch (RuntimeException e) {
-                removeScheduledPoll(image).completeExceptionally(e);
-                throw e;
-            }
-        };
-
         final CompletableFuture<DockerImage> completionListener;
         synchronized (monitor) {
             if (scheduledPulls.containsKey(image)) {
@@ -159,7 +135,7 @@ public class DockerImpl implements Docker {
             completionListener = new CompletableFuture<>();
             scheduledPulls.put(image, completionListener);
         }
-        executor.submit(task);
+        docker.pullImageCmd(image.asString()).exec(new ImagePullCallback(image));
         return completionListener;
     }
 
@@ -414,15 +390,14 @@ public class DockerImpl implements Docker {
         try {
             return docker.listContainersCmd().withShowAll(alsoGetStoppedContainers).exec().stream()
                     .filter(this::isManaged)
-                    .filter(container -> matchName(container, containerName.asString())).
-                    findFirst();
+                    .filter(container -> matchName(container, containerName.asString()))
+                    .findFirst();
         } catch (DockerException e) {
             throw new RuntimeException("Failed to get container from name", e);
         }
     }
 
 
-
     private boolean isManaged(final com.github.dockerjava.api.model.Container container) {
         final Map<String, String> labels = container.getLabels();
         if (labels == null) {
@@ -549,4 +524,28 @@ public class DockerImpl implements Docker {
                     + " }";
         }
     }
-}
+
+    private class ImagePullCallback extends PullImageResultCallback {
+        private final DockerImage dockerImage;
+
+        ImagePullCallback(DockerImage dockerImage) {
+            this.dockerImage = dockerImage;
+        }
+
+        @Override
+        public void onError(Throwable throwable) {
+            removeScheduledPoll(dockerImage).completeExceptionally(throwable);
+        }
+
+
+        @Override
+        public void onComplete() {
+            if (imageIsDownloaded(dockerImage)) {
+                removeScheduledPoll(dockerImage).complete(dockerImage);
+            } else {
+                removeScheduledPoll(dockerImage).completeExceptionally(
+                        new DockerClientException("Could not download image: " + dockerImage));
+            }
+        }
+    }
+}
\ No newline at end of file