Conversation
We need more space than ostree as we have UKIs and UKI addons We might also need to store UKIs for pinned deployments Signed-off-by: Pragyan Poudyal <pragyanpoudyal41999@gmail.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces several enhancements to composefs functionality. Key changes include increasing the EFI System Partition (ESP) size to 1GB for composefs installations, which is a sensible change to accommodate UKIs and their addons. The upgrade command has been improved to handle cases where an update is already staged or a different image has been staged via switch. Additionally, a new --check option is implemented for upgrade to preview changes. Finally, the system now extracts the OS version from the UKI for better display in boot menus.
The implementation is solid, but I've identified a potential panic in the update logic due to an incorrect bounds check and a small opportunity for code simplification. My review includes suggestions to address these points.
Handle the case when an update is already staged. Handle the case when the staged image is not the same as the update image. Implement `--check` option Signed-off-by: Pragyan Poudyal <pragyanpoudyal41999@gmail.com>
Johan-Liebert1
force-pushed
the
various-composefs-changes
branch
from
25e057d to
fea6681
Compare
Signed-off-by: Pragyan Poudyal <pragyanpoudyal41999@gmail.com>
Johan-Liebert1
force-pushed
the
various-composefs-changes
branch
from
fea6681 to
25a26f3
Compare
cgwalters
left a comment
cgwalters
left a comment
There was a problem hiding this comment.
Thanks! Nothing blocking, but some things to maybe look at in followups.
| } | ||
|
|
||
| boot_label = Some(uki::get_boot_label(&efi_bin).context("Getting UKI boot label")?); | ||
| let osrel = uki::get_text_section(&efi_bin, ".osrel") |
There was a problem hiding this comment.
Yes...though we should also have exactly this same data in /usr/lib/os-release in the target image right? My inclination would be to use that over parsing the UKI, but in the end it doesn't really matter.
There was a problem hiding this comment.
Yes, I think we could've gone either way as the data from /usr/lib/os-release will be embedded in the UKI anyway
|
|
||
| #[context("Getting SHA256 Digest for {id}")] | ||
| pub fn str_to_sha256digest(id: &str) -> Result<Sha256Digest> { | ||
| let id = id.strip_prefix("sha256:").unwrap_or(id); |
There was a problem hiding this comment.
Hmm it feels like being "loose" in our inputs is not a good idea; we should either strictly require a digest prefix or not.
There was a problem hiding this comment.
I see. This was just me being careful. I think the layer ids always start with sha256:
| // We already have this container config. No update available | ||
| if img_pulled { | ||
| println!("No changes in: {imgref:#}"); | ||
| // TODO(Johan-Liebert1): What if we have the config but we failed the previous update in the middle? |
There was a problem hiding this comment.
Yes, I think we need to only write the config object after all of its dependent layers.
There was a problem hiding this comment.
That's what we do. This comment was for the cases where we pull the entire image and create an erofs image, but fail to create some files in the state dir, or maybe something else. The bootloader entries will be overwritten, but we will fail the update if the state directory already exists.
| } | ||
|
|
||
| if opts.check { | ||
| // TODO(Johan-Liebert1): If we have the previous, i.e. the current manifest with us then we can replace the |
There was a problem hiding this comment.
And yes we need to store the manifest.
| /// EFI Partition size for composefs installations | ||
| /// We need more space than ostree as we have UKIs and UKI addons | ||
| /// We might also need to store UKIs for pinned deployments | ||
| pub(crate) const CFS_EFIPN_SIZE_MB: u32 = 1024; |
There was a problem hiding this comment.
(Ultimately of course we want this to be more OS controllable, and that thread is adding support for repart.d in to-disk)
2 participants
Increase ESP size to 1GB for composefs-backend
Handle the case when an update is already staged. Handle the case when
the staged image is not the same as the update image. Implement
--checkoption
Get OS version from UKI for display