Skip to content
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

Enable to pass additional handler on pull for stargz-based remote snapshots #1431

Merged
merged 1 commit into from Apr 16, 2020

Conversation

@ktock
Copy link
Contributor

ktock commented Apr 8, 2020

Recently, we started a non-core subproject stargz-snapshotter aiming to speed up pull operation and we are seeing performance improvement on containerd.Client's Pull operation as the following (measured on Github Actions with Docker Hub. please also see README for more details).

The benchmarking result on 288c338

We've implemented the fundamental features and stargz snapshotter passes critest with a patched version of CRI plugin. Now it's ready for being experimentally used on k8s-based workloads and seeking adoptabilities on it.

Stargz snapshotter is implemented as a standard proxy plugin but because it queries layer contents to the registry it requires containerd clients (including CRI plugin) to pass additional information (image ref and layer digest) for each pull operation through an image handler.

This commit enables to use stargz-based remote snapshots through CRI optionally, by integrating the image handler with CRI. You can test this patch with this node image, which is tested using critest.

@k8s-ci-robot
Copy link
Collaborator

k8s-ci-robot commented Apr 8, 2020

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.

@k8s-ci-robot k8s-ci-robot added the size/L label Apr 8, 2020
@mikebrow
Copy link
Member

mikebrow commented Apr 8, 2020

/ok-to-test

Copy link
Member

mikebrow left a comment

I may be missing something, but it looks like all you are doing with the additional boolean is injecting a couple annotations? I wonder if there isn't a more generic way to add the desired annotations.

@@ -120,6 +121,10 @@ func (c *criService) PullImage(ctx context.Context, r *runtime.PullImageRequest)
}

pullOpts = append(pullOpts, c.encryptedImagesPullOpts()...)
if c.config.ContainerdConfig.StargzSnapshots {
pullOpts = append(pullOpts,
containerd.WithImageHandlerWrapper(stargz.AppendInfoHandlerWrapper(ref)))

This comment has been minimized.

@fuweid

fuweid Apr 8, 2020 Member

AppendInfoHandlerWrapper is to pass image ref/digest into snapshotter. It looks for more generic and it can be independent handler in cri-containerd.

IMO, we can define more generics labels which can be passed into snapshotter, not just stargz :)

@AkihiroSuda
Copy link
Member

AkihiroSuda commented Apr 8, 2020

@mikebrow

I wonder if there isn't a more generic way to add the desired annotations.

containerd/containerd#4044

pkg/config/config.go Outdated Show resolved Hide resolved
@ktock
Copy link
Contributor Author

ktock commented Apr 9, 2020

Thanks for the feedback!

@mikebrow Yes, the new flag is for injecting some annotations. I'm looking for more generic ways on containerd/containerd#4044 (the list of other ideas on containerd/containerd#4044 (comment)) but so far we haven't reached to the consensus yet.

I also like @fuweid's idea "define more generics labels which can be passed into snapshotter". Can we define the following labels?

  • "containerd.io/snapshot/remote/reference" for image reference
  • "containerd.io/snapshot/remote/digest" for layer digest
@ktock ktock closed this Apr 9, 2020
@ktock ktock reopened this Apr 9, 2020
@ktock
Copy link
Contributor Author

ktock commented Apr 9, 2020

And sorry for accidentally closing the PR. I clicked the wrong button 🙏

@ktock ktock force-pushed the ktock:stargz branch from 4194412 to b530b42 Apr 9, 2020
@mikebrow
Copy link
Member

mikebrow commented Apr 9, 2020

Thanks for the feedback!

@mikebrow Yes, the new flag is for injecting some annotations. I'm looking for more generic ways on containerd/containerd#4044 (the list of other ideas on containerd/containerd#4044 (comment)) but so far we haven't reached to the consensus yet.

I also like @fuweid's idea "define more generics labels which can be passed into snapshotter". Can we define the following labels?

  • "containerd.io/snapshot/remote/reference" for image reference
  • "containerd.io/snapshot/remote/digest" for layer digest

Off the top of my head maybe:
"io.kubernetes.cri.image-ref", "io.kubernetes.cri.layer-digest"

Then some way to turn it off disable_adding_additional_annotations_to_image_layers (default false...) Can't think of a reason to not add them by default.

@ktock
Copy link
Contributor Author

ktock commented Apr 10, 2020

Thanks for the suggestion!
I'll fix this patch to pass labels by default ("io.kubernetes.cri.image-ref", "io.kubernetes.cri.layer-digest") and to add a flag for disabling this.

@ktock ktock force-pushed the ktock:stargz branch from b530b42 to a61b5db Apr 10, 2020
@k8s-ci-robot k8s-ci-robot added size/M and removed size/L labels Apr 10, 2020
pkg/config/config.go Outdated Show resolved Hide resolved
@ktock ktock force-pushed the ktock:stargz branch from a61b5db to 13ff44c Apr 10, 2020
@mikebrow
Copy link
Member

mikebrow commented Apr 10, 2020

/test pull-cri-containerd-windows-cri

Copy link
Member

mikebrow left a comment

LGTM
on green

@mikebrow
Copy link
Member

mikebrow commented Apr 10, 2020

/test pull-cri-containerd-windows-cri

@ktock
Copy link
Contributor Author

ktock commented Apr 11, 2020

ERROR: (gcloud.compute.instances.create) Could not fetch resource:
 - Quota 'CPUS' exceeded.  Limit: 100.0 in region us-west1.

Seems not related to this patch🤔

@ktock
Copy link
Contributor Author

ktock commented Apr 11, 2020

/retest

@fuweid
fuweid approved these changes Apr 11, 2020
Copy link
Member

fuweid left a comment

LGTM

@ktock
Copy link
Contributor Author

ktock commented Apr 11, 2020

ERROR: (gcloud.compute.instances.create) Could not fetch resource:
 - Quota 'CPUS' exceeded.  Limit: 100.0 in region us-west1.

Getting the same error. I'll try it again later.

@ktock
Copy link
Contributor Author

ktock commented Apr 11, 2020

/retest

1 similar comment
@ktock
Copy link
Contributor Author

ktock commented Apr 13, 2020

/retest

@k8s-ci-robot
Copy link
Collaborator

k8s-ci-robot commented Apr 13, 2020

@ktock: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
pull-cri-containerd-windows-cri 13ff44c link /test pull-cri-containerd-windows-cri

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

@mikebrow
Copy link
Member

mikebrow commented Apr 13, 2020

ERROR: (gcloud.compute.instances.create) Could not fetch resource:
 - Quota 'CPUS' exceeded.  Limit: 100.0 in region us-west1.

Seems not related to this patch🤔

correct not your patch!

@mikebrow
Copy link
Member

mikebrow commented Apr 15, 2020

@ktock pls rebase.. pull-cri-containerd-windows-cri test has been moved to git hub actions.

…pshots

Throughout container lifecycle, pulling image is one of the time-consuming
steps. Recently, containerd community started to tackle this issue with
stargz-based remote snapshots, as a non-core
subproject(https://github.com/containerd/stargz-snapshotter).

This snapshotter is implemented as a standard proxy plugin but it requires the
client to pass some additional information (image ref and layer digest) for each
pull operation to query layer contents on the registry. Stargz snapshotter
project provides an image handler to do this and stargz snapshot users need to
pass this handler to containerd client.

This commit enables to use stargz-based remote snapshots through CRI by passing
the handler to containerd client on pull operation.

Signed-off-by: Kohei Tokunaga <ktokunaga.mail@gmail.com>
@ktock ktock force-pushed the ktock:stargz branch from 13ff44c to c1b7bcf Apr 16, 2020
@mikebrow mikebrow merged commit 7ccd3f7 into containerd:master Apr 16, 2020
8 checks passed
8 checks passed
linux validate build and unit integration and CRI linux validate build and unit integration and CRI
Details
greeting
Details
Build and CRI Test Windows amd64
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
pull-cri-containerd-build Job succeeded.
Details
pull-cri-containerd-node-e2e Job succeeded.
Details
pull-cri-containerd-verify Job succeeded.
Details
@ktock ktock deleted the ktock:stargz branch Apr 17, 2020
@lorqor
Copy link

lorqor commented Jun 16, 2020

Hi Kohei/Mike, was the plan to have this commit in Containerd 1.3.x or 1.4.x?

Containerd did not pick the change in containerd/containerd#4193 in v1.3.4, given that remote snapshotters are already supported in 1.3.

@AkihiroSuda
Copy link
Member

AkihiroSuda commented Jun 16, 2020

1.4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.