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

kubelet: setup WindowsContainerResources for windows containers #59333

Merged
merged 4 commits into from
Feb 28, 2018

Conversation

feiskyer
Copy link
Member

@feiskyer feiskyer commented Feb 5, 2018

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:

WindowsContainerResources is set now for windows containers

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Feb 5, 2018
@feiskyer
Copy link
Member Author

feiskyer commented Feb 5, 2018

/sig node
/sig windows

@k8s-ci-robot k8s-ci-robot added sig/node Categorizes an issue or PR as relevant to SIG Node. sig/windows Categorizes an issue or PR as relevant to SIG Windows. labels Feb 5, 2018
@feiskyer
Copy link
Member Author

feiskyer commented Feb 5, 2018

@k8s-ci-robot
Copy link
Contributor

@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:

/cc @PatrickLang @taylorb-microsoft @michmike @JiangtianLi

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.

@feiskyer
Copy link
Member Author

feiskyer commented Feb 5, 2018

/retest

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 6, 2018
wc.Resources.CpuMaximum = cpuMaximum
}

cpuShares := milliCPUToShares(cpuLimit.MilliValue())
Copy link
Contributor

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?

Copy link
Contributor

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

Copy link
Member Author

@feiskyer feiskyer Feb 7, 2018

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

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.

Copy link
Member Author

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?

@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 7, 2018
@feiskyer feiskyer added this to the v1.10 milestone Feb 8, 2018
@k8s-github-robot k8s-github-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Feb 8, 2018
@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 9, 2018
@feiskyer
Copy link
Member Author

Rebased and resolved conflicts. @dchen1107 @michmike PTAL

@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 11, 2018
@feiskyer
Copy link
Member Author

/retest

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 22, 2018
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 23, 2018
@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 23, 2018
@feiskyer
Copy link
Member Author

Rebased and addressed comments. @JiangtianLi @dchen1107 @darrenstahlmsft PTAL

@feiskyer feiskyer force-pushed the win branch 3 times, most recently from a7837f1 to f2ea654 Compare February 23, 2018 07:47
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 24, 2018
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 24, 2018
@feiskyer
Copy link
Member Author

Made another rebase

@feiskyer
Copy link
Member Author

ping @dchen1107 @JiangtianLi

@dchen1107
Copy link
Member

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

@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 27, 2018
// +build windows

/*
Copyright 2017 The Kubernetes Authors.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Copyright 2018

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

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

@JiangtianLi JiangtianLi left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot
Copy link
Contributor

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

@feiskyer
Copy link
Member Author

@dchen1107 @JiangtianLi Thanks

@k8s-github-robot
Copy link

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot
Copy link

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit a21a750 into kubernetes:master Feb 28, 2018
@feiskyer feiskyer deleted the win branch February 28, 2018 04:44
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. release-note Denotes a PR that will be considered when it comes time to generate release notes. 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/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet