Windows Support for Sync implementation #172

jsturtevant · 2023-02-01T03:41:01Z

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:

moves unix specific calls out of the main server and client functions. It does this by introducing several new types: PipeListener, PipeConnection, and ClientConnection. 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.
Adds the windows functionality for PipeListener, PipeConnection, and ClientConnection 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 the async 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.
Add windows build agents and modifies the example so it runs in an integration test using 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.

codecov · 2023-02-01T03:46:31Z

Codecov Report

Patch coverage has no change and project coverage change: -1.33 ⚠️

Comparison is base (34003e3) 25.76% compared to head (4975099) 24.43%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #172      +/-   ##
==========================================
- Coverage   25.76%   24.43%   -1.33%     
==========================================
  Files          16       17       +1     
  Lines        2395     2525     +130     
==========================================
  Hits          617      617              
- Misses       1778     1908     +130

Impacted Files	Coverage Δ
src/asynchronous/client.rs	`0.00% <0.00%> (ø)`
src/asynchronous/stream.rs	`0.00% <0.00%> (ø)`
src/common.rs	`35.57% <ø> (ø)`
src/error.rs	`55.55% <ø> (ø)`
src/lib.rs	`100.00% <ø> (ø)`
src/sync/channel.rs	`0.00% <0.00%> (ø)`
src/sync/client.rs	`0.00% <0.00%> (ø)`
src/sync/server.rs	`0.00% <0.00%> (ø)`
src/sync/sys/unix/net.rs	`0.00% <0.00%> (ø)`
src/sync/utils.rs	`0.00% <ø> (ø)`

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Cargo.toml

example/client.rs

kzys · 2023-02-09T01:09:06Z

example/client.rs

        now.elapsed()
    );
    let show = match ac.online_cpu_mem(default_ctx(), &agent::OnlineCPUMemRequest::new()) {
-        Err(e) => format!("{:?}", e),
-        Ok(s) => format!("{:?}", s),
+        Err(Error::RpcStatus(s)) => {


assert! and assert_eq! can take additional parameters to act like format!

https://doc.rust-lang.org/std/macro.assert.html#custom-messages

I tried this out but since the macro doesn't return anything it isn't working. It seems the formatting is only used when failed. From the doc: Expressions used as format arguments will only be evaluated if the assertion fails.

tests/sync-test.rs

src/sync/sys/windows/net.rs

Tim-Zhang · 2023-02-09T07:04:01Z

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.

Makefile

src/sync/utils.rs

jiangliu · 2023-02-09T16:27:34Z

src/sync/sys/windows/net.rs

@@ -0,0 +1,303 @@
+// Copyright (c) 2019 Ant Financial


Seems need to update copyright?

not sure what the policy is for this project?

Is Copyright The containerd Authors. better?
And also for other new files.

I think it'd make sense for @liubin's suggestion given this lives in the containerd org. The language shouldn't matter at all, but another data point for rust code is the rust-extensions repo follows the convention from the other projects and has the containerd authors copyrights for all of its crates.

I pulled the content for containerd/containerd (same as rust-extensions). Do all the mod.rs files need it too? Maybe we can follow up and change it all over with a CI check too?

jsturtevant · 2023-02-09T18:31:26Z

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?

jsturtevant · 2023-02-09T18:33:29Z

I've also started to use these changes for add support for Windows to the shim code: containerd/rust-extensions#4 (comment). This will help me validate the some of the changes too.

Tim-Zhang · 2023-02-13T08:31:19Z

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.

src/sync/sys/windows/mod.rs

liubin · 2023-02-10T03:43:23Z

src/sync/sys/windows/net.rs

@@ -0,0 +1,303 @@
+// Copyright (c) 2019 Ant Financial


Is Copyright The containerd Authors. better?
And also for other new files.

src/sync/client.rs

src/sync/channel.rs

src/sync/sys/unix/net.rs

src/sync/server.rs

Cargo.toml

Tim-Zhang · 2023-02-14T03:32:30Z

example/Cargo.toml

@@ -10,6 +10,10 @@ repository = "https://github.com/alipay/ttrpc-rust"
 homepage = "https://github.com/alipay/ttrpc-rust"
 description = "An example of ttrpc."

+[dependencies]


I think cargo run --example async-server --features=async is not that easy to use, in my opinion, We can keep the example/Cargo.toml unchanged, import all features from ttrpc is ok because this is just an example

ok this makes sense, didn't think about it from that perspective. I did it this way since when you import async from ttrpc it fails to build due to async not working with Windows. I tried using conditional dependencies that brought in a feature based on OS but that didn't work due to rust-lang/cargo#1197 (note the nightly changes in cargo did work with rust-lang/cargo#7914 using rustup override set nightly & cargo +nightly build -Z features=itarget)

I was able to work around this by updating the async marcro to check for linux. Doing that also let me add the examples project to the workspace as well which is a nice improvement for the developer experience in VScode (gives intelisense and compile errors when editing it).

example/Cargo.toml

example/protocols/mod.rs

src/asynchronous/client.rs

src/sync/sys/windows/net.rs

example/utils.rs

Tim-Zhang · 2023-02-14T10:03:58Z

@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.

dcantah · 2023-02-14T10:05:14Z

@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

src/sync/sys/windows/net.rs

jsturtevant · 2023-02-21T19:06:29Z

thanks for the reviews everyone! I've been testing this with the Rust Extensions and runwasi. Will be addressing feedback this week.

src/asynchronous/client.rs

src/sync/channel.rs

src/sync/client.rs

src/sync/sys/windows/net.rs

dcantah · 2023-02-22T08:21:16Z

src/sync/sys/windows/net.rs

+        let h = unsafe {
+            CreateNamedPipeW(
+                name.as_ptr(),
+                openmode,
+                PIPE_TYPE_BYTE,
+                PIPE_UNLIMITED_INSTANCES,
+                65536,
+                65536,
+                0,
+                std::ptr::null_mut(), // todo set this on first instance
+            )
+        };


We should add PIPE_REJECT_REMOTE_CLIENTS

@dcantah I was looking into the security descriptor. A null value sets the default security descriptor which only allows Admins and the current user (https://learn.microsoft.com/en-us/windows/win32/ipc/named-pipe-security-and-access-rights).

I was comparing to the way go-wino creates it, And I believe this is the same behavior although go-winio is using undocumented calls (ntCreateNamedPipeFile and rtlDefaultNpAcl) so has the additional calls to rtlDefaultNpAcl to set the default. In the case of the documented CreateNamedPipeW null provides a sane default of requiring admins.

src/sync/sys/windows/net.rs

dcantah · 2023-02-22T08:29:01Z

@jsturtevant I'll try and do another pass soon, left some comments. Sorry for not getting to this on Friday 😐

src/sync/sys/windows/net.rs

jsturtevant · 2023-02-28T00:17:44Z

@Tim-Zhang @dcantah @jiangliu @kzys This is ready for another round of reviews. I can squash all the commits but left them for now.

Tim-Zhang

LGTM, thanks @jsturtevant

jsturtevant · 2023-03-01T23:10:34Z

I've rebased and added commit messages

dcantah

Mostly bunch of formatting comments, but LGTM! I took this for a spin and it seemed to work fine. We can always iterate also

src/sync/client.rs

src/sync/channel.rs

src/sync/sys/windows/net.rs

src/sync/sys/unix/net.rs

dcantah · 2023-03-02T10:27:29Z

src/sync/sys/windows/net.rs

+                    0 => {
+                        return Err(Error::Windows(io::Error::last_os_error().raw_os_error().unwrap()))
+                    }
+                    _ => {
+                        return Ok(bytes_transfered as usize)
+                    }


Wonder if we should have a helper for this match pattern given how common it is

Had the same thought and tried to move it to something like process_overlapped() but when I did things didn't work properly. Given it is only repeated twice (for read and write) I decided to keep it as is.

Oh I meant the general 0 or _ match pattern. accept, read, write, shutdown and close_handle follow this trend, but might not be worth the hassle. Probably clearer to have it in the call site

dcantah

lgtm again

jsturtevant · 2023-03-02T23:55:35Z

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 3ac5b6e for review

src/sync/client.rs

quanweiZhou

Thanks, @jsturtevant few comments.

example/client.rs

src/sync/sys/unix/net.rs

src/sync/utils.rs

Tim-Zhang · 2023-03-20T03:27:35Z

@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 · 2023-03-20T19:04:02Z

@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 #178 to resolve them and will rebase this once that is in.

Adds asserts to the example project so it will fail if something isn't work between the client and server. This also adds an integration test that builds and runs the sync example so the changes to the project can be validated e2e. Signed-off-by: James Sturtevant <[email protected]>

This moves unix specific calls out of the main server and client functions. It does this by introducing several new types: PipeListener, PipeConnection, and ClientConnection. These types are contain the unix specific functionality to communitcate with Unix Domain sockets and are hidden behind a conditional compilation flag. Signed-off-by: James Sturtevant <[email protected]>

Adds the windows functionality for PipeListener, PipeConnection, and ClientConnection 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 the async projects (as the unix specific code hasn't been removed yet). Namedpipes are used as Containerd is one of the main use cases for this project on Windows and containerd only supports namedpipes. Signed-off-by: James Sturtevant <[email protected]>

The connect namedpipe thread would be in a suspended state when shutdown on the server is called. Setting the event to a signalled state to wake the thread up so everything can shut down properly. Signed-off-by: James Sturtevant <[email protected]>

jsturtevant · 2023-03-24T16:36:07Z

I've rebased ontop of #179. Thanks for the help unblocking

cpuguy83 requested review from mxpv and lifupan February 8, 2023 19:17

dcantah self-requested a review February 8, 2023 19:25

kzys reviewed Feb 9, 2023

View reviewed changes

Tim-Zhang requested review from jodh-intel, liubin, quanweiZhou and wllenyj February 9, 2023 06:12

jiangliu reviewed Feb 9, 2023

View reviewed changes

Makefile Show resolved Hide resolved

jiangliu reviewed Feb 9, 2023

View reviewed changes

src/sync/utils.rs Outdated Show resolved Hide resolved

jiangliu reviewed Feb 9, 2023

View reviewed changes

liubin reviewed Feb 14, 2023

View reviewed changes

Tim-Zhang reviewed Feb 14, 2023

View reviewed changes

wllenyj reviewed Feb 17, 2023

View reviewed changes

src/sync/sys/windows/net.rs Outdated Show resolved Hide resolved

dcantah reviewed Feb 22, 2023

View reviewed changes

src/sync/sys/windows/net.rs Outdated Show resolved Hide resolved

dcantah reviewed Feb 22, 2023

View reviewed changes

src/sync/sys/windows/net.rs Outdated Show resolved Hide resolved

dcantah reviewed Feb 22, 2023

View reviewed changes

src/sync/sys/windows/net.rs Outdated Show resolved Hide resolved

jsturtevant force-pushed the windows3 branch 3 times, most recently from 263aea0 to d5b76e3 Compare February 23, 2023 18:46

jsturtevant force-pushed the windows3 branch from 61bc260 to c1171ec Compare February 27, 2023 23:56

Tim-Zhang approved these changes Mar 1, 2023

View reviewed changes

jsturtevant force-pushed the windows3 branch from 65af714 to 7cbc764 Compare March 1, 2023 22:54

jsturtevant mentioned this pull request Mar 2, 2023

Update project to latest shim-protos and ttrpc using protobuf 3.x containerd/runwasi#81

Merged

dcantah approved these changes Mar 2, 2023

View reviewed changes

jsturtevant force-pushed the windows3 branch from 7cbc764 to 746ddd1 Compare March 2, 2023 17:40

dcantah approved these changes Mar 2, 2023

View reviewed changes

jsturtevant force-pushed the windows3 branch from df403fb to 3ac5b6e Compare March 2, 2023 23:53

jsturtevant force-pushed the windows3 branch from 3ac5b6e to 7d40cc7 Compare March 3, 2023 18:15

Tim-Zhang reviewed Mar 6, 2023

View reviewed changes

src/sync/client.rs Show resolved Hide resolved

jsturtevant force-pushed the windows3 branch 2 times, most recently from aa6685a to 416d571 Compare March 7, 2023 16:47

jsturtevant mentioned this pull request Mar 7, 2023

Windows support containerd/runwasi#49

Open

30 tasks

quanweiZhou reviewed Mar 11, 2023

View reviewed changes

example/client.rs Outdated Show resolved Hide resolved

example/client.rs Outdated Show resolved Hide resolved

src/sync/sys/unix/net.rs Show resolved Hide resolved

src/sync/sys/unix/net.rs Show resolved Hide resolved

src/sync/utils.rs Show resolved Hide resolved

jsturtevant force-pushed the windows3 branch from 416d571 to 1b0dd65 Compare March 17, 2023 23:39

quanweiZhou approved these changes Mar 19, 2023

View reviewed changes

jsturtevant force-pushed the windows3 branch from 1b0dd65 to fd884af Compare March 20, 2023 17:54

jsturtevant added 4 commits March 24, 2023 09:00

Fix stuck thread on server shutdown

4975099

The connect namedpipe thread would be in a suspended state when shutdown on the server is called. Setting the event to a signalled state to wake the thread up so everything can shut down properly. Signed-off-by: James Sturtevant <[email protected]>

jsturtevant force-pushed the windows3 branch from fd884af to 4975099 Compare March 24, 2023 16:16

lifupan merged commit ebbb58e into containerd:master Mar 27, 2023

jsturtevant mentioned this pull request Apr 26, 2023

Windows support for the synchronous shim containerd/rust-extensions#139

Merged

3 tasks

Windows Support for Sync implementation #172

Windows Support for Sync implementation #172

Conversation

jsturtevant commented Feb 1, 2023

codecov bot commented Feb 1, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tim-Zhang commented Feb 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsturtevant commented Feb 9, 2023

jsturtevant commented Feb 9, 2023

Tim-Zhang commented Feb 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tim-Zhang commented Feb 14, 2023

dcantah commented Feb 14, 2023 • edited Loading

jsturtevant commented Feb 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcantah commented Feb 22, 2023

jsturtevant commented Feb 28, 2023

Tim-Zhang left a comment

Choose a reason for hiding this comment

jsturtevant commented Mar 1, 2023

dcantah left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcantah left a comment

Choose a reason for hiding this comment

jsturtevant commented Mar 2, 2023

quanweiZhou left a comment

Choose a reason for hiding this comment

Tim-Zhang commented Mar 20, 2023

jsturtevant commented Mar 20, 2023

jsturtevant commented Mar 24, 2023

codecov bot commented Feb 1, 2023 •

edited

Loading

Tim-Zhang commented Feb 13, 2023 •

edited

Loading

dcantah commented Feb 14, 2023 •

edited

Loading