Always snapshot files in COPY and RUN commands

Kaniko uses mtime (as well as file contents and other attributes) to
determine if files have changed. COPY and ADD commands should _always_
update the mtime, because they actually overwrite the files. However it
turns out that the mtime can lag, so kaniko would sometimes add a new
layer when using COPY or ADD on a file, and sometimes would not. This
leads to a non-deterministic number of layers.

To fix this, we have updated the kaniko commands to be more
authoritative in declaring when they have changed a file (e.g. WORKDIR
will now only create the directory when it doesn't exist) and we will
trust those files and _always_ add them, instead of only adding them if
they haven't changed.

It is possible for RUN commands to also change the filesystem, in which
case kaniko has no choice but to look at the filesystem to determine
what has changed. For this case we have added a call to `sync` however
we still cannot guarantee that sometimes the mtime will lag, causing the
number of layers to be non-deterministic. However when I tried to cause
this behaviour with the RUN command, I couldn't.

This changes the snapshotting logic a bit; before this change, the last
command of the last stage in a Dockerfile would always scan the whole
file system and ignore the files returned by the kaniko command. Instead
we will now trust those files and assume that the snapshotting
performed by previous commands will be adequate.

Docker itself seems to rely on the storage driver to determine when
files have changed and so doesn't have to deal with these problems
directly.

An alternative implementation would use `inotify` to track which files
have changed. However that would mean watching every file in the
filesystem, and adding new watches as files are added. Not only is there
a limit on the number of files that can be watched, but according to the
man pages a) this can take a significant amount of time b) there is
complication around when events arrive (e.g. by the time they arrive,
the files may have changed) and lastly c) events can be lost, which
would mean we'd run into this non-deterministic behaviour again anyway.

Fixes GoogleContainerTools#251

README.md

@@ -256,7 +256,8 @@ To configure credentials, you will need to do the following:
 #### --snapshotMode
 
 You can set the `--snapshotMode=<full (default), time>` flag to set how kaniko will snapshot the filesystem.
-If `--snapshotMode=time` is set, only file mtime will be considered when snapshotting.
+If `--snapshotMode=time` is set, only file mtime will be considered when snapshotting (see
+[limitations related to mtime](#mtime-and-snapshotting)).
 
 #### --build-arg
 
@@ -352,3 +353,19 @@ provides.
 [kaniko-users](https://groups.google.com/forum/#!forum/kaniko-users) Google group
 
 To Contribute to kaniko, see [DEVELOPMENT.md](DEVELOPMENT.md) and [CONTRIBUTING.md](CONTRIBUTING.md).
+
+## Limitations
+
+### mtime and snapshotting
+
+When taking a snapshot, kaniko's hashing algorithms include (or in the case of
+[`--snapshotMode=time`](#--snapshotmode), only use) a file's
+[`mtime`](https://en.wikipedia.org/wiki/Inode#POSIX_inode_description) to determine
+if the file has changed. Unfortunately there is a delay between when changes to a
+file are made and when the `mtime` is updated. This means:
+
+* With the default snapshot mode (`--snapshotMode=full`), whether or not kaniko will
+  add a layer in the case where a `RUN` command modifies a file but the contents do
+  not change is non-deterministic.
+* With the time-only snapshot mode (`--snapshotMode=time`), kaniko may miss changes
+  introduced by `RUN` commands entirely.

integration/dockerfiles/Dockerfile_test_copy_same_file_many_times

@@ -0,0 +1,49 @@
+FROM alpine@sha256:5ce5f501c457015c4b91f91a15ac69157d9b06f1a75cf9107bf2b62e0843983a
+COPY context/foo /foo
+COPY context/foo /foo
+COPY context/foo /foo
+COPY context/foo /foo
+COPY context/foo /foo
+COPY context/foo /foo
+COPY context/foo /foo
+COPY context/foo /foo
+COPY context/foo /foo
+COPY context/foo /foo
+COPY context/foo /foo
+COPY context/foo /foo
+COPY context/foo /foo
+COPY context/foo /foo
+COPY context/foo /foo
+COPY context/foo /foo
+COPY context/foo /foo
+COPY context/foo /foo
+COPY context/foo /foo
+COPY context/foo /foo
+COPY context/foo /foo
+COPY context/foo /foo
+COPY context/foo /foo
+COPY context/foo /foo
+COPY context/foo /foo
+COPY context/foo /foo
+COPY context/foo /foo
+COPY context/foo /foo
+COPY context/foo /foo
+COPY context/foo /foo
+COPY context/foo /foo
+COPY context/foo /foo
+COPY context/foo /foo
+COPY context/foo /foo
+COPY context/foo /foo
+COPY context/foo /foo
+COPY context/foo /foo
+COPY context/foo /foo
+COPY context/foo /foo
+COPY context/foo /foo
+COPY context/foo /foo
+COPY context/foo /foo
+COPY context/foo /foo
+COPY context/foo /foo
+COPY context/foo /foo
+COPY context/foo /foo
+COPY context/foo /foo
+COPY context/foo /foo

integration/integration_test.go

@@ -135,11 +135,10 @@ func TestMain(m *testing.M) {
 	defer DeleteFromBucket(fileInBucket)
 
 	fmt.Println("Building kaniko image")
-	buildKaniko := exec.Command("docker", "build", "-t", ExecutorImage, "-f", "../deploy/Dockerfile", "..")
-	err = buildKaniko.Run()
+	cmd := exec.Command("docker", "build", "-t", ExecutorImage, "-f", "../deploy/Dockerfile", "..")
+	_, err = RunCommandWithoutTest(cmd)
 	if err != nil {
-		fmt.Print(err)
-		fmt.Print("Building kaniko failed.")
+		fmt.Printf("Building kaniko failed: %s", err)
 		os.Exit(1)
 	}
 

pkg/commands/volume.go

@@ -17,6 +17,7 @@ limitations under the License.
 package commands
 
 import (
+	"fmt"
 	"os"
 	"strings"
 
@@ -53,13 +54,14 @@ func (v *VolumeCommand) ExecuteCommand(config *v1.Config, buildArgs *dockerfile.
 			return err
 		}
 
-		logrus.Infof("Creating directory %s", volume)
-		if err := os.MkdirAll(volume, 0755); err != nil {
-			return err
+		// Only create and snapshot the dir if it didn't exist already
+		if _, err := os.Stat(volume); os.IsNotExist(err) {
+			logrus.Infof("Creating directory %s", volume)
+			v.snapshotFiles = []string{volume}
+			if err := os.MkdirAll(volume, 0755); err != nil {
+				return fmt.Errorf("Could not create directory for volume %s: %s", volume, err)
+			}
 		}
-
-		//Check if directory already exists?
-		v.snapshotFiles = append(v.snapshotFiles, volume)
 	}
 	config.Volumes = existingVolumes
 

pkg/commands/workdir.go

@@ -47,8 +47,14 @@ func (w *WorkdirCommand) ExecuteCommand(config *v1.Config, buildArgs *dockerfile
 		config.WorkingDir = filepath.Join(config.WorkingDir, resolvedWorkingDir)
 	}
 	logrus.Infof("Changed working directory to %s", config.WorkingDir)
-	w.snapshotFiles = []string{config.WorkingDir}
-	return os.MkdirAll(config.WorkingDir, 0755)
+
+	// Only create and snapshot the dir if it didn't exist already
+	if _, err := os.Stat(config.WorkingDir); os.IsNotExist(err) {
+		logrus.Infof("Creating directory %s", config.WorkingDir)
+		w.snapshotFiles = []string{config.WorkingDir}
+		return os.MkdirAll(config.WorkingDir, 0755)
+	}
+	return nil
 }
 
 // FilesToSnapshot returns the workingdir, which should have been created if it didn't already exist

pkg/executor/executor.go

@@ -106,23 +106,41 @@ func DoBuild(k KanikoBuildArgs) (v1.Image, error) {
 			if err := dockerCommand.ExecuteCommand(&imageConfig.Config, buildArgs); err != nil {
 				return nil, err
 			}
-			// Don't snapshot if it's not the final stage and not the final command
-			// Also don't snapshot if it's the final stage, not the final command, and single snapshot is set
-			if (!finalStage && !finalCmd) || (finalStage && !finalCmd && k.SingleSnapshot) {
-				continue
-			}
-			// Now, we get the files to snapshot from this command and take the snapshot
 			snapshotFiles := dockerCommand.FilesToSnapshot()
-			if finalCmd {
-				snapshotFiles = nil
+			var contents []byte
+
+			// If this is an intermediate stage, we only snapshot for the last command and we
+			// want to snapshot the entire filesystem since we aren't tracking what was changed
+			// by previous commands.
+			if !finalStage {
+				if finalCmd {
+					contents, err = snapshotter.TakeSnapshotFS()
+				}
+			} else {
+				// If we are in single snapshot mode, we only take a snapshot once, after all
+				// commands have completed.
+				if k.SingleSnapshot {
+					if finalCmd {
+						contents, err = snapshotter.TakeSnapshotFS()
+					}
+				} else {
+					// Otherwise, in the final stage we take a snapshot at each command. If we know
+					// the files that were changed, we'll snapshot those explicitly, otherwise we'll
+					// check if anything in the filesystem changed.
+					if snapshotFiles != nil {
+						contents, err = snapshotter.TakeSnapshot(snapshotFiles)
+					} else {
+						contents, err = snapshotter.TakeSnapshotFS()
+					}
+				}
 			}
-			contents, err := snapshotter.TakeSnapshot(snapshotFiles)
 			if err != nil {
-				return nil, err
+				return nil, fmt.Errorf("Error taking snapshot of files for command %s: %s", dockerCommand, err)
 			}
+
 			util.MoveVolumeWhitelistToWhitelist()
 			if contents == nil {
-				logrus.Info("No files were changed, appending empty layer to config.")
+				logrus.Info("No files were changed, appending empty layer to config. No layer added to image.")
 				continue
 			}
 			// Append the layer to the image

pkg/snapshot/layered_map.go

@@ -17,6 +17,7 @@ limitations under the License.
 package snapshot
 
 import (
+	"fmt"
 	"path/filepath"
 	"strings"
 )
@@ -82,6 +83,20 @@ func (l *LayeredMap) MaybeAddWhiteout(s string) (bool, error) {
 	return true, nil
 }
 
+// Add will add the specified file s to the layered map.
+func (l *LayeredMap) Add(s string) error {
+	newV, err := l.hasher(s)
+	if err != nil {
+		return fmt.Errorf("Error creating hash for %s: %s", s, err)
+	}
+	l.layers[len(l.layers)-1][s] = newV
+	return nil
+}
+
+// MaybeAdd will add the specified file s to the layered map if
+// the layered map's hashing function determines it has changed. If
+// it has not changed, it will not be added. Returns true if the file
+// was added.
 func (l *LayeredMap) MaybeAdd(s string) (bool, error) {
 	oldV, ok := l.Get(s)
 	newV, err := l.hasher(s)

pkg/snapshot/snapshot.go

@@ -19,10 +19,12 @@ package snapshot
 import (
 	"archive/tar"
 	"bytes"
+	"fmt"
 	"io"
 	"io/ioutil"
 	"os"
 	"path/filepath"
+	"syscall"
 
 	"github.com/GoogleContainerTools/kaniko/pkg/util"
 	"github.com/sirupsen/logrus"
@@ -48,17 +50,28 @@ func (s *Snapshotter) Init() error {
 	return nil
 }
 
-// TakeSnapshot takes a snapshot of the filesystem, avoiding directories in the whitelist, and creates
+// TakeSnapshot takes a snapshot of the specified files, avoiding directories in the whitelist, and creates
 // a tarball of the changed files. Return contents of the tarball, and whether or not any files were changed
 func (s *Snapshotter) TakeSnapshot(files []string) ([]byte, error) {
 	buf := bytes.NewBuffer([]byte{})
 	var filesAdded bool
-	var err error
-	if files == nil {
-		filesAdded, err = s.snapShotFS(buf)
-	} else {
-		filesAdded, err = s.snapshotFiles(buf, files)
+	filesAdded, err := s.snapshotFiles(buf, files)
+	if err != nil {
+		return nil, err
+	}
+	contents := buf.Bytes()
+	if !filesAdded {
+		return nil, nil
 	}
+	return contents, err
+}
+
+// TakeSnapshotFS takes a snapshot of the filesystem, avoiding directories in the whitelist, and creates
+// a tarball of the changed files. Return contents of the tarball, and whether or not any files were changed
+func (s *Snapshotter) TakeSnapshotFS() ([]byte, error) {
+	buf := bytes.NewBuffer([]byte{})
+	var filesAdded bool
+	filesAdded, err := s.snapShotFS(buf)
 	if err != nil {
 		return nil, err
 	}
@@ -69,8 +82,8 @@ func (s *Snapshotter) TakeSnapshot(files []string) ([]byte, error) {
 	return contents, err
 }
 
-// snapshotFiles takes a snapshot of specific files
-// Used for ADD/COPY commands, when we know which files have changed
+// snapshotFiles creates a snapshot (tar) and adds the specified files.
+// It will not add files which are whitelisted.
 func (s *Snapshotter) snapshotFiles(f io.Writer, files []string) (bool, error) {
 	s.hardlinks = map[uint64]string{}
 	s.l.Snapshot()
@@ -103,23 +116,29 @@ func (s *Snapshotter) snapshotFiles(f io.Writer, files []string) (bool, error) {
 		if err != nil {
 			return false, err
 		}
-		// Only add to the tar if we add it to the layeredmap.
-		addFile, err := s.l.MaybeAdd(file)
+		err = s.l.Add(file)
 		if err != nil {
-			return false, err
+			return false, fmt.Errorf("Unable to add file %s to layered map: %s", file, err)
 		}
-		if addFile {
-			filesAdded = true
-			if err := util.AddToTar(file, info, s.hardlinks, w); err != nil {
-				return false, err
-			}
+		filesAdded = true
+		if err := util.AddToTar(file, info, s.hardlinks, w); err != nil {
+			return false, err
 		}
 	}
 	return filesAdded, nil
 }
 
+// shapShotFS creates a snapshot (tar) of all files in the system which are not
+// whitelisted and which have changed.
 func (s *Snapshotter) snapShotFS(f io.Writer) (bool, error) {
 	logrus.Info("Taking snapshot of full filesystem...")
+
+	// Some of the operations that follow (e.g. hashing) depend on the file system being synced,
+	// for example the hashing function that determines if files are equal uses the mtime of the files,
+	// which can lag if sync is not called. Unfortunately there can still be lag if too much data needs
+	// to be flushed or the disk does its own caching/buffering.
+	syscall.Sync()
+
 	s.hardlinks = map[uint64]string{}
 	s.l.Snapshot()
 	existingPaths := s.l.GetFlattenedPathsForWhiteOut()

pkg/snapshot/snapshot_test.go

@@ -29,7 +29,7 @@ import (
 	"github.com/pkg/errors"
 )
 
-func TestSnapshotFileChange(t *testing.T) {
+func TestSnapshotFSFileChange(t *testing.T) {
 
 	testDir, snapshotter, err := setUpTestDir()
 	defer os.RemoveAll(testDir)
@@ -45,7 +45,7 @@ func TestSnapshotFileChange(t *testing.T) {
 		t.Fatalf("Error setting up fs: %s", err)
 	}
 	// Take another snapshot
-	contents, err := snapshotter.TakeSnapshot(nil)
+	contents, err := snapshotter.TakeSnapshotFS()
 	if err != nil {
 		t.Fatalf("Error taking snapshot of fs: %s", err)
 	}
@@ -81,7 +81,7 @@ func TestSnapshotFileChange(t *testing.T) {
 	}
 }
 
-func TestSnapshotChangePermissions(t *testing.T) {
+func TestSnapshotFSChangePermissions(t *testing.T) {
 	testDir, snapshotter, err := setUpTestDir()
 	defer os.RemoveAll(testDir)
 	if err != nil {
@@ -93,7 +93,7 @@ func TestSnapshotChangePermissions(t *testing.T) {
 		t.Fatalf("Error changing permissions on %s: %v", batPath, err)
 	}
 	// Take another snapshot
-	contents, err := snapshotter.TakeSnapshot(nil)
+	contents, err := snapshotter.TakeSnapshotFS()
 	if err != nil {
 		t.Fatalf("Error taking snapshot of fs: %s", err)
 	}
@@ -166,14 +166,14 @@ func TestSnapshotFiles(t *testing.T) {
 	testutil.CheckErrorAndDeepEqual(t, false, nil, expectedFiles, actualFiles)
 }
 
-func TestEmptySnapshot(t *testing.T) {
+func TestEmptySnapshotFS(t *testing.T) {
 	testDir, snapshotter, err := setUpTestDir()
 	defer os.RemoveAll(testDir)
 	if err != nil {
 		t.Fatal(err)
 	}
 	// Take snapshot with no changes
-	contents, err := snapshotter.TakeSnapshot(nil)
+	contents, err := snapshotter.TakeSnapshotFS()
 	if err != nil {
 		t.Fatalf("Error taking snapshot of fs: %s", err)
 	}