Skip to content
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

Merged
merged 6 commits into from
Feb 2, 2018

Conversation

JiangtianLi
Copy link
Contributor

No description provided.

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Dec 14, 2017
@k8s-github-robot k8s-github-robot added the sig/node Categorizes an issue or PR as relevant to SIG Node. label Dec 14, 2017
@JiangtianLi
Copy link
Contributor Author

/sig node
/sig windows

@k8s-ci-robot k8s-ci-robot added the sig/windows Categorizes an issue or PR as relevant to SIG Windows. label Dec 14, 2017
@JiangtianLi
Copy link
Contributor Author

/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.
Copy link
Contributor

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?).

Copy link
Contributor Author

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).

Copy link
Contributor

@PatrickLang PatrickLang Dec 27, 2017

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

Copy link
Contributor

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

Copy link
Contributor

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"`
Copy link
Member

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?

Patrick Lang and others added 2 commits January 22, 2018 09:45
@feiskyer
Copy link
Member

/ok-to-test

@k8s-ci-robot k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jan 23, 2018
// 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;
Copy link
Contributor

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?

Copy link
Member

@feiskyer feiskyer Jan 24, 2018

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.

Copy link
Contributor

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.

Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link

@darstahl darstahl Jan 25, 2018

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@feiskyer
Copy link
Member

ping @dchen1107 @derekwaynecarr Would you like to take a look at this?

@JiangtianLi JiangtianLi force-pushed the jiangtli-wincri branch 5 times, most recently from 6ed4994 to a65926e Compare January 26, 2018 07:11

// WindowsContainerResources specifies Windows specific configuration for
// resources.
message WindowsContainerResources {
Copy link
Member

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?

Copy link
Contributor Author

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.

@dchen1107
Copy link
Member

LGTM thanks for updating with the mapping function too.

@PatrickLang
Copy link
Contributor

@derekwaynecarr did you have any feedback?

@dchen1107
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 2, 2018
@k8s-ci-robot
Copy link
Contributor

[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 /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 2, 2018
@k8s-ci-robot k8s-ci-robot merged commit 79082b2 into kubernetes:master Feb 2, 2018
k8s-github-robot pushed a commit to kubernetes/kubernetes that referenced this pull request Feb 28, 2018
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
```
MadhavJivrajani pushed a commit to MadhavJivrajani/community that referenced this pull request Nov 30, 2021
Design proposal for Windows Container Configuration in CRI
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/windows Categorizes an issue or PR as relevant to SIG Windows. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants