8000 Merge pull request #50105 from jsternberg/revert-build-dangling · moby/moby@45873be · GitHub
[go: up one dir, main page]
Skip to content

Commit 45873be

Browse files
vvolandvvoland
authored
Merge pull request #50105 from jsternberg/revert-build-dangling
Revert "containerd: images overridden by a build are kept dangling"
2 parents f144264 + 7994426 commit 45873be

File tree

5 files changed

+51
-53
lines changed

5 files changed

+51
-53
lines changed

‎builder/builder-next/controller.go

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,6 @@ import (
2121
"github.com/docker/docker/builder/builder-next/adapters/containerimage"
2222
"github.com/docker/docker/builder/builder-next/adapters/localinlinecache"
2323
"github.com/docker/docker/builder/builder-next/adapters/snapshot"
24-
"github.com/docker/docker/builder/builder-next/exporter"
2524
"github.com/docker/docker/builder/builder-next/exporter/mobyexporter"
2625
"github.com/docker/docker/builder/builder-next/imagerefchecker"
2726
mobyworker "github.com/docker/docker/builder/builder-next/worker"
@@ -169,10 +168,7 @@ func newSnapshotterController(ctx context.Context, rt http.RoundTripper, opt Opt
169168
}
170169
wo.Executor = exec
171170

172-
w, err := mobyworker.NewContainerdWorker(ctx, wo, exporter.Opt{
173-
Callbacks: opt.Callbacks,
174-
ImageTagger: opt.ImageTagger,
175-
}, rt)
171+
w, err := mobyworker.NewContainerdWorker(ctx, wo, opt.Callbacks, rt)
176172
if err != nil {
177173
return nil, err
178174
}

‎builder/builder-next/exporter/wrapper.go

Lines changed: 37 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -2,49 +2,39 @@ package exporter
22

33
import (
44
"context"
5-
"fmt"
65
"strings"
76

8-
distref "github.com/distribution/reference"
7+
"github.com/containerd/log"
8+
"github.com/distribution/reference"
99
"github.com/docker/docker/builder/builder-next/exporter/overrides"
10-
"github.com/docker/docker/image"
1110
"github.com/moby/buildkit/exporter"
1211
"github.com/moby/buildkit/exporter/containerimage/exptypes"
13-
"github.com/moby/buildkit/util/progress"
12+
1413
ocispec "github.com/opencontainers/image-spec/specs-go/v1"
1514
)
1615

17-
// Opt are options for the exporter wrapper.
18-
type Opt struct {
19-
// Callbacks contains callbacks used by the image exporter.
20-
Callbacks BuildkitCallbacks
21-
22-
// ImageTagger is used to tag the image after it is exported.
23-
ImageTagger ImageTagger
24-
}
25-
2616
type BuildkitCallbacks struct {
2717
// Exported is a Called when an image is exported by buildkit.
2818
Exported func(ctx context.Context, id string, desc ocispec.Descriptor)
29-
}
3019

31-
type ImageTagger interface {
32-
TagImage(ctx context.Context, imageID image.ID, newTag distref.Named) error
20+
// Named is a callback that is called when an image is created in the
21+
// containerd image store by buildkit.
22+
Named func(ctx context.Context, ref reference.NamedTagged, desc ocispec.Descriptor)
3323
}
3424

3525
// Wraps the containerimage exporter's Resolve method to apply moby-specific
3626
// overrides to the exporter attributes.
3727
type imageExporterMobyWrapper struct {
38-
exp exporter.Exporter
39-
opt Opt
28+
exp exporter.Exporter
29+
callbacks BuildkitCallbacks
4030
}
4131

4232
// NewWrapper returns an exporter wrapper that applies moby specific attributes
4333
// and hooks the export process.
44-
func NewWrapper(exp exporter.Exporter, opt Opt) (exporter.Exporter, error) {
34+
func NewWrapper(exp exporter.Exporter, callbacks BuildkitCallbacks) (exporter.Exporter, error) {
4535
return &imageExporterMobyWrapper{
46-
exp: exp,
47-
opt: opt,
36+
exp: exp,
37+
callbacks: callbacks,
4838
}, nil
4939
}
5040

@@ -57,9 +47,7 @@ func (e *imageExporterMobyWrapper) Resolve(ctx context.Context, id int, exporter
5747
if err != nil {
5848
return nil, err
5949
}
60-
61-
// Force the exporter to not use a name so it always creates a dangling image.
62-
exporterAttrs[string(exptypes.OptKeyName)] = ""
50+
exporterAttrs[string(exptypes.OptKeyName)] = strings.Join(reposAndTags, ",")
6351
exporterAttrs[string(exptypes.OptKeyUnpack)] = "true"
6452
if _, has := exporterAttrs[string(exptypes.OptKeyDanglingPrefix)]; !has {
6553
exporterAttrs[string(exptypes.OptKeyDanglingPrefix)] = "moby-dangling"
@@ -73,15 +61,13 @@ func (e *imageExporterMobyWrapper) Resolve(ctx context.Context, id int, exporter
7361

7462
return &imageExporterInstanceWrapper{
7563
ExporterInstance: inst,
76-
reposAndTags: reposAndTags,
77-
opt: e.opt,
64+
callbacks: e.callbacks,
7865
}, nil
7966
}
8067

8168
type imageExporterInstanceWrapper struct {
8269
exporter.ExporterInstance
83-
reposAndTags []string
84-
opt Opt
70+
callbacks BuildkitCallbacks
8571
}
8672

8773
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
9278

9379
desc := ref.Descriptor()
9480
imageID := out[exptypes.ExporterImageDigestKey]
95-
if i.opt.Callbacks.Exported != nil {
96-
i.opt.Callbacks.Exported(ctx, imageID, desc)
81+
if i.callbacks.Exported != nil {
82+
i.callbacks.Exported(ctx, imageID, desc)
83+
}
84+
85+
if i.callbacks.Named != nil {
86+
i.processNamedCallback(ctx, out, desc)
9787
}
9888

99-
err = i.processNamed(ctx, image.ID(imageID), out, desc)
100-
return out, ref, err
89+
return out, ref, nil
10190
}
10291

103-
func (i *imageExporterInstanceWrapper) processNamed(ctx context.Context, imageID image.ID, out map[string]string, desc ocispec.Descriptor) error {
104-
if len(i.reposAndTags) == 0 {
105-
return nil
92+
func (i *imageExporterInstanceWrapper) processNamedCallback(ctx context.Context, out map[string]string, desc ocispec.Descriptor) {
93+
// TODO(vvoland): Change to exptypes.ExporterImageNameKey when BuildKit v0.21 is vendored.
94+
imageName := out["image.name"]
95+
if imageName == "" {
96+
log.G(ctx).Warn("image named with empty image.name produced by buildkit")
97+
return
10698
}
10799

108-
for _, repoAndTag := range i.reposAndTags {
109-
newTag, err := distref.ParseNamed(repoAndTag)
100+
for _, name := range strings.Split(imageName, ",") {
101+
ref, err := reference.ParseNormalizedNamed(name)
110102
if err != nil {
111-
return err
103+
// Shouldn't happen, but log if it does and continue.
104+
log.G(ctx).WithFields(log.Fields{
105+
"name": name,
106+
"error": err,
107+
}).Warn("image named with invalid reference produced by buildkit")
108+
continue
112109
}
113110

114-
done := progress.OneOff(ctx, fmt.Sprintf("naming to %s", newTag))
115-
if err := i.opt.ImageTagger.TagImage(ctx, imageID, newTag); err != nil {
116-
return done(err)
111+
if namedTagged, ok := reference.TagNameOnly(ref).(reference.NamedTagged); ok {
112+
i.callbacks.Named(ctx, namedTagged, desc)
117113
}
118-
done(nil)
119114
}
120-
out[exptypes.ExporterImageNameKey] = strings.Join(i.reposAndTags, ",")
121-
return nil
122115
}

‎builder/builder-next/worker/containerdworker.go

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -16,11 +16,11 @@ import (
1616
// ContainerdWorker is a local worker instance with dedicated snapshotter, cache, and so on.
1717
type ContainerdWorker struct {
1818
*base.Worker
19-
opt exporter.Opt
19+
callbacks exporter.BuildkitCallbacks
2020
}
2121

2222
// NewContainerdWorker instantiates a local worker.
23-
func NewContainerdWorker(ctx context.Context, wo base.WorkerOpt, opt exporter.Opt, rt nethttp.RoundTripper) (*ContainerdWorker, error) {
23+
func NewContainerdWorker(ctx context.Context, wo base.WorkerOpt, callbacks exporter.BuildkitCallbacks, rt nethttp.RoundTripper) (*ContainerdWorker, error) {
2424
bw, err := base.NewWorker(ctx, wo)
2525
if err != nil {
2626
return nil, err
@@ -35,7 +35,7 @@ func NewContainerdWorker(ctx context.Context, wo base.WorkerOpt, opt exporter.Op
3535
log.G(ctx).Warnf("Could not register builder http source: %s", err)
3636
}
3737

38-
return &ContainerdWorker{Worker: bw, opt: opt}, nil
38+
return &ContainerdWorker{Worker: bw, callbacks: callbacks}, nil
3939
}
4040

4141
// Exporter returns exporter by name
@@ -46,7 +46,7 @@ func (w *ContainerdWorker) Exporter(name string, sm *session.Manager) (bkexporte
4646
if err != nil {
4747
return nil, err
4848
}
49-
return exporter.NewWrapper(exp, w.opt)
49+
return exporter.NewWrapper(exp, w.callbacks)
5050
default:
5151
return w.Worker.Exporter(name, sm)
5252
}

‎ EED3 cmd/dockerd/daemon.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -431,6 +431,7 @@ func initBuildkit(ctx context.Context, d *daemon.Daemon) (_ builderOptions, clos
431431
ContainerdNamespace: cfg.ContainerdNamespace,
432432
Callbacks: exporter.BuildkitCallbacks{
433433
Exported: d.ImageExportedByBuildkit,
434+
Named: d.ImageNamedByBuildkit,
434435
},
435436
CDISpecDirs: cdiSpecDirs,
436437
})

‎daemon/build.go

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ package daemon
33
import (
44
"context"
55

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

0 commit comments

Comments
 (0)
0