Limit size of additional annotation for avoiding unpack failure #1572

ktock · 2020-09-10T12:22:51Z

Related: containerd/stargz-snapshotter#144

In containerd, there is a size limit for label size (4096 chars).
If an image has many layers (> (4096-39)/72 > 56), containerd.io/snapshot/cri.image-layers will hit the limit of label size and the unpack will fail because the annotation will be passed to the snapshotter as a label.
This commit fixes this by limiting the size of the annotation.

k8s-ci-robot · 2020-09-10T12:22:59Z

Hi @ktock. Thanks for your PR.

I'm waiting for a containerd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

jianzi123 · 2020-09-11T03:03:56Z

I wonder if we need to omit some labels or if we need to change the logic of the valid method.

ktock · 2020-09-11T03:13:39Z

This takes the former approach (omitting the contents of the label). The Validate method is used globally so changing the logic isn't easy.
Maybe we should add validation when inheriting annotations as labels and filter invalid annotations out. But let's keep it separated from this PR now.

mikebrow

after deciding what the plan is with respect to what happens if a layer goes over, should we have more labels etc., can you add a test bucket for this to match up expectations with output?

mikebrow · 2020-09-11T13:53:43Z

pkg/server/image_pull.go

+							if containerdimages.IsLayerType(l.MediaType) {
+								ls := fmt.Sprintf("%s,", l.Digest.String())
+								// This avoids the label hits the size limitation.
+								if err := labels.Validate(targetImageLayersLabel, layers+ls); err != nil {


I think you want labels.Validate(targetImageLayersLabel, layers+l.Digest.String())

mikebrow · 2020-09-11T13:57:27Z

pkg/server/image_pull.go

+								ls := fmt.Sprintf("%s,", l.Digest.String())
+								// This avoids the label hits the size limitation.
+								if err := labels.Validate(targetImageLayersLabel, layers+ls); err != nil {
+									break


i think we need a debug out here indicating why the layer is missing.. but that in and of itself points out that we may need a new plan with additional labels.. either one per layer or some other idea

mikebrow · 2020-09-11T14:01:18Z

pkg/server/image_pull.go

+								layers += ls
+							}
+						}
+						c.Annotations[targetImageLayersLabel] = strings.TrimSuffix(layers, ",")


can we switch to prefix comma? iow validate labels.Validate(targetImageLayersLabel, layers+","+l.Digest.String()) and if that is fine += ","+l.Digest.String())..

ktock · 2020-09-14T07:29:36Z

@mikebrow Thanks for the comments.

after deciding what the plan is with respect to what happens if a layer goes over

As mentioned in the above, I think we should add validation when inheriting annotations as labels and filter invalid annotations out. How do you think about it? I'll open another PR (on containerd) if we need this.

can you add a test bucket for this to match up expectations with output?

The label containerd.io/snapshot/cri.image-layers is used by stargz snapshotter to resolve layers in an image in parallel and incomplete (even if empty) list is allowed (but it possibly slows down the snapshot preparation). So in this label's case, if the label size of a layer goes over the limit, I think that simply omitting the content of the label is enough. I added a small unit test for this. Please tell me if I'm missing something or should add more tests, etc.

mikebrow · 2020-09-15T00:46:18Z

@mikebrow Thanks for the comments.

after deciding what the plan is with respect to what happens if a layer goes over

As mentioned in the above, I think we should add validation when inheriting annotations as labels and filter invalid annotations out. How do you think about it? I'll open another PR (on containerd) if we need this.

can you add a test bucket for this to match up expectations with output?

The label containerd.io/snapshot/cri.image-layers is used by stargz snapshotter to resolve layers in an image in parallel and incomplete (even if empty) list is allowed (but it possibly slows down the snapshot preparation). So in this label's case, if the label size of a layer goes over the limit, I think that simply omitting the content of the label is enough. I added a small unit test for this. Please tell me if I'm missing something or should add more tests, etc.

I see so perf only not critical. Just a warning or informational message should be fine then. Perhaps on a if first check.

mikebrow · 2020-09-15T00:49:03Z

/ok-to-test

mikebrow

looking good! Just a nit on the log level..

mikebrow · 2020-09-15T13:35:22Z

pkg/server/image_pull.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).Infof("%q is omitted in the layers list", l.Digest.String())


maybe debug?

In containerd, there is a size limit for label size (4096 chars). Currently if an image has many layers (> (4096-39)/72 > 56), `containerd.io/snapshot/cri.image-layers` will hit the limit of label size and the unpack will fail. This commit fixes this by limiting the size of the annotation. Signed-off-by: Kohei Tokunaga <[email protected]>

mikebrow

LGTM
on green

cpuguy83 · 2020-12-03T17:35:11Z

pkg/server/image_pull.go

@@ -495,11 +487,33 @@ func appendInfoHandlerWrapper(ref string) func(f containerdimages.Handler) conta
 						}
 						c.Annotations[targetRefLabel] = ref
 						c.Annotations[targetDigestLabel] = c.Digest.String()
-						c.Annotations[targetImageLayersLabel] = layers
+						c.Annotations[targetImageLayersLabel] = getLayers(ctx, targetImageLayersLabel, children[i:], labels.Validate)


This seems like an extra change that doesn't really fix the problem, except maybe now we store less redundant data.

"This" being changing from all layers to just children.

Could you elaborate this? Doesn't calling getLayers on children avoids the problem of hitting the label limit (see L511) ?

I was under the impression that the limit was per key, not total?

Currently, the limit is per key. If we need to have the "total" label size limit, what should be the limit (instead of 4096 chars per key)?

The way we have this now the root gets all the layers anyway, so the way we are doing children[:i] does not really fix the label size problem.

It is handled by truncating the values in getLayers, however.

So, 1.4.2 changed the behavior even for values that do not hit the limit.

getLayers solves the label limit problem but children[:i] isn't for avoiding the label size limit. containerd.io/snapshot/cri.image-layers is currently a label for enabling snapshotter to pre-process(resolve) layers related to the target layer in parallel. So this is for optimization.

The reason why I do children[:i] here is otherwise children[0:limit] is always passed to the snapshotter and it can't get chance to pre-process children[limit:] layers.

However I agree with that we should find the better ways instead of children[:i].

aojea · 2021-10-30T17:40:47Z

@mikebrow can you please take a look at this kubernetes-sigs/kind#2518 and check if there was a regression?

ktock · 2021-10-31T05:38:52Z

@aojea Please see containerd/containerd#6187.

aojea · 2021-10-31T11:35:50Z

@aojea Please see containerd/containerd#6187.

❤️

k8s-ci-robot added needs-ok-to-test size/S labels Sep 10, 2020

mikebrow reviewed Sep 11, 2020

View reviewed changes

ktock changed the title ~~Limit size of additional annotation for avoiding unpack failure~~ [WIP] Limit size of additional annotation for avoiding unpack failure Sep 14, 2020

ktock force-pushed the layers branch from ec1f874 to 6b13a0e Compare September 14, 2020 06:55

k8s-ci-robot added size/M and removed size/S labels Sep 14, 2020

ktock force-pushed the layers branch from 6b13a0e to 00f84a2 Compare September 14, 2020 07:21

ktock changed the title ~~[WIP] Limit size of additional annotation for avoiding unpack failure~~ Limit size of additional annotation for avoiding unpack failure Sep 14, 2020

k8s-ci-robot added ok-to-test and removed needs-ok-to-test labels Sep 15, 2020

ktock force-pushed the layers branch from 00f84a2 to 58d9ee3 Compare September 15, 2020 05:45

ktock requested a review from mikebrow September 15, 2020 12:35

mikebrow reviewed Sep 15, 2020

View reviewed changes

ktock force-pushed the layers branch from 58d9ee3 to e571fd8 Compare September 15, 2020 13:47

mikebrow approved these changes Sep 15, 2020

View reviewed changes

mikebrow merged commit ffe7970 into containerd:master Sep 15, 2020

ktock deleted the layers branch September 16, 2020 00:20

ktock mentioned this pull request Sep 17, 2020

vendor: update containerd/cri 210a86ca5bf6c8ca5f2553272d72c774b21fdec2 containerd/containerd#4570

Merged

ericpromislow mentioned this pull request Sep 17, 2020

cf push fails on KinD: cf-default-builder image "labels" size greater than max allowed size cloudfoundry/cf-for-k8s#444

Closed

ktock mentioned this pull request Sep 18, 2020

use stargz, find some error: label key and value greater than maximum size (4096bytes) containerd/stargz-snapshotter#144

Closed

dmcgowan mentioned this pull request Oct 28, 2020

Update make snapshot annotations disabled by default containerd/containerd#4665

Merged

estesp mentioned this pull request Nov 2, 2020

"Label key and value greater than maximum size (4096 bytes)" error when pulling image with many layers containerd/containerd#4684

Closed

cpuguy83 reviewed Dec 3, 2020

View reviewed changes

adamscott mentioned this pull request Dec 5, 2020

Enable kind v0.9.0 config fix tilt-dev/ctlptl#70

Closed

renovate bot mentioned this pull request May 25, 2021

Update dependency kubernetes-sigs/kind to v0.11.0 jpbaudoin/k8s-tools#1

Merged

1 task

Birdrock mentioned this pull request Oct 27, 2021

kindest/node:v1.22.2 -> label key and value greater than maximum size (4096 bytes), key: containerd: invalid argument kubernetes-sigs/kind#2518

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Limit size of additional annotation for avoiding unpack failure #1572

Limit size of additional annotation for avoiding unpack failure #1572

ktock commented Sep 10, 2020 •

edited

Loading

k8s-ci-robot commented Sep 10, 2020

jianzi123 commented Sep 11, 2020

ktock commented Sep 11, 2020 •

edited

Loading

mikebrow left a comment

mikebrow Sep 11, 2020

mikebrow Sep 11, 2020

mikebrow Sep 11, 2020

ktock commented Sep 14, 2020

mikebrow commented Sep 15, 2020

mikebrow commented Sep 15, 2020

mikebrow left a comment

mikebrow Sep 15, 2020

mikebrow left a comment

cpuguy83 Dec 3, 2020

cpuguy83 Dec 3, 2020

ktock Dec 4, 2020

cpuguy83 Dec 16, 2020

ktock Dec 16, 2020

cpuguy83 Dec 16, 2020

ktock Dec 16, 2020

aojea commented Oct 30, 2021

ktock commented Oct 31, 2021

aojea commented Oct 31, 2021

Limit size of additional annotation for avoiding unpack failure #1572

Limit size of additional annotation for avoiding unpack failure #1572

Conversation

ktock commented Sep 10, 2020 • edited Loading

k8s-ci-robot commented Sep 10, 2020

jianzi123 commented Sep 11, 2020

ktock commented Sep 11, 2020 • edited Loading

mikebrow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ktock commented Sep 14, 2020

mikebrow commented Sep 15, 2020

mikebrow commented Sep 15, 2020

mikebrow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikebrow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aojea commented Oct 30, 2021

ktock commented Oct 31, 2021

aojea commented Oct 31, 2021

ktock commented Sep 10, 2020 •

edited

Loading

ktock commented Sep 11, 2020 •

edited

Loading