Merge pull request #1214 from tejal29/experiment

Snapshot FS on first cache miss.

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
 }

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
-}

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())
-	}
 }

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
 

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"

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)
-			}
 		})
 	}
 }

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
+}

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)
-			}
 		})
 	}
 }

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
+}

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)
 			}
 		})

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
 }