-
Notifications
You must be signed in to change notification settings - Fork 399
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
RFC: feat(livenet): add memory size check depending on live image size #2344
RFC: feat(livenet): add memory size check depending on live image size #2344
Conversation
@tblume Thank you for working on this. Before considering this PR, I wanted to get your opinion on #1780 . #1780 is certainly different but between this PR and #1780 it is common that both trying to handle OOM and both using dmsquash-live dracut module. Do you see a way to resolve both issues at once, or you think that issues are different and needs to be resolved separately with separate PRs ? |
Yes, my patch would cover this issue. Would it be acceptable to set 1G as default lower limit and add an boot parameter that could modify this? |
I am not sure. #1780 is not using livenet, just local ISO. CC @Conan-Kudo |
Yes, I think that's reasonable. Distributions will want to set their own minimum limits, and giving a way to do that makes sense. |
Indeed, I overlooked this, sorry. |
d8c954c
to
7dc0638
Compare
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.
Thanks Thomas! Some things I miss:
- Could you add some doc about the new parameter
rd.minmem.liverw
indracut.cmdline.7
, close tord.writable.fsimg
? - All the calls to
curl
are encapsulated in functions withinurl-lib.sh
, maybe the best approach is create a new function there with the code that retrieves the value ofimgsize
. What do you think? - Some kind of error handling for getting
imgsize
. What happens if the status code of thecurl
request is not 200? If theliveurl
address cannot be reached, we get the errorbash: / (1024 * 1024): syntax error: operand expected (error token is "/ (1024 * 1024)")
. Another example that can be easily fixed, if it returns 302 (redirect), the headers does not containContent-Length
. E.g. with our OBS:
sh-5.2# curl -I "https://download.opensuse.org/repositories/systemsmanagement:/Agama:/Staging/images/iso/agama-live.aarch64-ALP.iso"
HTTP/2 302
date: Wed, 21 Jun 2023 13:34:58 GMT
server: Apache
location: https://ftp.gwdg.de/pub/opensuse/repositories/systemsmanagement%3A/Agama%3A/Staging/images/iso/agama-live.aarch64-ALP.iso
vary: Accept,COUNTRY
content-type: application/x-iso9660-image
sh-5.2# curl -I "https://ftp.gwdg.de/pub/opensuse/repositories/systemsmanagement%3A/Agama%3A/Staging/images/iso/agama-live.aarch64-ALP.iso"
HTTP/1.1 200 OK
Server: nginx
Date: Wed, 21 Jun 2023 13:36:04 GMT
Content-Type: application/octet-stream
Content-Length: 991166464
Last-Modified: Fri, 16 Jun 2023 14:09:36 GMT
Connection: keep-alive
ETag: "648c6d20-3b140000"
Strict-Transport-Security: max-age=63072000; includeSubDomains; preload
X-Frame-Options: SAMEORIGIN
X-Content-Type-Options: nosniff
Accept-Ranges: bytes
In this case, the -L
option does the trick:
sh-5.2# curl -IL "https://download.opensuse.org/repositories/systemsmanagement:/Agama:/Staging/images/iso/agama-live.aarch64-ALP.iso"
HTTP/2 302
server: Apache
date: Wed, 21 Jun 2023 13:46:55 GMT
vary: Accept,COUNTRY
location: https://ftp.gwdg.de/pub/opensuse/repositories/systemsmanagement%3A/Agama%3A/Staging/images/iso/agama-live.aarch64-ALP.iso
content-type: application/x-iso9660-image
HTTP/1.1 200 OK
Server: nginx
Date: Wed, 21 Jun 2023 13:46:56 GMT
Content-Type: application/octet-stream
Content-Length: 991166464
Last-Modified: Fri, 16 Jun 2023 14:09:36 GMT
Connection: keep-alive
ETag: "648c6d20-3b140000"
Strict-Transport-Security: max-age=63072000; includeSubDomains; preload
X-Frame-Options: SAMEORIGIN
X-Content-Type-Options: nosniff
Accept-Ranges: bytes
I hope to discuss the name of the new parameter a bit. As discussed there are other scenarios where something similar like a minmem can be used (e.g. together with To keep the possibility of reusing this argument, what do you think of simply using the name of
I am somewhat on a different opinion on this. I think of a function is only used by one dracut module, then it make sense to only include that logic in that one module - regardless of the functionality.
Yes, please ! |
7dc0638
to
988fd0c
Compare
Thanks for the feedback. |
bbbedab
to
3380ab8
Compare
modules.d/90livenet/livenetroot.sh
Outdated
@@ -16,6 +16,13 @@ netroot="$2" | |||
liveurl="${netroot#livenet:}" | |||
info "fetching $liveurl" | |||
|
|||
if getargbool 0 'rd.writable.fsimg'; then | |||
|
|||
imgsize=$(($(curl -IL "$liveurl" | sed -n 's/Content-Length: *\([[:digit:]]*\).*/\1/p') / (1024 * 1024))) |
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.
You accidentally deleted the -s
from the curl
call, otherwise it also prints the progress:
sh-5.2# curl -IL "$liveurl" | sed -n 's/Content-Length: *\([[:digit:]]*\).*/\1/p'
% Total % Received % Xferd Average Speed Time Time Time Current
Dload Upload Total Spent Left Speed
0 0 0 0 0 0 0 0 --:--:-- --:--:-- --:--:-- 0
0 885M 0 0 0 0 0 0 --:--:-- --:--:-- --:--:-- 0
928514048
sh-5.2# curl -sIL "$liveurl" | sed -n 's/Content-Length: *\([[:digit:]]*\).*/\1/p'
928514048
Is that what you intended to do?
Other than that, the patch looks good, thanks!
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.
No, that was an mistake.
Thanks for spotting!
->fixed
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.
thanks
For writeable live images, the memory must hold the complete image. That means it must be at least the size of the image plus some RAM necessary to run processes. With too small RAM, the OOM Killer steps in and makes the boot hang. This patch lets the system go into the emergency shell instead. The default minimum RAM after subtracting the live image size is set to 1G. The parameter rd.minmem is added to modify this.
3380ab8
to
1faf3a9
Compare
For writeable live images, the memory must hold the complete image. That means it must be at least the size of the image plus some RAM necessary to run processes.
With too small RAM, the OOM Killer steps in and makes the boot hang. This patch lets the system go into the emergency shell instead.
The minimum RAM of 512Mb is just arbitrarily choosen. We could set any other number that appears appropriate instead. Alternatively, we could add a new parameter rd.min.ram or such.
This pull request changes...
Changes
livenet module
Checklist
Fixes #
OOM in livenetboot with too low memory.