-
Notifications
You must be signed in to change notification settings - Fork 40k
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
kubelet: setup WindowsContainerResources for windows containers #59333
Conversation
/sig node |
@feiskyer: GitHub didn't allow me to request PR reviews from the following users: PatrickLang, taylorb-microsoft. Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs. In response to this: 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. |
/retest |
wc.Resources.CpuMaximum = cpuMaximum | ||
} | ||
|
||
cpuShares := milliCPUToShares(cpuLimit.MilliValue()) |
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.
Windows cpushares range from 1-10000, we need a different calculation than the one used by Linux. @darrenstahlmsft Any pointer to calculate cpushares?
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 it just a simple proportion? 100m = 1000 cpu shares (1/10 in both cases)? Interestingly, according to https://docs.microsoft.com/en-us/virtualization/windowscontainers/manage-containers/resource-controls#cpu-shares-without-hyper-v-isolation , the number passed in (if not using hyper-v isolation) will be translated to a value from 1 through 9 (default 5) for weight on the JOBOBJECT_CPU_RATE_CONTROL_INFORMATION structure
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 it just a simple proportion? 100m = 1000 cpu shares (1/10 in both cases)?
cpu shares = milliCPU * 1024 / 1000, so 100m = 102 cpu shares, and 10 cpus would be 10240 cpu shares (which exceeds 10000).
Updated the PR and scaled cpuShares within range [1,10000].
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.
The shares at the platform is only relative to other containers (of the same isolation type), and is not based on the host's resource maximums. If all containers are set to 1000 shares, they will all have (near) equal access to CPU when the system is loaded. If no value is specified, the default for Hyper-V isolation is 10. The default on Process isolation is 5000, which is only changeable if no other processor options are set on that container. Ie. CpuCount = 4, CpuShares = 10000 on a process isolated container will result in the CpuCount being used, and the shares being ignored (and defaulting to 5000).
I'm still attempting to find the guarantees regarding how the shares are applied relative to each other. I don't think we make any guarantees that jobs weighted at, for example, 1 and 2, would result in the job weighted at 2 having twice as much CPU, just that it will have higher priority access to the host's CPUs.
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.
@darrenstahlmsft So how do you suggest to calc shares? factor by 10 for hyperv and 5000 by process?
Rebased and resolved conflicts. @dchen1107 @michmike PTAL |
/retest |
Rebased and addressed comments. @JiangtianLi @dchen1107 @darrenstahlmsft PTAL |
a7837f1
to
f2ea654
Compare
Made another rebase |
ping @dchen1107 @JiangtianLi |
/approve I am approving this pr, and reviewed the changes / refactors touching the code for linux node, they look good to me. But I am relying on @JiangtianLi or someone from sig-windows to provide throughout review for windows related changes here. |
// +build windows | ||
|
||
/* | ||
Copyright 2017 The Kubernetes Authors. |
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.
Nit: Copyright 2018
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.
Fixed
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.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dchen1107, feiskyer, JiangtianLi The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
@dchen1107 @JiangtianLi Thanks |
/test all [submit-queue is verifying that this PR is safe to merge] |
Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here. |
What this PR does / why we need it:
This PR setups WindowsContainerResources for windows containers. It implements proposal here: kubernetes/community#1510.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes #56734
Special notes for your reviewer:
Release note: