Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Windows support for the synchronous shim #139

Merged
merged 5 commits into from
Jun 29, 2023
Merged

Conversation

jsturtevant
Copy link
Contributor

@jsturtevant jsturtevant commented Apr 26, 2023

Fixes: #4

This adds Windows support for the synchronous shim on windows. It builds on the support added to ttrpc-rust for Windows. It does require a couple updates (containerd/ttrpc-rust#182 and containerd/ttrpc-rust#185) and a release from that project.

Couple things to note:

  • Windows doesn't have the same signal handling as Linux so this adds a real basic implementation (thanks @danbugs for helping). It looks like the Linux implementation uses SIGCHILD to handle child processes but Windows doesn't have a similar concept. If required down the line we could look into something as described in https://learn.microsoft.com/en-us/previous-versions/ms811896(v=msdn.10)#signals-and-signal-handling
  • Windows shim logging is implemented using named Pipes in Containerd. I've added detailed comments in the code on some assumptions. The file logger does work for windows so I didn't completely remove it from compiling on windows, in case we wanted to make that configurable (it mostly makes debugging this crate easier).
  • The windows shim activation is slightly different. Again I've left comments inline but the basic pattern comes from https:/microsoft/hcsshim/blob/v0.10.0-rc.7/cmd/containerd-shim-runhcs-v1/serve.go#L57-L70. This seemed simpler than re-writing the rust command implementation in this repository but does add some complexity to how the shim boots.
  • There are several crate in here that either won't ever work (runc) or I didn't focus on initially. This means the cargo build from root won't just work. This means a user would need to call cargo build from the crate folders that do support windows (shim/shim-protos)

Todo:

  • add a Windows pipeline to validate code
  • respond to initial feedback
  • get a release of ttrpc

@jsturtevant jsturtevant force-pushed the windows branch 2 times, most recently from bd3b938 to dc59249 Compare April 26, 2023 00:10
Copy link
Member

@mxpv mxpv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is awesome!
Would be nice to add windows target to our CI to run tests on Windows
(and fix clippy/rustfmt complains)

crates/shim/src/synchronous/mod.rs Outdated Show resolved Hide resolved
@jsturtevant
Copy link
Contributor Author

Would be nice to add windows target to our CI to run tests on Windows

💯 one of the tasks I have to take care to get this out of draft. Thanks for the quick response!

@jsturtevant jsturtevant force-pushed the windows branch 13 times, most recently from c37f70f to 59f358f Compare April 27, 2023 23:08
@jsturtevant
Copy link
Contributor Author

Would be nice to add windows target to our CI to run tests on Windows

💯 one of the tasks I have to take care to get this out of draft. Thanks for the quick response!

I've added a job for Windows. Since it only supports a subset of the workspaces right now, it is is own job. Also note until (containerd/ttrpc-rust#182 and containerd/ttrpc-rust#185) both merge the unit tests on windows are flaky.

@jsturtevant
Copy link
Contributor Author

the ubuntu 20.04 failure doesn't look related:

sudo -E PATH=$PATH make integration
  shell: /usr/bin/bash -e {0}
  env:
    GOPROXY: direct
    TEST_RUNTIME: io.containerd.runc.v2-rs
    TESTFLAGS_PARALLEL: 1
    EXTRA_TESTFLAGS: -no-criu -test.skip='(TestContainerPTY|TestContainerExecLargeOutputWithTTY|TestTaskUpdate|TestTaskResize)'
    TESTFLAGS_RACE: -race
+ integration
go: google.golang.org/[email protected][3](https:/containerd/rust-extensions/actions/runs/4825275170/jobs/8595977056?pr=139#step:7:3)0306155012-7f2fa6fef1f[4](https:/containerd/rust-extensions/actions/runs/4825275170/jobs/8595977056?pr=139#step:7:4) requires
	cloud.google.com/go/[email protected] requires
	github.com/apache/arrow/go/[email protected] requires
	gonum.org/v1/[email protected] requires
	golang.org/x/[email protected][5](https:/containerd/rust-extensions/actions/runs/4825275170/jobs/8595977056?pr=139#step:7:5)5ae1e2c3 requires
	golang.org/x/[email protected]: invalid version: git ls-remote -q origin in /home/runner/go/pkg/mod/cache/vcs/1ba9cdedc[6](https:/containerd/rust-extensions/actions/runs/4825275170/jobs/8595977056?pr=139#step:7:6)63bd56a968[7](https:/containerd/rust-extensions/actions/runs/4825275170/jobs/8595977056?pr=139#step:7:7)f5910f19e1ac5f7f472742441d370be599c409f[8](https:/containerd/rust-extensions/actions/runs/4825275170/jobs/8595977056?pr=139#step:7:8)7ec: exit status [12](https:/containerd/rust-extensions/actions/runs/4825275170/jobs/8595977056?pr=139#step:7:13)8:
	fatal: unable to access 'https://go.googlesource.com/mobile/': The requested URL returned error: 502

@jsturtevant
Copy link
Contributor Author

I think this is ready for review, just waiting for a few merges and a release from ttrpc-rust which should be soon 🤞 . I will leave temporary commits and leave it in draft until then.

@jsturtevant
Copy link
Contributor Author

nothing major changed in the ttrpc version I had pinned and the released crate. Not sure if something else changed to cuase the integration tests to fail. I might split out the ttrpc verion upgrade anyways (not sure what this repositories standards are but that is generally asked for in other repos I've worked in).

@jsturtevant
Copy link
Contributor Author

oh, looks like nightly CI started failing last week: https:/containerd/rust-extensions/actions with the first failure in https:/containerd/rust-extensions/actions/runs/5273246763

@jsturtevant jsturtevant marked this pull request as ready for review June 21, 2023 23:37
@jsturtevant
Copy link
Contributor Author

ttrpc 0.8.0 has been released! I've moved this from draft to ready for review.

I might split out the ttrpc verion upgrade anyways (not sure what this repositories standards are but that is generally asked for in other repos I've worked in).

@mxpv thoughts?

jsturtevant and others added 4 commits June 22, 2023 10:06
Signed-off-by: James Sturtevant <[email protected]>
Signed-off-by: James Sturtevant <[email protected]>
Signed-off-by: James Sturtevant <[email protected]>
Signed-off-by: James Sturtevant <[email protected]>
Signed-off-by: James Sturtevant <[email protected]>
@jsturtevant
Copy link
Contributor Author

ci is passing again after #148

@mxpv mxpv merged commit b962626 into containerd:main Jun 29, 2023
@mxpv mxpv added C-shim-protos Shim protos C-shim Containerd shim T-CI Changes in project's CI OS-windows A change is specific to Windows labels Aug 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-shim Containerd shim C-shim-protos Shim protos OS-windows A change is specific to Windows T-CI Changes in project's CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Will the rust-extensions support Windows?
2 participants