We have individual goroutine for each sandbox container. If there is any
error in handler, that goroutine will put event in that backoff queue.
So we don't need event subscriber for podsandbox. Otherwise, there will
be two goroutines to cleanup sandbox container.
```
>>>> From EventMonitor
time="2025-10-23T19:30:59.626254404Z" level=debug msg="Received containerd event timestamp - 2025-10-23 19:30:59.624494674 +0000 UTC, namespace - \"k8s.io\", topic - \"/tasks/exit\""
time="2025-10-23T19:30:59.626301912Z" level=debug msg="TaskExit event in podsandbox handler container_id:\"22e15114133e4d461ab380654fb76f3e73d3e0323989c422fa17882762979ccf\" id:\"22e15114133e4d461ab380654fb76f3e73d3e0323989c422fa17882762979ccf\" pid:203121 exit_status:137 exited_at:{seconds:1761247859 nanos:624467824}"
>>> If EventMonitor handles task exit well, it will close ttrpc
connection and then waitSandboxExit could encounter ttrpc-closed error
time="2025-10-23T19:30:59.688031150Z" level=error msg="failed to delete task" error="ttrpc: closed" id=22e15114133e4d461ab380654fb76f3e73d3e0323989c422fa17882762979ccf
```
If both task.Delete calls fail but the shim has already been shut down, it
could trigger a new task.Exit event sent by cleanupAfterDeadShim. This would
result in three events in the EventMonitor's backoff queue, which is unnecessary
and could cause confusion due to duplicate events.
The worst-case scenario caused by two concurrent task.Delete calls is a shim
leak. The timeline for this scenario is as follows:
| Timestamp | Component | Action | Result |
| ------ | ----------- | -------- | -------- |
| T1 | EventMonitor | Sends `task.Delete` | Marked as Req-1 |
| T2 | waitSandboxExit | Sends `task.Delete` | Marked as Req-2 |
| T3 | containerd-shim | Handles Req-2 | Container transitions from stopped to deleted |
| T4 | containerd-shim | Handles Req-1 | Fails - container already deleted<br>Returns error: `cannot delete a deleted process: not found` |
| T5 | EventMonitor | Receives `not found` error | - |
| T6 | EventMonitor | Sends `shim.Shutdown` request | No-op (active container record still exists) |
| T7 | EventMonitor | Closes ttrpc connection | Clean container state dir |
| T8 | containerd-shim | Handles Req-2 | Removes container record from memory |
| T9 | waitSandboxExit | Receives error | Error: `ttrpc: closed` |
| T10 | waitSandboxExit | Sends `shim.Shutdown` request | Fails (connection already closed) |
| T11 | waitSandboxExit | Closes ttrpc connection | No-op (already closed) |
The containerd-shim is still running because shim.Shutdown was sent at T6
before T8. Because container's state dir is deleted at T7, it's unable to clean
it up after containerd restarted.
We should avoid concurrent task.Delete calls here.
I also add subcommand - shutdown - in `ctr shim` for debug.
Fixed: #12344
Signed-off-by: Wei Fu <fuweid89@gmail.com>
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 04fe0ff9d2/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 04fe0ff9d2/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: #11107Close: #11967
Signed-off-by: Wei Fu <fuweid89@gmail.com>
Copying of message structs is not allowed and results in the following
govet error. This commit fixes these errors by avoiding copying of the
messages.
> google.golang.org/protobuf/runtime/protoimpl.MessageState contains sync.Mutex (govet)
Signed-off-by: Kohei Tokunaga <ktokunaga.mail@gmail.com>
This commit makes all of the recommended changes to use the `testing`
package helper functions instead of doing the equivalent longhand
versions of the same thing.
This change was needed in order to properly detect errors, as the code
would previously skip running `tenv` stating that it had been deprecated
in favor of `usetesting`.
Signed-off-by: Enji Cooper <yaneurabeya@gmail.com>
Previously, to address issue #11708, PR #11793 changed containerd to always
invoke the shim binary to establish shim connections, rather than reusing the
sandbox shim. However, this change did not ensure that the Shutdown API was
called to stop the shim process.
Starting with containerd v2.0.0, the Shutdown API is only invoked for sandbox
containers (when container.SandboxID is empty). This approach works for
groupable shims, where multiple containers share a single socket address and
only require a single Shutdown call. However, for non-groupable shims, each
container requires its own Shutdown call during cleanup to avoid leaking shim
processes.
Additionally, PR #11793 introduced a corner case during upgrades:
- T1: An old container-shim-runc-v2 (<=v1.7.X) is running for pod A.
- T2: containerd is upgraded to v2.X.Y.
- T3: A new container A-C1 is created in pod A using the new shim-runc-v2 binary.
- T4: bootstrap.json indicates version:3 protocol, but it is downgraded to version:2 in memory.
- T5: containerd is restarted.
- T6: containerd fails to connect to A-C1.
- T7: The A-C1 container is left in EXITED status in the CRI plugin.
To address this, ensure that loadShimTask downgrades to version:2 if necessary,
and always invoke the Shutdown API for each non-groupable shim during cleanup to
prevent resource leaks and handle upgrade scenarios correctly.
(Introduced by #11793)
Signed-off-by: Wei Fu <fuweid89@gmail.com>
We've long been able to use these and they have a couple
great benefits:
1. Forces you to always access them atomically. With the pointer
variants it's completely valid to access the regular ol' int64/uint32
etc. without using the atomic.* methods. These wrappers don't provide
access to the underlying value so it forces correct usage always.
2. Conveys intent much better. Seeing the type be atomic.Int32 immediately
lets the reader know that this var will be used in a concurrent context,
and we no longer need comments like "this MUST be accessed atomically"
or similar.
Signed-off-by: Danny Canter <danny@dcantah.dev>
- adds a transfer service progress reporter to handle timeouts. Also other test fixes
- fallback to local image pull when configuration conflict
Signed-off-by: Tony Fang <nhfang@amazon.com>
Co-authored-by: Swagat Bora <sbora@amazon.com>
Schema 1 (`application/vnd.docker.distribution.manifest.v1+prettyjws`) has been
officially deprecated since containerd v1.7 (PR 6884), and disabled since v2.0 (PR 9765).
Users who have been seeing warnings like `conversion from schema 1 images is deprecated`
now have to rebuild the image with Schema 2 or OCI.
Schema 2 was introduced in Docker 1.10 (Feb 2016), so most users should have been already
using Schema 2 or OCI.
Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>
