containerd: images overridden by a build are kept dangling

The build exporter now clears the image tags and always exported to a
dangling image. It then uses the image tagger to perform the tagging
which causes the dangling image to be removed and the naming message to
be sent correctly.

An additional progress message is sent to indicate the renaming.

Signed-off-by: Jonathan A. Sternberg <jonathan.sternberg@docker.com>

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)
-}