for every huge page resource, we need to remove it from allocatable memory when Updating Node Allocatable limit across pods #86758

mysunshine92 · 2020-01-01T16:04:46Z

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespace from that line:

/kind bug

What this PR does / why we need it:

we need to remove huge page resource from allocatable memory when Updating Node Allocatable limit across pods.
here:
https://github.com/kubernetes/kubernetes/blob/master/pkg/kubelet/cm/node_container_manager_linux.go#L177

Which issue(s) this PR fixes:

Fixes #84426

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

/sig node

mattjmcnaughton

@boddumanohar I see you commented on the issue - do you mind weighing in here? From your comment, I'm having a little trouble determining if you were in favor of, or opposed to, this proposed change :)

k8s-ci-robot · 2020-01-02T01:39:58Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: mysunshine92
To complete the pull request process, please assign vishh
You can assign the PR to them by writing /assign @vishh in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

pkg/kubelet/cm/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

mysunshine92 · 2020-01-02T01:45:11Z

/kind bug

drsantos20

Hi @mysunshine92 do we have tests for this change?

mysunshine92 · 2020-01-02T06:29:20Z

Hi @mysunshine92 do we have tests for this change?

I have test on my k8s 1.13 cluster，it's ok.

mysunshine92 · 2020-01-02T07:13:41Z

/test pull-kubernetes-e2e-gce

mysunshine92 · 2020-01-02T08:12:47Z

/test pull-kubernetes-e2e-gce

mattjmcnaughton · 2020-01-02T13:16:37Z

Hi @mysunshine92 do we have tests for this change?

I have test on my k8s 1.13 cluster，it's ok.

Gotcha! Thoughts on if unit tests could also be beneficial? The advantageous aspect of unit tests is that they will run continuously, ensuring this feature continues to work in the future.

…emory when Updating Node Allocatable limit across pods

mysunshine92 · 2020-01-03T01:02:09Z

/test pull-kubernetes-e2e-gce

k8s-ci-robot · 2020-01-03T01:57:02Z

@mysunshine92: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-conformance-kind-ipv6-parallel	b2865abb319ebcd2b062abd50289888e080ff6cc	link	`/test pull-kubernetes-conformance-kind-ipv6-parallel`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

mysunshine92 · 2020-01-03T10:48:45Z

/test pull-kubernetes-e2e-gce

mysunshine92 · 2020-01-03T11:51:21Z

/lgtm

k8s-ci-robot · 2020-01-03T11:51:22Z

@mysunshine92: you cannot LGTM your own PR.

In response to this:

/lgtm

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.

mysunshine92 · 2020-01-03T11:51:57Z

/priority backlog

mysunshine92 · 2020-01-03T11:54:40Z

/assign @vishh

odinuge

This is exactly the same as we do when calculating node.Status.Allocatable[v1.ResourceMemory] here: https://github.com/kubernetes/kubernetes/blob/0599ca2/pkg/kubelet/nodestatus/setters.go#L354-L366

Think I would prefer a test in order to avoid regressions tho.

Other than the comments this looks good to me

/priority important-longterm

odinuge · 2020-01-07T08:39:22Z

pkg/kubelet/cm/node_container_manager_linux.go

@@ -190,6 +191,21 @@ func (cm *containerManagerImpl) getNodeAllocatableAbsoluteImpl(capacity v1.Resou
 		}
 		result[k] = value
 	}
+
+	// for every huge page reservation, we need to remove it from allocatable memory
+	for k, v := range result {


Suggested change

for k, v := range result {

for k, v := range capacity {

I believe this should be calculated by capacity not allocatable?

since memory uses does not include hugepage uses，here we should use allocatable

since memory uses does not include hugepage uses，here we should use allocatable

Not sure if I follow. If we reserve huge page memory in system-reserved and/or kube-reserved, we would like to decrement by all huge page memory, not only allocatable. Or?

pkg/kubelet/cm/node_container_manager_linux.go

+	// for every huge page reservation, we need to remove it from allocatable memory
+	for k, v := range result {
+		if v1helper.IsHugePageResourceName(k) {
+			allocatableMemory := result[v1.ResourceMemory]


odinuge · 2020-02-20T10:17:52Z

gentle ping @mysunshine92.

Would be nice to get this into 1.18 😄

mysunshine92 · 2020-04-12T03:18:50Z

gentle ping @mysunshine92.

Would be nice to get this into 1.18

please add lgtm labels，thanks

odinuge · 2020-06-08T11:25:46Z

ping @mysunshine92

k8s-ci-robot · 2020-06-08T11:25:53Z

@mysunshine92: PR needs rebase.

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.

fejta-bot · 2020-09-06T11:34:34Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

fejta-bot · 2020-10-06T12:17:49Z

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

mysunshine92 · 2020-11-05T09:33:24Z

/close

k8s-ci-robot · 2020-11-05T09:33:39Z

@mysunshine92: Closed this PR.

In response to this:

/close

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.

k8s-ci-robot requested review from resouer and yifan-gu January 1, 2020 16:05

k8s-ci-robot added the area/kubelet label Jan 1, 2020

mattjmcnaughton reviewed Jan 1, 2020

View reviewed changes

mysunshine92 force-pushed the cgroup-remove-hugepage-from-memory branch from 35d124f to b2865ab Compare January 2, 2020 01:20

mysunshine92 force-pushed the cgroup-remove-hugepage-from-memory branch from b2865ab to 706dcaf Compare January 2, 2020 01:39

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jan 2, 2020

k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. and removed needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Jan 2, 2020

drsantos20 reviewed Jan 2, 2020

View reviewed changes

mysunshine92 force-pushed the cgroup-remove-hugepage-from-memory branch from 706dcaf to 90cf46d Compare January 2, 2020 06:17

for every huge page resource, we need to remove it from allocatable m…

90cf46d

…emory when Updating Node Allocatable limit across pods

k8s-ci-robot added priority/backlog Higher priority than priority/awaiting-more-evidence. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Jan 3, 2020

k8s-ci-robot assigned vishh Jan 3, 2020

odinuge reviewed Jan 7, 2020

View reviewed changes

k8s-ci-robot added the priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. label Jan 7, 2020

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 8, 2020

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 6, 2020

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Oct 6, 2020

k8s-ci-robot closed this Nov 5, 2020

odinuge mentioned this pull request Apr 25, 2021

Delete the hugePages capacity from the memory resource capacity #99943

Closed

This was referenced Jul 8, 2023

remove huge page form allocatable memory #119171

Closed

remove huge page form allocatable memory #119172

Closed

Remove huge page form allocatable memory #119173

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

for every huge page resource, we need to remove it from allocatable memory when Updating Node Allocatable limit across pods #86758

for every huge page resource, we need to remove it from allocatable memory when Updating Node Allocatable limit across pods #86758

mysunshine92 commented Jan 1, 2020

mattjmcnaughton left a comment

k8s-ci-robot commented Jan 2, 2020

mysunshine92 commented Jan 2, 2020

drsantos20 left a comment

mysunshine92 commented Jan 2, 2020

mysunshine92 commented Jan 2, 2020

mysunshine92 commented Jan 2, 2020

mattjmcnaughton commented Jan 2, 2020

mysunshine92 commented Jan 3, 2020

k8s-ci-robot commented Jan 3, 2020 •

edited

mysunshine92 commented Jan 3, 2020

mysunshine92 commented Jan 3, 2020

k8s-ci-robot commented Jan 3, 2020

mysunshine92 commented Jan 3, 2020

mysunshine92 commented Jan 3, 2020

odinuge left a comment

odinuge Jan 7, 2020

odinuge Jan 7, 2020

mysunshine92 Apr 12, 2020

odinuge Jun 8, 2020

This comment was marked as resolved.

odinuge commented Feb 20, 2020

mysunshine92 commented Apr 12, 2020

odinuge commented Jun 8, 2020

k8s-ci-robot commented Jun 8, 2020

fejta-bot commented Sep 6, 2020

fejta-bot commented Oct 6, 2020

mysunshine92 commented Nov 5, 2020

k8s-ci-robot commented Nov 5, 2020

for every huge page resource, we need to remove it from allocatable memory when Updating Node Allocatable limit across pods #86758

for every huge page resource, we need to remove it from allocatable memory when Updating Node Allocatable limit across pods #86758

Conversation

mysunshine92 commented Jan 1, 2020

mattjmcnaughton left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jan 2, 2020

mysunshine92 commented Jan 2, 2020

drsantos20 left a comment

Choose a reason for hiding this comment

mysunshine92 commented Jan 2, 2020

mysunshine92 commented Jan 2, 2020

mysunshine92 commented Jan 2, 2020

mattjmcnaughton commented Jan 2, 2020

mysunshine92 commented Jan 3, 2020

k8s-ci-robot commented Jan 3, 2020 • edited

mysunshine92 commented Jan 3, 2020

mysunshine92 commented Jan 3, 2020

k8s-ci-robot commented Jan 3, 2020

mysunshine92 commented Jan 3, 2020

mysunshine92 commented Jan 3, 2020

odinuge left a comment

Choose a reason for hiding this comment

odinuge Jan 7, 2020

Choose a reason for hiding this comment

odinuge Jan 7, 2020

Choose a reason for hiding this comment

mysunshine92 Apr 12, 2020

Choose a reason for hiding this comment

odinuge Jun 8, 2020

Choose a reason for hiding this comment

This comment was marked as resolved.

odinuge commented Feb 20, 2020

mysunshine92 commented Apr 12, 2020

odinuge commented Jun 8, 2020

k8s-ci-robot commented Jun 8, 2020

fejta-bot commented Sep 6, 2020

fejta-bot commented Oct 6, 2020

mysunshine92 commented Nov 5, 2020

k8s-ci-robot commented Nov 5, 2020

k8s-ci-robot commented Jan 3, 2020 •

edited