-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
fix max open files limit for qemu #20643
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: protosam, rhatdan 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 |
Ephemeral COPR build failed. @containers/packit-build please check. |
pkg/machine/qemu/machine_unix.go
Outdated
|
||
// 3) change rLimit to maximum value | ||
err = syscall.Setrlimit(syscall.RLIMIT_NOFILE, &syscall.Rlimit{ | ||
Max: syscall.RLIM_INFINITY, |
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.
Setting the limit this high can potentially fail. On Linux anything greater than /proc/sys/fs/nr_open can throw EPERM, and on Mac OS, some versions can through EINVAL if you exceed OPEN_MAX(10240). There could also be cases where the hard limit is set lower than the system max, in which you could raise the soft to match the hard limit. Most likely podman machine is not running as root, so that could be the solution to just set soft to match the current hard limit
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.
Can't find solid documentation on it, so I'm guessing that syscall.Rlimit.Max
is the hard limit and syscall.Rlimit.Cur
is the soft limit.
Does the last change I made resolve this?
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.
Yes, looks good and thats right Cur = soft. FYI you can find the docs in the OSX/Linux man pages man setrlimit
Thanks for the PR! Just one note above on the usage of RLIIMIT_INFINITY |
Renamed the variable |
Compilation is failing. |
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.
There is already code in cmd/podman/early_init_linux.go to handle this for linux for all podman processes.
I suggest you simply extend that file to also run on macos instead of setting this only for a single command.
This looks like dead code to me. I don't see that % grep -ril setRLimits
./cmd/podman/early_init_linux.go
./vendor/github.com/containers/buildah/CHANGELOG.md
./vendor/github.com/containers/buildah/changelog.txt
./vendor/github.com/containers/buildah/chroot/run_common.go |
it is called in |
Works for me. I've made changes to that affect. Let's see if this passes CI. 🤞 |
Not sure what to do about the "Build Each Commit" job. Should I just squash this commit history? Idk if it understand how to handle that based on it's output. |
Yes please squash your commits. The rule is that if it's needed (make review easier) you can have multiple commits, but each individual commit has to be buildable and pass lint and whitespace checks on its own. Also, its good practice to keep the history of the project clean as possible: the commits should all build toward the same destination vs a local iterative work-stream where the changes replace and undo each other. What I recommend doing if you want to preserve your local history, is to just create a new branch as a local backup, and rebase the existing one to a clean patch set, and then force push. In this case it's a small enough change that one commit should be sufficient. |
[NO NEW TESTS NEEDED] Signed-off-by: Sam Peterson <[email protected]>
LGTM |
/lgtm thanks again! |
Does this PR introduce a user-facing change?
No.
Fixes issue: #16106