beenull/moby
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity

pkg/archive fixes, and port most unit tests to Windows

Browse source 
Signed-off-by: John Howard <jhoward@microsoft.com>

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.
This commit is contained in:
John Howard 2018-10-05 15:46:59 -07:00
parent c77cfbfef5
commit 56b732058e
7 changed files with 106 additions and 111 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

38
pkg/archive/archive_test.go
View file

@ -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)

12
pkg/archive/changes.go
View file

@ -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))
}

116
pkg/archive/changes_test.go
View file

@ -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++ {

8
pkg/archive/changes_unix.go
View file

@ -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

8
pkg/archive/changes_windows.go
View file

@ -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

10
pkg/archive/diff.go
View file

@ -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)

25
pkg/archive/diff_test.go
View file

@ -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 {