Repository navigation
UKI Addons Support - #2448
UKI Addons Support#2448Johan-Liebert1 wants to merge 23 commits into
Conversation
2579e09 to
e31ce45
Compare
|
Can we install UKI addons as part of a deployment with |
| pub(crate) enum UkiAddonCliOpts { | ||
| /// List all installed UKI Addons | ||
| List { | ||
| /// Output in JSON format |
That's in the docs here right? |
|
Or well actually, I think what you may be asking about is external UKI addons (i.e. not included in the container image), in which case it would totally make sense to do. |
Yea I wasn't super clear but that was what I was thinking about 🙂 |
3422e57 to
4b63be2
Compare
cgwalters
left a comment
There was a problem hiding this comment.
How about bootc install --included-uki-addon foo --external-uki-addon /path/to/external.efi i.e. we always disambiguate between internal and external?
I don't understand what we mean by external here? I'm guessing it doesn't mean "global" addons |
|
external ➡️ #2448 (comment) |
bcafaf3 to
77db443
Compare
b3b9d76 to
3ca9715
Compare
c11883b to
ec66b3d
Compare
Prefix global addon filenames with the bootc identifier in the ESP so we can distinguish bootc-managed global addons from third-party ones Signed-off-by: Pragyan Poudyal <pragyanpoudyal41999@gmail.com>
Add `bootc uki-addon` subcommand with three operations: - `bootc uki-addon list`: List installed UKI addons Supports `--json` for JSON output - `bootc uki-addon add <name> <global|scoped>`: Install an addon from the booted image onto the ESP - `bootc uki-addon remove <name> [deployment_id]`: Remove an addon Add Display and Serialize to UkiAddonType/UkiAddonsList Signed-off-by: Pragyan Poudyal <pragyanpoudyal41999@gmail.com>
- Add GC tests for Addons - Add CLI tests Signed-off-by: Pragyan Poudyal <pragyanpoudyal41999@gmail.com>
Mark UKI Addons as experimental Assisted-by: Claude-Code (Opus) Signed-off-by: Pragyan Poudyal <pragyanpoudyal41999@gmail.com>
UkiAddonOpts is now flattened into both upgrade and switch commands, which don't have a --composefs-backend flag. The `requires = "composefs_backend"` constraint causes a panic at clap validation time because the referenced argument doesn't exist in those command contexts. This is generally safe as the options are ignored for ostree installs anyway Signed-off-by: Pragyan Poudyal <pragyanpoudyal41999@gmail.com>
Signed-off-by: Pragyan Poudyal <pragyanpoudyal41999@gmail.com>
Extract cmdline parsing from write_pe_to_esp into parse_uki_cmdline so it runs for both the UKI and UKI addons. The `composefs=` parameter can now be found in the main UKI or a scoped addon - At most one composefs= cmdline across all PE binaries (UKI + addons). A second one is rejected even if the digest matches. - Global UKI addons must never contain `composefs=` cmdline - At least one `composefs=` cmdline must be found or the install fails Signed-off-by: Pragyan Poudyal <pragyanpoudyal41999@gmail.com>
Before removing a scoped addon, parse its PE binary and check for a composefs= kernel parameter. If found, early exit as removing that addon would make the system unbootable Global addons are not checked because `composefs=` is rejected at install time for global addons Signed-off-by: Pragyan Poudyal <pragyanpoudyal41999@gmail.com>
Test that bootc rejects composefs= in the wrong places - Build a global addon containing composefs= from `bootc compute-composefs-digest`, attempt switch with --global-uki-addon, assert failure - Build a scoped addon containing composefs= alongside the UKI (which already has it), attempt switch with --uki-addon, assert failure due to duplicate composefs= It's a shame that we can't test UKI Addon only cmdline without piling on a bunch of hacks since `bootc container ukify` unconditionally puts the cmdline inside the UKI Signed-off-by: Pragyan Poudyal <pragyanpoudyal41999@gmail.com>
This option lists all the UKI Addons across all deployments. Also, it associates global addons with the deployments that reference them. It is useful for GC-ing global UKI Addons if no deployments refer to them Signed-off-by: Pragyan Poudyal <pragyanpoudyal41999@gmail.com>
Similar to how we GC UKIs and scoped UKI Addons, GC Global UKI addons if we have no EROFS images remaining that hold a reference to them, which means that the deployments that depended on the Global addons have been removed Signed-off-by: Pragyan Poudyal <pragyanpoudyal41999@gmail.com>
Reject an addon whose cmdline embeds a `composefs=` or `composefs.digest=` argument before copying it. An addon added this way must not be able to override the composefs digest. Write the addon with `atomic_replace_with` and fsync the destination directory, so a crash mid add can't leave a truncated PE on the ESP. Add a tmt test to verify. Signed-off-by: Pragyan Poudyal <pragyanpoudyal41999@gmail.com>
`--uki-addon` was a no-op on ostree backend, now we bail if it's passed during ostree installs/upgrades Also, bail if we fail to find an addon in the image instead of just logging as info Update manpage links Signed-off-by: Pragyan Poudyal <pragyanpoudyal41999@gmail.com>
We get referenced global UKI Addons by mounting non-bootable EROFS images and collecting all the addons referenced. This is a pretty expensive operation, so now we only do that if: - The system is booted via UKI, and - Global Addons are actually installed Also, update docs on how scoped and global addons are updated Signed-off-by: Pragyan Poudyal <pragyanpoudyal41999@gmail.com>
If we have a staged deployment present, then `uki-addon add/remove` should act on the staged deployment as users usually will expect the addon to take effect after reboot. If we add it to the booted deployment while we have a staged deployment, users won't get the addon after reboot as we'd reboot into the staged deployment Signed-off-by: Pragyan Poudyal <pragyanpoudyal41999@gmail.com>
d5b75aa to
f356441
Compare
|
@jmarrero Addressed all of the comments, except for #2448 (comment) We can do that as a followup as this PR's already huge? |
f356441 to
1381cf0
Compare
Create a staged directory for global uki addons `loader/addons.stage` in the ESP which we will atomically rename-exchange with the the actual addons directory `loader/addons` `uki-addon add`/`remove` already act on a staged deployment for scoped addons. Extend the same behaviour to global addons. When a deployment is staged, add/remove operate on the staged set at `loader/addons.staged` instead of the live directory. Document the staged global-addon removal behaviour in the man page and the composefs overview, and add a tmt tests. Signed-off-by: Pragyan Poudyal <pragyanpoudyal41999@gmail.com>
Global UKI addons are removed once no deployment's image references them, but that removal happened silently and only after the addon was already orphaned, too late for the user to act. During `bootc upgrade/switch`, warn about installed global addons that the newly staged image no longer provides, i.e. they'll be GC'd in the next upgrade/switch. Extend the UKI GC test to assert the notice fires when switching to an image that drops a global addon, and does not fire when the image still provides it. Tests are written by LLM Signed-off-by: Pragyan Poudyal <pragyanpoudyal41999@gmail.com>
1381cf0 to
354977f
Compare
jmarrero-bot
left a comment
There was a problem hiding this comment.
I tested 354977f in VMs (composefs, systemd-boot, xfs, unsealed UKI, the CI variant), and three of the staged global addon issues reproduce, so this needs changes first. The inline comments have the details:
uki-addon add <name> globalwhile a deployment is staged makesloader/addons.stagedhold only that addon. Finalize then deletes every other live global addon, including ones bootc doesn't manage.loader/addons.stagedis never cleared (not on re-staging, rollback or delete), so the global addons of an abandoned staged deployment get applied to the next deployment.- GC counts staged globals but deletes them from
loader/addons. Deleting a staged deployment that added a global fails with ENOENT, and GC keeps failing after that.
Should fix (details in the gist): a live global can't be removed while a deployment is staged; the advice in the pending-removal notice can't work; no-op upgrade/switch silently ignore the addon flags; the remove man pages document arguments clap doesn't take; leftovers in boot.rs; and the tmt remove nonexistent-addon check passes on a clap usage error.
Repro steps, output, and tmt tests for the three blocking issues: https://gist.github.com/jmarrero-bot/2191254aacdd3bc3c5c517b1a0c9ea1d. Tested on a 16-core RHEL 10 runner with KVM. The PR's plan-51 passes. The repros ran in bcvk VMs with real reboots and as tmt tests, and all three fail at the expected assertion. The leftovers and nits come from reading the code only.
Generated-by: https://github.com/jmarrero-bot#llms
- Make sure to remove the staged global addons dir when a staged deployment is deleted or before restaging a deployment - Make sure to delete the staged global addons dir when rolling back a deployment similar to what we do with BLS entries - Fix boot aftifact GC by handling a case where we could have the same UKI addons present more than once in `/boot` Signed-off-by: Pragyan Poudyal <pragyanpoudyal41999@gmail.com>
jmarrero-bot
left a comment
There was a problem hiding this comment.
Not ready yet, but close. I re-ran our tests on 4942382 in VMs (same CI variant). B1 and B3 as reported are fixed, and so is B2 in the scenario I reported, but B2 still reproduces in two nearby cases and the GC part of B3 now fails differently. Each fix is a line or two (inline):
- With no live
loader/addons, staging returns before clearing a staleaddons.staged, so an abandoned deployment's global still lands on the next one. - The carry-forward set still counts staged globals.
- Seeding the staged dir makes GC see every live global twice while something is staged, so
delete-deploymentof the rollback fails with ENOENT.
Also, re-staging now silently undoes a remove ... global made while staged. S2 to S5 and N1 from the first review are unchanged, and deferring the status listing is fine. plan-51 passes. Details, VM logs and tmt tests are in the gist: https://gist.github.com/jmarrero-bot/2191254aacdd3bc3c5c517b1a0c9ea1d#file-1-rereview-2448-49423828-md
Generated-by: https://github.com/jmarrero-bot#llms
| /// an upgrade/switch | ||
| #[context("Staging Global UKI Addons")] | ||
| fn stage_global_uki_addons(esp_fd: &Dir) -> Result<()> { | ||
| let Some(global_addons_dir) = esp_fd.open_dir_optional(GLOBAL_UKI_ADDONS_DIR)? else { |
There was a problem hiding this comment.
This returns before the new remove_dir_all below, so a stale addons.staged survives re-staging whenever no global addon is live yet.
Reproduced: boot A (ships x, none enabled), switch --global-uki-addon x to B, then switch to C (no globals) and reboot. x is active in C. Clearing addons.staged unconditionally first (remove_all_optional), before this early return, fixes it.
| // if we find in the new image | ||
| // | ||
| // Only keep addons referenced by the current deployment | ||
| let installed_addons = list_installed_uki_addons(storage, &host.status.staged)? |
There was a problem hiding this comment.
This still includes addons.staged, and it runs before stage_global_uki_addons wipes that dir, so a global staged for an abandoned deployment gets re-selected for the next one if that image ships it too.
Reproduced: boot A (ships foo and x, only foo enabled), stage B with --global-uki-addon x, then switch to C (also ships both, no flags) and reboot. x is active in C. Filtering !addon.staged here fixes it.
| ) -> Result<()> { | ||
| // GC always runs *after* finalizing deployments so there would be no staged deployment | ||
| // by definition | ||
| let currently_installed_addons = list_installed_uki_addons(storage, &None)?; |
There was a problem hiding this comment.
Now that staging always seeds addons.staged from the live dir, this lists every live global twice while a deployment is staged. An unreferenced one is then removed from loader/addons twice, and the second removal fails with ENOENT. The delete.rs change only covers deleting the staged deployment.
Reproduced: booted B, rollback A (ships the live global foo), stage C, bootc delete-deployment <A> fails with GC Global UKI Addons: Removing global addon bootc_composefs-foo.addon.efi: No such file or directory, after A's entry is already gone. bootc internals composefs-gc then fails the same way until the next finalize. Please only consider staged == false here, as the comment above assumes, and treat ENOENT as already removed.
|
|
||
| assert ("/var/tmp/efi/loader/addons.staged/bootc_composefs-global-cmdline.addon.efi" | path exists) "Global addon should be staged under loader/addons.staged" | ||
| # This is the first global addon, so the live dir must not hold it yet | ||
| assert (not ("/var/tmp/efi/loader/addonsbootc_composefs-global-cmdline.addon.efi" | path exists)) "Live addons must not be updated before reboot" |
There was a problem hiding this comment.
Nit: missing / after addons, so this assertion can never fail.
uki-addon: Update addons on update/switch
Introduce a function to gather all currently installed addons, scoped
and global. On upgrade/switch, gather all installed addons and if an
addon with the same name is found in the upgrade image, update that
particular addon automatically
uki/addon: Support global addons
We were partially supporting global addons, but they were lumped in with
scoped/local addons. Add a new cli option to composefs installs called
--global-uki-addonwhich would determine which global addon toinstall.
cfs/upgrade/switch: Handle UKI Addons
Here is what we do now with UKI Addons
Accept
--uki-addonand--global-uki-addoncli options for bootcswitch/upgrade commands
If we find an installed addon with the same name as the one in the new
image, we update it
global-uki-addons: Add prefix to name
Prefix global addon filenames with the bootc identifier in the ESP so
we can distinguish bootc-managed global addons from third-party ones
uki-addon: Add CLI for managing UKI Addons
Add
bootc uki-addonsubcommand with three operations:bootc uki-addon list: List installed UKI addonsSupports
--jsonfor JSON outputbootc uki-addon add <name> <global|scoped>: Install an addon fromthe booted image onto the ESP
bootc uki-addon remove <name> [deployment_id]: Remove an addonAdd Display and Serialize to UkiAddonType/UkiAddonsList