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

KEP-2570: Support memory qos with cgroups v2 #2571

Merged
merged 1 commit into from
May 13, 2021
Merged

KEP-2570: Support memory qos with cgroups v2 #2571

merged 1 commit into from
May 13, 2021

Conversation

xiaoxubeii
Copy link
Member

@xiaoxubeii xiaoxubeii commented Mar 14, 2021 •
edited
Loading

This proposal introduces memory qos with cgroups v2 for pod/container and qos/node level.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 14, 2021
@k8s-ci-robot k8s-ci-robot added kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory sig/node Categorizes an issue or PR as relevant to SIG Node. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 14, 2021
@xiaoxubeii xiaoxubeii mentioned this pull request Mar 14, 2021
Open
8 tasks
@k8s-ci-robot k8s-ci-robot assigned giuseppe and unassigned giuseppe Mar 15, 2021
@bobbypage
Copy link
Member

/cc

@k8s-ci-robot k8s-ci-robot requested a review from bobbypage March 16, 2021 01:30
bobbypage
bobbypage reviewed Mar 16, 2021
View reviewed changes
keps/sig-node/2570-memory-qos/README.md Outdated Show resolved Hide resolved
keps/sig-node/2570-memory-qos/README.md Outdated Show resolved Hide resolved
keps/sig-node/2570-memory-qos/README.md Outdated Show resolved Hide resolved
keps/sig-node/2570-memory-qos/README.md Outdated Show resolved Hide resolved
keps/sig-node/2570-memory-qos/README.md Outdated Show resolved Hide resolved
keps/sig-node/2570-memory-qos/README.md Outdated Show resolved Hide resolved
@xiaoxubeii xiaoxubeii requested a review from bobbypage March 17, 2021 11:25
@xiaoxubeii xiaoxubeii force-pushed the kep-memory-qos branch 2 times, most recently from c696fa8 to 454ed3c Compare March 19, 2021 09:42
@xiaoxubeii xiaoxubeii requested a review from bobbypage March 22, 2021 02:17
@xiaoxubeii xiaoxubeii force-pushed the kep-memory-qos branch 2 times, most recently from acbf716 to 37d8e31 Compare March 24, 2021 09:28
@xiaoxubeii
Copy link
Member Author

@bobbypage New updates have removed memory.high related contents. Please review again. Thanks : )

@xiaoxubeii
Copy link
Member Author

xiaoxubeii commented Mar 25, 2021 •
edited
Loading

cc @kubernetes/sig-node-pr-reviews @kubernetes/sig-node-proposals @kubernetes/sig-node-feature-requests

@k8s-ci-robot k8s-ci-robot added the kind/design Categorizes issue or PR as related to design. label Mar 25, 2021
@xiaoxubeii xiaoxubeii force-pushed the kep-memory-qos branch 3 times, most recently from bb416be to 6bc1cec Compare March 25, 2021 13:37
@k8s-ci-robot k8s-ci-robot added the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Mar 25, 2021
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 12, 2021
@mrunalp
Copy link
Contributor

mrunalp commented May 12, 2021

/approve

@mrunalp
Copy link
Contributor

mrunalp commented May 12, 2021

/assign @johnbelamaric

@xiaoxubeii
Copy link
Member Author

Thank you for making the updates.

After meeting with Red Hat kernel engineers, @mrunalp and I felt like we could leave the KEP language on memory.min and memory.low a little more open. There is an argument that we should always map requests to memory.low and we wanted to basically measure and observe the real world impact over the iteration of this feature. If you want to soften the language on that to say its open to iterative testing, I think this is largely lgtm from my part.

Of course no problem, I will keep watching for that. Really appreciate for that :)

@xiaoxubeii
Copy link
Member Author

/cc @ehashman

@k8s-ci-robot k8s-ci-robot requested a review from ehashman May 12, 2021 23:12
johnbelamaric
johnbelamaric reviewed May 12, 2021
View reviewed changes
keps/sig-node/2570-memory-qos/README.md Outdated
- [x] Feature gate (also fill in values in `kep.yaml`)
- Feature gate name: MemoryQoS
- Components depending on the feature gate: kubelet
- [x] Other
Copy link
Member

Choose a reason for hiding this comment

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

What other mechanism is used? It's not just the feature gate?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is typos, have updated.

keps/sig-node/2570-memory-qos/README.md Outdated
with and without the feature, are necessary. At the very least, think about
conversion tests if API types are being modified.
-->
Yes, some unit tests are exercised with the feature both enabled and disabled to verify proper behavior in both cases.
Copy link
Member

Choose a reason for hiding this comment

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

What about transitioning from enabled to disabled and vice-versa? Is there a way to test that changes to running workloads' cgroups happen the way you expect?

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated test details.

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 13, 2021
@xiaoxubeii xiaoxubeii requested a review from johnbelamaric May 13, 2021 00:59
@johnbelamaric
Copy link
Member

/approve
/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 13, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: derekwaynecarr, giuseppe, johnbelamaric, mrunalp, xiaoxubeii

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 May 13, 2021
@k8s-ci-robot k8s-ci-robot merged commit 2b3a1cc into kubernetes:master May 13, 2021
@k8s-ci-robot k8s-ci-robot added this to the v1.22 milestone May 13, 2021
@derekwaynecarr derekwaynecarr mentioned this pull request May 13, 2021
Merged
@payall4u payall4u mentioned this pull request Jun 4, 2021
Merged
borgerli pushed a commit to borgerli/kubernetes that referenced this pull request Jun 4, 2021
@payall4u @borgerli
feature: support memory qos for cgroup v2
75f7521 
Map container.requests.memory to memory.min on cgroup v2.
See kubernetes/enhancements#2571
payall4u added a commit to tkestack/kubernetes that referenced this pull request Jun 4, 2021
@payall4u
feature: support memory qos for cgroup v2
cecd80e 
Map container.requests.memory to memory.min on cgroup v2 and update cm
to support kubelet/systemd memory qos.
See kubernetes/enhancements#2571

Signed-off-by: payall4u <payall4u@qq.com>
borgerli pushed a commit to tkestack/kubernetes that referenced this pull request Jun 5, 2021
@payall4u @borgerli
feature: support memory qos for cgroup v2
9fc7922 
Map container.requests.memory to memory.min on cgroup v2 and update cm
to support kubelet/systemd memory qos.
See kubernetes/enhancements#2571

Signed-off-by: payall4u <payall4u@qq.com>
@borgerli borgerli mentioned this pull request Jun 5, 2021
Closed
payall4u added a commit to payall4u/kubernetes that referenced this pull request Jun 15, 2021
@payall4u
feature: support memory qos for cgroup v2
6ea0ce6 
Map container.requests.memory to memory.min on cgroup v2 and update cm
to support kubelet/systemd memory qos.
See kubernetes/enhancements#2571

Signed-off-by: payall4u <payall4u@qq.com>
@borgerli borgerli mentioned this pull request Jun 18, 2021
Merged
@payall4u payall4u mentioned this pull request Jun 21, 2021
Merged
2 tasks
@xiaoxubeii xiaoxubeii mentioned this pull request Jun 22, 2021
Merged
@payall4u payall4u mentioned this pull request Aug 23, 2021
Merged
@saintube saintube mentioned this pull request Apr 27, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@derekwaynecarr derekwaynecarr derekwaynecarr requested changes

@giuseppe giuseppe giuseppe approved these changes

@dchen1107 dchen1107 Awaiting requested review from dchen1107

@bobbypage bobbypage Awaiting requested review from bobbypage

@mrunalp mrunalp Awaiting requested review from mrunalp

@odinuge odinuge Awaiting requested review from odinuge

@ehashman ehashman Awaiting requested review from ehashman

@johnbelamaric johnbelamaric Awaiting requested review from johnbelamaric

Assignees

@mrunalp mrunalp

@derekwaynecarr derekwaynecarr

@johnbelamaric johnbelamaric

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. kind/design Categorizes issue or PR as related to design. kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/node Categorizes an issue or PR as relevant to SIG Node. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v1.22
Development

Successfully merging this pull request may close these issues.
8 participants
@xiaoxubeii @bobbypage @derekwaynecarr @odinuge @mrunalp @johnbelamaric @k8s-ci-robot @giuseppe