Windows: named pipe mounts by jstarks · Pull Request #33852 · moby/moby

jstarks · 2017-06-27T19:32:27Z

Current insider builds of Windows have support for mounting individual
named pipe servers from the host to the guest. This allows, for example,
exposing the docker engine's named pipe to a container.

This change allows the user to request such a mount via the normal bind
mount syntax in the CLI:

docker run -v \\.\pipe\docker_engine:\\.\pipe\docker_engine <args>

jstarks · 2017-06-27T19:33:31Z

@jhowardmsft PTAL. One thing I didn't add was a version check; if you do this on an older Windows version it will silently ignore the pipe list. If you can suggest a proper version check, I can add one.

lowenna · 2017-06-27T20:07:50Z

libcontainerd/client_windows.go

lowenna · 2017-06-27T20:08:51Z

libcontainerd/client_windows.go

Could add a continue after this line, then can remove the else below and out-dent it.

Is that good go style? I thought since this was an either-or and not a special case + skip situation, the indentation was better.

That's fair

lowenna · 2017-06-27T20:19:51Z

libcontainerd/client_windows.go

For a version check, you could use system.GetOSVersion() and fail the attempt if build < ~16230. Here might be a good spot and check mps.Length > 0

I picked an older build number since the platform support has been available in insider builds for a little while.

lowenna · 2017-06-27T20:21:17Z

@johnstep FYI

lowenna

LGTM

lowenna · 2017-06-27T21:25:38Z

libcontainerd/client_windows.go

If you mark this 'TODO @jhowardmsft:...', I'll have a better chance of remembering 😊

I think at RTM we just need to search the code base for GetOSVersion and reevaluate all the checks, like we did at Server 2016 release.

AkihiroSuda · 2017-06-29T07:59:38Z

volume/validate.go

if mnt.ReadOnly { errExtraField("ReadOnly") }?

AkihiroSuda · 2017-06-29T08:02:15Z

Please add integration test? (But probably Skip() on current windows CI?)
No support for swarm-mode? (can be another PR?)

AkihiroSuda · 2017-07-12T03:12:45Z

ping @jstarks

johnstep

LGTM

johnstep · 2017-07-24T18:25:15Z

@jstarks, it looks good but will you address the feedback from @AkihiroSuda when you have a chance?

jstarks · 2017-07-28T02:00:31Z

I've pushed a new change that adds an integration test (only runs against RS3) and adds the read-only check, as requested by @AkihiroSuda. Thanks for the suggestions!

Swarm will have to be a separate PR.

AkihiroSuda · 2017-07-28T02:05:20Z

integration-cli/requirements_test.go

Is -> GreaterThan?

How about AtLeast?

AkihiroSuda · 2017-07-28T02:07:16Z

integration-cli/docker_cli_run_windows_test.go

IIUC, this requires newer CLI binary? (new test should be an API test)

I don't think this requires a CLI binary change since the bind mount parsing happens on the server side. When I ran this test locally I used an existing CLI.

Regardless, I didn't realize that new tests were at the API level, but I can change+move this if necessary.

Yes, please convert the test to API test?
https://forums.mobyproject.org/t/evolution-of-testing-moby/38

I don't see any existing API tests that launch a container using the API. They all use the CLI... E.g. TestGetContainersAttachWebsocket. Are there any samples I'm missing?

TestContainersAPICreateMountsTmpfs is one of API tests that is similar to your workload

moby/integration-cli/docker_api_containers_test.go

Line 1895 in 9d87e6e

func (s *DockerSuite) TestContainersAPICreateMountsTmpfs(c *check.C) {

Ah, thanks! Updated. By going through the API directly I was able to find a bug, which is now fixed. I also added some unit tests in volume.

simonferquel · 2017-08-01T08:48:52Z

volume/validate.go

Hmm, should'nt we honor the skipAbsolutePathCheck option ?

This option is obsolete. I removed it.

simonferquel · 2017-08-01T08:54:41Z

volume/volume_windows.go

I think we should do a 2 pass regex parsing because if source is a pipe, dest should be a pipe as well
Is this case tested somewhere btw ? (making sure c:\windows:\.\pipe\docker_engine and \.\pipe\docker_engine:c:\data are invalid)

Also with LCOW, how would we do named pipe bindings ? would it be mounted as a domain socket ? (would be awesome to be able to do \.pipe\docker_engine:/var/run/docker.sock mapping)

Named pipe semantics aren't quite the same as Unix socket semantics, unfortunately (e.g. for Docker's named pipe transport we simulate shutdown() by sending a zero-byte write). Given that, we're not going to support this kind of mapping at the platform level. However, we could add a relay mechanism to Docker to support this specific case with the semantics Docker expects.

I'm also thinking about whether we can add AF_UNIX support to Windows directly... then we could change Docker to bind to a Unix socket in Windows by default (as well as the named pipe, for compatibility reasons), and bind mounting this would be more natural and perhaps supportable in the platform. But that wouldn't be for LCOW v1.

Regarding two-pass regex, the second pass validation I've just now added to validateMountConfig so that it occurs even if you pass pre-parsed mounts via the API instead of via the raw (-v) field.

Agreed, the validateMountConfig is a better place to validate that.

@dhiltgen looks like the named pipe / unix socket mapping we were counting on to make linux flavored ucp controller run on windows won't be so easy then... We'll still have to rely on tcp/tls to connect to the daemon

@simonferquel well right now the UCP agent is a windows container, and that can have the daemon pipe mounted in as a pipe.

simonferquel · 2017-08-01T09:10:47Z

volume/volume_test.go

add failing cases for \.\pipe\foo:c:\data and c:\windows:\.\pipe\foo

Good idea. Done.

lowenna · 2017-08-01T23:01:16Z

Just realized, HCSShim should probably be v0.5.28 rather than .26 (latest pre-Sirupsen), or v0.6.1 if #34272 gets merged first (which is imminent).

lowenna · 2017-08-02T02:23:04Z

34272 is merged and HCSShim v0.6.1 is currently in moby/moby master. You can just drop that commit now, and all should be good.

simonferquel

LGTM

jstarks · 2017-08-03T17:42:53Z

Fixed gofmt issue that was blocking janky.

Current insider builds of Windows have support for mounting individual named pipe servers from the host to the guest. This allows, for example, exposing the docker engine's named pipe to a container. This change allows the user to request such a mount via the normal bind mount syntax in the CLI: docker run -v \\.\pipe\docker_engine:\\.\pipe\docker_engine <args> Signed-off-by: John Starks <jostarks@microsoft.com>

jstarks · 2017-08-07T18:34:59Z

Re-pushed to resolve merge conflicts

lowenna · 2017-08-07T18:38:06Z

Moving to merge when green CI

yongtang · 2017-08-07T23:54:02Z

All Jenkins tests passed. Merging.

GordonTheTurtle added the status/0-triage label Jun 27, 2017

jstarks force-pushed the win_named_pipes branch from 6f3e795 to a71d364 Compare June 27, 2017 19:46

lowenna reviewed Jun 27, 2017

View reviewed changes

jstarks force-pushed the win_named_pipes branch from a71d364 to 864c58e Compare June 27, 2017 20:51

lowenna approved these changes Jun 27, 2017

View reviewed changes

lowenna added status/2-code-review and removed status/0-triage labels Jun 27, 2017

AkihiroSuda reviewed Jun 29, 2017

View reviewed changes

volume/validate.go Outdated

Copy link
Copy Markdown

Member

AkihiroSuda Jun 29, 2017

Choose a reason for hiding this comment

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

if mnt.ReadOnly { errExtraField("ReadOnly") }?

GordonTheTurtle assigned AkihiroSuda Jul 12, 2017

johnstep approved these changes Jul 18, 2017

View reviewed changes

jstarks force-pushed the win_named_pipes branch 2 times, most recently from 0318e48 to 4748196 Compare July 28, 2017 01:59

AkihiroSuda reviewed Jul 28, 2017

View reviewed changes

jstarks force-pushed the win_named_pipes branch from 4748196 to 9ddef6f Compare July 31, 2017 21:24

simonferquel suggested changes Aug 1, 2017

View reviewed changes

jstarks force-pushed the win_named_pipes branch from 9ddef6f to 69d6949 Compare August 1, 2017 17:53

jstarks force-pushed the win_named_pipes branch from 69d6949 to b1db3e0 Compare August 2, 2017 18:05

simonferquel approved these changes Aug 3, 2017

View reviewed changes

jstarks force-pushed the win_named_pipes branch from b1db3e0 to dd35bc5 Compare August 3, 2017 17:42

thaJeztah mentioned this pull request Aug 6, 2017

[Feature Request] Support for Docker Engine managed plugins on Windows #34410

Open

yongtang added the rebuild/z label Aug 6, 2017

GordonTheTurtle removed the rebuild/z label Aug 6, 2017

jstarks force-pushed the win_named_pipes branch from dd35bc5 to 54354db Compare August 7, 2017 18:34

lowenna added status/4-merge and removed status/2-code-review labels Aug 7, 2017

yongtang added the rebuild/windowsRS1 label Aug 7, 2017

GordonTheTurtle removed the rebuild/windowsRS1 label Aug 7, 2017

yongtang merged commit 202cf00 into moby:master Aug 7, 2017

This was referenced Sep 7, 2017

Install Docker 17.09 RC StefanScherer/insider-docker-machine#1

Merged

Allow mapping of Windows named pipe into Windows container docker/compose#5181

Closed

Windows: Support named pipe mounts in docker service create + stack yml #34795

Closed

thaJeztah added the impact/changelog label Sep 16, 2017

thaJeztah mentioned this pull request Sep 25, 2017

tcp support for docker volume plugin v2 (not only unix socket) #32754

Open

This was referenced Jul 6, 2018

Support for Windows named pipes mounts moby/swarmkit#2691

Merged

Allow mount type npipe on service/stack #37400

Merged

Is there a roadmap for docker swarm? moby/swarmkit#2665

Open

mclapinski mentioned this pull request Aug 1, 2018

Document describing contrail ansible deployer for windows design Juniper/contrail-windows-docs#28

Merged

olljanat mentioned this pull request Sep 26, 2018

Allow npipe volume type on stack file docker/cli#1195

Merged

Conversation

jstarks commented Jun 27, 2017

Uh oh!

jstarks commented Jun 27, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lowenna commented Jun 27, 2017

Uh oh!

lowenna left a comment

Choose a reason for hiding this comment

Uh oh!

lowenna Jun 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AkihiroSuda commented Jun 29, 2017

Uh oh!

AkihiroSuda commented Jul 12, 2017

Uh oh!

johnstep left a comment

Choose a reason for hiding this comment

Uh oh!

johnstep commented Jul 24, 2017

Uh oh!

jstarks commented Jul 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lowenna commented Aug 1, 2017

Uh oh!

lowenna commented Aug 2, 2017

lowenna Jun 27, 2017 •

edited

Loading