Make rlimits configurable by adding three variables: RLimitFiles, RLimitProcesses and RLimitStack #5373
Conversation
lib/base/application.cpp
Outdated
| @@ -180,7 +180,7 @@ void Application::SetResourceLimits(void) | |||
| rlimit rl; | |||
|
|
|||
| # ifdef RLIMIT_NOFILE | |||
| rl.rlim_cur = 16 * 1024; | |||
| rl.rlim_cur = GetRLimitFiles(); | |||
There was a problem hiding this comment.
We should allow a user to set to values to NULL, and then not set limits at all.
For all 3 limits...
There was a problem hiding this comment.
There's still --no-stack-rlimit available to entirely disable the limits.
There was a problem hiding this comment.
No, this disables only setting of a stack limit, and it does not disable setting the limit. It sets the limit to MAX.
if (set_stack_rlimit)
There was a problem hiding this comment.
Ah ok. Is there a sensible limit for these three options if the user puts in garbage, or like 10 files?
There was a problem hiding this comment.
I would set the current values as minimal. Everything lower would fail, only setting to 0 or NULL should be valid.
Then it's the users problem.
There was a problem hiding this comment.
The patch sets the lower limits already for Linux, and allows the user to override this with their own values in the future. I wouldn't change this behaviour to make it "full user controllable", then the benefit of default values is entirely gone and users might run into issues again.
Keep in mind that init.conf is a configuration file, and we cannot override this for 2.7.
|
This issue is in addition to RedHat's kernel fixes an option to set limits in the configuration of Icinga, see #5367 |
9d42b3f to
01d29a4
Compare
|
The new patch has lower limits for the RLimit* configuration options. Also, users can bypass setrlimit() entirely by setting the RLimit* global variables to 0. I still need to update the documentation though, assuming this is the behavior we want. :) |
|
LGTM, what would you change/add in docs? I wouldn't document how to disable now, we could add it when it makes sense to use it later. |
|
The docs should be fairly low level, similar to UseVFork and Concurrency. If you know what you're doing, you can use the options. I'm currently testing the patch on a CentOS 7 box, will approve soon. |
|
My tests are running fine, I've also taken the chance to again test a RedHat test build for the kernel in a fresh box. Fixed kernel and default values Disabling the settings works fine too, no extra test case here. I've slightly updated the documentation, and will merge soon. |
…mitProcesses and RLimitStack refs #5367
01d29a4 to
c8b4fee
Compare
| } | ||
| if (setrlimit(RLIMIT_STACK, &rl) < 0) | ||
| Log(LogNotice, "Application", "Could not adjust resource limit for stack size (RLIMIT_STACK)"); | ||
| else if (set_stack_rlimit) { |
There was a problem hiding this comment.
Hello @dnsmichi @lazyfrosch
Is this condition wrong?
When setrlimit(RLIMIT_STACK, &rl) succeeded, why we call execvp with --no-stack-rlimit next?
I think the else if (set_stack_rlimit) should be deleted.
There was a problem hiding this comment.
Fails with trying to set limits and you don't have permissions to set limits
There was a problem hiding this comment.
@IkeEichenberger Which part are you trying to answer? the setrlimit may fail - it's ok, when it fails, then we should execvp with --no-stack-rlimit - the original logic it's opposite and wrong.
|
When setrlimit returns permission denied, the logging command sends output
that fills variable with the error message, as the code then continues,
messing up followon code. Perhaps just change logging, send warning to
file? I just committed our all the code with setrlimit to fix it for me
quick, but ...
…On Tue, May 8, 2018, 11:18 PM Harry Lee ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In lib/base/application.cpp
<#5373 (comment)>:
>
- if (!new_argv[1]) {
- perror("strdup");
- exit(1);
- }
+ if (setrlimit(RLIMIT_STACK, &rl) < 0)
+ Log(LogNotice, "Application", "Could not adjust resource limit for stack size (RLIMIT_STACK)");
+ else if (set_stack_rlimit) {
@IkeEichenberger <https://github.com/IkeEichenberger> Which part are you
trying to answer? the setrlimit may fail - it's ok, when it fails, then
we should execvp with --no-stack-rlimit - the original logic it's
opposite and wrong.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#5373 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AH4t4An4SCMV0L4uZm_HOV1e9dAUuV8sks5twl_vgaJpZM4OB_N9>
.
|
|
Can you please move the discussion to either a new issue or a new PR? This one has been merged and reverted later on, and won't be tracked any further. |
No description provided.