Merge pull request #1341 from mlaventure/enable-test-parallelism

Enable test parallelism

.appveyor.yml

@@ -29,6 +29,7 @@ test_script:
   # TODO: need an equivalent of TRAVIS_COMMIT_RANGE
   # - GIT_CHECK_EXCLUDE="./vendor" TRAVIS_COMMIT_RANGE="${TRAVIS_COMMIT_RANGE/.../..}" C:\MinGW\bin\mingw32-make.exe dco
   - bash.exe -elc "export PATH=/c/tools/mingw64/bin:/c/gopath/src/github.com/containerd/containerd/bin:$PATH ; mingw32-make.exe integration"
+  - bash.exe -elc "export PATH=/c/tools/mingw64/bin:/c/gopath/src/github.com/containerd/containerd/bin:$PATH ; mingw32-make.exe integration-parallel"
   # TODO: re-enable once the content unit-test have been updated to pass on windows
   #- bash.exe -lc "export PATH=/c/tools/mingw64/bin:/c/gopath/src/github.com/containerd/containerd/bin:$PATH ; mingw32-make.exe coverage"
   #- bash.exe -lc "export PATH=/c/tools/mingw64/bin:/c/gopath/src/github.com/containerd/containerd/bin:$PATH ; mingw32-make.exe root-coverage"

.travis.yml

@@ -63,6 +63,7 @@ script:
   - if [ "$GOOS" = "linux" ]; then make coverage ; fi
   - if [ "$GOOS" = "linux" ]; then sudo PATH=$PATH GOPATH=$GOPATH make root-coverage ; fi
   - if [ "$GOOS" = "linux" ]; then sudo PATH=$PATH GOPATH=$GOPATH make integration ; fi
+  - if [ "$GOOS" = "linux" ]; then sudo PATH=$PATH GOPATH=$GOPATH make integration-parallel ; fi
 
 after_success:
   - bash <(curl -s https://codecov.io/bash)

Makefile

@@ -69,7 +69,8 @@ ifeq ($(filter \
 endif
 
 # Flags passed to `go test`
-TESTFLAGS ?=-parallel 8 -v $(TESTFLAGS_RACE)
+TESTFLAGS ?= -v $(TESTFLAGS_RACE)
+TESTFLAGS_PARALLEL ?= 8
 
 .PHONY: clean all AUTHORS fmt vet lint dco build binaries test integration setup generate protos checkprotos coverage ci check help install uninstall vendor release
 .DEFAULT: default
@@ -154,7 +155,11 @@ root-test: ## run tests, except integration tests
 
 integration: ## run integration tests
 	@echo "$(WHALE) $@"
-	@go test ${TESTFLAGS} -test.root
+	@go test ${TESTFLAGS} -test.root -parallel 1
+
+integration-parallel: ## run integration tests
+	@echo "$(WHALE) $@"
+	@go test ${TESTFLAGS} -test.root -parallel ${TESTFLAGS_PARALLEL}
 
 benchmark: ## run benchmarks tests
 	@echo "$(WHALE) $@"

client_test.go

@@ -152,6 +152,8 @@ func newClient(t testing.TB, address string, opts ...ClientOpt) (*Client, error)
 }
 
 func TestNewClient(t *testing.T) {
+	t.Parallel()
+
 	client, err := newClient(t, address)
 	if err != nil {
 		t.Fatal(err)
@@ -164,6 +166,7 @@ func TestNewClient(t *testing.T) {
 	}
 }
 
+// All the container's tests depends on this, we need it to run first.
 func TestImagePull(t *testing.T) {
 	if runtime.GOOS == "windows" {
 		// TODO: remove once Windows has a snapshotter

container_linux_test.go

@@ -15,6 +15,8 @@ import (
 )
 
 func TestContainerUpdate(t *testing.T) {
+	t.Parallel()
+
 	client, err := newClient(t, address)
 	if err != nil {
 		t.Fatal(err)
@@ -104,6 +106,8 @@ func TestContainerUpdate(t *testing.T) {
 }
 
 func TestShimInCgroup(t *testing.T) {
+	t.Parallel()
+
 	client, err := newClient(t, address)
 	if err != nil {
 		t.Fatal(err)

container_test.go

@@ -43,6 +43,8 @@ func TestContainerList(t *testing.T) {
 }
 
 func TestNewContainer(t *testing.T) {
+	t.Parallel()
+
 	id := t.Name()
 	client, err := newClient(t, address)
 	if err != nil {
@@ -79,6 +81,8 @@ func TestNewContainer(t *testing.T) {
 }
 
 func TestContainerStart(t *testing.T) {
+	t.Parallel()
+
 	client, err := newClient(t, address)
 	if err != nil {
 		t.Fatal(err)
@@ -150,6 +154,8 @@ func TestContainerStart(t *testing.T) {
 }
 
 func TestContainerOutput(t *testing.T) {
+	t.Parallel()
+
 	client, err := newClient(t, address)
 	if err != nil {
 		t.Fatal(err)
@@ -224,6 +230,8 @@ func TestContainerOutput(t *testing.T) {
 }
 
 func TestContainerExec(t *testing.T) {
+	t.Parallel()
+
 	client, err := newClient(t, address)
 	if err != nil {
 		t.Fatal(err)
@@ -321,6 +329,8 @@ func TestContainerExec(t *testing.T) {
 }
 
 func TestContainerPids(t *testing.T) {
+	t.Parallel()
+
 	client, err := newClient(t, address)
 	if err != nil {
 		t.Fatal(err)
@@ -400,6 +410,8 @@ func TestContainerPids(t *testing.T) {
 }
 
 func TestContainerCloseIO(t *testing.T) {
+	t.Parallel()
+
 	client, err := newClient(t, address)
 	if err != nil {
 		t.Fatal(err)
@@ -491,6 +503,8 @@ func TestContainerCloseIO(t *testing.T) {
 }
 
 func TestContainerAttach(t *testing.T) {
+	t.Parallel()
+
 	if runtime.GOOS == "windows" {
 		// On windows, closing the write side of the pipe closes the read
 		// side, sending an EOF to it and preventing reopening it.
@@ -624,6 +638,8 @@ func TestContainerAttach(t *testing.T) {
 }
 
 func TestDeleteRunningContainer(t *testing.T) {
+	t.Parallel()
+
 	client, err := newClient(t, address)
 	if err != nil {
 		t.Fatal(err)
@@ -693,6 +709,8 @@ func TestDeleteRunningContainer(t *testing.T) {
 }
 
 func TestContainerKill(t *testing.T) {
+	t.Parallel()
+
 	client, err := newClient(t, address)
 	if err != nil {
 		t.Fatal(err)
@@ -763,6 +781,8 @@ func TestContainerKill(t *testing.T) {
 }
 
 func TestContainerNoBinaryExists(t *testing.T) {
+	t.Parallel()
+
 	client, err := newClient(t, address)
 	if err != nil {
 		t.Fatal(err)
@@ -784,7 +804,7 @@ func TestContainerNoBinaryExists(t *testing.T) {
 		}
 	}
 
-	spec, err := generateSpec(withImageConfig(ctx, image), withProcessArgs("nothing"))
+	spec, err := generateSpec(withImageConfig(ctx, image), WithProcessArgs("nothing"))
 	if err != nil {
 		t.Error(err)
 		return
@@ -800,11 +820,11 @@ func TestContainerNoBinaryExists(t *testing.T) {
 	switch runtime.GOOS {
 	case "windows":
 		if err != nil {
-			t.Errorf("failed to create task %v", err)
+			t.Fatalf("failed to create task %v", err)
 		}
-		if err := task.Start(ctx); err != nil {
+		defer task.Delete(ctx, WithProcessKill)
+		if err := task.Start(ctx); err == nil {
 			t.Error("task.Start() should return an error when binary does not exist")
-			task.Delete(ctx)
 		}
 	default:
 		if err == nil {
@@ -815,6 +835,8 @@ func TestContainerNoBinaryExists(t *testing.T) {
 }
 
 func TestContainerExecNoBinaryExists(t *testing.T) {
+	t.Parallel()
+
 	client, err := newClient(t, address)
 	if err != nil {
 		t.Fatal(err)
@@ -890,6 +912,8 @@ func TestContainerExecNoBinaryExists(t *testing.T) {
 }
 
 func TestUserNamespaces(t *testing.T) {
+	t.Parallel()
+
 	client, err := newClient(t, address)
 	if err != nil {
 		t.Fatal(err)
@@ -968,6 +992,8 @@ func TestUserNamespaces(t *testing.T) {
 }
 
 func TestWaitStoppedTask(t *testing.T) {
+	t.Parallel()
+
 	client, err := newClient(t, address)
 	if err != nil {
 		t.Fatal(err)
@@ -1038,6 +1064,8 @@ func TestWaitStoppedTask(t *testing.T) {
 }
 
 func TestWaitStoppedProcess(t *testing.T) {
+	t.Parallel()
+
 	client, err := newClient(t, address)
 	if err != nil {
 		t.Fatal(err)
@@ -1133,6 +1161,8 @@ func TestWaitStoppedProcess(t *testing.T) {
 }
 
 func TestTaskForceDelete(t *testing.T) {
+	t.Parallel()
+
 	client, err := newClient(t, address)
 	if err != nil {
 		t.Fatal(err)
@@ -1184,6 +1214,8 @@ func TestTaskForceDelete(t *testing.T) {
 }
 
 func TestProcessForceDelete(t *testing.T) {
+	t.Parallel()
+
 	client, err := newClient(t, address)
 	if err != nil {
 		t.Fatal(err)
@@ -1263,6 +1295,8 @@ func TestProcessForceDelete(t *testing.T) {
 }
 
 func TestContainerHostname(t *testing.T) {
+	t.Parallel()
+
 	client, err := newClient(t, address)
 	if err != nil {
 		t.Fatal(err)

snapshot/testsuite/issues.go

@@ -22,6 +22,8 @@ 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),
@@ -53,6 +55,8 @@ 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),
@@ -76,6 +80,8 @@ 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),
@@ -118,6 +124,8 @@ 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

@@ -20,24 +20,26 @@ 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.Run("Basic", makeTest(t, name, snapshotterFn, checkSnapshotterBasic))
+	t.Parallel()
-	t.Run("StatActive", makeTest(t, name, snapshotterFn, checkSnapshotterStatActive))
-	t.Run("StatComitted", makeTest(t, name, snapshotterFn, checkSnapshotterStatCommitted))
-	t.Run("TransitivityTest", makeTest(t, name, snapshotterFn, checkSnapshotterTransitivity))
-	t.Run("PreareViewFailingtest", makeTest(t, name, snapshotterFn, checkSnapshotterPrepareView))
-	t.Run("Update", makeTest(t, name, snapshotterFn, checkUpdate))
-	t.Run("Remove", makeTest(t, name, snapshotterFn, checkRemove))
 
-	t.Run("LayerFileupdate", makeTest(t, name, snapshotterFn, checkLayerFileUpdate))
+	t.Run("Basic", makeTest(name, snapshotterFn, checkSnapshotterBasic))
-	t.Run("RemoveDirectoryInLowerLayer", makeTest(t, name, snapshotterFn, checkRemoveDirectoryInLowerLayer))
+	t.Run("StatActive", makeTest(name, snapshotterFn, checkSnapshotterStatActive))
-	t.Run("Chown", makeTest(t, name, snapshotterFn, checkChown))
+	t.Run("StatComitted", makeTest(name, snapshotterFn, checkSnapshotterStatCommitted))
-	t.Run("DirectoryPermissionOnCommit", makeTest(t, name, snapshotterFn, checkDirectoryPermissionOnCommit))
+	t.Run("TransitivityTest", makeTest(name, snapshotterFn, checkSnapshotterTransitivity))
+	t.Run("PreareViewFailingtest", makeTest(name, snapshotterFn, checkSnapshotterPrepareView))
+	t.Run("Update", makeTest(name, snapshotterFn, checkUpdate))
+	t.Run("Remove", makeTest(name, snapshotterFn, checkRemove))
+
+	t.Run("LayerFileupdate", makeTest(name, snapshotterFn, checkLayerFileUpdate))
+	t.Run("RemoveDirectoryInLowerLayer", makeTest(name, snapshotterFn, checkRemoveDirectoryInLowerLayer))
+	t.Run("Chown", makeTest(name, snapshotterFn, checkChown))
+	t.Run("DirectoryPermissionOnCommit", makeTest(name, snapshotterFn, checkDirectoryPermissionOnCommit))
 
 	// Rename test still fails on some kernels with overlay
-	//t.Run("Rename", makeTest(t, name, snapshotterFn, checkRename))
+	//t.Run("Rename", makeTest(name, snapshotterFn, checkRename))
 }
 
-func makeTest(t *testing.T, 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), fn func(ctx context.Context, t *testing.T, snapshotter snapshot.Snapshotter, work string)) func(t *testing.T) {
 	return func(t *testing.T) {
 		ctx := context.Background()
 		ctx = namespaces.WithNamespace(ctx, "testsuite")
@@ -78,6 +80,9 @@ func makeTest(t *testing.T, name string, snapshotterFn func(ctx context.Context,
 
 // 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),
@@ -211,6 +216,8 @@ 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)
@@ -245,6 +252,8 @@ 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)
@@ -306,6 +315,8 @@ 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)
@@ -360,6 +371,8 @@ 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)
@@ -456,6 +469,8 @@ 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)
@@ -607,6 +622,8 @@ 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)
 	}

spec_unix_test.go

@@ -9,6 +9,8 @@ import (
 )
 
 func TestGenerateSpec(t *testing.T) {
+	t.Parallel()
+
 	s, err := GenerateSpec()
 	if err != nil {
 		t.Fatal(err)
@@ -47,6 +49,8 @@ func TestGenerateSpec(t *testing.T) {
 }
 
 func TestSpecWithTTY(t *testing.T) {
+	t.Parallel()
+
 	s, err := GenerateSpec(WithTTY)
 	if err != nil {
 		t.Fatal(err)
@@ -61,6 +65,8 @@ func TestSpecWithTTY(t *testing.T) {
 }
 
 func TestWithLinuxNamespace(t *testing.T) {
+	t.Parallel()
+
 	replacedNS := specs.LinuxNamespace{Type: specs.NetworkNamespace, Path: "/var/run/netns/test"}
 	s, err := GenerateSpec(WithLinuxNamespace(replacedNS))
 	if err != nil {

task.go

@@ -17,6 +17,7 @@ import (
 	"github.com/containerd/containerd/content"
 	"github.com/containerd/containerd/errdefs"
 	"github.com/containerd/containerd/mount"
+	"github.com/containerd/containerd/plugin"
 	"github.com/containerd/containerd/rootfs"
 	"github.com/containerd/containerd/runtime"
 	"github.com/containerd/containerd/typeurl"
@@ -117,7 +118,8 @@ type Task interface {
 var _ = (Task)(&task{})
 
 type task struct {
-	client *Client
+	client    *Client
+	container Container
 
 	io  IO
 	id  string
@@ -250,6 +252,12 @@ func (t *task) Delete(ctx context.Context, opts ...ProcessDeleteOpts) (uint32, e
 	}
 	switch status.Status {
 	case Stopped, Unknown, "":
+	case Created:
+		if t.client.runtime == fmt.Sprintf("%s.%s", plugin.RuntimePlugin, "windows") {
+			// On windows Created is akin to Stopped
+			break
+		}
+		fallthrough
 	default:
 		return UnknownExitStatus, errors.Wrapf(errdefs.ErrFailedPrecondition, "task must be stopped before deletion: %s", status.Status)
 	}

task_opts.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"syscall"
 
+	"github.com/containerd/containerd/errdefs"
 	"github.com/containerd/containerd/linux/runcopts"
 	"github.com/containerd/containerd/mount"
 )
@@ -33,13 +34,20 @@ type ProcessDeleteOpts func(context.Context, Process) error
 // WithProcessKill will forcefully kill and delete a process
 func WithProcessKill(ctx context.Context, p Process) error {
 	s := make(chan struct{}, 1)
+	ctx, cancel := context.WithCancel(ctx)
+	defer cancel()
 	// ignore errors to wait and kill as we are forcefully killing
 	// the process and don't care about the exit status
 	go func() {
 		p.Wait(ctx)
 		close(s)
 	}()
-	p.Kill(ctx, syscall.SIGKILL)
+	if err := p.Kill(ctx, syscall.SIGKILL); err != nil {
+		if errdefs.IsFailedPrecondition(err) || errdefs.IsNotFound(err) {
+			return nil
+		}
+		return err
+	}
 	// wait for the process to fully stop before letting the rest of the deletion complete
 	<-s
 	return nil

windows/task.go

@@ -114,6 +114,7 @@ func (t *task) Start(ctx context.Context) error {
 		return err
 	}
 	if err := p.Start(ctx); err != nil {
+		t.removeProcess(t.id)
 		return err
 	}
 	t.publisher.Publish(ctx,