Composefs native refactor #1599
Conversation
Move all composefs-native related stuff into `lib/src/bootc_composefs`. This might help us to spot diff while merging into main, also should hopefully create less conflicts Signed-off-by: Pragyan Poudyal <[email protected]>
Refactor to use `#[cfg(feature = "composefs-backend")]` to gate composefs native features behind the flag. Gate the following features - `--composefs-native` and its corresponding cli args - Installing/Switching/Upgrading/RollingBack of composefs native system - Create separate install, rollback functions for ostree for a bit cleaner of a setup Signed-off-by: Pragyan Poudyal <[email protected]>
![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 is a significant and well-executed refactoring that moves all composefs-native related code into a dedicated crates/lib/src/bootc_composefs module and gates it behind a composefs-backend feature flag. The changes are clean and align with the stated goals. I've identified a high-severity bug in switch.rs where the transport protocol is hardcoded, which would prevent switching to images from non-docker registries. Additionally, I've found a couple of minor typos in error messages and suggested a small refactoring in repo.rs for improved efficiency and code clarity. Overall, great work on this refactoring.
| let repo = open_composefs_repo(&rootfs_dir).context("Opening compoesfs repo")?; | ||
|
|
||
| let (id, verity) = | ||
| composefs_oci_pull(&Arc::new(repo), &format!("{transport}:{image}"), None, None) | ||
| .await | ||
| .context("Pulling composefs repo")?; | ||
|
|
||
| tracing::debug!( | ||
| "id = {id}, verity = {verity}", | ||
| id = hex::encode(id), | ||
| verity = verity.to_hex() | ||
| ); | ||
|
|
||
| let repo = open_composefs_repo(&rootfs_dir)?; | ||
| let mut fs = create_composefs_filesystem(&repo, &hex::encode(id), None) | ||
| .context("Failed to create composefs filesystem")?; |
There was a problem hiding this comment.
To avoid opening the composefs repository twice, you can create an Arc for the repository and reuse it. This is slightly more efficient and makes the code cleaner. This change also fixes a typo in the error message on line 71.
| let repo = open_composefs_repo(&rootfs_dir).context("Opening compoesfs repo")?; | |
| let (id, verity) = | |
| composefs_oci_pull(&Arc::new(repo), &format!("{transport}:{image}"), None, None) | |
| .await | |
| .context("Pulling composefs repo")?; | |
| tracing::debug!( | |
| "id = {id}, verity = {verity}", | |
| id = hex::encode(id), | |
| verity = verity.to_hex() | |
| ); | |
| let repo = open_composefs_repo(&rootfs_dir)?; | |
| let mut fs = create_composefs_filesystem(&repo, &hex::encode(id), None) | |
| .context("Failed to create composefs filesystem")?; | |
| let repo = Arc::new(open_composefs_repo(&rootfs_dir).context("Opening composefs repo")?); | |
| let (id, verity) = | |
| composefs_oci_pull(&repo, &format!("{transport}:{image}"), None, None) | |
| .await | |
| .context("Pulling composefs repo")?; | |
| tracing::debug!( | |
| "id = {id}, verity = {verity}", | |
| id = hex::encode(id), | |
| verity = verity.to_hex() | |
| ); | |
| let mut fs = create_composefs_filesystem(&repo, &hex::encode(id), None) | |
| .context("Failed to create composefs filesystem")?; |
There was a problem hiding this comment.
We return the repo right now and most functions accept a non Arc repo
Fix typo in error message Unhardcode transport while pulling a composefs repository on switch Signed-off-by: Pragyan Poudyal <[email protected]>
|
Might be worth opening the same PR but against main directly if we think we are ready to merge this gated by a feature flag. Might be easier to read the diff. |
cgwalters
left a comment
cgwalters
left a comment
There was a problem hiding this comment.
I'm tentatively OK as is but there's a lot of followup here
| #[cfg(feature = "composefs-backend")] | ||
| if composefs_booted()?.is_some() { | ||
| composefs_rollback().await? | ||
| } else { | ||
| rollback(&opts).await? | ||
| } | ||
|
|
||
| #[cfg(not(feature = "composefs-backend"))] | ||
| rollback(&opts).await?; |
There was a problem hiding this comment.
This code pattern is the problem in a nutshell
- It's duplicative and easy to mess up in later refactoring
- As we move forward I think we should be thinking of composefs as the core/future and so we may want to actually start naming things
ostree_as a prefix instead of prefixing everything withcomposefs_ - But really more generally we should operate in terms of methods
- I want to move the composefs repo loading into
mod storage
There was a problem hiding this comment.
Agreed with all the points. This was best effort that didn't require any other huge overhaul
3 participants
Changes
Move all composefs-native related code to
crates/lib/src/bootc_composefs. This might help us to spot diff while merging, also should hopefully create less conflicts on subsequentGate composefs-native behind
composefs-backendflagRefactor to use
#[cfg(feature = "composefs-backend")]to gate composefs native features behind the flag.Gate the following features
--composefs-nativeand its corresponding cli args