Skip to content

Conversation

@Johan-Liebert1
Copy link
Collaborator

@Johan-Liebert1 Johan-Liebert1 commented Jan 15, 2026

Most of this is in prep for #1913

  1. Add option to reset soft reboot state

  2. Don't soft-reboot automatically
    Aligning with ostree API, now we only initiate soft-reboot if --apply
    is passed to bootc update, bootc switch, else we only prepare the
    soft reboot

  3. Update image digest query format
    After bootc/commit/49d753f996747a9b1f531abf35ba4e207cf4f020,
    composefs-rs saves config in the format oci-config-sha256:.
    Update to match the same

  4. composefs/update: Handle --download-only flag
    When --download-only is passed, only download the image into the
    composefs repository but don't finalize it.
    Conver the /run/composefs/staged-deployment to a JSON file and Add a
    finalization_locked field depending upon which the finalize service will
    either finalize the staged deployment or leave it as is for garbage
    collection (even though GC is not fully implemented right now).

Some commits are split from #1913

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces several enhancements to composefs functionality. It adds an option to reset the soft reboot state and modifies the soft reboot logic to align with the ostree API, where a soft reboot is only initiated if --apply is passed. Additionally, it updates the image digest query format to match recent changes in composefs-rs. The changes are well-structured and improve the consistency and usability of the soft reboot feature. My review includes a suggestion to refactor a part of the new reset_soft_reboot function to improve code clarity and maintainability.

@Johan-Liebert1 Johan-Liebert1 force-pushed the cfs-changes branch 2 times, most recently from 0193cf9 to 364e600 Compare January 19, 2026 09:25
@Johan-Liebert1
Copy link
Collaborator Author

Setting up boot components seem to be failing in centos9, f42 with

Bootloader: grub
Installing bootloader via bootupd
error: boot data installation failed: installing component EFI: No such file or directory (os error 2)
error: Installing to disk: Installing bootloader: Failed to run command: Command {
    program: "bootupctl",
    args: [
        "bootupctl",
        "backend",
        "install",
        "--write-uuid",
        "--src-root",
        "/run/bootc/mounts/rootfs/ostree/deploy/default/deploy/3c393b017a7d9c64b3cab4c74372e0226024e26c15983479c3e70455ba0ac62a.0",
        "--device",
        "/dev/loop1",
        "/run/bootc/mounts/rootfs",
    ],
    create_pidfd: false,
}

@Johan-Liebert1
Copy link
Collaborator Author

I guess #1926 is related

const NEXTROOT: &str = "/run/nextroot";

#[context("Resetting soft reboot state")]
fn reset_soft_reboot() -> Result<()> {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think functions like this should accept their global state, like Storage.

Copy link
Collaborator Author

@Johan-Liebert1 Johan-Liebert1 Jan 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feels a bit weird to have an argument when they don't use it. We could use run from the Storage, but we need to bind mount /run from host any to unmount nextroot, unless I'm missing something here


let Some(nextroot) = nextroot else {
tracing::debug!("Nextroot is not a directory");
println!("No deployment staged for soft rebooting");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally we share these output messages between the two backends

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be great, but afaik the implementation for soft reboot for ostree backend is in the ostree repo and not bootc. In the ostree path, we don't print anything if there is no deployment staged for soft rebooting. I have updated the final messages, i.e. soft reboot prep complete and soft reboot state reset messages, to match the ostree ones


if opts.soft_reboot.is_some() {
prepare_soft_reboot_composefs(storage, booted_cfs, &id.to_hex(), true).await?;
prepare_soft_reboot_composefs(storage, booted_cfs, Some(&id.to_hex()), true, false).await?;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having more than one boolean value to functions I think leads to confusion as to what they mean. I think this can become an enum since they're mutually exclusive no?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is from an older commit? I had changed this to an enum

Signed-off-by: Pragyan Poudyal <pragyanpoudyal41999@gmail.com>
Aligning with ostree API, now we only initiate soft-reboot if `--apply`
is passed to `bootc update`, `bootc switch`, else we only prepare the
soft reboot

Signed-off-by: Pragyan Poudyal <pragyanpoudyal41999@gmail.com>
After bootc/commit/49d753f996747a9b1f531abf35ba4e207cf4f020,
composefs-rs saves config in the format `oci-config-sha256:`.

Update to match the same

Signed-off-by: Pragyan Poudyal <pragyanpoudyal41999@gmail.com>
When `--download-only` is passed, only download the image into the
composefs repository but don't finalize it.

Conver the /run/composefs/staged-deployment to a JSON file and Add a
finalization_locked field depending upon which the finalize service will
either finalize the staged deployment or leave it as is for garbage
collection (even though GC is not fully implemented right now).

Signed-off-by: Pragyan Poudyal <pragyanpoudyal41999@gmail.com>
@cgwalters cgwalters self-assigned this Jan 21, 2026
@jeckersb jeckersb self-assigned this Jan 21, 2026
Comment on lines +53 to +54
// We want the digest in the form of "sha256:abc123"
let config_digest = format!("{}", imginfo.manifest.config().digest());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm isn't this us losing type safety in that basically composefs-rs is using a different type alias for digests than exists in https://docs.rs/oci-spec/latest/oci_spec/image/struct.Digest.html ?

I think we could probably impl From<oci_spec::Digest> for composefs_rs::Sha256Digest or so?

Having to go through strings is definitely a big trap.

#[context("Resetting soft reboot state")]
pub(crate) fn reset_soft_reboot() -> Result<()> {
let run = Utf8Path::new("/run");
bind_mount_from_pidns(PID1, &run, &run, true).context("Bind mounting /run")?;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is necessary, we only need to do that when run under podman.

.context("Opening nextroot")?;

let Some(nextroot) = nextroot else {
tracing::debug!("Nextroot is not a directory");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't exist, not just not a directory

return Ok(());
}

unmount(NEXTROOT, UnmountFlags::DETACH).context("Unmounting nextroot")?;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this work though? We always enter a mount namespace, so this is where...ohhhh, I see that's why you did the bind mount of /run from the host mountns above.

And indeed I think we have similar code in ostree.

But...hmmm I think it'd be cleaner to avoid mutating global state this way - we could fork a child process that does the umount (look at similar code in bind_mount_from_pidns) or we could always pull in the real /run right after we do the re-exec to enter a new mountns.

storage: &Storage,
booted_cfs: &BootedComposefs,
deployment_id: &String,
deployment_id: Option<&String>,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prefer Option<&str>

}

#[derive(Debug, Serialize, Deserialize)]
pub(crate) struct StagedDeployment {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one seems like it could use a comment

.atomic_replace_with(
COMPOSEFS_STAGED_DEPLOYMENT_FNAME,
|f| -> std::io::Result<()> {
f.write_all(new_staged.to_canon_json_string()?.as_bytes())
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't matter here but we can optimize this by not serializing to an intermediate string by using the canon-json-rs formatter with serde_json::to_writer

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants