Merge pull request #49702 from jsternberg/containerd-dangling-build-image

containerd: images overridden by a build are kept dangling

builder/builder-next/controller.go

@@ -21,6 +21,7 @@ import (
 	"github.com/docker/docker/builder/builder-next/adapters/containerimage"
 	"github.com/docker/docker/builder/builder-next/adapters/localinlinecache"
 	"github.com/docker/docker/builder/builder-next/adapters/snapshot"
+	"github.com/docker/docker/builder/builder-next/exporter"
 	"github.com/docker/docker/builder/builder-next/exporter/mobyexporter"
 	"github.com/docker/docker/builder/builder-next/imagerefchecker"
 	mobyworker "github.com/docker/docker/builder/builder-next/worker"
@@ -157,7 +158,10 @@ func newSnapshotterController(ctx context.Context, rt http.RoundTripper, opt Opt
 	}
 	wo.Executor = exec
 
-	w, err := mobyworker.NewContainerdWorker(ctx, wo, opt.Callbacks, rt)
+	w, err := mobyworker.NewContainerdWorker(ctx, wo, exporter.Opt{
+		Callbacks:   opt.Callbacks,
+		ImageTagger: opt.ImageTagger,
+	}, rt)
 	if err != nil {
 		return nil, err
 	}

builder/builder-next/exporter/wrapper.go

@@ -2,39 +2,49 @@ package exporter
 
 import (
 	"context"
+	"fmt"
 	"strings"
 
-	"github.com/containerd/log"
-	"github.com/distribution/reference"
+	distref "github.com/distribution/reference"
 	"github.com/docker/docker/builder/builder-next/exporter/overrides"
+	"github.com/docker/docker/image"
 	"github.com/moby/buildkit/exporter"
 	"github.com/moby/buildkit/exporter/containerimage/exptypes"
-
+	"github.com/moby/buildkit/util/progress"
 	ocispec "github.com/opencontainers/image-spec/specs-go/v1"
 )
 
+// Opt are options for the exporter wrapper.
+type Opt struct {
+	// Callbacks contains callbacks used by the image exporter.
+	Callbacks BuildkitCallbacks
+
+	// ImageTagger is used to tag the image after it is exported.
+	ImageTagger ImageTagger
+}
+
 type BuildkitCallbacks struct {
 	// Exported is a Called when an image is exported by buildkit.
 	Exported func(ctx context.Context, id string, desc ocispec.Descriptor)
+}
 
-	// Named is a callback that is called when an image is created in the
-	// containerd image store by buildkit.
-	Named func(ctx context.Context, ref reference.NamedTagged, desc ocispec.Descriptor)
+type ImageTagger interface {
+	TagImage(ctx context.Context, imageID image.ID, newTag distref.Named) error
 }
 
 // Wraps the containerimage exporter's Resolve method to apply moby-specific
 // overrides to the exporter attributes.
 type imageExporterMobyWrapper struct {
-	exp       exporter.Exporter
-	callbacks BuildkitCallbacks
+	exp exporter.Exporter
+	opt Opt
 }
 
 // NewWrapper returns an exporter wrapper that applies moby specific attributes
 // and hooks the export process.
-func NewWrapper(exp exporter.Exporter, callbacks BuildkitCallbacks) (exporter.Exporter, error) {
+func NewWrapper(exp exporter.Exporter, opt Opt) (exporter.Exporter, error) {
 	return &imageExporterMobyWrapper{
-		exp:       exp,
-		callbacks: callbacks,
+		exp: exp,
+		opt: opt,
 	}, nil
 }
 
@@ -47,7 +57,9 @@ func (e *imageExporterMobyWrapper) Resolve(ctx context.Context, id int, exporter
 	if err != nil {
 		return nil, err
 	}
-	exporterAttrs[string(exptypes.OptKeyName)] = strings.Join(reposAndTags, ",")
+
+	// Force the exporter to not use a name so it always creates a dangling image.
+	exporterAttrs[string(exptypes.OptKeyName)] = ""
 	exporterAttrs[string(exptypes.OptKeyUnpack)] = "true"
 	if _, has := exporterAttrs[string(exptypes.OptKeyDanglingPrefix)]; !has {
 		exporterAttrs[string(exptypes.OptKeyDanglingPrefix)] = "moby-dangling"
@@ -61,13 +73,15 @@ func (e *imageExporterMobyWrapper) Resolve(ctx context.Context, id int, exporter
 
 	return &imageExporterInstanceWrapper{
 		ExporterInstance: inst,
-		callbacks:        e.callbacks,
+		reposAndTags:     reposAndTags,
+		opt:              e.opt,
 	}, nil
 }
 
 type imageExporterInstanceWrapper struct {
 	exporter.ExporterInstance
-	callbacks BuildkitCallbacks
+	reposAndTags []string
+	opt          Opt
 }
 
 func (i *imageExporterInstanceWrapper) Export(ctx context.Context, src *exporter.Source, inlineCache exptypes.InlineCache, sessionID string) (map[string]string, exporter.DescriptorReference, error) {
@@ -78,38 +92,31 @@ func (i *imageExporterInstanceWrapper) Export(ctx context.Context, src *exporter
 
 	desc := ref.Descriptor()
 	imageID := out[exptypes.ExporterImageDigestKey]
-	if i.callbacks.Exported != nil {
-		i.callbacks.Exported(ctx, imageID, desc)
+	if i.opt.Callbacks.Exported != nil {
+		i.opt.Callbacks.Exported(ctx, imageID, desc)
 	}
 
-	if i.callbacks.Named != nil {
-		i.processNamedCallback(ctx, out, desc)
-	}
-
-	return out, ref, nil
+	err = i.processNamed(ctx, image.ID(imageID), out, desc)
+	return out, ref, err
 }
 
-func (i *imageExporterInstanceWrapper) processNamedCallback(ctx context.Context, out map[string]string, desc ocispec.Descriptor) {
-	// TODO(vvoland): Change to exptypes.ExporterImageNameKey when BuildKit v0.21 is vendored.
-	imageName := out["image.name"]
-	if imageName == "" {
-		log.G(ctx).Warn("image named with empty image.name produced by buildkit")
-		return
+func (i *imageExporterInstanceWrapper) processNamed(ctx context.Context, imageID image.ID, out map[string]string, desc ocispec.Descriptor) error {
+	if len(i.reposAndTags) == 0 {
+		return nil
 	}
 
-	for _, name := range strings.Split(imageName, ",") {
-		ref, err := reference.ParseNormalizedNamed(name)
+	for _, repoAndTag := range i.reposAndTags {
+		newTag, err := distref.ParseNamed(repoAndTag)
 		if err != nil {
-			// Shouldn't happen, but log if it does and continue.
-			log.G(ctx).WithFields(log.Fields{
-				"name":  name,
-				"error": err,
-			}).Warn("image named with invalid reference produced by buildkit")
-			continue
+			return err
 		}
 
-		if namedTagged, ok := reference.TagNameOnly(ref).(reference.NamedTagged); ok {
-			i.callbacks.Named(ctx, namedTagged, desc)
+		done := progress.OneOff(ctx, fmt.Sprintf("naming to %s", newTag))
+		if err := i.opt.ImageTagger.TagImage(ctx, imageID, newTag); err != nil {
+			return done(err)
 		}
+		done(nil)
 	}
+	out[exptypes.ExporterImageNameKey] = strings.Join(i.reposAndTags, ",")
+	return nil
 }

builder/builder-next/worker/containerdworker.go

@@ -16,11 +16,11 @@ import (
 // ContainerdWorker is a local worker instance with dedicated snapshotter, cache, and so on.
 type ContainerdWorker struct {
 	*base.Worker
-	callbacks exporter.BuildkitCallbacks
+	opt exporter.Opt
 }
 
 // NewContainerdWorker instantiates a local worker.
-func NewContainerdWorker(ctx context.Context, wo base.WorkerOpt, callbacks exporter.BuildkitCallbacks, rt nethttp.RoundTripper) (*ContainerdWorker, error) {
+func NewContainerdWorker(ctx context.Context, wo base.WorkerOpt, opt exporter.Opt, rt nethttp.RoundTripper) (*ContainerdWorker, error) {
 	bw, err := base.NewWorker(ctx, wo)
 	if err != nil {
 		return nil, err
@@ -35,7 +35,7 @@ func NewContainerdWorker(ctx context.Context, wo base.WorkerOpt, callbacks expor
 		log.G(ctx).Warnf("Could not register builder http source: %s", err)
 	}
 
-	return &ContainerdWorker{Worker: bw, callbacks: callbacks}, nil
+	return &ContainerdWorker{Worker: bw, opt: opt}, nil
 }
 
 // Exporter returns exporter by name
@@ -46,7 +46,7 @@ func (w *ContainerdWorker) Exporter(name string, sm *session.Manager) (bkexporte
 		if err != nil {
 			return nil, err
 		}
-		return exporter.NewWrapper(exp, w.callbacks)
+		return exporter.NewWrapper(exp, w.opt)
 	default:
 		return w.Worker.Exporter(name, sm)
 	}

cmd/dockerd/daemon.go

@@ -423,7 +423,6 @@ func initBuildkit(ctx context.Context, d *daemon.Daemon) (_ builderOptions, clos
 		ContainerdNamespace: cfg.ContainerdNamespace,
 		Callbacks: exporter.BuildkitCallbacks{
 			Exported: d.ImageExportedByBuildkit,
-			Named:    d.ImageNamedByBuildkit,
 		},
 		CDISpecDirs: cdiSpecDirs,
 	})

daemon/build.go

@@ -3,7 +3,6 @@ package daemon
 import (
 	"context"
 
-	"github.com/distribution/reference"
 	"github.com/docker/docker/api/types/events"
 	ocispec "github.com/opencontainers/image-spec/specs-go/v1"
 )
@@ -15,10 +14,3 @@ import (
 func (daemon *Daemon) ImageExportedByBuildkit(ctx context.Context, id string, desc ocispec.Descriptor) {
 	daemon.imageService.LogImageEvent(ctx, id, id, events.ActionCreate)
 }
-
-// ImageNamedByBuildkit is a callback that is called when an image is tagged by buildkit.
-// Note: It is only called if the buildkit didn't call the image service itself to perform the tagging.
-// Currently this only happens when the containerd image store is used.
-func (daemon *Daemon) ImageNamedByBuildkit(ctx context.Context, ref reference.NamedTagged, desc ocispec.Descriptor) {
-	daemon.imageService.LogImageEvent(ctx, desc.Digest.String(), reference.FamiliarString(ref), events.ActionTag)
-}