Skip to content

Windows support for the synchronous shim#139

Merged
mxpv merged 5 commits intocontainerd:mainfrom
jsturtevant:windows
Jun 29, 2023
Merged

Windows support for the synchronous shim#139
mxpv merged 5 commits intocontainerd:mainfrom
jsturtevant:windows

Conversation

@jsturtevant
Copy link
Copy Markdown
Contributor

@jsturtevant jsturtevant commented Apr 26, 2023 •
edited
Loading

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://github.com/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
mxpv
mxpv reviewed Apr 26, 2023
View reviewed changes
Copy link
Copy Markdown
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)

@jsturtevant
Copy link
Copy Markdown
Contributor Author

jsturtevant commented Apr 26, 2023

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
Copy Markdown
Contributor Author

jsturtevant commented Apr 27, 2023

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
Copy Markdown
Contributor Author

jsturtevant commented Apr 28, 2023

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/genproto@v0.0.0-202[3](https://github.com/containerd/rust-extensions/actions/runs/4825275170/jobs/8595977056?pr=139#step:7:3)0306155012-7f2fa6fef1f[4](https://github.com/containerd/rust-extensions/actions/runs/4825275170/jobs/8595977056?pr=139#step:7:4) requires
	cloud.google.com/go/bigquery@v1.48.0 requires
	github.com/apache/arrow/go/v10@v10.0.1 requires
	gonum.org/v1/gonum@v0.11.0 requires
	golang.org/x/exp@v0.0.0-20191002040644-a13[5](https://github.com/containerd/rust-extensions/actions/runs/4825275170/jobs/8595977056?pr=139#step:7:5)5ae1e2c3 requires
	golang.org/x/mobile@v0.0.0-20190719004257-d2bd2a29d028: invalid version: git ls-remote -q origin in /home/runner/go/pkg/mod/cache/vcs/1ba9cdedc[6](https://github.com/containerd/rust-extensions/actions/runs/4825275170/jobs/8595977056?pr=139#step:7:6)63bd56a968[7](https://github.com/containerd/rust-extensions/actions/runs/4825275170/jobs/8595977056?pr=139#step:7:7)f5910f19e1ac5f7f472742441d370be599c409f[8](https://github.com/containerd/rust-extensions/actions/runs/4825275170/jobs/8595977056?pr=139#step:7:8)7ec: exit status [12](https://github.com/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 jsturtevant mentioned this pull request Apr 28, 2023
Merged
mxpv
mxpv reviewed Apr 28, 2023
View reviewed changes
@jsturtevant
Copy link
Copy Markdown
Contributor Author

jsturtevant commented Apr 28, 2023

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 jsturtevant mentioned this pull request May 1, 2023
Open
30 tasks
This was referenced May 12, 2023
Merged
Merged
@jsturtevant jsturtevant force-pushed the windows branch 2 times, most recently from db51ca4 to 14d0240 Compare June 21, 2023 22:57
@jsturtevant
Copy link
Copy Markdown
Contributor Author

jsturtevant commented Jun 21, 2023

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
Copy Markdown
Contributor Author

jsturtevant commented Jun 21, 2023

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

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

jsturtevant commented Jun 21, 2023

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
@jsturtevant
Windows support for the synchronous shim
bceaf4a 
Signed-off-by: James Sturtevant <jsturtevant@gmail.com>
Signed-off-by: James Sturtevant <jstur@microsoft.com>
@jsturtevant
Make one function that works for both platforms
fa4a004 
Signed-off-by: James Sturtevant <jstur@microsoft.com>
@jsturtevant
Clean up signal processing
b979559 
Signed-off-by: James Sturtevant <jstur@microsoft.com>
@jsturtevant
Use inline platform branches
38545a7 
Signed-off-by: James Sturtevant <jstur@microsoft.com>
@jsturtevant
Update to use latest ttrpc library
bdd17eb 
Signed-off-by: James Sturtevant <jstur@microsoft.com>
@jsturtevant
Copy link
Copy Markdown
Contributor Author

jsturtevant commented Jun 22, 2023

ci is passing again after #148

@jsturtevant jsturtevant mentioned this pull request Jun 23, 2023
Closed
@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

Reviewers

@mxpv mxpv mxpv approved these changes

Assignees

No one assigned

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Will the rust-extensions support Windows?

2 participants

@jsturtevant @mxpv