From ea58dab95e03e89ab518cbff8d9eee4200452b6e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Pawe=C5=82=20Gronowski?= <pawel.gronowski@docker.com>
Date: Mon, 26 Aug 2024 14:46:26 +0200
Subject: [PATCH] c8d/pull: Keep the replaced image as dangling
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

With graphdrivers, the old image was still kept as a dangling image.
Keep the same behavior with containerd.

Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
(cherry picked from commit db40a6132ba7b1959dc3860612c64a49a85237fe)
Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
---
 daemon/containerd/image_pull.go | 40 ++++++++++++++++++++++++++++++---
 integration/image/pull_test.go  | 40 +++++++++++++++++++++++++++++++++
 2 files changed, 77 insertions(+), 3 deletions(-)

diff --git a/daemon/containerd/image_pull.go b/daemon/containerd/image_pull.go
index d3479a815d..b5de8e3679 100644
--- a/daemon/containerd/image_pull.go
+++ b/daemon/containerd/image_pull.go
@@ -10,6 +10,7 @@ import (
 
 	"github.com/containerd/containerd"
 	"github.com/containerd/containerd/images"
+	"github.com/containerd/containerd/leases"
 	"github.com/containerd/containerd/pkg/snapshotters"
 	"github.com/containerd/containerd/remotes/docker"
 	cerrdefs "github.com/containerd/errdefs"
@@ -79,10 +80,43 @@ func (i *ImageService) pullTag(ctx context.Context, ref reference.Named, platfor
 	resolver, _ := i.newResolverFromAuthConfig(ctx, authConfig, ref)
 	opts = append(opts, containerd.WithResolver(resolver))
 
-	old, err := i.resolveDescriptor(ctx, ref.String())
+	oldImage, err := i.resolveImage(ctx, ref.String())
 	if err != nil && !errdefs.IsNotFound(err) {
 		return err
 	}
+
+	// Will be set to the new image after pull succeeds.
+	var outNewImg *containerd.Image
+
+	if oldImage.Target.Digest != "" {
+		// Lease the old image content to prevent it from being garbage collected until we keep it as dangling image.
+		lm := i.client.LeasesService()
+		lease, err := lm.Create(ctx, leases.WithRandomID())
+		if err != nil {
+			return errdefs.System(fmt.Errorf("failed to create lease: %w", err))
+		}
+
+		err = leaseContent(ctx, i.content, lm, lease, oldImage.Target)
+		if err != nil {
+			return errdefs.System(fmt.Errorf("failed to lease content: %w", err))
+		}
+
+		// If the pulled image is different than the old image, we will keep the old image as a dangling image.
+		defer func() {
+			if outNewImg != nil {
+				img := *outNewImg
+				if img.Target().Digest != oldImage.Target.Digest {
+					if err := i.ensureDanglingImage(ctx, oldImage); err != nil {
+						log.G(ctx).WithError(err).Warn("failed to keep the previous image as dangling")
+					}
+				}
+			}
+			if err := lm.Delete(ctx, lease); err != nil {
+				log.G(ctx).WithError(err).Warn("failed to delete lease")
+			}
+		}()
+	}
+
 	p := platforms.Default()
 	if platform != nil {
 		p = platforms.Only(*platform)
@@ -100,7 +134,6 @@ func (i *ImageService) pullTag(ctx context.Context, ref reference.Named, platfor
 	pp := pullProgress{store: i.content, showExists: true}
 	finishProgress := jobs.showProgress(ctx, out, pp)
 
-	var outNewImg *containerd.Image
 	defer func() {
 		finishProgress()
 
@@ -116,7 +149,8 @@ func (i *ImageService) pullTag(ctx context.Context, ref reference.Named, platfor
 		if outNewImg != nil {
 			img := *outNewImg
 			progress.Message(out, "", "Digest: "+img.Target().Digest.String())
-			writeStatus(out, reference.FamiliarString(ref), old.Digest != img.Target().Digest)
+			newer := oldImage.Target.Digest != img.Target().Digest
+			writeStatus(out, reference.FamiliarString(ref), newer)
 		}
 	}()
 
diff --git a/integration/image/pull_test.go b/integration/image/pull_test.go
index 54ae6b0305..b51eedb68b 100644
--- a/integration/image/pull_test.go
+++ b/integration/image/pull_test.go
@@ -1,6 +1,7 @@
 package image
 
 import (
+	"bytes"
 	"context"
 	"encoding/json"
 	"fmt"
@@ -17,6 +18,7 @@ import (
 	"github.com/containerd/platforms"
 	"github.com/docker/docker/api/types/image"
 	"github.com/docker/docker/errdefs"
+	"github.com/docker/docker/testutil/daemon"
 	"github.com/docker/docker/testutil/registry"
 	"github.com/opencontainers/go-digest"
 	"github.com/opencontainers/image-spec/specs-go"
@@ -195,3 +197,41 @@ func TestImagePullNonExisting(t *testing.T) {
 		})
 	}
 }
+
+func TestImagePullKeepOldAsDangling(t *testing.T) {
+	skip.If(t, testEnv.IsRemoteDaemon, "cannot run daemon when remote daemon")
+	skip.If(t, testEnv.DaemonInfo.OSType == "windows", "Can't run new daemons on Windows")
+
+	ctx := setupTest(t)
+
+	d := daemon.New(t)
+	d.StartWithBusybox(ctx, t)
+	defer d.Cleanup(t)
+
+	apiClient := d.NewClientT(t)
+
+	inspect1, _, err := apiClient.ImageInspectWithRaw(ctx, "busybox:latest")
+	assert.NilError(t, err)
+
+	prevID := inspect1.ID
+
+	t.Log(inspect1)
+
+	assert.NilError(t, apiClient.ImageTag(ctx, "busybox:latest", "alpine:latest"))
+
+	_, err = apiClient.ImageRemove(ctx, "busybox:latest", image.RemoveOptions{})
+	assert.NilError(t, err)
+
+	rc, err := apiClient.ImagePull(ctx, "alpine:latest", image.PullOptions{})
+	assert.NilError(t, err)
+
+	defer rc.Close()
+
+	var b bytes.Buffer
+	_, _ = io.Copy(&b, rc)
+
+	t.Log(b.String())
+
+	_, _, err = apiClient.ImageInspectWithRaw(ctx, prevID)
+	assert.NilError(t, err)
+}