Stop using DockerImage in garabage collector

3 files changed, 31 insertions, 34 deletions

diff --git a/docker-api/src/main/java/com/yahoo/vespa/hosted/dockerapi/DockerEngine.java b/docker-api/src/main/java/com/yahoo/vespa/hosted/dockerapi/DockerEngine.java
index 1f2a35a2a38..0322059745d 100644
--- a/docker-api/src/main/java/com/yahoo/vespa/hosted/dockerapi/DockerEngine.java
+++ b/docker-api/src/main/java/com/yahoo/vespa/hosted/dockerapi/DockerEngine.java
@@ -1,4 +1,4 @@
-// Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
+// Copyright Verizon Media. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 package com.yahoo.vespa.hosted.dockerapi;
 
 import com.github.dockerjava.api.DockerClient;
@@ -41,6 +41,7 @@ import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.TimeUnit;
 import java.util.logging.Level;
 import java.util.logging.Logger;
+import java.util.stream.Collectors;
 import java.util.stream.Stream;
 
 public class DockerEngine implements ContainerEngine {
@@ -320,20 +321,21 @@ public class DockerEngine implements ContainerEngine {
         }
     }
 
-    void deleteImage(DockerImage dockerImage) {
+    void deleteImage(String imageReference) {
         try {
-            dockerClient.removeImageCmd(dockerImage.asString()).exec();
+            dockerClient.removeImageCmd(imageReference).exec();
         } catch (NotFoundException ignored) {
             // Image was already deleted, ignore
         } catch (RuntimeException e) {
             numberOfDockerApiFails.increment();
-            throw new DockerException("Failed to delete docker image " + dockerImage.asString(), e);
+            throw new DockerException("Failed to delete image by reference '" + imageReference + "'", e);
         }
     }
 
     @Override
     public boolean deleteUnusedDockerImages(List<DockerImage> excludes, Duration minImageAgeToDelete) {
-        return dockerImageGC.deleteUnusedDockerImages(excludes, minImageAgeToDelete);
+        List<String> excludedRefs = excludes.stream().map(DockerImage::asString).collect(Collectors.toList());
+        return dockerImageGC.deleteUnusedDockerImages(excludedRefs, minImageAgeToDelete);
     }
 
     private class ImagePullCallback extends PullImageResultCallback {
diff --git a/docker-api/src/main/java/com/yahoo/vespa/hosted/dockerapi/DockerImageGarbageCollector.java b/docker-api/src/main/java/com/yahoo/vespa/hosted/dockerapi/DockerImageGarbageCollector.java
index b0bd1e3c65a..0560d84577a 100644
--- a/docker-api/src/main/java/com/yahoo/vespa/hosted/dockerapi/DockerImageGarbageCollector.java
+++ b/docker-api/src/main/java/com/yahoo/vespa/hosted/dockerapi/DockerImageGarbageCollector.java
@@ -1,11 +1,10 @@
-// Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
+// Copyright Verizon Media. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 package com.yahoo.vespa.hosted.dockerapi;
 
 import com.github.dockerjava.api.model.Container;
 import com.github.dockerjava.api.model.Image;
 import com.google.common.base.Strings;
 import com.yahoo.collections.Pair;
-import com.yahoo.config.provision.DockerImage;
 
 import java.time.Clock;
 import java.time.Duration;
@@ -68,11 +67,11 @@ class DockerImageGarbageCollector {
     /**
      * This method must be called frequently enough to see all containers to know which images are being used
      *
-     * @param excludes List of images (by tag or id) that should not be deleted regardless of their used status
+     * @param excludes List of image references (tag or id) that should not be deleted regardless of their used status
      * @param minImageAgeToDelete Minimum duration after which an image can be removed if it has not been used
      * @return true iff at least 1 image was deleted
      */
-    boolean deleteUnusedDockerImages(List<DockerImage> excludes, Duration minImageAgeToDelete) {
+    boolean deleteUnusedDockerImages(List<String> excludes, Duration minImageAgeToDelete) {
         List<Image> images = docker.listAllImages();
         List<Container> containers = docker.listAllContainers();
 
@@ -100,7 +99,7 @@ class DockerImageGarbageCollector {
         Set<String> imagesToKeep = Stream
                 .concat(
                         getRecentlyUsedImageIds(images, containers, minImageAgeToDelete).stream(), // 1
-                        dockerImageToImageIds(excludes, images).stream()) // 2
+                        referencesToImages(excludes, images).stream()) // 2
                 .flatMap(imageId -> ancestorsByImageId.getOrDefault(imageId, Collections.emptySet()).stream()) // 3
                 .collect(Collectors.toSet());
 
@@ -127,8 +126,8 @@ class DockerImageGarbageCollector {
                 .peek(image -> {
                     // Deleting an image by image ID with multiple tags will fail -> delete by tags instead
                     referencesOf(image).forEach(imageReference -> {
-                        logger.info("Deleting unused docker image " + imageReference);
-                        docker.deleteImage(DockerImage.fromString(imageReference));
+                        logger.info("Deleting unused image " + imageReference);
+                        docker.deleteImage(imageReference);
                     });
                     lastTimeUsedByImageId.remove(image.getId());
                 })
@@ -152,20 +151,20 @@ class DockerImageGarbageCollector {
     }
 
     /**
-     * Attemps to make dockerImages which may be image tags or image ids to image ids. This only works
-     * if the given tag is actually present locally. This is fine, because if it isn't - we can't delete
+     * Map given references (image tags or ids) to images.
+     *
+     * This only works if the given tag is actually present locally. This is fine, because if it isn't - we can't delete
      * it, so no harm done.
      */
-    private Set<String> dockerImageToImageIds(List<DockerImage> dockerImages, List<Image> images) {
+    private Set<String> referencesToImages(List<String> references, List<Image> images) {
         Map<String, String> imageIdByImageTag = images.stream()
                 .flatMap(image -> referencesOf(image).stream()
                                                      .map(repoTag -> new Pair<>(repoTag, image.getId())))
                 .collect(Collectors.toMap(Pair::getFirst, Pair::getSecond));
 
-        return dockerImages.stream()
-                .map(DockerImage::asString)
-                .map(tag -> imageIdByImageTag.getOrDefault(tag, tag))
-                .collect(Collectors.toSet());
+        return references.stream()
+                         .map(ref -> imageIdByImageTag.getOrDefault(ref, ref))
+                         .collect(Collectors.toUnmodifiableSet());
     }
 
     /**
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 a5dfd91051f..c725b0642c9 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
@@ -1,11 +1,10 @@
-// Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
+// Copyright Verizon Media. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 package com.yahoo.vespa.hosted.dockerapi;
 
 import com.fasterxml.jackson.annotation.JsonProperty;
 import com.fasterxml.jackson.core.JsonProcessingException;
 import com.fasterxml.jackson.databind.ObjectMapper;
 import com.github.dockerjava.api.model.Image;
-import com.yahoo.config.provision.DockerImage;
 import com.yahoo.test.ManualClock;
 import org.junit.Test;
 
@@ -147,7 +146,7 @@ public class DockerImageGarbageCollectionTest {
                     ImageBuilder.forId("image-2").withParentId("parent-image"))
                 // Normally, image-1 and parent-image should also be deleted, but because we exclude image-1
                 // we cannot delete parent-image, so only image-2 is deleted
-                .expectDeletedImages(Collections.singletonList("image-1"), "image-2");
+                .expectDeletedImages(List.of("image-1"), "image-2");
     }
 
     /** Same as in {@link #doesNotDeleteExcludedByIdImages()} but with tags */
@@ -157,7 +156,7 @@ public class DockerImageGarbageCollectionTest {
                     ImageBuilder.forId("parent-image").withTags("rhel-6"),
                     ImageBuilder.forId("image-1").withParentId("parent-image").withTags("vespa:6.288.16"),
                     ImageBuilder.forId("image-2").withParentId("parent-image").withTags("vespa:6.289.94"))
-                .expectDeletedImages(Collections.singletonList("vespa:6.288.16"), "vespa:6.289.94");
+                .expectDeletedImages(List.of("vespa:6.288.16"), "vespa:6.289.94");
     }
 
     @Test
@@ -166,14 +165,14 @@ public class DockerImageGarbageCollectionTest {
                     ImageBuilder.forId("parent-image").withTags("rhel-6"),
                     ImageBuilder.forId("image-1").withParentId("parent-image").withTags("vespa:6.288.16"),
                     ImageBuilder.forId("image-2").withParentId("parent-image").withTags("vespa:6.289.94"))
-                .expectDeletedImages(Collections.singletonList("vespa:6.300.1"), "vespa:6.288.16", "vespa:6.289.94", "rhel-6");
+                .expectDeletedImages(List.of("vespa:6.300.1"), "vespa:6.288.16", "vespa:6.289.94", "rhel-6");
     }
 
     private class ImageGcTester {
         private final DockerEngine docker = mock(DockerEngine.class);
         private final ManualClock clock = new ManualClock();
         private final DockerImageGarbageCollector imageGC = new DockerImageGarbageCollector(docker, clock);
-        private final Map<DockerImage, Integer> numDeletes = new HashMap<>();
+        private final Map<String, Integer> numDeletes = new HashMap<>();
         private boolean initialized = false;
 
         private ImageGcTester withExistingImages(ImageBuilder... images) {
@@ -210,17 +209,14 @@ public class DockerImageGarbageCollectionTest {
 
             clock.advance(Duration.ofMinutes(minutesAfter));
 
-            imageGC.deleteUnusedDockerImages(
-                    except.stream().map(DockerImage::fromString).collect(Collectors.toList()),
-                    Duration.ofHours(1).minusSeconds(1));
+            imageGC.deleteUnusedDockerImages(except, Duration.ofHours(1).minusSeconds(1));
 
             Arrays.stream(imageIds)
-                    .map(DockerImage::fromString)
-                    .forEach(image -> {
-                        int newValue = numDeletes.getOrDefault(image, 0) + 1;
-                        numDeletes.put(image, newValue);
-                        verify(docker, times(newValue)).deleteImage(eq(image));
-                    });
+                  .forEach(imageId -> {
+                      int newValue = numDeletes.getOrDefault(imageId, 0) + 1;
+                      numDeletes.put(imageId, newValue);
+                      verify(docker, times(newValue)).deleteImage(eq(imageId));
+                  });
 
             verify(docker, times(numDeletes.values().stream().mapToInt(i -> i).sum())).deleteImage(any());
             return this;