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: new kubelet config option for disabling group oom kill #126096

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

utam0k
Copy link
Member

@utam0k utam0k commented Jul 15, 2024

What type of PR is this?

/kind feature

What this PR does / why we need it:

There is a difference in the OOM behavior between cgroup v1 and v2 by default. It's important to note that in cgroup v1 if one of the processes within a container is killed, the other processes will remain running. Ideally, we might want to align with cgroup v2's behavior, where all processes within a container are killed if an OOM event occurs. However, in reality, we need to maintain the behavior of cgroup v1 for the time being. Therefore, this pull request will introduce the singleProcessOOMKill flag to enable like cgroup v1's behavior in cgroup v2.

Appearently, this workaround has already been discussed in a SIG-Node meeting.
#117793 (comment)

Which issue(s) this PR fixes:

Carry: #122813 by @tzneal

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Added `singleProcessOOMKill` flag to the kubelet configuration. Setting that to true enable single process OOM killing in cgroups v2. In this mode, if a single process is OOM killed within a container, the remaining processes will not be OOM killed.

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


@k8s-ci-robot
Copy link
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. 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. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jul 15, 2024
@k8s-ci-robot
Copy link
Contributor

This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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-sigs/prow repository.

@k8s-ci-robot k8s-ci-robot added the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Jul 15, 2024
@k8s-ci-robot k8s-ci-robot added area/code-generation area/kubelet area/test kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API 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. sig/testing Categorizes an issue or PR as relevant to SIG Testing. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jul 15, 2024
@utam0k utam0k force-pushed the support-disabling-oom-group-kill branch from 12311ee to 6e7d175 Compare July 15, 2024 06:43
@utam0k utam0k marked this pull request as ready for review July 15, 2024 06:45
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 15, 2024
@utam0k utam0k marked this pull request as draft July 15, 2024 06:54
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 15, 2024
@utam0k utam0k force-pushed the support-disabling-oom-group-kill branch from 6e7d175 to 00f4125 Compare July 15, 2024 12:01
@utam0k utam0k marked this pull request as ready for review July 18, 2024 12:04
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 18, 2024
@kannon92
Copy link
Contributor

/cc @tzneal

@k8s-ci-robot k8s-ci-robot requested a review from tzneal July 22, 2024 19:13
@kannon92
Copy link
Contributor

/label api-review

@k8s-ci-robot k8s-ci-robot added the api-review Categorizes an issue or PR as actively needing an API review. label Jul 22, 2024
@liggitt
Copy link
Member

liggitt commented Jul 23, 2024

cc @bobbypage for review of cgroupv1-compatible options when running with cgroupv2

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: utam0k
Once this PR has been reviewed and has the lgtm label, please assign liggitt for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found 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 sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Jul 24, 2024
@utam0k utam0k force-pushed the support-disabling-oom-group-kill branch 2 times, most recently from 907fc20 to d1b6bb6 Compare July 25, 2024 00:40
@cici37
Copy link
Contributor

cici37 commented Jul 25, 2024

/remove-sig api-machinery

@k8s-ci-robot k8s-ci-robot removed the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Jul 25, 2024
@kannon92
Copy link
Contributor

@utam0k It would be really nice to get this into 1.32. Can you please address @liggitt comments?

@yujuhong
Copy link
Contributor

@utam0k It would be really nice to get this into 1.32. Can you please address @liggitt comments?

+1

Would like to see this get in. If not, maybe we can try to find someone else to pick up?

@utam0k
Copy link
Member Author

utam0k commented Aug 22, 2024

I will make time on Saturday and Sunday for the challenge. If I can't, I will ask someone else to pick up it.

tzneal and others added 2 commits August 24, 2024 14:57
Co-authored-by: utam0k <k0ma@utam0k.jp>
Signed-off-by: utam0k <k0ma@utam0k.jp>
@utam0k utam0k force-pushed the support-disabling-oom-group-kill branch from d1b6bb6 to 392f7a6 Compare August 24, 2024 05:58
@k8s-ci-robot k8s-ci-robot added the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Aug 24, 2024
@utam0k utam0k force-pushed the support-disabling-oom-group-kill branch 2 times, most recently from 417caf3 to 1875ca5 Compare August 24, 2024 08:10
@utam0k utam0k force-pushed the support-disabling-oom-group-kill branch from 1875ca5 to 916b167 Compare August 24, 2024 08:19
@@ -286,4 +288,15 @@ func SetDefaults_KubeletConfiguration(obj *kubeletconfigv1beta1.KubeletConfigura
if obj.PodLogsDir == "" {
obj.PodLogsDir = DefaultPodLogsDir
}

if obj.SingleProcessOOMKill == nil {

Choose a reason for hiding this comment

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

On non-Linux platforms, the default value for SingleProcessOOMKill is set to false and has no effect. Since the IsCgroup2UnifiedMode(), which checks if the system is running in cgroup v2 mode, always returns false on non-Linux platforms, I think it makes more sense to align the behavior with what it does in cgroup v1 mode. If it's fine for SingleProcessOOMKill to default to true, then we could simplify the defaulting logic like this:

		if !kubeletutil.IsCgroup2UnifiedMode() {
			// This is a default behavior for cgroups v1 and non-Linux platforms.
			obj.SingleProcessOOMKill = utilpointer.Bool(true)
		 else {
			obj.SingleProcessOOMKill = utilpointer.Bool(false)
		}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-review Categorizes an issue or PR as actively needing an API review. area/code-generation area/kubelet area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/feature Categorizes issue or PR as related to a new feature. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. release-note Denotes a PR that will be considered when it comes time to generate release notes. 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. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
Status: No status
Status: Archive-it
Status: Needs Reviewer
Development

Successfully merging this pull request may close these issues.

None yet

9 participants