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 was sending negative allocatable values #46516

Merged

Conversation

derekwaynecarr
Copy link
Member

What this PR does / why we need it:
if you set reservations > node capacity, the node sent negative values for allocatable values on create. setting negative values on update is rejected.

Which issue this PR fixes
xref https://bugzilla.redhat.com/show_bug.cgi?id=1455420

Special notes for your reviewer:
at this time, the node is allowed to set status on create. without this change, a node was being registered with negative allocatable values. i think we need to revisit letting node set status on create, and i will send a separate pr to debate the merits of that point.

Prevent kubelet from setting allocatable < 0 for a resource upon initial creation.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 26, 2017
@derekwaynecarr
Copy link
Member Author

cc @sjenning - ptal

@k8s-github-robot k8s-github-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels May 26, 2017
@sjenning
Copy link
Contributor

@derekwaynecarr just to check my understanding. If the reserve > capacity and allocatable value would be negative, we return 0 instead, which effectively makes the node unschedulable right?

@sjenning
Copy link
Contributor

@derekwaynecarr also, do we need a test? I don't see an existing test for exercising the node allocatable reservation stuff.

@k8s-github-robot k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels May 26, 2017
@derekwaynecarr
Copy link
Member Author

@sjenning - i added a test. the node is not necessarily unschedulable. it depends on the resource. if you have 3 gpus, and reserve 4, the node will work just fine. if you have 4 cpus, and reserve 5, the node will run besteffort pods just fine. if you have 4Gi memory, and reserve 5Gi, we will evict all pods and always report memory pressure.

for reference, node allocatable cgroup enforcement protects for this:
https://github.com/kubernetes/kubernetes/blob/master/pkg/kubelet/cm/node_container_manager.go#L173

Copy link
Contributor

@sjenning sjenning left a comment

Choose a reason for hiding this comment

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

Looks good

@sjenning
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 May 26, 2017
@dchen1107
Copy link
Member

/lgtm

Should we cherrypick this to 1.6? cc/ @dashpole

@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dchen1107, derekwaynecarr, sjenning

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-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 45809, 46515, 46484, 46516, 45614)

@k8s-github-robot k8s-github-robot merged commit 71e0204 into kubernetes:master May 26, 2017
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. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants