Skip cache lookup for "FROM scratch" in containerd#45818
Merged
neersighted merged 1 commit intomoby:masterfrom Jun 27, 2023
Merged
Skip cache lookup for "FROM scratch" in containerd#45818neersighted merged 1 commit intomoby:masterfrom
neersighted merged 1 commit intomoby:masterfrom
Conversation
Ideally, this should actually do a lookup across images that have no parent, but I wasn't 100% sure how to accomplish that so I opted for the smaller change of having `FROM scratch` builds not be cached for now. Signed-off-by: Tianon Gravi <admwiggin@gmail.com>
Member
Author
the well-placed `panic()` I used to narrow down where this was breaking:2023/06/26 22:34:54 http: panic serving @: wtf tianon
goroutine 273 [running]:
net/http.(*conn).serve.func1()
net/http/server.go:1854 +0xbf
panic({0x1fb07c0, 0x27b49e0})
runtime/panic.go:890 +0x263
github.com/docker/distribution/reference.Parse({0xc000fcfe30, 0x12})
github.com/docker/distribution@v2.8.2+incompatible/reference/reference.go:198 +0x2c5
github.com/docker/distribution/reference.ParseNormalizedNamed({0x0, 0x0})
github.com/docker/distribution@v2.8.2+incompatible/reference/normalize.go:48 +0x16b
github.com/docker/distribution/reference.ParseAnyReference({0x0, 0x0})
github.com/docker/distribution@v2.8.2+incompatible/reference/normalize.go:181 +0x9f
github.com/docker/docker/daemon/containerd.(*ImageService).resolveImage(0xc0010d3080, {0x27df0b8, 0xc0001a6008}, {0x0, 0x0})
github.com/docker/docker/daemon/containerd/image.go:232 +0x7e
github.com/docker/docker/daemon/containerd.(*ImageService).resolveDescriptor(...)
github.com/docker/docker/daemon/containerd/image.go:223
github.com/docker/docker/daemon/containerd.(*ImageService).GetImage(0xc0010d3080, {0x27df0b8, 0xc0001a6008}, {0x0?, 0xc0010122a8?}, {0x0?, 0xc0?})
github.com/docker/docker/daemon/containerd/image.go:36 +0xbb
github.com/docker/docker/daemon/containerd.(*imageCache).GetCache(0xc0010aa160, {0x0?, 0x100c0005a8780?}, 0x48cdfe?)
github.com/docker/docker/daemon/containerd/cache.go:34 +0x72
github.com/docker/docker/builder/dockerfile.(*imageProber).Probe(0xc0010aa060, {0x0?, 0x4f?}, 0xc001093400)
github.com/docker/docker/builder/dockerfile/imageprobe.go:58 +0x4a
github.com/docker/docker/builder/dockerfile.(*Builder).probeCache(0xc0010f6300, 0xc00109f9d0, 0x1?)
github.com/docker/docker/builder/dockerfile/internals.go:331 +0x4a
github.com/docker/docker/builder/dockerfile.(*Builder).performCopy(0xc0010f6300, {0x27df080, 0xc000ff5900}, {0xc00109f9d0, 0xc0010a80c8, 0xc0010f6300, {0x27d3e00, 0xc000ec43f0}, 0xc0010aa0c0}, {{0x2420cd9, ...}, ...})
github.com/docker/docker/builder/dockerfile/internals.go:133 +0x2f7
github.com/docker/docker/builder/dockerfile.dispatchAdd({0x27df080, 0xc000ff5900}, {0xc00109f9d0, 0xc0010a80c8, 0xc0010f6300, {0x27d3e00, 0xc000ec43f0}, 0xc0010aa0c0}, 0xc000cd0370)
github.com/docker/docker/builder/dockerfile/dispatchers.go:105 +0x4f9
github.com/docker/docker/builder/dockerfile.dispatch({0x27df080, 0xc000ff5900}, {0xc00109f9d0, 0xc0010a80c8, 0xc0010f6300, {0x27d3e00, 0xc000ec43f0}, 0xc0010aa0c0}, {0x27cb670, 0xc000cd0370})
github.com/docker/docker/builder/dockerfile/evaluator.go:76 +0x5bb
github.com/docker/docker/builder/dockerfile.(*Builder).dispatchDockerfileWithCancellation(0xc0010f6300, {0x27df080, 0xc000ff5900}, {0xc00109a580, 0x1, 0xc000ec43c0?}, {0x0, 0x0, 0x0?}, 0x5c, ...)
github.com/docker/docker/builder/dockerfile/builder.go:296 +0x8f2
github.com/docker/docker/builder/dockerfile.(*Builder).build(0xc0010f6300, {0x27df080, 0xc000ff5900}, {0x27d3e00, 0xc000ec43f0}, 0xc001094a50)
github.com/docker/docker/builder/dockerfile/builder.go:211 +0x2e5
github.com/docker/docker/builder/dockerfile.(*BuildManager).Build(0xc00091e500, {0x27df080, 0xc000ff5900}, {{0x27ca9c8, 0xc000ec4240}, {{0x27bc700, 0xc000198120}, {0x27bd380, 0xc000198138}, {0x27bd380, ...}, ...}, ...})
github.com/docker/docker/builder/dockerfile/builder.go:98 +0x385
github.com/docker/docker/api/server/backend/build.(*Backend).Build(0xc0008e06f0, {0x27df128, 0xc000ec41b0}, {{0x27ca9c8, 0xc000ec4240}, {{0x27bc700, 0xc000198120}, {0x27bd380, 0xc000198138}, {0x27bd380, ...}, ...}, ...})
github.com/docker/docker/api/server/backend/build/backend.go:69 +0x186
github.com/docker/docker/api/server/router/build.(*buildRouter).postBuild(0xc00056c980, {0x27df128, 0xc000ec41b0}, {0x27ddac0, 0xc0000fe540}, 0xc0009a0800, 0xc000c02160?)
github.com/docker/docker/api/server/router/build/build_routes.go:280 +0x7c6
github.com/docker/docker/api/server/middleware.ExperimentalMiddleware.WrapHandler.func1({0x27df128, 0xc000ec41b0}, {0x27ddac0?, 0xc0000fe540?}, 0x1fb07c0?, 0xc000e1a0d0?)
github.com/docker/docker/api/server/middleware/experimental.go:26 +0x15b
github.com/docker/docker/api/server/middleware.VersionMiddleware.WrapHandler.func1({0x27df128, 0xc000ec4180}, {0x27ddac0, 0xc0000fe540}, 0xc000efa080?, 0x0?)
github.com/docker/docker/api/server/middleware/version.go:62 +0x4d7
github.com/docker/docker/pkg/authorization.(*Middleware).WrapHandler.func1({0x27df128, 0xc000ec4180}, {0x27ddac0?, 0xc0000fe540?}, 0xc0009a0800, 0xc000e1a080?)
github.com/docker/docker/pkg/authorization/middleware.go:59 +0x6b4
github.com/docker/docker/api/server.(*Server).makeHTTPHandler.func1({0x27ddac0, 0xc0000fe540}, 0xc0009a0700)
github.com/docker/docker/api/server/server.go:53 +0x1ce
net/http.HandlerFunc.ServeHTTP(0xc0009a0600?, {0x27ddac0?, 0xc0000fe540?}, 0x800?)
net/http/server.go:2122 +0x2f
github.com/gorilla/mux.(*Router).ServeHTTP(0xc0010f6240, {0x27ddac0, 0xc0000fe540}, 0xc0009a0300)
github.com/gorilla/mux@v1.8.0/mux.go:210 +0x1cf
net/http.serverHandler.ServeHTTP({0x27ce5a0?}, {0x27ddac0, 0xc0000fe540}, 0xc0009a0300)
net/http/server.go:2936 +0x316
net/http.(*conn).serve(0xc000628000, {0x27df128, 0xc001094810})
net/http/server.go:1995 +0x612
created by net/http.(*Server).Serve
net/http/server.go:3089 +0x5ed |
Member
Author
|
Ok, here's an implementation of diff --git a/daemon/containerd/cache.go b/daemon/containerd/cache.go
index 5e696c5967..5b4ec557c6 100644
--- a/daemon/containerd/cache.go
+++ b/daemon/containerd/cache.go
@@ -32,14 +32,13 @@ type imageCache struct {
func (ic *imageCache) GetCache(parentID string, cfg *container.Config) (imageID string, err error) {
ctx := context.TODO()
- if parentID == "" {
- // TODO handle "parentless" image cache lookups ("FROM scratch")
- return "", nil
- }
-
- parent, err := ic.c.GetImage(ctx, parentID, imagetype.GetImageOpts{})
- if err != nil {
- return "", err
+ var parent *image.Image
+ if parentID != "" { // not "FROM scratch"
+ var err error
+ parent, err = ic.c.GetImage(ctx, parentID, imagetype.GetImageOpts{})
+ if err != nil {
+ return "", err
+ }
}
for _, localCachedImage := range ic.images {
@@ -48,6 +47,24 @@ func (ic *imageCache) GetCache(parentID string, cfg *container.Config) (imageID
}
}
+ if parent == nil { // "FROM scratch"
+ imgs, err := ic.c.client.ImageService().List(ctx) // TODO is this right??
+ if err != nil {
+ return "", err
+ }
+ for _, img := range imgs {
+ id := img.Target.Digest.String()
+ image, err := ic.c.GetImage(ctx, id, imagetype.GetImageOpts{})
+ if err != nil {
+ return "", err
+ }
+ if isMatch(image, parent, cfg) {
+ return id, nil
+ }
+ }
+ return "", nil
+ }
+
children, err := ic.c.Children(ctx, parent.ID())
if err != nil {
return "", err
@@ -73,10 +90,19 @@ func (ic *imageCache) GetCache(parentID string, cfg *container.Config) (imageID
// 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 {
+ if target == nil || cfg == nil {
return false
}
+ childCreatedBy := target.History[len(target.History)-1].CreatedBy
+ if childCreatedBy != strings.Join(cfg.Cmd, " ") {
+ return false
+ }
+
+ if parent == nil { // "FROM scratch"
+ return len(target.History) == 1 && len(target.RootFS.DiffIDs) == 1
+ }
+
if len(target.History) != len(parent.History)+1 ||
len(target.RootFS.DiffIDs) != len(parent.RootFS.DiffIDs)+1 {
return false
@@ -88,6 +114,5 @@ func isMatch(target, parent *image.Image, cfg *container.Config) bool {
}
}
- childCreatedBy := target.History[len(target.History)-1].CreatedBy
- return childCreatedBy == strings.Join(cfg.Cmd, " ")
+ return true
} |
Member
Author
|
(I guess my proposal for the v25 milestone @thaJeztah put this in would be that I propose that diff as a draft in a follow-up, since just not ever considering cache for |
Member
Author
|
Oh, a simpler reproducer is this: FROM scratch
CMD []I guess we should probably put that in an integration test somewhere? 👀 If we add FROM scratch
ENTRYPOINT []
CMD []Edit: oh, looking through #45232, there are quite a few tests in "integration-cli (DockerCLIBuildSuite)" which caught this very error 😂 🙈 ❤️ |
Member
|
I did a quick rebase of #45232, and cherry-picked this PR into that branch 👍 |
vvoland
approved these changes
Jun 27, 2023
neersighted
approved these changes
Jun 27, 2023
Member
Author
|
Welp, I think this only fixes the |
Member
Author
|
(however, instances of "invalid reference format" in the logs for that integration-cli test went down from 78 to 68, so that's something!) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Ideally, this should actually do a lookup across images that have no parent, but I wasn't 100% sure how to accomplish that so I opted for the smaller change of having
FROM scratchbuilds not be cached for now.Before:
After: