diff --git a/container_test.go b/container_test.go index a2627114c..0c0d8416c 100644 --- a/container_test.go +++ b/container_test.go @@ -2,7 +2,6 @@ package containerd import ( "bytes" - "fmt" "io/ioutil" "os" "runtime" diff --git a/metadata/snapshot_test.go b/metadata/snapshot_test.go index 573d3b83d..37800dc40 100644 --- a/metadata/snapshot_test.go +++ b/metadata/snapshot_test.go @@ -13,7 +13,7 @@ import ( "github.com/containerd/containerd/testutil" ) -func newSnapshotter(ctx context.Context, root string) (snapshot.Snapshotter, func(), error) { +func newSnapshotter(ctx context.Context, root string) (snapshot.Snapshotter, func() error, error) { naiveRoot := filepath.Join(root, "naive") if err := os.Mkdir(naiveRoot, 0770); err != nil { return nil, nil, err @@ -30,8 +30,8 @@ func newSnapshotter(ctx context.Context, root string) (snapshot.Snapshotter, fun sn := NewSnapshotter(db, "naive", snapshotter) - return sn, func() { - db.Close() + return sn, func() error { + return db.Close() }, nil } diff --git a/snapshot/btrfs/btrfs_test.go b/snapshot/btrfs/btrfs_test.go index 738da0048..83fd7d54d 100644 --- a/snapshot/btrfs/btrfs_test.go +++ b/snapshot/btrfs/btrfs_test.go @@ -16,9 +16,10 @@ import ( "github.com/containerd/containerd/snapshot/testsuite" "github.com/containerd/containerd/testutil" "github.com/pkg/errors" + "golang.org/x/sys/unix" ) -func boltSnapshotter(t *testing.T) func(context.Context, string) (snapshot.Snapshotter, func(), error) { +func boltSnapshotter(t *testing.T) func(context.Context, string) (snapshot.Snapshotter, func() error, error) { mkbtrfs, err := exec.LookPath("mkfs.btrfs") if err != nil { t.Skipf("could not find mkfs.btrfs: %v", err) @@ -26,7 +27,7 @@ func boltSnapshotter(t *testing.T) func(context.Context, string) (snapshot.Snaps // TODO: Check for btrfs in /proc/module and skip if not loaded - return func(ctx context.Context, root string) (snapshot.Snapshotter, func(), error) { + return func(ctx context.Context, root string) (snapshot.Snapshotter, func() error, error) { deviceName, cleanupDevice, err := testutil.NewLoopback(100 << 20) // 100 MB if err != nil { @@ -45,11 +46,14 @@ func boltSnapshotter(t *testing.T) func(context.Context, string) (snapshot.Snaps return nil, nil, errors.Wrap(err, "failed to create new snapshotter") } - return snapshotter, func() { - testutil.Unmount(t, root) - if err := cleanupDevice(); err != nil { - t.Errorf("Device cleanup failed: %v", err) + return snapshotter, func() (err error) { + merr := mount.UnmountAll(root, unix.MNT_DETACH) + if err = cleanupDevice(); err != nil { + return errors.Wrap(err, "device cleanup failed") + } else { + err = merr } + return err }, nil } } diff --git a/snapshot/naive/naive_test.go b/snapshot/naive/naive_test.go index 67ac2cc4f..279e95505 100644 --- a/snapshot/naive/naive_test.go +++ b/snapshot/naive/naive_test.go @@ -9,13 +9,13 @@ import ( "github.com/containerd/containerd/testutil" ) -func newSnapshotter(ctx context.Context, root string) (snapshot.Snapshotter, func(), error) { +func newSnapshotter(ctx context.Context, root string) (snapshot.Snapshotter, func() error, error) { snapshotter, err := NewSnapshotter(root) if err != nil { return nil, nil, err } - return snapshotter, func() {}, nil + return snapshotter, nil, nil } func TestNaive(t *testing.T) { diff --git a/snapshot/overlay/overlay_test.go b/snapshot/overlay/overlay_test.go index 5b47f07ae..870b31a3f 100644 --- a/snapshot/overlay/overlay_test.go +++ b/snapshot/overlay/overlay_test.go @@ -18,13 +18,13 @@ import ( "github.com/containerd/containerd/testutil" ) -func newSnapshotter(ctx context.Context, root string) (snapshot.Snapshotter, func(), error) { +func newSnapshotter(ctx context.Context, root string) (snapshot.Snapshotter, func() error, error) { snapshotter, err := NewSnapshotter(root) if err != nil { return nil, nil, err } - return snapshotter, func() {}, nil + return snapshotter, nil, nil } func TestOverlay(t *testing.T) { diff --git a/snapshot/testsuite/helpers.go b/snapshot/testsuite/helpers.go index a0be722d1..8d4acb904 100644 --- a/snapshot/testsuite/helpers.go +++ b/snapshot/testsuite/helpers.go @@ -21,11 +21,11 @@ func applyToMounts(m []mount.Mount, work string, a fstest.Applier) (err error) { defer os.RemoveAll(td) if err := mount.MountAll(m, td); err != nil { - return err + return errors.Wrap(err, "failed to mount") } defer func() { - if err1 := mount.UnmountAll(td, 0); err == nil { - err = err1 + if err1 := mount.UnmountAll(td, umountflags); err == nil { + err = errors.Wrap(err1, "failed to unmount") } }() @@ -40,26 +40,30 @@ func createSnapshot(ctx context.Context, sn snapshot.Snapshotter, parent, work s m, err := sn.Prepare(ctx, prepare, parent) if err != nil { - return "", err + return "", errors.Wrap(err, "failed to prepare snapshot") } if err := applyToMounts(m, work, a); err != nil { - return "", err + return "", errors.Wrap(err, "failed to apply") } if err := sn.Commit(ctx, n, prepare); err != nil { - return "", err + return "", errors.Wrap(err, "failed to commit") } return n, nil } -func checkSnapshot(ctx context.Context, sn snapshot.Snapshotter, work, name, check string) error { +func checkSnapshot(ctx context.Context, sn snapshot.Snapshotter, work, name, check string) (err error) { td, err := ioutil.TempDir(work, "check") if err != nil { return errors.Wrap(err, "failed to create temp dir") } - defer os.RemoveAll(td) + defer func() { + if err1 := os.RemoveAll(td); err == nil { + err = errors.Wrapf(err1, "failed to remove temporary directory %s", td) + } + }() view := fmt.Sprintf("%s-view", name) m, err := sn.View(ctx, view, name) @@ -73,10 +77,10 @@ func checkSnapshot(ctx context.Context, sn snapshot.Snapshotter, work, name, che }() if err := mount.MountAll(m, td); err != nil { - return errors.Wrap(err, "failed to unmount") + return errors.Wrap(err, "failed to mount") } defer func() { - if err1 := mount.UnmountAll(td, 0); err == nil { + if err1 := mount.UnmountAll(td, umountflags); err == nil { err = errors.Wrap(err1, "failed to unmount view") } }() diff --git a/snapshot/testsuite/issues.go b/snapshot/testsuite/issues.go index 84b7a5e01..a41c401f0 100644 --- a/snapshot/testsuite/issues.go +++ b/snapshot/testsuite/issues.go @@ -22,8 +22,6 @@ import ( // avoid such issues by not relying on tar to create layers. // See https://github.com/docker/docker/issues/21555 func checkLayerFileUpdate(ctx context.Context, t *testing.T, sn snapshot.Snapshotter, work string) { - t.Parallel() - l1Init := fstest.Apply( fstest.CreateDir("/etc", 0700), fstest.CreateFile("/etc/hosts", []byte("mydomain 10.0.0.1"), 0644), @@ -55,8 +53,6 @@ func checkLayerFileUpdate(ctx context.Context, t *testing.T, sn snapshot.Snapsho // checkRemoveDirectoryInLowerLayer // See https://github.com/docker/docker/issues/25244 func checkRemoveDirectoryInLowerLayer(ctx context.Context, t *testing.T, sn snapshot.Snapshotter, work string) { - t.Parallel() - l1Init := fstest.Apply( fstest.CreateDir("/lib", 0700), fstest.CreateFile("/lib/hidden", []byte{}, 0644), @@ -80,8 +76,6 @@ func checkRemoveDirectoryInLowerLayer(ctx context.Context, t *testing.T, sn snap // See https://github.com/docker/docker/issues/24913 overlay // see https://github.com/docker/docker/issues/28391 overlay2 func checkChown(ctx context.Context, t *testing.T, sn snapshot.Snapshotter, work string) { - t.Parallel() - l1Init := fstest.Apply( fstest.CreateDir("/opt", 0700), fstest.CreateDir("/opt/a", 0700), @@ -124,8 +118,6 @@ func checkRename(ctx context.Context, t *testing.T, sn snapshot.Snapshotter, wor // checkDirectoryPermissionOnCommit // https://github.com/docker/docker/issues/27298 func checkDirectoryPermissionOnCommit(ctx context.Context, t *testing.T, sn snapshot.Snapshotter, work string) { - t.Parallel() - l1Init := fstest.Apply( fstest.CreateDir("/dir1", 0700), fstest.CreateDir("/dir2", 0700), diff --git a/snapshot/testsuite/testsuite.go b/snapshot/testsuite/testsuite.go index 467b30104..df934c7f8 100644 --- a/snapshot/testsuite/testsuite.go +++ b/snapshot/testsuite/testsuite.go @@ -19,8 +19,9 @@ import ( ) // SnapshotterSuite runs a test suite on the snapshotter given a factory function. -func SnapshotterSuite(t *testing.T, name string, snapshotterFn func(ctx context.Context, root string) (snapshot.Snapshotter, func(), error)) { - t.Parallel() +func SnapshotterSuite(t *testing.T, name string, snapshotterFn func(ctx context.Context, root string) (snapshot.Snapshotter, func() error, error)) { + restoreMask := clearMask() + defer restoreMask() t.Run("Basic", makeTest(name, snapshotterFn, checkSnapshotterBasic)) t.Run("StatActive", makeTest(name, snapshotterFn, checkSnapshotterStatActive)) @@ -39,12 +40,12 @@ func SnapshotterSuite(t *testing.T, name string, snapshotterFn func(ctx context. //t.Run("Rename", makeTest(name, snapshotterFn, checkRename)) } -func makeTest(name string, snapshotterFn func(ctx context.Context, root string) (snapshot.Snapshotter, func(), error), fn func(ctx context.Context, t *testing.T, snapshotter snapshot.Snapshotter, work string)) func(t *testing.T) { +func makeTest(name string, snapshotterFn func(ctx context.Context, root string) (snapshot.Snapshotter, func() error, error), fn func(ctx context.Context, t *testing.T, snapshotter snapshot.Snapshotter, work string)) func(t *testing.T) { return func(t *testing.T) { + t.Parallel() + ctx := context.Background() ctx = namespaces.WithNamespace(ctx, "testsuite") - restoreMask := clearMask() - defer restoreMask() // Make two directories: a snapshotter root and a play area for the tests: // // /tmp @@ -64,9 +65,15 @@ func makeTest(name string, snapshotterFn func(ctx context.Context, root string) snapshotter, cleanup, err := snapshotterFn(ctx, root) if err != nil { - t.Fatal(err) + t.Fatalf("Failed to initialize snapshotter: %+v", err) } - defer cleanup() + defer func() { + if cleanup != nil { + if err := cleanup(); err != nil { + t.Errorf("Cleanup failed: %v", err) + } + } + }() work := filepath.Join(tmpDir, "work") if err := os.MkdirAll(work, 0777); err != nil { @@ -80,9 +87,6 @@ func makeTest(name string, snapshotterFn func(ctx context.Context, root string) // checkSnapshotterBasic tests the basic workflow of a snapshot snapshotter. func checkSnapshotterBasic(ctx context.Context, t *testing.T, snapshotter snapshot.Snapshotter, work string) { - // TODO: this always fails when run in parallel, why? - // t.Parallel() - initialApplier := fstest.Apply( fstest.CreateFile("/foo", []byte("foo\n"), 0777), fstest.CreateDir("/a", 0755), @@ -216,8 +220,6 @@ func checkSnapshotterBasic(ctx context.Context, t *testing.T, snapshotter snapsh // Create a New Layer on top of base layer with Prepare, Stat on new layer, should return Active layer. func checkSnapshotterStatActive(ctx context.Context, t *testing.T, snapshotter snapshot.Snapshotter, work string) { - t.Parallel() - preparing := filepath.Join(work, "preparing") if err := os.MkdirAll(preparing, 0777); err != nil { t.Fatal(err) @@ -252,8 +254,6 @@ func checkSnapshotterStatActive(ctx context.Context, t *testing.T, snapshotter s // Commit a New Layer on top of base layer with Prepare & Commit , Stat on new layer, should return Committed layer. func checkSnapshotterStatCommitted(ctx context.Context, t *testing.T, snapshotter snapshot.Snapshotter, work string) { - t.Parallel() - preparing := filepath.Join(work, "preparing") if err := os.MkdirAll(preparing, 0777); err != nil { t.Fatal(err) @@ -315,8 +315,6 @@ func snapshotterPrepareMount(ctx context.Context, snapshotter snapshot.Snapshott // Given A <- B <- C, B is the parent of C and A is a transitive parent of C (in this case, a "grandparent") func checkSnapshotterTransitivity(ctx context.Context, t *testing.T, snapshotter snapshot.Snapshotter, work string) { - t.Parallel() - preparing, err := snapshotterPrepareMount(ctx, snapshotter, "preparing", "", work) if err != nil { t.Fatal(err) @@ -371,8 +369,6 @@ func checkSnapshotterTransitivity(ctx context.Context, t *testing.T, snapshotter // Creating two layers with Prepare or View with same key must fail. func checkSnapshotterPrepareView(ctx context.Context, t *testing.T, snapshotter snapshot.Snapshotter, work string) { - t.Parallel() - preparing, err := snapshotterPrepareMount(ctx, snapshotter, "preparing", "", work) if err != nil { t.Fatal(err) @@ -469,8 +465,6 @@ func baseTestSnapshots(ctx context.Context, snapshotter snapshot.Snapshotter) er } func checkUpdate(ctx context.Context, t *testing.T, snapshotter snapshot.Snapshotter, work string) { - t.Parallel() - t1 := time.Now().UTC() if err := baseTestSnapshots(ctx, snapshotter); err != nil { t.Fatalf("Failed to create base snapshots: %v", err) @@ -622,8 +616,6 @@ func assertLabels(t *testing.T, actual, expected map[string]string) { } func checkRemove(ctx context.Context, t *testing.T, snapshotter snapshot.Snapshotter, work string) { - t.Parallel() - if _, err := snapshotter.Prepare(ctx, "committed-a", ""); err != nil { t.Fatal(err) } diff --git a/snapshot/testsuite/testsuite_unix.go b/snapshot/testsuite/testsuite_unix.go index 6a02eda9d..14dc85864 100644 --- a/snapshot/testsuite/testsuite_unix.go +++ b/snapshot/testsuite/testsuite_unix.go @@ -2,7 +2,13 @@ package testsuite -import "syscall" +import ( + "syscall" + + "golang.org/x/sys/unix" +) + +const umountflags int = unix.MNT_DETACH func clearMask() func() { oldumask := syscall.Umask(0) diff --git a/snapshot/testsuite/testsuite_windows.go b/snapshot/testsuite/testsuite_windows.go index c3cc8c22b..c48cca3e6 100644 --- a/snapshot/testsuite/testsuite_windows.go +++ b/snapshot/testsuite/testsuite_windows.go @@ -1,5 +1,7 @@ package testsuite +const umountflags int = 0 + func clearMask() func() { return func() {} } diff --git a/snapshot_test.go b/snapshot_test.go index ccf28c70c..0bcab7876 100644 --- a/snapshot_test.go +++ b/snapshot_test.go @@ -9,7 +9,7 @@ import ( "github.com/containerd/containerd/snapshot/testsuite" ) -func newSnapshotter(ctx context.Context, root string) (snapshot.Snapshotter, func(), error) { +func newSnapshotter(ctx context.Context, root string) (snapshot.Snapshotter, func() error, error) { client, err := New(address) if err != nil { return nil, nil, err @@ -17,8 +17,8 @@ func newSnapshotter(ctx context.Context, root string) (snapshot.Snapshotter, fun sn := client.SnapshotService(DefaultSnapshotter) - return sn, func() { - client.Close() + return sn, func() error { + return client.Close() }, nil } diff --git a/testutil/helpers_unix.go b/testutil/helpers_unix.go index 398209df1..fd207449e 100644 --- a/testutil/helpers_unix.go +++ b/testutil/helpers_unix.go @@ -9,12 +9,13 @@ import ( "github.com/containerd/containerd/mount" "github.com/stretchr/testify/assert" + "golang.org/x/sys/unix" ) // Unmount unmounts a given mountPoint and sets t.Error if it fails func Unmount(t *testing.T, mountPoint string) { t.Log("unmount", mountPoint) - if err := mount.Unmount(mountPoint, 0); err != nil { + if err := mount.UnmountAll(mountPoint, unix.MNT_DETACH); err != nil { t.Error("Could not umount", mountPoint, err) } }