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

cri: set default RLIMIT_NOFILE #1180

Merged
merged 1 commit into from
Nov 6, 2020

Conversation

zmrow
Copy link
Contributor

@zmrow zmrow commented Oct 23, 2020

Issue number:
Related to #1136

Description of changes:

The `cri` plugin currently inherits the limit from the default OCI spec or the
containerd process.  This change sets the default RLIMIT_NOFILE to 1048576 in
the OCI spec for any container spawned using `cri`.

The intent is to upstream this patch, but there is additional work to be done to make it compatible with containerd >1.3. We also don't have some of the upstream unit tests in our containerd package; we'll need to write one for this change when we submit upstream.

Given that the AL2 AMI uses this limit, albeit on the containerd process, it seems a reasonable number to use for container processes spawned by cri.

Testing done:
Ran a busybox pod on a Bottlerocket AMI built using this patch:

~/bottlerocket/bottlerocket $ kubectl run -i -t busybox --image=busybox --restart=Never                                                  
If you don't see a command prompt, try pressing enter.
/ # ulimit -a
...
open files                      (-n) 65536
...

Ran a busybox pod on the latest released Bottlerocket AMI:

~/bottlerocket/bottlerocket $ kubectl run -i -t busybox --image=busybox --restart=Never
If you don't see a command prompt, try pressing enter.
/ # ulimit -a
...
open files                      (-n) 1073741816
...

Ran the MySQL manifest from #1136 with an AMI built using this patch and confirmed the pod runs just fine without any OOM issues that I could see. (I also ran the pod with the current released Bottlerocket AMI and confirmed that it does indeed OOM)

Terms of contribution:

By submitting this pull request, I agree that this contribution is dual-licensed under the terms of both the Apache License, version 2.0, and the MIT license.

Copy link
Contributor

@samuelkarp samuelkarp left a comment

Choose a reason for hiding this comment

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

Code looks good to me, just one question about what the value should be.

Copy link
Contributor

@webern webern left a comment

Choose a reason for hiding this comment

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

Wow, nice job.

@zmrow
Copy link
Contributor Author

zmrow commented Oct 27, 2020

^ added a soft limit of 65536 and confirmed this is what the node sees.

open files                      (-n) 65536

@zmrow zmrow changed the title cri: set a default RLIMIT_NOFILE of 1048576 cri: set default RLIMIT_NOFILE Oct 28, 2020
@zmrow
Copy link
Contributor Author

zmrow commented Oct 28, 2020

I ran the MySQL manifest from #1136 with an AMI built using this patch and confirmed the pod runs just fine without any OOM issues that I could see. (I also ran the pod with the current released Bottlerocket AMI and confirmed that it does indeed OOM)

apiVersion: v1
kind: Pod
metadata:
  name: mysql-test
  namespace: default
spec:
  containers:
    - name: mysql
      imagePullPolicy: Always
      image: 'mysql:5.7'
      env:
        - name: MYSQL_DATABASE
          value: test
        - name: MYSQL_ROOT_PASSWORD
          value: test

The `cri` plugin currently inherits the limit from the default OCI spec
or the containerd process.  This change sets the default hard
RLIMIT_NOFILE to 1048576 and the soft limit to 65536 in the OCI spec for
any container spawned using `cri`.
@zmrow
Copy link
Contributor Author

zmrow commented Oct 28, 2020

^ the above force push fixes up the spacing issues @bcressey caught

(Thanks :set list !)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants