diff --git a/builder/builder.go b/builder/builder.go index f01563812f..e74fc54a49 100644 --- a/builder/builder.go +++ b/builder/builder.go @@ -15,6 +15,7 @@ import ( "github.com/docker/docker/image" "github.com/docker/docker/layer" "github.com/docker/docker/pkg/containerfs" + ocispec "github.com/opencontainers/image-spec/specs-go/v1" ) const ( @@ -89,7 +90,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 2c413ca047..b052527a1f 100644 --- a/builder/dockerfile/copy.go +++ b/builder/dockerfile/copy.go @@ -9,7 +9,6 @@ import ( "net/url" "os" "path/filepath" - "runtime" "sort" "strings" "time" @@ -25,7 +24,7 @@ import ( "github.com/docker/docker/pkg/streamformatter" "github.com/docker/docker/pkg/system" "github.com/moby/buildkit/frontend/dockerfile/instructions" - specs "github.com/opencontainers/image-spec/specs-go/v1" + ocispec "github.com/opencontainers/image-spec/specs-go/v1" "github.com/pkg/errors" ) @@ -75,7 +74,7 @@ type copier struct { source builder.Source pathCache pathCache download sourceDownloader - platform *specs.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. @@ -84,19 +83,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 = &specs.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/dispatchers.go b/builder/dockerfile/dispatchers.go index 481ece049b..c36c28d0f0 100644 --- a/builder/dockerfile/dispatchers.go +++ b/builder/dockerfile/dispatchers.go @@ -345,9 +345,16 @@ func dispatchRun(d dispatchRequest, c *instructions.RunCommand) error { 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 diff --git a/builder/dockerfile/imageprobe.go b/builder/dockerfile/imageprobe.go index 6960bf8897..c2a8d116b1 100644 --- a/builder/dockerfile/imageprobe.go +++ b/builder/dockerfile/imageprobe.go @@ -3,6 +3,7 @@ package dockerfile // import "github.com/docker/docker/builder/dockerfile" import ( "github.com/docker/docker/api/types/container" "github.com/docker/docker/builder" + ocispec "github.com/opencontainers/image-spec/specs-go/v1" "github.com/sirupsen/logrus" ) @@ -10,7 +11,7 @@ import ( // cache. type ImageProber interface { Reset() - Probe(parentID string, runConfig *container.Config) (string, error) + Probe(parentID string, runConfig *container.Config, platform ocispec.Platform) (string, error) } type imageProber struct { @@ -37,11 +38,11 @@ func (c *imageProber) Reset() { // 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 } @@ -58,6 +59,6 @@ type nopProber struct{} func (c *nopProber) Reset() {} -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 1bc445d383..c8ece04ca0 100644 --- a/builder/dockerfile/internals.go +++ b/builder/dockerfile/internals.go @@ -10,6 +10,7 @@ import ( "io" "strings" + "github.com/containerd/containerd/platforms" "github.com/docker/docker/api/types" "github.com/docker/docker/api/types/backend" "github.com/docker/docker/api/types/container" @@ -364,7 +365,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 } @@ -424,3 +425,17 @@ func hostConfigFromOptions(options *types.ImageBuildOptions) *container.HostConf } return hc } + +func (b *Builder) getPlatform(state *dispatchState) specs.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 0310374a69..5cd91a5374 100644 --- a/builder/dockerfile/mockbackend_test.go +++ b/builder/dockerfile/mockbackend_test.go @@ -14,6 +14,7 @@ import ( "github.com/docker/docker/image" "github.com/docker/docker/layer" "github.com/docker/docker/pkg/containerfs" + ocispec "github.com/opencontainers/image-spec/specs-go/v1" ) // MockBackend implements the builder.Backend interface for unit testing @@ -111,7 +112,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/images/image_builder.go b/daemon/images/image_builder.go index 52cd3d88ee..79894332e3 100644 --- a/daemon/images/image_builder.go +++ b/daemon/images/image_builder.go @@ -250,6 +250,9 @@ func (i *ImageService) CreateImage(config []byte, parent string) (builder.Image, 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 dcde88adc4..54435ac011 100644 --- a/daemon/images/image_commit.go +++ b/daemon/images/image_commit.go @@ -57,6 +57,9 @@ func (i *ImageService) CommitImage(c backend.CommitConfig) (image.ID, error) { 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/cache/cache.go b/image/cache/cache.go index 6d3f4c57b5..ee89a171e2 100644 --- a/image/cache/cache.go +++ b/image/cache/cache.go @@ -6,11 +6,14 @@ import ( "reflect" "strings" + "github.com/containerd/containerd/platforms" 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" + "github.com/sirupsen/logrus" ) // NewLocal returns a local image cache, based on parent chain @@ -26,8 +29,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 @@ -51,8 +54,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 } @@ -215,7 +218,23 @@ 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 + } + + isBuiltLocally := func(id image.ID) bool { + builtLocally, err := imageStore.IsBuiltLocally(id) + if err != nil { + logrus.WithFields(logrus.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 +244,25 @@ func getLocalCachedImage(imageStore image.Store, imgID image.ID, config *contain return nil, fmt.Errorf("unable to find image %q", id) } + if !isBuiltLocally(id) { + continue + } + + imgPlatform := ocispec.Platform{ + Architecture: img.Architecture, + OS: img.OS, + OSVersion: img.OSVersion, + OSFeatures: img.OSFeatures, + Variant: img.Variant, + } + // 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 match == nil || match.Created.Before(img.Created) { @@ -238,11 +276,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 +307,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 +} diff --git a/image/cache/compare.go b/image/cache/compare.go index e31e9c8bdf..d438b65be7 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,84 @@ 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.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 } diff --git a/image/store.go b/image/store.go index 291150c607..3d945405ce 100644 --- a/image/store.go +++ b/image/store.go @@ -2,6 +2,7 @@ package image // import "github.com/docker/docker/image" import ( "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 @@ -291,6 +294,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()