treefile: Add no-initramfs: knob
#5529
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a no-initramfs knob to the treefile, allowing for the skipping of initramfs generation during the compose process. This is a valuable addition for more complex build workflows, such as those for FCOS/RHCOS. The implementation is well-structured, including documentation updates, necessary changes across the C++ and Rust bridge, and a new test case to validate the functionality. I've identified one potential issue regarding errno handling that could lead to subtle bugs, which I've detailed in a specific comment. The PR also includes a refactoring of normalize_etc_shadow, which is a reasonable change to support the new workflow. Overall, this is a good change that improves the flexibility of the compose process.
| if (!glnx_fstatat_allow_noent (rootfs_dfd, initramfs_modules_path, NULL, AT_SYMLINK_NOFOLLOW, | ||
| error)) | ||
| return FALSE; | ||
| if (errno == 0) |
There was a problem hiding this comment.
The check if (errno == 0) is not guaranteed to be safe. The glnx_fstatat_allow_noent function does not reset errno to 0 on success. If a previous, unrelated syscall failed, errno could hold a non-zero value, leading this check to incorrectly assume the file does not exist when it does. To prevent this potential bug, you should explicitly set errno = 0; before calling glnx_fstatat_allow_noent.
errno = 0;
if (!glnx_fstatat_allow_noent (rootfs_dfd, initramfs_modules_path, NULL, AT_SYMLINK_NOFOLLOW,
error))
return FALSE;
if (errno == 0)There was a problem hiding this comment.
There are other uses in this file that don't seem to reset errno. Maybe we should be doing that everywhere?
There was a problem hiding this comment.
This is a good observation by Gemini, but since it only sees the code in this repo it doesn't know that actually that API does always set errno = 0: https://gitlab.gnome.org/GNOME/libglnx/-/blob/de29c5f7d9df8d57b4f5caa9920f5d4caa7a8cfc/glnx-fdio.h#L347
|
related to this is: https://gitlab.com/fedora/bootc/tracker/-/issues/78 |
|
ostree doesn't like the trailing |
In the work to rebase FCOS/RHCOS to fedora-bootc/rhel-bootc, we want to run postprocess scripts for CoreOS specific changes to the rootfs, but we want changes made in those postprocess scripts to affect the initramfs generation too. The new experimental `no-initramfs` knob will allow us to disable the generation of the initramfs via dracut in the initial compose of the rootfs. Then we can run our postprocess scripts on the rootfs and later run dracut to generate the initramfs. This came from discussion in https://gitlab.com/fedora/bootc/base-images/-/merge_requests/315 where it was decided to not add a `--postprocess` argument to bootc-base-imagectl. Co-authored-by: Jonathan Lebon <jonathan@jlebon.com>
With the no-initramfs treefile knob the underlying code that normalizes
/etc/shadow when dracut is run isn't getting called. Let's just call it
unconditionally here.
This yielded a diff like the following when this function wasn't getting
called:
```
$ diff -ur tmp/diff-cache/metal/43.20251104.dev.{0,1}/ostree/deploy/fedora-coreos/deploy/XXXXXXXXXXXXXXXX.0/etc/shadow
--- tmp/diff-cache/metal/43.20251104.dev.0/ostree/deploy/fedora-coreos/deploy/XXXXXXXXXXXXXXXX.0/etc/shadow 2022-08-01 19:42:11.000000000 -0400
+++ tmp/diff-cache/metal/43.20251104.dev.1/ostree/deploy/fedora-coreos/deploy/XXXXXXXXXXXXXXXX.0/etc/shadow 2022-08-01 19:42:11.000000000 -0400
@@ -30,9 +30,9 @@
systemd-resolve:*::0:99999:7:::
systemd-timesync:*::0:99999:7:::
tcpdump:*::0:99999:7:::
-zincati:!*:::::::
-clevis:!*:::::::
-dnsmasq:!*:::::::
-systemd-coredump:!*:::::::
-systemd-oom:!*:::::::
-tss:!*:::::::
+zincati:!*:20396::::::
+clevis:!*:20396::::::
+dnsmasq:!*:20396::::::
+systemd-coredump:!*:20396::::::
+systemd-oom:!*:20396::::::
+tss:!*:20396::::::
```
dustymabe
force-pushed
the
dusty-no-initramfs
branch
from
53cc170 to
989ff9e
Compare
fixed! also tried to fix the differential shellcheck. The e2e test is failing with: but I see that on other recent PRs so I don't think it's related to this change. |
2 participants
In the work to rebase FCOS/RHCOS to fedora-bootc/rhel-bootc, we want to
run postprocess scripts for CoreOS specific changes to the rootfs, but
we want changes made in those postprocess scripts to affect the
initramfs generation too.
The new experimental
no-initramfsknob will allow us to disable thegeneration of the initramfs via dracut in the initial compose of the
rootfs. Then we can run our postprocess scripts on the rootfs and later
run dracut to generate the initramfs.
This came from discussion in https://gitlab.com/fedora/bootc/base-images/-/merge_requests/315
where it was decided to not add a
--postprocessargument to bootc-base-imagectl.Co-authored-by: Jonathan Lebon jonathan@jlebon.com