-
Notifications
You must be signed in to change notification settings - Fork 171
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
live-iso: enable building with squashfs or erofs #4012
Conversation
f-c-c PR: coreos/fedora-coreos-config#3342 |
4bf53a4
to
9f785e1
Compare
9f785e1
to
e187641
Compare
osbuild PR: osbuild/osbuild#2002 |
e187641
to
90e6747
Compare
src/image-default.yaml
Outdated
|
||
# Use erofs by default | ||
live-rootfs-fstype: "erofs" | ||
live-rootfs-fsoptions: "-Eall-fragments,fragdedupe=inode -C131072 --quiet" |
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.
coreos/fedora-coreos-tracker#1852 (comment) mentions
-zlzma,6 -Eall-fragments,fragdedupe=inode -C1048576
as the best option I think.
@jlebon do you think it's a good idea to set the defaults here, or should we just put this info just in the config repos?
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.
This is changing quite fast currently, so it might be premature to bake it here. I would leave everything off here to just --quiet
and let it live in the config for now. Also because those changes are not in RHEL yet AFAIK. Once things stabilize we can add defaults here.
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.
-zlzma,6 -Eall-fragments,fragdedupe=inode -C1048576
on my system (F41) that segfaults:
Creating erofs with -zlzma,6 -Eall-fragments,fragdedupe=inode -C1048576 --quiet
Filesystem Size Used Avail Use% Mounted on
/dev/vdb1 30G 8.7G 22G 29% /srv/cache
ThreadSanitizer:DEADLYSIGNAL
==56==ERROR: ThreadSanitizer: SEGV on unknown address 0x000c0001000f (pc 0x7fda1608efb5 bp 0x7fd9d9c9d490 sp 0x7fd9d9c9d460 T73)
==56==The signal is caused by a READ memory access.
ThreadSanitizer:DEADLYSIGNAL
ThreadSanitizer: nested bug in the same thread, aborting.
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.
can you try with https://bodhi.fedoraproject.org/updates/FEDORA-2025-71df831525
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.
can you try with https://bodhi.fedoraproject.org/updates/FEDORA-2025-71df831525
same failure
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.
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.
I would leave everything off here to just
--quiet
and let it live in the config for now.
Taking this a step farther I think I'm going to argue to just not include defaults in COSA here at all for now. The OSBuild stage (for now) will default to squashfs/zstd
and it will be easier to ratchet in erofs
in the streams we want if COSA isn't setting it too I think. My proposal for now is to just put these values here and comment them out:
# an example of setting ISO/PXE rootfs fstype and fsoptions
# live-rootfs-fstype: "erofs"
# live-rootfs-fsoptions: "--quiet"
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 filing this! Hopefully we can sort this out and leave the supermin requirement alone (or only slightly bump it).
In practice, if we really have to... I think even if we did bump it to 6G it shouldn't affect our pipeline requests. We already request more there and the point at which we run osbuild, it runs alone. And additionally AIUI osbuild still currently runs pipelines serially even when there is no dependence between two branches.
Though we should at least make it dynamic; e.g. cmd-osbuild already loads image.json
so it could trivially check whether EROFS was requested and size the supermin VM correspondingly. This helps ease memory pressure on the multi-arch builders for RHCOS where we'll be building squashfs also for a while and also the developer case. But again, let's see with the maintainer whether we can avoid it at all (or maybe just do it short-term).
Can we make sure as part of this One option may be to rename some of the options
The cc @jlebon since he wrote |
Those renames make sense to me! |
37c49c2
to
202968c
Compare
202968c
to
fb88169
Compare
The last commit there is interesting/potentially concerning. Can you go into more details of what you're seeing there? Was this just the rootfs image itself being larger and so needing to bump RAM requirements to even store it? Or was it e.g. mounting the EROFS itself permanently carving off a lot of RAM? What was the minimum amount of additional RAM that made it work? |
On my system
|
Can you show me Also for LZMA, Zstd algorithms, the kernel will allocate a internal decompression dictionary for each CPU (just like SQUASHFS_DECOMP_MULTI_PERCPU). For LZMA, the dictionary size is *8 by default, so Also if if image sizes show little difference, I suggest to use smaller |
I'm wondering the cause of it too, the issue is actually a mkfs issue, and increase tmpfs space will generate the proper image finally. But the @nikita-dubrovskii recent message is actually a runtime report, are those two different reports actually? Use Also would you mind sharing your produced rootfs to me as well if possible?
|
6328c76
to
99e4b5a
Compare
@hsiangkao , i've built locally FCOS with Now will rebuild and retest using |
but why Also I didn't find any dmesg like |
@hsiangkao I think maybe you should ignore this conversation for now. We're talking about something very internal to CoreOS (i.e. testing the ISO created that has the erofs baked inside) and it's probably misleading to what actual root causes are.
I think this is because your actual ISO is large because you weren't using compression when creating the erofs ( @nikita-dubrovskii try with this patch and see if you get the ISO generation to work fine with diff --git a/src/cmdlib.sh b/src/cmdlib.sh
index f83ae98a4..a089a46c7 100755
--- a/src/cmdlib.sh
+++ b/src/cmdlib.sh
@@ -767,7 +767,7 @@ EOF
# There seems to be some false positives in shellcheck
# https://github.com/koalaman/shellcheck/issues/2217
- memory_default=2048
+ memory_default=6096
# shellcheck disable=2031
case $arch in
# Power 8 page faults with 2G of memory in rpm-ostree |
@dustymabe thx, that |
99e4b5a
to
8ebefbe
Compare
Currently `testiso` only uses 4Gb and ignores `--qemu-memory`, which leads to a failure when testing new live-images with `/root.erofs`: ``` [ 2.133469] dracut-cmdline[536]: /usr/sbin/initqueue: line 65: echo: write error: No space left on device ``` With this PR it's now possible to override default settings: ``` $ cosa kola testiso pxe* --pxe-append-rootfs --qemu-memory 8192 Running test: pxe-offline-install.bios PASS: pxe-offline-install.bios (1m27.545s) ... ```
8ebefbe
to
7951e2c
Compare
Differ("live-sysroot-ls", "Diff live '/root.[erofs|squash]fs' (embed into live-rootfs) listings", | ||
needs_ostree=False, function=diff_live_sysroot_tree), | ||
Differ("live-sysroot", "Diff live '/root.[ero|squash]fs' (embed into live-rootfs) content", | ||
needs_ostree=False, function=diff_live_sysroot), |
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.
I think these could be named better, but I'll argue that point in a separate PR and see what other people think.
functionally this should work great!
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.
I'll hold off reviewing this hunk until your PR. :)
src/image-default.yaml
Outdated
# Use erofs by default | ||
live-rootfs-fstype: "erofs" | ||
live-rootfs-fsoptions: "-zlzma,level=6 -Eall-fragments,fragdedupe=inode -C1048576 --quiet" |
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.
still think we should leave this out of COSA for now:
# Use erofs by default | |
live-rootfs-fstype: "erofs" | |
live-rootfs-fsoptions: "-zlzma,level=6 -Eall-fragments,fragdedupe=inode -C1048576 --quiet" | |
# Defaults for the root filesystem in the Live ISO/PXE artifacts. Left unset | |
# for now as we ratchet in erofs support. | |
# live-rootfs-fstype: "erofs" | |
# live-rootfs-fsoptions: "-zlzma,level=6 -Eall-fragments,fragdedupe=inode -C1048576 --quiet" |
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.
added a new commit here for this. This also happens to make CI pass again. Because of erofs/erofs-utils#13 it won't pass unless we bump the supermin VM memory and I'm not sure if we really want to do that just yet.
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.
Yeah agree, changing the defaults should come later.
Right now we need to ratchet in these changes using image.yaml from the config repos rather than changing the default for all consumers off the bat. Note that the OSBuild stage defaults to squashfs/zstd so this is just making it so that it what gets picked up and used for anywhere where it's not otherwise configured. This also has the side effect of making CI pass again because of -zlzma,level=6 requiring a lot of memory right now: erofs/erofs-utils#13
squashfs-compression: zstd will become obsolete soon. Let's drop a note about it so anyone happening by can clean it up.
I ran through the various options to |
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
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.
Some minor comments but we can address them in follow-ups to not waste CI. Nice work!
@@ -98,3 +98,6 @@ fedora-repos-ostree | |||
|
|||
# For graphing manifest includes using `manifest_graph` | |||
python-anytree | |||
|
|||
# For mkfs.erofs | |||
erofs-utils |
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.
Minor/optional: this is already included in vmdeps.txt
so it doesn't need to be here as well.
def diff_live_sysroot_tree(diff_from, diff_to): | ||
(dir_from, dir_to) = extract_live_sysroot_img(diff_from, diff_to) | ||
diff_cmd_outputs(['find', '{}', '-printf', "%P\n"], dir_from, dir_to) |
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.
Hmm right, that's much more expensive now that we have to extract the full squashfs/erofs but there isn't really a listing equivalent for erofs we could use here (there's dump.erofs --ls
, but there's no option to make it recurse) which I guess is how you ended up here. And I guess it's nice that it allows comparing across the squashfs -> erofs transition.
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.
That said, I think we want to sort the output of find
since it doesn't guarantee lexicographic order which is important here for a good diff. We could add an option to diff_cmd_outputs
to pipe the output of the cmd to sort
.
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.
there's no option to make it recurse) which I guess is how you ended up here
Yes, and anyhow we unpack it in other diff
cmd, so it's expensive, but probably we can stay with it. Other option - there are some tools available for erofs
, but i haven't tested them
Differ("live-sysroot-ls", "Diff live '/root.[erofs|squash]fs' (embed into live-rootfs) listings", | ||
needs_ostree=False, function=diff_live_sysroot_tree), | ||
Differ("live-sysroot", "Diff live '/root.[ero|squash]fs' (embed into live-rootfs) content", | ||
needs_ostree=False, function=diff_live_sysroot), |
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.
I'll hold off reviewing this hunk until your PR. :)
No description provided.