From e2c6a8be7c03d3c8405421c07ec6fb1c5c3a6200 Mon Sep 17 00:00:00 2001 From: Shijiang Wei Date: Fri, 29 May 2015 16:39:14 +0800 Subject: [PATCH] Add the parent directory to changes set if new files are generated The "TestChangesWithChanges" case randomlly fails on my development VM with the following errors: ``` --- FAIL: TestChangesWithChanges (0.00s) changes_test.go:201: no change for expected change C /dir1/subfolder != A /dir1/subfolder/newFile ``` If I apply the following patch to changes_test.go, the test passes. ```diff diff --git a/pkg/archive/changes_test.go b/pkg/archive/changes_test.go index 290b2dd..ba1aca0 100644 --- a/pkg/archive/changes_test.go +++ b/pkg/archive/changes_test.go @@ -156,6 +156,7 @@ func TestChangesWithChanges(t *testing.T) { } defer os.RemoveAll(layer) createSampleDir(t, layer) + time.Sleep(5 * time.Millisecond) os.MkdirAll(path.Join(layer, "dir1/subfolder"), 0740) // Let's modify modtime for dir1 to be sure it's the same for the two layer (to not having false positive) ``` It seems that if a file is created immediately after the directory is created, the `archive.Changes` function could't recognize that the parent directory of the new file is modified. Perhaps the problem may reproduce on machines with low time precision? I had successfully reproduced the failure on my development VM as well as a VM on DigitalOcean. Signed-off-by: Shijiang Wei --- pkg/archive/changes.go | 21 +++++- pkg/archive/changes_test.go | 130 +++++++++++++++++++++++++----------- 2 files changed, 110 insertions(+), 41 deletions(-) diff --git a/pkg/archive/changes.go b/pkg/archive/changes.go index affafad39e..66a1cc73c6 100644 --- a/pkg/archive/changes.go +++ b/pkg/archive/changes.go @@ -68,7 +68,11 @@ func sameFsTimeSpec(a, b syscall.Timespec) bool { // Changes walks the path rw and determines changes for the files in the path, // with respect to the parent layers func Changes(layers []string, rw string) ([]Change, error) { - var changes []Change + var ( + changes []Change + changedDirs = make(map[string]struct{}) + ) + err := filepath.Walk(rw, func(path string, f os.FileInfo, err error) error { if err != nil { return err @@ -129,6 +133,21 @@ func Changes(layers []string, rw string) ([]Change, error) { } } + // If /foo/bar/file.txt is modified, then /foo/bar must be part of the changed files. + // This block is here to ensure the change is recorded even if the + // modify time, mode and size of the parent directoriy in the rw and ro layers are all equal. + // Check https://github.com/docker/docker/pull/13590 for details. + if f.IsDir() { + changedDirs[path] = struct{}{} + } + if change.Kind == ChangeAdd || change.Kind == ChangeDelete { + parent := filepath.Dir(path) + if _, ok := changedDirs[parent]; !ok && parent != "/" { + changes = append(changes, Change{Path: parent, Kind: ChangeModify}) + changedDirs[parent] = struct{}{} + } + } + // Record change changes = append(changes, change) return nil diff --git a/pkg/archive/changes_test.go b/pkg/archive/changes_test.go index 290b2dd402..509bdb2e6d 100644 --- a/pkg/archive/changes_test.go +++ b/pkg/archive/changes_test.go @@ -6,7 +6,6 @@ import ( "os/exec" "path" "sort" - "syscall" "testing" "time" ) @@ -132,12 +131,23 @@ func TestChangesWithNoChanges(t *testing.T) { } func TestChangesWithChanges(t *testing.T) { + // Mock the readonly layer + layer, err := ioutil.TempDir("", "docker-changes-test-layer") + if err != nil { + t.Fatal(err) + } + defer os.RemoveAll(layer) + createSampleDir(t, layer) + os.MkdirAll(path.Join(layer, "dir1/subfolder"), 0740) + + // Mock the RW layer rwLayer, err := ioutil.TempDir("", "docker-changes-test") if err != nil { t.Fatal(err) } defer os.RemoveAll(rwLayer) - // Create a folder + + // Create a folder in RW layer dir1 := path.Join(rwLayer, "dir1") os.MkdirAll(dir1, 0740) deletedFile := path.Join(dir1, ".wh.file1-2") @@ -149,58 +159,76 @@ func TestChangesWithChanges(t *testing.T) { os.MkdirAll(subfolder, 0740) newFile := path.Join(subfolder, "newFile") ioutil.WriteFile(newFile, []byte{}, 0740) - // Let's create folders that with have the role of layers with the same data - layer, err := ioutil.TempDir("", "docker-changes-test-layer") - if err != nil { - t.Fatal(err) - } - defer os.RemoveAll(layer) - createSampleDir(t, layer) - os.MkdirAll(path.Join(layer, "dir1/subfolder"), 0740) - - // Let's modify modtime for dir1 to be sure it's the same for the two layer (to not having false positive) - fi, err := os.Stat(dir1) - if err != nil { - return - } - mtime := fi.ModTime() - stat := fi.Sys().(*syscall.Stat_t) - atime := time.Unix(int64(stat.Atim.Sec), int64(stat.Atim.Nsec)) - - layerDir1 := path.Join(layer, "dir1") - os.Chtimes(layerDir1, atime, mtime) changes, err := Changes([]string{layer}, rwLayer) if err != nil { t.Fatal(err) } - sort.Sort(changesByPath(changes)) - expectedChanges := []Change{ + {"/dir1", ChangeModify}, {"/dir1/file1-1", ChangeModify}, {"/dir1/file1-2", ChangeDelete}, {"/dir1/subfolder", ChangeModify}, {"/dir1/subfolder/newFile", ChangeAdd}, } + checkChanges(expectedChanges, changes, t) +} - for i := 0; i < max(len(changes), len(expectedChanges)); i++ { - if i >= len(expectedChanges) { - t.Fatalf("unexpected change %s\n", changes[i].String()) - } - if i >= len(changes) { - t.Fatalf("no change for expected change %s\n", expectedChanges[i].String()) - } - if changes[i].Path == expectedChanges[i].Path { - if changes[i] != expectedChanges[i] { - t.Fatalf("Wrong change for %s, expected %s, got %s\n", changes[i].Path, changes[i].String(), expectedChanges[i].String()) - } - } else if changes[i].Path < expectedChanges[i].Path { - t.Fatalf("unexpected change %s\n", changes[i].String()) - } else { - t.Fatalf("no change for expected change %s != %s\n", expectedChanges[i].String(), changes[i].String()) - } +// See https://github.com/docker/docker/pull/13590 +func TestChangesWithChangesGH13590(t *testing.T) { + baseLayer, err := ioutil.TempDir("", "docker-changes-test.") + defer os.RemoveAll(baseLayer) + + dir3 := path.Join(baseLayer, "dir1/dir2/dir3") + os.MkdirAll(dir3, 07400) + + file := path.Join(dir3, "file.txt") + ioutil.WriteFile(file, []byte("hello"), 0666) + + layer, err := ioutil.TempDir("", "docker-changes-test2.") + defer os.RemoveAll(layer) + + // Test creating a new file + if err := copyDir(baseLayer+"/dir1", layer+"/"); err != nil { + t.Fatalf("Cmd failed: %q", err) } + + os.Remove(path.Join(layer, "dir1/dir2/dir3/file.txt")) + file = path.Join(layer, "dir1/dir2/dir3/file1.txt") + ioutil.WriteFile(file, []byte("bye"), 0666) + + changes, err := Changes([]string{baseLayer}, layer) + if err != nil { + t.Fatal(err) + } + + expectedChanges := []Change{ + {"/dir1/dir2/dir3", ChangeModify}, + {"/dir1/dir2/dir3/file1.txt", ChangeAdd}, + } + checkChanges(expectedChanges, changes, t) + + // Now test changing a file + layer, err = ioutil.TempDir("", "docker-changes-test3.") + defer os.RemoveAll(layer) + + if err := copyDir(baseLayer+"/dir1", layer+"/"); err != nil { + t.Fatalf("Cmd failed: %q", err) + } + + file = path.Join(layer, "dir1/dir2/dir3/file.txt") + ioutil.WriteFile(file, []byte("bye"), 0666) + + changes, err = Changes([]string{baseLayer}, layer) + if err != nil { + t.Fatal(err) + } + + expectedChanges = []Change{ + {"/dir1/dir2/dir3/file.txt", ChangeModify}, + } + checkChanges(expectedChanges, changes, t) } // Create an directory, copy it, make sure we report no changes between the two @@ -443,3 +471,25 @@ func TestChangesSize(t *testing.T) { t.Fatalf("ChangesSizes with only delete changes should be 0, was %d", size) } } + +func checkChanges(expectedChanges, changes []Change, t *testing.T) { + sort.Sort(changesByPath(expectedChanges)) + sort.Sort(changesByPath(changes)) + for i := 0; i < max(len(changes), len(expectedChanges)); i++ { + if i >= len(expectedChanges) { + t.Fatalf("unexpected change %s\n", changes[i].String()) + } + if i >= len(changes) { + t.Fatalf("no change for expected change %s\n", expectedChanges[i].String()) + } + if changes[i].Path == expectedChanges[i].Path { + if changes[i] != expectedChanges[i] { + t.Fatalf("Wrong change for %s, expected %s, got %s\n", changes[i].Path, changes[i].String(), expectedChanges[i].String()) + } + } else if changes[i].Path < expectedChanges[i].Path { + t.Fatalf("unexpected change %s\n", changes[i].String()) + } else { + t.Fatalf("no change for expected change %s != %s\n", expectedChanges[i].String(), changes[i].String()) + } + } +}