From 7b4b768edf29ee718627a41dfe54fa4ab09dc20e Mon Sep 17 00:00:00 2001 From: Cole Wippern Date: Mon, 25 Nov 2019 14:25:20 -0800 Subject: [PATCH 1/2] Update copy command cache key logic Include the digest of the stage specified in the --from argument for COPY commands which use --from --- pkg/commands/copy.go | 8 +++++ pkg/config/stage.go | 4 ++- pkg/executor/build.go | 71 +++++++++++++++++++++++++++++--------- pkg/executor/build_test.go | 48 ++++++++++++++++++++++++++ 4 files changed, 114 insertions(+), 17 deletions(-) diff --git a/pkg/commands/copy.go b/pkg/commands/copy.go index 4071f957b..dab5917fa 100644 --- a/pkg/commands/copy.go +++ b/pkg/commands/copy.go @@ -161,6 +161,10 @@ func (c *CopyCommand) CacheCommand(img v1.Image) DockerCommand { } } +func (c *CopyCommand) From() string { + return c.cmd.From +} + type CachingCopyCommand struct { BaseCommand img v1.Image @@ -187,6 +191,10 @@ func (cr *CachingCopyCommand) String() string { return cr.cmd.String() } +func (cr *CachingCopyCommand) From() string { + return cr.cmd.From +} + func resolveIfSymlink(destPath string) (string, error) { baseDir := filepath.Dir(destPath) if info, err := os.Lstat(baseDir); err == nil { diff --git a/pkg/config/stage.go b/pkg/config/stage.go index 56c4a3f0f..fad18049c 100644 --- a/pkg/config/stage.go +++ b/pkg/config/stage.go @@ -16,7 +16,9 @@ limitations under the License. package config -import "github.com/moby/buildkit/frontend/dockerfile/instructions" +import ( + "github.com/moby/buildkit/frontend/dockerfile/instructions" +) // KanikoStage wraps a stage of the Dockerfile and provides extra information type KanikoStage struct { diff --git a/pkg/executor/build.go b/pkg/executor/build.go index e3c48b5e1..47c456359 100644 --- a/pkg/executor/build.go +++ b/pkg/executor/build.go @@ -64,9 +64,6 @@ type stageBuilder struct { stage config.KanikoStage image v1.Image cf *v1.ConfigFile - snapshotter snapShotter - layerCache cache.LayerCache - pushCache cachePusher baseImageDigest string finalCacheKey string opts *config.KanikoOptions @@ -74,6 +71,9 @@ type stageBuilder struct { args *dockerfile.BuildArgs crossStageDeps map[int][]string digestToCacheKeyMap map[string]string + snapshotter snapShotter + layerCache cache.LayerCache + pushCache cachePusher } // newStageBuilder returns a new type stageBuilder which contains all the information required to build the stage @@ -146,6 +146,38 @@ func initializeConfig(img partial.WithConfigFile) (*v1.ConfigFile, error) { return imageConfig, nil } +func (s *stageBuilder) populateCompositeKey(command commands.DockerCommand, files []string, compositeKey CompositeCache) (CompositeCache, error) { + // Add the next command to the cache key. + compositeKey.AddKey(command.String()) + switch v := command.(type) { + case *commands.CopyCommand: + if v.From() != "" { + digest, ok := s.digestMap[v.From()] + if ok { + ds := digest.String() + logrus.Debugf("adding digest %v from previous stage to composite key for %v", ds, command.String()) + compositeKey.AddKey(ds) + } + } + case *commands.CachingCopyCommand: + if v.From() != "" { + digest, ok := s.digestMap[v.From()] + if ok { + ds := digest.String() + logrus.Debugf("adding digest %v from previous stage to composite key for %v", ds, command.String()) + compositeKey.AddKey(ds) + } + } + } + + for _, f := range files { + if err := compositeKey.AddPath(f); err != nil { + return compositeKey, err + } + } + return compositeKey, nil +} + func (s *stageBuilder) optimize(compositeKey CompositeCache, cfg v1.Config) error { if !s.opts.Cache { return nil @@ -160,16 +192,14 @@ func (s *stageBuilder) optimize(compositeKey CompositeCache, cfg v1.Config) erro if command == nil { continue } - compositeKey.AddKey(command.String()) - // If the command uses files from the context, add them. files, err := command.FilesUsedFromContext(&cfg, s.args) if err != nil { return errors.Wrap(err, "failed to get files used from context") } - for _, f := range files { - if err := compositeKey.AddPath(f); err != nil { - return errors.Wrap(err, "failed to add path to composite key") - } + + compositeKey, err = s.populateCompositeKey(command, files, compositeKey) + if err != nil { + return err } ck, err := compositeKey.Hash() @@ -256,19 +286,19 @@ func (s *stageBuilder) build() error { continue } - // Add the next command to the cache key. - compositeKey.AddKey(command.String()) t := timing.Start("Command: " + command.String()) + // If the command uses files from the context, add them. files, err := command.FilesUsedFromContext(&s.cf.Config, s.args) if err != nil { return errors.Wrap(err, "failed to get files used from context") } - for _, f := range files { - if err := compositeKey.AddPath(f); err != nil { - return errors.Wrap(err, fmt.Sprintf("failed to add path to composite key %v", f)) - } + + *compositeKey, err = s.populateCompositeKey(command, files, *compositeKey) + if err != nil { + return err } + logrus.Info(command.String()) if err := command.ExecuteCommand(&s.cf.Config, s.args); err != nil { @@ -303,6 +333,7 @@ func (s *stageBuilder) build() error { if err := cacheGroup.Wait(); err != nil { logrus.Warnf("error uploading layer to cache: %s", err) } + return nil } @@ -374,7 +405,6 @@ func (s *stageBuilder) saveSnapshotToImage(createdBy string, tarPath string) err }, ) return err - } func CalculateDependencies(opts *config.KanikoOptions) (map[int][]string, error) { @@ -443,6 +473,7 @@ func CalculateDependencies(opts *config.KanikoOptions) (map[int][]string, error) func DoBuild(opts *config.KanikoOptions) (v1.Image, error) { t := timing.Start("Total Build Time") digestToCacheKeyMap := make(map[string]string) + // Parse dockerfile and unpack base image to root stages, err := dockerfile.Stages(opts) if err != nil { @@ -470,6 +501,14 @@ func DoBuild(opts *config.KanikoOptions) (v1.Image, error) { if err := sb.build(); err != nil { return nil, errors.Wrap(err, "error building stage") } + + d, err := sb.image.Digest() + if err != nil { + return nil, err + } + + digestMap[fmt.Sprintf("%d", sb.stage.Index)] = d + reviewConfig(stage, &sb.cf.Config) sourceImage, err := mutate.Config(sb.image, sb.cf.Config) if err != nil { diff --git a/pkg/executor/build_test.go b/pkg/executor/build_test.go index 66c0ab5b5..0b4a546f8 100644 --- a/pkg/executor/build_test.go +++ b/pkg/executor/build_test.go @@ -919,3 +919,51 @@ func generateTar(t *testing.T, dir string, fileNames ...string) []byte { } return buf.Bytes() } + +//======= +// testCases := []struct { +// opts *config.KanikoOptions +// retrieve bool +// }{ +// { +// opts: &config.KanikoOptions{Cache: true}, +// }, +// { +// opts: &config.KanikoOptions{Cache: true}, +// retrieve: true, +// }, +// { +// opts: &config.KanikoOptions{Cache: false}, +// }, +// { +// opts: &config.KanikoOptions{Cache: false}, +// retrieve: true, +// }, +// } +// for i, tc := range testCases { +// t.Run(fmt.Sprintf("Case %d", i), func(t *testing.T) { +// file, err := ioutil.TempFile("", "foo") +// if err != nil { +// t.Error(err) +// } + +// cf := &v1.ConfigFile{} +// snap := fakeSnapShotter{file: file.Name()} +// lc := fakeLayerCache{retrieve: tc.retrieve} +// sb := &stageBuilder{opts: tc.opts, cf: cf, snapshotter: snap, layerCache: lc, pushCache: fakeCachePush} + +// command := MockDockerCommand{ +// contextFiles: []string{file.Name()}, +// cacheCommand: MockCachedDockerCommand{ +// contextFiles: []string{file.Name()}, +// }, +// } +// sb.cmds = []commands.DockerCommand{command} +// err = sb.build() +// if err != nil { +// t.Errorf("Expected error to be nil but was %v", err) +// } +// }) +// } +//} +//>>>>>>> Add unit tests for stagebuilder build and optimize From b19214ad1ebe8e113fdcbdd8fe413992ae5f0cd3 Mon Sep 17 00:00:00 2001 From: Cole Wippern Date: Wed, 27 Nov 2019 16:35:52 -0800 Subject: [PATCH 2/2] Use cachekey not digest for COPY --from src * use the cachekey of the src stage rather than the digest for COPY --from commands as they are reproducible unlike digests * track digest to cache keys and stage indexes to digest * add extra debug logging for troubleshooting cachekey building issues * convert Sha256 hashes to hex encoded strings rather than plain strings for easier human reading --- pkg/executor/build.go | 109 +++++++++++++++++--------------- pkg/executor/build_test.go | 49 +------------- pkg/executor/composite_cache.go | 5 +- 3 files changed, 63 insertions(+), 100 deletions(-) diff --git a/pkg/executor/build.go b/pkg/executor/build.go index 47c456359..caa1cf063 100644 --- a/pkg/executor/build.go +++ b/pkg/executor/build.go @@ -61,23 +61,24 @@ type snapShotter interface { // stageBuilder contains all fields necessary to build one stage of a Dockerfile type stageBuilder struct { - stage config.KanikoStage - image v1.Image - cf *v1.ConfigFile - baseImageDigest string - finalCacheKey string - opts *config.KanikoOptions - cmds []commands.DockerCommand - args *dockerfile.BuildArgs - crossStageDeps map[int][]string - digestToCacheKeyMap map[string]string - snapshotter snapShotter - layerCache cache.LayerCache - pushCache cachePusher + stage config.KanikoStage + image v1.Image + cf *v1.ConfigFile + baseImageDigest string + finalCacheKey string + opts *config.KanikoOptions + cmds []commands.DockerCommand + args *dockerfile.BuildArgs + crossStageDeps map[int][]string + digestToCacheKey map[string]string + stageIdxToDigest map[string]string + snapshotter snapShotter + layerCache cache.LayerCache + pushCache cachePusher } // newStageBuilder returns a new type stageBuilder which contains all the information required to build the stage -func newStageBuilder(opts *config.KanikoOptions, stage config.KanikoStage, crossStageDeps map[int][]string, dcm map[string]string) (*stageBuilder, error) { +func newStageBuilder(opts *config.KanikoOptions, stage config.KanikoStage, crossStageDeps map[int][]string, dcm map[string]string, sid map[string]string) (*stageBuilder, error) { sourceImage, err := util.RetrieveSourceImage(stage, opts) if err != nil { return nil, err @@ -104,14 +105,15 @@ func newStageBuilder(opts *config.KanikoOptions, stage config.KanikoStage, cross return nil, err } s := &stageBuilder{ - stage: stage, - image: sourceImage, - cf: imageConfig, - snapshotter: snapshotter, - baseImageDigest: digest.String(), - opts: opts, - crossStageDeps: crossStageDeps, - digestToCacheKeyMap: dcm, + stage: stage, + image: sourceImage, + cf: imageConfig, + snapshotter: snapshotter, + baseImageDigest: digest.String(), + opts: opts, + crossStageDeps: crossStageDeps, + digestToCacheKey: dcm, + stageIdxToDigest: sid, layerCache: &cache.RegistryCache{ Opts: opts, }, @@ -146,28 +148,14 @@ func initializeConfig(img partial.WithConfigFile) (*v1.ConfigFile, error) { return imageConfig, nil } -func (s *stageBuilder) populateCompositeKey(command commands.DockerCommand, files []string, compositeKey CompositeCache) (CompositeCache, error) { +func (s *stageBuilder) populateCompositeKey(command fmt.Stringer, files []string, compositeKey CompositeCache) (CompositeCache, error) { // Add the next command to the cache key. compositeKey.AddKey(command.String()) switch v := command.(type) { case *commands.CopyCommand: - if v.From() != "" { - digest, ok := s.digestMap[v.From()] - if ok { - ds := digest.String() - logrus.Debugf("adding digest %v from previous stage to composite key for %v", ds, command.String()) - compositeKey.AddKey(ds) - } - } + compositeKey = s.populateCopyCmdCompositeKey(command, v.From(), compositeKey) case *commands.CachingCopyCommand: - if v.From() != "" { - digest, ok := s.digestMap[v.From()] - if ok { - ds := digest.String() - logrus.Debugf("adding digest %v from previous stage to composite key for %v", ds, command.String()) - compositeKey.AddKey(ds) - } - } + compositeKey = s.populateCopyCmdCompositeKey(command, v.From(), compositeKey) } for _, f := range files { @@ -178,6 +166,22 @@ func (s *stageBuilder) populateCompositeKey(command commands.DockerCommand, file return compositeKey, nil } +func (s *stageBuilder) populateCopyCmdCompositeKey(command fmt.Stringer, from string, compositeKey CompositeCache) CompositeCache { + if from != "" { + digest, ok := s.stageIdxToDigest[from] + if ok { + ds := digest + cacheKey, ok := s.digestToCacheKey[ds] + if ok { + logrus.Debugf("adding digest %v from previous stage to composite key for %v", ds, command.String()) + compositeKey.AddKey(cacheKey) + } + } + } + + return compositeKey +} + func (s *stageBuilder) optimize(compositeKey CompositeCache, cfg v1.Config) error { if !s.opts.Cache { return nil @@ -202,10 +206,12 @@ func (s *stageBuilder) optimize(compositeKey CompositeCache, cfg v1.Config) erro return err } + logrus.Debugf("optimize: composite key for command %v %v", command.String(), compositeKey) ck, err := compositeKey.Hash() if err != nil { return errors.Wrap(err, "failed to hash composite key") } + logrus.Debugf("optimize: cache key for command %v %v", command.String(), ck) s.finalCacheKey = ck if command.ShouldCacheOutput() && !stopCache { img, err := s.layerCache.RetrieveLayer(ck) @@ -236,7 +242,7 @@ func (s *stageBuilder) optimize(compositeKey CompositeCache, cfg v1.Config) erro func (s *stageBuilder) build() error { // Set the initial cache key to be the base image digest, the build args and the SrcContext. var compositeKey *CompositeCache - if cacheKey, ok := s.digestToCacheKeyMap[s.baseImageDigest]; ok { + if cacheKey, ok := s.digestToCacheKey[s.baseImageDigest]; ok { compositeKey = NewCompositeCache(cacheKey) } else { compositeKey = NewCompositeCache(s.baseImageDigest) @@ -316,6 +322,7 @@ func (s *stageBuilder) build() error { return errors.Wrap(err, "failed to take snapshot") } + logrus.Debugf("build: composite key for command %v %v", command.String(), compositeKey) ck, err := compositeKey.Hash() if err != nil { return errors.Wrap(err, "failed to hash composite key") @@ -472,7 +479,8 @@ func CalculateDependencies(opts *config.KanikoOptions) (map[int][]string, error) // DoBuild executes building the Dockerfile func DoBuild(opts *config.KanikoOptions) (v1.Image, error) { t := timing.Start("Total Build Time") - digestToCacheKeyMap := make(map[string]string) + digestToCacheKey := make(map[string]string) + stageIdxToDigest := make(map[string]string) // Parse dockerfile and unpack base image to root stages, err := dockerfile.Stages(opts) @@ -494,7 +502,7 @@ func DoBuild(opts *config.KanikoOptions) (v1.Image, error) { logrus.Infof("Built cross stage deps: %v", crossStageDependencies) for index, stage := range stages { - sb, err := newStageBuilder(opts, stage, crossStageDependencies, digestToCacheKeyMap) + sb, err := newStageBuilder(opts, stage, crossStageDependencies, digestToCacheKey, stageIdxToDigest) if err != nil { return nil, err } @@ -502,23 +510,24 @@ func DoBuild(opts *config.KanikoOptions) (v1.Image, error) { return nil, errors.Wrap(err, "error building stage") } - d, err := sb.image.Digest() - if err != nil { - return nil, err - } - - digestMap[fmt.Sprintf("%d", sb.stage.Index)] = d - reviewConfig(stage, &sb.cf.Config) + sourceImage, err := mutate.Config(sb.image, sb.cf.Config) if err != nil { return nil, err } + d, err := sourceImage.Digest() if err != nil { return nil, err } - digestToCacheKeyMap[d.String()] = sb.finalCacheKey + + stageIdxToDigest[fmt.Sprintf("%d", sb.stage.Index)] = d.String() + logrus.Debugf("mapping stage idx %v to digest %v", sb.stage.Index, d.String()) + + digestToCacheKey[d.String()] = sb.finalCacheKey + logrus.Debugf("mapping digest %v to cachekey %v", d.String(), sb.finalCacheKey) + if stage.Final { sourceImage, err = mutate.CreatedAt(sourceImage, v1.Time{Time: time.Now()}) if err != nil { diff --git a/pkg/executor/build_test.go b/pkg/executor/build_test.go index 0b4a546f8..d5c07acfc 100644 --- a/pkg/executor/build_test.go +++ b/pkg/executor/build_test.go @@ -888,6 +888,7 @@ func tempDirAndFile(t *testing.T) (string, []string) { return dir, filenames } + func generateTar(t *testing.T, dir string, fileNames ...string) []byte { buf := bytes.NewBuffer([]byte{}) writer := tar.NewWriter(buf) @@ -919,51 +920,3 @@ func generateTar(t *testing.T, dir string, fileNames ...string) []byte { } return buf.Bytes() } - -//======= -// testCases := []struct { -// opts *config.KanikoOptions -// retrieve bool -// }{ -// { -// opts: &config.KanikoOptions{Cache: true}, -// }, -// { -// opts: &config.KanikoOptions{Cache: true}, -// retrieve: true, -// }, -// { -// opts: &config.KanikoOptions{Cache: false}, -// }, -// { -// opts: &config.KanikoOptions{Cache: false}, -// retrieve: true, -// }, -// } -// for i, tc := range testCases { -// t.Run(fmt.Sprintf("Case %d", i), func(t *testing.T) { -// file, err := ioutil.TempFile("", "foo") -// if err != nil { -// t.Error(err) -// } - -// cf := &v1.ConfigFile{} -// snap := fakeSnapShotter{file: file.Name()} -// lc := fakeLayerCache{retrieve: tc.retrieve} -// sb := &stageBuilder{opts: tc.opts, cf: cf, snapshotter: snap, layerCache: lc, pushCache: fakeCachePush} - -// command := MockDockerCommand{ -// contextFiles: []string{file.Name()}, -// cacheCommand: MockCachedDockerCommand{ -// contextFiles: []string{file.Name()}, -// }, -// } -// sb.cmds = []commands.DockerCommand{command} -// err = sb.build() -// if err != nil { -// t.Errorf("Expected error to be nil but was %v", err) -// } -// }) -// } -//} -//>>>>>>> Add unit tests for stagebuilder build and optimize diff --git a/pkg/executor/composite_cache.go b/pkg/executor/composite_cache.go index 15a88a40b..ae8ff0609 100644 --- a/pkg/executor/composite_cache.go +++ b/pkg/executor/composite_cache.go @@ -18,6 +18,7 @@ package executor import ( "crypto/sha256" + "fmt" "os" "path/filepath" "strings" @@ -75,7 +76,7 @@ func (s *CompositeCache) AddPath(p string) error { return err } - s.keys = append(s.keys, string(sha.Sum(nil))) + s.keys = append(s.keys, fmt.Sprintf("%x", sha.Sum(nil))) return nil } @@ -98,5 +99,5 @@ func HashDir(p string) (string, error) { return "", err } - return string(sha.Sum(nil)), nil + return fmt.Sprintf("%x", sha.Sum(nil)), nil }