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

[release/1.6] cherry-pick: ContainerStatus to return container resources #7410

Merged
merged 4 commits into from Sep 22, 2022

Conversation

ruiwen-zhao
Copy link
Member

@ruiwen-zhao ruiwen-zhao commented Sep 20, 2022

As discussed in community meeting, this PR backports #6517 to release 1.6.

When cherry-picking commit Update CRI-API, I

  1. resolved conflicts in go.mod and integration/client/go.mod, by keeping every the same as HEAD of release/1.6 except changing cri-api to k8s.io/cri-api v0.25.0-alpha.3.0.20220803042751-33bb7ae0d927.
  2. ran go mod tidy and go mod vendor under root dir.
  3. ran go mod tidy under integration/client/.

Note that the cri-api version used in this PR is not a released version k8s.io/cri-api v0.25.0-alpha.3.0.20220803042751-33bb7ae0d927. If this is not allowed in release branch, I will go cherry-pick #7287 in this PR as well.

Two integration tests covering this change both pass:

=== RUN   TestUpdateContainerResources_StatusUpdated
    container_update_resources_test.go:296: Create a sandbox
    common.go:115: Image "k8s.gcr.io/pause:3.6" already exists, not pulling.
    container_update_resources_test.go:301: Create a container with memory limit
    container_update_resources_test.go:312: Check memory limit in container status
    container_update_resources_test.go:317: Update container memory limit after created
    container_update_resources_test.go:323: Check memory limit in container status
    container_update_resources_test.go:328: Start the container
    container_update_resources_test.go:331: Update container memory limit after started
    container_update_resources_test.go:337: Check memory limit in container status
--- PASS: TestUpdateContainerResources_StatusUpdated (5.50s)
PASS

=== RUN   TestContainerdImage
    containerd_image_test.go:39: make sure the test image doesn't exist in the cri plugin
    containerd_image_test.go:46: pull the image into containerd
    containerd_image_test.go:57: the image should be seen by the cri plugin
    containerd_image_test.go:65: Image "docker.io/library/busybox:latest" not show up in the cri plugin yet
    containerd_image_test.go:114: the image should be marked as managed
    containerd_image_test.go:119: the image id should be created and managed
    containerd_image_test.go:124: the image should be labeled
    containerd_image_test.go:129: should be able to start container with the image
    containerd_image_test.go:90: image should still be seen by id if only tag get deleted
    containerd_image_test.go:101: image should be removed from the cri plugin if all references get deleted
--- PASS: TestContainerdImage (5.66s)

@k8s-ci-robot
Copy link

Hi @ruiwen-zhao. 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.

@dmcgowan dmcgowan added this to New in Code Review via automation Sep 20, 2022
@dmcgowan dmcgowan moved this from New to Needs Contributor Update in Code Review Sep 20, 2022
Signed-off-by: ruiwen-zhao <ruiwen@google.com>
Signed-off-by: ruiwen-zhao <ruiwen@google.com>
@mikebrow
Copy link
Member

nod let's do the full cherry pick of v25 to have a supportable release baseline for the cri api

@mikebrow
Copy link
Member

/ok-to-test

Signed-off-by: zounengren <zouyee1989@gmail.com>
Signed-off-by: zounengren <zouyee1989@gmail.com>
@ruiwen-zhao
Copy link
Member Author

Sync'ed with @mikebrow over Slack, we will use cri-api 0.25.0 on release/1.6 branch, so I cherry-picked #7287 into this PR as well.

@samuelkarp samuelkarp added the area/cri Container Runtime Interface (CRI) label Sep 21, 2022
Copy link
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

@dmcgowan dmcgowan moved this from Needs Contributor Update to Ready For Review in Code Review Sep 22, 2022
@dmcgowan
Copy link
Member

Since this a backport, not a blocking question, but why is errors.New("not implemented") used over an existing not implemented error. Is there something else that makes sure the not implemented grpc code gets set?

@ruiwen-zhao
Copy link
Member Author

ruiwen-zhao commented Sep 22, 2022

Since this a backport, not a blocking question, but why is errors.New("not implemented") used over an existing not implemented error. Is there something else that makes sure the not implemented grpc code gets set?

Good point. I guess I just missed setting the grpc code here. I should have used

return status.Errorf(codes.Unimplemented, "method GetContainerEvents not implemented")

Since this is a backport PR so I will not change the code here. I guess I can create another PR to update the code and maybe backport that as well?

Update: Created #7417

@mxpv mxpv merged commit 462caf1 into containerd:release/1.6 Sep 22, 2022
Code Review automation moved this from Ready For Review to Done Sep 22, 2022
@ruiwen-zhao
Copy link
Member Author

Created a cherry-pick PR for #7417: #7421

@ruiwen-zhao ruiwen-zhao deleted the container_resource_1.6 branch September 22, 2022 20:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/cri Container Runtime Interface (CRI) ok-to-test
Projects
Development

Successfully merging this pull request may close these issues.

None yet

7 participants