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

Respect hard limit when setting open file limit (NOFILE) #2386

Merged
merged 2 commits into from
Aug 29, 2023

Conversation

benofbrown
Copy link
Contributor

In some cases (such as the default settings in Debian Linux) the hard limit for open files is much higher than the minimum open files limit of 10000. By respecting this hard limit instead of requesting an infinite number of open files this allows us to set the open files to 10000, when attepting to set it to unlimited will fail.

In some cases (such as the default settings in Debian Linux) the hard
limit for open files is much higher than the minimum open files limit
of 10000. By respecting this hard limit instead of requesting an
infinite number of open files this allows us to set the open files to
10000, when attepting to set it to unlimited will fail.
@cyberw
Copy link
Collaborator

cyberw commented Aug 29, 2023

Ignore the readthedocs failure...

I cant wrap my head around this if statement:

if current_open_file_limit < minimum_open_file_limit and minimum_open_file_limit < current_open_file_limit_hard:

Can you rewrite it in a way that makes more sense? Maybe rename the variables to just soft_limit and hard_limit..

The new code doesnt try to change the hard limit at all, it looks like? That is not good. Unless I'm mistaken and the hard limit can never be changed anyway :)

@cyberw
Copy link
Collaborator

cyberw commented Aug 29, 2023

Also, this code would give a ValueError if current_open_file_limit_hard is less than 10000, which is also bad.

resource.setrlimit(resource.RLIMIT_NOFILE, [minimum_open_file_limit, current_open_file_limit_hard])

@benofbrown
Copy link
Contributor Author

Thanks for the prompt feedback!

Can you rewrite it in a way that makes more sense? Maybe rename the variables to just soft_limit and hard_limit..

Will do.

Unless I'm mistaken and the hard limit can never be changed anyway :)

The hard limit cannot be changed by an unprivileged user, and in the case of NOFILE on Linux even a privileged user can't set it to unlimited, it needs to be less than the maximum open files in /proc/sys/fs/file-max.

Also, this code would give a ValueError if current_open_file_limit_hard is less than 10000, which is also bad.

I'll look at improving that.

@benofbrown
Copy link
Contributor Author

Also, this code would give a ValueError if current_open_file_limit_hard is less than 10000, which is also bad.

That code wouldn't be run in this instance, though I'll rewrite the if to make that more clear.

Thinking more about this, what do we want to happen in this case? Raising a ValueError would be handled by the BaseException block and give the warning as before. Should we give a different warning in this case that makes it clear that the limit cannot be raised? Or possibly even an error?

@cyberw
Copy link
Collaborator

cyberw commented Aug 29, 2023

I think the current warning is ok. The "proper" solution is setting this outside of locust, this code is just an "ugly" workaround :)

@cyberw
Copy link
Collaborator

cyberw commented Aug 29, 2023

The hard limit cannot be changed by an unprivileged user, and in the case of NOFILE on Linux even a privileged user can't set it to unlimited, it needs to be less than the maximum open files in /proc/sys/fs/file-max.

I see, then we shouldnt try to change hard limit at all I guess (because people won't be running locust as root)... Probably just random that it worked for me (I'm mostly on MacOS, where hard limit is unlimited for user accounts by default)

In the instance of the requested new soft limit being higher than the
hard limit, the call will raise an exception and the warning will be
displayed, so I have removed the explicit check for it.
@benofbrown
Copy link
Contributor Author

I've pushed a new commit that simplifies it somewhat, hopefully this will do!

@cyberw cyberw changed the title Respect hard limit when setting NOFILE Respect hard limit when setting open file limit (NOFILE) Aug 29, 2023
@cyberw cyberw merged commit bf3523a into locustio:master Aug 29, 2023
10 checks passed
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.

None yet

2 participants