Skip to content

aboot support followup - #2593

Open
alexlarsson wants to merge 4 commits into
bootc-dev:mainfrom
alexlarsson:aboot-followup
Open

alexlarsson wants to merge 4 commits into
bootc-dev:mainfrom
alexlarsson:aboot-followup

Conversation

@alexlarsson

Copy link
Copy Markdown
Contributor

There were some remaining comments in #2490 and these are handled in this PR.

Changes:

  • Check that deployed aboot images have aboot-deploy so we don't end up in s setup where we can't further update
  • Track the bootloader in the origin file so that we can correctly report this in the status
  • Use tracked bootloader to ensure that we don't update to an aboot image with the wrong bootloader
  • After failed update, allow later update/switch to replace the existing failed pending update.

@bootc-bot
bootc-bot Bot requested a review from henrywang October 27, 2026 15:28
@alexlarsson

Copy link
Copy Markdown
Contributor Author

@jmarrero moved the followup here

@github-actions github-actions Bot added the area/documentation Updates to the documentation label Oct 27, 2026
Without this, later updates will fail, so ensure we don't get into
this bad state.

Signed-off-by: Alexander Larsson <alexl@redhat.com>
Comment thread crates/lib/src/bootc_composefs/state.rs Outdated
.section(ORIGIN_KEY_BOOT)
.item(ORIGIN_KEY_BOOT_TYPE, boot_type);
.item(ORIGIN_KEY_BOOT_TYPE, boot_type)
.item(ORIGIN_KEY_BOOTLOADER, bootloader);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We've been refraining from storing the bootloader in the origin file. Is there a way to determine this at runtime as we do with other bootloaders?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Its a bit tricky, as it will have to encode some internal knowledge, like config files. But, I'll give it a go.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ok, I replaced this with reading /etc/aboot.cfg

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

hmm... this is also pretty weird. So the reason for us not storing bootloader info in origin file is we can easily figure it out at runtime, by reading efivars, which also helps us again some accidental file corruption. I think the origin file approach is much better than storing this in /etc. But, ideally we'd want to read some sort of system variable to figure it out

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We're already storing this in /etc though. That is how aboot-deploy knows how do do the update for the current system. This is just reading that data.

Retaining an attempted aboot deployment currently blocks further updates
and switches after a failed flash or firmware fallback. Allow an explicit
update or switch to discard it after reboot, preserving the selected
image source and refreshing status before checking deployment collisions.

Keep the same-boot guard and leave failed attempts disarmed across ordinary
reboots. Rollback can also discard an older failed attempt when a valid
rollback deployment remains available.

Document recovery and extend the unit and VM tests to cover failed
finalization, repeated reboots without reflashing, and explicit retry.

Validation: 320 library tests passed, with one ignored. The VM regression
test was extended but not run.

Assisted-by: AI
Signed-off-by: Alexander Larsson <alexl@redhat.com>
We're currently always showing bootloader None in the aboot entires,
this is not right for ukiboot hosts. Read BOOT_TYPE from the running
system's /etc/aboot.cfg instead, reporting ukiboot for ukiboot
configurations and none for other values.

Assisted-by: AI
Signed-off-by: Alexander Larsson <alexl@redhat.com>
Android aboot and ukiboot share the Aboot boot type but require different
bootloaders. Checking only the boot type allows updates across
incompatible layouts. Reuse the install validator against the running
host's detected bootloader before staging an update.

Assisted-by: AI
Signed-off-by: Alexander Larsson <alexl@redhat.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Changes recommended

An unchanged-spec shortcut can make bootc switch --apply report success without restaging or rebooting when retrying a failed mutable-tag upgrade.

1 open issue
What changed in this PR

This follow-up strengthens composefs Android boot support with deployment checks, bootloader reporting, and explicit recovery from failed updates.

Changes:

  • Require aboot-deploy in target images and validate bootloader compatibility.
  • Derive the host bootloader from aboot configuration for deployment status.
  • Allow failed updates to be discarded and retried after reboot, with documentation and test coverage.
File Description
tmt/​tests/​booted/​test-aboot-update-rollback.py Tests failed deployment retention, explicit retry, and rollback.
tmt/​tests/​booted/​aboot_testlib.py Checks bootloader identity and supports expected staged deployments.
docs/​src/​bootc-experimental-composefs-aboot.7.md Documents failed-update recovery and retry behavior.
crates/​lib/​src/​bootc_composefs/​update.rs Adds target validation and failed-update cleanup during upgrades.
crates/​lib/​src/​bootc_composefs/​switch.rs Clears failed aboot attempts before preparing switches.
crates/​lib/​src/​bootc_composefs/​status.rs Reports the configured host bootloader for aboot deployments.
crates/​lib/​src/​bootc_composefs/​boot.rs Requires deployment tooling during installation and exposes bootloader validation.
crates/​lib/​src/​bootc_composefs/​aboot.rs Adds bootloader detection, tooling checks, and failed-attempt cleanup with unit tests.

Review effort: Balanced

Comment on lines +52 to +54
if discard_failed_aboot_update(storage, &host)? {
host = get_composefs_status(storage, booted_cfs).await?;
}

@jmarrero-bot jmarrero-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Looks right overall; one bug to fix in the switch retry path (inline), and the PR body is stale: it says the bootloader is tracked in the origin file, but this revision reads the host's /etc/aboot.cfg instead. Commit 2's "VM regression test was extended but not run" line is also out of date now that CI ran it.

Tested at efaa310: cargo test -p bootc-lib --lib (336 passed) and just test-ukiboot on a Fedora 44 base (plan-61 and plan-62 pass, all five reboot stages of plan-62).

Test gaps worth covering: the switch retry, a target image without aboot-deploy being rejected (e.g. a v3 image built with rm /usr/bin/aboot-deploy), and the bootloader-change rejection on update (a unit test on a small helper is probably the realistic option without an Android image in CI).

Generated-by: https://github.com/jmarrero-bot#llms

delta.validate_image_reference(&target)?;
}

if discard_failed_aboot_update(storage, &host)? {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

After a failed upgrade of a mutable tag, bootc switch <same ref> discards the attempt here, the status reload puts spec.image back to the booted ref, and then it hits "Image specification is unchanged" and stages nothing (exit 0, and --apply doesn't reboot). Copilot raised this too (r4421862897).

Related: here and in update.rs (upgrade_composefs) the discard happens before the target is pulled or validated, so a typo'd ref or a registry outage throws away the attempt the docs say is kept for inspection, and the command fails anyway. Could the discard happen only once the target is resolved, and skip the unchanged-spec shortcut when something was discarded?

let image_root = Dir::reopen_dir(&composefs_mnt_fd)?;

if boot_type == BootType::Aboot {
super::aboot::require_aboot_deploy_in_image(&image_root)?;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Install picks the bootloader from the payload encoding (image_bootloader()), but status and the new update check use the host's /etc/aboot.cfg. If an image ships a UKI payload but its etc/aboot.cfg is missing or isn't BOOT_TYPE=ukiboot, install succeeds, status says none, and every later upgrade (even to the same image) fails with "aboot encoding Uki cannot be used with bootloader none". Same reasoning as commit 1: maybe run bootloader_from_config on the image's etc/aboot.cfg here and check it with validate_bootloader?

pub(crate) fn get_bootloader() -> Result<Bootloader> {
let config = match std::fs::read_to_string(ABOOT_CONFIG) {
Ok(config) => config,
Err(err) if err.kind() == std::io::ErrorKind::NotFound => return Ok(Bootloader::None),

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

aboot-deploy falls back to /boot/aboot.cfg when /etc/aboot.cfg is missing (resolve_aboot_config_path / LEGACY_ABOOT_CFG_PATH), so mirroring that would keep the two in agreement; at least a debug log would help. (The parser itself matches aboot_config_load().) Nit: aboot::get_bootloader and status::get_bootloader are both used in status.rs; host_bootloader would read better.

)?;

if boot_type == BootType::Aboot {
aboot::require_aboot_deploy_in_image(&mounted_fs)?;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This checks the target image, but finalize and rollback run the host's aboot-deploy. A host installed before this PR without it still records the attempt and fails late, and can now loop stage → fail → discard → retry. A cheap host-side check here would turn that into an early error.

Also, the mismatch error from validate_bootloader above doesn't say where "bootloader none" came from; something like "host BOOT_TYPE in /etc/aboot.cfg is X but the target payload is Y" would make a missing config easier to diagnose.

This branch has not been deployed

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

Labels

area/documentation Updates to the documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants