Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
KEP: in-place update of pod resources #686
KEP: in-place update of pod resources #686
Changes from 2 commits
cd94808
7fb66f1
b8c1f4e
5d00f9f
9580642
b8d814e
df1c8f8
17923eb
e5052fc
1194243
bfab6a3
69f9190
199a008
574737c
5bdcd57
bc9dc2b
533c3c6
29a22b6
20cbea6
0ed9505
c745563
55c8e56
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a corresponding CRI change for review? That shouldn't block merging this KEP draft but it is going to be important for implementers as it would need to be reviewed for Linux+Windows compatibility and runtime compatibility (dockershim/kata/hyper-v)
cc @feiskyer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not very confident we have reached agreement that it's the direction we will go. If yes, a CRI change should be included here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@PatrickLang In our implementation, is was sufficient to make changes in kubelet to detect a resources-only container spec update, and call UpdateContainerResources CRI API without any changes to the CRI itself. We have tested it with docker, we are yet to try kata.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vinaykul Kata does not update "container" resource for now :-) Also, it's related to how CRI shim is implemented, in containerd shimv2 work, I remembered we didn't handle this update at least month ago.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While if we decide to go with the current narrative in this KEP, CRI do need to be updated (new filed:
ResourceAllocated
) and CRI shim & crictl maintainers should be notified about the incompatible change of meaning ofLinuxContainerResources
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah that's good to know. I last toyed with Kata at GA, and they were working on getting cpu/mem update working.
I tried out krt1.4.1 earlier today, and found OCI mostly works. CPU / memory increase & decrease reflects in the cgroup inside kata container and enforced, but VSZ/RSS isn't lowered when memory is lowered, and Get doesn't reflect the actual usage.
I'll try k8s-crio-kata tomorrow or friday to see how well crio-oci translation works and identify gaps. It probably won't work if containerd shim doesnt handle it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@resouer kata-runtime 1.4.1 seems to handle updating cpu/memory via CRI-O (below example)
Regarding CRI, kubelet would merely switch from using PodSpec.Container.Container.ResourceRequirements to PodStatus.Container.ResourceAllocated to get the limits when invoking the CRI API in this function for e.g: https://github.com/Huawei-PaaS/kubernetes/blob/vert-scaling-cp-review/pkg/kubelet/kuberuntime/kuberuntime_container.go#L619
Did I miss something in thinking that CRI update is not necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I just check the kata-runtime's api and now every CRI shim could support container level resource adjustment. In that case, no CRI change is required, we can simply just use
ResourceAllocated
to generate containerResources.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure about "every CRI"? I'll try to look on that deeper during next days, but almost for sure that will break https://github.com/Mirantis/virtlet/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@PatrickLang Over the past week, I experimented with WinServer 2019 (for another project planning that I'm working on), and got the chance to try a windows cross-compile kubelet of my implementation and take a closer look at how to set the updated limits. Windows does create the container with specified limits (perhaps using information from ContainerConfig.WindowsContainerConfig.WindowsContainerResources struct).
For cleanliness, I do see that we should update the CRI API to specify ContainerResources instead of LinuxContainerResources (which would have pointers to LinuxContainerResources or WindowsContainerResources similar to ContainerConfig). Do you think containerID + WindowsContainerResources is sufficient for Windows to successfully update the limits?
@jellonek I've not looked at virtlet. If you have had the chance, can you please check if its CRI shim is able to use LinuxContainerResources?