From 691acd094ab6e9796c16d11694cfef083b05ed2e Mon Sep 17 00:00:00 2001 From: Tejal Desai Date: Wed, 29 Apr 2020 22:42:33 -0700 Subject: [PATCH 01/12] try different root dir --- pkg/constants/constants.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/constants/constants.go b/pkg/constants/constants.go index 8dd5893ea7..19c46cef36 100644 --- a/pkg/constants/constants.go +++ b/pkg/constants/constants.go @@ -18,7 +18,7 @@ package constants const ( // RootDir is the path to the root directory - RootDir = "/" + RootDir = "/root" // WorkspaceDir is the path to the workspace directory WorkspaceDir = "/workspace" From 8fb17f60d9fda35d9b460f928447c58110b55e08 Mon Sep 17 00:00:00 2001 From: Tejal Desai Date: Thu, 30 Apr 2020 13:25:22 -0700 Subject: [PATCH 02/12] Defer initial snapshot. Remove ReadSuccess() --- pkg/commands/cache.go | 6 ------ pkg/commands/copy.go | 2 -- pkg/commands/run.go | 1 - pkg/constants/constants.go | 2 +- pkg/executor/build.go | 38 +++++++++++++++++++++----------------- pkg/snapshot/snapshot.go | 1 + 6 files changed, 23 insertions(+), 27 deletions(-) diff --git a/pkg/commands/cache.go b/pkg/commands/cache.go index 1d37e62511..63970734fb 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 } func (c caching) Layer() v1.Layer { return c.layer } - -func (c caching) ReadSuccess() bool { - return c.readSuccess -} diff --git a/pkg/commands/copy.go b/pkg/commands/copy.go index e31efe2f3a..472218fd66 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) diff --git a/pkg/commands/run.go b/pkg/commands/run.go index 23b7f5fd74..d4a8003bff 100644 --- a/pkg/commands/run.go +++ b/pkg/commands/run.go @@ -200,7 +200,6 @@ func (cr *CachingRunCommand) ExecuteCommand(config *v1.Config, buildArgs *docker } cr.layer = layers[0] - cr.readSuccess = true cr.extractedFiles, err = util.GetFSFromLayers( kConfig.RootDir, diff --git a/pkg/constants/constants.go b/pkg/constants/constants.go index 19c46cef36..8dd5893ea7 100644 --- a/pkg/constants/constants.go +++ b/pkg/constants/constants.go @@ -18,7 +18,7 @@ package constants const ( // RootDir is the path to the root directory - RootDir = "/root" + RootDir = "/" // WorkspaceDir is the path to the workspace directory WorkspaceDir = "/workspace" diff --git a/pkg/executor/build.go b/pkg/executor/build.go index 015466bf7b..7090edc2f1 100644 --- a/pkg/executor/build.go +++ b/pkg/executor/build.go @@ -317,13 +317,7 @@ 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 - } - - timing.DefaultRun.Stop(t) + initSnapshotTaken := false cacheGroup := errgroup.Group{} for index, command := range s.cmds { @@ -346,6 +340,24 @@ func (s *stageBuilder) build() error { logrus.Info(command.String()) + isCacheCommand := func() bool { + switch command.(type) { + case commands.Cached: + return true + default: + return false + } + } + if !isCacheCommand() && !initSnapshotTaken { + // Take initial snapshot + t := timing.Start("Initial FS snapshot") + if err := s.snapshotter.Init(); err != nil { + return err + } + timing.DefaultRun.Stop(t) + initSnapshotTaken = true + } + if err := command.ExecuteCommand(&s.cf.Config, s.args); err != nil { return errors.Wrap(err, "failed to execute command") } @@ -356,16 +368,7 @@ func (s *stageBuilder) build() error { 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 { @@ -388,6 +391,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 { + logrus.Infof("%s is saved at %s", ck, tarPath) return s.pushLayerToCache(s.opts, ck, tarPath, command.String()) }) } diff --git a/pkg/snapshot/snapshot.go b/pkg/snapshot/snapshot.go index 7b3df5661e..d750601a41 100644 --- a/pkg/snapshot/snapshot.go +++ b/pkg/snapshot/snapshot.go @@ -51,6 +51,7 @@ func NewSnapshotter(l *LayeredMap, d string) *Snapshotter { // Init initializes a new snapshotter func (s *Snapshotter) Init() error { + logrus.Info("Taking initial snapshot") _, _, err := s.scanFullFilesystem() return err } From 44b35b28eb3618abb4ede53d9695cccdafdedff4 Mon Sep 17 00:00:00 2001 From: Tejal Desai Date: Thu, 30 Apr 2020 15:18:39 -0700 Subject: [PATCH 03/12] remove read succesS --- pkg/commands/cache_test.go | 6 +----- pkg/commands/copy_test.go | 4 ---- pkg/commands/run_test.go | 4 ---- 3 files changed, 1 insertion(+), 13 deletions(-) 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/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_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) - } }) } } From 8b3ff95b098b07be047796c1e46902b231ced991 Mon Sep 17 00:00:00 2001 From: Tejal Desai Date: Thu, 30 Apr 2020 17:07:18 -0700 Subject: [PATCH 04/12] fmt --- pkg/commands/cache.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/commands/cache.go b/pkg/commands/cache.go index 63970734fb..878440df47 100644 --- a/pkg/commands/cache.go +++ b/pkg/commands/cache.go @@ -23,7 +23,7 @@ type Cached interface { } type caching struct { - layer v1.Layer + layer v1.Layer } func (c caching) Layer() v1.Layer { From 2e1ca5f19df4c8a9c5218815bea3c2924b56959d Mon Sep 17 00:00:00 2001 From: Tejal Desai Date: Fri, 1 May 2020 09:40:39 -0700 Subject: [PATCH 05/12] remove log added for debugging --- pkg/executor/build.go | 1 - 1 file changed, 1 deletion(-) diff --git a/pkg/executor/build.go b/pkg/executor/build.go index 7090edc2f1..45773b6c2d 100644 --- a/pkg/executor/build.go +++ b/pkg/executor/build.go @@ -391,7 +391,6 @@ 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 { - logrus.Infof("%s is saved at %s", ck, tarPath) return s.pushLayerToCache(s.opts, ck, tarPath, command.String()) }) } From 32e3336d4c6d4ee2ebdbd6b80ff42c7d5d406146 Mon Sep 17 00:00:00 2001 From: Tejal Desai Date: Fri, 1 May 2020 15:50:46 -0700 Subject: [PATCH 06/12] add a new method to indicate if the command provides files to snapshot --- pkg/commands/add.go | 4 ++++ pkg/commands/base_command.go | 4 ++++ pkg/commands/commands.go | 4 ++++ pkg/commands/copy.go | 12 ++++++++++++ pkg/commands/run.go | 12 ++++++++++++ pkg/commands/workdir.go | 4 ++++ pkg/executor/build.go | 21 ++++++++++++--------- pkg/executor/fakes.go | 6 ++++++ pkg/snapshot/snapshot.go | 4 +++- 9 files changed, 61 insertions(+), 10 deletions(-) diff --git a/pkg/commands/add.go b/pkg/commands/add.go index b39b5eb982..75a6b5b20c 100644 --- a/pkg/commands/add.go +++ b/pkg/commands/add.go @@ -116,6 +116,10 @@ func (a *AddCommand) FilesToSnapshot() []string { return a.snapshotFiles } +func (a *AddCommand) ProvidesFilesToSnapshot() bool { + return true +} + // String returns some information about the command for the image config func (a *AddCommand) String() string { return a.cmd.String() diff --git a/pkg/commands/base_command.go b/pkg/commands/base_command.go index ddbfd650bd..b96bf85d91 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 false +} + func (b *BaseCommand) FilesUsedFromContext(_ *v1.Config, _ *dockerfile.BuildArgs) ([]string, error) { return []string{}, nil } 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 472218fd66..35505284f6 100644 --- a/pkg/commands/copy.go +++ b/pkg/commands/copy.go @@ -138,6 +138,10 @@ func (c *CopyCommand) MetadataOnly() bool { return false } +func (c *CopyCommand) ProvidesFilesToSnapshot() bool { + return true +} + func (c *CopyCommand) RequiresUnpackedFS() bool { return true } @@ -210,6 +214,14 @@ func (cr *CachingCopyCommand) FilesToSnapshot() []string { return f } +func (cr *CachingCopyCommand) MetadataOnly() bool { + return false +} + +func (cr *CachingCopyCommand) ProvidesFilesToSnapshot() bool { + return true +} + func (cr *CachingCopyCommand) String() string { if cr.cmd == nil { return "nil command" diff --git a/pkg/commands/run.go b/pkg/commands/run.go index d4a8003bff..d3ed0d1bbf 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 { @@ -227,3 +231,11 @@ func (cr *CachingRunCommand) String() string { } return cr.cmd.String() } + +func (cr *CachingRunCommand) MetadataOnly() bool { + return false +} + +func (cr *CachingRunCommand) RequiresUnpackedFS() bool { + return true +} diff --git a/pkg/commands/workdir.go b/pkg/commands/workdir.go index 2734320521..96104bb131 100644 --- a/pkg/commands/workdir.go +++ b/pkg/commands/workdir.go @@ -75,3 +75,7 @@ func (w *WorkdirCommand) String() string { func (w *WorkdirCommand) MetadataOnly() bool { return false } + +func (w *WorkdirCommand) ProvidesFilesToSnapshot() bool { + return true +} diff --git a/pkg/executor/build.go b/pkg/executor/build.go index 45773b6c2d..b6961989fe 100644 --- a/pkg/executor/build.go +++ b/pkg/executor/build.go @@ -347,15 +347,18 @@ func (s *stageBuilder) build() error { default: return false } - } - if !isCacheCommand() && !initSnapshotTaken { - // Take initial snapshot - t := timing.Start("Initial FS snapshot") - if err := s.snapshotter.Init(); err != nil { - return err + }() + if !initSnapshotTaken && !isCacheCommand && !command.MetadataOnly() { + if !command.ProvidesFilesToSnapshot() { + // Take initial snapshot if command is not metadata only + // and does not return a list of files changed + t := timing.Start("Initial FS snapshot") + if err := s.snapshotter.Init(); err != nil { + return err + } + timing.DefaultRun.Stop(t) + initSnapshotTaken = true } - timing.DefaultRun.Stop(t) - initSnapshotTaken = true } if err := command.ExecuteCommand(&s.cf.Config, s.args); err != nil { @@ -368,7 +371,7 @@ func (s *stageBuilder) build() error { continue } - if isCacheCommand() { + if isCacheCommand { v := command.(commands.Cached) layer := v.Layer() if err := s.saveLayerToImage(layer, command.String()); err != nil { 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 } diff --git a/pkg/snapshot/snapshot.go b/pkg/snapshot/snapshot.go index d750601a41..6b8f9a8a53 100644 --- a/pkg/snapshot/snapshot.go +++ b/pkg/snapshot/snapshot.go @@ -39,6 +39,7 @@ var snapshotPathPrefix = config.KanikoDir // Snapshotter holds the root directory from which to take snapshots, and a list of snapshots taken type Snapshotter struct { + i int l *LayeredMap directory string whitelist []util.WhitelistEntry @@ -51,7 +52,8 @@ func NewSnapshotter(l *LayeredMap, d string) *Snapshotter { // Init initializes a new snapshotter func (s *Snapshotter) Init() error { - logrus.Info("Taking initial snapshot") + s.i++ + logrus.Infof("Taking initial snapshot %d", s.i) _, _, err := s.scanFullFilesystem() return err } From 7d32139a131050214d2fd017b4b6603242d60084 Mon Sep 17 00:00:00 2001 From: Tejal Desai Date: Fri, 1 May 2020 22:35:59 -0700 Subject: [PATCH 07/12] fix a bug where file check nil is insufficient --- pkg/executor/build.go | 8 ++++---- pkg/executor/build_test.go | 37 ++++++++++++++++++++++++++++++++++--- pkg/snapshot/snapshot.go | 3 --- 3 files changed, 38 insertions(+), 10 deletions(-) diff --git a/pkg/executor/build.go b/pkg/executor/build.go index b6961989fe..b07be09e59 100644 --- a/pkg/executor/build.go +++ b/pkg/executor/build.go @@ -367,7 +367,7 @@ func (s *stageBuilder) build() error { files = command.FilesToSnapshot() timing.DefaultRun.Stop(t) - if !s.shouldTakeSnapshot(index, files) { + if !s.shouldTakeSnapshot(index, files, command.ProvidesFilesToSnapshot()) { continue } @@ -425,7 +425,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.stage.Commands)-1 // We only snapshot the very end with single snapshot mode on. @@ -438,8 +438,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 } diff --git a/pkg/executor/build_test.go b/pkg/executor/build_test.go index ff97aa03ea..18e64623ff 100644 --- a/pkg/executor/build_test.go +++ b/pkg/executor/build_test.go @@ -114,8 +114,9 @@ func Test_stageBuilder_shouldTakeSnapshot(t *testing.T) { opts *config.KanikoOptions } type args struct { - index int - files []string + index int + files []string + hasFiles bool } tests := []struct { name string @@ -162,6 +163,36 @@ 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{ + Final: false, + Stage: stage, + }, + }, + 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, + Stage: stage, + }, + }, + args: args{ + index: 0, + files: nil, + hasFiles: false, + }, + want: true, + }, { name: "caching enabled intermediate container", fields: fields{ @@ -187,7 +218,7 @@ func Test_stageBuilder_shouldTakeSnapshot(t *testing.T) { stage: tt.fields.stage, opts: tt.fields.opts, } - 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/snapshot/snapshot.go b/pkg/snapshot/snapshot.go index 6b8f9a8a53..7b3df5661e 100644 --- a/pkg/snapshot/snapshot.go +++ b/pkg/snapshot/snapshot.go @@ -39,7 +39,6 @@ var snapshotPathPrefix = config.KanikoDir // Snapshotter holds the root directory from which to take snapshots, and a list of snapshots taken type Snapshotter struct { - i int l *LayeredMap directory string whitelist []util.WhitelistEntry @@ -52,8 +51,6 @@ func NewSnapshotter(l *LayeredMap, d string) *Snapshotter { // Init initializes a new snapshotter func (s *Snapshotter) Init() error { - s.i++ - logrus.Infof("Taking initial snapshot %d", s.i) _, _, err := s.scanFullFilesystem() return err } From 75864d5c85375a9211c0a3e9a55be91f065e3dd5 Mon Sep 17 00:00:00 2001 From: Tejal Desai Date: Fri, 1 May 2020 23:41:06 -0700 Subject: [PATCH 08/12] flip the flag --- pkg/commands/add.go | 4 ---- pkg/commands/base_command.go | 2 +- pkg/commands/copy.go | 8 -------- pkg/commands/workdir.go | 6 +----- pkg/executor/build.go | 4 +++- 5 files changed, 5 insertions(+), 19 deletions(-) diff --git a/pkg/commands/add.go b/pkg/commands/add.go index 75a6b5b20c..b39b5eb982 100644 --- a/pkg/commands/add.go +++ b/pkg/commands/add.go @@ -116,10 +116,6 @@ func (a *AddCommand) FilesToSnapshot() []string { return a.snapshotFiles } -func (a *AddCommand) ProvidesFilesToSnapshot() bool { - return true -} - // String returns some information about the command for the image config func (a *AddCommand) String() string { return a.cmd.String() diff --git a/pkg/commands/base_command.go b/pkg/commands/base_command.go index b96bf85d91..94c4fe1561 100644 --- a/pkg/commands/base_command.go +++ b/pkg/commands/base_command.go @@ -33,7 +33,7 @@ func (b *BaseCommand) FilesToSnapshot() []string { } func (b *BaseCommand) ProvidesFilesToSnapshot() bool { - return false + return true } func (b *BaseCommand) FilesUsedFromContext(_ *v1.Config, _ *dockerfile.BuildArgs) ([]string, error) { diff --git a/pkg/commands/copy.go b/pkg/commands/copy.go index 35505284f6..4e667a3731 100644 --- a/pkg/commands/copy.go +++ b/pkg/commands/copy.go @@ -138,10 +138,6 @@ func (c *CopyCommand) MetadataOnly() bool { return false } -func (c *CopyCommand) ProvidesFilesToSnapshot() bool { - return true -} - func (c *CopyCommand) RequiresUnpackedFS() bool { return true } @@ -218,10 +214,6 @@ func (cr *CachingCopyCommand) MetadataOnly() bool { return false } -func (cr *CachingCopyCommand) ProvidesFilesToSnapshot() bool { - return true -} - func (cr *CachingCopyCommand) String() string { if cr.cmd == nil { return "nil command" diff --git a/pkg/commands/workdir.go b/pkg/commands/workdir.go index 96104bb131..e95d0ea3e9 100644 --- a/pkg/commands/workdir.go +++ b/pkg/commands/workdir.go @@ -74,8 +74,4 @@ func (w *WorkdirCommand) String() string { func (w *WorkdirCommand) MetadataOnly() bool { return false -} - -func (w *WorkdirCommand) ProvidesFilesToSnapshot() bool { - return true -} +} \ No newline at end of file diff --git a/pkg/executor/build.go b/pkg/executor/build.go index b07be09e59..45adc8bc46 100644 --- a/pkg/executor/build.go +++ b/pkg/executor/build.go @@ -318,6 +318,7 @@ func (s *stageBuilder) build() error { } initSnapshotTaken := false + layer := 0 cacheGroup := errgroup.Group{} for index, command := range s.cmds { @@ -370,7 +371,8 @@ func (s *stageBuilder) build() error { if !s.shouldTakeSnapshot(index, files, command.ProvidesFilesToSnapshot()) { continue } - + layer++ + logrus.Infof(fmt.Sprintf("%d", layer)) if isCacheCommand { v := command.(commands.Cached) layer := v.Layer() From db7687dbce92172854d6fb9d75e751a4b2d7ff58 Mon Sep 17 00:00:00 2001 From: Tejal Desai Date: Sun, 3 May 2020 18:37:45 -0700 Subject: [PATCH 09/12] wip --- pkg/executor/build.go | 29 ++++++++++++++++------------- 1 file changed, 16 insertions(+), 13 deletions(-) diff --git a/pkg/executor/build.go b/pkg/executor/build.go index 45adc8bc46..1ccee67cba 100644 --- a/pkg/executor/build.go +++ b/pkg/executor/build.go @@ -318,7 +318,6 @@ func (s *stageBuilder) build() error { } initSnapshotTaken := false - layer := 0 cacheGroup := errgroup.Group{} for index, command := range s.cmds { @@ -349,17 +348,15 @@ func (s *stageBuilder) build() error { return false } }() - if !initSnapshotTaken && !isCacheCommand && !command.MetadataOnly() { - if !command.ProvidesFilesToSnapshot() { - // Take initial snapshot if command is not metadata only - // and does not return a list of files changed + if !initSnapshotTaken && !command.ProvidesFilesToSnapshot() { + // Take initial snapshot if command does not expect to return + // a list of files. t := timing.Start("Initial FS snapshot") if err := s.snapshotter.Init(); err != nil { return err } timing.DefaultRun.Stop(t) initSnapshotTaken = true - } } if err := command.ExecuteCommand(&s.cf.Config, s.args); err != nil { @@ -371,8 +368,6 @@ func (s *stageBuilder) build() error { if !s.shouldTakeSnapshot(index, files, command.ProvidesFilesToSnapshot()) { continue } - layer++ - logrus.Infof(fmt.Sprintf("%d", layer)) if isCacheCommand { v := command.(commands.Cached) layer := v.Layer() @@ -415,6 +410,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() @@ -427,12 +423,19 @@ func (s *stageBuilder) takeSnapshot(files []string) (string, error) { return snapshot, err } -func (s *stageBuilder) shouldTakeSnapshot(index int, files []string, provideFiles bool) bool { - isLastCommand := index == len(s.stage.Commands)-1 +func (s *stageBuilder) singleSnapshot(index int) bool { + isLastCommand := index == len(s.stage.Commands)-1 - // We only snapshot the very end with single snapshot mode on. - if s.opts.SingleSnapshot { - return isLastCommand + // We only snapshot the very end with single snapshot mode on. + if s.opts.SingleSnapshot { + return isLastCommand + } + return false +} + +func (s *stageBuilder) shouldTakeSnapshot(index int, files []string, provideFiles bool) bool { + if s.singleSnapshot(index){ + return true } // Always take snapshots if we're using the cache. From 5090baafda838ada09796edac0c26ffe0d52ad93 Mon Sep 17 00:00:00 2001 From: Tejal Desai Date: Sun, 3 May 2020 20:23:07 -0700 Subject: [PATCH 10/12] fix single snapshot --- pkg/commands/workdir.go | 2 +- pkg/executor/build.go | 48 +++++++++++++++++++++++------------------ 2 files changed, 28 insertions(+), 22 deletions(-) diff --git a/pkg/commands/workdir.go b/pkg/commands/workdir.go index e95d0ea3e9..2734320521 100644 --- a/pkg/commands/workdir.go +++ b/pkg/commands/workdir.go @@ -74,4 +74,4 @@ func (w *WorkdirCommand) String() string { func (w *WorkdirCommand) MetadataOnly() bool { return false -} \ No newline at end of file +} diff --git a/pkg/executor/build.go b/pkg/executor/build.go index 1ccee67cba..f920034546 100644 --- a/pkg/executor/build.go +++ b/pkg/executor/build.go @@ -318,6 +318,12 @@ func (s *stageBuilder) build() error { } initSnapshotTaken := false + if s.opts.SingleSnapshot { + if err := s.initSnapshotWithTimings(); err != nil { + return err + } + initSnapshotTaken = true + } cacheGroup := errgroup.Group{} for index, command := range s.cmds { @@ -348,15 +354,13 @@ func (s *stageBuilder) build() error { return false } }() - if !initSnapshotTaken && !command.ProvidesFilesToSnapshot() { - // Take initial snapshot if command does not expect to return - // a list of files. - t := timing.Start("Initial FS snapshot") - if err := s.snapshotter.Init(); err != nil { - return err - } - timing.DefaultRun.Stop(t) - initSnapshotTaken = true + 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 { @@ -423,19 +427,12 @@ func (s *stageBuilder) takeSnapshot(files []string) (string, error) { return snapshot, err } -func (s *stageBuilder) singleSnapshot(index int) bool { - isLastCommand := index == len(s.stage.Commands)-1 - - // We only snapshot the very end with single snapshot mode on. - if s.opts.SingleSnapshot { - return isLastCommand - } - return false -} - func (s *stageBuilder) shouldTakeSnapshot(index int, files []string, provideFiles bool) bool { - if s.singleSnapshot(index){ - return true + isLastCommand := index == len(s.stage.Commands)-1 + + // We only snapshot the very end with single snapshot mode on. + if s.opts.SingleSnapshot { + return isLastCommand } // Always take snapshots if we're using the cache. @@ -846,3 +843,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 +} From 095ea2991abfad616b1f0cce73548941f9882ab1 Mon Sep 17 00:00:00 2001 From: Tejal Desai Date: Sun, 3 May 2020 20:40:41 -0700 Subject: [PATCH 11/12] remove unused command --- pkg/commands/run.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/pkg/commands/run.go b/pkg/commands/run.go index d3ed0d1bbf..fd0d7fa957 100644 --- a/pkg/commands/run.go +++ b/pkg/commands/run.go @@ -235,7 +235,3 @@ func (cr *CachingRunCommand) String() string { func (cr *CachingRunCommand) MetadataOnly() bool { return false } - -func (cr *CachingRunCommand) RequiresUnpackedFS() bool { - return true -} From ee097f9b707ea038fd322a7d379ab348c925ad2f Mon Sep 17 00:00:00 2001 From: Tejal Desai Date: Sun, 3 May 2020 22:01:50 -0700 Subject: [PATCH 12/12] fix unit tests --- pkg/executor/build_test.go | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/pkg/executor/build_test.go b/pkg/executor/build_test.go index 42c67a7fb7..385f5fe7ec 100644 --- a/pkg/executor/build_test.go +++ b/pkg/executor/build_test.go @@ -155,10 +155,7 @@ func Test_stageBuilder_shouldTakeSnapshot(t *testing.T) { { name: "not final stage not last command but empty list of files", fields: fields{ - stage: config.KanikoStage{ - Final: false, - Stage: stage, - }, + stage: config.KanikoStage{}, }, args: args{ index: 0, @@ -172,7 +169,6 @@ func Test_stageBuilder_shouldTakeSnapshot(t *testing.T) { fields: fields{ stage: config.KanikoStage{ Final: false, - Stage: stage, }, }, args: args{