Skip to content

Commit

Permalink
Sync filesystem before snapshotting so mtime can be relied on
Browse files Browse the repository at this point in the history
The default hashing algorithm used by kaniko to determine if two files
are the same uses the files' mtime (the inode's modification time). It
turns out that this time is not always up to date, meaning that a file
could be modified but when you stat the file, the modification time may
not yet have been updated.

The copy integration tests were adding the same directory twice, the
second instance being to test copying a directory with a wilcard '*'.
Since the mtime is sometimes not updated, this caused kaniko to
sometimes think the files were the same, and sometimes think they were
different, varying the number of layers it created.

Now we will update those tests to use a completely different set of
files instead of copying the same files again, and we add a new test
(`Dockerfile_test_copy_same_file`) which intentionally copies the same
file multiple times, which would reliably reproduce the issue.

We fix the issue by calling `sync` before we start comparing mtimes.
This will slow down layer snapshotting - on my personal machine it costs
~30 ms to call, and added ~4 seconds to running all of the
`Dockerfile_test_copy*` tests. I'm assuming that adding 30ms per layer
is okay, but it's a potential place to speed things up later if we need
to.

Fixes GoogleContainerTools#251

_Interesting note, if you build this same Dockerfile with devicemapper,
you end up with only 2 layers! `¯\_(ツ)_/¯` _
  • Loading branch information
bobcatfish committed Aug 2, 2018
1 parent 954b612 commit 76a8ab7
Show file tree
Hide file tree
Showing 8 changed files with 56 additions and 3 deletions.
1 change: 1 addition & 0 deletions integration/context/qux/qup
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
qup
1 change: 1 addition & 0 deletions integration/context/qux/quw/que
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
que
1 change: 1 addition & 0 deletions integration/context/qux/quz
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
quz
2 changes: 1 addition & 1 deletion integration/dockerfiles/Dockerfile_test_copy
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ COPY ./ dir/
COPY . newdir
COPY context/bar /baz/
COPY ["context/foo", "/tmp/foo" ]
COPY context/b* /baz/
COPY context/q* /qux/
COPY context/foo context/bar/ba? /test/
COPY context/arr[[]0].txt /mydir/
COPY context/bar/bat .
Expand Down
2 changes: 1 addition & 1 deletion integration/dockerfiles/Dockerfile_test_copy_bucket
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ COPY ./ dir/
COPY . newdir
COPY context/bar /baz/
COPY ["context/foo", "/tmp/foo" ]
COPY context/b* /baz/
COPY context/q* /qux/
COPY context/foo context/bar/ba? /test/
COPY context/arr[[]0].txt /mydir/
COPY context/bar/bat .
Expand Down
2 changes: 1 addition & 1 deletion integration/dockerfiles/Dockerfile_test_copy_reproducible
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ COPY ./ dir/
COPY . newdir
COPY context/bar /baz/
COPY ["context/foo", "/tmp/foo" ]
COPY context/b* /baz/
COPY context/q* /qux/
COPY context/foo context/bar/ba? /test/
COPY context/arr[[]0].txt /mydir/
COPY context/bar/bat .
Expand Down
43 changes: 43 additions & 0 deletions integration/dockerfiles/Dockerfile_test_copy_same_file
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
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
7 changes: 7 additions & 0 deletions pkg/snapshot/snapshot.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import (
"io/ioutil"
"os"
"path/filepath"
"syscall"

"github.com/GoogleContainerTools/kaniko/pkg/util"
"github.com/sirupsen/logrus"
Expand Down Expand Up @@ -54,6 +55,12 @@ func (s *Snapshotter) TakeSnapshot(files []string) ([]byte, error) {
buf := bytes.NewBuffer([]byte{})
var filesAdded bool
var err error

// 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.
syscall.Sync()

if files == nil {
filesAdded, err = s.snapShotFS(buf)
} else {
Expand Down

0 comments on commit 76a8ab7

Please sign in to comment.