Do not SIGKILL container if container stop is cancelled.

Signed-off-by: Lantao Liu <lantaol@google.com>

integration/container_stop_test.go

@@ -0,0 +1,91 @@
+/*
+Copyright 2019 The containerd Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package integration
+
+import (
+	"context"
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	runtime "k8s.io/kubernetes/pkg/kubelet/apis/cri/runtime/v1alpha2"
+)
+
+func TestContainerStopCancellation(t *testing.T) {
+	t.Log("Create a pod sandbox")
+	sbConfig := PodSandboxConfig("sandbox", "cancel-container-stop")
+	sb, err := runtimeService.RunPodSandbox(sbConfig)
+	require.NoError(t, err)
+	defer func() {
+		assert.NoError(t, runtimeService.StopPodSandbox(sb))
+		assert.NoError(t, runtimeService.RemovePodSandbox(sb))
+	}()
+
+	const (
+		testImage     = "busybox"
+		containerName = "test-container"
+	)
+	t.Logf("Pull test image %q", testImage)
+	img, err := imageService.PullImage(&runtime.ImageSpec{Image: testImage}, nil)
+	require.NoError(t, err)
+	defer func() {
+		assert.NoError(t, imageService.RemoveImage(&runtime.ImageSpec{Image: img}))
+	}()
+
+	t.Log("Create a container which traps sigterm")
+	cnConfig := ContainerConfig(
+		containerName,
+		testImage,
+		WithCommand("sh", "-c", `trap "echo ignore sigterm" TERM; sleep 1000`),
+	)
+	cn, err := runtimeService.CreateContainer(sb, cnConfig, sbConfig)
+	require.NoError(t, err)
+
+	t.Log("Start the container")
+	require.NoError(t, runtimeService.StartContainer(cn))
+
+	t.Log("Stop the container with 3s timeout, but 1s context timeout")
+	// Note that with container pid namespace, the sleep process
+	// is pid 1, and SIGTERM sent by `StopContainer` will be ignored.
+	rawClient, err := RawRuntimeClient()
+	require.NoError(t, err)
+	ctx, cancel := context.WithTimeout(context.Background(), 1*time.Second)
+	defer cancel()
+	_, err = rawClient.StopContainer(ctx, &runtime.StopContainerRequest{
+		ContainerId: cn,
+		Timeout:     3,
+	})
+	assert.Error(t, err)
+
+	t.Log("The container should still be running even after 5 seconds")
+	assert.NoError(t, Consistently(func() (bool, error) {
+		s, err := runtimeService.ContainerStatus(cn)
+		if err != nil {
+			return false, err
+		}
+		return s.GetState() == runtime.ContainerState_CONTAINER_RUNNING, nil
+	}, 100*time.Millisecond, 5*time.Second))
+
+	t.Log("Stop the container with 1s timeout, without shorter context timeout")
+	assert.NoError(t, runtimeService.StopContainer(cn, 1))
+
+	t.Log("The container state should be exited")
+	s, err := runtimeService.ContainerStatus(cn)
+	require.NoError(t, err)
+	assert.Equal(t, s.GetState(), runtime.ContainerState_CONTAINER_EXITED)
+}

integration/test_utils.go

@@ -261,6 +261,26 @@ func Eventually(f CheckFunc, period, timeout time.Duration) error {
 	}
 }
 
+// Consistently makes sure that f consistently returns true without
+// error before timeout exceeds. If f returns error, Consistently
+// will return the same error immediately.
+func Consistently(f CheckFunc, period, timeout time.Duration) error {
+	start := time.Now()
+	for {
+		ok, err := f()
+		if !ok {
+			return errors.New("get false")
+		}
+		if err != nil {
+			return err
+		}
+		if time.Since(start) >= timeout {
+			return nil
+		}
+		time.Sleep(period)
+	}
+}
+
 // Randomize adds uuid after a string.
 func Randomize(str string) string {
 	return str + "-" + util.GenerateID()

pkg/server/container_stop.go

@@ -111,8 +111,9 @@ func (c *criService) stopContainer(ctx context.Context, container containerstore
 			return errors.Wrapf(err, "failed to stop container %q", id)
 		}
 
-		if err = c.waitContainerStop(ctx, container, timeout); err == nil {
-			return nil
+		if err = c.waitContainerStop(ctx, container, timeout); err == nil || errors.Cause(err) == ctx.Err() {
+			// Do not SIGKILL container if the context is cancelled.
+			return err
 		}
 		logrus.WithError(err).Errorf("An error occurs during waiting for container %q to be stopped", id)
 	}
@@ -156,7 +157,7 @@ func (c *criService) waitContainerStop(ctx context.Context, container containers
 	defer timeoutTimer.Stop()
 	select {
 	case <-ctx.Done():
-		return errors.Errorf("wait container %q is cancelled", container.ID)
+		return errors.Wrapf(ctx.Err(), "wait container %q is cancelled", container.ID)
 	case <-timeoutTimer.C:
 		return errors.Errorf("wait container %q stop timeout", container.ID)
 	case <-container.Stopped():

pkg/server/sandbox_stop.go

@@ -129,7 +129,7 @@ func (c *criService) waitSandboxStop(ctx context.Context, sandbox sandboxstore.S
 	defer timeoutTimer.Stop()
 	select {
 	case <-ctx.Done():
-		return errors.Errorf("wait sandbox container %q is cancelled", sandbox.ID)
+		return errors.Wrapf(ctx.Err(), "wait sandbox container %q is cancelled", sandbox.ID)
 	case <-timeoutTimer.C:
 		return errors.Errorf("wait sandbox container %q stop timeout", sandbox.ID)
 	case <-sandbox.Stopped():