-
Notifications
You must be signed in to change notification settings - Fork 65
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
Add support for rlimits #48
Conversation
Signed-off-by: Samuel Karp <[email protected]>
Signed-off-by: Samuel Karp <[email protected]>
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #48 +/- ##
==========================================
+ Coverage 64.44% 64.50% +0.05%
==========================================
Files 9 9
Lines 1800 1834 +34
==========================================
+ Hits 1160 1183 +23
- Misses 494 500 +6
- Partials 146 151 +5
☔ View full report in Codecov by Sentry. |
Signed-off-by: Samuel Karp <[email protected]>
ulimit-adjuster tests are written with plain Go tests and do not run through ginkgo. Signed-off-by: Samuel Karp <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you ! Looks pretty good. I'll read through it again in the morning just in case, but my impression is that it is very much ready to go in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
Nice to see additional use cases and example plugins.
|
||
ulimits are annotated using the key | ||
`ulimits.nri.containerd.io/container.$CONTAINER_NAME`, which adjusts ulimits | ||
for `$CONTAINER_NAME`. The ulimit names are the valid names of Linux resource |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are there scenarios where customer may want to set a single per-pod override that will apply to all containers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possibly, but I don't have a specific use-case for that right now. I was looking to just build the simplest thing for now; we can always add functionality later if it's desired.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you! If there will be a scenario with some global setting for a Pod that will require all containers to adjust limits, it will make sense to have a single override. I am also not aware of scenarios that would need this.
hard limits should always be >= soft limits Signed-off-by: Samuel Karp <[email protected]>
lgtm |
lgtm +1 to add support for rlimits in NRI, it will be helpful for folks who need to configure rlimits in k8s environments where rlimits are not natively supported in pod specs. |
This PR adds support for adjusting rlimits and provides a sample plugin. I was able to test this using a patched version of containerd.