diff --git a/integration/sandbox_clean_remove_test.go b/integration/sandbox_clean_remove_test.go index 3d4f9eb43..020fea7d1 100644 --- a/integration/sandbox_clean_remove_test.go +++ b/integration/sandbox_clean_remove_test.go @@ -34,48 +34,6 @@ import ( 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" diff --git a/pkg/server/container_remove.go b/pkg/server/container_remove.go index e0f7a853e..6426635dd 100644 --- a/pkg/server/container_remove.go +++ b/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 { diff --git a/pkg/server/sandbox_remove.go b/pkg/server/sandbox_remove.go index e81437a28..2c2deb2e0 100644 --- a/pkg/server/sandbox_remove.go +++ b/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. diff --git a/pkg/server/sandbox_stop.go b/pkg/server/sandbox_stop.go index 1bc622dbc..9b6e0a6ec 100644 --- a/pkg/server/sandbox_stop.go +++ b/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 { - return nil, errors.Wrapf(err, "failed to stop container %q", container.ID) + if err := c.stopContainer(ctx, container, 0); err != nil { + 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 { - return nil, errors.Wrapf(err, "failed to remove network namespace for sandbox %q", id) + if err := sandbox.NetNS.Remove(); err != nil { + 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.