Make buildah match podman for handling of ulimits #5275
Conversation
| @@ -54,6 +54,9 @@ const ( | |||
| SNP TeeType = "snp" | |||
| ) | |||
|
|
|||
| // DefaultRlimitValue is the value set by default for nofile and nproc | |||
| const RLimitDefaultValue = uint64(1048576) | |||
There was a problem hiding this comment.
Where do these values come from? Are they always the default everywhere?
There was a problem hiding this comment.
This is the default from Podman, no idea where it came from.
There was a problem hiding this comment.
Might have been a value from Docker?
https://bbs.archlinux.org/viewtopic.php?id=285058
There was a problem hiding this comment.
As of 7 years ago was a kernel max.
https://serverfault.com/questions/819119/xtrabackup-too-many-open-files-more-than-1048576
Might want to use something even higher.
cat /proc/sys/kernel/pid_max
4194304
There was a problem hiding this comment.
Under Moby I see.
./libnetwork/docs/vagrant-systemd/docker.service:LimitNOFILE=1048576
./libnetwork/docs/vagrant-systemd/docker.service:LimitNPROC=1048576
There was a problem hiding this comment.
Digging into the git history, these defaults were introduced in Podman with:
to address:
There was a problem hiding this comment.
The question is do we continue to roll with it, or should we increase it?
There was a problem hiding this comment.
I'd vote to keep Podman and Buildah in lockstep. I don't know if there's a place to tuck the default values so both could reference them from the same place, that would be nice. I think 1048576 for the kernel max is good for now.
There was a problem hiding this comment.
Can we move this constant to c/common? No need to block this PR on it but we should use every occasion to avoid future code divergence; this PR smells like a good example :)
rhatdan
force-pushed
the
ulimit
branch
2 times, most recently
from
bd9c5f0
to
7569418
Compare
|
I must be missing something. In the test, the build is printing the soft limits it's seeing (here), but the test is expecting those values to match the hard limits (gathered here and here) it has. |
|
@nalind we are only printing the hard limit and testing off of it. |
I think you're mistaken about which field the RUN commands in |
Podman currently sets the ulimits of nofile and nproc to max in rootless mode, if the user does not override. Buildah on the other hand just passes in the current defaults. Podman build should match podman run, and this will fix that problem. Fixes: containers#5273 Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>
|
@nalind this is ready to go in, I believe. |
| @@ -54,6 +54,9 @@ const ( | |||
| SNP TeeType = "snp" | |||
| ) | |||
|
|
|||
| // DefaultRlimitValue is the value set by default for nofile and nproc | |||
| const RLimitDefaultValue = uint64(1048576) | |||
There was a problem hiding this comment.
Can we move this constant to c/common? No need to block this PR on it but we should use every occasion to avoid future code divergence; this PR smells like a good example :)
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: giuseppe, rhatdan, vrothberg 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 |
Podman currently sets the ulimits of nofile and nproc to max in rootless mode, if the user does not override.
Buildah on the other hand just passes in the current defaults.
Podman build should match podman run, and this will fix that problem.
Fixes: #5273
What type of PR is this?
What this PR does / why we need it:
How to verify it
Which issue(s) this PR fixes:
Special notes for your reviewer:
Does this PR introduce a user-facing change?