-
Notifications
You must be signed in to change notification settings - Fork 124
Composefs-native backend #1314
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
base: main
Are you sure you want to change the base?
Composefs-native backend #1314
Conversation
Done to facilitate the installation of a composefs repository to disk Signed-off-by: Pragyan Poudyal <[email protected]>
Signed-off-by: Pragyan Poudyal <[email protected]>
Signed-off-by: Pragyan Poudyal <[email protected]>
Signed-off-by: Pragyan Poudyal <[email protected]>
cd42079
to
b3d1952
Compare
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.
Thanks for working on this!!
lib/src/install.rs
Outdated
.create_dir_all("composefs") | ||
.context("Creating dir 'composefs'")?; | ||
|
||
tracing::warn!("STATE: {state:#?}"); |
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.
I'd recommend dbg!
for this type of stuff; it will build locally but not pass CI so it can't be accidentally merged.
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.
I intend to remove all these logs. They were only for debugging purposes
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.
Yep that makes sense, what I meant is that use dbg!
locally. (Or use tracing::debug!
and keep the tracing in there and just use the tracing selector to ensure you see what you want...we are really chatty with a big env RUST_LOG=debug
unfortunately today, mostly in container fetches)
@@ -1199,7 +1199,7 @@ async fn run_from_opt(opt: Opt) -> Result<()> { | |||
let fd = | |||
std::fs::File::open(&path).with_context(|| format!("Reading {path}"))?; | |||
let digest: fsverity::Sha256HashValue = fsverity::measure_verity(&fd)?; | |||
let digest = hex::encode(digest); | |||
let digest = digest.to_hex(); |
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.
I think this should be squashed with the previous commit, and it may make sense to have a "bump composefs-rs" prep PR split out right?
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.
Right, that makes sense. I'll put this in a separate PR
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.
Done in #1403
lib/src/install.rs
Outdated
async fn install_to_filesystem_impl( | ||
state: &State, | ||
rootfs: &mut RootSetup, | ||
cleanup: Cleanup, | ||
composefs: bool, |
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.
I think this would make sense as part of state
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.
I was conflicted on where to put this. Thanks for the review. I'll add this to State
&imgstore, | ||
) | ||
.await?; | ||
tracing::warn!( |
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.
Same comment re dbg!
or this one could also make sense as a tracing::debug!
that gets left in right?
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.
Ah, yes. We can keep this one as debug
The branding here is a bit confusing because of course, bootc already uses composefs. Just the https://github.com/composefs/composefs/ version, and that is "indirectly" via ostree. Bigger picture I think what we're trying to aim for here primarily is "sealed" images right? In that case then, one thing I think would make sense is that we don't even have an option to install - we determine what to do by inspecting the container. It does of course make sense to eventually support unsealed images this way too...which will be extremely important for eventually migrating existing ostree systems. There's an enormous set of details there though. So my proposal in a nutshell:
|
I think this works. Or we could have
Curious about this. Is there a reason we'd want composfs native storage in this case? |
Isn't the whole idea that we're aiming for the sealed case? |
Signed-off-by: Pragyan Poudyal <[email protected]>
Signed-off-by: Pragyan Poudyal <[email protected]>
I'll be getting back to this PR next Monday. Been working on a separate issue this week... |
lib/src/install/baseline.rs
Outdated
@@ -403,6 +409,8 @@ pub(crate) fn install_create_rootfs( | |||
"root", | |||
opts.wipe, | |||
mkfs_options.iter().copied(), | |||
// TODO: Add cli option for this | |||
Some(uuid::uuid!("6523f8ae-3eb1-4e2a-a05a-18b695ae656f")), |
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.
It looks like you chose the alpha architecture uuid; it'd make sense potentially to use the DPS UUIDs by default for to-disk
but that will require some work.
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.
Signed-off-by: Pragyan Poudyal <[email protected]>
Signed-off-by: Pragyan Poudyal <[email protected]>
Signed-off-by: Pragyan Poudyal <[email protected]>
Signed-off-by: Pragyan Poudyal <[email protected]>
Signed-off-by: Pragyan Poudyal <[email protected]>
5979f04
to
42632ce
Compare
Also exit early if kargs are passed for `--boot=uki` Signed-off-by: Pragyan Poudyal <[email protected]>
Instead of putting the UKI in `/boot` in the root partition, this commit moves it to the ESP. Update grub user.cfg and pass the disk UUID to `--fs-uuid` Signed-off-by: Pragyan Poudyal <[email protected]>
This is currently WIP, as it only shows the currently booted image in use and not any staged or rollback deployments. Signed-off-by: Pragyan Poudyal <[email protected]>
Create a .origin file in /sysroot/state to store image info Signed-off-by: Pragyan Poudyal <[email protected]>
lib/src/install.rs
Outdated
let esp_devices = [COREOS_ESP_PART_LABEL, ANACONDA_ESP_PART_LABEL] | ||
.into_iter() | ||
.map(|p| Path::new("/dev/disk/by-partlabel/").join(p)); |
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.
Two things
- We shouldn't use
/dev/disk/by-partlabel
because that's global state, in the general case we may be creating partitions on a loopback mounted device and don't want to touch any global state - There's a well known UUID for the ESP to use
Combining these, see https://github.com/coreos/bootupd/blob/92a8fe2326e35d893afbe464857408c403d9e7ea/src/blockdev.rs#L41 which perhaps we could move into this codebase and have bootupd use it (because bootupd already uses bootc).
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.
Ah, okay I'll update to not use by-partlabel
perhaps we could move into this codebase and have bootupd use it (because bootupd already uses bootc)
I found this little bit weird, as I feel like this should be the other way around. Is there a reason why bootc uses the bootupctl
command rather than the bootup
crate?
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.
Is there a reason why bootc uses the bootupctl command rather than the bootup crate?
Well...unless we took over that CLI, that'd mean we duplicate the code.
lib/src/install.rs
Outdated
create_dir_all(&boot_dir).context("Failed to create boot dir")?; | ||
|
||
// Add the user grug cfg | ||
let grub_user_config = format!( |
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.
In my ideal world grub related things live in bootupd, not in this project.
And even better if we can make this a static configuration...where grub basically implements the BLS spec and finds things in /EFI/Linux
on its own right?
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.
It makes sense to put this in bootupd, but upon discussions with @travier we're still debating on whether we want to push systemd-boot or not, so this is more or less temporary
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.
This part of the GRUB config can not be generated by bootup as it is the deployment boot order in the UKI + GRUB case. See: containers/composefs-rs#38 (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.
It's not possible to use the BLS spec partial support in GRUB to boot UKI. It does not support the key that we need to do so, thus why we generate GRUB config snippets: ostreedev/ostree#2753 (comment)
Also create `/var` inside `/state/deploy/<image_id>` as a symlink to `../../fedora/os/var` which is to be the case for every deployment Signed-off-by: Pragyan Poudyal <[email protected]>
Signed-off-by: Pragyan Poudyal <[email protected]>
* Implement `bootc upgrade` command for a composefs-native booted system * Refactor UKI and BLS boot entry writing functions to work for upgrade * Prevent using global `/dev/disk/by-partuuid` to get UUID for ESP * Refactor out code for populating `/sysroot/state` Signed-off-by: Pragyan Poudyal <[email protected]>
Store the staged deployment at `/run/composefs/staged-deployment` similar to how it's done in ostree Signed-off-by: Pragyan Poudyal <[email protected]>
On upgrade/switch write UKI and BLS entries with the suffix .staged This should be atomically renamed by the equivalent of `ostree-finalize.service` for composefs. In case of grub menuentries for UKI case, rewrite the entire file by reading /sysroot/state/deploy/<deployment_id> Signed-off-by: Pragyan Poudyal <[email protected]>
This does not yet "apply" the switch, it only stages a deployment Also refactor pulling of a composefs repository to a separate function Signed-off-by: Pragyan Poudyal <[email protected]>
Fixes: #1190
A rough draft for installing and booting using
composefs-rs
. Current only works forinstall to-disk
viabootc install to-disk --composefs [options]
Issues:
continers-storage
, viz, #117. Needs some ironing out.composfs-rs
components, namelycomposfs-setup-root
I have a few logs being logged as warning, just for visibility. Will remove them when it's ready.
Same with
composefs-rs
patch incargo.toml