Skip to content

Add manifest digest annotation for snapshotters#4610

Merged
AkihiroSuda merged 1 commit intocontainerd:masterfrom
shahzzzam:samashah/add-annotations
Oct 28, 2020
Merged

Add manifest digest annotation for snapshotters#4610
AkihiroSuda merged 1 commit intocontainerd:masterfrom
shahzzzam:samashah/add-annotations

Conversation

@shahzzzam
Copy link
Copy Markdown
Contributor

@shahzzzam shahzzzam commented Oct 7, 2020

Signed-off-by: Samarth Shah [email protected]

We leverage stargz-based snapshotter and have implemented our own Filesystem interface. We need the image digest to get some metadata for the image itself.

cc: @cpuguy83 @juliusl @northtyphoon @mikebrow

@k8s-ci-robot
Copy link
Copy Markdown

k8s-ci-robot commented Oct 7, 2020

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

Details

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.

@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Oct 7, 2020

Build succeeded.

@shahzzzam
Copy link
Copy Markdown
Contributor Author

shahzzzam commented Oct 9, 2020

@dmcgowan

mikebrow
mikebrow approved these changes Oct 9, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@mikebrow mikebrow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mikebrow
Copy link
Copy Markdown
Member

mikebrow commented Oct 9, 2020

carry over from: containerd/cri#1585 where derek had the following question...

shahzzzam: We leverage stargz-based snapshotter and have implemented our own Filesystem interface. We need the image digest to get some metadata for the image itself.

Derek: What sort of metadata? I ask because "image digest" can be an ambiguous term since it is interchangeably used to refer to a manifest index/list digest, manifest digest, or image config digest. Generally speaking I would like to figure out cleaner ways for stargz to operate with metadata rather than feeding it more metadata labels, not sure where to draw the line on how it is now.

@shahzzzam
Copy link
Copy Markdown
Contributor Author

shahzzzam commented Oct 12, 2020

@dmcgowan makes sense. I renamed it to "containerd.io/snapshot/cri.manifest-digest" to be more specific. Its the manifest digest that we need.

cpuguy83
cpuguy83 reviewed Oct 14, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@cpuguy83 cpuguy83 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM, but I am very close to this change.

The idea here is that the custom snapshotter needs the context of all the layers to know if it has the data available or the layer needs to be pulled to the node.

@ktock
Copy link
Copy Markdown
Member

ktock commented Oct 15, 2020

@shahzzzam LGTM about this change:+1: but I'm curious about which (and how) metadata in the manifest are consumed in your snapshotter.

fuweid
fuweid approved these changes Oct 15, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@fuweid fuweid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@fuweid fuweid requested a review from dmcgowan October 15, 2020 13:00
@shahzzzam
Copy link
Copy Markdown
Contributor Author

shahzzzam commented Oct 16, 2020

@ktock @dmcgowan Teleport snapshot uses the manifest digest to uniquely identify the image and pull the layers from azure container registry remote image store.

@shahzzzam
Copy link
Copy Markdown
Contributor Author

shahzzzam commented Oct 23, 2020

Any updates on this?

ktock
ktock approved these changes Oct 23, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@ktock ktock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

And I also agree with that we should seek a cleaner way to pass metadata from client to the snapshotter (maybe after this patch).

Possible solutions:

  • Enabling to filter snapshot labels in config.toml (easier) or
  • storing an all-in-one JSON blob (like the following) to the content store and pass only a digest label of that JSON blob to the snapshotter (maybe requires further discussion)
type SnapshotTarget struct {
	ImageRef     string             `json:"image_ref"`
	LayerDesc    ocispec.Descriptor `json:"layer_desc"`
	ManifestDesc ocispec.Descriptor `json:"manifest_desc"`
	Platform     ocispec.Platform   `json:"platform"`
}

WDYT? @dmcgowan

@shahzzzam
Copy link
Copy Markdown
Contributor Author

shahzzzam commented Oct 24, 2020

@ktock in the 1st solution the snapshotters will only have to parse the labels. In the 2nd solution, they will have to connect to containerd for ContentStore, right?

@shahzzzam
Copy link
Copy Markdown
Contributor Author

shahzzzam commented Oct 26, 2020

Ping

@ktock
Copy link
Copy Markdown
Member

ktock commented Oct 27, 2020

@shahzzzam

in the 1st solution the snapshotters will only have to parse the labels.

Yes. 1st one won't require any changes to the snapshotters. Users just add some filter expression (or label key names, etc) to the config.toml, based on the snapshoter they want to use. This improves the current default behaviour where many labels are appended to snapshots even if users don't use remote snapshots.

In the 2nd solution, they will have to connect to containerd for ContentStore, right?

Yes.

@shahzzzam
Copy link
Copy Markdown
Contributor Author

shahzzzam commented Oct 28, 2020 •
edited
Loading

@ktock Sure, i am interested and can take a look at it later.

For now, can you please help approve and merge this change?

@AkihiroSuda
Copy link
Copy Markdown
Member

AkihiroSuda commented Oct 28, 2020

/ok-to-test

@AkihiroSuda AkihiroSuda merged commit 8ff2707 into containerd:master Oct 28, 2020
@shahzzzam shahzzzam deleted the samashah/add-annotations branch October 28, 2020 07:12
@ktock ktock mentioned this pull request Oct 28, 2020
Merged
@cpuguy83
Copy link
Copy Markdown
Member

cpuguy83 commented Oct 28, 2020

We'd like to backport this change since it's necessary for Azure's snapshotter.

@shahzzzam
Copy link
Copy Markdown
Contributor Author

shahzzzam commented Nov 16, 2020

@cpuguy83 Can you please help us backport this to 1.4?

@shahzzzam shahzzzam mentioned this pull request Nov 17, 2020
Closed
@thaJeztah
Copy link
Copy Markdown
Member

thaJeztah commented Nov 19, 2020

this was cherry-picked in containerd/cri#1609, but still needs a revendor of containerd/cri

@thaJeztah thaJeztah added cherry-picked/1.4.x PR commits are cherry picked into the release/1.4 branch and removed cherry-pick/1.4.x labels Nov 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cpuguy83 cpuguy83 cpuguy83 left review comments

@AkihiroSuda AkihiroSuda AkihiroSuda approved these changes

@mikebrow mikebrow mikebrow approved these changes

@fuweid fuweid fuweid approved these changes

@ktock ktock ktock approved these changes

@dmcgowan dmcgowan Awaiting requested review from dmcgowan

Assignees

No one assigned

Labels

cherry-picked/1.4.x PR commits are cherry picked into the release/1.4 branch ok-to-test

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@shahzzzam @k8s-ci-robot @mikebrow @ktock @AkihiroSuda @cpuguy83 @thaJeztah @fuweid