-
Notifications
You must be signed in to change notification settings - Fork 717
Refactor image management to use transfer service #4583
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
285c273 to
156a92a
Compare
b5584fe to
18b0448
Compare
a49ea0a to
f66db0d
Compare
f66db0d to
d36732a
Compare
|
Most image management operations now use the transfer service:
LimitationsDue to current transfer API constraints, the following features continue to use the legacy implementation: 1. IPFS Registry and Nondistributable ArtifactsReason: The transfer service API does not support custom registry resolver implementations. Affected features:
Status: These features remain on the legacy resolver-based approach until the transfer API supports custom resolvers. 2. Containerd 1.7.x Compatibility IssueIssue: When using containerd 1.7.x daemons, registry options like Technical details:
Impact:
|
|
For limitation 2, there are two potential solutions:
For limitation 1, there are two potential solutions:
|
|
Thanks @ChengyuZhu6
2 in the long term, 1 in the short term |
d36732a to
c5552d0
Compare
c5552d0 to
95e03e0
Compare
f2e210d to
d3b9cab
Compare
Switch image operations to the transfer API with structured progress reporting and improved TLS/HTTP fallback behavior. Introduce shared helpers for credentials, error classification, progress rendering, and transfer-based import/tag/save flows, updating tests to reflect the new UX. Signed-off-by: ChengyuZhu6 <hudson@cyzhu.com>
Add remote snapshot annotations and transfer unpack config for stargz, soci, and fuse-overlayfs snapshotter plugins. Signed-off-by: ChengyuZhu6 <hudson@cyzhu.com>
d3b9cab to
d099f97
Compare
Add version detection to automatically select Transfer service (2.0+) or legacy resolver methods (< 2.0) for better compatibility. Signed-off-by: ChengyuZhu6 <hudson@cyzhu.com>
d099f97 to
3341fbd
Compare
| enable_remote_snapshot_annotations = "true" | ||
| [[plugins."io.containerd.transfer.v1.local".unpack_config]] | ||
| platform = "linux" | ||
| snapshotter = "overlayfs" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if this config is not specified?
i.e., what happens with the default config?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By default, it would be
[[plugins."io.containerd.transfer.v1.local".unpack_config]]
platform = "linux/amd64"(matches the host's OS/Architecture)
snapshotter = "overlayfs"
differ = "walking"
| "github.com/containerd/containerd/v2/pkg/progress" | ||
| ) | ||
|
|
||
| // From https://github.com/containerd/containerd/blob/v2.2.0-rc.0/cmd/ctr/commands/image/pull.go#L240-L473 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't we import the package from containerd/containerd to reduce code clones?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The progress handling code is in cmd/ctr/commands/images, not in pkg/ or core/, so it cannot be imported directly.
And I removed displayName() function that splits names by '-':
func displayName(name string) string {
parts := strings.Split(name, "-")
for i := range parts {
parts[i] = shortenName(parts[i])
}
return strings.Join(parts, " ")
}
which would break display for image names containing - (e.g., ubuntu-base )
| } | ||
|
|
||
| // SupportsFullTransferService checks if the containerd version fully supports the Transfer service. | ||
| // While containerd 1.7 has Transfer service, full support is only available in 2.0+. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // While containerd 1.7 has Transfer service, full support is only available in 2.0+. | |
| // While containerd 1.7 has Transfer service, full support is only available in 2.0+. | |
| // The following features are missing in containerd 1.7: | |
| // - [ADD MISSING FEATURES] |
| snapshotter = "fuse-overlayfs" | ||
| [[plugins."io.containerd.transfer.v1.local".unpack_config]] | ||
| platform = "linux" | ||
| snapshotter = "overlayfs" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a fan of complicating the config.
What happens if these lines are missing?
ctr doesn't need them AFAICS?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By default, it would be
[[plugins."io.containerd.transfer.v1.local".unpack_config]]
platform = "linux/amd64"(matches the host's OS/Architecture)
snapshotter = "overlayfs"
differ = "walking"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This config is required to specify which snapshotter the transfer service should use when unpacking images. We define multiple unpack targets:
- overlayfs for standard OCI images
- stargz / soci for lazy-pulling support
Without this, containerd defaults to basic unpacking and won't leverage stargz/soci snapshotters needed for lazy-pulling.
And if we only configure stargz in unpack_config, then all images would be forced to use the stargz snapshotter during transfer/unpacking, even standard OCI images that don't support lazy-pulling.
Fixes: #4573