KEP-5758: Per-container ulimits configuration

[HirazawaUi]

• One-line PR description: This KEP proposes adding support for per-container ulimit configurations in pod.

• Issue link: Per-container ulimits configuration #5758

• Other comments:

[HirazawaUi]

/cc @thockin Though it's been over ten years since this feature was first proposed, you might still be interested in it :)

[zvonkok]

/cc @zvonkok holy smokes over ten years... @dims FYI

[kannon92]

Please request a PRR reviewer if you are hoping to get this in for 1.36 cycle.

You can add me as an approver.

[HirazawaUi]

@kannon92 Thank you so much for being willing to serve as the PRR reviewer for this KEP.

[kannon92]

Left a few comments from PRR side but I'd like to see a review from container runtime folks (cc @haircommander @saschagrunert @sohankunkerkar @mikebrow).

[HirazawaUi]

ping @kannon92 @haircommander @samuelkarp

I have responded to all comments. Given that the KEP freeze deadline is very close, we may need to accelerate our efforts.

Also ping @mrunalp @SergeyKanzhelev

[mikebrow]

couple comments

[HirazawaUi]

Let me explain the changes I made:

Only Pods with PSS in Privileged mode can use the ulimit feature

Regardless of whether the pod is in securityContext's privileged mode, it can set ulimit to -1, because even if setting to -1 is not allowed, users could set a sufficiently large value instead, making this restriction meaningless

Changed RuntimeHandlerFeatures to RuntimeFeatures because ulimit is an existing field in the oci spec, and the underlying container runtime (runc/crun) already implements it

[haircommander]

Okay given we're restricting to privileged PSA, I think this is safe to try. It's been asked for a number of times over the years, and I think we have space to iterate on how to make it safe for baseline too (maybe baseline can only set soft or something, to be discussed in the future)

/lgtm

@tallclair WDYT about the PSA changes? @mikebrow @samuelkarp any additional thoughts?

[HirazawaUi]

ping @kannon92 @samuelkarp @mikebrow @thockin

Apologies for the repeated pings. Since we are just one day away from the KEP freeze, I am very much looking forward to your further suggestions. If possible, I will address or resolve all comments again before the KEP freeze

[HirazawaUi]

Okay given we're restricting to privileged PSA, I think this is safe to try. It's been asked for a number of times over the years, and I think we have space to iterate on how to make it safe for baseline too (maybe baseline can only set soft or something, to be discussed in the future)

/lgtm

@tallclair WDYT about the PSA changes? @mikebrow @samuelkarp any additional thoughts?

Thank you very much. My github page didn’t refresh this comment in time, so I posted the one above.

[mikebrow]

/lgtm
no additional comments for this stage

[mrunalp]

I would suggest dropping support for this in favor of pids cgroup support that we have.

[samuelkarp]

+1, let's drop nproc for now.

[HirazawaUi]

Removed.

[HirazawaUi]

@mrunalp Thank you for your comments. I have addressed all of them. Please take another look.

[HirazawaUi]

@mrunalp This PR needs another lgtm since it has been updated.

[SergeyKanzhelev]

/lgtm

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: HirazawaUi, kannon92, mrunalp, SergeyKanzhelev

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• keps/prod-readiness/OWNERS [kannon92]
• keps/sig-node/OWNERS [mrunalp]

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