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

@dcantah Thanks for review, do you have any idea why sandboxed tests are getting failed? they are failing in general or only on this PR?

cc @SergeyKanzhelev @mikebrow

@dcantah Thanks for review, do you have any idea why sandboxed tests are getting failed? they are failing in general or only on this PR?

The sandboxed test will run the same integration test with sandbox servers. So I think you need to make the same changes on pkg/cri/sbserver as well.

@adisky

do you have any idea why sandboxed tests are getting failed? they are failing in general or only on this PR?

The new test you added is failing:

=== RUN   TestContainerdSandboxImage
    containerd_image_test.go:215: make sure the pause image exist
    containerd_image_test.go:218: ensure correct labels are set on pause image
    containerd_image_test.go:219: 
        	Error Trace:	/home/runner/work/containerd/containerd/containerd_image_test.go:219
        	Error:      	Not equal: 
        	            	expected: ""
        	            	actual  : "pinned"
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -1 +1 @@
        	            	-
        	            	+pinned
        	Test:       	TestContainerdSandboxImage
    containerd_image_test.go:221: pause image should be seen by cri plugin
    containerd_image_test.go:225: verify pinned field is set for pause image
    containerd_image_test.go:226: 
        	Error Trace:	/home/runner/work/containerd/containerd/containerd_image_test.go:226
        	Error:      	Should be true
        	Test:       	TestContainerdSandboxImage
--- FAIL: TestContainerdSandboxImage (0.00s)

@ruiwen-zhao is correct; the test will run for both the server and sbserver implementations. It looks like you haven't implemented your changes for sbserver yet, so the new test fails.

@ruiwen-zhao is correct; the test will run for both the server and sbserver implementations. It looks like you haven't implemented your changes for sbserver yet, so the new test fails.

@ruiwen-zhao @samuelkarp
I haven't read the sandbox API proposal but heard it wont require the pause container? So Does it makes sense to add these changes in sbserver?

@ruiwen-zhao is correct; the test will run for both the server and sbserver implementations. It looks like you haven't implemented your changes for sbserver yet, so the new test fails.

@ruiwen-zhao @samuelkarp I haven't read the sandbox API proposal but heard it wont require the pause container? So Does it makes sense to add these changes in sbserver?

The proposal is here: #4131. I am not sure if we have removed the requirement for pause container with current implementation.

I think the current requirement is that all new changes to cri/server will need to be made on cri/sbserver as well. You just need to copy your changes under cri/server over to cri/sbserver.

@ruiwen-zhao @pacoxu can you take a look again? added implementation to sbserver as well

Details

Thanks @adisky for updating the code! Looks good except for one comment above.

/ok-to-test

looks like flake
/test pull-containerd-node-e2e

ERROR: (gcloud.auth.activate-service-account) There was a problem refreshing your current auth tokens: ('invalid_grant: Invalid JWT Signature.', {'error': 'invalid_grant', 'error_description': 'Invalid JWT Signature.'})

/test pull-containerd-node-e2e

a couple lint issues to address:20

Running [/home/runner/golangci-lint-1.52.2-linux-amd64/golangci-lint run --out-format=github-actions --timeout=8m] in [] ...
21
Error: const imageLabelKey is unused (unused)
22
Error: const imageLabelValue is unused (unused)

cri-o related PR cri-o/cri-o#6903

@adisky sorry looks like another merge conflict on helpers.go could you rebase one more time :)

Thank @mikebrow for review, rebased

Also linking the discussion we had on containerd slack
https://cloud-native.slack.com/archives/CGEQHPYF4/p1685533468086829

@mikebrow @dmcgowan Any objection to backporting this (or at least the logic setting the pinned property based on the io.cri-containerd.pinned label) to 1.7 and 1.6?

Thanks @ruiwen-zhao for opening #8718 and #8720.