cli/cfsctl: Use proxy from composefs-rs#2023
Conversation
There was a problem hiding this comment.
Code Review
The pull request successfully migrates the cfsctl functionality into an external composefs-rs library, which is a positive step towards code modularity and adherence to DRY principles. The dependency updates across Cargo.lock, Cargo.toml, and deny.toml are consistent with this change. The removal of the internal cfsctl.rs module and the corresponding updates in cli.rs and lib.rs correctly reflect the new architecture.
|
Ok wow |
|
Looks like an issue with creating loop device Precisely the issue seems to be here https://github.com/composefs/composefs-rs/blob/022c9c62216615af471acaa78f486203a53f06e3/crates/composefs-ioctls/src/loop_device.rs#L154 Using raw if libc::ioctl(loop_dev.as_raw_fd(), 0x4C0A, &config) < 0 {
println!("IOCTL RAW FAILED");
}which is really weird to me as I don't see any difference between the outputs for both produced by strace |
|
We regressed with this commit composefs/composefs-rs@984ce03 But I don't see the issue immediately |
|
Okay issues seems to be that |
|
Fix here composefs/composefs-rs#240 |
04f8096 to
bb9b581
Compare
5f14b82 to
8dd25ba
Compare
Also, update the git URL to point to https://github.com/composefs/composefs-rs Signed-off-by: Johan-Liebert1 <pragyanpoudyal41999@gmail.com>
Now that we have cfsctl as a library, we simply use that to proxy all of our `bootc internal cfs` commands. Cleans up the codebase a bit, DRYs it and keeps things in sync Signed-off-by: Johan-Liebert1 <pragyanpoudyal41999@gmail.com>
8dd25ba to
46fc035
Compare
crates/lib/src/store/mod.rs
Outdated
| /// This lazily opens the composefs repository, creating the directory if needed | ||
| /// and bootstrapping verity settings from the ostree configuration. | ||
| pub(crate) fn get_ensure_composefs(&self) -> Result<Arc<ComposefsRepository>> { | ||
| pub(crate) fn get_ensure_composefs( |
There was a problem hiding this comment.
I think this PR is trying to solve the same problem ("ro sysroot") in two different ways.
The simplest and IMO best way is what you've already done in the test code below: change the cfsctl invocation to operate on something in /var/tmp.
Changing this core storage code is I believe unnecessary and conceptually wrong because this particular module is really about the bootc-owned storage only.
There was a problem hiding this comment.
Yes, in hindsight this makes sense. Reverted the change in get_ensure_composefs
46fc035 to
b3cb91a
Compare
The `bootc internals cfs` test was running into an issue where pulling an image into a composefs repository at `/sysroot/composefs` was failing due to /sysroot being mounted ro. We did remount it rw in an earlier cmd, but that was in a separate mount ns which did not carry over to the next commands Instead of globally remounting /sysroot as rw, we now simply create a new repository at /var/tmp/sysroot/composefs and perform all operations there. Also, pass in `--insecure` to `bootc internals cfs ...` as in the ostree case we're testing in xfs which does not support fs-verity Signed-off-by: Pragyan Poudyal <pragyanpoudyal41999@gmail.com>
b3cb91a to
cff08ec
Compare
|
BTW once this lands we should also be able to bump the bootc commit in composefs-rs and restore the revdep check. |
Bump composefs-rs
Also, update the git URL to point to
https://github.com/composefs/composefs-rs
cli/cfsctl: Use proxy from composefs-rs
Now that we have cfsctl as a library, we simply use that to proxy all of
our
bootc internal cfscommands. Cleans up the codebase a bit, DRYsit and keeps things in sync