From feade98473aec03a8bffd1d611b76ed64637771a Mon Sep 17 00:00:00 2001 From: Sebastien Boeuf Date: Fri, 13 Apr 2018 15:22:22 -0700 Subject: [PATCH 1/3] virtcontainers: Fix unit tests relying on noopShim When using noopShim type from the unit tests, we were ending up getting a PID 1000, and when checking if the shim was around, we were always expecting the shim to be "not running", based on the fact that the process was not there anymore. Unfortunately, this was a very wrong assumption because we cannot control which PIDs are running or not on the system. The way to simplify this is to return a PID 0 in case of noopShim, processed as a special case by the function waitForShim(). Fixes #208 Signed-off-by: Sebastien Boeuf --- virtcontainers/noop_shim.go | 2 +- virtcontainers/noop_shim_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/virtcontainers/noop_shim.go b/virtcontainers/noop_shim.go index ca1426b552..e7d34c1631 100644 --- a/virtcontainers/noop_shim.go +++ b/virtcontainers/noop_shim.go @@ -21,5 +21,5 @@ type noopShim struct{} // start is the noopShim start implementation for testing purpose. // It does nothing. func (s *noopShim) start(sandbox Sandbox, params ShimParams) (int, error) { - return 1000, nil + return 0, nil } diff --git a/virtcontainers/noop_shim_test.go b/virtcontainers/noop_shim_test.go index fcff148df7..f34e800c69 100644 --- a/virtcontainers/noop_shim_test.go +++ b/virtcontainers/noop_shim_test.go @@ -24,7 +24,7 @@ func TestNoopShimStart(t *testing.T) { s := &noopShim{} sandbox := Sandbox{} params := ShimParams{} - expected := 1000 + expected := 0 pid, err := s.start(sandbox, params) if err != nil { From 2c3cfed608083f5aef7e9b1a8f066254f0eaec44 Mon Sep 17 00:00:00 2001 From: Sebastien Boeuf Date: Fri, 13 Apr 2018 15:42:13 -0700 Subject: [PATCH 2/3] virtcontainers: mock: Properly end cc_proxy_mock goroutines Because of the bad design of the cc_proxy_mock go routine, we were leaving an infinite loop running into this go routine behind. This was consuming a lot of resources and it was obviously slowing down the tests being run in parallel. That's one of the reason we were hitting the 10 seconds timeout when running go tests. Fixes #208 Signed-off-by: Sebastien Boeuf --- virtcontainers/pkg/mock/cc_proxy_mock.go | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/virtcontainers/pkg/mock/cc_proxy_mock.go b/virtcontainers/pkg/mock/cc_proxy_mock.go index 3acee1a67e..743e3ffa8a 100644 --- a/virtcontainers/pkg/mock/cc_proxy_mock.go +++ b/virtcontainers/pkg/mock/cc_proxy_mock.go @@ -31,6 +31,8 @@ const testToken = "pF56IaDpuax6hihJ5PneB8JypqmOvjkqY-wKGVYqgIM=" // CCProxyMock is an object mocking clearcontainers Proxy type CCProxyMock struct { + sync.Mutex + t *testing.T wg sync.WaitGroup connectionPath string @@ -50,6 +52,8 @@ type CCProxyMock struct { Signal chan ShimSignal ShimDisconnected chan bool StdinReceived chan bool + + stopped bool } // NewCCProxyMock creates a hyperstart instance @@ -296,10 +300,19 @@ func (proxy *CCProxyMock) serve() { // Start invokes mock proxy instance to start listening. func (proxy *CCProxyMock) Start() { + proxy.stopped = false proxy.startListening() go func() { for { proxy.serve() + + proxy.Lock() + stopped := proxy.stopped + proxy.Unlock() + + if stopped { + break + } } }() } @@ -307,6 +320,10 @@ func (proxy *CCProxyMock) Start() { // Stop causes mock proxy instance to stop listening, // close connection to client and close all channels func (proxy *CCProxyMock) Stop() { + proxy.Lock() + proxy.stopped = true + proxy.Unlock() + proxy.listener.Close() if proxy.cl != nil { From 92577c635fdb186242d7dc73d08f24770cbd96c9 Mon Sep 17 00:00:00 2001 From: Sebastien Boeuf Date: Fri, 13 Apr 2018 15:58:20 -0700 Subject: [PATCH 3/3] virtcontainers: Properly end up go routines using channels Those different files were all calling into a go routine that was eventually reporting some result through a go channel. The problem was the way those routine were implemented, as they were hanging around forever. Indeed, nothing was actually listening to the channel in some cases, and those routines never ended. This was one of the problem detected by the fact that our unit tests needed more time to pass because when they were all run in parallel, the resources consumed by those routines were increasing the time for other tests to complete. Fixes #208 Signed-off-by: Sebastien Boeuf --- virtcontainers/cc_shim_test.go | 3 ++- virtcontainers/hook.go | 13 ++++++++++--- virtcontainers/kata_shim_test.go | 3 ++- 3 files changed, 14 insertions(+), 5 deletions(-) diff --git a/virtcontainers/cc_shim_test.go b/virtcontainers/cc_shim_test.go index 960af3c3ec..3d1c01ed04 100644 --- a/virtcontainers/cc_shim_test.go +++ b/virtcontainers/cc_shim_test.go @@ -242,8 +242,9 @@ func TestCCShimStartDetachSuccessful(t *testing.T) { testCCShimStart(t, sandbox, params, false) - readCh := make(chan error) + readCh := make(chan error, 1) go func() { + defer close(readCh) bufStdout := make([]byte, 1024) n, err := rStdout.Read(bufStdout) if err != nil && err != io.EOF { diff --git a/virtcontainers/hook.go b/virtcontainers/hook.go index 5da2bb6dbe..66c8674064 100644 --- a/virtcontainers/hook.go +++ b/virtcontainers/hook.go @@ -22,6 +22,7 @@ import ( "fmt" "os" "os/exec" + "syscall" "time" specs "github.com/opencontainers/runtime-spec/specs-go" @@ -82,9 +83,11 @@ func (h *Hook) runHook() error { return fmt.Errorf("%s: stdout: %s, stderr: %s", err, stdout.String(), stderr.String()) } } else { - done := make(chan error) - - go func() { done <- cmd.Wait() }() + done := make(chan error, 1) + go func() { + done <- cmd.Wait() + close(done) + }() select { case err := <-done: @@ -92,6 +95,10 @@ func (h *Hook) runHook() error { return fmt.Errorf("%s: stdout: %s, stderr: %s", err, stdout.String(), stderr.String()) } case <-time.After(time.Duration(h.Timeout) * time.Second): + if err := syscall.Kill(cmd.Process.Pid, syscall.SIGKILL); err != nil { + return err + } + return fmt.Errorf("Hook timeout") } } diff --git a/virtcontainers/kata_shim_test.go b/virtcontainers/kata_shim_test.go index 4d43780b0d..4ae1057f3e 100644 --- a/virtcontainers/kata_shim_test.go +++ b/virtcontainers/kata_shim_test.go @@ -236,8 +236,9 @@ func TestKataShimStartDetachSuccessful(t *testing.T) { testKataShimStart(t, sandbox, params, false) - readCh := make(chan error) + readCh := make(chan error, 1) go func() { + defer close(readCh) bufStdout := make([]byte, 1024) n, err := rStdout.Read(bufStdout) if err != nil && err != io.EOF {