sys: refactor OOM utilities#5253
Conversation
|
Skipping CI for Draft Pull Request. |
There was a problem hiding this comment.
Hope this is correct then to require !user-namspaces (otherwise I'll drop this commit); I based this on the fact we're suppressing failures in this situation in SetOOMScore, but perhaps that was the wrong assumption 😅
Line 47 in 717dde3
|
@AkihiroSuda updated; PTAL |
This comment has been minimized.
This comment has been minimized.
|
Build succeeded.
|
^^ fixed the test @AkihiroSuda @kzys PTAL |
estesp
left a comment
estesp
left a comment
There was a problem hiding this comment.
A couple pretty minor points about the constants and use of them; otherwise looks good
|
@estesp Updated with your suggestions. I also pushed one more commit with a change I wanted to make, but forgot to include; last commit removes the |
|
Build succeeded.
|
These are currently only used inside this package, so we might as well un-export them until we need them elsewhere. Also updated SetOOMScore() to first check for privileged; check for privileged looks to be the "faster" path, and checking it first could (in case of non- privileged) save having to read and parse /proc/self/uid_map. Signed-off-by: Sebastiaan van Stijn <[email protected]>
SetOOMScore requires both privileged (root) and non-user namespace, for negative values, so adjust the pre-conditions accordingly. Signed-off-by: Sebastiaan van Stijn <[email protected]>
Slightly easier to read, and we were already using assert in this file Signed-off-by: Sebastiaan van Stijn <[email protected]>
Produce a more user-friendly error in the odd case we would try to set a score out of range Signed-off-by: Sebastiaan van Stijn <[email protected]>
Handle the limits in this function so that consumers don't have to perform the boundary checks. Signed-off-by: Sebastiaan van Stijn <[email protected]>
…illable The shim.SetScore() utility was no longer used since 7dfc605. Checking for uses outside of this repository, I found only one external use of this in gVisor; https://github.com/google/gvisor/blob/a9441aea2780da8c93da1c73da860219f98438de/pkg/shim/service.go#L262-L264 Signed-off-by: Sebastiaan van Stijn <[email protected]>
|
rebased to trigger CI (which should now be fixed) |
|
Build succeeded.
|
6 participants
Noticed these when I was working on #5252. Not attached to all of them, so feel free to shoot 😂
sys: un-export runningPrivileged() / runningUnprivileged()
These are currently only used inside this package, so we might
as well un-export them until we need them elsewhere.
Also updated SetOOMScore() to first check for privileged; check for privileged
looks to be the "faster" path, and checking it first could (in case of non-
privileged) save having to read and parse /proc/self/uid_map.
sys: add missing pre-condition checks in tests
SetOOMScore requires both privileged (root) and non-user namespace
for negative values, so adjust the pre-conditions accordingly.
sys: use assert for error checks in OOM tests
Just slightly easier to read, and we were already using assert in this file
sys: add boundary checks to SetOOMScore()
Produce a more user-friendly error in the odd case we would try to set a
score out of bounds
sys: add AdjustOOMScore() utility
Handle the limits in this function so that consumers don't have to perform the
boundary checks.
runtime/v2/shim: remove unused SetScore() and remove sys.OOMScoreMaxKillable
The shim.SetScore() utility was no longer used since 7dfc605 (#3380).
Checking for uses outside of this repository, I found only one external use of this in gVisor; https://github.com/google/gvisor/blob/a9441aea2780da8c93da1c73da860219f98438de/pkg/shim/service.go#L262-L264
Which was added in google/gvisor-containerd-shim@35db607 (google/gvisor-containerd-shim#13) by @Random-Liu, but looks like it can be easily replaced by either calling
sys.SetOOMScore()directly, or replaced withshim.AdjustOOMScore()as is done in this repository now.