From 62e75d484add5efa87ffecb063d05cdb6a5f3585 Mon Sep 17 00:00:00 2001 From: Valerij Fredriksen Date: Wed, 10 Nov 2021 21:34:14 +0100 Subject: Remove parentId from Image --- .../container/image/ContainerImagePruner.java | 84 ++++++-------------- .../hosted/node/admin/container/image/Image.java | 14 +--- .../node/admin/container/ContainerEngineMock.java | 2 +- .../container/image/ContainerImagePrunerTest.java | 91 +++++----------------- 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 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> ancestorsByImageId = images.stream() - .map(Image::id) - .collect(Collectors.toMap( - Function.identity(), - imageId -> { - Set 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 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 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 updateRecentlyUsedImageIds(List images, List containers, Duration minImageAgeToDelete) { @@ -168,18 +144,6 @@ public class ContainerImagePruner { .collect(Collectors.toUnmodifiableSet()); } - /** - * @return true if ancestor is a parent or grand-parent or grand-grand-parent, etc. of img - */ - private boolean imageIsDescendantOf(Map 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. * 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 parentId; private final List names; - public Image(String id, Optional parentId, List names) { + public Image(String id, List 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 parentId() { - return parentId; - } - /** Names for this image, such as tags or digests */ public List 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, ":"), - image("image2", null, ":")) + tester.withExistingImages(image("image1", ":"), + image("image2", ":")) .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 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); -- cgit v1.2.3