Simplify DockerImpl

3 files changed, 30 insertions, 71 deletions

diff --git a/docker-api/src/main/java/com/yahoo/vespa/hosted/dockerapi/Docker.java b/docker-api/src/main/java/com/yahoo/vespa/hosted/dockerapi/Docker.java
index 966b5da9def..8bde491d83b 100644
--- a/docker-api/src/main/java/com/yahoo/vespa/hosted/dockerapi/Docker.java
+++ b/docker-api/src/main/java/com/yahoo/vespa/hosted/dockerapi/Docker.java
@@ -2,6 +2,7 @@
 package com.yahoo.vespa.hosted.dockerapi;
 
 import java.net.InetAddress;
+import java.time.Duration;
 import java.util.List;
 import java.util.Optional;
 
@@ -11,7 +12,8 @@ import java.util.Optional;
  */
 public interface Docker {
     /**
-     * Must be called before any other method. May be called more than once.
+     * Should only be called by non-host-admin. May be called more than once.
+     * TODO: Remove when migration to host-admin is done
      */
     void start();
 
@@ -86,7 +88,7 @@ public interface Docker {
     /**
      * Deletes the local images that are currently not in use by any container and not recently used.
      */
-    void deleteUnusedDockerImages();
+    boolean deleteUnusedDockerImages(List<DockerImage> excludes, Duration minImageAgeToDelete);
 
     /**
      * Execute a command in docker container as $VESPA_USER. Will block until the command is finished.
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 00753f59461..69ab697ea27 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
@@ -53,58 +53,37 @@ import static com.yahoo.vespa.hosted.dockerapi.DockerNetworkCreator.NetworkAddre
 public class DockerImpl implements Docker {
     private static final Logger logger = Logger.getLogger(DockerImpl.class.getName());
 
-    private static final String DOCKER_CUSTOM_MACVLAN_NETWORK_NAME = "vespa-macvlan";
     static final String LABEL_NAME_MANAGEDBY = "com.yahoo.vespa.managedby";
+    private static final String DOCKER_CUSTOM_MACVLAN_NETWORK_NAME = "vespa-macvlan";
     private static final String FRAMEWORK_CONTAINER_PREFIX = "/";
-
-    private final DockerConfig config;
-    private final Optional<DockerImageGarbageCollector> dockerImageGC;
-    private final int secondsToWaitBeforeKilling;
-    private CounterWrapper numberOfDockerDaemonFails;
-    private boolean started = false;
+    private static final int SECONDS_TO_WAIT_BEFORE_KILLING = 10;
 
     private final Object monitor = new Object();
     private final Set<DockerImage> scheduledPulls = new HashSet<>();
 
-    private DockerClient dockerClient;
+    private final DockerClient dockerClient;
+    private final DockerImageGarbageCollector dockerImageGC;
+    private final CounterWrapper numberOfDockerDaemonFails;
 
     @Inject
     public DockerImpl(DockerConfig config, MetricReceiverWrapper metricReceiverWrapper) {
-        this.config = config;
-
-        secondsToWaitBeforeKilling = Optional.ofNullable(config)
-                .map(DockerConfig::secondsToWaitBeforeKillingContainer)
-                .orElse(10);
-
-        dockerImageGC = Optional.ofNullable(config)
-                .map(DockerConfig::imageGCMinTimeToLiveMinutes)
-                .map(Duration::ofMinutes)
-                .map(DockerImageGarbageCollector::new);
-
-        Optional.ofNullable(metricReceiverWrapper).ifPresent(this::setMetrics);
+        this(createDockerClient(config), metricReceiverWrapper);
     }
 
-    // For testing
-    DockerImpl(DockerClient dockerClient) {
-        this(null, null);
+    DockerImpl(DockerClient dockerClient, MetricReceiverWrapper metricReceiver) {
         this.dockerClient = dockerClient;
+        this.dockerImageGC = new DockerImageGarbageCollector(this);
+
+        Dimensions dimensions = new Dimensions.Builder().add("role", "docker").build();
+        numberOfDockerDaemonFails = metricReceiver.declareCounter(MetricReceiverWrapper.APPLICATION_DOCKER, dimensions, "daemon.api_fails");
     }
 
     @Override
     public void start() {
-        if (started) return;
-        started = true;
-
-        if (config != null) {
-            dockerClient = createDockerClient(config);
-
-            if (!config.networkNATed()) {
-                try {
-                    setupDockerNetworkIfNeeded();
-                } catch (Exception e) {
-                    throw new DockerException("Could not setup docker network", e);
-                }
-            }
+        try {
+            setupDockerNetworkIfNeeded();
+        } catch (Exception e) {
+            throw new DockerException("Could not setup docker network", e);
         }
     }
 
@@ -306,7 +285,7 @@ public class DockerImpl implements Docker {
     @Override
     public void stopContainer(ContainerName containerName) {
         try {
-            dockerClient.stopContainerCmd(containerName.asString()).withTimeout(secondsToWaitBeforeKilling).exec();
+            dockerClient.stopContainerCmd(containerName.asString()).withTimeout(SECONDS_TO_WAIT_BEFORE_KILLING).exec();
         } catch (NotFoundException e) {
             throw new ContainerNotFoundException(containerName);
         } catch (NotModifiedException ignored) {
@@ -320,11 +299,6 @@ public class DockerImpl implements Docker {
     @Override
     public void deleteContainer(ContainerName containerName) {
         try {
-            dockerImageGC.ifPresent(imageGC -> {
-                Optional<InspectContainerResponse> inspectResponse = inspectContainerCmd(containerName.asString());
-                inspectResponse.ifPresent(response -> imageGC.updateLastUsedTimeFor(response.getImageId()));
-            });
-
             dockerClient.removeContainerCmd(containerName.asString()).exec();
         } catch (NotFoundException e) {
             throw new ContainerNotFoundException(containerName);
@@ -373,7 +347,7 @@ public class DockerImpl implements Docker {
         return encodedContainerName.substring(FRAMEWORK_CONTAINER_PREFIX.length());
     }
 
-    private List<com.github.dockerjava.api.model.Container> listAllContainers() {
+    List<com.github.dockerjava.api.model.Container> listAllContainers() {
         try {
             return dockerClient.listContainersCmd().withShowAll(true).exec();
         } catch (RuntimeException e) {
@@ -382,7 +356,7 @@ public class DockerImpl implements Docker {
         }
     }
 
-    private List<Image> listAllImages() {
+    List<Image> listAllImages() {
         try {
             return dockerClient.listImagesCmd().withShowAll(true).exec();
         } catch (RuntimeException e) {
@@ -391,7 +365,7 @@ public class DockerImpl implements Docker {
         }
     }
 
-    private void deleteImage(final DockerImage dockerImage) {
+    void deleteImage(DockerImage dockerImage) {
         try {
             dockerClient.removeImageCmd(dockerImage.asString()).exec();
         } catch (NotFoundException ignored) {
@@ -403,13 +377,8 @@ public class DockerImpl implements Docker {
     }
 
     @Override
-    public void deleteUnusedDockerImages() {
-        if (!dockerImageGC.isPresent()) return;
-
-        List<Image> images = listAllImages();
-        List<com.github.dockerjava.api.model.Container> containers = listAllContainers();
-
-        dockerImageGC.get().getUnusedDockerImages(images, containers).forEach(this::deleteImage);
+    public boolean deleteUnusedDockerImages(List<DockerImage> excludes, Duration minImageAgeToDelete) {
+        return dockerImageGC.deleteUnusedDockerImages(excludes, minImageAgeToDelete);
     }
 
     private class ImagePullCallback extends PullImageResultCallback {
@@ -428,10 +397,8 @@ public class DockerImpl implements Docker {
 
         @Override
         public void onComplete() {
-            Optional<InspectImageResponse> image = inspectImage(dockerImage);
-            if (image.isPresent()) { // Download successful, update image GC with the newly downloaded image
+            if (imageIsDownloaded(dockerImage)) {
                 logger.log(LogLevel.INFO, "Download completed: " + dockerImage.asString());
-                dockerImageGC.ifPresent(imageGC -> imageGC.updateLastUsedTimeFor(image.get().getId()));
                 removeScheduledPoll(dockerImage);
             } else {
                 throw new DockerClientException("Could not download image: " + dockerImage);
@@ -476,9 +443,4 @@ public class DockerImpl implements Docker {
         return DockerClientImpl.getInstance(dockerClientConfig)
                 .withDockerCmdExecFactory(dockerFactory);
     }
-
-    void setMetrics(MetricReceiverWrapper metricReceiver) {
-        Dimensions dimensions = new Dimensions.Builder().add("role", "docker").build();
-        numberOfDockerDaemonFails = metricReceiver.declareCounter(MetricReceiverWrapper.APPLICATION_DOCKER, dimensions, "daemon.api_fails");
-    }
 }
diff --git a/docker-api/src/test/java/com/yahoo/vespa/hosted/dockerapi/DockerImplTest.java b/docker-api/src/test/java/com/yahoo/vespa/hosted/dockerapi/DockerImplTest.java
index 13ff45808cf..f1a8d4ef65e 100644
--- a/docker-api/src/test/java/com/yahoo/vespa/hosted/dockerapi/DockerImplTest.java
+++ b/docker-api/src/test/java/com/yahoo/vespa/hosted/dockerapi/DockerImplTest.java
@@ -33,6 +33,10 @@ import static org.mockito.Mockito.when;
  */
 public class DockerImplTest {
 
+    private final DockerClient dockerClient = mock(DockerClient.class);
+    private final MetricReceiverWrapper metricReceiver = new MetricReceiverWrapper(MetricReceiver.nullImplementation);
+    private final DockerImpl docker = new DockerImpl(dockerClient, metricReceiver);
+
     @Test
     public void testExecuteCompletes() {
         final String containerId = "container-id";
@@ -40,8 +44,6 @@ public class DockerImplTest {
         final String execId = "exec-id";
         final int exitCode = 3;
 
-        final DockerClient dockerClient = mock(DockerClient.class);
-
         final ExecCreateCmdResponse response = mock(ExecCreateCmdResponse.class);
         when(response.getId()).thenReturn(execId);
 
@@ -64,7 +66,6 @@ public class DockerImplTest {
         when(state.isRunning()).thenReturn(false);
         when(state.getExitCode()).thenReturn(exitCode);
 
-        final Docker docker = new DockerImpl(dockerClient);
         final ProcessResult result = docker.executeInContainer(new ContainerName(containerId), command);
         assertThat(result.getExitStatus(), is(exitCode));
     }
@@ -87,12 +88,9 @@ public class DockerImplTest {
         PullImageCmd pullImageCmd = mock(PullImageCmd.class);
         when(pullImageCmd.exec(resultCallback.capture())).thenReturn(null);
 
-        final DockerClient dockerClient = mock(DockerClient.class);
         when(dockerClient.inspectImageCmd(image.asString())).thenReturn(imageInspectCmd);
         when(dockerClient.pullImageCmd(eq(image.asString()))).thenReturn(pullImageCmd);
 
-        final DockerImpl docker = new DockerImpl(dockerClient);
-        docker.setMetrics(new MetricReceiverWrapper(MetricReceiver.nullImplementation));
         assertTrue("Should return true, we just scheduled the pull", docker.pullImageAsyncIfNeeded(image));
         assertTrue("Should return true, the pull i still ongoing", docker.pullImageAsyncIfNeeded(image));
 
@@ -114,12 +112,9 @@ public class DockerImplTest {
         PullImageCmd pullImageCmd = mock(PullImageCmd.class);
         when(pullImageCmd.exec(resultCallback.capture())).thenReturn(null);
 
-        final DockerClient dockerClient = mock(DockerClient.class);
         when(dockerClient.inspectImageCmd(image.asString())).thenReturn(imageInspectCmd);
         when(dockerClient.pullImageCmd(eq(image.asString()))).thenReturn(pullImageCmd);
 
-        final DockerImpl docker = new DockerImpl(dockerClient);
-        docker.setMetrics(new MetricReceiverWrapper(MetricReceiver.nullImplementation));
         assertTrue("Should return true, we just scheduled the pull", docker.pullImageAsyncIfNeeded(image));
         assertTrue("Should return true, the pull i still ongoing", docker.pullImageAsyncIfNeeded(image));