Skip to content

Commit

Permalink
Fix multistage caching with COPY --from
Browse files Browse the repository at this point in the history
* Removed block on use --cache-copy-layers with multistage builds
* Removed using digest in composite key with command COPY --from
* COPY --from command uses src as file context (only changed files will be reason for change hash)
* ARG and ENV changed before COPY dont change composite key
* Add and fix some tests
* Caching work same as caching in docker buildx
  • Loading branch information
kt315 committed Jun 15, 2023
1 parent 0790e8b commit c4aa728
Show file tree
Hide file tree
Showing 10 changed files with 149 additions and 81 deletions.
4 changes: 4 additions & 0 deletions pkg/commands/base_command.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,10 @@ import (
type BaseCommand struct {
}

func (b *BaseCommand) IsArgsEnvsRequiredInCache() bool {
return false
}

func (b *BaseCommand) CacheCommand(v1.Image) DockerCommand {
return nil
}
Expand Down
4 changes: 4 additions & 0 deletions pkg/commands/commands.go
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,10 @@ type DockerCommand interface {

// ShouldDetectDeletedFiles returns true if the command could delete files.
ShouldDetectDeletedFiles() bool

// True if need add ARGs and EVNs to composite cache string with resolved command
// need only for RUN instruction
IsArgsEnvsRequiredInCache() bool
}

func GetCommand(cmd instructions.Command, fileContext util.FileContext, useNewRun bool, cacheCopy bool, cacheRun bool) (DockerCommand, error) {
Expand Down
3 changes: 1 addition & 2 deletions pkg/commands/copy.go
Original file line number Diff line number Diff line change
Expand Up @@ -270,9 +270,8 @@ func copyCmdFilesUsedFromContext(
config *v1.Config, buildArgs *dockerfile.BuildArgs, cmd *instructions.CopyCommand,
fileContext util.FileContext,
) ([]string, error) {
// We don't use the context if we're performing a copy --from.
if cmd.From != "" {
return nil, nil
fileContext = util.FileContext{Root: filepath.Join(kConfig.KanikoDir, cmd.From)}
}

replacementEnvs := buildArgs.ReplacementEnvs(config.Env)
Expand Down
8 changes: 8 additions & 0 deletions pkg/commands/run.go
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,10 @@ var (
userLookup = util.LookupUser
)

func (r *RunCommand) IsArgsEnvsRequiredInCache() bool {
return true
}

func (r *RunCommand) ExecuteCommand(config *v1.Config, buildArgs *dockerfile.BuildArgs) error {
return runCommandInExec(config, buildArgs, r.cmd)
}
Expand Down Expand Up @@ -200,6 +204,10 @@ type CachingRunCommand struct {
extractFn util.ExtractFunction
}

func (cr *CachingRunCommand) IsArgsEnvsRequiredInCache() bool {
return true
}

func (cr *CachingRunCommand) ExecuteCommand(config *v1.Config, buildArgs *dockerfile.BuildArgs) error {
logrus.Infof("Found cached layer, extracting to filesystem")
var err error
Expand Down
4 changes: 4 additions & 0 deletions pkg/commands/run_marker.go
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,10 @@ func (r *RunMarkerCommand) ProvidesFilesToSnapshot() bool {
return true
}

func (r *RunMarkerCommand) IsArgsEnvsRequiredInCache() bool {
return true
}

// CacheCommand returns true since this command should be cached
func (r *RunMarkerCommand) CacheCommand(img v1.Image) DockerCommand {

Expand Down
4 changes: 0 additions & 4 deletions pkg/dockerfile/dockerfile.go
Original file line number Diff line number Diff line change
Expand Up @@ -58,10 +58,6 @@ func ParseStages(opts *config.KanikoOptions) ([]instructions.Stage, []instructio
return nil, nil, errors.Wrap(err, "parsing dockerfile")
}

if opts.CacheCopyLayers && len(stages) >= 2 {
return nil, nil, errors.New("kaniko does not support caching copy layers in multistage builds")
}

metaArgs, err = expandNested(metaArgs, opts.BuildArgs)
if err != nil {
return nil, nil, errors.Wrap(err, "expanding meta ARGs")
Expand Down
33 changes: 0 additions & 33 deletions pkg/dockerfile/dockerfile_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,39 +29,6 @@ import (
"github.com/moby/buildkit/frontend/dockerfile/instructions"
)

func Test_ParseStages_NoMultistageWithCacheCopy(t *testing.T) {
dockerfile := `
FROM scratch as first
COPY testfile /
FROM scratch as second
COPY --from=second testfile /
`
tmpfile, err := ioutil.TempFile("", "Dockerfile.test")
if err != nil {
t.Fatal(err)
}

defer os.Remove(tmpfile.Name())

if _, err := tmpfile.Write([]byte(dockerfile)); err != nil {
t.Fatal(err)
}
if err := tmpfile.Close(); err != nil {
t.Fatal(err)
}

opts := &config.KanikoOptions{
DockerfilePath: tmpfile.Name(),
CacheCopyLayers: true,
}

_, _, err = ParseStages(opts)
if err == nil {
t.Fatal("expected ParseStages to fail on MultiStage build if CacheCopyLayers=true")
}
}

func Test_ParseStages_ArgValueWithQuotes(t *testing.T) {
dockerfile := `
ARG IMAGE="ubuntu:16.04"
Expand Down
31 changes: 8 additions & 23 deletions pkg/executor/build.go
Original file line number Diff line number Diff line change
Expand Up @@ -198,7 +198,7 @@ func isOCILayout(path string) bool {
return strings.HasPrefix(path, "oci:")
}

func (s *stageBuilder) populateCompositeKey(command fmt.Stringer, files []string, compositeKey CompositeCache, args *dockerfile.BuildArgs, env []string) (CompositeCache, error) {
func (s *stageBuilder) populateCompositeKey(command commands.DockerCommand, files []string, compositeKey CompositeCache, args *dockerfile.BuildArgs, env []string) (CompositeCache, error) {
// First replace all the environment variables or args in the command
replacementEnvs := args.ReplacementEnvs(env)
// The sort order of `replacementEnvs` is basically undefined, sort it
Expand All @@ -212,15 +212,16 @@ func (s *stageBuilder) populateCompositeKey(command fmt.Stringer, files []string
// avoid conflicts with any RUN command since commands can not
// start with | (vertical bar). The "#" (number of build envs) is there to
// help ensure proper cache matches.
if len(replacementEnvs) > 0 {
compositeKey.AddKey(fmt.Sprintf("|%d", len(replacementEnvs)))
compositeKey.AddKey(replacementEnvs...)

if command.IsArgsEnvsRequiredInCache() {
if len(replacementEnvs) > 0 {
compositeKey.AddKey(fmt.Sprintf("|%d", len(replacementEnvs)))
compositeKey.AddKey(replacementEnvs...)
}
}

// Add the next command to the cache key.
compositeKey.AddKey(resolvedCmd)
if copyCmd, ok := commands.CastAbstractCopyCommand(command); ok == true {
compositeKey = s.populateCopyCmdCompositeKey(command, copyCmd.From(), compositeKey)
}

for _, f := range files {
if err := compositeKey.AddPath(f, s.fileContext); err != nil {
Expand All @@ -230,22 +231,6 @@ func (s *stageBuilder) populateCompositeKey(command fmt.Stringer, files []string
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
Expand Down
Loading

0 comments on commit c4aa728

Please sign in to comment.