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 <derek@mcgstyle.net>

container_test.go

@@ -2,7 +2,6 @@ package containerd
 
 import (
 	"bytes"
-	"fmt"
 	"io/ioutil"
 	"os"
 	"runtime"

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() {
+	return sn, func() error {
-		db.Close()
+		return db.Close()
 	}, nil
 }
 

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() {
+		return snapshotter, func() (err error) {
-			testutil.Unmount(t, root)
+			merr := mount.UnmountAll(root, unix.MNT_DETACH)
-			if err := cleanupDevice(); err != nil {
+			if err = cleanupDevice(); err != nil {
-				t.Errorf("Device cleanup failed: %v", err)
+				return errors.Wrap(err, "device cleanup failed")
+			} else {
+				err = merr
 			}
+			return err
 		}, nil
 	}
 }

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

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

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 {
+		if err1 := mount.UnmountAll(td, umountflags); err == nil {
-			err = err1
+			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")
 		}
 	}()

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

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)) {
+func SnapshotterSuite(t *testing.T, name string, snapshotterFn func(ctx context.Context, root string) (snapshot.Snapshotter, func() error, error)) {
-	t.Parallel()
+	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)
 	}

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)

snapshot/testsuite/testsuite_windows.go

@@ -1,5 +1,7 @@
 package testsuite
 
+const umountflags int = 0
+
 func clearMask() func() {
 	return func() {}
 }

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() {
+	return sn, func() error {
-		client.Close()
+		return client.Close()
 	}, nil
 }
 

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