-
Notifications
You must be signed in to change notification settings - Fork 149
Various composefs work #1662
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Various composefs work #1662
Conversation
There was a problem hiding this 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 a significant amount of work related to composefs, including switching the fs-verity hashing algorithm from SHA256 to SHA512, adding a new workflow for building 'sealed' images with Unified Kernel Images (UKIs), and making the composefs backend a default feature. The changes are mostly consistent and well-structured, with good use of type aliases to improve code clarity.
However, I've found a few issues that need attention:
- There's some dead and potentially buggy code in the new
xtaskfor building sealed images. - A new Dockerfile contains a leftover command that should be removed.
- The documentation for newly added CLI options and subcommands is incomplete.
Please see my detailed comments for suggestions on how to address these points.
| **--composefs-native** | ||
|
|
||
|
|
||
|
|
||
| **--insecure** | ||
|
|
||
|
|
||
|
|
||
| Default: false | ||
|
|
||
| **--bootloader**=*BOOTLOADER* | ||
|
|
||
|
|
||
|
|
||
| Default: grub |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| | **bootc usr-overlay** | Add a transient writable overlayfs on `/usr` | | ||
| | **bootc install** | Install the running container to a target | | ||
| | **bootc container** | Operations which can be executed as part of a container build | | ||
| | **bootc composefs-finalize-staged** | | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
Split prep work for this to #1665 |
837dc44 to
cccc2f8
Compare
|
@jeckersb I ended up reworking this one to use the bootc-in-container for the sealing side because it would be too annoying in our CI to have it be on the host (which is ubuntu here). |
|
I'm guessing this is something to do with me running |
|
Hmm I don't know what's up with that error. Offhand it's perhaps something to do with the source bind path when it's relative? Does it work if you make it an absolute path? What's your wrapper for |
Nope still doesn't work with absolute paths I don't have any wrapper for and then... I suspect there's just something weird going on with passing secrets across via podman-remote from inside of the toolbox. I'll poke at it more tomorrow. Given the eyeball test the changes look good to me. From CI it looks like it needs a |
Testing a stripped-down case this morning, it works fine for me to pass secrets inside of the toolbox over podman-remote to the host. I've also verified that if I run the original podman command directly on the host, it works properly. So I'm thinking now that it has something to do with passing secrets over podman-remote in combination with multistage builds. |
Ahhhhhh it's this - containers/podman#25314 I'll put in a separate PR to add that silly workaround to our .dockerignore |
3dbc6dd to
ee05675
Compare
|
OK it took me a bit too long to figure out the reason that we were getting This will get naturally fixed when we move to using bcvk here. However...for now I moved the outer portion of the sealing back to shell script - we've already moved some of the inner portion into the Rust code. |
ee05675 to
a58d0a3
Compare
|
OK though, unfortunately right now tmt's virtual provisioner doesn't support uefi. That looks pretty easy to fix, but I'm a bit tempted to try out having |
bb56008 to
b70bee9
Compare
This ensures we're SHA-512 across the board. Signed-off-by: Colin Walters <[email protected]>
- Use bash strict mode more consistently - Drop the error redirections which can mask problems as recommended by AI Signed-off-by: Colin Walters <[email protected]>
f500118 to
39fffbd
Compare
Yeah I think so, filed teemtee/tmt#4203 That said, we can work around this by using bcvk to provision a system external to tmt. That's not hard, but the downside is that it's logic that'd need to be replicated into anything else that wants to use tmt. |
39fffbd to
1c4b1d9
Compare
|
This looks good testing it out on my end, looks like CI is failing because |
1c4b1d9 to
c5c0137
Compare
Yeah I'd messed up the bootupd detection, fixed now |
jeckersb
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍, assuming the in-flight jobs pass just needs trivial validation cleanup
- Change the install logic to detect UKIs and automatically enable composefs - Change the install logic to detect absence of bootupd and default to installing systemd-boot - Move sealing bits to the toplevel - Add Justfile entrypoints - Add basic end-to-end CI coverage (install + run) using our integration tests - Change lints to ignore `/boot/EFI` Signed-off-by: Colin Walters <[email protected]>
c5c0137 to
77685f0
Compare
|
Holy 🐮 so I removed the background Of course, us having to clean up tons of garbage from the stock runners is ridiculous and hopefully GH will implement #1669 But while let's not kill CI for this one, I'm going to try to go back to optimizing the provisioner... |
|
OK no, it's just 2/4 jobs here that had some kind of pathological slowness going on, but we don't have enough timing information to have a good idea of what specifically that was. Working on a followup commit. |
|
There was some chat (internal) that this might have broken Anaconda bootc installs. If so I have a theory why: We might not be correctly detecting bootupd presence in the case where we're doing a to-filesystem install outside of a container. We have CI that covers that this installs, but not that it boots. Well, glancing at the CI job for this https://github.com/bootc-dev/bootc/actions/runs/18566247913/job/52928051266 I do see bootupd being run. Are the target systems here being booted via UEFI or bios? Can you link to a failing CI job? Is this only happening in rawhide with the bootc COPR? Does it also happen on c10s for example? And this doesn't reproduce with the ostreecontainer flow? |
|
Hi @cgwalters , thanks for checking this out. So I was testing images published by bootc and the last one working fine was: https://download.copr.fedorainfracloud.org/results/rhcontainerbot/bootc/fedora-rawhide-x86_64/09691020-bootc/ I saw your changes in this commit and I forced Anaconda to have all binaries bootc is looking for installed: The result is that after |
|
This is the log from the manual install process triggered in the Anaconda runtime (fails to boot): |
|
I found some of my old install logs and the only difference I can see is: But this is something |
|
Oh yes that's the regression, looking |
|
It's odd, I do see that output in our CI job here. What's the reproducer for this again? Does it need rhinstaller/anaconda#6298 ? |
Yes, I am running this on top of the ISO produced out of this PR. I just did a small test and manually amended And now deployed OS was able to boot. |
|
To rebuild an ISO I am using this script: Then on top of that I am updating the ISO with: I am not sure which version of the bootc Lorax is using (it is part of the toolchain to rebuild the ISO). |
|
To run the ISO I am using qemu: Now I started to wondering if this is not some Bios vs UEFI related story with |
|
@cgwalters Simon de Vlieger tracked down the important missing piece as blscfg. What do you think? Is this something that anaconda is doing wrong or is this a regression in bootc code that we just need to wait for a fix for? (This is the only thing left before we can merge the PR implementing the bootc kicakstart command). |
|
It must be a bootc regression. I will look at this tomorrow. |
|
To explicitly close the loop here, I couldn't reproduce this failure except when using a really old image. Please ping if that's incorrect. |
|
Thanks for checking this @cgwalters . I can confirm that with the latest images it works fine (https://quay.io/repository/fedora/fedora-bootc?tab=tags&tag=rawhide) and the grub config is consistent. |
No description provided.