diff --git a/pkg/commands/base_command.go b/pkg/commands/base_command.go index ddbfd650bd..94c4fe1561 100644 --- a/pkg/commands/base_command.go +++ b/pkg/commands/base_command.go @@ -32,6 +32,10 @@ func (b *BaseCommand) FilesToSnapshot() []string { return []string{} } +func (b *BaseCommand) ProvidesFilesToSnapshot() bool { + return true +} + func (b *BaseCommand) FilesUsedFromContext(_ *v1.Config, _ *dockerfile.BuildArgs) ([]string, error) { return []string{}, nil } diff --git a/pkg/commands/cache.go b/pkg/commands/cache.go index 1d37e62511..878440df47 100644 --- a/pkg/commands/cache.go +++ b/pkg/commands/cache.go @@ -20,18 +20,12 @@ import v1 "github.com/google/go-containerregistry/pkg/v1" type Cached interface { Layer() v1.Layer - ReadSuccess() bool } type caching struct { - layer v1.Layer - readSuccess bool + layer v1.Layer } func (c caching) Layer() v1.Layer { return c.layer } - -func (c caching) ReadSuccess() bool { - return c.readSuccess -} diff --git a/pkg/commands/cache_test.go b/pkg/commands/cache_test.go index b6201af836..1bc2bec59b 100644 --- a/pkg/commands/cache_test.go +++ b/pkg/commands/cache_test.go @@ -21,7 +21,7 @@ import ( ) func Test_caching(t *testing.T) { - c := caching{layer: fakeLayer{}, readSuccess: true} + c := caching{layer: fakeLayer{}} actual := c.Layer().(fakeLayer) expected := fakeLayer{} @@ -29,8 +29,4 @@ func Test_caching(t *testing.T) { if actualLen != expectedLen { t.Errorf("expected layer tar content to be %v but was %v", expectedLen, actualLen) } - - if !c.ReadSuccess() { - t.Errorf("expected ReadSuccess to be %v but was %v", true, c.ReadSuccess()) - } } diff --git a/pkg/commands/commands.go b/pkg/commands/commands.go index a2ea7788d1..0a4e571345 100644 --- a/pkg/commands/commands.go +++ b/pkg/commands/commands.go @@ -37,6 +37,10 @@ type DockerCommand interface { // A list of files to snapshot, empty for metadata commands or nil if we don't know FilesToSnapshot() []string + // ProvidesFileToSnapshot is true for all metadata commands and commands which know + // list of files changed. False for Run command. + ProvidesFilesToSnapshot() bool + // Return a cache-aware implementation of this command, if it exists. CacheCommand(v1.Image) DockerCommand diff --git a/pkg/commands/copy.go b/pkg/commands/copy.go index 5d58dd6d20..72696649f8 100644 --- a/pkg/commands/copy.go +++ b/pkg/commands/copy.go @@ -189,8 +189,6 @@ func (cr *CachingCopyCommand) ExecuteCommand(config *v1.Config, buildArgs *docke } cr.layer = layers[0] - cr.readSuccess = true - cr.extractedFiles, err = util.GetFSFromLayers(kConfig.RootDir, layers, util.ExtractFunc(cr.extractFn), util.IncludeWhiteout()) logrus.Debugf("extractedFiles: %s", cr.extractedFiles) @@ -213,6 +211,10 @@ func (cr *CachingCopyCommand) FilesToSnapshot() []string { return f } +func (cr *CachingCopyCommand) MetadataOnly() bool { + return false +} + func (cr *CachingCopyCommand) String() string { if cr.cmd == nil { return "nil command" diff --git a/pkg/commands/copy_test.go b/pkg/commands/copy_test.go index dbcacfd114..09d60c5c54 100755 --- a/pkg/commands/copy_test.go +++ b/pkg/commands/copy_test.go @@ -385,10 +385,6 @@ func Test_CachingCopyCommand_ExecuteCommand(t *testing.T) { } else if c.layer != nil && !tc.expectLayer { t.Error("expected the command to have no layer set but instead found a layer") } - - if c.readSuccess != tc.expectLayer { - t.Errorf("expected read success to be %v but was %v", tc.expectLayer, c.readSuccess) - } }) } } diff --git a/pkg/commands/run.go b/pkg/commands/run.go index b5096c5d8b..0a40eb5986 100644 --- a/pkg/commands/run.go +++ b/pkg/commands/run.go @@ -151,6 +151,10 @@ func (r *RunCommand) FilesToSnapshot() []string { return nil } +func (r *RunCommand) ProvidesFilesToSnapshot() bool { + return false +} + // CacheCommand returns true since this command should be cached func (r *RunCommand) CacheCommand(img v1.Image) DockerCommand { @@ -200,7 +204,6 @@ func (cr *CachingRunCommand) ExecuteCommand(config *v1.Config, buildArgs *docker } cr.layer = layers[0] - cr.readSuccess = true cr.extractedFiles, err = util.GetFSFromLayers( kConfig.RootDir, @@ -229,3 +232,7 @@ func (cr *CachingRunCommand) String() string { } return cr.cmd.String() } + +func (cr *CachingRunCommand) MetadataOnly() bool { + return false +} diff --git a/pkg/commands/run_test.go b/pkg/commands/run_test.go index f6accc3bbd..5a7139a6bb 100644 --- a/pkg/commands/run_test.go +++ b/pkg/commands/run_test.go @@ -313,10 +313,6 @@ func Test_CachingRunCommand_ExecuteCommand(t *testing.T) { } else if c.layer != nil && !tc.expectLayer { t.Error("expected the command to have no layer set but instead found a layer") } - - if c.readSuccess != tc.expectLayer { - t.Errorf("expected read success to be %v but was %v", tc.expectLayer, c.readSuccess) - } }) } } diff --git a/pkg/executor/build.go b/pkg/executor/build.go index cce78b3a86..374f626a5b 100644 --- a/pkg/executor/build.go +++ b/pkg/executor/build.go @@ -317,14 +317,14 @@ func (s *stageBuilder) build() error { return errors.Wrap(err, "failed to check filesystem whitelist") } - // Take initial snapshot - t := timing.Start("Initial FS snapshot") - if err := s.snapshotter.Init(); err != nil { - return err + initSnapshotTaken := false + if s.opts.SingleSnapshot { + if err := s.initSnapshotWithTimings(); err != nil { + return err + } + initSnapshotTaken = true } - timing.DefaultRun.Stop(t) - cacheGroup := errgroup.Group{} for index, command := range s.cmds { if command == nil { @@ -348,26 +348,33 @@ func (s *stageBuilder) build() error { logrus.Info(command.String()) + isCacheCommand := func() bool { + switch command.(type) { + case commands.Cached: + return true + default: + return false + } + }() + if !initSnapshotTaken && !isCacheCommand && !command.ProvidesFilesToSnapshot() { + // Take initial snapshot if command does not expect to return + // a list of files. + if err := s.initSnapshotWithTimings(); err != nil { + return err + } + initSnapshotTaken = true + } + if err := command.ExecuteCommand(&s.cf.Config, s.args); err != nil { return errors.Wrap(err, "failed to execute command") } files = command.FilesToSnapshot() timing.DefaultRun.Stop(t) - if !s.shouldTakeSnapshot(index, files) { + if !s.shouldTakeSnapshot(index, files, command.ProvidesFilesToSnapshot()) { continue } - - fn := func() bool { - switch v := command.(type) { - case commands.Cached: - return v.ReadSuccess() - default: - return false - } - } - - if fn() { + if isCacheCommand { v := command.(commands.Cached) layer := v.Layer() if err := s.saveLayerToImage(layer, command.String()); err != nil { @@ -411,6 +418,7 @@ func (s *stageBuilder) build() error { func (s *stageBuilder) takeSnapshot(files []string) (string, error) { var snapshot string var err error + t := timing.Start("Snapshotting FS") if files == nil || s.opts.SingleSnapshot { snapshot, err = s.snapshotter.TakeSnapshotFS() @@ -423,7 +431,7 @@ func (s *stageBuilder) takeSnapshot(files []string) (string, error) { return snapshot, err } -func (s *stageBuilder) shouldTakeSnapshot(index int, files []string) bool { +func (s *stageBuilder) shouldTakeSnapshot(index int, files []string, provideFiles bool) bool { isLastCommand := index == len(s.cmds)-1 // We only snapshot the very end with single snapshot mode on. @@ -436,8 +444,8 @@ func (s *stageBuilder) shouldTakeSnapshot(index int, files []string) bool { return true } - // nil means snapshot everything. - if files == nil { + // if command does not provide files, snapshot everything. + if !provideFiles { return true } @@ -848,3 +856,12 @@ func ResolveCrossStageInstructions(stages []instructions.Stage) map[string]strin logrus.Debugf("Built stage name to index map: %v", nameToIndex) return nameToIndex } + +func (s stageBuilder) initSnapshotWithTimings() error { + t := timing.Start("Initial FS snapshot") + if err := s.snapshotter.Init(); err != nil { + return err + } + timing.DefaultRun.Stop(t) + return nil +} diff --git a/pkg/executor/build_test.go b/pkg/executor/build_test.go index f3d4f363bc..385f5fe7ec 100644 --- a/pkg/executor/build_test.go +++ b/pkg/executor/build_test.go @@ -103,8 +103,9 @@ func Test_stageBuilder_shouldTakeSnapshot(t *testing.T) { cmds []commands.DockerCommand } type args struct { - index int - files []string + index int + files []string + hasFiles bool } tests := []struct { name string @@ -151,6 +152,32 @@ func Test_stageBuilder_shouldTakeSnapshot(t *testing.T) { }, want: true, }, + { + name: "not final stage not last command but empty list of files", + fields: fields{ + stage: config.KanikoStage{}, + }, + args: args{ + index: 0, + files: []string{}, + hasFiles: true, + }, + want: false, + }, + { + name: "not final stage not last command no files provided", + fields: fields{ + stage: config.KanikoStage{ + Final: false, + }, + }, + args: args{ + index: 0, + files: nil, + hasFiles: false, + }, + want: true, + }, { name: "caching enabled intermediate container", fields: fields{ @@ -177,7 +204,7 @@ func Test_stageBuilder_shouldTakeSnapshot(t *testing.T) { opts: tt.fields.opts, cmds: tt.fields.cmds, } - if got := s.shouldTakeSnapshot(tt.args.index, tt.args.files); got != tt.want { + if got := s.shouldTakeSnapshot(tt.args.index, tt.args.files, tt.args.hasFiles); got != tt.want { t.Errorf("stageBuilder.shouldTakeSnapshot() = %v, want %v", got, tt.want) } }) diff --git a/pkg/executor/fakes.go b/pkg/executor/fakes.go index e89d5ac47d..41793af124 100644 --- a/pkg/executor/fakes.go +++ b/pkg/executor/fakes.go @@ -55,6 +55,9 @@ func (m MockDockerCommand) String() string { func (m MockDockerCommand) FilesToSnapshot() []string { return []string{"meow-snapshot-no-cache"} } +func (m MockDockerCommand) ProvidesFilesToSnapshot() bool { + return true +} func (m MockDockerCommand) CacheCommand(image v1.Image) commands.DockerCommand { return m.cacheCommand } @@ -84,6 +87,9 @@ func (m MockCachedDockerCommand) String() string { func (m MockCachedDockerCommand) FilesToSnapshot() []string { return []string{"meow-snapshot"} } +func (m MockCachedDockerCommand) ProvidesFilesToSnapshot() bool { + return true +} func (m MockCachedDockerCommand) CacheCommand(image v1.Image) commands.DockerCommand { return nil }