Skip to content

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

Merged
mxpv merged 4 commits intocontainerd:release/1.6from
ruiwen-zhao:container_resource_1.6
Sep 22, 2022
Merged

[release/1.6] cherry-pick: ContainerStatus to return container resources#7410
mxpv merged 4 commits intocontainerd:release/1.6from
ruiwen-zhao:container_resource_1.6

Conversation

@ruiwen-zhao
Copy link
Copy Markdown
Member

@ruiwen-zhao ruiwen-zhao commented Sep 20, 2022 •
edited
Loading

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
Copy Markdown

k8s-ci-robot commented Sep 20, 2022

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.

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.

@ruiwen-zhao ruiwen-zhao force-pushed the container_resource_1.6 branch from 7296ce1 to ae85986 Compare September 20, 2022 20:19
@mikebrow
Copy link
Copy Markdown
Member

mikebrow commented Sep 20, 2022

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

@mikebrow
Copy link
Copy Markdown
Member

mikebrow commented Sep 20, 2022

/ok-to-test

@mikebrow mikebrow added this to the 1.6.9 milestone Sep 20, 2022
@mikebrow mikebrow self-assigned this Sep 20, 2022
@ruiwen-zhao ruiwen-zhao mentioned this pull request Sep 20, 2022
Closed
@zouyee @ruiwen-zhao
bump cri-api
6ee5bb7 
Signed-off-by: zounengren <[email protected]>
@zouyee @ruiwen-zhao
update intergration
fb753e5 
Signed-off-by: zounengren <[email protected]>
@ruiwen-zhao
Copy link
Copy Markdown
Member Author

ruiwen-zhao commented Sep 20, 2022

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
mikebrow
mikebrow approved these changes Sep 21, 2022
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

@dmcgowan
Copy link
Copy Markdown
Member

dmcgowan 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?

@ruiwen-zhao
Copy link
Copy Markdown
Member Author

ruiwen-zhao commented Sep 22, 2022 •
edited
Loading

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

@ruiwen-zhao ruiwen-zhao mentioned this pull request Sep 22, 2022
Merged
@mxpv mxpv merged commit 462caf1 into containerd:release/1.6 Sep 22, 2022
@ruiwen-zhao
Copy link
Copy Markdown
Member Author

ruiwen-zhao commented Sep 22, 2022

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

Reviewers

@dmcgowan dmcgowan dmcgowan approved these changes

@mxpv mxpv mxpv approved these changes

@mikebrow mikebrow mikebrow approved these changes

Assignees

@mikebrow mikebrow

Labels

area/cri Container Runtime Interface (CRI) ok-to-test

Projects

None yet

Milestone

1.6.9

Development

Successfully merging this pull request may close these issues.

7 participants

@ruiwen-zhao @k8s-ci-robot @mikebrow @dmcgowan @mxpv @samuelkarp @zouyee