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

[containerd] Limit number of open files per container #9319

Merged

Conversation

fungusakafungus
Copy link
Contributor

@fungusakafungus fungusakafungus commented Sep 23, 2022

by setting a default runtime spec with a patch for RLIMIT_NOFILE.

Introduces containerd_base_runtime_spec_rlimit_nofile, with default of 16384

Slightly more aggressive version of #9302

  • uses base_runtime_spec by default
  • sets max number of open files per container by default to containerd_base_runtime_spec_rlimit_nofile: 16384

This is the version I would use in production.

What type of PR is this?
/kind feature

What this PR does / why we need it:
See #9302

Does this PR introduce a user-facing change?:

[containerd] Newly started containers will be limited to 16384 open files. To change this number, set `containerd_base_runtime_spec_rlimit_nofile`, or remove `base_runtime_spec` from runc runtime to revert to previous behaviour.

@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. kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Sep 23, 2022
@k8s-ci-robot
Copy link
Contributor

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 k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Sep 23, 2022
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 23, 2022
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 24, 2022
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 17, 2022
@fungusakafungus fungusakafungus changed the title [containerd] Allow limiting number of open files per container [containerd] Limit number of open files per container Oct 17, 2022
@fungusakafungus fungusakafungus marked this pull request as ready for review October 17, 2022 14:40
@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 Oct 17, 2022
@fungusakafungus
Copy link
Contributor Author

Very useful test failures. This would break every cluster! (as it just did with ours)

@fungusakafungus fungusakafungus marked this pull request as draft October 17, 2022 17:03
@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 Oct 17, 2022
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 18, 2022
by setting a default runtime spec with a patch for RLIMIT_NOFILE.

- Introduces containerd_base_runtime_spec_rlimit_nofile.
- Generates base_runtime_spec on-the-fly, to use the containerd version
  of the node.
@fungusakafungus
Copy link
Contributor Author

Fixed and ready for review. The result can be seen in any new pod with a shell like this:

$ kubectl exec some-pod-with-sh -- sh -c 'ulimit -n'
16384

CI was broken because the cri-base.json file was generated some time ago by a previous containerd version. Now this file is generated on-the-fly.

@fungusakafungus fungusakafungus marked this pull request as ready for review October 19, 2022 17:35
@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 Oct 19, 2022
@fungusakafungus
Copy link
Contributor Author

fungusakafungus commented Oct 19, 2022

To the default limit of open files of 16,384:
containerd sets a limit of 1,048,576 for all running containers in its systemd unit: containerd/containerd#3202
max pods per node is usually 110, if all would would be running and using max open files, we'd have 16,384 x 110 = 1,802,240 open files, close enough.

@mzaian
Copy link
Contributor

mzaian commented Oct 24, 2022

/ok-to-test
/assign @oomichi

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 24, 2022
@oomichi
Copy link
Contributor

oomichi commented Nov 7, 2022

To the default limit of open files of 16,384: containerd sets a limit of 1,048,576 for all running containers in its systemd unit: containerd/containerd#3202 max pods per node is usually 110, if all would would be running and using max open files, we'd have 16,384 x 110 = 1,802,240 open files, close enough.

Thanks for explaining the reason of the default value.
That seems reasonable to change it here.
In addition, it is nice to generate the default config file with ctr command in a task instead of the static file.

/approve

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 7, 2022
Copy link
Member

@floryut floryut left a comment

Choose a reason for hiding this comment

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

/lgtm
@fungusakafungus Thank you, seems like a nice way to do it!

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 8, 2022
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

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

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

@floryut floryut added kind/container-managers Containers section in the release note and removed kind/feature Categorizes issue or PR as related to a new feature. labels Nov 8, 2022
@k8s-ci-robot k8s-ci-robot merged commit 5a8cf82 into kubernetes-sigs:master Nov 8, 2022
@fungusakafungus fungusakafungus deleted the configure_rlimit_nofile branch November 9, 2022 09:01
salifou pushed a commit to salifou/kubespray that referenced this pull request Nov 13, 2022
…bernetes-sigs#9319)

by setting a default runtime spec with a patch for RLIMIT_NOFILE.

- Introduces containerd_base_runtime_spec_rlimit_nofile.
- Generates base_runtime_spec on-the-fly, to use the containerd version
  of the node.
@floryut floryut mentioned this pull request Jan 4, 2023
enneitex pushed a commit to enneitex/kubespray that referenced this pull request Jan 25, 2023
…bernetes-sigs#9319)

by setting a default runtime spec with a patch for RLIMIT_NOFILE.

- Introduces containerd_base_runtime_spec_rlimit_nofile.
- Generates base_runtime_spec on-the-fly, to use the containerd version
  of the node.
HoKim98 pushed a commit to ulagbulag/kubespray that referenced this pull request Mar 8, 2023
…bernetes-sigs#9319)

by setting a default runtime spec with a patch for RLIMIT_NOFILE.

- Introduces containerd_base_runtime_spec_rlimit_nofile.
- Generates base_runtime_spec on-the-fly, to use the containerd version
  of the node.
nolimitkun pushed a commit to nolimitkun/kubespray that referenced this pull request Mar 19, 2023
…bernetes-sigs#9319)

by setting a default runtime spec with a patch for RLIMIT_NOFILE.

- Introduces containerd_base_runtime_spec_rlimit_nofile.
- Generates base_runtime_spec on-the-fly, to use the containerd version
  of the node.
notCalle pushed a commit to notCalle/kubespray that referenced this pull request Dec 18, 2023
…bernetes-sigs#9319)

by setting a default runtime spec with a patch for RLIMIT_NOFILE.

- Introduces containerd_base_runtime_spec_rlimit_nofile.
- Generates base_runtime_spec on-the-fly, to use the containerd version
  of the node.

(cherry picked from commit 5a8cf82)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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/container-managers Containers section in the release note lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants