Skip to content

Commit

Permalink
Merge pull request #1085 from tejal29/rm_buildargs_from_cache_key
Browse files Browse the repository at this point in the history 
remove build args from composite key and replace all build args
  • Loading branch information
@tejal29
tejal29 authored Mar 17, 2020
2 parents 0c42223 + 579ec52 commit 0302e51
Show file tree
Hide file tree
Showing 3 changed files with 272 additions and 13 deletions.
22 changes: 13 additions & 9 deletions pkg/executor/build.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ type stageBuilder struct {
stageIdxToDigest map[string]string
snapshotter snapShotter
layerCache cache.LayerCache
pushCache cachePusher
pushLayerToCache cachePusher
}

// newStageBuilder returns a new type stageBuilder which contains all the information required to build the stage
Expand Down Expand Up @@ -116,7 +116,7 @@ func newStageBuilder(opts *config.KanikoOptions, stage config.KanikoStage, cross
layerCache: &cache.RegistryCache{
Opts: opts,
},
pushCache: pushLayerToCache,
pushLayerToCache: pushLayerToCache,
}

for _, cmd := range s.stage.Commands {
Expand Down Expand Up @@ -166,9 +166,15 @@ func initializeConfig(img partial.WithConfigFile, opts *config.KanikoOptions) (*
return imageConfig, nil
}

func (s *stageBuilder) populateCompositeKey(command fmt.Stringer, files []string, compositeKey CompositeCache) (CompositeCache, error) {
func (s *stageBuilder) populateCompositeKey(command fmt.Stringer, 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)
resolvedCmd, err := util.ResolveEnvironmentReplacement(command.String(), replacementEnvs, false)
if err != nil {
return compositeKey, err
}
// Add the next command to the cache key.
compositeKey.AddKey(command.String())
compositeKey.AddKey(resolvedCmd)
switch v := command.(type) {
case *commands.CopyCommand:
compositeKey = s.populateCopyCmdCompositeKey(command, v.From(), compositeKey)
Expand Down Expand Up @@ -221,7 +227,7 @@ func (s *stageBuilder) optimize(compositeKey CompositeCache, cfg v1.Config) erro
return errors.Wrap(err, "failed to get files used from context")
}

compositeKey, err = s.populateCompositeKey(command, files, compositeKey)
compositeKey, err = s.populateCompositeKey(command, files, compositeKey, s.args, cfg.Env)
if err != nil {
return err
}
Expand Down Expand Up @@ -271,8 +277,6 @@ func (s *stageBuilder) build() error {
compositeKey = NewCompositeCache(s.baseImageDigest)
}

compositeKey.AddKey(s.opts.BuildArgs...)

// Apply optimizations to the instructions.
if err := s.optimize(*compositeKey, s.cf.Config); err != nil {
return errors.Wrap(err, "failed to optimize instructions")
Expand Down Expand Up @@ -329,7 +333,7 @@ func (s *stageBuilder) build() error {
return errors.Wrap(err, "failed to get files used from context")
}

*compositeKey, err = s.populateCompositeKey(command, files, *compositeKey)
*compositeKey, err = s.populateCompositeKey(command, files, *compositeKey, s.args, s.cf.Config.Env)
if err != nil {
return err
}
Expand Down Expand Up @@ -378,7 +382,7 @@ func (s *stageBuilder) build() error {
// Push layer to cache (in parallel) now along with new config file
if s.opts.Cache && command.ShouldCacheOutput() {
cacheGroup.Go(func() error {
return s.pushCache(s.opts, ck, tarPath, command.String())
return s.pushLayerToCache(s.opts, ck, tarPath, command.String())
})
}
if err := s.saveSnapshotToImage(command.String(), tarPath); err != nil {
Expand Down
260 changes: 257 additions & 3 deletions pkg/executor/build_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -497,7 +497,8 @@ func Test_stageBuilder_optimize(t *testing.T) {
cf := &v1.ConfigFile{}
snap := fakeSnapShotter{}
lc := &fakeLayerCache{retrieve: tc.retrieve}
sb := &stageBuilder{opts: tc.opts, cf: cf, snapshotter: snap, layerCache: lc}
sb := &stageBuilder{opts: tc.opts, cf: cf, snapshotter: snap, layerCache: lc,
args: dockerfile.NewBuildArgs([]string{})}
ck := CompositeCache{}
file, err := ioutil.TempFile("", "foo")
if err != nil {
Expand All @@ -517,10 +518,135 @@ func Test_stageBuilder_optimize(t *testing.T) {
}
}

type stageContext struct {
command fmt.Stringer
args *dockerfile.BuildArgs
env []string
}

func newStageContext(command string, args map[string]string, env []string) stageContext {
dockerArgs := dockerfile.NewBuildArgs([]string{})
for k, v := range args {
dockerArgs.AddArg(k, &v)
}
return stageContext{MockDockerCommand{command: command}, dockerArgs, env}
}

func Test_stageBuilder_populateCompositeKey(t *testing.T) {
testCases := []struct {
description string
cmd1 stageContext
cmd2 stageContext
shdEqual bool
}{
{
description: "cache key for same command, different buildargs, args not used in command",
cmd1: newStageContext(
"RUN echo const > test",
map[string]string{"ARG": "foo"},
[]string{"ENV=foo1"},
),
cmd2: newStageContext(
"RUN echo const > test",
map[string]string{"ARG": "bar"},
[]string{"ENV=bar1"},
),
shdEqual: true,
},
{
description: "cache key for same command with same build args",
cmd1: newStageContext(
"RUN echo $ARG > test",
map[string]string{"ARG": "foo"},
[]string{},
),
cmd2: newStageContext(
"RUN echo $ARG > test",
map[string]string{"ARG": "foo"},
[]string{},
),
shdEqual: true,
},
{
description: "cache key for same command with same env",
cmd1: newStageContext(
"RUN echo $ENV > test",
map[string]string{"ARG": "foo"},
[]string{"ENV=same"},
),
cmd2: newStageContext(
"RUN echo $ENV > test",
map[string]string{"ARG": "bar"},
[]string{"ENV=same"},
),
shdEqual: true,
},
{
description: "cache key for same command with a build arg values",
cmd1: newStageContext(
"RUN echo $ARG > test",
map[string]string{"ARG": "foo"},
[]string{},
),
cmd2: newStageContext(
"RUN echo $ARG > test",
map[string]string{"ARG": "bar"},
[]string{},
),
},
{
description: "cache key for same command with different env values",
cmd1: newStageContext(
"RUN echo $ENV > test",
map[string]string{"ARG": "foo"},
[]string{"ENV=1"},
),
cmd2: newStageContext(
"RUN echo $ENV > test",
map[string]string{"ARG": "foo"},
[]string{"ENV=2"},
),
},
{
description: "cache key for different command same context",
cmd1: newStageContext(
"RUN echo other > test",
map[string]string{"ARG": "foo"},
[]string{"ENV=1"},
),
cmd2: newStageContext(
"RUN echo another > test",
map[string]string{"ARG": "foo"},
[]string{"ENV=1"},
),
},
}
for _, tc := range testCases {
t.Run(tc.description, func(t *testing.T) {
sb := &stageBuilder{opts: &config.KanikoOptions{SrcContext: "workspace"}}
ck := CompositeCache{}

ck1, err := sb.populateCompositeKey(tc.cmd1.command, []string{}, ck, tc.cmd1.args, tc.cmd1.env)
if err != nil {
t.Errorf("Expected error to be nil but was %v", err)
}
ck2, err := sb.populateCompositeKey(tc.cmd2.command, []string{}, ck, tc.cmd2.args, tc.cmd2.env)
if err != nil {
t.Errorf("Expected error to be nil but was %v", err)
}
key1, key2 := hashCompositeKeys(t, ck1, ck2)
if b := key1 == key2; b != tc.shdEqual {
t.Errorf("expected keys to be equal as %t but found %t", tc.shdEqual, !tc.shdEqual)
}
})
}
}

func Test_stageBuilder_build(t *testing.T) {
type testcase struct {
description string
opts *config.KanikoOptions
args map[string]string
layerCache *fakeLayerCache
expectedCacheKeys []string
pushedCacheKeys []string
Expand All @@ -544,6 +670,7 @@ func Test_stageBuilder_build(t *testing.T) {
t.Errorf("couldn't create hash %v", err)
}
command := MockDockerCommand{
command: "meow",
contextFiles: []string{filePath},
cacheCommand: MockCachedDockerCommand{
contextFiles: []string{filePath},
Expand Down Expand Up @@ -576,6 +703,7 @@ func Test_stageBuilder_build(t *testing.T) {
t.Errorf("couldn't create hash %v", err)
}
command := MockDockerCommand{
command: "meow",
contextFiles: []string{filePath},
cacheCommand: MockCachedDockerCommand{
contextFiles: []string{filePath},
Expand Down Expand Up @@ -838,6 +966,117 @@ COPY %s bar.txt
commands: getCommands(dir, cmds),
}
}(),
func() testcase {
dir, _ := tempDirAndFile(t)
ch := NewCompositeCache("")
ch.AddKey("RUN foobar")
hash, err := ch.Hash()
if err != nil {
t.Errorf("couldn't create hash %v", err)
}

command := MockDockerCommand{
command: "RUN foobar",
contextFiles: []string{},
cacheCommand: MockCachedDockerCommand{
contextFiles: []string{},
},
}

return testcase{
description: "cached run command with no build arg value used uses cached layer and does not push anything",
config: &v1.ConfigFile{Config: v1.Config{WorkingDir: dir}},
opts: &config.KanikoOptions{Cache: true},
args: map[string]string{
"test": "value",
},
expectedCacheKeys: []string{hash},
commands: []commands.DockerCommand{command},
// layer key needs to be read.
layerCache: &fakeLayerCache{
img: &fakeImage{ImageLayers: []v1.Layer{fakeLayer{}}},
keySequence: []string{hash},
},
rootDir: dir,
}
}(),
func() testcase {
dir, _ := tempDirAndFile(t)

ch := NewCompositeCache("")
ch.AddKey("RUN value")
hash, err := ch.Hash()
if err != nil {
t.Errorf("couldn't create hash %v", err)
}

command := MockDockerCommand{
command: "RUN $arg",
contextFiles: []string{},
cacheCommand: MockCachedDockerCommand{
contextFiles: []string{},
},
}

return testcase{
description: "cached run command with same build arg does not push layer",
config: &v1.ConfigFile{Config: v1.Config{WorkingDir: dir}},
opts: &config.KanikoOptions{Cache: true},
args: map[string]string{
"arg": "value",
},
// layer key that exists
layerCache: &fakeLayerCache{
img: &fakeImage{ImageLayers: []v1.Layer{fakeLayer{}}},
keySequence: []string{hash},
},
expectedCacheKeys: []string{hash},
commands: []commands.DockerCommand{command},
rootDir: dir,
}
}(),
func() testcase {
dir, _ := tempDirAndFile(t)

ch1 := NewCompositeCache("")
ch1.AddKey("RUN value")
hash1, err := ch1.Hash()
if err != nil {
t.Errorf("couldn't create hash %v", err)
}

ch2 := NewCompositeCache("")
ch2.AddKey("RUN anotherValue")
hash2, err := ch2.Hash()
if err != nil {
t.Errorf("couldn't create hash %v", err)
}
command := MockDockerCommand{
command: "RUN $arg",
contextFiles: []string{},
cacheCommand: MockCachedDockerCommand{
contextFiles: []string{},
},
}

return testcase{
description: "cached run command with another build arg pushes layer",
config: &v1.ConfigFile{Config: v1.Config{WorkingDir: dir}},
opts: &config.KanikoOptions{Cache: true},
args: map[string]string{
"arg": "anotherValue",
},
// layer for arg=value already exists
layerCache: &fakeLayerCache{
img: &fakeImage{ImageLayers: []v1.Layer{fakeLayer{}}},
keySequence: []string{hash1},
},
expectedCacheKeys: []string{hash2},
pushedCacheKeys: []string{hash2},
commands: []commands.DockerCommand{command},
rootDir: dir,
}
}(),
}
for _, tc := range testCases {
t.Run(tc.description, func(t *testing.T) {
Expand Down Expand Up @@ -875,18 +1114,21 @@ COPY %s bar.txt
}
keys := []string{}
sb := &stageBuilder{
args: &dockerfile.BuildArgs{}, //required or code will panic
args: dockerfile.NewBuildArgs([]string{}), //required or code will panic
image: tc.image,
opts: tc.opts,
cf: cf,
snapshotter: snap,
layerCache: lc,
pushCache: func(_ *config.KanikoOptions, cacheKey, _, _ string) error {
pushLayerToCache: func(_ *config.KanikoOptions, cacheKey, _, _ string) error {
keys = append(keys, cacheKey)
return nil
},
}
sb.cmds = tc.commands
for key, value := range tc.args {
sb.args.AddArg(key, &value)
}
tmp := commands.RootDir
if tc.rootDir != "" {
commands.RootDir = tc.rootDir
Expand Down Expand Up @@ -993,3 +1235,15 @@ func generateTar(t *testing.T, dir string, fileNames ...string) []byte {
}
return buf.Bytes()
}

func hashCompositeKeys(t *testing.T, ck1 CompositeCache, ck2 CompositeCache) (string, string) {
key1, err := ck1.Hash()
if err != nil {
t.Errorf("could not hash composite key due to %s", err)
}
key2, err := ck2.Hash()
if err != nil {
t.Errorf("could not hash composite key due to %s", err)
}
return key1, key2
}
Loading

0 comments on commit 0302e51

Please sign in to comment.