From 56b732058e4d55aa5559996cce03fe11e6ff3baa Mon Sep 17 00:00:00 2001 From: John Howard Date: Fri, 5 Oct 2018 15:46:59 -0700 Subject: [PATCH] pkg/archive fixes, and port most unit tests to Windows Signed-off-by: John Howard If fixes an error in sameFsTime which was using `==` to compare two times. The correct way is to use go's built-in timea.Equals(timeb). In changes_windows, it uses sameFsTime to compare mTim of a `system.StatT` to allow TestChangesDirsMutated to operate correctly now. Note there is slight different between the Linux and Windows implementations of detecting changes. Due to https://github.com/moby/moby/issues/9874, and the fix at https://github.com/moby/moby/pull/11422, Linux does not consider a change to the directory time as a change. Windows on NTFS does. See https://github.com/moby/moby/pull/37982 for more information. The result in `TestChangesDirsMutated`, `dir3` is NOT considered a change in Linux, but IS considered a change on Windows. The test mutates dir3 to have a mtime of +1 second. With a handful of tests still outstanding, this change ports most of the unit tests under pkg/archive to Windows. It provides an implementation of `copyDir` in tests for Windows. To make a copy similar to Linux's `cp -a` while preserving timestamps and links to both valid and invalid targets, xcopy isn't sufficient. So I used robocopy, but had to circumvent certain exit codes that robocopy exits with which are warnings. Link to article describing this is in the code. --- pkg/archive/archive_test.go | 38 ++--------- pkg/archive/changes.go | 12 ++-- pkg/archive/changes_test.go | 116 +++++++++++++++++++-------------- pkg/archive/changes_unix.go | 8 ++- pkg/archive/changes_windows.go | 8 ++- pkg/archive/diff.go | 10 +-- pkg/archive/diff_test.go | 25 ++----- 7 files changed, 106 insertions(+), 111 deletions(-) diff --git a/pkg/archive/archive_test.go b/pkg/archive/archive_test.go index b448bac49a..affc7fa83b 100644 --- a/pkg/archive/archive_test.go +++ b/pkg/archive/archive_test.go @@ -305,7 +305,7 @@ func TestUntarPathWithInvalidSrc(t *testing.T) { } func TestUntarPath(t *testing.T) { - skip.If(t, os.Getuid() != 0, "skipping test that requires root") + skip.If(t, runtime.GOOS != "windows" && os.Getuid() != 0, "skipping test that requires root") tmpFolder, err := ioutil.TempDir("", "docker-archive-test") assert.NilError(t, err) defer os.RemoveAll(tmpFolder) @@ -436,7 +436,7 @@ func TestCopyWithTarInvalidSrc(t *testing.T) { } func TestCopyWithTarInexistentDestWillCreateIt(t *testing.T) { - skip.If(t, os.Getuid() != 0, "skipping test that requires root") + skip.If(t, runtime.GOOS != "windows" && os.Getuid() != 0, "skipping test that requires root") tempFolder, err := ioutil.TempDir("", "docker-archive-test") if err != nil { t.Fatal(nil) @@ -608,10 +608,6 @@ func TestCopyFileWithTarSrcFile(t *testing.T) { } func TestTarFiles(t *testing.T) { - // TODO Windows: Figure out how to port this test. - if runtime.GOOS == "windows" { - t.Skip("Failing on Windows") - } // try without hardlinks if err := checkNoChanges(1000, false); err != nil { t.Fatal(err) @@ -690,10 +686,6 @@ func tarUntar(t *testing.T, origin string, options *TarOptions) ([]Change, error } func TestTarUntar(t *testing.T) { - // TODO Windows: Figure out how to fix this test. - if runtime.GOOS == "windows" { - t.Skip("Failing on Windows") - } origin, err := ioutil.TempDir("", "docker-test-untar-origin") if err != nil { t.Fatal(err) @@ -722,7 +714,7 @@ func TestTarUntar(t *testing.T) { t.Fatalf("Error tar/untar for compression %s: %s", c.Extension(), err) } - if len(changes) != 1 || changes[0].Path != "/3" { + if len(changes) != 1 || changes[0].Path != string(filepath.Separator)+"3" { t.Fatalf("Unexpected differences after tarUntar: %v", changes) } } @@ -780,10 +772,6 @@ func TestTarWithOptionsChownOptsAlwaysOverridesIdPair(t *testing.T) { } func TestTarWithOptions(t *testing.T) { - // TODO Windows: Figure out how to fix this test. - if runtime.GOOS == "windows" { - t.Skip("Failing on Windows") - } origin, err := ioutil.TempDir("", "docker-test-untar-origin") if err != nil { t.Fatal(err) @@ -942,10 +930,6 @@ func BenchmarkTarUntarWithLinks(b *testing.B) { } func TestUntarInvalidFilenames(t *testing.T) { - // TODO Windows: Figure out how to fix this test. - if runtime.GOOS == "windows" { - t.Skip("Passes but hits breakoutError: platform and architecture is not supported") - } for i, headers := range [][]*tar.Header{ { { @@ -970,9 +954,7 @@ func TestUntarInvalidFilenames(t *testing.T) { } func TestUntarHardlinkToSymlink(t *testing.T) { - // TODO Windows. There may be a way of running this, but turning off for now - skip.If(t, runtime.GOOS == "windows", "hardlinks on Windows") - skip.If(t, os.Getuid() != 0, "skipping test that requires root") + skip.If(t, runtime.GOOS != "windows" && os.Getuid() != 0, "skipping test that requires root") for i, headers := range [][]*tar.Header{ { { @@ -1001,10 +983,6 @@ func TestUntarHardlinkToSymlink(t *testing.T) { } func TestUntarInvalidHardlink(t *testing.T) { - // TODO Windows. There may be a way of running this, but turning off for now - if runtime.GOOS == "windows" { - t.Skip("hardlinks on Windows") - } for i, headers := range [][]*tar.Header{ { // try reading victim/hello (../) { @@ -1085,10 +1063,6 @@ func TestUntarInvalidHardlink(t *testing.T) { } func TestUntarInvalidSymlink(t *testing.T) { - // TODO Windows. There may be a way of running this, but turning off for now - if runtime.GOOS == "windows" { - t.Skip("hardlinks on Windows") - } for i, headers := range [][]*tar.Header{ { // try reading victim/hello (../) { @@ -1254,7 +1228,7 @@ func TestReplaceFileTarWrapper(t *testing.T) { // TestPrefixHeaderReadable tests that files that could be created with the // version of this package that was built with <=go17 are still readable. func TestPrefixHeaderReadable(t *testing.T) { - skip.If(t, os.Getuid() != 0, "skipping test that requires root") + skip.If(t, runtime.GOOS != "windows" && os.Getuid() != 0, "skipping test that requires root") // https://gist.github.com/stevvooe/e2a790ad4e97425896206c0816e1a882#file-out-go var testFile = []byte("\x1f\x8b\x08\x08\x44\x21\x68\x59\x00\x03\x74\x2e\x74\x61\x72\x00\x4b\xcb\xcf\x67\xa0\x35\x30\x80\x00\x86\x06\x10\x47\x01\xc1\x37\x40\x00\x54\xb6\xb1\xa1\xa9\x99\x09\x48\x25\x1d\x40\x69\x71\x49\x62\x91\x02\xe5\x76\xa1\x79\x84\x21\x91\xd6\x80\x72\xaf\x8f\x82\x51\x30\x0a\x46\x36\x00\x00\xf0\x1c\x1e\x95\x00\x06\x00\x00") @@ -1312,7 +1286,7 @@ func appendModifier(path string, header *tar.Header, content io.Reader) (*tar.He } func readFileFromArchive(t *testing.T, archive io.ReadCloser, name string, expectedCount int, doc string) string { - skip.If(t, os.Getuid() != 0, "skipping test that requires root") + skip.If(t, runtime.GOOS != "windows" && os.Getuid() != 0, "skipping test that requires root") destDir, err := ioutil.TempDir("", "docker-test-destDir") assert.NilError(t, err) defer os.RemoveAll(destDir) diff --git a/pkg/archive/changes.go b/pkg/archive/changes.go index 43734db5b1..aedb91b035 100644 --- a/pkg/archive/changes.go +++ b/pkg/archive/changes.go @@ -63,12 +63,16 @@ func (c changesByPath) Less(i, j int) bool { return c[i].Path < c[j].Path } func (c changesByPath) Len() int { return len(c) } func (c changesByPath) Swap(i, j int) { c[j], c[i] = c[i], c[j] } -// Gnu tar and the go tar writer don't have sub-second mtime -// precision, which is problematic when we apply changes via tar -// files, we handle this by comparing for exact times, *or* same +// Gnu tar doesn't have sub-second mtime precision. The go tar +// writer (1.10+) does when using PAX format, but we round times to seconds +// to ensure archives have the same hashes for backwards compatibility. +// See https://github.com/moby/moby/pull/35739/commits/fb170206ba12752214630b269a40ac7be6115ed4. +// +// Non-sub-second is problematic when we apply changes via tar +// files. We handle this by comparing for exact times, *or* same // second count and either a or b having exactly 0 nanoseconds func sameFsTime(a, b time.Time) bool { - return a == b || + return a.Equal(b) || (a.Unix() == b.Unix() && (a.Nanosecond() == 0 || b.Nanosecond() == 0)) } diff --git a/pkg/archive/changes_test.go b/pkg/archive/changes_test.go index f2527cd936..a5c00b87c4 100644 --- a/pkg/archive/changes_test.go +++ b/pkg/archive/changes_test.go @@ -5,8 +5,10 @@ import ( "os" "os/exec" "path" + "path/filepath" "runtime" "sort" + "syscall" "testing" "time" @@ -23,7 +25,24 @@ func max(x, y int) int { } func copyDir(src, dst string) error { - return exec.Command("cp", "-a", src, dst).Run() + if runtime.GOOS != "windows" { + return exec.Command("cp", "-a", src, dst).Run() + } + + // Could have used xcopy src dst /E /I /H /Y /B. However, xcopy has the + // unfortunate side effect of not preserving timestamps of newly created + // directories in the target directory, so we don't get accurate changes. + // Use robocopy instead. Note this isn't available in microsoft/nanoserver. + // But it has gotchas. See https://weblogs.sqlteam.com/robv/archive/2010/02/17/61106.aspx + err := exec.Command("robocopy", filepath.FromSlash(src), filepath.FromSlash(dst), "/SL", "/COPYALL", "/MIR").Run() + if exiterr, ok := err.(*exec.ExitError); ok { + if status, ok := exiterr.Sys().(syscall.WaitStatus); ok { + if status.ExitStatus()&24 == 0 { + return nil + } + } + } + return err } type FileType uint32 @@ -113,11 +132,6 @@ func TestChangeString(t *testing.T) { } func TestChangesWithNoChanges(t *testing.T) { - // TODO Windows. There may be a way of running this, but turning off for now - // as createSampleDir uses symlinks. - if runtime.GOOS == "windows" { - t.Skip("symlinks on Windows") - } rwLayer, err := ioutil.TempDir("", "docker-changes-test") assert.NilError(t, err) defer os.RemoveAll(rwLayer) @@ -133,11 +147,6 @@ func TestChangesWithNoChanges(t *testing.T) { } func TestChangesWithChanges(t *testing.T) { - // TODO Windows. There may be a way of running this, but turning off for now - // as createSampleDir uses symlinks. - if runtime.GOOS == "windows" { - t.Skip("symlinks on Windows") - } // Mock the readonly layer layer, err := ioutil.TempDir("", "docker-changes-test-layer") assert.NilError(t, err) @@ -167,21 +176,20 @@ func TestChangesWithChanges(t *testing.T) { assert.NilError(t, err) expectedChanges := []Change{ - {"/dir1", ChangeModify}, - {"/dir1/file1-1", ChangeModify}, - {"/dir1/file1-2", ChangeDelete}, - {"/dir1/subfolder", ChangeModify}, - {"/dir1/subfolder/newFile", ChangeAdd}, + {filepath.FromSlash("/dir1"), ChangeModify}, + {filepath.FromSlash("/dir1/file1-1"), ChangeModify}, + {filepath.FromSlash("/dir1/file1-2"), ChangeDelete}, + {filepath.FromSlash("/dir1/subfolder"), ChangeModify}, + {filepath.FromSlash("/dir1/subfolder/newFile"), ChangeAdd}, } checkChanges(expectedChanges, changes, t) } // See https://github.com/docker/docker/pull/13590 func TestChangesWithChangesGH13590(t *testing.T) { - // TODO Windows. There may be a way of running this, but turning off for now - // as createSampleDir uses symlinks. + // TODO Windows. Needs further investigation to identify the failure if runtime.GOOS == "windows" { - t.Skip("symlinks on Windows") + t.Skip("needs more investigation") } baseLayer, err := ioutil.TempDir("", "docker-changes-test.") assert.NilError(t, err) @@ -238,11 +246,6 @@ func TestChangesWithChangesGH13590(t *testing.T) { // Create a directory, copy it, make sure we report no changes between the two func TestChangesDirsEmpty(t *testing.T) { - // TODO Windows. There may be a way of running this, but turning off for now - // as createSampleDir uses symlinks. - if runtime.GOOS == "windows" { - t.Skip("symlinks on Windows") - } src, err := ioutil.TempDir("", "docker-changes-test") assert.NilError(t, err) defer os.RemoveAll(src) @@ -325,11 +328,6 @@ func mutateSampleDir(t *testing.T, root string) { } func TestChangesDirsMutated(t *testing.T) { - // TODO Windows. There may be a way of running this, but turning off for now - // as createSampleDir uses symlinks. - if runtime.GOOS == "windows" { - t.Skip("symlinks on Windows") - } src, err := ioutil.TempDir("", "docker-changes-test") assert.NilError(t, err) createSampleDir(t, src) @@ -347,20 +345,37 @@ func TestChangesDirsMutated(t *testing.T) { sort.Sort(changesByPath(changes)) expectedChanges := []Change{ - {"/dir1", ChangeDelete}, - {"/dir2", ChangeModify}, - {"/dirnew", ChangeAdd}, - {"/file1", ChangeDelete}, - {"/file2", ChangeModify}, - {"/file3", ChangeModify}, - {"/file4", ChangeModify}, - {"/file5", ChangeModify}, - {"/filenew", ChangeAdd}, - {"/symlink1", ChangeDelete}, - {"/symlink2", ChangeModify}, - {"/symlinknew", ChangeAdd}, + {filepath.FromSlash("/dir1"), ChangeDelete}, + {filepath.FromSlash("/dir2"), ChangeModify}, } + // Note there is slight difference between the Linux and Windows + // implementations here. Due to https://github.com/moby/moby/issues/9874, + // and the fix at https://github.com/moby/moby/pull/11422, Linux does not + // consider a change to the directory time as a change. Windows on NTFS + // does. See https://github.com/moby/moby/pull/37982 for more information. + // + // Note also: https://github.com/moby/moby/pull/37982#discussion_r223523114 + // that differences are ordered in the way the test is currently written, hence + // this is in the middle of the list of changes rather than at the start or + // end. Potentially can be addressed later. + if runtime.GOOS == "windows" { + expectedChanges = append(expectedChanges, Change{filepath.FromSlash("/dir3"), ChangeModify}) + } + + expectedChanges = append(expectedChanges, []Change{ + {filepath.FromSlash("/dirnew"), ChangeAdd}, + {filepath.FromSlash("/file1"), ChangeDelete}, + {filepath.FromSlash("/file2"), ChangeModify}, + {filepath.FromSlash("/file3"), ChangeModify}, + {filepath.FromSlash("/file4"), ChangeModify}, + {filepath.FromSlash("/file5"), ChangeModify}, + {filepath.FromSlash("/filenew"), ChangeAdd}, + {filepath.FromSlash("/symlink1"), ChangeDelete}, + {filepath.FromSlash("/symlink2"), ChangeModify}, + {filepath.FromSlash("/symlinknew"), ChangeAdd}, + }...) + for i := 0; i < max(len(changes), len(expectedChanges)); i++ { if i >= len(expectedChanges) { t.Fatalf("unexpected change %s\n", changes[i].String()) @@ -373,7 +388,7 @@ func TestChangesDirsMutated(t *testing.T) { 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()) + t.Fatalf("unexpected change %q %q\n", changes[i].String(), expectedChanges[i].Path) } else { t.Fatalf("no change for expected change %s != %s\n", expectedChanges[i].String(), changes[i].String()) } @@ -381,10 +396,13 @@ func TestChangesDirsMutated(t *testing.T) { } func TestApplyLayer(t *testing.T) { - // TODO Windows. There may be a way of running this, but turning off for now - // as createSampleDir uses symlinks. + // TODO Windows. This is very close to working, but it fails with changes + // to \symlinknew and \symlink2. The destination has an updated + // Access/Modify/Change/Birth date to the source (~3/100th sec different). + // Needs further investigation as to why, but I currently believe this is + // just the way NTFS works. I don't think it's a bug in this test or archive. if runtime.GOOS == "windows" { - t.Skip("symlinks on Windows") + t.Skip("needs further investigation") } src, err := ioutil.TempDir("", "docker-changes-test") assert.NilError(t, err) @@ -417,10 +435,10 @@ func TestApplyLayer(t *testing.T) { } func TestChangesSizeWithHardlinks(t *testing.T) { - // TODO Windows. There may be a way of running this, but turning off for now - // as createSampleDir uses symlinks. + // TODO Windows. Needs further investigation. Likely in ChangeSizes not + // coping correctly with hardlinks on Windows. if runtime.GOOS == "windows" { - t.Skip("hardlinks on Windows") + t.Skip("needs further investigation") } srcDir, err := ioutil.TempDir("", "docker-test-srcDir") assert.NilError(t, err) @@ -481,7 +499,7 @@ func TestChangesSize(t *testing.T) { } func checkChanges(expectedChanges, changes []Change, t *testing.T) { - skip.If(t, os.Getuid() != 0, "skipping test that requires root") + skip.If(t, runtime.GOOS != "windows" && os.Getuid() != 0, "skipping test that requires root") sort.Sort(changesByPath(expectedChanges)) sort.Sort(changesByPath(changes)) for i := 0; i < max(len(changes), len(expectedChanges)); i++ { diff --git a/pkg/archive/changes_unix.go b/pkg/archive/changes_unix.go index c06a209d8e..06217b7161 100644 --- a/pkg/archive/changes_unix.go +++ b/pkg/archive/changes_unix.go @@ -16,7 +16,13 @@ func statDifferent(oldStat *system.StatT, newStat *system.StatT) bool { oldStat.UID() != newStat.UID() || oldStat.GID() != newStat.GID() || oldStat.Rdev() != newStat.Rdev() || - // Don't look at size for dirs, its not a good measure of change + // Don't look at size or modification time for dirs, its not a good + // measure of change. See https://github.com/moby/moby/issues/9874 + // for a description of the issue with modification time, and + // https://github.com/moby/moby/pull/11422 for the change. + // (Note that in the Windows implementation of this function, + // modification time IS taken as a change). See + // https://github.com/moby/moby/pull/37982 for more information. (oldStat.Mode()&unix.S_IFDIR != unix.S_IFDIR && (!sameFsTimeSpec(oldStat.Mtim(), newStat.Mtim()) || (oldStat.Size() != newStat.Size()))) { return true diff --git a/pkg/archive/changes_windows.go b/pkg/archive/changes_windows.go index 6555c01368..9906685e4b 100644 --- a/pkg/archive/changes_windows.go +++ b/pkg/archive/changes_windows.go @@ -7,9 +7,13 @@ import ( ) func statDifferent(oldStat *system.StatT, newStat *system.StatT) bool { + // Note there is slight difference between the Linux and Windows + // implementations here. Due to https://github.com/moby/moby/issues/9874, + // and the fix at https://github.com/moby/moby/pull/11422, Linux does not + // consider a change to the directory time as a change. Windows on NTFS + // does. See https://github.com/moby/moby/pull/37982 for more information. - // Don't look at size for dirs, its not a good measure of change - if oldStat.Mtim() != newStat.Mtim() || + if !sameFsTime(oldStat.Mtim(), newStat.Mtim()) || oldStat.Mode() != newStat.Mode() || oldStat.Size() != newStat.Size() && !oldStat.Mode().IsDir() { return true diff --git a/pkg/archive/diff.go b/pkg/archive/diff.go index 6883ba1326..146e21fe18 100644 --- a/pkg/archive/diff.go +++ b/pkg/archive/diff.go @@ -240,11 +240,13 @@ func applyLayerHandler(dest string, layer io.Reader, options *TarOptions, decomp dest = filepath.Clean(dest) // We need to be able to set any perms - oldmask, err := system.Umask(0) - if err != nil { - return 0, err + if runtime.GOOS != "windows" { + oldmask, err := system.Umask(0) + if err != nil { + return 0, err + } + defer system.Umask(oldmask) } - defer system.Umask(oldmask) // ignore err, ErrNotSupportedPlatform if decompress { decompLayer, err := DecompressStream(layer) diff --git a/pkg/archive/diff_test.go b/pkg/archive/diff_test.go index 19f2555e1a..1d3f166127 100644 --- a/pkg/archive/diff_test.go +++ b/pkg/archive/diff_test.go @@ -7,17 +7,12 @@ import ( "os" "path/filepath" "reflect" - "runtime" "testing" "github.com/docker/docker/pkg/ioutils" ) func TestApplyLayerInvalidFilenames(t *testing.T) { - // TODO Windows: Figure out how to fix this test. - if runtime.GOOS == "windows" { - t.Skip("Passes but hits breakoutError: platform and architecture is not supported") - } for i, headers := range [][]*tar.Header{ { { @@ -42,9 +37,6 @@ func TestApplyLayerInvalidFilenames(t *testing.T) { } func TestApplyLayerInvalidHardlink(t *testing.T) { - if runtime.GOOS == "windows" { - t.Skip("TypeLink support on Windows") - } for i, headers := range [][]*tar.Header{ { // try reading victim/hello (../) { @@ -125,9 +117,6 @@ func TestApplyLayerInvalidHardlink(t *testing.T) { } func TestApplyLayerInvalidSymlink(t *testing.T) { - if runtime.GOOS == "windows" { - t.Skip("TypeSymLink support on Windows") - } for i, headers := range [][]*tar.Header{ { // try reading victim/hello (../) { @@ -208,11 +197,6 @@ func TestApplyLayerInvalidSymlink(t *testing.T) { } func TestApplyLayerWhiteouts(t *testing.T) { - // TODO Windows: Figure out why this test fails - if runtime.GOOS == "windows" { - t.Skip("Failing on Windows") - } - wd, err := ioutil.TempDir("", "graphdriver-test-whiteouts") if err != nil { return @@ -339,7 +323,9 @@ func makeTestLayer(paths []string) (rc io.ReadCloser, err error) { } }() for _, p := range paths { - if p[len(p)-1] == filepath.Separator { + // Source files are always in Unix format. But we use filepath on + // creation to be platform agnostic. + if p[len(p)-1] == '/' { if err = os.MkdirAll(filepath.Join(tmpDir, p), 0700); err != nil { return } @@ -374,9 +360,10 @@ func readDirContents(root string) ([]string, error) { return err } if info.IsDir() { - rel = rel + "/" + rel = rel + string(filepath.Separator) } - files = append(files, rel) + // Append in Unix semantics + files = append(files, filepath.ToSlash(rel)) return nil }) if err != nil {