Revert "containerd: images overridden by a build are kept dangling"

This reverts commit 50a856157c.

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

builder/builder-next/controller.go

@@ -21,7 +21,6 @@ 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"
@@ -169,10 +168,7 @@ func newSnapshotterController(ctx context.Context, rt http.RoundTripper, opt Opt
 	}
 	wo.Executor = exec
 
-	w, err := mobyworker.NewContainerdWorker(ctx, wo, exporter.Opt{
-		Callbacks:   opt.Callbacks,
-		ImageTagger: opt.ImageTagger,
-	}, rt)
+	w, err := mobyworker.NewContainerdWorker(ctx, wo, opt.Callbacks, rt)
 	if err != nil {
 		return nil, err
 	}

builder/builder-next/exporter/wrapper.go

@@ -2,49 +2,39 @@ package exporter
 
 import (
 	"context"
-	"fmt"
 	"strings"
 
-	distref "github.com/distribution/reference"
+	"github.com/containerd/log"
+	"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)
-}
 
-type ImageTagger interface {
-	TagImage(ctx context.Context, imageID image.ID, newTag distref.Named) error
+	// 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)
 }
 
 // Wraps the containerimage exporter's Resolve method to apply moby-specific
 // overrides to the exporter attributes.
 type imageExporterMobyWrapper struct {
-	exp exporter.Exporter
-	opt Opt
+	exp       exporter.Exporter
+	callbacks BuildkitCallbacks
 }
 
 // NewWrapper returns an exporter wrapper that applies moby specific attributes
 // and hooks the export process.
-func NewWrapper(exp exporter.Exporter, opt Opt) (exporter.Exporter, error) {
+func NewWrapper(exp exporter.Exporter, callbacks BuildkitCallbacks) (exporter.Exporter, error) {
 	return &imageExporterMobyWrapper{
-		exp: exp,
-		opt: opt,
+		exp:       exp,
+		callbacks: callbacks,
 	}, nil
 }
 
@@ -57,9 +47,7 @@ func (e *imageExporterMobyWrapper) Resolve(ctx context.Context, id int, exporter
 	if err != nil {
 		return nil, err
 	}
-
-	// Force the exporter to not use a name so it always creates a dangling image.
-	exporterAttrs[string(exptypes.OptKeyName)] = ""
+	exporterAttrs[string(exptypes.OptKeyName)] = strings.Join(reposAndTags, ",")
 	exporterAttrs[string(exptypes.OptKeyUnpack)] = "true"
 	if _, has := exporterAttrs[string(exptypes.OptKeyDanglingPrefix)]; !has {
 		exporterAttrs[string(exptypes.OptKeyDanglingPrefix)] = "moby-dangling"
@@ -73,15 +61,13 @@ func (e *imageExporterMobyWrapper) Resolve(ctx context.Context, id int, exporter
 
 	return &imageExporterInstanceWrapper{
 		ExporterInstance: inst,
-		reposAndTags:     reposAndTags,
-		opt:              e.opt,
+		callbacks:        e.callbacks,
 	}, nil
 }
 
 type imageExporterInstanceWrapper struct {
 	exporter.ExporterInstance
-	reposAndTags []string
-	opt          Opt
+	callbacks BuildkitCallbacks
 }
 
 func (i *imageExporterInstanceWrapper) Export(ctx context.Context, src *exporter.Source, inlineCache exptypes.InlineCache, sessionID string) (map[string]string, exporter.DescriptorReference, error) {
@@ -92,31 +78,38 @@ func (i *imageExporterInstanceWrapper) Export(ctx context.Context, src *exporter
 
 	desc := ref.Descriptor()
 	imageID := out[exptypes.ExporterImageDigestKey]
-	if i.opt.Callbacks.Exported != nil {
-		i.opt.Callbacks.Exported(ctx, imageID, desc)
+	if i.callbacks.Exported != nil {
+		i.callbacks.Exported(ctx, imageID, desc)
 	}
 
-	err = i.processNamed(ctx, image.ID(imageID), out, desc)
-	return out, ref, err
+	if i.callbacks.Named != nil {
+		i.processNamedCallback(ctx, out, desc)
+	}
+
+	return out, ref, nil
 }
 
-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
+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
 	}
 
-	for _, repoAndTag := range i.reposAndTags {
-		newTag, err := distref.ParseNamed(repoAndTag)
+	for _, name := range strings.Split(imageName, ",") {
+		ref, err := reference.ParseNormalizedNamed(name)
 		if err != nil {
-			return err
+			// 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
 		}
 
-		done := progress.OneOff(ctx, fmt.Sprintf("naming to %s", newTag))
-		if err := i.opt.ImageTagger.TagImage(ctx, imageID, newTag); err != nil {
-			return done(err)
+		if namedTagged, ok := reference.TagNameOnly(ref).(reference.NamedTagged); ok {
+			i.callbacks.Named(ctx, namedTagged, desc)
 		}
-		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
-	opt exporter.Opt
+	callbacks exporter.BuildkitCallbacks
 }
 
 // NewContainerdWorker instantiates a local worker.
-func NewContainerdWorker(ctx context.Context, wo base.WorkerOpt, opt exporter.Opt, rt nethttp.RoundTripper) (*ContainerdWorker, error) {
+func NewContainerdWorker(ctx context.Context, wo base.WorkerOpt, callbacks exporter.BuildkitCallbacks, 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, opt exporter.Op
 		log.G(ctx).Warnf("Could not register builder http source: %s", err)
 	}
 
-	return &ContainerdWorker{Worker: bw, opt: opt}, nil
+	return &ContainerdWorker{Worker: bw, callbacks: callbacks}, 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.opt)
+		return exporter.NewWrapper(exp, w.callbacks)
 	default:
 		return w.Worker.Exporter(name, sm)
 	}

cmd/dockerd/daemon.go

@@ -431,6 +431,7 @@ 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,6 +3,7 @@ package daemon
 import (
 	"context"
 
+	"github.com/distribution/reference"
 	"github.com/docker/docker/api/types/events"
 	ocispec "github.com/opencontainers/image-spec/specs-go/v1"
 )
@@ -14,3 +15,10 @@ 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)
+}