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
Member

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

This comment has been minimized.

Member

feiskyer commented Feb 5, 2018

/sig node
/sig windows

@feiskyer

This comment has been minimized.

Member

feiskyer commented Feb 5, 2018

@k8s-ci-robot k8s-ci-robot requested review from michmike and JiangtianLi Feb 5, 2018

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Feb 5, 2018

@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

This comment has been minimized.

Member

feiskyer commented Feb 5, 2018

/retest

wc.Resources.CpuMaximum = cpuMaximum
}
cpuShares := milliCPUToShares(cpuLimit.MilliValue())

This comment has been minimized.

@JiangtianLi

JiangtianLi Feb 6, 2018

Contributor

Windows cpushares range from 1-10000, we need a different calculation than the one used by Linux. @darrenstahlmsft Any pointer to calculate cpushares?

This comment has been minimized.

@bsteciuk

bsteciuk Feb 6, 2018

Contributor

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

This comment has been minimized.

@feiskyer

feiskyer Feb 7, 2018

Member

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

This comment has been minimized.

@darstahl

darstahl Feb 12, 2018

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.

This comment has been minimized.

@feiskyer

feiskyer Feb 13, 2018

Member

@darrenstahlmsft So how do you suggest to calc shares? factor by 10 for hyperv and 5000 by process?

@feiskyer feiskyer added this to the v1.10 milestone Feb 8, 2018

@feiskyer

This comment has been minimized.

Member

feiskyer commented Feb 11, 2018

Rebased and resolved conflicts. @dchen1107 @michmike PTAL

@feiskyer

This comment has been minimized.

Member

feiskyer commented Feb 11, 2018

/retest

cpuMaximum = 10000
}
wc.Resources.CpuCount = cpuCount

This comment has been minimized.

@darstahl

darstahl Feb 12, 2018

In the case of a container not isolated by Hyper-V, setting the CpuCount and CpuMaximum will only use the CpuCount which has much less precision than using CpuMaximum. I'd suggest only setting CpuCount for Hyper-V isolated containers when setting based on MilliCpus. See for examples the Moby setting based on NanoCPUs

This comment has been minimized.

@feiskyer

feiskyer Feb 13, 2018

Member

ACK, will move cpuCount setting inside hyperv check loop

cpuLimit := container.Resources.Limits.Cpu()
if !cpuLimit.IsZero() {
cpuCount := int64(cpuLimit.MilliValue()+999) / 1000
cpuMaximum := 10000 * cpuLimit.MilliValue() / int64(sysinfo.NumCPU()) / 1000

This comment has been minimized.

@darstahl

darstahl Feb 12, 2018

Note that this method of getting the count of CPUs sysinfo.NumCPU() is limited to 64 CPUs on Windows due to Processor Groups, as only 64 processors are available for execution by a given process without extra work to schedule them, which Go does not support. This causes some oddities on systems with more than 64 processors (eg. requesting 64 cpus on a 128 core system will give 100% of the platform's CPUs). I don't have a solution right now, as I need to do some more digging on how Job Objects support scheduling across Processor Groups in order to understand this better in the process isolation case, and I don't have a 128 core machine on hand to test with. I don't think this should block this PR, but it is a limitation to be aware of.

This same bug exists in Moby, assigned to me when I can get back to it.

This comment has been minimized.

@feiskyer

feiskyer Feb 13, 2018

Member

ACK, will add a comment of this limitation.

wc.Resources.CpuMaximum = cpuMaximum
}
cpuShares := milliCPUToShares(cpuLimit.MilliValue())

This comment has been minimized.

@darstahl

darstahl Feb 12, 2018

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.

@jdumars

This comment has been minimized.

Member

jdumars commented Feb 13, 2018

@khenidak could you do the LGTM when this looks good to you? Thanks!

@khenidak

This comment has been minimized.

Contributor

khenidak commented Feb 14, 2018

@jdumars -- @JiangtianLi is much better qualified to review and LGTM this one.

@jberkus

This comment has been minimized.

jberkus commented Feb 21, 2018

As it is now Code Slush, this PR needs approved-for-milestone to merge now. Please add when ready.

@JiangtianLi

This comment has been minimized.

Contributor

JiangtianLi commented Feb 21, 2018

/lgtm

@feiskyer

This comment has been minimized.

Member

feiskyer commented Feb 23, 2018

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

@feiskyer

This comment has been minimized.

Member

feiskyer commented Feb 24, 2018

Made another rebase

@feiskyer

This comment has been minimized.

Member

feiskyer commented Feb 24, 2018

@dchen1107

This comment has been minimized.

Member

dchen1107 commented Feb 27, 2018

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

This comment has been minimized.

@JiangtianLi

JiangtianLi Feb 28, 2018

Contributor

Nit: Copyright 2018

This comment has been minimized.

@feiskyer

@k8s-ci-robot k8s-ci-robot added the lgtm label Feb 28, 2018

@JiangtianLi

/lgtm

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Feb 28, 2018

[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

This comment has been minimized.

Member

feiskyer commented Feb 28, 2018

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Feb 28, 2018

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

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Feb 28, 2018

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

@k8s-merge-robot k8s-merge-robot merged commit a21a750 into kubernetes:master Feb 28, 2018

12 of 13 checks passed

Submit Queue Required Github CI test is not green: pull-kubernetes-verify
Details
cla/linuxfoundation feiskyer authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-unit Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details

@feiskyer feiskyer deleted the feiskyer:win branch Feb 28, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment