Merge pull request #45839 from vvoland/c8d-dont-prune-used

c8d/prune: Fix images being deleted when they're still used with a different reference

daemon/containerd/image_prune.go

@@ -2,6 +2,7 @@ package containerd
 
 import (
 	"context"
+	"strings"
 
 	cerrdefs "github.com/containerd/containerd/errdefs"
 	containerdimages "github.com/containerd/containerd/images"
@@ -70,35 +71,50 @@ func (i *ImageService) pruneUnused(ctx context.Context, filterFunc imageFilterFu
 		return nil, err
 	}
 
+	// How many images make reference to a particular target digest.
+	digestRefCount := map[digest.Digest]int{}
+	// Images considered for pruning.
 	imagesToPrune := map[string]containerdimages.Image{}
 	for _, img := range allImages {
+		digestRefCount[img.Target.Digest] += 1
+
 		if !danglingOnly || isDanglingImage(img) {
+			canBePruned := filterFunc(img)
+			log.G(ctx).WithFields(logrus.Fields{
+				"image":       img.Name,
+				"canBePruned": canBePruned,
+			}).Debug("considering image for pruning")
+
+			if canBePruned {
 				imagesToPrune[img.Name] = img
 			}
-	}
 
-	// Apply filters
-	for name, img := range imagesToPrune {
-		filteredOut := !filterFunc(img)
-		log.G(ctx).WithFields(logrus.Fields{
-			"image":       name,
-			"filteredOut": filteredOut,
-		}).Debug("filtering image")
-
-		if filteredOut {
-			delete(imagesToPrune, name)
 		}
 	}
 
-	containers := i.containers.List()
+	// Image specified by digests that are used by containers.
+	usedDigests := map[digest.Digest]struct{}{}
 
-	var errs error
 	// Exclude images used by existing containers
-	for _, ctr := range containers {
+	for _, ctr := range i.containers.List() {
+		// If the original image was deleted, make sure we don't delete the dangling image
+		delete(imagesToPrune, danglingImageName(ctr.ImageID.Digest()))
+
 		// Config.Image is the image reference passed by user.
-		// For example: container created by `docker run alpine` will have Image="alpine"
+		// Config.ImageID is the resolved content digest based on the user's Config.Image.
-		// Warning: This doesn't handle truncated ids:
+		// For example: container created by:
-		//          `docker run 124c7d2` will have Image="124c7d270790"
+		//           `docker run alpine` will have Config.Image="alpine"
+		//           `docker run 82d1e9d` will have Config.Image="82d1e9d"
+		// but both will have ImageID="sha256:82d1e9d7ed48a7523bdebc18cf6290bdb97b82302a8a9c27d4fe885949ea94d1"
+		imageDgst := ctr.ImageID.Digest()
+
+		// If user didn't specify an explicit image, mark the digest as used.
+		normalizedImageID := "sha256:" + strings.TrimPrefix(ctr.Config.Image, "sha256:")
+		if strings.HasPrefix(imageDgst.String(), normalizedImageID) {
+			usedDigests[imageDgst] = struct{}{}
+			continue
+		}
+
 		ref, err := reference.ParseNormalizedNamed(ctr.Config.Image)
 		log.G(ctx).WithFields(logrus.Fields{
 			"ctr":          ctr.ID,
@@ -107,12 +123,28 @@ func (i *ImageService) pruneUnused(ctx context.Context, filterFunc imageFilterFu
 		}).Debug("filtering container's image")
 
 		if err == nil {
+			// If user provided a specific image name, exclude that image.
 			name := reference.TagNameOnly(ref)
 			delete(imagesToPrune, name.String())
 		}
 	}
 
+	// Create dangling images for images that will be deleted but are still in use.
+	for _, img := range imagesToPrune {
+		dgst := img.Target.Digest
+
+		digestRefCount[dgst] -= 1
+		if digestRefCount[dgst] == 0 {
+			if _, isUsed := usedDigests[dgst]; isUsed {
+				if err := i.ensureDanglingImage(ctx, img); err != nil {
+					return &report, errors.Wrapf(err, "failed to create ensure dangling image for %s", img.Name)
+				}
+			}
+		}
+	}
+
 	possiblyDeletedConfigs := map[digest.Digest]struct{}{}
+	var errs error
 
 	// Workaround for https://github.com/moby/buildkit/issues/3797
 	defer func() {

daemon/containerd/soft_delete.go

@@ -30,19 +30,12 @@ func (i *ImageService) softImageDelete(ctx context.Context, img containerdimages
 
 	// Create dangling image if this is the last image pointing to this target.
 	if len(imgs) == 1 {
-		danglingImage := img
+		err = i.ensureDanglingImage(context.Background(), img)
-
-		danglingImage.Name = danglingImageName(img.Target.Digest)
-		delete(danglingImage.Labels, containerdimages.AnnotationImageName)
-		delete(danglingImage.Labels, ocispec.AnnotationRefName)
-
-		_, err = is.Create(context.Background(), danglingImage)
 
 		// Error out in case we couldn't persist the old image.
-		// If it already exists, then just continue.
+		if err != nil {
-		if err != nil && !cerrdefs.IsAlreadyExists(err) {
 			return errdefs.System(errors.Wrapf(err, "failed to create a dangling image for the replaced image %s with digest %s",
-				danglingImage.Name, danglingImage.Target.Digest.String()))
+				img.Name, img.Target.Digest.String()))
 		}
 	}
 
@@ -57,6 +50,29 @@ func (i *ImageService) softImageDelete(ctx context.Context, img containerdimages
 	return nil
 }
 
+func (i *ImageService) ensureDanglingImage(ctx context.Context, from containerdimages.Image) error {
+	danglingImage := from
+
+	danglingImage.Labels = make(map[string]string)
+	for k, v := range from.Labels {
+		switch k {
+		case containerdimages.AnnotationImageName, ocispec.AnnotationRefName:
+			// Don't copy name labels.
+		default:
+			danglingImage.Labels[k] = v
+		}
+	}
+	danglingImage.Name = danglingImageName(from.Target.Digest)
+
+	_, err := i.client.ImageService().Create(context.Background(), danglingImage)
+	// If it already exists, then just continue.
+	if cerrdefs.IsAlreadyExists(err) {
+		return nil
+	}
+
+	return err
+}
+
 func danglingImageName(digest digest.Digest) string {
 	return "moby-dangling@" + digest.String()
 }

integration/image/inspect_test.go

@@ -19,12 +19,9 @@ func TestImageInspectEmptyTagsAndDigests(t *testing.T) {
 	client := testEnv.APIClient()
 	ctx := context.Background()
 
-	danglingId := environment.DanglingImageIdGraphDriver
+	danglingID := environment.GetTestDanglingImageId(testEnv)
-	if testEnv.UsingSnapshotter() {
-		danglingId = environment.DanglingImageIdSnapshotter
-	}
 
-	inspect, raw, err := client.ImageInspectWithRaw(ctx, danglingId)
+	inspect, raw, err := client.ImageInspectWithRaw(ctx, danglingID)
 	assert.NilError(t, err)
 
 	// Must be a zero length array, not null.

integration/image/prune_test.go

@@ -0,0 +1,33 @@
+package image
+
+import (
+	"context"
+	"testing"
+
+	"github.com/docker/docker/api/types/filters"
+	"github.com/docker/docker/integration/internal/container"
+	"github.com/docker/docker/testutil/environment"
+	"gotest.tools/v3/assert"
+	is "gotest.tools/v3/assert/cmp"
+	"gotest.tools/v3/skip"
+)
+
+// Regression test for: https://github.com/moby/moby/issues/45732
+func TestPruneDontDeleteUsedDangling(t *testing.T) {
+	skip.If(t, testEnv.DaemonInfo.OSType == "windows", "FIXME: hack/make/.build-empty-images doesn't run on Windows")
+
+	defer setupTest(t)()
+	client := testEnv.APIClient()
+	ctx := context.Background()
+
+	danglingID := environment.GetTestDanglingImageId(testEnv)
+
+	container.Create(ctx, t, client,
+		container.WithImage(danglingID),
+		container.WithCmd("sleep", "60"))
+
+	pruned, err := client.ImagesPrune(ctx, filters.NewArgs(filters.Arg("dangling", "true")))
+
+	assert.NilError(t, err)
+	assert.Check(t, is.Len(pruned.ImagesDeleted, 0))
+}

testutil/environment/special_images.go

@@ -5,3 +5,10 @@ const DanglingImageIdGraphDriver = "sha256:0df1207206e5288f4a989a2f13d1f5b3c4e70
 
 // The containerd image store identifies images by the ID of their manifest/manifest list.
 const DanglingImageIdSnapshotter = "sha256:16d365089e5c10e1673ee82ab5bba38ade9b763296ad918bd24b42a1156c5456"
+
+func GetTestDanglingImageId(testEnv *Execution) string {
+	if testEnv.UsingSnapshotter() {
+		return DanglingImageIdSnapshotter
+	}
+	return DanglingImageIdGraphDriver
+}