WIP: composefs branch #1444

cgwalters · 2025-07-24T13:05:59Z

Moved from #1314 (a lot of prior discussion and reviews there!)

Tracking issue: #1190

This is now a branch on the main repo to make it easier for folks to collaborate.

gemini-code-assist

Code Review

This pull request introduces composefs as a new backend, which is a significant and extensive change. The new code paths for installation, upgrades, switching, and rollbacks for composefs are well-structured.

My review has identified a few issues, including a potential bug with a hardcoded transport value and an assert! that could lead to a panic. I've also included some suggestions for improving code style and robustness. Given this is a work-in-progress, these findings are not unexpected. Overall, this is a solid foundation for the new composefs functionality.

gemini-code-assist · 2025-07-24T13:07:37Z

crates/lib/src/cli.rs

+        anyhow::bail!("Target image is undefined")
+    };
+
+    let (repo, entries, id) = pull_composefs_repo(&"docker".into(), &target_imgref.image).await?;


The transport is hardcoded to "docker". The target_imgref variable already contains the correct transport from the user's options. Using a hardcoded value will likely cause switch to fail for other transports.

let (repo, entries, id) = pull_composefs_repo(&target_imgref.transport, &target_imgref.image).await?;

gemini-code-assist · 2025-07-24T13:07:37Z

crates/lib/src/deploy.rs

+        cfg.version = idx as u32;
+    }
+
+    assert!(all_configs.len() == 2);


Using assert! here will cause a panic if the number of boot configurations is not exactly 2. This can happen if there's only one deployment. It would be more robust to use anyhow::ensure! to return a proper error that can be handled gracefully.

Suggested change

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

anyhow::ensure!(

all_configs.len() == 2,

"Expected exactly 2 boot configurations for rollback, found {}",

all_configs.len()

);

gemini-code-assist · 2025-07-24T13:07:38Z

crates/lib/src/cli.rs

@@ -881,6 +931,63 @@
    let sigverify = sigpolicy_from_opt(opts.enforce_container_sigpolicy);
    let target = ostree_container::OstreeImageReference { sigverify, imgref };
    let target = ImageReference::from(target);
+
+    return Ok(target);


In Rust, it's more idiomatic to omit the return keyword for the final expression in a function.

Ok(target)

gemini-code-assist · 2025-07-24T13:07:38Z

crates/lib/src/deploy.rs

+
+    all_configs.sort_by(|a, b| if ascending { a.cmp(b) } else { b.cmp(a) });
+
+    return Ok(all_configs);


In Rust, it's more idiomatic to omit the return keyword for the final expression in a function.

Suggested change

return Ok(all_configs);

Ok(all_configs)

gemini-code-assist · 2025-07-24T13:07:38Z

crates/lib/src/install.rs

+"#
+    );
+
+    return s;


In Rust, it's more idiomatic to omit the return keyword for the final expression in a function.

Suggested change

return s;

s

gemini-code-assist · 2025-07-24T13:07:38Z

crates/lib/src/install.rs

+
+    origin_file
+        .write(config.to_string().as_bytes())
+        .context("Falied to write to .origin file")?;


There's a typo in the error message: "Falied" should be "Failed".

Suggested change

.context("Falied to write to .origin file")?;

.context("Failed to write to .origin file")?;

gemini-code-assist · 2025-07-24T13:07:38Z

crates/lib/src/install.rs

+        tracing::warn!(
+            "id = {id}, verity = {verity}",
+            id = hex::encode(id),
+            verity = verity.to_hex()
+        );


Using tracing::warn! here seems inappropriate, as logging the image ID and verity hash is informational rather than a warning. Consider using tracing::info! or tracing::debug! instead to better reflect the nature of this log message.

Suggested change

tracing::warn!(

"id = {id}, verity = {verity}",

id = hex::encode(id),

verity = verity.to_hex()

);

tracing::info!(

"id = {id}, verity = {verity}",

id = hex::encode(id),

verity = verity.to_hex()

);

gemini-code-assist · 2025-07-24T13:07:38Z

crates/lib/src/status.rs

+        composefs: Some(crate::spec::BootEntryComposefs { verity, boot_type }),
+    };
+
+    return Ok(e);


In Rust, it's more idiomatic to omit the return keyword for the final expression in a function.

Suggested change

return Ok(e);

Ok(e)

cgwalters · 2025-08-04T13:06:19Z

I'm going to rebase this. In the interest of preserving history I've saved a copy of the branch as composefs-branch-20250804

cgwalters · 2025-08-05T13:41:31Z

OK at least this passes the existing tests.

Signed-off-by: Pragyan Poudyal <[email protected]> Signed-off-by: Colin Walters <[email protected]>

Signed-off-by: Colin Walters <[email protected]>

cgwalters · 2025-08-07T15:18:55Z

I rebased 🏄 this again (and in the process reworked the composefs karg stuff to use the new kernel_cmdline &str APIs)

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 <[email protected]>

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 <[email protected]>

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 <[email protected]>

…iles Signed-off-by: Johan-Liebert1 <[email protected]>

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 <[email protected]>

Signed-off-by: Johan-Liebert1 <[email protected]>

Centralize all constants in a separate file Signed-off-by: Johan-Liebert1 <[email protected]>

Instead of `/sysroot/state/os/fedora` use `/sysroot/state/os/default` as the default state directory. Signed-off-by: Johan-Liebert1 <[email protected]>

Signed-off-by: Johan-Liebert1 <[email protected]>

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 <[email protected]>

Signed-off-by: Johan-Liebert1 <[email protected]>

This allows for easier testing Signed-off-by: Pragyan Poudyal <[email protected]>

Add tests for functions `get_sorted_bls_boot_entries` and `get_sorted_uki_boot_entries` Signed-off-by: Pragyan Poudyal <[email protected]>

composefs: Add missed commits when rebasing

Just reducing code here.

install: Use read_file from composefs-boot

gemini-code-assist bot reviewed Jul 24, 2025

View reviewed changes

cgwalters mentioned this pull request Aug 1, 2025

Composefs-native backend #1314

Merged

1 task

cgwalters force-pushed the composefs-backend branch 2 times, most recently from 33e4511 to 7d596fe Compare August 4, 2025 19:56

Johan-Liebert1 and others added 2 commits August 7, 2025 10:53

WIP: composefs backend

5885686

Signed-off-by: Pragyan Poudyal <[email protected]> Signed-off-by: Colin Walters <[email protected]>

Rework composefs_booted to use kernel cmdline

d9b384d

Signed-off-by: Colin Walters <[email protected]>

cgwalters force-pushed the composefs-backend branch from 7d596fe to d9b384d Compare August 7, 2025 15:18

Johan-Liebert1 and others added 17 commits August 12, 2025 09:53

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 <[email protected]>

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 <[email protected]>

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

d3e8d36

…iles Signed-off-by: Johan-Liebert1 <[email protected]>

parser/bls: impl Display for BLSConfig

805a42d

Signed-off-by: Johan-Liebert1 <[email protected]>

lib/composefs: Centralize constants

009ee03

Centralize all constants in a separate file Signed-off-by: Johan-Liebert1 <[email protected]>

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 <[email protected]>

parser/bls: Add tests for bls parser

a53ca13

Signed-off-by: Johan-Liebert1 <[email protected]>

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 <[email protected]>

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

d425878

Signed-off-by: Johan-Liebert1 <[email protected]>

refactor: Pass boot dir to boot entry readers

521a2ed

This allows for easier testing Signed-off-by: Pragyan Poudyal <[email protected]>

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 <[email protected]>

Drop duplicate bls_config

95bbfe6

Merge pull request #1502 from cgwalters/composefs-add-lost-commits

b091e0f

composefs: Add missed commits when rebasing

install: Use read_file from composefs-boot

06693d6

Just reducing code here.

Merge pull request #1507 from cgwalters/composefs-more-cleanups

e2b19f9

install: Use read_file from composefs-boot

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

WIP: composefs branch #1444

WIP: composefs branch #1444

Uh oh!

cgwalters commented Jul 24, 2025

Uh oh!

gemini-code-assist bot left a comment

Uh oh!

gemini-code-assist bot Jul 24, 2025

Uh oh!

gemini-code-assist bot Jul 24, 2025

Uh oh!

gemini-code-assist bot Jul 24, 2025

Uh oh!

gemini-code-assist bot Jul 24, 2025

Uh oh!

gemini-code-assist bot Jul 24, 2025

Uh oh!

gemini-code-assist bot Jul 24, 2025

Uh oh!

gemini-code-assist bot Jul 24, 2025

Uh oh!

gemini-code-assist bot Jul 24, 2025

Uh oh!

cgwalters commented Aug 4, 2025 •

edited

Loading

Uh oh!

cgwalters commented Aug 5, 2025

Uh oh!

cgwalters commented Aug 7, 2025

Uh oh!

Uh oh!

-    assert!(all_configs.len() == 2);
+    anyhow::ensure!(
+        all_configs.len() == 2,
+        "Expected exactly 2 boot configurations for rollback, found {}",
+        all_configs.len()
+    );


		all_configs.sort_by(\|a, b\| if ascending { a.cmp(b) } else { b.cmp(a) });

		return Ok(all_configs);

	.context("Falied to write to .origin file")?;
	.context("Failed to write to .origin file")?;

WIP: composefs branch #1444

Are you sure you want to change the base?

WIP: composefs branch #1444

Uh oh!

Conversation

cgwalters commented Jul 24, 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 Jul 24, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Jul 24, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Jul 24, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Jul 24, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Jul 24, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Jul 24, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Jul 24, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Jul 24, 2025

Choose a reason for hiding this comment

Uh oh!

cgwalters commented Aug 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cgwalters commented Aug 5, 2025

Uh oh!

cgwalters commented Aug 7, 2025

Uh oh!

Uh oh!

cgwalters commented Aug 4, 2025 •

edited

Loading