Support for Windows named pipes mounts #2691

olljanat · 2018-07-06T20:49:29Z

Support for Windows named pipes mounts

- What I did
Added support for mount type npipe same way than it is added for engine on: moby/moby#33852

- How I did it
Added protocol NPIPE and command line switch --npipe

- How to test it
Build on Windows:

mkdir bin
$env:GOOS="windows"
$env:GOARCH="amd64"
go build -o .\bin\swarmd.exe .\cmd\swarmd\main.go
go build -o .\bin\swarmctl.exe .\cmd\swarmctl\main.go

Deploy my test version of Portainer using command:

.\swarmctl.exe service create --name portainer --npipe \\.\pipe\docker_engine:\\.\pipe\docker_engine --ports 9000:9000 --image ollijanatuinen/portainer:windows1803-amd64-npipe-3

Check using docker ps which port container actually is running (some reason source port is randomly selected), connect it from another machine (because on Windows limit of network connections) and test that Portainer can connect to local docker (environment type local and Windows containers which on).

Pre-requirement for moby/moby#37400 to be able to fix moby/moby#34795

- Description for the changelog

502d108 => Added type NPIPE to be able to use it with SwarmKit
3ece1d6 => Added Windows specific default settings for swarmd to be able to run tests on Windows.
b37c210 => Corrected --npipe parameter parsing.

dperny · 2018-07-06T20:58:22Z

i'm not sure how, but the proto messages have gotten messed up. you'll need to re-run make generate. if make generate produces identical output, i think your version of protobuild may be out-of-date, in which case re-running make setup should get you up-to-date.

olljanat · 2018-07-06T22:56:34Z

@dperny issue looks to be 3.0 version of protobuf which comes using apt-get install protobuf-compiler
It started to working after I manually installed same version than there is on circleci config.

Dockerfile which creates build/test environment with correct versions of tools would be nice.

There looks to be still some other issues too. I will investigate more them on tomorrow/next week.

Signed-off-by: Olli Janatuinen <olli.janatuinen@gmail.com>

codecov · 2018-07-07T08:13:34Z

Codecov Report

Merging #2691 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2691      +/-   ##
==========================================
- Coverage   61.84%   61.83%   -0.02%     
==========================================
  Files         134      134              
  Lines       21764    21766       +2     
==========================================
- Hits        13461    13460       -1     
- Misses       6853     6862       +9     
+ Partials     1450     1444       -6

thaJeztah · 2018-07-07T12:16:32Z

cmd/swarmctl/service/flagparser/npipe.go

+		container := spec.Task.GetContainer()
+
+		for _, npipe := range npipes {
+			if strings.Contains(npipe, ":") {


@johnstep @cpuguy83 should we have more/other validation here? (don't know if there's a utility somewhere to validate these)

@thaJeztah good question, there looks to be quite a lot validations on engine side already. Is there is any point to duplicate them to here?

Signed-off-by: Olli Janatuinen <olli.janatuinen@gmail.com>

olljanat · 2018-07-07T21:57:44Z

This looked to be much bigger task than I expected so it can take while before I'm sure that PR is OK (even from my point of view).

Anyway, after my second commit it is now possible to build and run swarmd on Windows. Main functionality was added on a1c8923 but default settings was incorrect.

Build on Windows can be done using following commands:

mkdir bin
$env:GOOS="windows"
$env:GOARCH="amd64"
go build -o .\bin\swarmd.exe .\cmd\swarmd\main.go
go build -o .\bin\swarmctl.exe .\cmd\swarmctl\main.go

And after that you can just start swarmd without any extra parameters:

bin\swarmd.exe

Maybe we should also add Windows CI builds to here?

Signed-off-by: Olli Janatuinen <olli.janatuinen@gmail.com>

olljanat · 2018-07-08T14:49:41Z

OK. I got it working on my env now and updated testing information to first post.

Please, review.

dperny · 2018-07-09T17:57:31Z

@olljanat i have a PR out right now for a dockerfile actually, right here: #2687. feel free to take that for a spin and let me know if it does the job for you.

olljanat · 2018-07-11T14:52:03Z

@thaJeztah I changed that one test which you picked up and now this looks working correctly. First post contains all info how to test. Can you review or ask someone else do so?

cpuguy83 · 2018-07-12T22:36:12Z

This looks OK.
I wonder if we should add a node filter for supported mount types on a node (just as a thought separate from this proposal).

GordonTheTurtle · 2018-07-13T13:57:43Z

Please sign your commits following these rules:
https://github.com/moby/moby/blob/master/CONTRIBUTING.md#sign-your-work
The easiest way to do this is to amend the last commit:

$ git clone -b "34795-npipe-mount-type" git@github.com:olljanat/swarmkit.git somewhere
$ cd somewhere
$ git rebase -i HEAD~842354049336
editor opens
change each 'pick' to 'edit'
save the file and quit
$ git commit --amend -s --no-edit
$ git rebase --continue # and repeat the amend for each commit
$ git push -f

Amending updates the existing PR. You DO NOT need to open a new one.

Signed-off-by: Olli Janatuinen <olli.janatuinen@gmail.com>

olljanat · 2018-08-01T05:15:15Z

@anshulpundir @cyli I can see that you have been active here on lately. Can I ask you review this one too or ping someone who can?

Portainer.io recently introduced Windows containers support using named pipe and it would be very nice to be able do deployment without extra tweaking (step 4 on this).

I will be also offline next three weeks starting from weekend so if this need changes I would like handle them still during this week.

cyli · 2018-08-01T20:55:23Z

The code LGTM - I'm not super familiar with the mount types and the docker PR itself - cc @johnstep possibly?

olljanat · 2018-08-02T15:48:59Z

Maybe @jhowardmsft also knows this stuff?

olljanat · 2018-08-22T19:51:45Z

ping @cyli @johnstep any possibility to get this one forward?

olljanat · 2018-08-28T13:02:30Z

@thaJeztah @dperny can you review this one?

dperny · 2018-08-28T16:31:19Z

LGTM now. I believe you will also need to update the executor in moby/moby to have this work all the way through.

olljanat · 2018-08-28T16:49:58Z

@dperny This worked fine with moby/moby without executor changes when I tested it on July but I will of course test it again after this on is merged and updated to there.

Who can do merging?

Vacant0mens · 2018-08-31T20:46:29Z

Any update on this? I would really love to get service discovery going by reading the default npipe so that i don't have to map docker to a TCP port.

olljanat · 2018-09-03T06:23:09Z

@thaJeztah @cyli @dperny @anshulpundir there is two "LGTM" comments here already so can someone merge this one?

thaJeztah · 2018-09-06T09:40:20Z

I don't have permissions to merge. @anshulpundir @dperny ok to merge this one?

This also brings in these PRs from swarmkit: - moby/swarmkit#2691 - moby/swarmkit#2744 - moby/swarmkit#2732 - moby/swarmkit#2729 - moby/swarmkit#2748 Signed-off-by: Tibor Vass <tibor@docker.com>

This also brings in these PRs from swarmkit: - moby/swarmkit#2691 - moby/swarmkit#2744 - moby/swarmkit#2732 - moby/swarmkit#2729 - moby/swarmkit#2748 Signed-off-by: Tibor Vass <tibor@docker.com> Upstream-commit: cce1763d57b5c8fc446b0863517bb5313e7e53be Component: engine

ggruber4711 · 2018-10-20T12:45:36Z

When will this feature be available in Docker-ee for windows?

olljanat · 2018-10-20T13:09:43Z

@ggruber4711 as far I have understood this will be available for Docker CE on 19.03 and hopefully quite soon after that also Docker EE version will be released.

Docker EE 18.03.1 was released three months after Docker CE 18.03.0 so I guess that it will be some where on next June.

@thaJeztah any comments?

thaJeztah · 2018-10-22T12:16:06Z

Yes, this didn't make it for the 18.09 release, so will be in the release after that (19.03).

and hopefully quite soon after that also Docker EE version will be released.

Going forward, Docker CE and Docker EE will be released in sync again, so they should be released at the same time

Vacant0mens · 2019-07-19T15:08:41Z

@thaJeztah Is there a short and long syntax for this? or is it only allowed in short syntax for Stacks? Volumes aren't described very in-depth in the Stack File documentation (as compared to the Compose File documentation).
Also, I see this was added to the 19.03 release, is the EE version 19.03.0-rc2 included as well, or is it just CE so far?

olljanat · 2019-07-19T15:21:48Z

@Vacant0mens only long syntax works. Look example on moby/moby#37400 (comment)

Both CE and EE will have same features on 19.03 so this will be included to both of them.

Vacant0mens · 2019-07-19T15:41:07Z

Thanks @olljanat . Is there a scheduled release date for 19.03 for EE? I saw earlier that CE was already released, just wanted to know what to expect. I'd like to get off the -rc2 version as soon as I can.

Vacant0mens · 2019-07-19T16:24:40Z

@olljanat
Looks like the documentation for Compose and for Stacks will need to be updated to reflect your example.

Also, I'm not sure if this is a bug or not, but it seems that the npipe volume mapping is incompatible when used along side other volumes or volume types.

version: '3.7'
services:
  portainer:
    image: portainer/portainer:windows1809-amd64
    ports:
      - 9000:9000
    volumes:
      - E:\Portainer:C:\data
      - type: npipe
        source: \\.\pipe\docker_engine
        target: \\.\pipe\docker_engine

If I docker stack deploy this file, I get the error starting container failed: hcsshim::CreateComputeSystem [guid]: The request is not supported. . But if I remove the line - E:\Portainer:C:\data , it deploys and starts the service with no issues. I get the same error if I use the long syntax with - type: bind.

olljanat · 2019-07-19T16:32:35Z

@Vacant0mens are you sure that folder E:\Portainer exists and that it works without npipe line? If so then please report it as new issue to moby/moby.

Docs update PR is open on docker/docs#7427

Vacant0mens · 2019-07-19T16:43:58Z

Yes, I'm sure because this works:

docker run -d -p 9000:9000 --name portainer --restart always -v \\.\pipe\docker_engine:\\.\pipe\docker_engine -v E:\Portainer:C:\data portainer/portainer:windows1809-amd64

I'll open a new issue as you said.

olljanat mentioned this pull request Jul 6, 2018

Allow mount type npipe on service/stack moby/moby#37400

Merged

olljanat changed the title ~~Support for Windows named pipes~~ WIP: Support for Windows named pipes Jul 7, 2018

Support for Windows named pipes

502d108

Signed-off-by: Olli Janatuinen <olli.janatuinen@gmail.com>

olljanat force-pushed the 34795-npipe-mount-type branch from 8fee694 to 502d108 Compare July 7, 2018 08:03

thaJeztah reviewed Jul 7, 2018

View reviewed changes

olljanat force-pushed the 34795-npipe-mount-type branch from 3ece1d6 to 580483a Compare July 7, 2018 21:37

Corrected swarmd default settings on Windows

990bcf3

Signed-off-by: Olli Janatuinen <olli.janatuinen@gmail.com>

olljanat force-pushed the 34795-npipe-mount-type branch from 580483a to 990bcf3 Compare July 7, 2018 21:47

Corrected --npipe parameter parsing

b37c210

Signed-off-by: Olli Janatuinen <olli.janatuinen@gmail.com>

olljanat changed the title ~~WIP: Support for Windows named pipes~~ Support for Windows named pipes Jul 8, 2018

olljanat changed the title ~~Support for Windows named pipes~~ Support for Windows named pipes mounts Jul 9, 2018

GordonTheTurtle added the dco/no label Jul 13, 2018

Fixed merge conflicts

85277de

Signed-off-by: Olli Janatuinen <olli.janatuinen@gmail.com>

olljanat force-pushed the 34795-npipe-mount-type branch from 29b5f76 to 85277de Compare July 13, 2018 14:01

GordonTheTurtle removed the dco/no label Jul 13, 2018

olljanat mentioned this pull request Jul 31, 2018

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

Closed

dperny merged commit a5a4101 into moby:master Sep 12, 2018

tiborvass mentioned this pull request Sep 22, 2018

[18.09] Remove boltdb dependency docker/engine#60

Merged

olljanat deleted the 34795-npipe-mount-type branch September 30, 2018 12:17

deviantony mentioned this pull request Nov 13, 2018

Portainer agent on Windows Server 1803 portainer/portainer#2463

Closed

olljanat mentioned this pull request Jan 2, 2019

Use official golang images for windows moby/moby#38410

Open

deviantony mentioned this pull request Jan 25, 2019

[Doubt] Stack File and npipe \\.\pipe\docker_engine portainer/portainer#2649

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for Windows named pipes mounts #2691

Support for Windows named pipes mounts #2691

olljanat commented Jul 6, 2018 •

edited

dperny commented Jul 6, 2018

olljanat commented Jul 6, 2018

codecov bot commented Jul 7, 2018 •

edited

thaJeztah Jul 7, 2018

olljanat Jul 7, 2018 •

edited

olljanat commented Jul 7, 2018

olljanat commented Jul 8, 2018

dperny commented Jul 9, 2018

olljanat commented Jul 11, 2018

cpuguy83 commented Jul 12, 2018

GordonTheTurtle commented Jul 13, 2018

olljanat commented Aug 1, 2018

cyli commented Aug 1, 2018

olljanat commented Aug 2, 2018

olljanat commented Aug 22, 2018

olljanat commented Aug 28, 2018

dperny commented Aug 28, 2018

olljanat commented Aug 28, 2018

Vacant0mens commented Aug 31, 2018

olljanat commented Sep 3, 2018

thaJeztah commented Sep 6, 2018

ggruber4711 commented Oct 20, 2018

olljanat commented Oct 20, 2018

thaJeztah commented Oct 22, 2018

Vacant0mens commented Jul 19, 2019

olljanat commented Jul 19, 2019

Vacant0mens commented Jul 19, 2019 •

edited

Vacant0mens commented Jul 19, 2019

olljanat commented Jul 19, 2019

Vacant0mens commented Jul 19, 2019

Support for Windows named pipes mounts #2691

Support for Windows named pipes mounts #2691

Conversation

olljanat commented Jul 6, 2018 • edited

Support for Windows named pipes mounts

dperny commented Jul 6, 2018

olljanat commented Jul 6, 2018

codecov bot commented Jul 7, 2018 • edited

Codecov Report

thaJeztah Jul 7, 2018

Choose a reason for hiding this comment

olljanat Jul 7, 2018 • edited

Choose a reason for hiding this comment

olljanat commented Jul 7, 2018

olljanat commented Jul 8, 2018

dperny commented Jul 9, 2018

olljanat commented Jul 11, 2018

cpuguy83 commented Jul 12, 2018

GordonTheTurtle commented Jul 13, 2018

olljanat commented Aug 1, 2018

cyli commented Aug 1, 2018

olljanat commented Aug 2, 2018

olljanat commented Aug 22, 2018

olljanat commented Aug 28, 2018

dperny commented Aug 28, 2018

olljanat commented Aug 28, 2018

Vacant0mens commented Aug 31, 2018

olljanat commented Sep 3, 2018

thaJeztah commented Sep 6, 2018

ggruber4711 commented Oct 20, 2018

olljanat commented Oct 20, 2018

thaJeztah commented Oct 22, 2018

Vacant0mens commented Jul 19, 2019

olljanat commented Jul 19, 2019

Vacant0mens commented Jul 19, 2019 • edited

Vacant0mens commented Jul 19, 2019

olljanat commented Jul 19, 2019

Vacant0mens commented Jul 19, 2019

olljanat commented Jul 6, 2018 •

edited

codecov bot commented Jul 7, 2018 •

edited

olljanat Jul 7, 2018 •

edited

Vacant0mens commented Jul 19, 2019 •

edited