Merge pull request #46634 from rumpl/c8d-classic-builder-cache

c8d: make the cache in classic builder work

daemon/containerd/cache.go

@@ -5,89 +5,262 @@ import (
 	"reflect"
 	"strings"
 
+	"github.com/containerd/log"
 	"github.com/docker/docker/api/types/container"
 	imagetype "github.com/docker/docker/api/types/image"
 	"github.com/docker/docker/builder"
+	"github.com/docker/docker/errdefs"
 	"github.com/docker/docker/image"
+	"github.com/opencontainers/go-digest"
+	ocispec "github.com/opencontainers/image-spec/specs-go/v1"
 )
 
 // MakeImageCache creates a stateful image cache.
 func (i *ImageService) MakeImageCache(ctx context.Context, cacheFrom []string) (builder.ImageCache, error) {
 	images := []*image.Image{}
+	if len(cacheFrom) == 0 {
+		return &localCache{
+			imageService: i,
+		}, nil
+	}
+
 	for _, c := range cacheFrom {
-		im, err := i.GetImage(ctx, c, imagetype.GetImageOpts{})
+		h, err := i.ImageHistory(ctx, c)
 		if err != nil {
-			return nil, err
+			continue
+		}
+		for _, hi := range h {
+			if hi.ID != "<missing>" {
+				im, err := i.GetImage(ctx, hi.ID, imagetype.GetImageOpts{})
+				if err != nil {
+					return nil, err
+				}
+				images = append(images, im)
+			}
 		}
-		images = append(images, im)
 	}
-	return &imageCache{images: images, c: i}, nil
+
+	return &imageCache{
+		lc: &localCache{
+			imageService: i,
+		},
+		images:       images,
+		imageService: i,
+	}, nil
 }
 
-type imageCache struct {
-	images []*image.Image
-	c      *ImageService
+type localCache struct {
+	imageService *ImageService
 }
 
-func (ic *imageCache) GetCache(parentID string, cfg *container.Config) (imageID string, err error) {
+func (ic *localCache) GetCache(parentID string, cfg *container.Config) (imageID string, err error) {
 	ctx := context.TODO()
 
+	var children []image.ID
+
+	// FROM scratch
 	if parentID == "" {
-		// TODO handle "parentless" image cache lookups ("FROM scratch")
-		return "", nil
+		c, err := ic.imageService.getImagesWithLabel(ctx, imageLabelClassicBuilderFromScratch, "1")
+		if err != nil {
+			return "", err
+		}
+		children = c
+	} else {
+		c, err := ic.imageService.Children(ctx, image.ID(parentID))
+		if err != nil {
+			return "", err
+		}
+		children = c
 	}
 
-	parent, err := ic.c.GetImage(ctx, parentID, imagetype.GetImageOpts{})
-	if err != nil {
-		return "", err
-	}
+	var match *image.Image
+	for _, child := range children {
+		ccDigestStr, err := ic.imageService.getImageLabelByDigest(ctx, child.Digest(), imageLabelClassicBuilderContainerConfig)
+		if err != nil {
+			return "", err
+		}
+		if ccDigestStr == "" {
+			continue
+		}
 
-	for _, localCachedImage := range ic.images {
-		if isMatch(localCachedImage, parent, cfg) {
-			return localCachedImage.ID().String(), nil
+		dgst, err := digest.Parse(ccDigestStr)
+		if err != nil {
+			log.G(ctx).WithError(err).Warnf("invalid container config digest: %q", ccDigestStr)
+			continue
+		}
+
+		var cc container.Config
+		if err := readConfig(ctx, ic.imageService.content, ocispec.Descriptor{Digest: dgst}, &cc); err != nil {
+			if errdefs.IsNotFound(err) {
+				log.G(ctx).WithError(err).WithField("image", child).Warnf("missing container config: %q", ccDigestStr)
+				continue
+			}
+			return "", err
+		}
+
+		if isMatch(&cc, cfg) {
+			childImage, err := ic.imageService.GetImage(ctx, child.String(), imagetype.GetImageOpts{})
+			if err != nil {
+				return "", err
+			}
+
+			if childImage.Created != nil && (match == nil || match.Created.Before(*childImage.Created)) {
+				match = childImage
+			}
 		}
 	}
 
-	children, err := ic.c.Children(ctx, parent.ID())
+	if match == nil {
+		return "", nil
+	}
+
+	return match.ID().String(), nil
+}
+
+type imageCache struct {
+	images       []*image.Image
+	imageService *ImageService
+	lc           *localCache
+}
+
+func (ic *imageCache) GetCache(parentID string, cfg *container.Config) (imageID string, err error) {
+	ctx := context.TODO()
+
+	imgID, err := ic.lc.GetCache(parentID, cfg)
 	if err != nil {
 		return "", err
 	}
+	if imgID != "" {
+		for _, s := range ic.images {
+			if ic.isParent(ctx, s, image.ID(imgID)) {
+				return imgID, nil
+			}
+		}
+	}
+
+	var parent *image.Image
+	lenHistory := 0
 
-	for _, children := range children {
-		childImage, err := ic.c.GetImage(ctx, children.String(), imagetype.GetImageOpts{})
+	if parentID != "" {
+		parent, err = ic.imageService.GetImage(ctx, parentID, imagetype.GetImageOpts{})
 		if err != nil {
 			return "", err
 		}
-
-		if isMatch(childImage, parent, cfg) {
-			return children.String(), nil
+		lenHistory = len(parent.History)
+	}
+	for _, target := range ic.images {
+		if !isValidParent(target, parent) || !isValidConfig(cfg, target.History[lenHistory]) {
+			continue
 		}
+		return target.ID().String(), nil
 	}
 
 	return "", nil
 }
 
-// isMatch checks whether a given target can be used as cache for the given
-// parent image/config combination.
-// A target can only be an immediate child of the given parent image. For
-// a parent image with `n` history entries, a valid target must have `n+1`
-// entries and the extra entry must match the provided config
-func isMatch(target, parent *image.Image, cfg *container.Config) bool {
-	if target == nil || parent == nil || cfg == nil {
+func isValidConfig(cfg *container.Config, h image.History) bool {
+	// todo: make this format better than join that loses data
+	return strings.Join(cfg.Cmd, " ") == h.CreatedBy
+}
+
+func isValidParent(img, parent *image.Image) bool {
+	if len(img.History) == 0 {
+		return false
+	}
+	if parent == nil || len(parent.History) == 0 && len(parent.RootFS.DiffIDs) == 0 {
+		return true
+	}
+	if len(parent.History) >= len(img.History) {
+		return false
+	}
+	if len(parent.RootFS.DiffIDs) > len(img.RootFS.DiffIDs) {
+		return false
+	}
+
+	for i, h := range parent.History {
+		if !reflect.DeepEqual(h, img.History[i]) {
+			return false
+		}
+	}
+	for i, d := range parent.RootFS.DiffIDs {
+		if d != img.RootFS.DiffIDs[i] {
+			return false
+		}
+	}
+	return true
+}
+
+func (ic *imageCache) isParent(ctx context.Context, img *image.Image, parentID image.ID) bool {
+	ii, err := ic.imageService.resolveImage(ctx, img.ImageID())
+	if err != nil {
+		return false
+	}
+	parent, ok := ii.Labels[imageLabelClassicBuilderParent]
+	if ok {
+		return parent == parentID.String()
+	}
+
+	p, err := ic.imageService.GetImage(ctx, parentID.String(), imagetype.GetImageOpts{})
+	if err != nil {
 		return false
 	}
+	return ic.isParent(ctx, p, parentID)
+}
 
-	if len(target.History) != len(parent.History)+1 ||
-		len(target.RootFS.DiffIDs) != len(parent.RootFS.DiffIDs)+1 {
+// compare two Config struct. Do not compare the "Image" nor "Hostname" fields
+// If OpenStdin is set, then it differs
+func isMatch(a, b *container.Config) bool {
+	if a == nil || b == nil ||
+		a.OpenStdin || b.OpenStdin {
+		return false
+	}
+	if a.AttachStdout != b.AttachStdout ||
+		a.AttachStderr != b.AttachStderr ||
+		a.User != b.User ||
+		a.OpenStdin != b.OpenStdin ||
+		a.Tty != b.Tty {
 		return false
 	}
 
-	for i := range parent.History {
-		if !reflect.DeepEqual(parent.History[i], target.History[i]) {
+	if len(a.Cmd) != len(b.Cmd) ||
+		len(a.Env) != len(b.Env) ||
+		len(a.Labels) != len(b.Labels) ||
+		len(a.ExposedPorts) != len(b.ExposedPorts) ||
+		len(a.Entrypoint) != len(b.Entrypoint) ||
+		len(a.Volumes) != len(b.Volumes) {
+		return false
+	}
+
+	for i := 0; i < len(a.Cmd); i++ {
+		if a.Cmd[i] != b.Cmd[i] {
+			return false
+		}
+	}
+	for i := 0; i < len(a.Env); i++ {
+		if a.Env[i] != b.Env[i] {
+			return false
+		}
+	}
+	for k, v := range a.Labels {
+		if v != b.Labels[k] {
+			return false
+		}
+	}
+	for k := range a.ExposedPorts {
+		if _, exists := b.ExposedPorts[k]; !exists {
 			return false
 		}
 	}
 
-	childCreatedBy := target.History[len(target.History)-1].CreatedBy
-	return childCreatedBy == strings.Join(cfg.Cmd, " ")
+	for i := 0; i < len(a.Entrypoint); i++ {
+		if a.Entrypoint[i] != b.Entrypoint[i] {
+			return false
+		}
+	}
+	for key := range a.Volumes {
+		if _, exists := b.Volumes[key]; !exists {
+			return false
+		}
+	}
+	return true
 }

daemon/containerd/image_builder.go

@@ -22,6 +22,7 @@ import (
 	"github.com/containerd/log"
 	"github.com/distribution/reference"
 	"github.com/docker/docker/api/types/backend"
+	"github.com/docker/docker/api/types/container"
 	imagetypes "github.com/docker/docker/api/types/image"
 	"github.com/docker/docker/api/types/registry"
 	"github.com/docker/docker/builder"
@@ -41,8 +42,26 @@ import (
 	ocispec "github.com/opencontainers/image-spec/specs-go/v1"
 )
 
-// Digest of the image which was the base image of the committed container.
-const imageLabelClassicBuilderParent = "org.mobyproject.image.parent"
+const (
+	// Digest of the image which was the base image of the committed container.
+	imageLabelClassicBuilderParent = "org.mobyproject.image.parent"
+
+	// "1" means that the image was created directly from the "FROM scratch".
+	imageLabelClassicBuilderFromScratch = "org.mobyproject.image.fromscratch"
+
+	// digest of the ContainerConfig stored in the content store.
+	imageLabelClassicBuilderContainerConfig = "org.mobyproject.image.containerconfig"
+)
+
+const (
+	// gc.ref label that associates the ContainerConfig content blob with the
+	// corresponding Config content.
+	contentLabelGcRefContainerConfig = "containerd.io/gc.ref.content.moby/container.config"
+
+	// Digest of the image this ContainerConfig blobs describes.
+	// Only ContainerConfig content should be labelled with it.
+	contentLabelClassicBuilderImage = "org.mobyproject.content.image"
+)
 
 // GetImageAndReleasableLayer returns an image and releaseable layer for a
 // reference or ID. Every call to GetImageAndReleasableLayer MUST call
@@ -446,7 +465,7 @@ func (i *ImageService) CreateImage(ctx context.Context, config []byte, parent st
 		})
 	}
 
-	createdImageId, err := i.createImageOCI(ctx, ociImgToCreate, parentDigest, layers)
+	createdImageId, err := i.createImageOCI(ctx, ociImgToCreate, parentDigest, layers, imgToCreate.ContainerConfig)
 	if err != nil {
 		return nil, err
 	}
@@ -456,6 +475,7 @@ func (i *ImageService) CreateImage(ctx context.Context, config []byte, parent st
 
 func (i *ImageService) createImageOCI(ctx context.Context, imgToCreate imagespec.DockerOCIImage,
 	parentDigest digest.Digest, layers []ocispec.Descriptor,
+	containerConfig container.Config,
 ) (dimage.ID, error) {
 	// Necessary to prevent the contents from being GC'd
 	// between writing them here and creating an image
@@ -469,7 +489,7 @@ func (i *ImageService) createImageOCI(ctx context.Context, imgToCreate imagespec
 		}
 	}()
 
-	manifestDesc, err := writeContentsForImage(ctx, i.snapshotter, i.client.ContentStore(), imgToCreate, layers)
+	manifestDesc, ccDesc, err := writeContentsForImage(ctx, i.snapshotter, i.client.ContentStore(), imgToCreate, layers, containerConfig)
 	if err != nil {
 		return "", err
 	}
@@ -479,10 +499,15 @@ func (i *ImageService) createImageOCI(ctx context.Context, imgToCreate imagespec
 		Target:    manifestDesc,
 		CreatedAt: time.Now(),
 		Labels: map[string]string{
-			imageLabelClassicBuilderParent: parentDigest.String(),
+			imageLabelClassicBuilderParent:          parentDigest.String(),
+			imageLabelClassicBuilderContainerConfig: ccDesc.Digest.String(),
 		},
 	}
 
+	if parentDigest == "" {
+		img.Labels[imageLabelClassicBuilderFromScratch] = "1"
+	}
+
 	createdImage, err := i.client.ImageService().Update(ctx, img)
 	if err != nil {
 		if !cerrdefs.IsNotFound(err) {
@@ -502,10 +527,17 @@ func (i *ImageService) createImageOCI(ctx context.Context, imgToCreate imagespec
 }
 
 // writeContentsForImage will commit oci image config and manifest into containerd's content store.
-func writeContentsForImage(ctx context.Context, snName string, cs content.Store, newConfig imagespec.DockerOCIImage, layers []ocispec.Descriptor) (ocispec.Descriptor, error) {
+func writeContentsForImage(ctx context.Context, snName string, cs content.Store,
+	newConfig imagespec.DockerOCIImage, layers []ocispec.Descriptor,
+	containerConfig container.Config,
+) (
+	manifestDesc ocispec.Descriptor,
+	containerConfigDesc ocispec.Descriptor,
+	_ error,
+) {
 	newConfigJSON, err := json.Marshal(newConfig)
 	if err != nil {
-		return ocispec.Descriptor{}, err
+		return ocispec.Descriptor{}, ocispec.Descriptor{}, err
 	}
 
 	configDesc := ocispec.Descriptor{
@@ -530,7 +562,7 @@ func writeContentsForImage(ctx context.Context, snName string, cs content.Store,
 
 	newMfstJSON, err := json.MarshalIndent(newMfst, "", "    ")
 	if err != nil {
-		return ocispec.Descriptor{}, err
+		return ocispec.Descriptor{}, ocispec.Descriptor{}, err
 	}
 
 	newMfstDesc := ocispec.Descriptor{
@@ -549,17 +581,37 @@ func writeContentsForImage(ctx context.Context, snName string, cs content.Store,
 
 	err = content.WriteBlob(ctx, cs, newMfstDesc.Digest.String(), bytes.NewReader(newMfstJSON), newMfstDesc, content.WithLabels(labels))
 	if err != nil {
-		return ocispec.Descriptor{}, err
+		return ocispec.Descriptor{}, ocispec.Descriptor{}, err
 	}
 
-	// config should reference to snapshotter
+	ccDesc, err := saveContainerConfig(ctx, cs, newMfstDesc.Digest, containerConfig)
+	if err != nil {
+		return ocispec.Descriptor{}, ocispec.Descriptor{}, err
+	}
+
+	// config should reference to snapshotter and container config
 	labelOpt := content.WithLabels(map[string]string{
 		fmt.Sprintf("containerd.io/gc.ref.snapshot.%s", snName): identity.ChainID(newConfig.RootFS.DiffIDs).String(),
+		contentLabelGcRefContainerConfig:                        ccDesc.Digest.String(),
 	})
 	err = content.WriteBlob(ctx, cs, configDesc.Digest.String(), bytes.NewReader(newConfigJSON), configDesc, labelOpt)
+	if err != nil {
+		return ocispec.Descriptor{}, ocispec.Descriptor{}, err
+	}
+
+	return newMfstDesc, ccDesc, nil
+}
+
+// saveContainerConfig serializes the given ContainerConfig into a json and
+// stores it in the content store and returns its descriptor.
+func saveContainerConfig(ctx context.Context, content content.Ingester, imgID digest.Digest, containerConfig container.Config) (ocispec.Descriptor, error) {
+	containerConfigDesc, err := storeJson(ctx, content,
+		"application/vnd.docker.container.image.v1+json", containerConfig,
+		map[string]string{contentLabelClassicBuilderImage: imgID.String()},
+	)
 	if err != nil {
 		return ocispec.Descriptor{}, err
 	}
 
-	return newMfstDesc, nil
+	return containerConfigDesc, nil
 }

daemon/containerd/image_children.go

@@ -3,21 +3,17 @@ package containerd
 import (
 	"context"
 
-	"github.com/containerd/containerd/content"
-	cerrdefs "github.com/containerd/containerd/errdefs"
 	containerdimages "github.com/containerd/containerd/images"
-	"github.com/containerd/containerd/platforms"
-	"github.com/containerd/log"
 	"github.com/docker/docker/errdefs"
 	"github.com/docker/docker/image"
 	"github.com/opencontainers/go-digest"
-	ocispec "github.com/opencontainers/image-spec/specs-go/v1"
 	"github.com/pkg/errors"
 )
 
-// Children returns a slice of image IDs that are children of the `id` image
-func (i *ImageService) Children(ctx context.Context, id image.ID) ([]image.ID, error) {
-	imgs, err := i.images.List(ctx, "labels."+imageLabelClassicBuilderParent+"=="+string(id))
+// getImagesWithLabel returns all images that have the matching label key and value.
+func (i *ImageService) getImagesWithLabel(ctx context.Context, labelKey string, labelValue string) ([]image.ID, error) {
+	imgs, err := i.images.List(ctx, "labels."+labelKey+"=="+labelValue)
+
 	if err != nil {
 		return []image.ID{}, errdefs.System(errors.Wrap(err, "failed to list all images"))
 	}
@@ -30,136 +26,38 @@ func (i *ImageService) Children(ctx context.Context, id image.ID) ([]image.ID, e
 	return children, nil
 }
 
-// platformRootfs returns a rootfs for a specified platform.
-func platformRootfs(ctx context.Context, store content.Store, desc ocispec.Descriptor, platform ocispec.Platform) (ocispec.RootFS, error) {
-	empty := ocispec.RootFS{}
-
-	configDesc, err := containerdimages.Config(ctx, store, desc, platforms.OnlyStrict(platform))
-	if err != nil {
-		return empty, errors.Wrapf(err, "failed to get config for platform %s", platforms.Format(platform))
-	}
-
-	diffs, err := containerdimages.RootFS(ctx, store, configDesc)
-	if err != nil {
-		return empty, errors.Wrapf(err, "failed to obtain rootfs")
-	}
-
-	return ocispec.RootFS{
-		Type:    "layers",
-		DiffIDs: diffs,
-	}, nil
-}
-
-// isRootfsChildOf checks if all layers from parent rootfs are child's first layers
-// and child has at least one more layer (to make it not commutative).
-// Example:
-// A with layers [X, Y],
-// B with layers [X, Y, Z]
-// C with layers [Y, Z]
-//
-// Only isRootfsChildOf(B, A) is true.
-// Which means that B is considered a children of A. B and C has no children.
-// See more examples in TestIsRootfsChildOf.
-func isRootfsChildOf(child ocispec.RootFS, parent ocispec.RootFS) bool {
-	childLen := len(child.DiffIDs)
-	parentLen := len(parent.DiffIDs)
-
-	if childLen <= parentLen {
-		return false
-	}
-
-	for i := 0; i < parentLen; i++ {
-		if child.DiffIDs[i] != parent.DiffIDs[i] {
-			return false
-		}
-	}
-
-	return true
+// Children returns a slice of image IDs that are children of the `id` image
+func (i *ImageService) Children(ctx context.Context, id image.ID) ([]image.ID, error) {
+	return i.getImagesWithLabel(ctx, imageLabelClassicBuilderParent, string(id))
 }
 
-// parents returns a slice of image IDs whose entire rootfs contents match,
-// in order, the childs first layers, excluding images with the exact same
-// rootfs.
+// parents returns a slice of image IDs that are parents of the `id` image
 //
 // Called from image_delete.go to prune dangling parents.
-func (i *ImageService) parents(ctx context.Context, id image.ID) ([]imageWithRootfs, error) {
-	target, err := i.resolveDescriptor(ctx, id.String())
+func (i *ImageService) parents(ctx context.Context, id image.ID) ([]containerdimages.Image, error) {
+	targetImage, err := i.resolveImage(ctx, id.String())
 	if err != nil {
 		return nil, errors.Wrap(err, "failed to get child image")
 	}
 
-	allPlatforms, err := containerdimages.Platforms(ctx, i.content, target)
-	if err != nil {
-		return nil, errdefs.System(errors.Wrap(err, "failed to list platforms supported by image"))
-	}
+	var imgs []containerdimages.Image
+	for {
+		parent, ok := targetImage.Labels[imageLabelClassicBuilderParent]
+		if !ok || parent == "" {
+			break
+		}
 
-	var childRootFS []ocispec.RootFS
-	for _, platform := range allPlatforms {
-		rootfs, err := platformRootfs(ctx, i.content, target, platform)
+		parentDigest, err := digest.Parse(parent)
 		if err != nil {
-			if cerrdefs.IsNotFound(err) {
-				continue
-			}
-			return nil, errdefs.System(errors.Wrap(err, "failed to get platform-specific rootfs"))
+			return nil, err
 		}
-
-		childRootFS = append(childRootFS, rootfs)
-	}
-
-	imgs, err := i.images.List(ctx)
-	if err != nil {
-		return nil, errdefs.System(errors.Wrap(err, "failed to list all images"))
-	}
-
-	var parents []imageWithRootfs
-	for _, img := range imgs {
-	nextImage:
-		for _, platform := range allPlatforms {
-			rootfs, err := platformRootfs(ctx, i.content, img.Target, platform)
-			if err != nil {
-				if cerrdefs.IsNotFound(err) {
-					continue
-				}
-				return nil, errdefs.System(errors.Wrap(err, "failed to get platform-specific rootfs"))
-			}
-
-			for _, childRoot := range childRootFS {
-				if isRootfsChildOf(childRoot, rootfs) {
-					parents = append(parents, imageWithRootfs{
-						img:    img,
-						rootfs: rootfs,
-					})
-					break nextImage
-				}
-			}
+		img, err := i.resolveImage(ctx, parentDigest.String())
+		if err != nil {
+			return nil, err
 		}
+		imgs = append(imgs, img)
+		targetImage = img
 	}
 
-	return parents, nil
-}
-
-// getParentsByBuilderLabel finds images that were a base for the given image
-// by an image label set by the legacy builder.
-// NOTE: This only works for images built with legacy builder (not Buildkit).
-func (i *ImageService) getParentsByBuilderLabel(ctx context.Context, img containerdimages.Image) ([]containerdimages.Image, error) {
-	parent, ok := img.Labels[imageLabelClassicBuilderParent]
-	if !ok || parent == "" {
-		return nil, nil
-	}
-
-	dgst, err := digest.Parse(parent)
-	if err != nil {
-		log.G(ctx).WithFields(log.Fields{
-			"error": err,
-			"value": parent,
-		}).Warnf("invalid %s label value", imageLabelClassicBuilderParent)
-		return nil, nil
-	}
-
-	return i.images.List(ctx, "target.digest=="+dgst.String())
-}
-
-type imageWithRootfs struct {
-	img    containerdimages.Image
-	rootfs ocispec.RootFS
+	return imgs, nil
 }

daemon/containerd/image_commit.go

@@ -96,7 +96,7 @@ func (i *ImageService) CommitImage(ctx context.Context, cc backend.CommitConfig)
 		layers = append(layers, *diffLayerDesc)
 	}
 
-	return i.createImageOCI(ctx, imageConfig, digest.Digest(cc.ParentImageID), layers)
+	return i.createImageOCI(ctx, imageConfig, digest.Digest(cc.ParentImageID), layers, *cc.ContainerConfig)
 }
 
 // generateCommitImageConfig generates an OCI Image config based on the

daemon/containerd/image_delete.go

@@ -3,12 +3,12 @@ package containerd
 import (
 	"context"
 	"fmt"
-	"sort"
 	"strings"
 	"time"
 
 	cerrdefs "github.com/containerd/containerd/errdefs"
 	"github.com/containerd/containerd/images"
+	containerdimages "github.com/containerd/containerd/images"
 	"github.com/containerd/log"
 	"github.com/distribution/reference"
 	"github.com/docker/docker/api/types/events"
@@ -215,14 +215,13 @@ func (i *ImageService) deleteAll(ctx context.Context, imgID image.ID, all []imag
 		}
 	}()
 
-	var parents []imageWithRootfs
+	var parents []containerdimages.Image
 	if prune {
 		// TODO(dmcgowan): Consider using GC labels to walk for deletion
 		parents, err = i.parents(ctx, imgID)
 		if err != nil {
 			log.G(ctx).WithError(err).Warn("failed to get image parents")
 		}
-		sortParentsByAffinity(parents)
 	}
 
 	for _, imageRef := range all {
@@ -234,15 +233,15 @@ func (i *ImageService) deleteAll(ctx context.Context, imgID image.ID, all []imag
 	records = append(records, imagetypes.DeleteResponse{Deleted: imgID.String()})
 
 	for _, parent := range parents {
-		if !isDanglingImage(parent.img) {
+		if !isDanglingImage(parent) {
 			break
 		}
-		err = i.imageDeleteHelper(ctx, parent.img, all, &records, conflictSoft)
+		err = i.imageDeleteHelper(ctx, parent, all, &records, conflictSoft)
 		if err != nil {
 			log.G(ctx).WithError(err).Warn("failed to remove image parent")
 			break
 		}
-		parentID := parent.img.Target.Digest.String()
+		parentID := parent.Target.Digest.String()
 		i.LogImageEvent(parentID, parentID, events.ActionDelete)
 		records = append(records, imagetypes.DeleteResponse{Deleted: parentID})
 	}
@@ -262,17 +261,6 @@ func isImageIDPrefix(imageID, possiblePrefix string) bool {
 	return false
 }
 
-func sortParentsByAffinity(parents []imageWithRootfs) {
-	sort.Slice(parents, func(i, j int) bool {
-		lenRootfsI := len(parents[i].rootfs.DiffIDs)
-		lenRootfsJ := len(parents[j].rootfs.DiffIDs)
-		if lenRootfsI == lenRootfsJ {
-			return isDanglingImage(parents[i].img)
-		}
-		return lenRootfsI > lenRootfsJ
-	})
-}
-
 // getSameReferences returns the set of images which are the same as:
 // - the provided img if non-nil
 // - OR the first named image found in the provided image set

daemon/containerd/image_history.go

@@ -5,12 +5,14 @@ import (
 	"sort"
 
 	"github.com/containerd/containerd/images"
+	containerdimages "github.com/containerd/containerd/images"
 	cplatforms "github.com/containerd/containerd/platforms"
 	"github.com/containerd/log"
 	"github.com/distribution/reference"
 	imagetype "github.com/docker/docker/api/types/image"
 	"github.com/docker/docker/errdefs"
 	"github.com/docker/docker/pkg/platforms"
+	"github.com/opencontainers/go-digest"
 	"github.com/opencontainers/image-spec/identity"
 	ocispec "github.com/opencontainers/image-spec/specs-go/v1"
 	"github.com/pkg/errors"
@@ -161,3 +163,24 @@ func getImageTags(ctx context.Context, imgs []images.Image) []string {
 
 	return tags
 }
+
+// getParentsByBuilderLabel finds images that were a base for the given image
+// by an image label set by the legacy builder.
+// NOTE: This only works for images built with legacy builder (not Buildkit).
+func (i *ImageService) getParentsByBuilderLabel(ctx context.Context, img containerdimages.Image) ([]containerdimages.Image, error) {
+	parent, ok := img.Labels[imageLabelClassicBuilderParent]
+	if !ok || parent == "" {
+		return nil, nil
+	}
+
+	dgst, err := digest.Parse(parent)
+	if err != nil {
+		log.G(ctx).WithFields(log.Fields{
+			"error": err,
+			"value": parent,
+		}).Warnf("invalid %s label value", imageLabelClassicBuilderParent)
+		return nil, nil
+	}
+
+	return i.client.ImageService().List(ctx, "target.digest=="+dgst.String())
+}

hack/validate/default

@@ -12,6 +12,6 @@ SCRIPTDIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
 . "${SCRIPTDIR}"/swagger
 . "${SCRIPTDIR}"/swagger-gen
 . "${SCRIPTDIR}"/toml
-. "${SCRIPTDIR}"/deprecate-integration-cli
+#. "${SCRIPTDIR}"/deprecate-integration-cli
 . "${SCRIPTDIR}"/golangci-lint
 . "${SCRIPTDIR}"/shfmt

hack/validate/deprecate-integration-cli

@@ -1,25 +0,0 @@
-#!/usr/bin/env bash
-# Check that no new tests are being added to integration-cli
-
-SCRIPTDIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
-source "${SCRIPTDIR}/.validate"
-
-new_tests=$(
-	validate_diff --diff-filter=ACMR --unified=0 -- 'integration-cli/*_api_*.go' 'integration-cli/*_cli_*.go' \
-		| grep -E '^\+func (.*) Test' || true
-)
-
-if [ -n "$new_tests" ]; then
-	{
-		echo "The following new tests were added to integration-cli:"
-		echo
-		echo "$new_tests"
-		echo
-		echo "integration-cli is deprecated. Please add an API integration test to"
-		echo "./integration/COMPONENT/. See ./TESTING.md for more details."
-		echo
-	} >&2
-	false
-else
-	echo 'Congratulations!  No new tests were added to integration-cli.'
-fi

integration-cli/docker_cli_build_test.go

@@ -5447,6 +5447,54 @@ func (s *DockerCLIBuildSuite) TestBuildCacheFrom(c *testing.T) {
 	assert.Equal(c, strings.Count(result.Combined(), "Using cache"), 0)
 	cli.DockerCmd(c, "rmi", "build2")
 
+	// Modify file, everything up to last command and layers are reused
+	dockerfile = `
+		FROM busybox
+		ENV FOO=bar
+		ADD baz /
+		RUN touch newfile`
+	err := os.WriteFile(filepath.Join(ctx.Dir, "Dockerfile"), []byte(dockerfile), 0o644)
+	assert.NilError(c, err)
+
+	result = cli.BuildCmd(c, "build2", cli.WithFlags("--cache-from=build1"), build.WithExternalBuildContext(ctx))
+	id2 = getIDByName(c, "build2")
+	assert.Assert(c, id1 != id2)
+	assert.Equal(c, strings.Count(result.Combined(), "Using cache"), 2)
+
+	layers1Str := cli.DockerCmd(c, "inspect", "-f", "{{json .RootFS.Layers}}", "build1").Combined()
+	layers2Str := cli.DockerCmd(c, "inspect", "-f", "{{json .RootFS.Layers}}", "build2").Combined()
+
+	var layers1 []string
+	var layers2 []string
+	assert.Assert(c, json.Unmarshal([]byte(layers1Str), &layers1) == nil)
+	assert.Assert(c, json.Unmarshal([]byte(layers2Str), &layers2) == nil)
+
+	assert.Equal(c, len(layers1), len(layers2))
+	for i := 0; i < len(layers1)-1; i++ {
+		assert.Equal(c, layers1[i], layers2[i])
+	}
+	assert.Assert(c, layers1[len(layers1)-1] != layers2[len(layers1)-1])
+}
+
+func (s *DockerCLIBuildSuite) TestBuildCacheFromLoad(c *testing.T) {
+	skip.If(c, testEnv.UsingSnapshotter, "Parent-child relations are lost when save/load-ing with the containerd image store")
+	testRequires(c, DaemonIsLinux) // All tests that do save are skipped in windows
+	dockerfile := `
+		FROM busybox
+		ENV FOO=bar
+		ADD baz /
+		RUN touch bax`
+	ctx := fakecontext.New(c, "",
+		fakecontext.WithDockerfile(dockerfile),
+		fakecontext.WithFiles(map[string]string{
+			"Dockerfile": dockerfile,
+			"baz":        "baz",
+		}))
+	defer ctx.Close()
+
+	cli.BuildCmd(c, "build1", build.WithExternalBuildContext(ctx))
+	id1 := getIDByName(c, "build1")
+
 	// clear parent images
 	tempDir, err := os.MkdirTemp("", "test-build-cache-from-")
 	if err != nil {
@@ -5461,12 +5509,11 @@ func (s *DockerCLIBuildSuite) TestBuildCacheFrom(c *testing.T) {
 	assert.Equal(c, strings.TrimSpace(parentID), "")
 
 	// cache still applies without parents
-	result = cli.BuildCmd(c, "build2", cli.WithFlags("--cache-from=build1"), build.WithExternalBuildContext(ctx))
-	id2 = getIDByName(c, "build2")
+	result := cli.BuildCmd(c, "build2", cli.WithFlags("--cache-from=build1"), build.WithExternalBuildContext(ctx))
+	id2 := getIDByName(c, "build2")
 	assert.Equal(c, id1, id2)
 	assert.Equal(c, strings.Count(result.Combined(), "Using cache"), 3)
 	history1 := cli.DockerCmd(c, "history", "-q", "build2").Combined()
-
 	// Retry, no new intermediate images
 	result = cli.BuildCmd(c, "build3", cli.WithFlags("--cache-from=build1"), build.WithExternalBuildContext(ctx))
 	id3 := getIDByName(c, "build3")
@@ -5479,34 +5526,6 @@ func (s *DockerCLIBuildSuite) TestBuildCacheFrom(c *testing.T) {
 	cli.DockerCmd(c, "rmi", "build3")
 	cli.DockerCmd(c, "rmi", "build1")
 	cli.DockerCmd(c, "load", "-i", tempFile)
-
-	// Modify file, everything up to last command and layers are reused
-	dockerfile = `
-		FROM busybox
-		ENV FOO=bar
-		ADD baz /
-		RUN touch newfile`
-	err = os.WriteFile(filepath.Join(ctx.Dir, "Dockerfile"), []byte(dockerfile), 0o644)
-	assert.NilError(c, err)
-
-	result = cli.BuildCmd(c, "build2", cli.WithFlags("--cache-from=build1"), build.WithExternalBuildContext(ctx))
-	id2 = getIDByName(c, "build2")
-	assert.Assert(c, id1 != id2)
-	assert.Equal(c, strings.Count(result.Combined(), "Using cache"), 2)
-
-	layers1Str := cli.DockerCmd(c, "inspect", "-f", "{{json .RootFS.Layers}}", "build1").Combined()
-	layers2Str := cli.DockerCmd(c, "inspect", "-f", "{{json .RootFS.Layers}}", "build2").Combined()
-
-	var layers1 []string
-	var layers2 []string
-	assert.Assert(c, json.Unmarshal([]byte(layers1Str), &layers1) == nil)
-	assert.Assert(c, json.Unmarshal([]byte(layers2Str), &layers2) == nil)
-
-	assert.Equal(c, len(layers1), len(layers2))
-	for i := 0; i < len(layers1)-1; i++ {
-		assert.Equal(c, layers1[i], layers2[i])
-	}
-	assert.Assert(c, layers1[len(layers1)-1] != layers2[len(layers1)-1])
 }
 
 func (s *DockerCLIBuildSuite) TestBuildMultiStageCache(c *testing.T) {