-
Notifications
You must be signed in to change notification settings - Fork 766
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
Make buildah's handling of ulimits match podman #5590
base: main
Are you sure you want to change the base?
Conversation
Ephemeral COPR build failed. @containers/packit-build please check. |
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.
Integration tests are failing for multiple envs.
The initial fix for this didn't account for the fact that privileged users can (usually) increase limits past the hard limit. This wasn't picked up when the tests were run locally, so they have been modified to ensure that this particular failure mode would be identified in future. |
@chris-reeves thanks for the PR! The tests are all very unhappy and it looks like you need to update your branch. |
d054820
to
6589a2e
Compare
@TomSweeneyRedHat It looks like this was just the linter complaining. I did wonder whether not checking Also rebased and signed the commit that I missed. |
@chris-reeves I kicked off the test again, a quick look seemed to show network hiccups. The branch looks like it needs a rebase too. |
Could you point me towards these network hiccups? The only two failing builds are the same two that appear to fail for most PRs (and this appears to be an issue with missing build tools on those platforms). I can rebase again if you like, but it looks like you're taking a branch/merge approach so I'm not sure what it gives us (other than another build to wait on!). |
There was a bug in containers#5275 which meant that in some cases (not rootless, with low limits) buildah tried to set limits to more than the hard limit. Fixing that issue made some of the tests fail as they only passed due to this bug. This was resolved by more accurately reflecting podman's handling of ulmits. Signed-off-by: Chris Reeves <chris.reeves@iname.com>
This is really a no-op to keep the validate_test check happy... Signed-off-by: Chris Reeves <chris.reeves@iname.com>
Try to set nofile limit to RLimitDefaultValue - this could potentially increase the limit past the current hard limit in non-rootless environments. This makes buildah behaviour match podman when a non-rootless environment has lower limits set. Signed-off-by: Chris Reeves <chris.reeves@iname.com>
Signed-off-by: Chris Reeves <chris.reeves@iname.com>
6589a2e
to
6646b57
Compare
LGTM |
Thanks @chris-reeves |
Anything else I can do to help get this PR merged? |
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
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: chris-reeves, giuseppe 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 |
var rlimit unix.Rlimit | ||
rlimit.Cur = define.RLimitDefaultValue | ||
rlimit.Max = define.RLimitDefaultValue | ||
err := unix.Setrlimit(unix.RLIMIT_NOFILE, &rlimit) |
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.
If the limits are set higher than this on entry, are we lowering them for the calling process? I don't know that I want that as a side-effect.
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.
If the limits are currently greater than define.RLimitDefaultValue
then yes, they will be lowered (but for the current buildah
process, not for any calling process).
There are three reasons why I don't think this is an issue:
define.RLimitDefaultValue
is 1048576, which matches the default kernel hard limit;- the purpose of Make buildah match podman for handling of ulimits #5275 (which this PR is a continuation of) was to match the behaviour of
podman
(and this is whatpodman
does); - if a user does need a higher limit for some reason, they can simply set the
--ulimit
option.
@chris-reeves Are you still working on this? |
Sorry, I missed the previous comment (which I've now responded to). I'm still keen to see this merged as our current workaround is rather hacky. |
A friendly reminder that this PR had no activity for 30 days. |
@nalind PTAL |
There was a bug in #5275 which assumed that non-rootless environments always had the ability to increase limits past the hard limit. This is not always the case. This was resolved by more accurately reflecting
podman
's handling of ulimits. The initial fix for this didn't account for the fact that privileged users can (usually) increase limits past the hard limit. This wasn't picked up when the tests were run locally, so they have been modified to ensure that this particular failure mode would be identified in future.What type of PR is this?
/kind bug
What this PR does / why we need it:
There was a bug in #5275 which assumed that non-rootless environments always had the ability to increase limits past the hard limit. This is not always the case. For example:
This was resolved by more accurately reflecting
podman
's handling of ulimits.How to verify it
The original bug can be replicated by running the test suite (specifically
bats tests/run.bats -f limit
) in a non-rootless mode wherenproc
limits are less thanRLimitDefaultValue
(1048576). For me the easiest way to do this was to run the test asroot
withRLimitDefaultValue
increased to 10485760.Which issue(s) this PR fixes:
None - no issue has been opened for this bug.
Special notes for your reviewer:
No tests have been added for the scenario that was originally encountered (non-rootless environment where the user doesn't have privileges to increase limits past the hard limit) due to the difficulty in manipulating limits and recreating this scenario in a test environment, however the tests which fail when
RLimitDefaultValue
is increased to 10485760 now pass with this fix in place.The initial fix for this didn't account for the fact that privileged users can (usually) increase limits past the hard limit. This wasn't picked up when the tests were run locally, so they have been modified to ensure that this particular failure mode would be identified in future.
The PR does make a call to
Setrlimit()
without checking the returned error value. This is intentional as the waypodman
handles the failure of this call is to try again with the current hard limit, but we effectively do this a little later with the call toAddProcessRlimits()
making another get/set round trip redundant.Does this PR introduce a user-facing change?