mirror of
https://github.com/containerd/containerd.git
synced 2026-11-06 21:37:23 +00:00
Wei Fu
4f130dbe73
Background: The TestRunPodSandboxWithShimDeleteFailure test case validates that the sandbox can be properly cleaned up when RunPodSandbox returns an error. This test simulates a RunPodSandbox failure by using a failpoint to inject an error during the startup of the sandbox container. It also blocks the first task deletion attempt within the deferred cleanup logic of RunPodSandbox. As a result, the sandbox remains after the failed RunPodSandbox call, allowing the test to verify the subsequent cleanup process. Race-condition causes flaky issue: The defer cleanup invokes `task.Delete` with `containerd.WithProcessKill` option so that it will kill sandbox container first. Since we don't block `Kill` call, cleanup will kill sandbox container successfully, even if `task.Delete` fails. ```go // Link https://github.com/containerd/containerd/blob/04fe0ff9d292d3fa29d239e6a7f312e8175415c8/internal/cri/server/podsandbox/sandbox_run.go#L248-L258 defer func() { if retErr != nil && cleanupErr == nil { deferCtx, deferCancel := ctrdutil.DeferContext() defer deferCancel() // Cleanup the sandbox container if an error is returned. if _, err := task.Delete(deferCtx, WithNRISandboxDelete(id), containerd.WithProcessKill); err != nil && !errdefs.IsNotFound(err) { log.G(ctx).WithError(err).Errorf("Failed to delete sandbox container %q", id) cleanupErr = err } } }() ``` That podSandboxEventHandler.HandleEvent receives task exit event during defer cleanup of RunPodSandbox and then it will delete task. That `task.Delete` call could take few second to shutdown containerd-shim. It could bring race-condition if we invoke RemovePodSandbox. ```go // Link https://github.com/containerd/containerd/blob/04fe0ff9d292d3fa29d239e6a7f312e8175415c8/internal/cri/server/podsandbox/events.go#L43-L61 func (p *podSandboxEventHandler) HandleEvent(any interface{}) error { switch e := any.(type) { case *eventtypes.TaskExit: log.L.Debugf("TaskExit event in podsandbox handler %+v", e) // Use ID instead of ContainerID to rule out TaskExit event for exec. sb := p.controller.store.Get(e.ID) if sb == nil || sb.Container == nil { return nil } ctx := ctrdutil.NamespacedContext() ctx, cancel := context.WithTimeout(ctx, handleEventTimeout) defer cancel() if err := handleSandboxTaskExit(ctx, sb, e); err != nil { return fmt.Errorf("failed to handle container TaskExit event: %w", err) } return nil } return nil } ``` If shim exits, that RemovePodSandbox could return error like ``` ... failed to kill pod sandbox container: ttrpc: closed ``` Retry and fix this transient issue: If `a ttrpc: closed` error occurs, we should retry the operation. There’s no need to introduce a mutex for this, as the kubelet already has retry logic to ensure the sandbox is eventually cleaned up. Other flaky issue: The event handle could be able to cleanup shim asynchronously. That could cause ListPodSandbox returns empty, which is not expected. Updated that test case with `Kill` failpoint instead of `Delete`. Fixes: #11107 Close: #11967 Signed-off-by: Wei Fu <fuweid89@gmail.com>