Skip to content

use memory.min for reservation memory instead of high#3208

Merged
bobbypage merged 1 commit into
google:masterfrom
pacoxu:patch-2
Jan 5, 2023
Merged

use memory.min for reservation memory instead of high#3208
bobbypage merged 1 commit into
google:masterfrom
pacoxu:patch-2

Conversation

@pacoxu
Copy link
Copy Markdown
Contributor

@pacoxu pacoxu commented Dec 2, 2022

high is throttling bar

Fixes #3207

File Description
memory.min memory.min specifies a minimum amount of memory the cgroup must always retain, i.e., memory that can never be reclaimed by the system. If the cgroup's memory usage reaches this low limit and can’t be increased, the system OOM killer will be invoked. We map it to requests.memory.
memory.max memory.max is the memory usage hard limit, acting as the final protection mechanism: If a cgroup's memory usage reaches this limit and can't be reduced, the system OOM killer is invoked on the cgroup. Under certain circumstances, usage may go over the memory.high limit temporarily. When the high limit is used and monitored properly, memory.max serves mainly to provide the final safety net. The default is max. We map it to limits.memory as consistent with existing memory.limit_in_bytes for cgroups v1.
memory.low memory.low is the best-effort memory protection, a "soft guarantee" that if the cgroup and all its descendants are below this threshold, the cgroup's memory won't be reclaimed unless memory can’t be reclaimed from any unprotected cgroups. Not yet considered for now.
memory.high memory.high is the memory usage throttle limit. This is the main mechanism to control a cgroup's memory use. If a cgroup's memory use goes over the high boundary specified here, the cgroup’s processes are throttled and put under heavy reclaim pressure. The default is max, meaning there is no limit. We use a formula to calculate memory.high depending on limits.memory/node allocatable memory and a memory throttling factor.

This was added in #2309.
/cc @dashpole @giuseppe

@k8s-ci-robot
Copy link
Copy Markdown
Collaborator

Hi @pacoxu. Thanks for your PR.

I'm waiting for a google member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

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.

Copy link
Copy Markdown
Contributor

@giuseppe giuseppe left a comment

Choose a reason for hiding this comment

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

LGTM

@dashpole
Copy link
Copy Markdown
Collaborator

dashpole commented Dec 2, 2022

/ok-to-test

@pacoxu
Copy link
Copy Markdown
Contributor Author

pacoxu commented Dec 2, 2022

The test failure is fixed.

@bobbypage bobbypage closed this Jan 4, 2023
@bobbypage bobbypage reopened this Jan 4, 2023
@bobbypage bobbypage closed this Jan 4, 2023
@bobbypage bobbypage reopened this Jan 4, 2023
@bobbypage
Copy link
Copy Markdown
Contributor

kick CI

@bobbypage
Copy link
Copy Markdown
Contributor

LGTM on the change

@bobbypage
Copy link
Copy Markdown
Contributor

CI is being a bit weird

error obtaining VCS status: exit status 128
	Use -buildvcs=false to disable VCS stamping.
+ delete
Deleting /tmp/tmp.NnvwDGEsX6...
+ echo 'Deleting /tmp/tmp.NnvwDGEsX6...'
+ [[ 1001 -ne 0 ]]
+ sudo rm -rf /tmp/tmp.NnvwDGEsX6
make: *** [Makefile:45: docker-test-integration] Error 1
Error: Process completed with exit code 2.

@bobbypage
Copy link
Copy Markdown
Contributor

Can you please rebase the PR against master? #3224 should fix the CI issue

@pacoxu
Copy link
Copy Markdown
Contributor Author

pacoxu commented Jan 5, 2023

Can you please rebase the PR against master? #3224 should fix the CI issue

Rebased, the failed test passes now.

@bobbypage
Copy link
Copy Markdown
Contributor

Thanks!

@bobbypage bobbypage merged commit c7714a7 into google:master Jan 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Memory.Reservation is using memory.high but kube is using memory.min as request

5 participants