Export remote snapshotter label handler #8036

ktock · 2023-01-31T13:46:12Z

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.

thaJeztah · 2023-01-31T14:06:03Z

/cc @rumpl @tianon

k8s-ci-robot · 2023-01-31T14:06:06Z

@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.

Details

In response to this:

/cc @rumpl @tianon

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.

AkihiroSuda · 2023-01-31T14:33:25Z

pkg/snapshotters/annotations.go

+	// 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"
+)


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?

pkg/snapshotters/annotations_test.go

ktock · 2023-02-01T11:03:46Z

/retest

thaJeztah

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)

thaJeztah · 2023-02-01T11:48:15Z

pkg/snapshotters/annotations.go

+import (
+	"context"
+
+	containerdimages "github.com/containerd/containerd/images"


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.

thaJeztah · 2023-02-01T11:49:15Z

pkg/snapshotters/annotations.go

+	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"


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.

thaJeztah · 2023-02-01T11:50:52Z

pkg/snapshotters/annotations.go

+// 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


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?

thaJeztah · 2023-02-01T11:51:25Z

pkg/snapshotters/annotations.go

+	var item string
+	for _, l := range descs {
+		if containerdimages.IsLayerType(l.MediaType) {
+			item = l.Digest.String()


(see comment above)

Suggested change

item = l.Digest.String()

item := l.Digest.String()

thaJeztah · 2023-02-01T11:54:49Z

pkg/snapshotters/annotations.go

+			}
+			// 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())


As we're already using a structured log, perhaps set digest as a field, instead of Debugf()?

Suggested change

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?

thaJeztah · 2023-02-01T11:57:15Z

pkg/snapshotters/annotations_test.go

+	for _, tt := range tests {
+		tt := tt
+		t.Run(tt.name, func(t *testing.T) {
+			var sampleLayers []imagespec.Descriptor


nit: not super important for this size, but this could be;

Suggested change

var sampleLayers []imagespec.Descriptor

sampleLayers := make([]imagespec.Descriptor, 0, tt.layersNum)

thaJeztah · 2023-02-01T12:06:26Z

pkg/snapshotters/annotations.go

+						}
+						c.Annotations[TargetRefLabel] = ref
+						c.Annotations[TargetLayerDigestLabel] = c.Digest.String()
+						c.Annotations[TargetImageLayersLabel] = getLayers(ctx, TargetImageLayersLabel, children[i:], labels.Validate)


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).

Thank you for the suggestion. Let's work on it in a follow-up.

👍 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]>

ktock · 2023-02-01T14:59:22Z

@thaJeztah Thank you for the review. Fixed the patch.

thaJeztah

LGTM, thank you!

thaJeztah · 2023-02-01T15:15:05Z

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!)

estesp

LGTM

ktock mentioned this pull request Jan 31, 2023

c8d/pull: Add CRI-compatible annotation of pulled content moby/moby#44810

Merged

ktock force-pushed the remotesnlabel branch 3 times, most recently from b3431cd to 9c3f4d8 Compare January 31, 2023 14:04

k8s-ci-robot requested a review from tianon January 31, 2023 14:06

AkihiroSuda reviewed Jan 31, 2023

View reviewed changes

ktock force-pushed the remotesnlabel branch from 9c3f4d8 to bd0ae31 Compare January 31, 2023 14:47

AkihiroSuda approved these changes Jan 31, 2023

View reviewed changes

dcantah approved these changes Feb 1, 2023

View reviewed changes

fuweid reviewed Feb 1, 2023

View reviewed changes

pkg/snapshotters/annotations_test.go Show resolved Hide resolved

ktock force-pushed the remotesnlabel branch from bd0ae31 to c899448 Compare February 1, 2023 10:04

thaJeztah reviewed Feb 1, 2023

View reviewed changes

Export remote snapshotter label handler

dbf384a

Signed-off-by: Kohei Tokunaga <[email protected]>

ktock force-pushed the remotesnlabel branch from c899448 to dbf384a Compare February 1, 2023 14:03

thaJeztah approved these changes Feb 1, 2023

View reviewed changes

fuweid approved these changes Feb 2, 2023

View reviewed changes

estesp approved these changes Feb 2, 2023

View reviewed changes

estesp merged commit 6116820 into containerd:main Feb 2, 2023

ktock deleted the remotesnlabel branch February 2, 2023 23:36

thaJeztah mentioned this pull request Feb 6, 2023

[release/1.6 backport] Export remote snapshotter label handler #8054

Merged

AkihiroSuda mentioned this pull request Feb 21, 2023

Snapshotters: use containerd's AppendInfoHandlerWrapper containerd/nerdctl#2040

Closed

ktock mentioned this pull request Mar 1, 2023

remote snapshotters: migrate to containerd-based label management containerd/nerdctl#2063

Merged

adamqqqplay mentioned this pull request Mar 23, 2023

Move images annotations that need to be passed to snapshotter to snapshotter repo dragonflyoss/nydus#264

Closed

	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")

	var sampleLayers []imagespec.Descriptor
	sampleLayers := make([]imagespec.Descriptor, 0, tt.layersNum)

Export remote snapshotter label handler #8036

Export remote snapshotter label handler #8036

Uh oh!

Conversation

ktock commented Jan 31, 2023

Uh oh!

thaJeztah commented Jan 31, 2023

Uh oh!

k8s-ci-robot commented Jan 31, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ktock commented Feb 1, 2023

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ktock commented Feb 1, 2023

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Feb 1, 2023

Uh oh!

estesp left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants