-
Notifications
You must be signed in to change notification settings - Fork 5.2k
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
Design proposal for Windows Container Configuration in CRI #1510
Conversation
4cc4909
to
93a929b
Compare
93a929b
to
b90f483
Compare
/sig node |
/cc @jhowardmsft; @darrenstahlmsft; @jstarks; @PatrickLang; @taylorb-microsoft; @michmike |
[#56734](https://github.com/kubernetes/kubernetes/issues/56734) | ||
|
||
## Motivation | ||
The goal is to fill the gap of platform support in CRI, specifically for Windows platform. For example, currrently in dockershim Windows containers are scheduled using the default resource constraints and does not respect the resource requests and limits specified in POD. With this proposal, Windows containers will be able to leverage POD spec and CRI to allocate compute resource and respect restriction. |
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.
It'd be nice to know besides resources, what other parts of the current API is lacking to support Windows containers (e.g., security context?).
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.
@yujuhong According to https://github.com/opencontainers/runtime-spec/blob/master/specs-go/config.go#L418-L434, I think there are a few other properties such as CredentialSpec or HyperV that could be useful and already implemented in Windows. How does LinuxContainerSecurityContext map to https://github.com/opencontainers/runtime-spec/blob/master/specs-go/config.go#L132-L164? I saw some similarity such as Seccomp. Selinux and Apparmor is in Process struct though (https://github.com/opencontainers/runtime-spec/blob/master/specs-go/config.go#L30-L57)
@taylorb-microsoft Please advise what property besides Resources is needed to support Windows containers now (https://github.com/opencontainers/runtime-spec/blob/master/specs-go/config.go#L418-L434).
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.
@yujuhong @JiangtianLi I have a draft on this that we're discussing in SIG-Windows. @feiskyer also has a draft that we're merging together for the SIG with @michmike. Next meeting is 9 Jan. After that we'll send it up to SIG-Node
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 feel free to email me the draft and i can take a look ahead of our jan9 meeting
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.
Next meeting is 9 Jan. After that we'll send it up to SIG-Node
Thanks for the update! I think we can hold on to this PR and wait for the more complete proposal to be presented in both SIGs.
// CPU resource restriction configuration. | ||
CPU *WindowsCPUResources `json:"cpu,omitempty"` | ||
// Storage restriction configuration. | ||
Storage *WindowsStorageResources `json:"storage,omitempty"` |
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 didn't see anywhere defining WindowsStorageResources. Since Storage is mentioned to support later in this proposal, can we remove this here?
… section Signed-off-by: Patrick Lang <plang@microsoft.com>
/ok-to-test |
// Specifies the portion of processor cycles that this container can use as a percentage times 100. | ||
int64 cpu_maximum = 3; | ||
// Memory limit in bytes. Default: 0 (not specified). | ||
int64 memory_limit_in_bytes = 4; |
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.
Could you give an example of how kubernetes cpu/memory requests/limit would map to these fields?
Also, are there any significant difference in these configurations and how kernel reacts to the conditions between linux and windows?
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.
example of how kubernetes cpu/memory requests/limit would map
// Refer https://github.com/moby/moby/blob/master/daemon/oci_windows.go#L265
// and https://github.com/Microsoft/hcsshim/blob/master/interface.go#L77.
CpuCount = int((container.Resources.Limits.Cpu().MilliValue() + 1000)/1000) // 0 if not set
// milliCPUToShares converts milliCPU to 0-10000
CpuShares = milliCPUToShares(container.Resources.Limits.Cpu().MilliValue())
if CpuShares == 0 {
CpuShares = milliCPUToShares(container.Resources.Request.Cpu().MilliValue())
}
CpuMaximum = container.Resources.Limits.Cpu().MilliValue()/sysinfo.NumCPU()/1000*10000
if isHyperV {
CpuMaximum = container.Resources.Limits.Cpu().MilliValue()/CpuCount/1000*10000
}
MemoryLimitInBytes = container.Resources.Limits.Memory().Value()
are there any significant difference in these configurations and how kernel reacts to the conditions between linux and windows?
Those CPU parameters are different from Linux, and there is no CFS on windows. this MSDN documentation explains how cpu/memory resource is controlled for Windows containers.
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.
Also to clarify - CpuMaximum is a limits that can't be exceeded. If the node is overprovisioned and under contention for multiple containers operating below their limit, then cycles will be weighted based on shares. If the system isn't overprovisioned then shares has no effect.
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 see. Thanks for explaining. Those are the things that I wanted to see in this proposal.
How does CpuCount interact with CpuMaximum?
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 for reviewing. I'll add to the proposal.
From https://github.com/docker/docker-ce/blob/master/components/cli/man/docker-run.1.md,
On Windows Server containers, the processor resource controls are mutually exclusive, the order of precedence is CPUCount first, then CPUShares, and CPUPercent last.
CPUMaximum is derived from CPUCount for HyperV container, and from sysinfo.NumCPU() for process container.
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.
Note that for Hyper-V containers these controls are not mutually exclusive. The CPUCount controls the number of virtual processors that the container has access to (and by extension, the host's logical processors available for execution). The CpuMaximum applies to each of these virtual processors independently, for example, CpuCount=2,CpuMaximum=5000 (50%) would limit each CPU to 50%. Running a single threaded application that uses as much CPU as available on the above configuration would be able to use at most 50% of a single host core.
As mentioned above, for Windows Server containers (process isolation) CpuCount would take precedence (and the cpu limit would be simulated based on the provided value compared to the number of host CPUs using JOB_OBJECT_CPU_RATE_CONTROL_HARD_CAP), and CpuMaximum would be ignored.
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 @darrenstahlmsft Thanks for the input. I have added part of your comment into design proposal. Is there any reference to what you have commented besides:
https://github.com/moby/moby/blob/master/daemon/oci_windows.go
https://github.com/Microsoft/hcsshim/blob/master/interface.go
https://github.com/docker/docker-ce/blob/master/components/cli/man/docker-run.1.md
https://msdn.microsoft.com/en-us/library/windows/desktop/hh448384.aspx
ping @dchen1107 @derekwaynecarr Would you like to take a look at this? |
6ed4994
to
a65926e
Compare
a65926e
to
2421112
Compare
|
||
// WindowsContainerResources specifies Windows specific configuration for | ||
// resources. | ||
message WindowsContainerResources { |
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 must miss something here. What are the between this: WindowsContainerResources and above API: WindowsResources?
From the explanation given below, what are defined here are the values passing to windows runtime?
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.
WindowsResources is from OCI spec (https://github.com/opencontainers/runtime-spec/blob/master/specs-go/config.go). It is essentially the same as WindowsContainerResources in CRI here.
LGTM thanks for updating with the mapping function too. |
@derekwaynecarr did you have any feedback? |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dchen1107, JiangtianLi The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. kubelet: setup WindowsContainerResources for windows containers **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**: ```release-note WindowsContainerResources is set now for windows containers ```
Design proposal for Windows Container Configuration in CRI
No description provided.