Export remote snapshotter label handler#8036
Conversation
ktock
force-pushed
the
remotesnlabel
branch
3 times, most recently
from
b3431cd to
9c3f4d8
Compare
|
@thaJeztah: GitHub didn't allow me to request PR reviews from the following users: rumpl. Note that only containerd members and repo collaborators can review this PR, and authors cannot review their own PRs. DetailsIn response to this: Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
| // the target image and will be passed to snapshotters for preparing layers in | ||
| // parallel. Skipping some layers is allowed and only affects performance. | ||
| TargetImageLayersLabel = "containerd.io/snapshot/cri.image-layers" | ||
| ) |
There was a problem hiding this comment.
Can we add a comment line to explain that these labels are not specific to CRI, but we have to retain "cri" in the label strings for historical reason?
|
/retest |
thaJeztah
left a comment
thaJeztah
left a comment
There was a problem hiding this comment.
Overall looks good (thanks!) I left some comments/suggestions; (most of) those were already in the existing code (and one of them definitely not needed for this PR), but perhaps now is a good opportunity to fix them up (no strong blockers though)
pkg/snapshotters/annotations.go
Outdated
| import ( | ||
| "context" | ||
|
|
||
| containerdimages "github.com/containerd/containerd/images" |
There was a problem hiding this comment.
Looks like in most places (except for 1 or 2 where we must use an alias) we don't use an alias for this; so I would suggest not aliasing the import here.
pkg/snapshotters/annotations.go
Outdated
| containerdimages "github.com/containerd/containerd/images" | ||
| "github.com/containerd/containerd/labels" | ||
| "github.com/containerd/containerd/log" | ||
| imagespec "github.com/opencontainers/image-spec/specs-go/v1" |
There was a problem hiding this comment.
In most places, we use ocispec as alias for this one. We should align those aliases for places where we use a different alias, but as this is a new package, it would be good to change this to ocispec.
pkg/snapshotters/annotations.go
Outdated
| // descriptors. The returned list contains as many digests as possible as well | ||
| // as meets the label validation. | ||
| func getLayers(ctx context.Context, key string, descs []imagespec.Descriptor, validate func(k, v string) error) (layers string) { | ||
| var item string |
There was a problem hiding this comment.
Looks like this variable is only used inside the loop, and inside the if branch. Probably better to define it inside the loop as a local variable?
pkg/snapshotters/annotations.go
Outdated
| var item string | ||
| for _, l := range descs { | ||
| if containerdimages.IsLayerType(l.MediaType) { | ||
| item = l.Digest.String() |
There was a problem hiding this comment.
(see comment above)
| item = l.Digest.String() | |
| item := l.Digest.String() |
pkg/snapshotters/annotations.go
Outdated
| } | ||
| // This avoids the label hits the size limitation. | ||
| if err := validate(key, layers+item); err != nil { | ||
| log.G(ctx).WithError(err).WithField("label", key).Debugf("%q is omitted in the layers list", l.Digest.String()) |
There was a problem hiding this comment.
As we're already using a structured log, perhaps set digest as a field, instead of Debugf()?
| log.G(ctx).WithError(err).WithField("label", key).Debugf("%q is omitted in the layers list", l.Digest.String()) | |
| log.G(ctx).WithError(err).WithField("label", key).WithField("digest", l.Digest.String()).Debug("omitting digest in the layers list") |
How likely is this to happen? Should this be an Info() or would that be too noisy?
pkg/snapshotters/annotations_test.go
Outdated
| for _, tt := range tests { | ||
| tt := tt | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| var sampleLayers []imagespec.Descriptor |
There was a problem hiding this comment.
nit: not super important for this size, but this could be;
| var sampleLayers []imagespec.Descriptor | |
| sampleLayers := make([]imagespec.Descriptor, 0, tt.layersNum) |
| } | ||
| c.Annotations[TargetRefLabel] = ref | ||
| c.Annotations[TargetLayerDigestLabel] = c.Digest.String() | ||
| c.Annotations[TargetImageLayersLabel] = getLayers(ctx, TargetImageLayersLabel, children[i:], labels.Validate) |
There was a problem hiding this comment.
This can be done in a follow-up, but we should look at getLayers() at it looks like we're effectively doing the same handling for every child (but each time with a shorter list). I have a feeling we can optimise some of that (but need to have a closer look, and not important for now).
There was a problem hiding this comment.
Thank you for the suggestion. Let's work on it in a follow-up.
There was a problem hiding this comment.
👍 I didn't fully look at it, or if it would save a lot of allocations, so it definitely may be a "micro optimisation", so not a top-priority (could be a "lunch-time" exercise for fun).
Signed-off-by: Kohei Tokunaga <[email protected]>
|
@thaJeztah Thank you for the review. Fixed the patch. |
|
I wonder if this could make sense for a 1.6 patch release if it would help transitioning. Overall it seems fairly low risk, but it's not a "bug-fix" or "security-fix", so not sure if it would qualify (would be nice though!) |
7 participants
Now the user of remote snapshotter labels appended by CRI plugin is not only stargz-snapshotter but also several remote snapshotter implementations in the community. (There is also discussion to use them from non-snapshotter tool moby/moby#44810)
Currently, the code is defined as private variables/functions so projects outside containerd need to copy-and-paste them. These clones aren't easy to maintain because they need to be copy-and-pasted again every time changes occur on the upstream code.
This PR exports remote snapshotter's label-related logic and tries to make it easier to maintain tools around remote snapshotters.