kubespray icon indicating copy to clipboard operation
kubespray copied to clipboard

Allow configuring base_runtime_spec per containerd runtime and supply a default runtime spec

Open fungusakafungus opened this issue 3 years ago • 3 comments

What type of PR is this? /kind feature

What this PR does / why we need it:

Sometimes, you feel like you'd like your cluster to be stable and withstand file descriptor explosions in pods.

This PR allows setting base_runtime_spec on containerd runtimes, which is the current way of enforcing limits on number of open files per container, see e.g. https://github.com/containerd/containerd/issues/6063

Related kubernetes issue(2015!): https://github.com/kubernetes/kubernetes/issues/3595

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

The configurability this PR provides is rather low-level. Enabling the default cri-base.json will restrict number of open files per container to just 1024, which is rather low. Configuring a different value for nofile rlimit would involve either provisioning a similar file manually(outside of kubespray), or setting containerd_base_runtime_specs: { cri-spec-custom.json: ...} to a huge json blob (about 5k).

I could alternatively provide a way to set just the open files rlimit by merging the default json blob with a setting. Should I do that?

This PR also improves docs for containerd runtimes, I needed something to base my explanation on :).

Does this PR introduce a user-facing change?:

Allow configuring base_runtime_spec per containerd runtime and supply a default runtime spec; useful for configuring file descriptor limits per container.

fungusakafungus avatar Sep 20 '22 18:09 fungusakafungus

Hi @fungusakafungus. Thanks for your PR.

I'm waiting for a kubernetes-sigs 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.

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 avatar Sep 20 '22 18:09 k8s-ci-robot

/ok-to-test

mzaian avatar Sep 22 '22 07:09 mzaian

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: floryut, fungusakafungus

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 avatar Sep 22 '22 20:09 k8s-ci-robot

Great PR description also, neat work @fungusakafungus !

floryut avatar Sep 22 '22 20:09 floryut

@floryut do you have an opinion on

The configurability this PR provides is rather low-level. ... I could alternatively provide a way to set just the open files rlimit by merging the default json blob with a setting.

Here's the alternative version: https://github.com/kubernetes-sigs/kubespray/pull/9319

fungusakafungus avatar Sep 23 '22 11:09 fungusakafungus

Thanks @fungusakafungus ! I love seeing folks pay attention to the docs as well.

/lgtm

cristicalin avatar Sep 23 '22 17:09 cristicalin