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>
This commit is contained in:
parent
6c2626b90e
commit
e10c7b3f07
2 changed files with 110 additions and 41 deletions
|
@ -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,
|
// Changes walks the path rw and determines changes for the files in the path,
|
||||||
// with respect to the parent layers
|
// with respect to the parent layers
|
||||||
func Changes(layers []string, rw string) ([]Change, error) {
|
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 {
|
err := filepath.Walk(rw, func(path string, f os.FileInfo, err error) error {
|
||||||
if err != nil {
|
if err != nil {
|
||||||
return err
|
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
|
// Record change
|
||||||
changes = append(changes, change)
|
changes = append(changes, change)
|
||||||
return nil
|
return nil
|
||||||
|
|
|
@ -6,7 +6,6 @@ import (
|
||||||
"os/exec"
|
"os/exec"
|
||||||
"path"
|
"path"
|
||||||
"sort"
|
"sort"
|
||||||
"syscall"
|
|
||||||
"testing"
|
"testing"
|
||||||
"time"
|
"time"
|
||||||
)
|
)
|
||||||
|
@ -132,12 +131,23 @@ func TestChangesWithNoChanges(t *testing.T) {
|
||||||
}
|
}
|
||||||
|
|
||||||
func TestChangesWithChanges(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")
|
rwLayer, err := ioutil.TempDir("", "docker-changes-test")
|
||||||
if err != nil {
|
if err != nil {
|
||||||
t.Fatal(err)
|
t.Fatal(err)
|
||||||
}
|
}
|
||||||
defer os.RemoveAll(rwLayer)
|
defer os.RemoveAll(rwLayer)
|
||||||
// Create a folder
|
|
||||||
|
// Create a folder in RW layer
|
||||||
dir1 := path.Join(rwLayer, "dir1")
|
dir1 := path.Join(rwLayer, "dir1")
|
||||||
os.MkdirAll(dir1, 0740)
|
os.MkdirAll(dir1, 0740)
|
||||||
deletedFile := path.Join(dir1, ".wh.file1-2")
|
deletedFile := path.Join(dir1, ".wh.file1-2")
|
||||||
|
@ -149,58 +159,76 @@ func TestChangesWithChanges(t *testing.T) {
|
||||||
os.MkdirAll(subfolder, 0740)
|
os.MkdirAll(subfolder, 0740)
|
||||||
newFile := path.Join(subfolder, "newFile")
|
newFile := path.Join(subfolder, "newFile")
|
||||||
ioutil.WriteFile(newFile, []byte{}, 0740)
|
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)
|
changes, err := Changes([]string{layer}, rwLayer)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
t.Fatal(err)
|
t.Fatal(err)
|
||||||
}
|
}
|
||||||
|
|
||||||
sort.Sort(changesByPath(changes))
|
|
||||||
|
|
||||||
expectedChanges := []Change{
|
expectedChanges := []Change{
|
||||||
|
{"/dir1", ChangeModify},
|
||||||
{"/dir1/file1-1", ChangeModify},
|
{"/dir1/file1-1", ChangeModify},
|
||||||
{"/dir1/file1-2", ChangeDelete},
|
{"/dir1/file1-2", ChangeDelete},
|
||||||
{"/dir1/subfolder", ChangeModify},
|
{"/dir1/subfolder", ChangeModify},
|
||||||
{"/dir1/subfolder/newFile", ChangeAdd},
|
{"/dir1/subfolder/newFile", ChangeAdd},
|
||||||
}
|
}
|
||||||
|
checkChanges(expectedChanges, changes, t)
|
||||||
|
}
|
||||||
|
|
||||||
for i := 0; i < max(len(changes), len(expectedChanges)); i++ {
|
// See https://github.com/docker/docker/pull/13590
|
||||||
if i >= len(expectedChanges) {
|
func TestChangesWithChangesGH13590(t *testing.T) {
|
||||||
t.Fatalf("unexpected change %s\n", changes[i].String())
|
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)
|
||||||
}
|
}
|
||||||
if i >= len(changes) {
|
|
||||||
t.Fatalf("no change for expected change %s\n", expectedChanges[i].String())
|
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)
|
||||||
}
|
}
|
||||||
if changes[i].Path == expectedChanges[i].Path {
|
|
||||||
if changes[i] != expectedChanges[i] {
|
expectedChanges := []Change{
|
||||||
t.Fatalf("Wrong change for %s, expected %s, got %s\n", changes[i].Path, changes[i].String(), expectedChanges[i].String())
|
{"/dir1/dir2/dir3", ChangeModify},
|
||||||
|
{"/dir1/dir2/dir3/file1.txt", ChangeAdd},
|
||||||
}
|
}
|
||||||
} else if changes[i].Path < expectedChanges[i].Path {
|
checkChanges(expectedChanges, changes, t)
|
||||||
t.Fatalf("unexpected change %s\n", changes[i].String())
|
|
||||||
} else {
|
// Now test changing a file
|
||||||
t.Fatalf("no change for expected change %s != %s\n", expectedChanges[i].String(), changes[i].String())
|
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
|
// 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)
|
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())
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
Loading…
Reference in a new issue