Remove parentId from Image

4 files changed, 46 insertions, 145 deletions

diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/container/image/ContainerImagePruner.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/container/image/ContainerImagePruner.java
index 82a898b4d4b..236a4d6718f 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/container/image/ContainerImagePruner.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/container/image/ContainerImagePruner.java
@@ -1,7 +1,6 @@
 // Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 package com.yahoo.vespa.hosted.node.admin.container.image;
 
-import com.google.common.base.Strings;
 import com.yahoo.collections.Pair;
 import com.yahoo.vespa.hosted.node.admin.component.TaskContext;
 import com.yahoo.vespa.hosted.node.admin.container.ContainerEngine;
@@ -10,12 +9,11 @@ import com.yahoo.vespa.hosted.node.admin.container.PartialContainer;
 import java.time.Clock;
 import java.time.Duration;
 import java.time.Instant;
+import java.util.ArrayList;
 import java.util.Collections;
-import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Objects;
-import java.util.Optional;
 import java.util.Set;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.function.Function;
@@ -78,61 +76,39 @@ public class ContainerImagePruner {
 
         Map<String, Image> imageByImageId = images.stream().collect(Collectors.toMap(Image::id, Function.identity()));
 
-        // Find all the ancestors for every local image id, this includes the image id itself
-        Map<String, Set<String>> ancestorsByImageId = images.stream()
-                                                            .map(Image::id)
-                                                            .collect(Collectors.toMap(
-                                                                    Function.identity(),
-                                                                    imageId -> {
-                                                                        Set<String> ancestors = new HashSet<>();
-                                                                        while (!Strings.isNullOrEmpty(imageId)) {
-                                                                            ancestors.add(imageId);
-                                                                            imageId = Optional.of(imageId).map(imageByImageId::get).flatMap(Image::parentId).orElse(null);
-                                                                        }
-                                                                        return ancestors;
-                                                                    }
-                                                            ));
-
         // The set of images that we want to keep is:
         // 1. The images that were recently used
         // 2. The images that were explicitly excluded
-        // 3. All of the ancestors of from images in 1 & 2
         Set<String> imagesToKeep = Stream
                 .concat(
                         updateRecentlyUsedImageIds(images, containers, minAge).stream(), // 1
                         referencesToImages(excludedRefs, images).stream()) // 2
-                .flatMap(imageId -> ancestorsByImageId.getOrDefault(imageId, Collections.emptySet()).stream()) // 3
                 .collect(Collectors.toSet());
 
         // Now take all the images we have locally
-        return imageByImageId.keySet().stream()
-
-                             // filter out images we want to keep
-                             .filter(imageId -> !imagesToKeep.contains(imageId))
-
-                             // Sort images in an order is safe to delete (children before parents)
-                             .sorted((o1, o2) -> {
-                                 // If image2 is parent of image1, image1 comes before image2
-                                 if (imageIsDescendantOf(imageByImageId, o1, o2)) return -1;
-                                     // If image1 is parent of image2, image2 comes before image1
-                                 else if (imageIsDescendantOf(imageByImageId, o2, o1)) return 1;
-                                     // Otherwise, sort lexicographically by image name (For testing)
-                                 else return o1.compareTo(o2);
-                             })
-
-                             // Map back to image
-                             .map(imageByImageId::get)
-
-                             // Delete image, if successful also remove last usage time to prevent re-download being instantly deleted
-                             .peek(image -> {
-                                 // Deleting an image by image ID with multiple tags will fail -> delete by tags instead
-                                 referencesOf(image).forEach(imageReference -> {
-                                     LOG.info("Deleting unused image " + imageReference);
-                                     containerEngine.removeImage(context, imageReference);
-                                 });
-                                 lastTimeUsedByImageId.remove(image.id());
-                             })
-                             .count() > 0;
+        List<Image> imagesToRemove = imageByImageId.keySet().stream()
+                // filter out images we want to keep
+                .filter(imageId -> !imagesToKeep.contains(imageId))
+                .map(imageByImageId::get)
+                .collect(Collectors.toCollection(ArrayList::new));
+
+        // We cannot delete an image that is referenced by other images as parent. Computing parent image is complicated, see
+        // https://github.com/containers/podman/blob/d7b2f03f8a5d0e3789ac185ea03989463168fb76/vendor/github.com/containers/common/libimage/layer_tree.go#L235:L299
+        // https://github.com/containers/podman/blob/d7b2f03f8a5d0e3789ac185ea03989463168fb76/vendor/github.com/containers/common/libimage/oci.go#L30:L97
+        // In practice, our images do not have any parents on prod machines, so we should be able to delete in any
+        // order. In case we ever do get a parent on a host somehow, we could get stuck if we always attempt to delete
+        // in wrong order, so shuffle first to ensure this eventually converges
+        Collections.shuffle(imagesToRemove);
+
+        imagesToRemove.forEach(image -> {
+            // Deleting an image by image ID with multiple tags will fail -> delete by tags instead
+                    referencesOf(image).forEach(imageReference -> {
+                        LOG.info("Deleting unused image " + imageReference);
+                        containerEngine.removeImage(context, imageReference);
+                    });
+                    lastTimeUsedByImageId.remove(image.id());
+        });
+        return !imagesToRemove.isEmpty();
     }
 
     private Set<String> updateRecentlyUsedImageIds(List<Image> images, List<PartialContainer> containers, Duration minImageAgeToDelete) {
@@ -169,18 +145,6 @@ public class ContainerImagePruner {
     }
 
     /**
-     * @return true if ancestor is a parent or grand-parent or grand-grand-parent, etc. of img
-     */
-    private boolean imageIsDescendantOf(Map<String, Image> imageIdToImage, String img, String ancestor) {
-        while (imageIdToImage.containsKey(img)) {
-            img = imageIdToImage.get(img).parentId().orElse(null);
-            if (img == null) return false;
-            if (ancestor.equals(img)) return true;
-        }
-        return false;
-    }
-
-    /**
      * Returns list of references to given image, preferring image tag(s), if any exist.
      *
      * If image is untagged, its ID is returned instead.
diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/container/image/Image.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/container/image/Image.java
index 5a47e5d335b..0f8e5cc6381 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/container/image/Image.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/container/image/Image.java
@@ -3,7 +3,6 @@ package com.yahoo.vespa.hosted.node.admin.container.image;
 
 import java.util.List;
 import java.util.Objects;
-import java.util.Optional;
 
 /**
  * This represents a container image that exists locally.
@@ -13,12 +12,10 @@ import java.util.Optional;
 public class Image {
 
     private final String id;
-    private final Optional<String> parentId;
     private final List<String> names;
 
-    public Image(String id, Optional<String> parentId, List<String> names) {
+    public Image(String id, List<String> names) {
         this.id = Objects.requireNonNull(id);
-        this.parentId = Objects.requireNonNull(parentId);
         this.names = List.copyOf(Objects.requireNonNull(names));
     }
 
@@ -27,11 +24,6 @@ public class Image {
         return id;
     }
 
-    /** ID of the parent image of this, if any */
-    public Optional<String> parentId() {
-        return parentId;
-    }
-
     /** Names for this image, such as tags or digests */
     public List<String> names() {
         return names;
@@ -42,12 +34,12 @@ public class Image {
         if (this == o) return true;
         if (o == null || getClass() != o.getClass()) return false;
         Image image = (Image) o;
-        return id.equals(image.id) && parentId.equals(image.parentId) && names.equals(image.names);
+        return id.equals(image.id) && names.equals(image.names);
     }
 
     @Override
     public int hashCode() {
-        return Objects.hash(id, parentId, names);
+        return Objects.hash(id, names);
     }
 
     @Override
diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/container/ContainerEngineMock.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/container/ContainerEngineMock.java
index 4507c424c98..21118ff8914 100644
--- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/container/ContainerEngineMock.java
+++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/container/ContainerEngineMock.java
@@ -120,7 +120,7 @@ public class ContainerEngineMock implements ContainerEngine {
     @Override
     public void pullImage(TaskContext context, DockerImage image, RegistryCredentials registryCredentials) {
         String imageId = image.asString();
-        ImageDownload imageDownload = images.computeIfAbsent(imageId, (ignored) -> new ImageDownload(new Image(imageId, Optional.empty(), List.of(imageId))));
+        ImageDownload imageDownload = images.computeIfAbsent(imageId, (ignored) -> new ImageDownload(new Image(imageId, List.of(imageId))));
         if (!asyncImageDownload) {
             imageDownload.complete();
         }
diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/container/image/ContainerImagePrunerTest.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/container/image/ContainerImagePrunerTest.java
index 43d3fe94552..2b62f637474 100644
--- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/container/image/ContainerImagePrunerTest.java
+++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/container/image/ContainerImagePrunerTest.java
@@ -10,17 +10,12 @@ import com.yahoo.vespa.hosted.node.admin.container.ContainerEngineMock;
 import com.yahoo.vespa.hosted.node.admin.container.ContainerId;
 import com.yahoo.vespa.hosted.node.admin.container.ContainerName;
 import com.yahoo.vespa.hosted.node.admin.container.ContainerResources;
-import com.yahoo.vespa.hosted.node.admin.container.image.ContainerImagePruner;
-import com.yahoo.vespa.hosted.node.admin.container.image.Image;
 import org.junit.Test;
 
 import java.time.Duration;
-import java.util.Arrays;
-import java.util.Collections;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
-import java.util.Optional;
 
 import static org.junit.Assert.assertTrue;
 
@@ -54,70 +49,31 @@ public class ContainerImagePrunerTest {
               .expectDeletedImages();
     }
 
-
     @Test
     public void multipleUnusedImagesAreIdentified() {
         tester.withExistingImages(image("image-1"), image("image-2"))
               .expectDeletedImages("image-1", "image-2");
     }
 
-
-    @Test
-    public void multipleUnusedLeavesAreIdentified() {
-        tester.withExistingImages(image("parent-image"),
-                                  image("image-1", "parent-image"),
-                                  image("image-2", "parent-image"))
-              .expectDeletedImages("image-1", "image-2", "parent-image");
-    }
-
-
-    @Test
-    public void unusedLeafWithUsedSiblingIsIdentified() {
-        tester.withExistingImages(image("parent-image"),
-                                  image("image-1", "parent-image", "latest"),
-                                  image("image-2", "parent-image", "1.24"))
-              .withExistingContainers(container("vespa-node-1", "image-1"))
-              .expectDeletedImages("1.24"); // Deleting the only tag will delete the image
-    }
-
-
     @Test
     public void unusedImagesWithMultipleTags() {
-        tester.withExistingImages(image("parent-image"),
-                                  image("image-1", "parent-image", "vespa-6", "vespa-6.28", "vespa:latest"))
-              .expectDeletedImages("vespa-6", "vespa-6.28", "vespa:latest", "parent-image");
+        tester.withExistingImages(image("image-1", "vespa-6", "vespa-6.28", "vespa:latest"))
+              .expectDeletedImages("vespa-6", "vespa-6.28", "vespa:latest");
     }
 
-
     @Test
     public void unusedImagesWithMultipleUntagged() {
-        tester.withExistingImages(image("image1", null, "<none>:<none>"),
-                                  image("image2", null, "<none>:<none>"))
+        tester.withExistingImages(image("image1", "<none>:<none>"),
+                                  image("image2", "<none>:<none>"))
               .expectDeletedImages("image1", "image2");
     }
 
-
     @Test
     public void taggedImageWithNoContainersIsUnused() {
-        tester.withExistingImages(image("image-1", null, "vespa-6"))
+        tester.withExistingImages(image("image-1", "vespa-6"))
               .expectDeletedImages("vespa-6");
     }
 
-
-    @Test
-    public void unusedImagesWithSimpleImageGc() {
-        tester.withExistingImages(image("parent-image"))
-              .expectDeletedImagesAfterMinutes(30)
-              .withExistingImages(image("parent-image"),
-                                    image("image-1", "parent-image"))
-              .expectDeletedImagesAfterMinutes(0)
-              .expectDeletedImagesAfterMinutes(30)
-              // At this point, parent-image has been unused for 1h, but image-1 depends on parent-image and it has
-              // only been unused for 30m, so we cannot delete parent-image yet. 30 mins later both can be removed
-              .expectDeletedImagesAfterMinutes(30, "image-1", "parent-image");
-    }
-
-
     @Test
     public void reDownloadingImageIsNotImmediatelyDeleted() {
         tester.withExistingImages(image("image"))
@@ -127,50 +83,39 @@ public class ContainerImagePrunerTest {
               .expectDeletedImages("image"); // 1h after re-download it is deleted again
     }
 
-
     @Test
     public void reDownloadingImageIsNotImmediatelyDeletedWhenDeletingByTag() {
-        tester.withExistingImages(image("image", null, "image-1", "my-tag"))
-              .expectDeletedImages("image-1", "my-tag") // After 1h we delete image
+        tester.withExistingImages(image("image", "my-tag"))
+              .expectDeletedImages("my-tag") // After 1h we delete image
               .expectDeletedImagesAfterMinutes(0) // image is immediately re-downloaded, but is not deleted
               .expectDeletedImagesAfterMinutes(10)
-              .expectDeletedImages("image-1", "my-tag"); // 1h after re-download it is deleted again
+              .expectDeletedImages("my-tag"); // 1h after re-download it is deleted again
     }
 
     /** Same scenario as in {@link #multipleUnusedImagesAreIdentified()} */
     @Test
     public void doesNotDeleteExcludedByIdImages() {
-        tester.withExistingImages(image("parent-image"),
-                                  image("image-1", "parent-image"),
-                                  image("image-2", "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
+        tester.withExistingImages(image("image-1"), image("image-2"))
+              // Normally, image-1 should also be deleted, but because we exclude image-1 only image-2 is deleted
               .expectDeletedImages(List.of("image-1"), "image-2");
     }
 
     /** Same as in {@link #doesNotDeleteExcludedByIdImages()} but with tags */
     @Test
     public void doesNotDeleteExcludedByTagImages() {
-        tester.withExistingImages(image("parent-image", "rhel-6"),
-                                  image("image-1", "parent-image", "vespa:6.288.16"),
-                                  image("image-2", "parent-image", "vespa:6.289.94"))
+        tester.withExistingImages(image("image-1", "vespa:6.288.16"), image("image-2", "vespa:6.289.94"))
               .expectDeletedImages(List.of("vespa:6.288.16"), "vespa:6.289.94");
     }
 
     @Test
-    public void exludingNotDownloadedImageIsNoop() {
-        tester.withExistingImages(image("parent-image", "rhel-6"),
-                                  image("image-1", "parent-image", "vespa:6.288.16"),
-                                  image("image-2", "parent-image", "vespa:6.289.94"))
+    public void excludingNotDownloadedImageIsNoop() {
+        tester.withExistingImages(image("image-1", "vespa:6.288.16"),
+                                  image("image-2", "vespa:6.289.94"))
               .expectDeletedImages(List.of("vespa:6.300.1"), "vespa:6.288.16", "vespa:6.289.94", "rhel-6");
     }
 
-    private static Image image(String id) {
-        return image(id, null);
-    }
-
-    private static Image image(String id, String parentId, String... tags) {
-        return new Image(id, Optional.ofNullable(parentId), List.of(tags));
+    private static Image image(String id, String... tags) {
+        return new Image(id, List.of(tags));
     }
 
     private static Container container(String name, String imageId) {
@@ -209,7 +154,7 @@ public class ContainerImagePrunerTest {
         }
 
         private Tester expectDeletedImagesAfterMinutes(int minutesAfter, String... imageIds) {
-            return expectDeletedImagesAfterMinutes(minutesAfter, Collections.emptyList(), imageIds);
+            return expectDeletedImagesAfterMinutes(minutesAfter, List.of(), imageIds);
         }
 
         private Tester expectDeletedImagesAfterMinutes(int minutesAfter, List<String> excludedRefs, String... imageIds) {
@@ -223,7 +168,7 @@ public class ContainerImagePrunerTest {
 
             pruner.removeUnusedImages(context, excludedRefs, Duration.ofHours(1).minusSeconds(1));
 
-            Arrays.stream(imageIds)
+            List.of(imageIds)
                   .forEach(imageId -> {
                       int newValue = removalCountByImageId.getOrDefault(imageId, 0) + 1;
                       removalCountByImageId.put(imageId, newValue);