Merge pull request #1525 from containerd/forcibly-remove-sandbox-container

Forcibly remove sandbox container

hack/utils.sh

@@ -18,7 +18,7 @@ ROOT="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )"/..
 
 # Not from vendor.conf.
 KUBERNETES_VERSION="v1.19.0-beta.2"
-CRITOOL_VERSION=${CRITOOL_VERSION:-75ef33dc2b4ecb08e0237d91de1b664909d262de}
+CRITOOL_VERSION=${CRITOOL_VERSION:-baca4a152dfe671fc17911a7af74bcb61680ee39}
 CRITOOL_PKG=github.com/kubernetes-sigs/cri-tools
 CRITOOL_REPO=github.com/kubernetes-sigs/cri-tools
 

integration/sandbox_clean_remove_test.go

@@ -21,61 +21,16 @@ import (
 	"os"
 	"path/filepath"
 	"strings"
-	"syscall"
 	"testing"
 	"time"
 
-	"github.com/containerd/containerd"
 	runtimespec "github.com/opencontainers/runtime-spec/specs-go"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
-	"golang.org/x/net/context"
 	"golang.org/x/sys/unix"
 	runtime "k8s.io/cri-api/pkg/apis/runtime/v1alpha2"
 )
 
-func TestSandboxCleanRemove(t *testing.T) {
-	ctx := context.Background()
-	t.Logf("Create a sandbox")
-	sbConfig := PodSandboxConfig("sandbox", "clean-remove")
-	sb, err := runtimeService.RunPodSandbox(sbConfig, *runtimeHandler)
-	require.NoError(t, err)
-	defer func() {
-		// Make sure the sandbox is cleaned up in any case.
-		runtimeService.StopPodSandbox(sb)
-		runtimeService.RemovePodSandbox(sb)
-	}()
-
-	t.Logf("Should not be able to remove the sandbox when it's still running")
-	assert.Error(t, runtimeService.RemovePodSandbox(sb))
-
-	t.Logf("Delete sandbox task from containerd")
-	cntr, err := containerdClient.LoadContainer(ctx, sb)
-	require.NoError(t, err)
-	task, err := cntr.Task(ctx, nil)
-	require.NoError(t, err)
-	// Kill the task with signal SIGKILL, once the task exited,
-	// the TaskExit event will trigger the task.Delete().
-	err = task.Kill(ctx, syscall.SIGKILL, containerd.WithKillAll)
-	require.NoError(t, err)
-
-	t.Logf("Sandbox state should be NOTREADY")
-	assert.NoError(t, Eventually(func() (bool, error) {
-		status, err := runtimeService.PodSandboxStatus(sb)
-		if err != nil {
-			return false, err
-		}
-		return status.GetState() == runtime.PodSandboxState_SANDBOX_NOTREADY, nil
-	}, time.Second, 30*time.Second), "sandbox state should become NOTREADY")
-
-	t.Logf("Should not be able to remove the sandbox when netns is not closed")
-	assert.Error(t, runtimeService.RemovePodSandbox(sb))
-
-	t.Logf("Should be able to remove the sandbox after properly stopped")
-	assert.NoError(t, runtimeService.StopPodSandbox(sb))
-	assert.NoError(t, runtimeService.RemovePodSandbox(sb))
-}
-
 func TestSandboxRemoveWithoutIPLeakage(t *testing.T) {
 	const hostLocalCheckpointDir = "/var/lib/cni"
 

pkg/server/container_remove.go

@@ -21,6 +21,7 @@ import (
 	"github.com/containerd/containerd/errdefs"
 	"github.com/containerd/containerd/log"
 	"github.com/pkg/errors"
+	"github.com/sirupsen/logrus"
 	"golang.org/x/net/context"
 	runtime "k8s.io/cri-api/pkg/apis/runtime/v1alpha2"
 
@@ -29,7 +30,6 @@ import (
 )
 
 // RemoveContainer removes the container.
-// TODO(random-liu): Forcibly stop container if it's running.
 func (c *criService) RemoveContainer(ctx context.Context, r *runtime.RemoveContainerRequest) (_ *runtime.RemoveContainerResponse, retErr error) {
 	container, err := c.containerStore.Get(r.GetContainerId())
 	if err != nil {
@@ -42,6 +42,17 @@ func (c *criService) RemoveContainer(ctx context.Context, r *runtime.RemoveConta
 	}
 	id := container.ID
 
+	// Forcibly stop the containers if they are in running or unknown state
+	state := container.Status.Get().State()
+	if state == runtime.ContainerState_CONTAINER_RUNNING ||
+		state == runtime.ContainerState_CONTAINER_UNKNOWN {
+		logrus.Infof("Forcibly stopping container %q", id)
+		if err := c.stopContainer(ctx, container, 0); err != nil {
+			return nil, errors.Wrapf(err, "failed to forcibly stop container %q", id)
+		}
+
+	}
+
 	// Set removing state to prevent other start/remove operations against this container
 	// while it's being removed.
 	if err := setContainerRemoving(container); err != nil {

pkg/server/sandbox_remove.go

@@ -21,6 +21,7 @@ import (
 	"github.com/containerd/containerd/errdefs"
 	"github.com/containerd/containerd/log"
 	"github.com/pkg/errors"
+	"github.com/sirupsen/logrus"
 	"golang.org/x/net/context"
 	runtime "k8s.io/cri-api/pkg/apis/runtime/v1alpha2"
 
@@ -48,7 +49,10 @@ func (c *criService) RemovePodSandbox(ctx context.Context, r *runtime.RemovePodS
 	// Return error if sandbox container is still running or unknown.
 	state := sandbox.Status.Get().State
 	if state == sandboxstore.StateReady || state == sandboxstore.StateUnknown {
-		return nil, errors.Errorf("sandbox container %q is not fully stopped", id)
+		logrus.Infof("Forcibly stopping sandbox %q", id)
+		if err := c.stopPodSandbox(ctx, sandbox); err != nil {
+			return nil, errors.Wrapf(err, "failed to forcibly stop sandbox %q", id)
+		}
 	}
 
 	// Return error if sandbox network namespace is not closed yet.

pkg/server/sandbox_stop.go

@@ -39,6 +39,15 @@ func (c *criService) StopPodSandbox(ctx context.Context, r *runtime.StopPodSandb
 		return nil, errors.Wrapf(err, "an error occurred when try to find sandbox %q",
 			r.GetPodSandboxId())
 	}
+
+	if err := c.stopPodSandbox(ctx, sandbox); err != nil {
+		return nil, err
+	}
+
+	return &runtime.StopPodSandboxResponse{}, nil
+}
+
+func (c *criService) stopPodSandbox(ctx context.Context, sandbox sandboxstore.Sandbox) error {
 	// Use the full sandbox id.
 	id := sandbox.ID
 
@@ -52,20 +61,20 @@ func (c *criService) StopPodSandbox(ctx context.Context, r *runtime.StopPodSandb
 		}
 		// Forcibly stop the container. Do not use `StopContainer`, because it introduces a race
 		// if a container is removed after list.
-		if err = c.stopContainer(ctx, container, 0); err != nil {
+		if err := c.stopContainer(ctx, container, 0); err != nil {
-			return nil, errors.Wrapf(err, "failed to stop container %q", container.ID)
+			return errors.Wrapf(err, "failed to stop container %q", container.ID)
 		}
 	}
 
 	if err := c.cleanupSandboxFiles(id, sandbox.Config); err != nil {
-		return nil, errors.Wrap(err, "failed to cleanup sandbox files")
+		return errors.Wrap(err, "failed to cleanup sandbox files")
 	}
 
 	// Only stop sandbox container when it's running or unknown.
 	state := sandbox.Status.Get().State
 	if state == sandboxstore.StateReady || state == sandboxstore.StateUnknown {
 		if err := c.stopSandboxContainer(ctx, sandbox); err != nil {
-			return nil, errors.Wrapf(err, "failed to stop sandbox container %q in %q state", id, state)
+			return errors.Wrapf(err, "failed to stop sandbox container %q in %q state", id, state)
 		}
 	}
 
@@ -74,21 +83,21 @@ func (c *criService) StopPodSandbox(ctx context.Context, r *runtime.StopPodSandb
 		// Use empty netns path if netns is not available. This is defined in:
 		// https://github.com/containernetworking/cni/blob/v0.7.0-alpha1/SPEC.md
 		if closed, err := sandbox.NetNS.Closed(); err != nil {
-			return nil, errors.Wrap(err, "failed to check network namespace closed")
+			return errors.Wrap(err, "failed to check network namespace closed")
 		} else if closed {
 			sandbox.NetNSPath = ""
 		}
 		if err := c.teardownPodNetwork(ctx, sandbox); err != nil {
-			return nil, errors.Wrapf(err, "failed to destroy network for sandbox %q", id)
+			return errors.Wrapf(err, "failed to destroy network for sandbox %q", id)
 		}
-		if err = sandbox.NetNS.Remove(); err != nil {
+		if err := sandbox.NetNS.Remove(); err != nil {
-			return nil, errors.Wrapf(err, "failed to remove network namespace for sandbox %q", id)
+			return errors.Wrapf(err, "failed to remove network namespace for sandbox %q", id)
 		}
 	}
 
 	log.G(ctx).Infof("TearDown network for sandbox %q successfully", id)
 
-	return &runtime.StopPodSandboxResponse{}, nil
+	return nil
 }
 
 // stopSandboxContainer kills the sandbox container.