Support passing of Ulimits as -1 to mean max #19879
Conversation
openshift-ci
bot
added
release-note
approved
rhatdan
force-pushed
the
ulimits
branch
2 times, most recently
from
5a1bad0 to
1035f36
Compare
|
@Luap99 Does this risk the kernel-change break that we saw earlier? |
|
@rhatdan it looks like you have changes to label functionality in here too, was that intentional? If so, a note in the description would be appreciated. Lots of red tests here too, do we have something going on? |
|
I updated the Buildah Vendor. I will split them apart, since I have to take another pass. |
|
@rhatdan looks like you have a dirty tree with that vendor.... |
There was a problem hiding this comment.
Yes we cannot ever freeze limits container creation. This will reintroduce the same bug. You need to set the limit at container start time, see #18721
But than again why would we set anything if unlimited is requested as rootless if we treat as as much as we can? In this case we do not have to do anything as the limits will just naturally be inherited by the child processes.
If unlimited should really mean max then it should only do so as rootless. As root we can set unlimited, otherwise we may break backwards and docker compatibility.
I find it rather awkward to just silently assume unlimted == max you can get. If someone explicitly request unlimited they may want it for a reason. Changing the definition of what unlimited means for resource limits seems like a bad idea and IMO breaks POLA.
test/e2e/inspect_test.go
Outdated
| Expect(ulimit.Soft).To(BeNumerically("==", limit.Cur)) | ||
| Expect(ulimit.Hard).To(BeNumerically("==", limit.Max)) |
There was a problem hiding this comment.
This is a major regression, unlimited must mean unlimited. Everything else just breaks user assumption.
There was a problem hiding this comment.
Unlimited does not mean unlimited in rootful, since everything has a limit. The reason for this option for users is to allow them to up the current without effecting the max. Eliminating this ability means that, this functionality is not available to the users.
There was a problem hiding this comment.
let's discuss at tomorrows standup.
openshift-merge-robot
added
the
needs-rebase
|
@rhatdan getting closer. Now you have branch conflicts. |
openshift-merge-robot
removed
the
needs-rebase
|
Cockpit tests failed for commit 17aecd248e958380fe99cbb38b7cec8cb697bcfc. @martinpitt, @jelly, @mvollmer please check. |
rhatdan
force-pushed
the
ulimits
branch
4 times, most recently
from
9edf85e to
c9308eb
Compare
|
@rhatdan any further luck with this? Lot's of unhappy tests still. |
|
Hope to get back to it soon. |
Please take a look at this comment, we cannot ever freeze limits like this at create time without causing troubles down the road. |
|
I disagree, in the previous error we were doing this without user input. Now the user must specify the -1 field which is documented to set the "current" max to the container. We make no guarantees that this number might change in the future. Besides, the Kernel change could and should have been considered a breaking change, since it broke stuff, by changing defaults. Currently rootless podman is broken when giving it a -1 field, and this can happen in docker-compose. We need to allow this and make sure the users understand what they did. Perhaps in podman 5.* with boltdb podman update container could even change this field. |
|
I am not saying you cannot translate -1 to max in the rootless case, but when you do that it must happen at the container start time not the create time like you are doing right now, i.e. it should be in Limits are never static they can change due configuration changes or hardware changes, if I lower my system memory then with this patch the container would no longer start and become useless. |
Docker allows the passing of -1 to indicate the maximum limit allowed for the current process. Fixes: containers#19319 Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>
|
The limits are being stored as -1 now, and should be applied at start time, from my examining of the underlying data structure and from reading podman inspect. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Luap99, 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 |
|
LGTM |
github-actions
bot
added
the
locked - please file new issue/PR
Docker allows the passing of -1 to indicate the maximum limit allowed for the current process.
Fixes: #19319
Does this PR introduce a user-facing change?