Переглянути джерело

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 <mountkin@gmail.com>
Shijiang Wei 10 роки тому
батько
коміт
e2c6a8be7c
2 змінених файлів з 108 додано та 39 видалено
  1. 20 1
      pkg/archive/changes.go
  2. 88 38
      pkg/archive/changes_test.go

+ 20 - 1
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

+ 88 - 38
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")
+
+	changes, err := Changes([]string{layer}, rwLayer)
 	if err != nil {
 		t.Fatal(err)
 	}
+
+	expectedChanges := []Change{
+		{"/dir1", ChangeModify},
+		{"/dir1/file1-1", ChangeModify},
+		{"/dir1/file1-2", ChangeDelete},
+		{"/dir1/subfolder", ChangeModify},
+		{"/dir1/subfolder/newFile", ChangeAdd},
+	}
+	checkChanges(expectedChanges, changes, t)
+}
+
+// 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)
-	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
+	// Test creating a new file
+	if err := copyDir(baseLayer+"/dir1", layer+"/"); err != nil {
+		t.Fatalf("Cmd failed: %q", err)
 	}
-	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)
+	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{layer}, rwLayer)
+	changes, err := Changes([]string{baseLayer}, layer)
 	if err != nil {
 		t.Fatal(err)
 	}
 
-	sort.Sort(changesByPath(changes))
-
 	expectedChanges := []Change{
-		{"/dir1/file1-1", ChangeModify},
-		{"/dir1/file1-2", ChangeDelete},
-		{"/dir1/subfolder", ChangeModify},
-		{"/dir1/subfolder/newFile", ChangeAdd},
+		{"/dir1/dir2/dir3", ChangeModify},
+		{"/dir1/dir2/dir3/file1.txt", 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())
-		}
+	// 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())
+		}
+	}
+}