composefs: Add missed commits when rebasing #1502

cgwalters · 2025-08-12T08:49:00Z

I messed up the previous rebase/squash of the composefs branch. This adds commits since c89efab

There were definitely some messy conflicts, but this at least compiles

Parse the Grub menuentry file, `boot/grub2/user.cfg` to get a list of bootable UKIs and figure out if a rollback is currently queued. Signed-off-by: Johan-Liebert1 <pragyanpoudyal41999@gmail.com>

gemini-code-assist

Code Review

This pull request incorporates a number of commits that were missed during a previous rebase. The changes include significant refactoring, such as moving parsers to a dedicated module and centralizing constants, which improves code organization. The introduction of boot digest computation to avoid duplicating kernel and initrd files is a good optimization.

My review has identified a few critical issues. There are a couple of assert! macros that should be replaced with proper error handling to prevent panics in debug builds and undefined behavior in release builds. I also found a typo in kernel argument formatting that would lead to an invalid argument, and an incorrect usage of unlinkat for directory removal that would cause an error.

Apart from these issues, the changes look good and improve the codebase.

gemini-code-assist · 2025-08-12T08:50:19Z

crates/lib/src/deploy.rs

+        get_sorted_uki_boot_entries(&boot_dir, &mut str).context("Getting UKI boot entries")?;
+
+    // TODO(Johan-Liebert): Currently assuming there are only two deployments
+    assert!(menuentries.len() == 2);


The use of assert! here can lead to a panic in debug builds or incorrect behavior in release builds if the number of menu entries is not exactly 2. It's better to return an error, especially since the TODO comment acknowledges this is a known limitation.

Suggested change

assert!(menuentries.len() == 2);

if menuentries.len() != 2 {

anyhow::bail!("Expected exactly 2 UKI boot entries, found {}", menuentries.len());

}

gemini-code-assist · 2025-08-12T08:50:19Z

crates/lib/src/deploy.rs

    }

+    // TODO(Johan-Liebert): Currently assuming there are only two deployments
    assert!(all_configs.len() == 2);


Similar to the UKI rollback logic, this assert! can cause a panic in debug builds or lead to incorrect behavior in release builds. It should be replaced with proper error handling.

Suggested change

assert!(all_configs.len() == 2);

if all_configs.len() != 2 {

anyhow::bail!("Expected exactly 2 BLS boot entries, found {}", all_configs.len());

}

gemini-code-assist · 2025-08-12T08:50:19Z

crates/lib/src/install.rs

                }
                None | Some(..) => {
-                    cmdline_options.push_str(&format!(" composefs={id_hex}"));
+                    cmdline_options.push_str(&format!(" {COMPOSEFS_CMDLINE}{id_hex}"));


There seems to be a missing = in the format string. This will generate an invalid kernel argument composefs<id_hex> instead of composefs=<id_hex>.

Suggested change

cmdline_options.push_str(&format!(" {COMPOSEFS_CMDLINE}{id_hex}"));

cmdline_options.push_str(&format!(" {COMPOSEFS_CMDLINE}={id_hex}"));

gemini-code-assist · 2025-08-12T08:50:19Z

crates/lib/src/deploy.rs

+    rustix::fs::unlinkat(&dir, ROLLBACK_BOOT_LOADER_ENTRIES, AtFlags::empty())
+        .context("unlinkat")?;


The unlinkat call with AtFlags::empty() will fail with EISDIR because ROLLBACK_BOOT_LOADER_ENTRIES refers to a directory. The previous code with AtFlags::REMOVEDIR would also fail because the directory is not empty. To remove a directory and its contents, you should use a recursive removal function like remove_dir_all.

Suggested change

rustix::fs::unlinkat(&dir, ROLLBACK_BOOT_LOADER_ENTRIES, AtFlags::empty())

.context("unlinkat")?;

dir.remove_dir_all(ROLLBACK_BOOT_LOADER_ENTRIES)

.with_context(|| format!("Removing directory {}", ROLLBACK_BOOT_LOADER_ENTRIES))?;

Johan-Liebert1 · 2025-08-12T09:44:41Z

crates/lib/src/install.rs

                format!("root=UUID={DPS_UUID}"),
                RW_KARG.to_string(),
-                format!("composefs={id_hex}"),
+                format!("{COMPOSEFS_CMDLINE}{id_hex}"),


= missing here

Good catch, Gemini found the other one of this.

Johan-Liebert1 · 2025-08-12T09:49:00Z

crates/lib/src/bls_config.rs

-    pub(crate) fn to_string(&self) -> String {
-        let mut out = String::new();
-
+impl Display for BLSConfig {


This should've been moved to lib/src/parsers/bls_config.rs and indeed it has been. This piece seems leftover

Johan-Liebert1 · 2025-08-12T09:52:37Z

crates/lib/src/parsers/grub_menuconfig.rs

 /// Body content of a GRUB menuentry containing parsed commands.
 #[derive(Debug, PartialEq, Eq)]
 pub(crate) struct MenuentryBody<'a> {
-    /// Kernel modules to load


@cgwalters The changes you made, and the ones we pushed to main, seem to have been overwritten here. I think we should keep your changes for this particular file

Just the added comments right? The rest of this was changing things here to use an owned value for chainloader per your previous commit.

(That said, it's weird to have a mix of borrowed and owned data here...)

Yes, it is kinda weird, but getting rust to return a local ref without Box::leak is pretty complicated

There's https://lib.rs/crates/yoke among others

Returning a local reference to a `&str` is quite tricky with rust. Update `title` and `chainloader`, the two dynamic fields in the grub menuentry, to be `String` instead of `&str` Signed-off-by: Johan-Liebert1 <pragyanpoudyal41999@gmail.com>

We parse the grub menuentries, get the rollback deployment then perform the rollback, which basically consists of writing a new .staged menuentry file then atomically swapping the staged and the current menuentry. Rollback while there is a staged deployment is still to be handled. Signed-off-by: Johan-Liebert1 <pragyanpoudyal41999@gmail.com>

…iles Signed-off-by: Johan-Liebert1 <pragyanpoudyal41999@gmail.com>

If two deployments have the same VMLinuz + Initrd then, we can use the same binaries for both the deployments. Before writing the BLS entries to disk we calculate the SHA256Sum of VMLinuz + Initrd combo, then test if any other deployment has the same SHA256Sum for the binaries. Store the hash in the origin file under `boot -> hash` for future lookups. Signed-off-by: Johan-Liebert1 <pragyanpoudyal41999@gmail.com>

Signed-off-by: Johan-Liebert1 <pragyanpoudyal41999@gmail.com>

Centralize all constants in a separate file Signed-off-by: Johan-Liebert1 <pragyanpoudyal41999@gmail.com>

Instead of `/sysroot/state/os/fedora` use `/sysroot/state/os/default` as the default state directory. Signed-off-by: Johan-Liebert1 <pragyanpoudyal41999@gmail.com>

Signed-off-by: Johan-Liebert1 <pragyanpoudyal41999@gmail.com>

Instaed of writing all present menuentries, only write the menuentry for switch/upgrade and the menuentry for the currently booted deployment. Signed-off-by: Johan-Liebert1 <pragyanpoudyal41999@gmail.com>

Signed-off-by: Johan-Liebert1 <pragyanpoudyal41999@gmail.com>

This allows for easier testing Signed-off-by: Pragyan Poudyal <pragyanpoudyal41999@gmail.com>

Add tests for functions `get_sorted_bls_boot_entries` and `get_sorted_uki_boot_entries` Signed-off-by: Pragyan Poudyal <pragyanpoudyal41999@gmail.com>

cgwalters · 2025-08-12T10:10:13Z

OK, updated

Johan-Liebert1 · 2025-08-12T10:22:53Z

centos-10 tests seem to be failing due to package conflicts. Else LGTM

+ dnf -y builddep contrib/packaging/bootc.spec
Updating Subscription Management repositories.
Unable to read consumer identity

This system is not registered with an entitlement server. You can use "rhc" or "subscription-manager" to register.

CentOS Stream 10 - BaseOS                        35 MB/s | 6.7 MB     00:00    
CentOS Stream 10 - AppStream                     23 MB/s | 3.3 MB     00:00    
CentOS Stream 10 - CRB                          7.0 MB/s | 748 kB     00:00    
CentOS Stream 10 - Extras packages               54 kB/s | 6.9 kB     00:00    

Package ostree-2025.4-3.el10.x86_64 is already installed.
Package skopeo-2:1.20.0-1.el10.x86_64 is already installed.
Package systemd-257-11.el10.x86_64 is already installed.
Error: 
 Problem: cannot install both ostree-libs-2025.4-2.el10.x86_64 from appstream and ostree-libs-2025.4-3.el10.x86_64 from @System
  - package ostree-devel-2025.4-2.el10.x86_64 from crb requires ostree-libs(x86-64) = 2025.4-2.el10, but none of the providers can be installed
  - package ostree-2025.4-3.el10.x86_64 from @System requires ostree-libs(x86-64) = 2025.4-3.el10, but none of the providers can be installed
  - cannot install the best candidate for the job
(try to add '--allowerasing' to command line to replace conflicting packages or '--skip-broken' to skip uninstallable packages or '--nobest' to use not only best candidate packages)
Error: building at STEP "RUN <<EORUN": while running runtime: exit status 1

cgwalters · 2025-08-12T10:24:54Z

Yeah that's https://gitlab.com/redhat/centos-stream/containers/bootc/-/issues/1174 - unrelated to this PR

composefs/status: Read UKI entries to check for queued rollback

93a6d75

Parse the Grub menuentry file, `boot/grub2/user.cfg` to get a list of bootable UKIs and figure out if a rollback is currently queued. Signed-off-by: Johan-Liebert1 <pragyanpoudyal41999@gmail.com>

gemini-code-assist bot reviewed Aug 12, 2025

View reviewed changes

cgwalters mentioned this pull request Aug 12, 2025

bootc composefs-native backend #1190

Open

17 tasks

cgwalters requested a review from Johan-Liebert1 August 12, 2025 09:34

Johan-Liebert1 reviewed Aug 12, 2025

View reviewed changes

Johan-Liebert1 and others added 13 commits August 12, 2025 12:08

parser/grub: Use String instead of &str

dc862a2

Returning a local reference to a `&str` is quite tricky with rust. Update `title` and `chainloader`, the two dynamic fields in the grub menuentry, to be `String` instead of `&str` Signed-off-by: Johan-Liebert1 <pragyanpoudyal41999@gmail.com>

composefs/state: Use atomic writes for origin and staged deployment f…

d3e8d36

…iles Signed-off-by: Johan-Liebert1 <pragyanpoudyal41999@gmail.com>

parser/bls: impl Display for BLSConfig

805a42d

Signed-off-by: Johan-Liebert1 <pragyanpoudyal41999@gmail.com>

lib/composefs: Centralize constants

009ee03

Centralize all constants in a separate file Signed-off-by: Johan-Liebert1 <pragyanpoudyal41999@gmail.com>

composefs/state: Name state directory default

1b99733

Instead of `/sysroot/state/os/fedora` use `/sysroot/state/os/default` as the default state directory. Signed-off-by: Johan-Liebert1 <pragyanpoudyal41999@gmail.com>

parser/bls: Add tests for bls parser

a53ca13

Signed-off-by: Johan-Liebert1 <pragyanpoudyal41999@gmail.com>

install/composefs/uki: Write only staged + booted menuentry on upgrade

08b1cad

Instaed of writing all present menuentries, only write the menuentry for switch/upgrade and the menuentry for the currently booted deployment. Signed-off-by: Johan-Liebert1 <pragyanpoudyal41999@gmail.com>

rollback/composefs: Print whether we are reverting the queued rollback

d425878

Signed-off-by: Johan-Liebert1 <pragyanpoudyal41999@gmail.com>

refactor: Pass boot dir to boot entry readers

521a2ed

This allows for easier testing Signed-off-by: Pragyan Poudyal <pragyanpoudyal41999@gmail.com>

test: Add tests for reading boot entries

2018de3

Add tests for functions `get_sorted_bls_boot_entries` and `get_sorted_uki_boot_entries` Signed-off-by: Pragyan Poudyal <pragyanpoudyal41999@gmail.com>

Drop duplicate bls_config

95bbfe6

cgwalters force-pushed the composefs-add-lost-commits branch from 3348aba to 95bbfe6 Compare August 12, 2025 10:08

cgwalters merged commit b091e0f into bootc-dev:composefs-backend Aug 12, 2025
1 of 20 checks passed

-    assert!(menuentries.len() == 2);
+    if menuentries.len() != 2 {
+        anyhow::bail!("Expected exactly 2 UKI boot entries, found {}", menuentries.len());
+    }

-    assert!(all_configs.len() == 2);
+    if all_configs.len() != 2 {
+        anyhow::bail!("Expected exactly 2 BLS boot entries, found {}", all_configs.len());
+    }

	cmdline_options.push_str(&format!(" {COMPOSEFS_CMDLINE}{id_hex}"));
	cmdline_options.push_str(&format!(" {COMPOSEFS_CMDLINE}={id_hex}"));

		rustix::fs::unlinkat(&dir, ROLLBACK_BOOT_LOADER_ENTRIES, AtFlags::empty())
		.context("unlinkat")?;

composefs: Add missed commits when rebasing #1502

composefs: Add missed commits when rebasing #1502

Uh oh!

Conversation

cgwalters commented Aug 12, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Aug 12, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Aug 12, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Aug 12, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Aug 12, 2025

Choose a reason for hiding this comment

Uh oh!

Johan-Liebert1 Aug 12, 2025

Choose a reason for hiding this comment

Uh oh!

cgwalters Aug 12, 2025

Choose a reason for hiding this comment

Uh oh!

Johan-Liebert1 Aug 12, 2025

Choose a reason for hiding this comment

Uh oh!

Johan-Liebert1 Aug 12, 2025

Choose a reason for hiding this comment

Uh oh!

cgwalters Aug 12, 2025

Choose a reason for hiding this comment

Uh oh!

Johan-Liebert1 Aug 12, 2025

Choose a reason for hiding this comment

Uh oh!

cgwalters Aug 12, 2025

Choose a reason for hiding this comment

Uh oh!

cgwalters commented Aug 12, 2025

Uh oh!

Johan-Liebert1 commented Aug 12, 2025

Uh oh!

cgwalters commented Aug 12, 2025

Uh oh!

Uh oh!

Uh oh!