Fix docker image pull retry

5 files changed, 12 insertions, 56 deletions

diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/docker/DockerOperations.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/docker/DockerOperations.java
index 61f31ce9d8d..1d11e221a9b 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/docker/DockerOperations.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/docker/DockerOperations.java
@@ -17,12 +17,9 @@ public interface DockerOperations {
 
     void removeContainer(Container existingContainer);
 
-    // Returns false if image is already downloaded
-    boolean shouldScheduleDownloadOfImage(DockerImage dockerImage);
-
     Optional<Container> getContainer(ContainerName containerName);
 
-    void scheduleDownloadOfImage(ContainerName containerName, DockerImage dockerImage, Runnable callback);
+    boolean pullImageAsyncIfNeeded(DockerImage dockerImage);
 
     ProcessResult executeCommandInContainerAsRoot(ContainerName containerName, String... command);
 
diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/docker/DockerOperationsImpl.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/docker/DockerOperationsImpl.java
index dbf75d0ee03..b21fc573c0f 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/docker/DockerOperationsImpl.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/docker/DockerOperationsImpl.java
@@ -22,7 +22,6 @@ import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.Optional;
-import java.util.concurrent.CompletableFuture;
 import java.util.regex.Pattern;
 import java.util.stream.Stream;
 
@@ -169,12 +168,6 @@ public class DockerOperationsImpl implements DockerOperations {
         docker.deleteContainer(containerName);
     }
 
-    // Returns true if scheduling download
-    @Override
-    public boolean shouldScheduleDownloadOfImage(final DockerImage dockerImage) {
-        return !docker.imageIsDownloaded(dockerImage);
-    }
-
     @Override
     public Optional<Container> getContainer(ContainerName containerName) {
         return docker.getContainer(containerName);
@@ -212,18 +205,8 @@ public class DockerOperationsImpl implements DockerOperations {
     }
 
     @Override
-    public void scheduleDownloadOfImage(ContainerName containerName, DockerImage dockerImage, Runnable callback) {
-        PrefixLogger logger = PrefixLogger.getNodeAgentLogger(DockerOperationsImpl.class, containerName);
-
-        logger.info("Schedule async download of " + dockerImage);
-        final CompletableFuture<DockerImage> asyncPullResult = docker.pullImageAsync(dockerImage);
-        asyncPullResult.whenComplete((image, throwable) -> {
-            if (throwable != null) {
-                logger.warning("Failed to pull " + dockerImage, throwable);
-                return;
-            }
-            callback.run();
-        });
+    public boolean pullImageAsyncIfNeeded(DockerImage dockerImage) {
+        return docker.pullImageAsyncIfNeeded(dockerImage);
     }
 
     ProcessResult executeCommandInContainer(ContainerName containerName, String... command) {
diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentImpl.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentImpl.java
index 9d2198cedcc..0d110adf5a4 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentImpl.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentImpl.java
@@ -352,14 +352,8 @@ public class NodeAgentImpl implements NodeAgent {
     private void scheduleDownLoadIfNeeded(ContainerNodeSpec nodeSpec) {
         if (nodeSpec.currentDockerImage.equals(nodeSpec.wantedDockerImage)) return;
 
-        if (dockerOperations.shouldScheduleDownloadOfImage(nodeSpec.wantedDockerImage.get())) {
-            if (nodeSpec.wantedDockerImage.get().equals(imageBeingDownloaded)) {
-                // Downloading already scheduled, but not done.
-                return;
-            }
+        if (dockerOperations.pullImageAsyncIfNeeded(nodeSpec.wantedDockerImage.get())) {
             imageBeingDownloaded = nodeSpec.wantedDockerImage.get();
-            // Create a signalWorkToBeDone when download is finished.
-            dockerOperations.scheduleDownloadOfImage(containerName, imageBeingDownloaded, this::signalWorkToBeDone);
         } else if (imageBeingDownloaded != null) { // Image was downloading, but now it's ready
             imageBeingDownloaded = null;
         }
diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/DockerMock.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/DockerMock.java
index be7fa4e49c6..dc111251af7 100644
--- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/DockerMock.java
+++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/DockerMock.java
@@ -15,7 +15,6 @@ import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.Optional;
-import java.util.concurrent.CompletableFuture;
 import java.util.stream.Collectors;
 
 /**
@@ -111,31 +110,14 @@ public class DockerMock implements Docker {
     }
 
     @Override
-    public CompletableFuture<DockerImage> pullImageAsync(DockerImage image) {
+    public boolean pullImageAsyncIfNeeded(DockerImage image) {
         synchronized (monitor) {
-            callOrderVerifier.add("pullImageAsync with " + image);
-            final CompletableFuture<DockerImage> completableFuture = new CompletableFuture<>();
-            new Thread() {
-                public void run() {
-                    try {
-                        Thread.sleep(500);
-                        completableFuture.complete(image);
-                    } catch (InterruptedException e) {
-                        e.printStackTrace();
-                    }
-
-                }
-            }.start();
-            return completableFuture;
+            callOrderVerifier.add("pullImageAsyncIfNeeded with " + image);
+            return false;
         }
     }
 
     @Override
-    public boolean imageIsDownloaded(DockerImage image) {
-        return true;
-    }
-
-    @Override
     public void deleteImage(DockerImage dockerImage) {
 
     }
diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentImplTest.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentImplTest.java
index 3ddb28eed77..1c63c70453e 100644
--- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentImplTest.java
+++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentImplTest.java
@@ -114,7 +114,7 @@ public class NodeAgentImplTest {
 
         verify(dockerOperations, never()).removeContainer(any());
         verify(orchestrator, never()).suspend(any(String.class));
-        verify(dockerOperations, never()).scheduleDownloadOfImage(eq(containerName), any(), any());
+        verify(dockerOperations, never()).pullImageAsyncIfNeeded(any());
 
         final InOrder inOrder = inOrder(dockerOperations, orchestrator, nodeRepository);
         // TODO: Verify this isn't run unless 1st time
@@ -147,14 +147,15 @@ public class NodeAgentImplTest {
 
         when(nodeRepository.getContainerNodeSpec(hostName)).thenReturn(Optional.of(nodeSpec));
         when(pathResolver.getApplicationStoragePathForNodeAdmin()).thenReturn(Files.createTempDirectory("foo"));
+        when(dockerOperations.pullImageAsyncIfNeeded(eq(dockerImage))).thenReturn(false);
 
         nodeAgent.converge();
 
         verify(dockerOperations, never()).removeContainer(any());
         verify(orchestrator, never()).suspend(any(String.class));
-        verify(dockerOperations, never()).scheduleDownloadOfImage(eq(containerName), any(), any());
 
         final InOrder inOrder = inOrder(dockerOperations, orchestrator, nodeRepository, aclMaintainer);
+        inOrder.verify(dockerOperations, times(1)).pullImageAsyncIfNeeded(eq(dockerImage));
         inOrder.verify(aclMaintainer, times(1)).run();
         inOrder.verify(dockerOperations, times(1)).startContainer(eq(containerName), eq(nodeSpec));
         inOrder.verify(dockerOperations, times(1)).resumeNode(eq(containerName));
@@ -185,7 +186,7 @@ public class NodeAgentImplTest {
         NodeAgentImpl nodeAgent = makeNodeAgent(dockerImage, true);
 
         when(nodeRepository.getContainerNodeSpec(hostName)).thenReturn(Optional.of(nodeSpec));
-        when(dockerOperations.shouldScheduleDownloadOfImage(any())).thenReturn(true);
+        when(dockerOperations.pullImageAsyncIfNeeded(any())).thenReturn(true);
 
         nodeAgent.converge();
 
@@ -194,8 +195,7 @@ public class NodeAgentImplTest {
         verify(dockerOperations, never()).removeContainer(any());
 
         final InOrder inOrder = inOrder(dockerOperations);
-        inOrder.verify(dockerOperations, times(1)).shouldScheduleDownloadOfImage(eq(newDockerImage));
-        inOrder.verify(dockerOperations, times(1)).scheduleDownloadOfImage(eq(containerName), eq(newDockerImage), any());
+        inOrder.verify(dockerOperations, times(1)).pullImageAsyncIfNeeded(eq(newDockerImage));
     }
 
     @Test