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.

Sign up for GitHub

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

Jump to bottom

Enable allocatable support for Windows nodes #69960

Merged
merged 1 commit into from
Oct 31, 2018
Merged

Enable allocatable support for Windows nodes #69960

merged 1 commit into from
Oct 31, 2018

Conversation

feiskyer
Copy link
Member

@feiskyer feiskyer commented Oct 18, 2018 •
edited
Loading

What type of PR is this?

Uncomment only one, leave it on its own line:

/kind api-change
/kind bug
/kind cleanup
/kind design
/kind documentation
/kind failing-test
/kind feature
/kind flake

What this PR does / why we need it:

This PR enables kubelet options --system-reserved and --kube-reserved on Windows nodes.

Without this change, Windows nodes' allocatable are always same as capacity, even if you have set both options, e.g. --system-reserved=cpu=100m,memory=100Mi,ephemeral-storage=1Gi --kube-reserved=cpu=1200m,memory=100Mi,ephemeral-storage=1Gi:

Capacity:
 attachable-volumes-azure-disk:  4
 cpu:                            2
 ephemeral-storage:              0
 memory:                         8388148Ki
 pods:                           110
Allocatable:
 attachable-volumes-azure-disk:  4
 cpu:                            2
 ephemeral-storage:              0
 memory:                         8388148Ki
 pods:                           110

After this change, allocatable would be capacity- system-reserverd - kube-reserved:

Capacity:
 attachable-volumes-azure-disk:  4
 cpu:                            2
 ephemeral-storage:              268075004Ki
 memory:                         8388148Ki
 pods:                           110
Allocatable:
 attachable-volumes-azure-disk:  4
 cpu:                            700m
 ephemeral-storage:              244910439630
 memory:                         8080948Ki
 pods:                           110

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 #69938

Special notes for your reviewer:

Note that --enforce-node-allocatable is still not supported because there's no cgroups support on Windows.

Does this PR introduce a user-facing change?:

kubelet --system-reserved and --kube-reserved are supported now on Windows nodes

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/bug Categorizes issue or PR as related to a bug. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. area/kubelet sig/node Categorizes an issue or PR as relevant to SIG Node. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Oct 18, 2018
@feiskyer
Copy link
Member Author

/sig windows
/milestone v1.13

/assign @PatrickLang @yujuhong

@k8s-ci-robot k8s-ci-robot added the sig/windows Categorizes an issue or PR as relevant to SIG Windows. label Oct 18, 2018
@k8s-ci-robot k8s-ci-robot added this to the v1.13 milestone Oct 18, 2018
@neolit123
Copy link
Member

/retest

1 similar comment
@feiskyer
Copy link
Member Author

/retest

@feiskyer
Copy link
Member Author

Kindly ping @yujuhong @PatrickLang. Could you help to take a look at the PR?

@PatrickLang
Copy link
Contributor

/lgtm
Still need to do some testing on it though. @yujuhong or @pjh do you want to take a look?

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 23, 2018
@andyzhangx
Copy link
Member

/test pull-kubernetes-cross

@pjh
Copy link
Contributor

pjh commented Oct 24, 2018

Sounds like a nice improvement! I have nothing to add to the review. I'm not quite in a position to help test this yet, our automated tests on GCE are coming along but not fully ready.

@feiskyer
Copy link
Member Author

@pjh Thanks.

ping @yujuhong. Could you have a look at this?

yujuhong
yujuhong reviewed Oct 30, 2018
View reviewed changes
pkg/kubelet/cm/node_container_manager.go
@@ -213,30 +211,6 @@ func (cm *containerManagerImpl) GetNodeAllocatableReservation() v1.ResourceList
return result
}

// hardEvictionReservation returns a resourcelist that includes reservation of resources based on hard eviction thresholds.
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm...Rename this file to node_container_manager_linux.go

Copy link
Member Author

Choose a reason for hiding this comment

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

ACK

pkg/kubelet/cm/container_manager_windows.go Outdated
// Interface for cadvisor.
cadvisorInterface cadvisor.Interface
// Config of this node.
nodeConfig NodeConfig
}

var _ ContainerManager = &containerManagerImpl{}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: This is redundant.

pkg/kubelet/cm/container_manager_windows.go
}

func (cm *containerManagerImpl) InternalContainerLifecycle() InternalContainerLifecycle {
return &internalContainerLifecycleImpl{cpumanager.NewFakeManager()}
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the point of returning a fake manager in production?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is actually same with stub container manager, which is only for conforming to the interfaces, but without actual actions.

pkg/kubelet/cm/container_manager_windows.go
@@ -19,28 +19,149 @@ limitations under the License.
package cm

import (
"github.com/golang/glog"
"fmt"
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a paragraph comment in this file explaining the status of this "containermangerImpl" on windows.

Copy link
Member Author

Choose a reason for hiding this comment

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

ACK

@yujuhong
Copy link
Contributor

Looks good overall with some nits.
Will approve once they are addressed (since the PR has already been lgtm'd).

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 30, 2018
@feiskyer
Copy link
Member Author

@yujuhong Addressed comments. PTAL

@feiskyer
Copy link
Member Author

/test pull-kubernetes-local-e2e-containerized

@PatrickLang
Copy link
Contributor

/lgtm

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

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: feiskyer, yujuhong

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 30, 2018
@k8s-ci-robot k8s-ci-robot merged commit 5a8f831 into kubernetes:master Oct 31, 2018
@feiskyer feiskyer deleted the win-allocatable branch October 31, 2018 05:05
@rhockenbury rhockenbury mentioned this pull request Feb 18, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yujuhong yujuhong yujuhong left review comments

@mtaufen mtaufen Awaiting requested review from mtaufen

@tmrts tmrts Awaiting requested review from tmrts

Assignees

@yujuhong yujuhong

@PatrickLang PatrickLang

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/kubelet cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. 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/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v1.13
Development

Successfully merging this pull request may close these issues.

enforce-node-allocatable=kube-reserved/system-reserved don't work on Windows
7 participants
@feiskyer @neolit123 @PatrickLang @andyzhangx @pjh @yujuhong @k8s-ci-robot