From eb0970bbd1b91298a914981dd05c14045a4b79b3 Mon Sep 17 00:00:00 2001 From: Kenfe-Mickael Laventure Date: Fri, 11 Aug 2017 11:24:59 -0700 Subject: [PATCH 1/4] Mark relevant tests as elligible for parallelism Signed-off-by: Kenfe-Mickael Laventure --- Makefile | 9 +++- client_test.go | 3 ++ ...nt_test.go => container_checkpoint_test.go | 0 container_linux_test.go | 4 ++ container_test.go | 34 +++++++++++++++ snapshot/testsuite/issues.go | 8 ++++ snapshot/testsuite/testsuite.go | 43 +++++++++++++------ spec_unix_test.go | 6 +++ 8 files changed, 92 insertions(+), 15 deletions(-) rename checkpoint_test.go => container_checkpoint_test.go (100%) diff --git a/Makefile b/Makefile index 5813a1a38..943ae8252 100644 --- a/Makefile +++ b/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) $@" diff --git a/client_test.go b/client_test.go index 23e99875c..96be2b37b 100644 --- a/client_test.go +++ b/client_test.go @@ -149,6 +149,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) @@ -161,6 +163,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 diff --git a/checkpoint_test.go b/container_checkpoint_test.go similarity index 100% rename from checkpoint_test.go rename to container_checkpoint_test.go diff --git a/container_linux_test.go b/container_linux_test.go index 2744f7e99..84f7cc9aa 100644 --- a/container_linux_test.go +++ b/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) diff --git a/container_test.go b/container_test.go index e9ecb9e2f..ee09d81ac 100644 --- a/container_test.go +++ b/container_test.go @@ -44,6 +44,8 @@ func TestContainerList(t *testing.T) { } func TestNewContainer(t *testing.T) { + t.Parallel() + id := t.Name() client, err := newClient(t, address) if err != nil { @@ -80,6 +82,8 @@ func TestNewContainer(t *testing.T) { } func TestContainerStart(t *testing.T) { + t.Parallel() + client, err := newClient(t, address) if err != nil { t.Fatal(err) @@ -151,6 +155,8 @@ func TestContainerStart(t *testing.T) { } func TestContainerOutput(t *testing.T) { + t.Parallel() + client, err := newClient(t, address) if err != nil { t.Fatal(err) @@ -225,6 +231,8 @@ func TestContainerOutput(t *testing.T) { } func TestContainerExec(t *testing.T) { + t.Parallel() + client, err := newClient(t, address) if err != nil { t.Fatal(err) @@ -322,6 +330,8 @@ func TestContainerExec(t *testing.T) { } func TestContainerPids(t *testing.T) { + t.Parallel() + client, err := newClient(t, address) if err != nil { t.Fatal(err) @@ -401,6 +411,8 @@ func TestContainerPids(t *testing.T) { } func TestContainerCloseIO(t *testing.T) { + t.Parallel() + client, err := newClient(t, address) if err != nil { t.Fatal(err) @@ -492,6 +504,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. @@ -625,6 +639,8 @@ func TestContainerAttach(t *testing.T) { } func TestDeleteRunningContainer(t *testing.T) { + t.Parallel() + client, err := newClient(t, address) if err != nil { t.Fatal(err) @@ -694,6 +710,8 @@ func TestDeleteRunningContainer(t *testing.T) { } func TestContainerKill(t *testing.T) { + t.Parallel() + client, err := newClient(t, address) if err != nil { t.Fatal(err) @@ -764,6 +782,8 @@ func TestContainerKill(t *testing.T) { } func TestContainerNoBinaryExists(t *testing.T) { + t.Parallel() + client, err := newClient(t, address) if err != nil { t.Fatal(err) @@ -816,6 +836,8 @@ func TestContainerNoBinaryExists(t *testing.T) { } func TestContainerExecNoBinaryExists(t *testing.T) { + t.Parallel() + client, err := newClient(t, address) if err != nil { t.Fatal(err) @@ -891,6 +913,8 @@ func TestContainerExecNoBinaryExists(t *testing.T) { } func TestUserNamespaces(t *testing.T) { + t.Parallel() + client, err := newClient(t, address) if err != nil { t.Fatal(err) @@ -969,6 +993,8 @@ func TestUserNamespaces(t *testing.T) { } func TestWaitStoppedTask(t *testing.T) { + t.Parallel() + client, err := newClient(t, address) if err != nil { t.Fatal(err) @@ -1039,6 +1065,8 @@ func TestWaitStoppedTask(t *testing.T) { } func TestWaitStoppedProcess(t *testing.T) { + t.Parallel() + client, err := newClient(t, address) if err != nil { t.Fatal(err) @@ -1134,6 +1162,8 @@ func TestWaitStoppedProcess(t *testing.T) { } func TestTaskForceDelete(t *testing.T) { + t.Parallel() + client, err := newClient(t, address) if err != nil { t.Fatal(err) @@ -1185,6 +1215,8 @@ func TestTaskForceDelete(t *testing.T) { } func TestProcessForceDelete(t *testing.T) { + t.Parallel() + client, err := newClient(t, address) if err != nil { t.Fatal(err) @@ -1264,6 +1296,8 @@ func TestProcessForceDelete(t *testing.T) { } func TestContainerHostname(t *testing.T) { + t.Parallel() + client, err := newClient(t, address) if err != nil { t.Fatal(err) diff --git a/snapshot/testsuite/issues.go b/snapshot/testsuite/issues.go index a41c401f0..84b7a5e01 100644 --- a/snapshot/testsuite/issues.go +++ b/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), diff --git a/snapshot/testsuite/testsuite.go b/snapshot/testsuite/testsuite.go index f06adba68..467b30104 100644 --- a/snapshot/testsuite/testsuite.go +++ b/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.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.Parallel() - t.Run("LayerFileupdate", makeTest(t, name, snapshotterFn, checkLayerFileUpdate)) - t.Run("RemoveDirectoryInLowerLayer", makeTest(t, name, snapshotterFn, checkRemoveDirectoryInLowerLayer)) - t.Run("Chown", makeTest(t, name, snapshotterFn, checkChown)) - t.Run("DirectoryPermissionOnCommit", makeTest(t, name, snapshotterFn, checkDirectoryPermissionOnCommit)) + t.Run("Basic", makeTest(name, snapshotterFn, checkSnapshotterBasic)) + t.Run("StatActive", makeTest(name, snapshotterFn, checkSnapshotterStatActive)) + t.Run("StatComitted", makeTest(name, snapshotterFn, checkSnapshotterStatCommitted)) + 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) } diff --git a/spec_unix_test.go b/spec_unix_test.go index bd23b77ea..e7451fe22 100644 --- a/spec_unix_test.go +++ b/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 { From 104a8088cab6289265f91c01f316a8533653bb16 Mon Sep 17 00:00:00 2001 From: Kenfe-Mickael Laventure Date: Fri, 11 Aug 2017 11:40:53 -0700 Subject: [PATCH 2/4] Add integration-parallel to Travis & AppVeyor CIs Signed-off-by: Kenfe-Mickael Laventure --- .appveyor.yml | 1 + .travis.yml | 1 + 2 files changed, 2 insertions(+) diff --git a/.appveyor.yml b/.appveyor.yml index 8650ef08e..a77586470 100644 --- a/.appveyor.yml +++ b/.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" diff --git a/.travis.yml b/.travis.yml index da7e672c0..b52fa0120 100644 --- a/.travis.yml +++ b/.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) From b02e9a844e95c5dba6889d45ef7d6d6eec840bd8 Mon Sep 17 00:00:00 2001 From: Kenfe-Mickael Laventure Date: Fri, 11 Aug 2017 11:57:00 -0700 Subject: [PATCH 3/4] Fix TestContainerNoBinaryExists on windows Signed-off-by: Kenfe-Mickael Laventure --- container_test.go | 9 +++++---- task.go | 10 +++++++++- windows/task.go | 1 + 3 files changed, 15 insertions(+), 5 deletions(-) diff --git a/container_test.go b/container_test.go index ee09d81ac..ab4ecbba4 100644 --- a/container_test.go +++ b/container_test.go @@ -805,7 +805,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 @@ -821,11 +821,12 @@ 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) + if err := task.Start(ctx); err == nil { + task.Kill(ctx, syscall.SIGKILL) t.Error("task.Start() should return an error when binary does not exist") - task.Delete(ctx) } default: if err == nil { diff --git a/task.go b/task.go index 87c4defc3..2d49fad27 100644 --- a/task.go +++ b/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) } diff --git a/windows/task.go b/windows/task.go index f8caa67cd..76c3c1a61 100644 --- a/windows/task.go +++ b/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, From 2abaf6e964d8d3048d33683e74209e1f8f4cabe8 Mon Sep 17 00:00:00 2001 From: Kenfe-Mickael Laventure Date: Mon, 14 Aug 2017 10:34:53 -0700 Subject: [PATCH 4/4] Fix possible deadlock in WithProcessKill We were not checking the error value of `Kill` leading to deadlock if the process didn't exist. Signed-off-by: Kenfe-Mickael Laventure --- container_test.go | 3 +-- task_opts.go | 10 +++++++++- 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/container_test.go b/container_test.go index ab4ecbba4..aa13e8d04 100644 --- a/container_test.go +++ b/container_test.go @@ -823,9 +823,8 @@ func TestContainerNoBinaryExists(t *testing.T) { if err != nil { t.Fatalf("failed to create task %v", err) } - defer task.Delete(ctx) + defer task.Delete(ctx, WithProcessKill) if err := task.Start(ctx); err == nil { - task.Kill(ctx, syscall.SIGKILL) t.Error("task.Start() should return an error when binary does not exist") } default: diff --git a/task_opts.go b/task_opts.go index 271c4ee9c..0e4b75e85 100644 --- a/task_opts.go +++ b/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