From 750771f6d0f6e067b8c6b22e3a47a38944c73613 Mon Sep 17 00:00:00 2001 From: Derek McGowan Date: Tue, 22 Aug 2017 10:35:20 -0700 Subject: [PATCH] Remove snapshot test suite as a parallel test runner The testsuite alters global state by setting the umask, avoid running the testsuite in parallel and move umask manipulation to the test suite level to individual tests may run in parallel. Added better error messaging and handling. Removed reliance on testing object for handling cleanup failure. When a cleanup error occurred, it would fail the test but the log would get skipped. With this change the failure will show up for the running test. Update test unmounting to set the detach flag, avoiding races with btrfs which may see the device as busy when attempting to unmount. Signed-off-by: Derek McGowan --- container_test.go | 1 - metadata/snapshot_test.go | 6 ++--- snapshot/btrfs/btrfs_test.go | 16 ++++++----- snapshot/naive/naive_test.go | 4 +-- snapshot/overlay/overlay_test.go | 4 +-- snapshot/testsuite/helpers.go | 24 ++++++++++------- snapshot/testsuite/issues.go | 8 ------ snapshot/testsuite/testsuite.go | 36 ++++++++++--------------- snapshot/testsuite/testsuite_unix.go | 8 +++++- snapshot/testsuite/testsuite_windows.go | 2 ++ snapshot_test.go | 6 ++--- testutil/helpers_unix.go | 3 ++- 12 files changed, 59 insertions(+), 59 deletions(-) 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) } }