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: named pipe mounts #33852

Merged
merged 1 commit into from Aug 7, 2017

Conversation

@jstarks
Contributor

jstarks commented Jun 27, 2017

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

This comment has been minimized.

Show comment
Hide comment
@jstarks

jstarks Jun 27, 2017

Contributor

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

Contributor

jstarks commented Jun 27, 2017

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

Show outdated Hide outdated libcontainerd/client_windows.go Outdated
HostPath: mount.Source,
ContainerPipeName: mount.Destination[len(pipePrefix):],
}
mps = append(mps, mp)

This comment has been minimized.

@jhowardmsft

jhowardmsft Jun 27, 2017

Contributor

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

@jhowardmsft

jhowardmsft Jun 27, 2017

Contributor

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

This comment has been minimized.

@jstarks

jstarks Jun 27, 2017

Contributor

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

@jstarks

jstarks Jun 27, 2017

Contributor

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

This comment has been minimized.

@jhowardmsft

jhowardmsft Jun 27, 2017

Contributor

That's fair

@jhowardmsft

jhowardmsft Jun 27, 2017

Contributor

That's fair

Show outdated Hide outdated libcontainerd/client_windows.go Outdated
@jhowardmsft

This comment has been minimized.

Show comment
Hide comment
@jhowardmsft
Contributor

jhowardmsft commented Jun 27, 2017

@johnstep FYI

@jhowardmsft

LGTM

}
}
configuration.MappedDirectories = mds
if len(mps) > 0 && system.GetOSVersion().Build < 16210 { // replace with Win10 RS3 build number at RTM

This comment has been minimized.

@jhowardmsft

jhowardmsft Jun 27, 2017

Contributor

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

@jhowardmsft

jhowardmsft Jun 27, 2017

Contributor

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

This comment has been minimized.

@jstarks

jstarks Jun 27, 2017

Contributor

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.

@jstarks

jstarks Jun 27, 2017

Contributor

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.

if mnt.BindOptions != nil {
return &errMountConfig{mnt, errExtraField("BindOptions")}
}

This comment has been minimized.

@AkihiroSuda

AkihiroSuda Jun 29, 2017

Member

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

@AkihiroSuda

AkihiroSuda Jun 29, 2017

Member

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

@AkihiroSuda

This comment has been minimized.

Show comment
Hide comment
@AkihiroSuda

AkihiroSuda Jun 29, 2017

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

AkihiroSuda commented Jun 29, 2017

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

This comment has been minimized.

Show comment
Hide comment
@AkihiroSuda
Member

AkihiroSuda commented Jul 12, 2017

ping @jstarks

@johnstep

LGTM

@johnstep

This comment has been minimized.

Show comment
Hide comment
@johnstep

johnstep Jul 24, 2017

Contributor

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

Contributor

johnstep commented Jul 24, 2017

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

@jstarks

This comment has been minimized.

Show comment
Hide comment
@jstarks

jstarks Jul 28, 2017

Contributor

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.

Contributor

jstarks commented Jul 28, 2017

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.

@@ -30,10 +28,8 @@ func validateMountConfig(mnt *mount.Mount, options ...func(*validateOpts)) error
return &errMountConfig{mnt, err}
}
if !opts.skipAbsolutePathCheck {

This comment has been minimized.

@simonferquel

simonferquel Aug 1, 2017

Contributor

Hmm, should'nt we honor the skipAbsolutePathCheck option ?

@simonferquel

simonferquel Aug 1, 2017

Contributor

Hmm, should'nt we honor the skipAbsolutePathCheck option ?

This comment has been minimized.

@jstarks

jstarks Aug 1, 2017

Contributor

This option is obsolete. I removed it.

@jstarks

jstarks Aug 1, 2017

Contributor

This option is obsolete. I removed it.

// RXReservedNames are reserved names not possible on Windows
RXReservedNames = `(con)|(prn)|(nul)|(aux)|(com[1-9])|(lpt[1-9])`
// RXSource is the combined possibilities for a source
RXSource = `((?P<source>((` + RXHostDir + `)|(` + RXName + `))):)?`
RXSource = `((?P<source>((` + RXHostDir + `)|(` + RXName + `)|(` + RXPipe + `))):)?`

This comment has been minimized.

@simonferquel

simonferquel Aug 1, 2017

Contributor

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)

@simonferquel

simonferquel Aug 1, 2017

Contributor

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)

This comment has been minimized.

@simonferquel

simonferquel Aug 1, 2017

Contributor

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)

@simonferquel

simonferquel Aug 1, 2017

Contributor

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)

This comment has been minimized.

@jstarks

jstarks Aug 1, 2017

Contributor

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.

@jstarks

jstarks Aug 1, 2017

Contributor

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.

This comment has been minimized.

@jstarks

jstarks Aug 1, 2017

Contributor

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.

@jstarks

jstarks Aug 1, 2017

Contributor

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.

This comment has been minimized.

@simonferquel

simonferquel Aug 2, 2017

Contributor

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

simonferquel Aug 2, 2017

Contributor

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

This comment has been minimized.

@friism

friism Aug 2, 2017

Contributor

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

@friism

friism Aug 2, 2017

Contributor

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

Show outdated Hide outdated volume/volume_test.go Outdated
@jhowardmsft

This comment has been minimized.

Show comment
Hide comment
@jhowardmsft

jhowardmsft Aug 1, 2017

Contributor

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

Contributor

jhowardmsft commented Aug 1, 2017

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

@jhowardmsft

This comment has been minimized.

Show comment
Hide comment
@jhowardmsft

jhowardmsft Aug 2, 2017

Contributor

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.

Contributor

jhowardmsft commented Aug 2, 2017

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

This comment has been minimized.

Show comment
Hide comment
@jstarks

jstarks Aug 3, 2017

Contributor

Fixed gofmt issue that was blocking janky.

Contributor

jstarks commented Aug 3, 2017

Fixed gofmt issue that was blocking janky.

Windows: Add named pipe mount support
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

This comment has been minimized.

Show comment
Hide comment
@jstarks

jstarks Aug 7, 2017

Contributor

Re-pushed to resolve merge conflicts

Contributor

jstarks commented Aug 7, 2017

Re-pushed to resolve merge conflicts

@jhowardmsft

This comment has been minimized.

Show comment
Hide comment
@jhowardmsft

jhowardmsft Aug 7, 2017

Contributor

Moving to merge when green CI

Contributor

jhowardmsft commented Aug 7, 2017

Moving to merge when green CI

@yongtang

This comment has been minimized.

Show comment
Hide comment
@yongtang

yongtang Aug 7, 2017

Member

All Jenkins tests passed. Merging.

Member

yongtang commented Aug 7, 2017

All Jenkins tests passed. Merging.

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

6 checks passed

dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 36137 has succeeded
Details
janky Jenkins build Docker-PRs 44756 has succeeded
Details
powerpc Jenkins build Docker-PRs-powerpc 5139 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 16157 has succeeded
Details
z Jenkins build Docker-PRs-s390x 4872 has succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment