[WIP] composefs-splitfdstream: new crate#228
[WIP] composefs-splitfdstream: new crate#228giuseppe wants to merge 3 commits intocomposefs:mainfrom
Conversation
f59c77c to
eb7f1ef
Compare
| //! This module implements the JSON-RPC 2.0 protocol types used to communicate | ||
| //! with the containers-storage splitfdstream server. The protocol is line-delimited | ||
| //! JSON over Unix sockets with file descriptor passing via SCM_RIGHTS. |
There was a problem hiding this comment.
WDYT about https://github.com/cgwalters/jsonrpc-fdpass ? I spent a bit of time on that. I was thinking about transferring it either to github.com/composefs or github.com/bootc-dev.
| /// - `i64 LE >= 0`: external FD reference at index `value` | ||
| #[derive(Debug)] | ||
| pub struct SplitFDStreamReader { | ||
| cursor: Cursor<Vec<u8>>, |
There was a problem hiding this comment.
why wouldn't this accept any impl Read (or BufRead) per above
| @@ -0,0 +1,185 @@ | |||
| //! Binary stream format parser for splitfdstream. | |||
There was a problem hiding this comment.
I did a whole implementation of this over in https://github.com/containers/composefs-rs/pull/218/changes#diff-69fe9e0f1388b28cd8d358d8f76589aaf790016b57fca196c2d22e0f93012b9b - any reason not to unify?
There was a problem hiding this comment.
the version here is mostly vibe coded to get it working with the implementation in the storage library.
I guess we can get the other version work too with the c/storage version. Do you mind splitting the splitfdstream implementation from the rest of the PR?
There was a problem hiding this comment.
Do you mind splitting the splitfdstream implementation from the rest of the PR?
➡️ #229
crates/cfsctl/src/main.rs
Outdated
| digest: String, | ||
| /// Optional reference name for the layer | ||
| name: Option<String>, | ||
| /// Import from splitfdstream server socket instead of stdin |
There was a problem hiding this comment.
This one feels different enough to be a new verb
eb7f1ef to
4f80ec6
Compare
| /// | ||
| /// Uses libc recvmsg directly because rustix 1.x has a bug where | ||
| /// msg_controllen is set to 0, preventing SCM_RIGHTS reception. | ||
| #[allow(unsafe_code)] |
There was a problem hiding this comment.
Hmm really? Are you sure? We were successfully using rustix in https://github.com/cgwalters/jsonrpc-fdpass ...
(Also why are we not using that crate? I can publish it formally...ok just transferred it to the bootc-dev org along with the Go version too)
If there's fixes needed in rustix the maintainer is responsive.
We're actually very close to #[forbid(unsafe_code)] in this project, it's just #123
| let mut remaining = size; | ||
|
|
||
| while remaining > 0 { | ||
| let n = rustix::fs::copy_file_range( |
There was a problem hiding this comment.
I believe this optimization is already in std https://github.com/rust-lang/rust/blob/7057231bd78d6c7893f905ea1832365d4c5efe17/library/std/src/io/copy.rs#L54 so we can just use std::io::copy
| repo.finalize_object_tmpfile(File::from(tmpfile), size) | ||
| } | ||
|
|
||
| fn read_fd_contents(fd: &OwnedFd, size: u64) -> Result<Vec<u8>> { |
There was a problem hiding this comment.
Why load the whole thing into memory instead of consistently using the O_TMPFILE path?
| } | ||
| let request = serde_json::json!({ | ||
| "jsonrpc": "2.0", | ||
| "method": "GetSplitFDStream", |
There was a problem hiding this comment.
I think this is fine for PoC however bigger picture I think what we need to do is add this method to https://github.com/containers/skopeo/blob/main/cmd/skopeo/proxy.go and then wrap it with https://github.com/bootc-dev/containers-image-proxy-rs/ right?
(And maybe a side quest in there is "port proxy to jsonrpc-fdpass" as new API)
There was a problem hiding this comment.
Or in other words... look at what I did in #218 the key thing I think here is fast-pathing the containers-storage: transport using this.
I think in order to do that we really do need to have not just an "import a single layer" as a CLI verb but do the whole "fetch manifest+config + layers" right?
There was a problem hiding this comment.
@giuseppe did you see this one? I think to really make this work we need to wire it into the production pull code paths that are used by bootc, not just a lookaside CLI verb right?
Of course doing this is going to lead to conflicts with #218 because it basically wants the same "detect containers-storage, dispatch to different pull path" logic.
My preference honestly is to land that PR first to get us unblocked, then we continue iteration on this one and once we can rely on always having a new enough skopeo/podman we can drop the cstor crate or so?
4f80ec6 to
9d250d2
Compare
c768584 to
1ae8ee8
Compare
d2fd4ef to
d00f9ae
Compare
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
| let consumed = stream.byte_offset(); | ||
| self.buffer.drain(..consumed); | ||
|
|
||
| // Dequeue FDs indicated by the "fds" field. |
There was a problem hiding this comment.
This code can just use jsonrpc-fdpass rust crate now right?
| } | ||
| let request = serde_json::json!({ | ||
| "jsonrpc": "2.0", | ||
| "method": "GetSplitFDStream", |
There was a problem hiding this comment.
@giuseppe did you see this one? I think to really make this work we need to wire it into the production pull code paths that are used by bootc, not just a lookaside CLI verb right?
Of course doing this is going to lead to conflicts with #218 because it basically wants the same "detect containers-storage, dispatch to different pull path" logic.
My preference honestly is to land that PR first to get us unblocked, then we continue iteration on this one and once we can rely on always having a new enough skopeo/podman we can drop the cstor crate or so?
probably I can even close this PR, I am using it only to test the integration with the container-libs changes.
#218 still scares me because it is not clear for how long we'd use that and if something goes wrong with the new API we will be stuck with that implementation, but if you’re confident in the approach, please go ahead. |
Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
Add a client library for communicating with the containers-storage splitfdstream server. This enables importing OCI container layers via UNIX socket with file descriptor passing.
Needs: containers/container-libs#651