From 6d05b9b65b117d13344d9f71dcefca5ede9fb498 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pawe=C5=82=20Gronowski?= Date: Fri, 1 Dec 2023 10:24:23 +0100 Subject: [PATCH 1/5] image/cache: Compare all config fields MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add checks for some image config fields that were missing. Signed-off-by: Paweł Gronowski (cherry picked from commit 537348763faeb7adc515305e87910e11bdb14b00) Signed-off-by: Paweł Gronowski --- daemon/containerd/cache.go | 61 +------------- image/cache/compare.go | 158 ++++++++++++++++++++++++++++++------- 2 files changed, 130 insertions(+), 89 deletions(-) diff --git a/daemon/containerd/cache.go b/daemon/containerd/cache.go index 804f4c7926..787dde5e39 100644 --- a/daemon/containerd/cache.go +++ b/daemon/containerd/cache.go @@ -11,6 +11,7 @@ import ( "github.com/docker/docker/builder" "github.com/docker/docker/errdefs" "github.com/docker/docker/image" + "github.com/docker/docker/image/cache" "github.com/opencontainers/go-digest" ocispec "github.com/opencontainers/image-spec/specs-go/v1" ) @@ -98,7 +99,7 @@ func (ic *localCache) GetCache(parentID string, cfg *container.Config) (imageID return "", err } - if isMatch(&cc, cfg) { + if cache.CompareConfig(&cc, cfg) { childImage, err := ic.imageService.GetImage(ctx, child.String(), imagetype.GetImageOpts{}) if err != nil { return "", err @@ -206,61 +207,3 @@ func (ic *imageCache) isParent(ctx context.Context, img *image.Image, parentID i } return ic.isParent(ctx, p, parentID) } - -// compare two Config struct. Do not compare the "Image" nor "Hostname" fields -// If OpenStdin is set, then it differs -func isMatch(a, b *container.Config) bool { - if a == nil || b == nil || - a.OpenStdin || b.OpenStdin { - return false - } - if a.AttachStdout != b.AttachStdout || - a.AttachStderr != b.AttachStderr || - a.User != b.User || - a.OpenStdin != b.OpenStdin || - a.Tty != b.Tty { - return false - } - - if len(a.Cmd) != len(b.Cmd) || - len(a.Env) != len(b.Env) || - len(a.Labels) != len(b.Labels) || - len(a.ExposedPorts) != len(b.ExposedPorts) || - len(a.Entrypoint) != len(b.Entrypoint) || - len(a.Volumes) != len(b.Volumes) { - return false - } - - for i := 0; i < len(a.Cmd); i++ { - if a.Cmd[i] != b.Cmd[i] { - return false - } - } - for i := 0; i < len(a.Env); i++ { - if a.Env[i] != b.Env[i] { - return false - } - } - for k, v := range a.Labels { - if v != b.Labels[k] { - return false - } - } - for k := range a.ExposedPorts { - if _, exists := b.ExposedPorts[k]; !exists { - return false - } - } - - for i := 0; i < len(a.Entrypoint); i++ { - if a.Entrypoint[i] != b.Entrypoint[i] { - return false - } - } - for key := range a.Volumes { - if _, exists := b.Volumes[key]; !exists { - return false - } - } - return true -} diff --git a/image/cache/compare.go b/image/cache/compare.go index e31e9c8bdf..ec1971a207 100644 --- a/image/cache/compare.go +++ b/image/cache/compare.go @@ -4,42 +4,69 @@ import ( "github.com/docker/docker/api/types/container" ) -// compare two Config struct. Do not compare the "Image" nor "Hostname" fields -// If OpenStdin is set, then it differs +// TODO: Remove once containerd image service directly uses the ImageCache and +// LocalImageCache structs. +func CompareConfig(a, b *container.Config) bool { + return compare(a, b) +} + +// compare two Config struct. Do not container-specific fields: +// - Image +// - Hostname +// - Domainname +// - MacAddress func compare(a, b *container.Config) bool { - if a == nil || b == nil || - a.OpenStdin || b.OpenStdin { - return false - } - if a.AttachStdout != b.AttachStdout || - a.AttachStderr != b.AttachStderr || - a.User != b.User || - a.OpenStdin != b.OpenStdin || - a.Tty != b.Tty { + if a == nil || b == nil { return false } - if len(a.Cmd) != len(b.Cmd) || - len(a.Env) != len(b.Env) || - len(a.Labels) != len(b.Labels) || - len(a.ExposedPorts) != len(b.ExposedPorts) || - len(a.Entrypoint) != len(b.Entrypoint) || - len(a.Volumes) != len(b.Volumes) { + if len(a.Env) != len(b.Env) { + return false + } + if len(a.Cmd) != len(b.Cmd) { + return false + } + if len(a.Entrypoint) != len(b.Entrypoint) { + return false + } + if len(a.Shell) != len(b.Shell) { + return false + } + if len(a.ExposedPorts) != len(b.ExposedPorts) { + return false + } + if len(a.Volumes) != len(b.Volumes) { + return false + } + if len(a.Labels) != len(b.Labels) { + return false + } + if len(a.OnBuild) != len(b.OnBuild) { return false } - for i := 0; i < len(a.Cmd); i++ { - if a.Cmd[i] != b.Cmd[i] { - return false - } - } for i := 0; i < len(a.Env); i++ { if a.Env[i] != b.Env[i] { return false } } - for k, v := range a.Labels { - if v != b.Labels[k] { + for i := 0; i < len(a.OnBuild); i++ { + if a.OnBuild[i] != b.OnBuild[i] { + return false + } + } + for i := 0; i < len(a.Cmd); i++ { + if a.Cmd[i] != b.Cmd[i] { + return false + } + } + for i := 0; i < len(a.Entrypoint); i++ { + if a.Entrypoint[i] != b.Entrypoint[i] { + return false + } + } + for i := 0; i < len(a.Shell); i++ { + if a.Shell[i] != b.Shell[i] { return false } } @@ -48,16 +75,87 @@ func compare(a, b *container.Config) bool { return false } } - - for i := 0; i < len(a.Entrypoint); i++ { - if a.Entrypoint[i] != b.Entrypoint[i] { - return false - } - } for key := range a.Volumes { if _, exists := b.Volumes[key]; !exists { return false } } + for k, v := range a.Labels { + if v != b.Labels[k] { + return false + } + } + + if a.AttachStdin != b.AttachStdin { + return false + } + if a.AttachStdout != b.AttachStdout { + return false + } + if a.AttachStderr != b.AttachStderr { + return false + } + if a.NetworkDisabled != b.NetworkDisabled { + return false + } + if a.Tty != b.Tty { + return false + } + if a.OpenStdin != b.OpenStdin { + return false + } + if a.StdinOnce != b.StdinOnce { + return false + } + if a.ArgsEscaped != b.ArgsEscaped { + return false + } + if a.User != b.User { + return false + } + if a.WorkingDir != b.WorkingDir { + return false + } + if a.StopSignal != b.StopSignal { + return false + } + + if (a.StopTimeout == nil) != (b.StopTimeout == nil) { + return false + } + if a.StopTimeout != nil && b.StopTimeout != nil { + if *a.StopTimeout != *b.StopTimeout { + return false + } + } + if (a.Healthcheck == nil) != (b.Healthcheck == nil) { + return false + } + if a.Healthcheck != nil && b.Healthcheck != nil { + if a.Healthcheck.Interval != b.Healthcheck.Interval { + return false + } + if a.Healthcheck.StartInterval != b.Healthcheck.StartInterval { + return false + } + if a.Healthcheck.StartPeriod != b.Healthcheck.StartPeriod { + return false + } + if a.Healthcheck.Timeout != b.Healthcheck.Timeout { + return false + } + if a.Healthcheck.Retries != b.Healthcheck.Retries { + return false + } + if len(a.Healthcheck.Test) != len(b.Healthcheck.Test) { + return false + } + for i := 0; i < len(a.Healthcheck.Test); i++ { + if a.Healthcheck.Test[i] != b.Healthcheck.Test[i] { + return false + } + } + } + return true } From be7b60ef05a6d5c48826600ee07059f08b08c535 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pawe=C5=82=20Gronowski?= Date: Fri, 1 Dec 2023 10:24:25 +0100 Subject: [PATCH 2/5] daemon/imageStore: Mark images built locally MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Store additional image property which makes it possible to distinguish if image was built locally. Signed-off-by: Paweł Gronowski (cherry picked from commit c6156dc51bb74eef1b606376c576ce944b97bac6) Signed-off-by: Paweł Gronowski --- daemon/images/image_builder.go | 3 +++ daemon/images/image_commit.go | 3 +++ image/store.go | 20 ++++++++++++++++++++ 3 files changed, 26 insertions(+) diff --git a/daemon/images/image_builder.go b/daemon/images/image_builder.go index 5ff81d8954..283dcebcb3 100644 --- a/daemon/images/image_builder.go +++ b/daemon/images/image_builder.go @@ -255,6 +255,9 @@ func (i *ImageService) CreateImage(ctx context.Context, config []byte, parent st return nil, errors.Wrapf(err, "failed to set parent %s", parent) } } + if err := i.imageStore.SetBuiltLocally(id); err != nil { + return nil, errors.Wrapf(err, "failed to mark image %s as built locally", id) + } return i.imageStore.Get(id) } diff --git a/daemon/images/image_commit.go b/daemon/images/image_commit.go index f620b41790..00ce4fbc07 100644 --- a/daemon/images/image_commit.go +++ b/daemon/images/image_commit.go @@ -62,6 +62,9 @@ func (i *ImageService) CommitImage(ctx context.Context, c backend.CommitConfig) if err != nil { return "", err } + if err := i.imageStore.SetBuiltLocally(id); err != nil { + return "", err + } if c.ParentImageID != "" { if err := i.imageStore.SetParent(id, image.ID(c.ParentImageID)); err != nil { diff --git a/image/store.go b/image/store.go index 41a720bd02..3671436125 100644 --- a/image/store.go +++ b/image/store.go @@ -3,6 +3,7 @@ package image // import "github.com/docker/docker/image" import ( "context" "fmt" + "os" "sync" "time" @@ -24,6 +25,8 @@ type Store interface { GetParent(id ID) (ID, error) SetLastUpdated(id ID) error GetLastUpdated(id ID) (time.Time, error) + SetBuiltLocally(id ID) error + IsBuiltLocally(id ID) (bool, error) Children(id ID) []ID Map() map[ID]*Image Heads() map[ID]*Image @@ -295,6 +298,23 @@ func (is *store) GetLastUpdated(id ID) (time.Time, error) { return time.Parse(time.RFC3339Nano, string(bytes)) } +// SetBuiltLocally sets whether image can be used as a builder cache +func (is *store) SetBuiltLocally(id ID) error { + return is.fs.SetMetadata(id.Digest(), "builtLocally", []byte{1}) +} + +// IsBuiltLocally returns whether image can be used as a builder cache +func (is *store) IsBuiltLocally(id ID) (bool, error) { + bytes, err := is.fs.GetMetadata(id.Digest(), "builtLocally") + if err != nil || len(bytes) == 0 { + if errors.Is(err, os.ErrNotExist) { + err = nil + } + return false, err + } + return bytes[0] == 1, nil +} + func (is *store) Children(id ID) []ID { is.RLock() defer is.RUnlock() From 05a370f52f98f85f8d234ef68dd697cdfcd215dc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pawe=C5=82=20Gronowski?= Date: Fri, 1 Dec 2023 10:24:27 +0100 Subject: [PATCH 3/5] image/cache: Restrict cache candidates to locally built images MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Restrict cache candidates only to images that were built locally. This doesn't affect builds using `--cache-from`. Signed-off-by: Paweł Gronowski (cherry picked from commit 96ac22768a8a745b1fb5e4531eaca8d2ad7327ad) Signed-off-by: Paweł Gronowski --- image/cache/cache.go | 56 ++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 54 insertions(+), 2 deletions(-) diff --git a/image/cache/cache.go b/image/cache/cache.go index b36534e572..7bc4089a41 100644 --- a/image/cache/cache.go +++ b/image/cache/cache.go @@ -1,11 +1,13 @@ package cache // import "github.com/docker/docker/image/cache" import ( + "context" "encoding/json" "fmt" "reflect" "strings" + "github.com/containerd/log" containertypes "github.com/docker/docker/api/types/container" "github.com/docker/docker/dockerversion" "github.com/docker/docker/image" @@ -216,6 +218,22 @@ func getImageIDAndError(img *image.Image, err error) (string, error) { // created. nil is returned if a child cannot be found. An error is // returned if the parent image cannot be found. func getLocalCachedImage(imageStore image.Store, imgID image.ID, config *containertypes.Config) (*image.Image, error) { + if config == nil { + return nil, nil + } + + isBuiltLocally := func(id image.ID) bool { + builtLocally, err := imageStore.IsBuiltLocally(id) + if err != nil { + log.G(context.TODO()).WithFields(log.Fields{ + "error": err, + "id": id, + }).Warn("failed to check if image was built locally") + return false + } + return builtLocally + } + // Loop on the children of the given image and check the config getMatch := func(siblings []image.ID) (*image.Image, error) { var match *image.Image @@ -225,6 +243,10 @@ func getLocalCachedImage(imageStore image.Store, imgID image.ID, config *contain return nil, fmt.Errorf("unable to find image %q", id) } + if !isBuiltLocally(id) { + continue + } + if compare(&img.ContainerConfig, config) { // check for the most up to date match if img.Created != nil && (match == nil || match.Created.Before(*img.Created)) { @@ -238,11 +260,29 @@ func getLocalCachedImage(imageStore image.Store, imgID image.ID, config *contain // In this case, this is `FROM scratch`, which isn't an actual image. if imgID == "" { images := imageStore.Map() + var siblings []image.ID for id, img := range images { - if img.Parent == imgID { - siblings = append(siblings, id) + if img.Parent != "" { + continue } + + if !isBuiltLocally(id) { + continue + } + + // Do a quick initial filter on the Cmd to avoid adding all + // non-local images with empty parent to the siblings slice and + // performing a full config compare. + // + // config.Cmd is set to the current Dockerfile instruction so we + // check it against the img.ContainerConfig.Cmd which is the + // command of the last layer. + if !strSliceEqual(img.ContainerConfig.Cmd, config.Cmd) { + continue + } + + siblings = append(siblings, id) } return getMatch(siblings) } @@ -251,3 +291,15 @@ func getLocalCachedImage(imageStore image.Store, imgID image.ID, config *contain siblings := imageStore.Children(imgID) return getMatch(siblings) } + +func strSliceEqual(a, b []string) bool { + if len(a) != len(b) { + return false + } + for i := 0; i < len(a); i++ { + if a[i] != b[i] { + return false + } + } + return true +} From f3f5327b48ae2213f59d6440d98198219f3baa3b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pawe=C5=82=20Gronowski?= Date: Fri, 1 Dec 2023 10:24:29 +0100 Subject: [PATCH 4/5] image/cache: Check image platform MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Make sure the cache candidate platform matches the requested. Signed-off-by: Paweł Gronowski (cherry picked from commit 877ebbe038ee914e4243d5b7e198eda00494d757) Signed-off-by: Paweł Gronowski --- builder/builder.go | 3 ++- builder/dockerfile/copy.go | 17 ++--------------- builder/dockerfile/imageprobe.go | 9 +++++---- builder/dockerfile/internals.go | 17 ++++++++++++++++- builder/dockerfile/mockbackend_test.go | 3 ++- daemon/containerd/cache.go | 13 ++++++++----- image/cache/cache.go | 21 ++++++++++++++++----- 7 files changed, 51 insertions(+), 32 deletions(-) diff --git a/builder/builder.go b/builder/builder.go index ab3d2a9f25..fc855f133d 100644 --- a/builder/builder.go +++ b/builder/builder.go @@ -14,6 +14,7 @@ import ( "github.com/docker/docker/image" "github.com/docker/docker/layer" "github.com/opencontainers/go-digest" + ocispec "github.com/opencontainers/image-spec/specs-go/v1" ) const ( @@ -85,7 +86,7 @@ type ImageCacheBuilder interface { type ImageCache interface { // GetCache returns a reference to a cached image whose parent equals `parent` // and runconfig equals `cfg`. A cache miss is expected to return an empty ID and a nil error. - GetCache(parentID string, cfg *container.Config) (imageID string, err error) + GetCache(parentID string, cfg *container.Config, platform ocispec.Platform) (imageID string, err error) } // Image represents a Docker image used by the builder. diff --git a/builder/dockerfile/copy.go b/builder/dockerfile/copy.go index 42b4808db4..62fbfb656d 100644 --- a/builder/dockerfile/copy.go +++ b/builder/dockerfile/copy.go @@ -8,7 +8,6 @@ import ( "net/url" "os" "path/filepath" - "runtime" "sort" "strings" "time" @@ -74,7 +73,7 @@ type copier struct { source builder.Source pathCache pathCache download sourceDownloader - platform *ocispec.Platform + platform ocispec.Platform // for cleanup. TODO: having copier.cleanup() is error prone and hard to // follow. Code calling performCopy should manage the lifecycle of its params. // Copier should take override source as input, not imageMount. @@ -83,19 +82,7 @@ type copier struct { } func copierFromDispatchRequest(req dispatchRequest, download sourceDownloader, imageSource *imageMount) copier { - platform := req.builder.platform - if platform == nil { - // May be nil if not explicitly set in API/dockerfile - platform = &ocispec.Platform{} - } - if platform.OS == "" { - // Default to the dispatch requests operating system if not explicit in API/dockerfile - platform.OS = req.state.operatingSystem - } - if platform.OS == "" { - // This is a failsafe just in case. Shouldn't be hit. - platform.OS = runtime.GOOS - } + platform := req.builder.getPlatform(req.state) return copier{ source: req.source, diff --git a/builder/dockerfile/imageprobe.go b/builder/dockerfile/imageprobe.go index 18c25d6c0c..de00c912e8 100644 --- a/builder/dockerfile/imageprobe.go +++ b/builder/dockerfile/imageprobe.go @@ -6,13 +6,14 @@ import ( "github.com/containerd/log" "github.com/docker/docker/api/types/container" "github.com/docker/docker/builder" + ocispec "github.com/opencontainers/image-spec/specs-go/v1" ) // ImageProber exposes an Image cache to the Builder. It supports resetting a // cache. type ImageProber interface { Reset(ctx context.Context) error - Probe(parentID string, runConfig *container.Config) (string, error) + Probe(parentID string, runConfig *container.Config, platform ocispec.Platform) (string, error) } type resetFunc func(context.Context) (builder.ImageCache, error) @@ -51,11 +52,11 @@ func (c *imageProber) Reset(ctx context.Context) error { // Probe checks if cache match can be found for current build instruction. // It returns the cachedID if there is a hit, and the empty string on miss -func (c *imageProber) Probe(parentID string, runConfig *container.Config) (string, error) { +func (c *imageProber) Probe(parentID string, runConfig *container.Config, platform ocispec.Platform) (string, error) { if c.cacheBusted { return "", nil } - cacheID, err := c.cache.GetCache(parentID, runConfig) + cacheID, err := c.cache.GetCache(parentID, runConfig, platform) if err != nil { return "", err } @@ -74,6 +75,6 @@ func (c *nopProber) Reset(ctx context.Context) error { return nil } -func (c *nopProber) Probe(_ string, _ *container.Config) (string, error) { +func (c *nopProber) Probe(_ string, _ *container.Config, _ ocispec.Platform) (string, error) { return "", nil } diff --git a/builder/dockerfile/internals.go b/builder/dockerfile/internals.go index efe7c66e0c..b6b0b8f42a 100644 --- a/builder/dockerfile/internals.go +++ b/builder/dockerfile/internals.go @@ -10,6 +10,7 @@ import ( "fmt" "strings" + "github.com/containerd/containerd/platforms" "github.com/containerd/log" "github.com/docker/docker/api/types" "github.com/docker/docker/api/types/backend" @@ -328,7 +329,7 @@ func getShell(c *container.Config, os string) []string { } func (b *Builder) probeCache(dispatchState *dispatchState, runConfig *container.Config) (bool, error) { - cachedID, err := b.imageProber.Probe(dispatchState.imageID, runConfig) + cachedID, err := b.imageProber.Probe(dispatchState.imageID, runConfig, b.getPlatform(dispatchState)) if cachedID == "" || err != nil { return false, err } @@ -388,3 +389,17 @@ func hostConfigFromOptions(options *types.ImageBuildOptions) *container.HostConf } return hc } + +func (b *Builder) getPlatform(state *dispatchState) ocispec.Platform { + // May be nil if not explicitly set in API/dockerfile + out := platforms.DefaultSpec() + if b.platform != nil { + out = *b.platform + } + + if state.operatingSystem != "" { + out.OS = state.operatingSystem + } + + return out +} diff --git a/builder/dockerfile/mockbackend_test.go b/builder/dockerfile/mockbackend_test.go index 41c2e045e0..e21e5b9ba5 100644 --- a/builder/dockerfile/mockbackend_test.go +++ b/builder/dockerfile/mockbackend_test.go @@ -13,6 +13,7 @@ import ( "github.com/docker/docker/image" "github.com/docker/docker/layer" "github.com/opencontainers/go-digest" + ocispec "github.com/opencontainers/image-spec/specs-go/v1" ) // MockBackend implements the builder.Backend interface for unit testing @@ -106,7 +107,7 @@ type mockImageCache struct { getCacheFunc func(parentID string, cfg *container.Config) (string, error) } -func (mic *mockImageCache) GetCache(parentID string, cfg *container.Config) (string, error) { +func (mic *mockImageCache) GetCache(parentID string, cfg *container.Config, _ ocispec.Platform) (string, error) { if mic.getCacheFunc != nil { return mic.getCacheFunc(parentID, cfg) } diff --git a/daemon/containerd/cache.go b/daemon/containerd/cache.go index 787dde5e39..92fcf4ae06 100644 --- a/daemon/containerd/cache.go +++ b/daemon/containerd/cache.go @@ -54,7 +54,7 @@ type localCache struct { imageService *ImageService } -func (ic *localCache) GetCache(parentID string, cfg *container.Config) (imageID string, err error) { +func (ic *localCache) GetCache(parentID string, cfg *container.Config, platform ocispec.Platform) (imageID string, err error) { ctx := context.TODO() var children []image.ID @@ -100,8 +100,11 @@ func (ic *localCache) GetCache(parentID string, cfg *container.Config) (imageID } if cache.CompareConfig(&cc, cfg) { - childImage, err := ic.imageService.GetImage(ctx, child.String(), imagetype.GetImageOpts{}) + childImage, err := ic.imageService.GetImage(ctx, child.String(), imagetype.GetImageOpts{Platform: &platform}) if err != nil { + if errdefs.IsNotFound(err) { + continue + } return "", err } @@ -124,10 +127,10 @@ type imageCache struct { lc *localCache } -func (ic *imageCache) GetCache(parentID string, cfg *container.Config) (imageID string, err error) { +func (ic *imageCache) GetCache(parentID string, cfg *container.Config, platform ocispec.Platform) (imageID string, err error) { ctx := context.TODO() - imgID, err := ic.lc.GetCache(parentID, cfg) + imgID, err := ic.lc.GetCache(parentID, cfg, platform) if err != nil { return "", err } @@ -143,7 +146,7 @@ func (ic *imageCache) GetCache(parentID string, cfg *container.Config) (imageID lenHistory := 0 if parentID != "" { - parent, err = ic.imageService.GetImage(ctx, parentID, imagetype.GetImageOpts{}) + parent, err = ic.imageService.GetImage(ctx, parentID, imagetype.GetImageOpts{Platform: &platform}) if err != nil { return "", err } diff --git a/image/cache/cache.go b/image/cache/cache.go index 7bc4089a41..8b7632f5e1 100644 --- a/image/cache/cache.go +++ b/image/cache/cache.go @@ -7,11 +7,13 @@ import ( "reflect" "strings" + "github.com/containerd/containerd/platforms" "github.com/containerd/log" containertypes "github.com/docker/docker/api/types/container" "github.com/docker/docker/dockerversion" "github.com/docker/docker/image" "github.com/docker/docker/layer" + ocispec "github.com/opencontainers/image-spec/specs-go/v1" "github.com/pkg/errors" ) @@ -28,8 +30,8 @@ type LocalImageCache struct { } // GetCache returns the image id found in the cache -func (lic *LocalImageCache) GetCache(imgID string, config *containertypes.Config) (string, error) { - return getImageIDAndError(getLocalCachedImage(lic.store, image.ID(imgID), config)) +func (lic *LocalImageCache) GetCache(imgID string, config *containertypes.Config, platform ocispec.Platform) (string, error) { + return getImageIDAndError(getLocalCachedImage(lic.store, image.ID(imgID), config, platform)) } // New returns an image cache, based on history objects @@ -53,8 +55,8 @@ func (ic *ImageCache) Populate(image *image.Image) { } // GetCache returns the image id found in the cache -func (ic *ImageCache) GetCache(parentID string, cfg *containertypes.Config) (string, error) { - imgID, err := ic.localImageCache.GetCache(parentID, cfg) +func (ic *ImageCache) GetCache(parentID string, cfg *containertypes.Config, platform ocispec.Platform) (string, error) { + imgID, err := ic.localImageCache.GetCache(parentID, cfg, platform) if err != nil { return "", err } @@ -217,7 +219,7 @@ func getImageIDAndError(img *image.Image, err error) (string, error) { // of the image with imgID, that had the same config when it was // created. nil is returned if a child cannot be found. An error is // returned if the parent image cannot be found. -func getLocalCachedImage(imageStore image.Store, imgID image.ID, config *containertypes.Config) (*image.Image, error) { +func getLocalCachedImage(imageStore image.Store, imgID image.ID, config *containertypes.Config, platform ocispec.Platform) (*image.Image, error) { if config == nil { return nil, nil } @@ -247,6 +249,15 @@ func getLocalCachedImage(imageStore image.Store, imgID image.ID, config *contain continue } + imgPlatform := img.Platform() + // Discard old linux/amd64 images with empty platform. + if imgPlatform.OS == "" && imgPlatform.Architecture == "" { + continue + } + if !platforms.OnlyStrict(platform).Match(imgPlatform) { + continue + } + if compare(&img.ContainerConfig, config) { // check for the most up to date match if img.Created != nil && (match == nil || match.Created.Before(*img.Created)) { From d5eebf9e1938bf0097ed205ccdbf18ebd214bece Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pawe=C5=82=20Gronowski?= Date: Thu, 11 Jan 2024 14:04:02 +0100 Subject: [PATCH 5/5] builder/windows: Don't set ArgsEscaped for RUN cache probe MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously this was done indirectly - the `compare` function didn't check the `ArgsEscaped`. Signed-off-by: Paweł Gronowski (cherry picked from commit 96d461d27e8da97b64620c94a4a1b2448511c058) Signed-off-by: Paweł Gronowski --- builder/dockerfile/dispatchers.go | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/builder/dockerfile/dispatchers.go b/builder/dockerfile/dispatchers.go index 585d5fe033..7183042012 100644 --- a/builder/dockerfile/dispatchers.go +++ b/builder/dockerfile/dispatchers.go @@ -348,9 +348,16 @@ func dispatchRun(ctx context.Context, d dispatchRequest, c *instructions.RunComm saveCmd = prependEnvOnCmd(d.state.buildArgs, buildArgs, cmdFromArgs) } + cacheArgsEscaped := argsEscaped + // ArgsEscaped is not persisted in the committed image on Windows. + // Use the original from previous build steps for cache probing. + if d.state.operatingSystem == "windows" { + cacheArgsEscaped = stateRunConfig.ArgsEscaped + } + runConfigForCacheProbe := copyRunConfig(stateRunConfig, withCmd(saveCmd), - withArgsEscaped(argsEscaped), + withArgsEscaped(cacheArgsEscaped), withEntrypointOverride(saveCmd, nil)) if hit, err := d.builder.probeCache(d.state, runConfigForCacheProbe); err != nil || hit { return err