To make task.Delete retriable, this patch caches the delete result in
memory after the shim successfully deletes the task. If a later shutdown
attempt fails, a subsequent Delete retry can complete cleanup and return the
cached exit information.
If containerd restarts after deleting the task, the in-memory cache is lost,
and containerd will publish TaskExit and TaskDelete events again to
subscribers. Currently, the exit status returned by shim-delete is always
137. In a follow-up, containerd-shim-runc-v2 should store the actual exit
status in the state directory, so that shim-delete can reuse it instead of
returning a hard-coded value.
Implements: #8981
Signed-off-by: Wei Fu <fuweid89@gmail.com>
Kata's EROFS snapshotter shim set containerd.io/runtime-allow-mounts
before the MountCapabilities bootstrap extension existed
(https://github.com/kata-containers/kata-containers/pull/12763), so
removing the annotation outright would break it and anyone else who
adopted it early with no migration window.
Rather than keep the annotation as a first-class, permanently
supported mechanism, confine falling back to it in
task_mounts_deprecated.go: when a shim's bootstrap result carries no
MountCapabilities extension, its runtime name is checked against the
annotation as a migration path, with the result cached per runtime
name for the life of the daemon, same as the removed mechanism did.
Two runtimes known to never set it, runc.v2 and runhcs.v1, are
skipped outright rather than queried.
The extension stays authoritative whenever a shim provides it: this
fallback only runs in its absence, and is confined to one file so it
can be deleted outright once known early adopters have migrated.
Signed-off-by: Derek McGowan <derek@mcg.dev>
When a container joins an existing sandbox via a Task API address supplied
directly by the sandbox controller (rather than by restoring
bootstrap.json), ShimManager.Start synthesized a BootstrapResult carrying
only the version, protocol and address. A shim that advertised
MountCapabilities at sandbox startup was therefore treated as if it had
advertised nothing for every container that joins the sandbox this way,
silently ignoring mount handling it explicitly claimed.
Recover the extensions from the sandbox's shim instance, which containerd
already holds in memory, via the existing shimCapabilities interface. This
is best effort: an external sandboxer whose shim instance containerd does
not track degrades to no extensions rather than failing sandbox startup,
matching today's behavior.
Reported independently by fuweid and Copilot on containerd/containerd#14002.
Signed-off-by: Derek McGowan <derek@mcg.dev>
A shim declared the mount types it performs itself through the annotation
removed in an earlier commit, discovered by invoking the shim binary a
second time with -info. Read them instead from the MountCapabilities
extension a shim advertises in its bootstrap result, which is already
available from the shim start that Create() performs anyway.
This requires starting the shim before activating the task's mounts.
Starting a shim does not need the rootfs; only the task.Create call
consumes it. Reordering means a failure after the shim has started must
now tear the shim down, so the teardown that was inlined in the
task.Create error path becomes a deferred cleanup covering the whole
window, reusing the existing cleanupShimTask helper. That defer is
registered after the mount deactivation defer so that the shim, which
may still be using the mounts, is stopped first.
Extract the mount activation logic into a new taskMountController in
task_mounts.go, rather than growing it further inline in Create(): it
already has its own state transitions (owned vs. reused activation,
NotImplemented, AlreadyExists) independent of shim startup, and this
keeps that reasoning testable without spawning a shim.
Also fix two latent nil pointer dereferences: Create() and Delete() both
called the mount.Manager interface directly with no nil check, which
panics whenever the mount manager plugin is not registered.
taskMountController.Activate and Deactivate treat a nil manager as
nothing to do.
Signed-off-by: Derek McGowan <derek@mcg.dev>
A claimed transform was recorded but never actually left unapplied:
every parsed transformer in a mount's chain was applied in full
regardless of what the caller had claimed, so a shim advertising it
performs "mkdir" in "format/mkdir/overlay" never got the chance to,
since the manager silently applied it anyway. This predates the shim
capability protocol — it was reachable before through the "format/*"
annotation convention — but this series turns it into a documented,
supported way for a shim to claim a transform, so a shim relying on it
(at least one is: Kata's EROFS snapshotter shim, per its annotation
usage) would find its claim silently ignored.
A claimed transform can only be honored as a suffix of the chain: each
transform's input is the previous one's output, so an outer, unclaimed
transform must still be applied even when an inner one is claimed,
since the inner one cannot run without it. planActivation now tracks,
per mount, how much of its chain — starting from the beginning — the
manager must apply itself: through the last unclaimed transform, which
by construction covers every transform before it too. Applying only
that prefix and returning the true, contiguous claimed suffix to the
caller is what actually makes the claim meaningful. This is only
observable for mounts[firstSystemMount], the one mount partially
transformed by the manager and returned to the caller to finish; every
earlier mount is fully internal to the manager and was, and remains,
always fully transformed regardless of any claim.
While changing this expression, also guard it against the case where
firstSystemMount reaches len(mounts) — every mount consumed inside the
manager, with no system mount left to transform — which indexes out of
range today. This is a distinct, pre-existing latent panic (for
example, reachable via WithTemporary combined with a transformed mount
type), tracked separately from the claim-handling fix above; guarding
it here only prevents a crash newly reachable by touching this line,
it does not address the mount.ActivationInfo.System non-empty
invariant that scenario also violates.
Signed-off-by: Derek McGowan <derek@mcg.dev>
Activate's planning phase — deciding, for each mount, which transforms
the manager must apply and whether the manager or the caller performs
the mount itself — was inline in a bolt transaction spanning hundreds
of lines, and every existing test of it goes through the full Activate
path, which needs root to mount anything for real.
Extract it into planActivation, a pure function of the mount list,
activation options, and registered transforms/handlers. No behavior
change; this is what makes the next commit's fix testable without
root.
Signed-off-by: Derek McGowan <derek@mcg.dev>
A caller that applies a mount transform itself had to say so by naming the
mount type "format/*", a sentinel the mount manager constructs internally
by concatenating a transform name with "/*". Both sides had to
independently know and agree on that convention, and the resulting string
is neither a mount type nor a transform name.
Add AllowTransforms, and WithAllowTransform to set it, so a transform can
be claimed by its own name instead. The "/*" convention is removed rather
than kept as a second accepted spelling: it existed only to be produced by
the runtime-allow-mounts annotation, which is removed in an earlier commit
in this series, and there is no other producer of it in this tree.
Signed-off-by: Derek McGowan <derek@mcg.dev>
parseStartResponse decodes a JSON bootstrap response into the deprecated
three-field client.BootstrapParams, so every other field of BootstrapResult
is silently discarded.
This is reached when reloading a bundle: writeBootstrapParams stores
bootstrap.json with encoding/json, and readBootstrapParams parses it back
through here. It matters once BootstrapResult carries extensions, because a
container joining an existing sandbox shim recovers that shim's result from
bootstrap.json rather than from a fresh shim start, and so would have seen
none of them.
Decode into BootstrapResult instead. The stored format does not change and
neither reader nor writer changes what it produces; encoding/json already
round-trips every field, including extensions, whose Any is an opaque type
URL and byte slice to it and so needs no type resolution, and including a
capability value this version of containerd does not define, which must
survive rather than being silently dropped.
This also removes the last use of the deprecated client.BootstrapParams.
Also fix TestRestoreBootstrapParams, which compared protobuf messages with
reflect-based deep equality. That is not sound, as generated types carry
internal state that varies with how the message was built.
Signed-off-by: Derek McGowan <derek@mcg.dev>
A shim declared the mount types it performs itself through a comma
separated string in the generic annotations map of its RuntimeInfo,
discovered by invoking the shim binary a second time with -info. The
values had to reproduce by hand the "<transform>/*" sentinel the mount
manager constructs internally, there was nowhere to put anything but a
name, and it was keyed by runtime name rather than shim instance, so it
could not reflect anything about the environment a particular shim is
running in.
Remove it along with loadShimInfo, shimInfo and the shimInfos cache,
and the runc.v2/runhcs.v1 shortcut that only existed to avoid paying
for it on the default runtimes. This is a breaking change targeted at
2.4; known usage is limited to Nerdbox, which can adopt the shim
capability protocol added in the following commits after this merges.
getRuntimeInfo itself stays: it is also used by PluginInfo and
validateRuntimeFeatures.
Signed-off-by: Derek McGowan <derek@mcg.dev>
The snapshot garbage collector holds the snapshotter write lock while
calling the underlying Snapshotter.Remove. cleanupSnapshotter also
detaches cancellation (context.WithoutCancel), so a Remove that never
returns - e.g. a proxy snapshotter whose RPC connection has wedged -
holds the write lock forever. Every subsequent snapshot operation on
the node then blocks behind the lock, including CRI RunPodSandbox,
turning the node into a permanent scheduling black hole while it still
reports Ready.
Observed in production with a proxy snapshotter (soci) that stopped
answering after a file-descriptor exhaustion storm: the GC goroutine
sat in WaitGroup.Wait for 4.5 hours with the Remove RPC parked in
waitOnHeader, and 17 CRI operations queued behind the metadata lock.
Bound each GC-path Remove with a timeout registered via pkg/timeout
(io.containerd.timeout.gc.snapshotter.remove, default 30m,
operator-tunable through config.toml like the other timeout keys). A
timed-out Remove returns DeadlineExceeded, which is not a
FailedPrecondition, so it propagates out of pruneBranch and ends the
pass: a globally wedged snapshotter costs one timeout per GC cycle
rather than one per orphaned snapshot, the deferred Cleanup is skipped
because the pass failed, and the snapshots left behind stay orphaned
for the next pass.
Signed-off-by: Nahum Litvin <nahuml@wix.com>
TaskService.Shutdown may be invoked multiple times. A grouped shim
itself does not terminate while it still has active tasks. It terminates only
after receiving a Shutdown request when no active tasks remain.
The shim manager should invoke TaskService.Shutdown after deleting every task,
regardless of whether the task represents sandbox or not. This ensures that the
shim receives Shutdown when the last active task is deleted.
In CRI, the pause container uses oom_score_adj=-998, making it unlikely to be
OOM-killed. However, it may still exit or be killed before the other containers
in the sandbox. SandboxService.Shutdown and TaskService.Shutdown are independent
APIs. Sandbox service should make sure there is no active containers
before shutdown. So, for the task service, we should invoke Shutdown for
each task.
Removing the supportSandboxAPIVersion check does not change behavior for shims
using task API versions below v3. Those shims were already forced down the
non-sandboxed path and received Shutdown after every task deletion. The behavior
change applies only to sandboxed tasks using task API v3, which now receive
Shutdown after deletion as well.
Signed-off-by: Wei Fu <fuweid89@gmail.com>
loadShim runs during runtime plugin initialization and issued its task
RPCs on the caller's context, which carries no deadline. A shim that
still owns its socket but has stopped serving, for example one leaked by
a previous containerd whose shutdown timed out, accepts the connection
and then never answers. containerd then never finishes starting and its
API socket is never created.
Give loadShim a single loadTimeout budget covering the whole load. Nested
timeouts can only shorten a deadline, so the load stays bounded however
many calls it makes.
Reaping a shim we give up on runs on its own cleanup budget, detached
from the caller so that a cancelled load cannot leave the shim behind. A
failed delete returns before shutting the shim down and closing its
client, so both are done here, and the failure is now propagated: delete
removes the bundle only when it succeeds, and returning an error lets
loadShims remove it, so a shim that cannot be reaped is no longer
reloaded on every subsequent start.
Co-authored-by: Harshal Patel <106813066+HarshalPatel1972@users.noreply.github.com>
Signed-off-by: Austin Vazquez <austin.vazquez@docker.com>
When a shim delete hits a timeout, currently the error message does not
indicate that the delete was killed rather than failed to complete.
Signed-off-by: Derek McGowan <derek@mcg.dev>
When a layer fails to prepare during parallel unpack we break out of the
launch loop but never return the error, so unpack() can report success and
label the image with a chainID that was never created. Keep the error and
return it once the already queued layers have been drained, so those still
commit as they would in sequential mode. Also end the layer's tracing span,
which leaked on this path.
Signed-off-by: Maksym Pavlenko <pavlenko.maksym@gmail.com>
Replace the snapshots.ErrAlreadyStaged sentinel error with a
read-only-mounts signal: a snapshotter (e.g. erofs serving a layer
content cache hit) now returns Prepare mounts normally, with no
error, when the layer content is already staged into the active
snapshot. The unpacker's isStaged helper checks the last mount's
ReadOnly() to decide whether to skip fetch+apply and just commit.
Signed-off-by: Maksym Pavlenko <pavlenko.maksym@gmail.com>
The cache used to serve a hit by committing the layer during Prepare and
returning ErrAlreadyExists, which is incompatible with parallel unpacking:
the "rebase" capability defers the parent to Commit time, so a
commit-at-Prepare layer ends up parentless. The erofs snapshotter therefore
had to disable rebase whenever the cache was enabled, making every cache
*miss* fall back to a fully sequential (slower-than-cold) pull.
Introduce snapshots.ErrAlreadyStaged, returned from Prepare to mean "the
active snapshot's content is staged; skip the layer download and apply, but
still Commit it" (where the parent is applied). Unlike ErrAlreadyExists it
does not end the layer's lifecycle. The unpacker handles it by emitting a
status whose bottom half runs the normal serialized commit (rebasing the
parent in), and the metadata snapshotter threads it through Prepare like a
normal active snapshot. The erofs cache hit now stages the blob and returns
ErrAlreadyStaged instead of committing, so it can advertise "rebase"
unconditionally: hits skip download+conversion and misses stay parallel.
Also promote the "rebase" capability string to snapshots.RebaseCap (shared by
the unpacker, erofs, and overlay).
Follow up: fetch is range-based, so only a fully cached contiguous
prefix skips downloads; a miss in a lower layer still pulls everything above
it. Per-layer fetch-skip is left as a follow-up.
Signed-off-by: Maksym Pavlenko <pavlenko.maksym@gmail.com>
The shim imported github.com/checkpoint-restore/checkpointctl/lib for a
single string constant.
Consumers that embed only the runtime v2 plugin (and never touch
internal/cri) therefore had to vendor the checkpointctl package.
Declare the file name as a local constant instead.
checkpointctl still remains a direct requirement here because
internal/cri/server uses its types, JSON readers, and annotations, but
it is no longer reachable from the runtime v2 import graph.
Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
Add a --dmverity flag to `ctr images build-erofs-cache`. When set, each cached
erofs blob is dm-verity formatted (the hash tree is appended in place) and a
.dmverity sidecar is written alongside it. This is required when the erofs
snapshotter runs with dmverity_mode=on, which rejects cache hits that lack a
sidecar; without it such layers would have to be formatted out-of-band.
Extract the differ's dm-verity formatting into a shared dmverity.FormatLayer so
the differ and the cache builder share one implementation; the differ's
formatDmverityLayer now delegates to it.
Signed-off-by: Maksym Pavlenko <pavlenko.maksym@gmail.com>
Add `ctr images build-erofs-cache <image> <cache_dir>`, which reads an
already-pulled image's layers from the content store, converts each into a
directly-mountable erofs blob, and writes them under
<cache_dir>/<algorithm>/<hex>.erofs keyed by the layer's diffID. This
populates the directory the erofs snapshotter's layer_content_cache reads
on pull; because the key is the source diffID, layers shared across images
converge on one blob. No converted image is produced.
Extract the per-layer uncompress + mkfs.erofs step out of LayerConvertFunc
into an exported ConvertLayerToErofs so the image converter and the cache
builder share a single conversion path.
Signed-off-by: Maksym Pavlenko <pavlenko.maksym@gmail.com>
Add a layer_content_cache option to the erofs snapshotter: a directory of
pre-converted, diffID-keyed erofs layer blobs (in production a read-only
mountpoint-s3/FUSE mount) that the snapshotter sources layers from on pull
instead of downloading and converting them per node.
On an image-layer extraction Prepare, if the layer's blob is present in the
cache the snapshotter stages it as a symlink, commits it as the target
chainID in the same transaction, and returns ErrAlreadyExists. This uses the
existing remote-snapshot protocol, so containerd skips both the layer
download and the tar->erofs conversion; no core changes are needed. Any miss
(cache disabled, no snapshot.ref/diff-id labels, blob absent, unreadable
cache) falls through to the normal path, so pulls keep working.
Signed-off-by: Maksym Pavlenko <pavlenko.maksym@gmail.com>
Right now the statement is treating any error from the shim as leaking
(len(pInfo == 0 is true for any error). It seems the intent was to only
treat shims this way if the error was not found, or if there's
legitimately no pids associated with the shim. Let's fix that up to
avoid orphaning shims that just had a small error in reading pids.
Link: https://github.com/containerd/containerd/issues/13784
Signed-off-by: Andrew Halaney <ahalaney@netflix.com>
Because selinux contexts can contain commas, context strings may be
quoted in the single-syscall mount API. This quoting is not permitted
when using the fsconfig API, so strip the quotes when preparing the
system call arguments.
Signed-off-by: Daniel De Graaf <dgdegra@uwe.nsa.gov>
Shim start output was trimmed before protobuf decoding.
Because arbitrary protobuf fields may legitimately end with whitespace
bytes, including `\n`, trimming could corrupt metadata or capabilities
and cause decoding to fail.
Pass the raw command output to `parseStartResponse` and attempt protobuf
decoding before modifying the response.
Whitespace trimming now applies only to legacy JSON and plain-address
responses.
Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
When a registry returns 403 Forbidden on a HEAD request (e.g., manifest
resolve or push existence check), the diagnostic error body is lost
because HEAD responses carry no body per HTTP spec. This leaves users
with an opaque "403 Forbidden" message and no actionable guidance.
Add a follow-up GET on HEAD 403 to retrieve the registry's OCI error
body. The existing unexpectedResponseErr machinery already parses the
body into structured errors — it just needs the body to be present. The
fallback lives in a shared withGETErrorBody helper used by both the
pusher and resolver: it only enriches when the GET also returns 403, and
preserves the original HEAD request's method and status while borrowing
just the body, so the resulting error's status and body stay consistent.
Scoped to 403 only because it is rare (CMK key disabled, IP firewall,
RBAC misconfiguration) and its body is highly diagnostic, while other
status codes either already use GET or have bodies that add no value.
Fixes#8969
Signed-off-by: Andrew Au <cshung@gmail.com>
Disable stat usage since we never consume the data.
This can reduce unnecessary contention during transactions.
Signed-off-by: Wei Fu <fuweid89@gmail.com>
Perform an early validation check on both container checkpoint and
restore paths to fail-fast if the CRIU binary is missing or is older
than the minimum required version (3.16.0).
To support runtime-configured environments, the validation respects the
custom PATH from the shim manager environment if configured, skipping
any relative paths to avoid incorrect daemon-relative resolution. If not
configured, it falls back to a standard system PATH lookup. The check
result is cached using sync.Once to prevent redundant process spawning.
Assisted-by: Antigravity
Signed-off-by: Samuel Karp <samuelkarp@google.com>
Extend the garbage-collection framework so a collectible resource can emit
forward references during graph traversal, in addition to the existing
back-reference mechanism.
A CollectionContext may now implement the optional collectionWithReferences
interface:
References(ctx context.Context, node gc.Node, fn func(gc.Node))
When the GC visits a node whose resource type was registered by an external
collector, gcContext.references consults the per-type References
implementation after the built-in core resource types are handled.
This is the forward-reference analogue of collectionWithBackRefs. Whereas
ActiveWithBackRefs must enumerate every edge up front and the gcContext
holds all of them in its backRefs map for the entire collection, References
is invoked on demand for a single node. A collector whose resources fan
out to many other nodes can therefore emit those edges without retaining
them in memory for the gc context.
This commit is intentionally a no-op: no plugin registers a collector that
uses collectionWithReferences yet. It is isolated here so that concurrent
development efforts that depend on this interface can be proposed and
reviewed upstream independently.
Signed-off-by: Derek McGowan <derek@mcg.dev>
Allow the last host to retry on transient network errors to incrase the
likelihood of the operation succeeding and help reduce flaky tests.
Signed-off-by: Derek McGowan <derek@mcg.dev>
TestImagesCreateUpdateDelete asserts that an image's updatedat is
strictly after its createdat. Both timestamps are stamped via
time.Now().UTC(), which strips the monotonic reading, so the comparison
falls back to the wall clock. On platforms with coarse timer resolution
(e.g. Windows, which advances system time at the ~15.6ms tick), the
Create and Update calls can land in the same tick and produce identical
timestamps, making the strict After() check fail intermittently.
Wait for the wall clock to advance past the creation timestamp before
updating so the assertion stays meaningful without depending on clock
resolution. On fine-resolution clocks the loop runs zero iterations.
Signed-off-by: Austin Vazquez <austin.vazquez@docker.com>
Some proxy stream setup and receive paths still returned raw RPC
status errors while neighboring proxy methods normalized them with
errgrpc.ToNative. This made errdefs checks depend on which proxy API
surfaced the same remote failure.
Normalize event subscription setup and receive errors, and streaming
stream creation errors, while preserving io.EOF for completed receive
streams.
Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
Most content proxy operations normalize remote RPC errors before
returning them, including stream receive errors from Walk and write
errors from the remote writer. remoteReaderAt.ReadAt was an outlier and
returned raw status errors from Read and Recv.
Callers that use content.ReadBlob through the proxy can then fail
errdefs checks, such as treating concurrent content deletion as
NotFound.
Convert non-EOF read stream errors with errgrpc.ToNative so ReaderAt
matches the rest of the content proxy while preserving io.EOF.
Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
Wei Fu