kernel-install: Support drop-in config directories for layout=ostree#5551
kernel-install: Support drop-in config directories for layout=ostree#5551jmarrero merged 1 commit intocoreos:mainfrom
Conversation
There was a problem hiding this comment.
Code Review
This pull request addresses an issue with incomplete kernel directories being left behind during container builds. The fix involves returning SKIP for remove operations to prevent problematic systemd plugin logic from running, and adding a cleanup step for incomplete kernel directories. The changes are logical and well-supported by new tests. My feedback focuses on improving path handling in the new tests for better maintainability and robustness.
a8118b4 to
98551fc
Compare
|
It looks like the original idea is not the root cause at all and it's a bit more simple: |
90e997d to
7fdd17a
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request extends support for layout=ostree to include drop-in configuration directories, which is a valuable improvement. The changes in both the shell script and the Rust code correctly add more locations to check. However, my review identified a significant logical flaw in how configuration priority is handled in both implementations. They currently check for the presence of layout=ostree but do not correctly handle cases where a higher-priority configuration file specifies a different layout. This could lead to incorrect behavior. I've provided detailed comments with high severity for both the shell script and the Rust code, including a suggested fix for the script and a refactoring approach for the Rust code to address this issue.
7fdd17a to
038f0f6
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request aims to improve support for kernel-install drop-in configuration directories for layout=ostree. The change to the 05-rpmostree.install shell hook correctly leverages the KERNEL_INSTALL_LAYOUT environment variable set by kernel-install. However, the corresponding update to the Rust function is_ostree_layout is flawed and does not correctly implement the configuration parsing logic as described in kernel-install(8). I've identified a critical issue with the implementation logic and a medium-severity issue regarding insufficient test coverage for this new logic. Please see my detailed comments.
038f0f6 to
4319e3a
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request correctly implements support for kernel-install drop-in configuration directories for setting layout=ostree. The changes are well-structured, following the specified priority order for configuration files, and include comprehensive tests for the new logic. The shell hook is also updated to correctly use the KERNEL_INSTALL_LAYOUT environment variable. I've provided a couple of suggestions in the Rust code to improve idiomacity and robustness.
4319e3a to
4fbdb10
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request correctly implements support for kernel-install drop-in configuration directories, which is a valuable improvement for configuration management. The changes in the Rust module are well-structured, introducing new helper functions to parse configuration files and directories while respecting precedence rules. The addition of comprehensive tests covering various scenarios is excellent. The shell hook is also updated to correctly rely on the environment variable set by kernel-install. I have a few suggestions to further improve the implementation.
When systemd-udev is upgraded, it overwrites /usr/lib/kernel/install.conf with its default template, removing the layout=ostree setting. To avoid this, bootc base-images places the layout config in a drop-in file. Update is_ostree_layout() to parse kernel-install configuration per the kernel-install(8) specification, checking config sources in priority order: 1. /etc/kernel/install.conf 2. /etc/kernel/install.conf.d/*.conf 3. /usr/lib/kernel/install.conf 4. /usr/lib/kernel/install.conf.d/*.conf Drop-in files are processed in lexicographic order with later files overriding earlier ones, matching systemd conventions. Note: We cannot simply rely on the KERNEL_INSTALL_LAYOUT environment variable because this function is called at compose time and during cliwrap interception, when kernel-install is not running. The shell hook 05-rpmostree.install uses the env var since it is invoked by kernel-install itself. Closes: RHEL-128312 Assisted-by: OpenCode (Claude Sonnet 4) Signed-off-by: Joseph Marrero Corchado <jmarrero@redhat.com>
4fbdb10 to
3592bee
Compare
|
Failures here unrelated, tested this manually and with opencode. Generated a new image with this rpm-ostree change too. We should be good to go. |
kernel-install: Support drop-in config directories for layout=ostree
The 05-rpmostree.install hook and Rust kernel_install module previously
only checked /usr/lib/kernel/install.conf for the layout=ostree setting.
This breaks when the configuration is placed in a drop-in config file,
which per kernel-install(8) is the recommended way to preserve settings
that may be overwritten by package updates.
Update both the shell hook and Rust code to check all config sources
in priority order per kernel-install(8):
kernel-install based on its config file parsing)
This allows bootc base images to place the configuration in a drop-in
file (e.g., /usr/lib/kernel/install.conf.d/00-kernel-layout.conf) where
it will not be overwritten when systemd-udev updates install.conf,
while maintaining backward compatibility with existing configurations.
Closes: RHEL-128312
Assisted-by: OpenCode (Opus 4.5)
Signed-off-by: Joseph Marrero Corchado jmarrero@redhat.com