Update CRI to optionally use transfer service for image pull #8515

fangn2 · 2023-05-15T03:01:38Z

This PR is to update CRI to optionally use transfer service for image pull.

Part of #8227

Create a new CRI config ImagePullWithTransferService to specify whether to use transfer service for image pull
Move CRI image pull related configs to server side, i.e. transfer service.

The PR is still in progress, put out to for early review and utilize CI for integration tests.

k8s-ci-robot · 2023-05-15T03:01:47Z

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

mikebrow · 2023-05-17T00:51:24Z

/ok-to-test

pkg/cri/config/config.go

pkg/cri/server/image_pull.go

fuweid · 2023-06-12T05:35:32Z

pkg/cri/server/image_pull.go

+			return nil, fmt.Errorf("failed to pull and unpack image %q: %w", ref, err)
+		}
+
+		return &runtime.PullImageResponse{ImageRef: ref}, nil


need to update the image store.

Thanks for review! Will address it in the coming changes.

Done.
Just curious CRI currently maintain an in-memory image cache(there could be situations that the CRI image store and containerd image store are out of sync, like other clients e.g. ctr removed an image). Can we get rid of it by directly using contained image store?

estesp · 2023-10-03T18:37:23Z

CRI test failures seem related to the new (optional) image verifier service merged in #8493; the image verifier plugin is a required plugin for transfer plugin now and the error seems to be related to that? Seems you rebased, so I'm not sure what else has to change to make that work properly?

Error:

2023-10-03T02:31:08.9836244Z === NAME  TestCRIImagePullTimeout/HoldingContentOpenWriter
2023-10-03T02:31:08.9837268Z     build_local_containerd_helper_test.go:109: 
2023-10-03T02:31:08.9838989Z         	Error Trace:	/home/runner/work/containerd/containerd/integration/build_local_containerd_helper_test.go:109
2023-10-03T02:31:08.9840452Z         	            				/home/runner/work/containerd/containerd/integration/image_pull_timeout_test.go:78
2023-10-03T02:31:08.9841049Z         	Error:      	Received unexpected error:
2023-10-03T02:31:08.9842280Z         	            	no plugins registered for io.containerd.image-verifier.v1: not found
2023-10-03T02:31:08.9843017Z         	Test:       	TestCRIImagePullTimeout/HoldingContentOpenWriter
2023-10-03T02:31:08.9843552Z     build_local_containerd_helper_test.go:120: 
2023-10-03T02:31:08.9844427Z         	Error Trace:	/home/runner/work/containerd/containerd/integration/build_local_containerd_helper_test.go:120
2023-10-03T02:31:08.9845822Z         	            				/home/runner/work/containerd/containerd/integration/image_pull_timeout_test.go:78
2023-10-03T02:31:08.9846406Z         	Error:      	Received unexpected error:
2023-10-03T02:31:08.9847983Z         	            	failed to get "io.containerd.transfer.v1" plugin: no plugins registered for io.containerd.image-verifier.v1: not found
2023-10-03T02:31:08.9848807Z         	Test:       	TestCRIImagePullTimeout/HoldingContentOpenWriter
2023-10-03T02:31:08.9861131Z === NAME  TestCRIImagePullTimeout/NoDataTransferred
2023-10-03T02:31:08.9862011Z     build_local_containerd_helper_test.go:109: 
2023-10-03T02:31:08.9863128Z         	Error Trace:	/home/runner/work/containerd/containerd/integration/build_local_containerd_helper_test.go:109
2023-10-03T02:31:08.9864564Z         	            				/home/runner/work/containerd/containerd/integration/image_pull_timeout_test.go:216
2023-10-03T02:31:08.9865157Z         	Error:      	Received unexpected error:
2023-10-03T02:31:08.9866373Z         	            	no plugins registered for io.containerd.image-verifier.v1: not found
2023-10-03T02:31:08.9867085Z         	Test:       	TestCRIImagePullTimeout/NoDataTransferred
2023-10-03T02:31:08.9867605Z     build_local_containerd_helper_test.go:120: 
2023-10-03T02:31:08.9868487Z         	Error Trace:	/home/runner/work/containerd/containerd/integration/build_local_containerd_helper_test.go:120
2023-10-03T02:31:08.9870257Z         	            				/home/runner/work/containerd/containerd/integration/image_pull_timeout_test.go:216
2023-10-03T02:31:08.9871126Z         	Error:      	Received unexpected error:
2023-10-03T02:31:08.9872738Z         	            	failed to get "io.containerd.transfer.v1" plugin: no plugins registered for io.containerd.image-verifier.v1: not found
2023-10-03T02:31:08.9873513Z         	Test:       	TestCRIImagePullTimeout/NoDataTransferred
2023-10-03T02:31:08.9874021Z --- FAIL: TestCRIImagePullTimeout (0.01s)

fangn2 · 2024-02-14T03:48:38Z

plugins/transfer/plugin.go

@@ -130,6 +130,11 @@ func init() {
 					return nil, fmt.Errorf("no matching diff plugins: %w", errdefs.ErrNotFound)
 				}

+				// If CheckPlatformSupported is false, we will match all platforms
+				if !config.CheckPlatformSupported {


@dmcgowan I added this config to transfer plugin to optionally not enforcing strict platform matching in unpackConfiguration platform, snapshotter due to cri-tools pull test that pulls an image digest which is arm64 arch based.
This test failed on the CI with the default platform(amd64) and snapshotter combination.
Does it make sense to have such config here? or should we update containerd config file to pass the test in this case?

My understanding is with the defaultUnpackConfiguration, i.e. amd64,overlayfs, transfer service doesn't allow unpacking different arch(arm64) of image other than the running platform unless we explicitly add that UnpackConfiguration arm64, overlayfs to the supported platform. This is different with the client.pull approach.

We really need a better platform matcher for that case. For unpack configuration it should really be able to just match OS, the architecture does not matter too much for unpacker. Where is it enforcing this check today, if the image itself is pulled by digest.

fuweid · 2024-03-07T02:10:13Z

ping @fangn2 ~ any update here?

swagatbora90 · 2024-07-10T20:01:46Z

@fangn2 Are you still working on this? If not, I can help push this forward.

swagatbora90 · 2024-07-16T23:57:36Z

Test are passing now.
Added two main changes:

Added a WithResolver option to transfer service registry to pass custom resolver. CRI already configures it own resolver, so we can just pass that to transfer service.
Had to create a client.TransferService() method to use in initLocalCRIImageService which is being used by some of the CRI integration tests. Wondering if there is a better way to do this without exposing a new api just for the integration test?

fangn2 · 2024-07-18T17:47:18Z

/retest-required

Signed-off-by: Tony Fang <nhfang@amazon.com>

Signed-off-by: Swagat Bora <sbora@amazon.com>

dmcgowan · 2024-07-19T20:51:26Z

core/transfer/registry/registry.go

@@ -48,6 +48,7 @@ type registryOpts struct {
 	creds         CredentialHelper
 	hostDir       string
 	defaultScheme string
+	resolver      remotes.Resolver


Resolver is not serializable, what are the equivalent settings it needs to initialize it? It doesn't need to share resolvers

Thanks for pointing this out @dmcgowan. I can modify the implementation to ensure the necessary settings are provided directly to initialize the registry.

I ran into issue with one setting where a pull request reporter was passed to the resolver in order to wrap the underlying http client and track active requests and bytes read. This is used to implement image pull progress timeout. I think I should be able to handle this using transfer.Progress. I can give that a try but let me know if you have any suggestion.

fangn2 marked this pull request as draft May 15, 2023 03:01

k8s-ci-robot added the needs-ok-to-test label May 15, 2023

fangn2 changed the title ~~[WIP]Update CRI to optional use transfer service for image pull~~ [WIP]Update CRI to optionally use transfer service for image pull May 15, 2023

fangn2 force-pushed the cri-image-transfer branch from a62e6ed to ed26de0 Compare May 15, 2023 03:07

k8s-ci-robot added ok-to-test and removed needs-ok-to-test labels May 17, 2023

mikebrow reviewed May 17, 2023

View reviewed changes

pkg/cri/config/config.go Outdated Show resolved Hide resolved

mikebrow reviewed May 17, 2023

View reviewed changes

pkg/cri/config/config.go Outdated Show resolved Hide resolved

fangn2 force-pushed the cri-image-transfer branch from ed26de0 to 3ed06cd Compare May 30, 2023 16:56

dmcgowan self-assigned this May 30, 2023

dmcgowan reviewed Jun 11, 2023

View reviewed changes

pkg/cri/server/image_pull.go Outdated Show resolved Hide resolved

fangn2 force-pushed the cri-image-transfer branch from 3ed06cd to 88d975b Compare June 12, 2023 03:12

fuweid reviewed Jun 12, 2023

View reviewed changes

fangn2 force-pushed the cri-image-transfer branch 2 times, most recently from fd0d0c3 to 113293b Compare August 9, 2023 14:41

fangn2 force-pushed the cri-image-transfer branch 6 times, most recently from 45eaa5d to 88e6abe Compare August 16, 2023 16:52

mxpv added the status/needs-update Awaiting contributor update label Sep 7, 2023

fangn2 force-pushed the cri-image-transfer branch from 88e6abe to b47bf45 Compare September 21, 2023 21:34

fangn2 force-pushed the cri-image-transfer branch from b47bf45 to 95e3822 Compare October 3, 2023 01:58

fangn2 force-pushed the cri-image-transfer branch from 95e3822 to 3faa50c Compare October 4, 2023 15:09

fangn2 mentioned this pull request Oct 25, 2023

Enhance CRI to download image by image transfer service #9291

Closed

fangn2 force-pushed the cri-image-transfer branch from 919d22b to 97fd85e Compare October 30, 2023 01:58

k8s-ci-robot added the needs-rebase label Jan 20, 2024

dims added the area/cri Container Runtime Interface (CRI) label Feb 7, 2024

fangn2 force-pushed the cri-image-transfer branch from f284191 to 9d693b1 Compare February 13, 2024 03:50

k8s-ci-robot removed the needs-rebase label Feb 13, 2024

fangn2 force-pushed the cri-image-transfer branch 2 times, most recently from cdd0772 to cc07653 Compare February 13, 2024 22:04

fangn2 commented Feb 14, 2024

View reviewed changes

austinvazquez force-pushed the cri-image-transfer branch from cc07653 to 173942a Compare March 8, 2024 15:07

dmcgowan modified the milestones: 2.0, 2.1 Apr 25, 2024

k8s-ci-robot added the needs-rebase label May 28, 2024

austinvazquez force-pushed the cri-image-transfer branch from 173942a to de5e7fe Compare July 9, 2024 20:44

k8s-ci-robot removed the needs-rebase label Jul 9, 2024

swagatbora90 force-pushed the cri-image-transfer branch 3 times, most recently from bad8d2c to cbad8e6 Compare July 16, 2024 22:51

fangn2 changed the title ~~[WIP]Update CRI to optionally use transfer service for image pull~~ Update CRI to optionally use transfer service for image pull Jul 17, 2024

k8s-ci-robot added the do-not-merge/work-in-progress label Jul 17, 2024

fangn2 marked this pull request as ready for review July 17, 2024 01:58

k8s-ci-robot removed the do-not-merge/work-in-progress label Jul 17, 2024

fangn2 and others added 2 commits July 19, 2024 01:15

Update CRI image service to pull using transfer service

bd2e5ff

Signed-off-by: Tony Fang <nhfang@amazon.com>

add registry opts for resolver and other test fixes

ff50560

Signed-off-by: Swagat Bora <sbora@amazon.com>

swagatbora90 force-pushed the cri-image-transfer branch from cbad8e6 to ff50560 Compare July 19, 2024 01:16

just1not2 mentioned this pull request Jul 19, 2024

container image signature verification at containerd #10066

Open

AkihiroSuda requested a review from dmcgowan July 19, 2024 16:02

dmcgowan reviewed Jul 19, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update CRI to optionally use transfer service for image pull #8515

Update CRI to optionally use transfer service for image pull #8515

fangn2 commented May 15, 2023

k8s-ci-robot commented May 15, 2023

mikebrow commented May 17, 2023

fuweid Jun 12, 2023

fangn2 Jun 15, 2023

fangn2 Aug 11, 2023

estesp commented Oct 3, 2023

fangn2 Feb 14, 2024

dmcgowan Feb 14, 2024

fuweid commented Mar 7, 2024

swagatbora90 commented Jul 10, 2024

swagatbora90 commented Jul 16, 2024 •

edited

Loading

fangn2 commented Jul 18, 2024

dmcgowan Jul 19, 2024

swagatbora90 Jul 22, 2024 •

edited

Loading

Update CRI to optionally use transfer service for image pull #8515

Are you sure you want to change the base?

Update CRI to optionally use transfer service for image pull #8515

Conversation

fangn2 commented May 15, 2023

k8s-ci-robot commented May 15, 2023

mikebrow commented May 17, 2023

fuweid Jun 12, 2023

Choose a reason for hiding this comment

fangn2 Jun 15, 2023

Choose a reason for hiding this comment

fangn2 Aug 11, 2023

Choose a reason for hiding this comment

estesp commented Oct 3, 2023

fangn2 Feb 14, 2024

Choose a reason for hiding this comment

dmcgowan Feb 14, 2024

Choose a reason for hiding this comment

fuweid commented Mar 7, 2024

swagatbora90 commented Jul 10, 2024

swagatbora90 commented Jul 16, 2024 • edited Loading

fangn2 commented Jul 18, 2024

dmcgowan Jul 19, 2024

Choose a reason for hiding this comment

swagatbora90 Jul 22, 2024 • edited Loading

Choose a reason for hiding this comment

swagatbora90 commented Jul 16, 2024 •

edited

Loading

swagatbora90 Jul 22, 2024 •

edited

Loading