ContainerStatus to return container resources #6517
Conversation
|
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 Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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. |
effae37 to
39e5efb
Compare
f1c6dbb to
b404a27
Compare
de9d960 to
063f0fe
Compare
|
/ok-to-test |
|
@ruiwen-zhao looking good just some cleanup now I think.. |
063f0fe to
41ec725
Compare
Thanks Mike! I just updated the case of CPU (i.e. CpuPeriod -> CPUPeriod) to try to appease linter check. But since this PR is based on a manually copied CRI API proto, Project Checks will probably still fail. One more thing that I need some input: there is a new field rootfs_size_in_bytes under WindowsContainerResources [1], which was added to CRI API proto 4 months ago. Looking at containerd, I don't think containerd supports this yet [2]. I am going to leave this field out of the scope of this PR, and probably I should go create a separate issue to support that in containerd? [2] https://github.com/containerd/containerd/blob/main/pkg/cri/opts/spec_windows.go#L179 |
41ec725 to
cd31245
Compare
|
Another thing I missed and just found out is that UpdateContainerResources now needs to write the updated status onto disk, because with this change container status will be modified after UpdateContainerResources and therefore needs to be written on disk. See my most recent change in this PR [1]. |
|
good catch on the sync to store of status now that it holds spec updates, otherwise on restart... yes pls, on opening a new issue for windows cri for the new field! |
|
/cc @samuelkarp |
30251b0 to
ab50753
Compare
ca8d5bb to
ca1d45c
Compare
mikebrow
left a comment
There was a problem hiding this comment.
just the one question / nit really .. should we use the cri definition of container resources containing one of linux or windows resources as defined in cri... (probably z, mac, .. others later on) or should we keep our version in containerd sandboxed containers as a mirror but separately defined... Wiling to go either way as I see benefits to either.. but if we keep separate we should at least document in the code the desire to keep them in sync over disparate spans.
ca1d45c to
b71b3c4
Compare
|
/retest |
b71b3c4 to
1ab59e5
Compare
ebaa0f8 to
963c65c
Compare
963c65c to
931920e
Compare
Signed-off-by: ruiwen-zhao <ruiwen@google.com>
931920e to
b7b1200
Compare
|
modified this to be in service milestone per discussions at community meeting for putting in certain lower risk / higher priority features into service, if the 1.7 release schedule continues moving right.. |
|
if needs discussion we can yank the cherry tag |
This is part of the support of In-place Pod Vertical Scaling feature.
This PR handles an extension to the ContainerStatus CRI API and returns the Resources field as part of the response message to ContainerStatus API.
This PR also adds a fake
container.Containerfor the purpose of unit testing. (I am not entirely sure which dir I should put the fake_container.go, so happy to take some suggestion on that)See kubernetes/kubernetes#107911 for more details.
Note that this PR is still WIP and blocked by:
a)
CRI changes in kubernetes/kubernetes#102884, which addsResourcesfield toContainerStatus, andb) containerd updating cri-api version.