Closed jsturtevant closed 1 year ago
Patch coverage has no change and project coverage change: -1.33
:warning:
Comparison is base (
34003e3
) 25.76% compared to head (4975099
) 24.43%.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.
Wow, we have been looking forward for the feature for a long period of time, thank you very much for your PR @jsturtevant. Since the PR is a bit large, I think the reviewers need some time to give suggestions. thank you again.
Since the PR is a bit large, I think the reviewers need some time to give suggestions. thank you again.
makes sense. If it helps I could break this into several PR's or commits. I put it mostly together since I think it helps to understand the bigger picture but if general direction makes sense we can split it into PR's like the following:
This might make it easier to track and comment on changes?
I've also started to use these changes for add support for Windows to the shim code: https://github.com/containerd/rust-extensions/issues/4#issuecomment-1423125181. This will help me validate the some of the changes too.
Since the PR is a bit large, I think the reviewers need some time to give suggestions. thank you again.
makes sense. If it helps I could break this into several PR's or commits. I put it mostly together since I think it helps to understand the bigger picture but if general direction makes sense we can split it into PR's like the following:
- update examples to add asserts and run them as integration test
- refactor Linux to move code to the conditional compilation
- add windows implementation behind compilation
This might make it easier to track and comment on changes?
After several reviewers have done their reviews, there is no need to split it I think.
@jsturtevant I have reviewed the code and left a few comments. I have also built a windows env and tested it and it works great.
@jsturtevant Going to block off some time on Friday to review this, thanks a ton! I'll set this up on my desktop to trial it out
thanks for the reviews everyone! I've been testing this with the Rust Extensions and runwasi. Will be addressing feedback this week.
@jsturtevant I'll try and do another pass soon, left some comments. Sorry for not getting to this on Friday π
The additional locks that where required on the mio namedpipe objects were bothering me, so I re-wrote the windows part to use namedpipes more directly (https://github.com/containerd/ttrpc-rust/pull/172/commits/4e5eb9832c9e5102515af3f826ac5ea442c13111). This removed the need for the additional locks and simplified some of the logic. I maybe able to remove a few of the Arc
's in the main server implementation as well. fyi @dcantah
I am not sure why the macos build is timing out. I don't have access to a mac and the logs are not helpful
@Tim-Zhang @dcantah @jiangliu @kzys This is ready for another round of reviews. I can squash all the commits but left them for now.
I've rebased and added commit messages
I was testing with https://github.com/containerd/rust-extensions/tree/main/crates/shim and found the server.shutdown() would hang. I've pushed the fix on https://github.com/containerd/ttrpc-rust/pull/172/commits/3ac5b6e1ed52e77ba01e59be3a13d7f07eac884e for review
@jsturtevant Hi, It seems that there is a bug here:
error[E0308]: mismatched types
--> src/sync/client.rs:56:9
|
53 | pub fn new(fd: RawFd) -> Client {
| ------ expected `sync::client::Client` because of return type
...
56 | Self::new_client(conn)
| ^^^^^^^^^^^^^^^^^^^^^^ expected struct `Client`, found enum `Result`
|
= note: expected struct `sync::client::Client`
found enum `std::result::Result<sync::client::Client, error::Error>`
@jsturtevant Hi, It seems that there is a bug here:
error[E0308]: mismatched types --> src/sync/client.rs:56:9 | 53 | pub fn new(fd: RawFd) -> Client { | ------ expected `sync::client::Client` because of return type ... 56 | Self::new_client(conn) | ^^^^^^^^^^^^^^^^^^^^^^ expected struct `Client`, found enum `Result` | = note: expected struct `sync::client::Client` found enum `std::result::Result<sync::client::Client, error::Error>`
I was missing the update on the linux file. I've fixed with the comment to resolve in the next update.
The pr is failing on clippy updates, I've opened https://github.com/containerd/ttrpc-rust/pull/178 to resolve them and will rebase this once that is in.
I've rebased ontop of #179. Thanks for the help unblocking
Fixes: #132
This PR adds initial support for Windows. The scope is fairly large, so it only adds support for
sync
implementation to keep it fairly reasonable π¬. It would be possible to split out some parts of the work into smaller PR's or commits as needed. I have two main commits but happy to make them more distinct. It also has all the changes from #171 so I can drop those once that merges πNote: I am fairly new to Rust so might need some guidance on choices I made here. There is still some more work to be done (adding security file descriptors to named pipe for instance) in the PR but wanted to get it open for discussion on the general direction.
This PR does three main things:
PipeListener
,PipeConnection
, andClientConnection
. These types contain the unix specific functionality to communicate with Unix Domain sockets and are hidden behind a conditional compilation flag for the OS target.PipeListener
,PipeConnection
, andClientConnection
and does the few other changes required to build and run the example projects. This includes adding feature support to the examples so they wouldn't build theasync
projects (as the unix specific code hasn't been removed yet). Importantly I avoided changes to the code generation and service api, so there are minimal changes to the example code beyond changing the socket address to a namedpipe format.cargo test
. This seemed like a good way to ensure the whole system (code generation, client build and sever/client) worked e2e for all the OSes.Another thing to note is that this PR implements Windows support using named pipes. While windows does have a UDS implementation, Containerd does not currently support this. As one of the main uses for this is containerd so adding namedpipe support is needed.