support windows named pipe by chanwit · Pull Request #941 · moby/swarmkit

chanwit · 2016-06-12T16:24:23Z

This PR adds Windows named pipe support for SwarmKit to allow running a Worker on Windows and also forming a cluster. Instead of listening a Unix socket, this PR uses Microsoft's go-winio to start a named pipe server.

Tested on a Windows box:

swarmd -l debug -d c:/tmp/node1 --listen-control-api npipe:////./pipe/swarmd.sock --hostname node1 --listen-remote-api 127.0.0.1:4242 --engine-addr <REMOTE DOCKER_HOST>:2375

Then
set SWARM_SOCKET=npipe:////./pipe/swarmd.sock

swarmctl task ls hello_world
ID                         Service        Desired State  Last State              Node
--                         -------        -------------  ----------              ----
33fufdzwywh8p5u9vl8cidy99  hello_world.2  RUNNING        RUNNING 22 minutes ago  node1
ejoseszph4xukbp8e9wkggibt  hello_world.1  RUNNING        RUNNING 22 minutes ago  node1

Updated: to make parsing different kind of protocols (unix, tcp, npipe) being more deterministic, I changed the default sock from /var/... to unix:///var/....

chanwit · 2016-06-12T17:12:45Z

Test passed locally (with make coverage) but not sure why the CircleCI failed.

icecrime · 2016-06-12T17:53:41Z

👍

stevvooe · 2016-06-12T23:44:27Z

xnet/util.go

+	logrus.Debugf("parse proto addr: %s", s)
+	parts := strings.SplitN(s, "://", 2)
+	if len(parts) == 1 {
+		_, _, err := net.SplitHostPort(parts[0])


How can we assume that an error here means we have a unix address?

Signed-off-by: Chanwit Kaewkasi <chanwit@gmail.com>

chanwit · 2016-06-13T14:47:03Z

Thank you @stevvooe I reverted some changes and did a proper error handling.
Could you please review again?

lowenna · 2016-06-23T16:32:16Z

@chanwit Are you planning on moving this across to the engine? If not, I'm happy to do it. Thanks :)

chanwit · 2016-06-23T16:57:36Z

@jhowardmsft please do it! Let me know when you start the PR, I'll join you there.

TheJayMann · 2016-07-06T00:57:11Z

I managed to mostly port this to the engine, as shown in the patch I posted on moby/moby#24189. One issue I found with the code in your pull request is that you are passing an empty string as the second parameter to winio.ListenPipe, but when I attempted to compile this, it failed, as it expected a winio.PipeConfig to be passed instead. I attempted to create a PipeConfig based on another example in the engine. Ultimately, I was able to get the daemon to listed to the pipe, but, when creating a cluster, the client hangs, and when connecting a second client, the node sees itself as a manager, but, when listing nodes, lists itself in a down state. My guess is that I used an incorrect security descriptor, but I really don't have much of a clue. I figured I'd mention this here, as the last few comments mention porting this to the engine.

mwieczorek · 2016-08-04T07:17:08Z

@stevvooe any plan to merge or close this PR? is there anything that can be improved?

stevvooe · 2016-08-04T20:58:59Z

agent/node.go

 				ProtoAddr: map[string]string{
-					"tcp":  n.config.ListenRemoteAPI,
-					"unix": n.config.ListenControlAPI,
+					"remote":  n.config.ListenRemoteAPI,


This doesn't look right. The whole ProtoAddr concept here is a little weird. Both APIs should be mountable to unix or tcp. This makes the mistake of separating control and remote, which is only an implementation detail of docker.

stevvooe · 2016-08-04T20:59:36Z

@mwieczorek It will probably need a rebase.

I think the biggest thing that such a PR needs is to ensure that the APIs can be mounted via any listener. I'm not sure if we want to carry windows support -- ideally that would be handled via downstream projects. It also looks like the concept of control and remote being separated is a little hardcoded -- this should be a lot more flexible in practice.

aaronlehmann · 2016-08-04T22:49:10Z

It also looks like the concept of control and remote being separated is a little hardcoded -- this should be a lot more flexible in practice.

To be fair, I don't think this is at all specific to this PR. The ProtoAddr stuff is part of the existing codebase. It's designed this way because there was a hard requirement that the control API should never be exposed in an unauthenticated way to the outside world.

stevvooe · 2016-08-05T19:59:01Z

It's designed this way because there was a hard requirement that the control API should never be exposed in an unauthenticated way to the outside world.

The policy of what is and isn't exposed belongs in docker, not swarmkit.

If we look at the, some really bad things are going on here:

                ProtoAddr: map[string]string{
 -                  "tcp":  n.config.ListenRemoteAPI,
 -                  "unix": n.config.ListenControlAPI,
 +                  "remote":  n.config.ListenRemoteAPI,
 +                  "control": n.config.ListenControlAPI,
                },

SwarmKit should provide components that can be served. The level of coupling is dangerous and limits the ultimate usefulness of the project (granted, grpc suffers from this, as well).

We may want to expose the GRPC some day, and that should be done by changing the listener for that part of the interface. If we intertwine this into a mess of spaghetti, we are just hurting ourselves in the future.

aluzzardi · 2016-08-23T23:31:06Z

@chanwit @jhowardmsft What are we missing to move this forward? Windows support would be awesome.

lowenna · 2016-08-24T02:52:51Z

Time ;) I'm on vacation and when back still have a pile of other things before I could start looking at this.

aluzzardi · 2016-08-25T18:23:23Z

@chanwit Could you please rebase?

I think the engine changes are going to be small - we can carry them over if you don't have the time to do them.

@aaronlehmann @stevvooe: Any objection in merging this? I see your comments @stevvooe but I think those are unrelated from this PR - those problems were already existing and this PR just adds named pipe support.

/cc @friism @icecrime

aaronlehmann · 2016-08-25T20:32:28Z

I suggest not changing the ProtoAddr keys to control/remote. Instead, add support for a npipe key.

One reason for this is that moving the protocol from the key to the value is effectively an API change - it requires ListenRemoteAPI to be prefixed with tcp://.

TheJayMann · 2016-08-25T21:24:46Z

@aluzzardi I must also agree with @aaronlehmann regarding changing control/remote back to unix/tcp and adding npipe, as when I attempted earlier to take these changes and apply them to swarmkit in docker, I got issues where at some parts, the tcp:// prefix was not being removed. I was going to make this change myself and reapply it to swarmkit in docker, but hadn't the time to do so.

chanwit · 2016-08-26T01:58:35Z

@aluzzardi I'll rebase it and be taking those comments into account

Thank you for all comments. I got some pictures of how to proceed

aluzzardi · 2016-08-27T01:30:50Z

Thanks @chanwit !

aluzzardi · 2016-09-09T22:37:29Z

Hey @chanwit, any news regarding this?

No pressure whatsoever but since we'd like to have Windows support we were considering having someone over here carry over the work.

However, before doing that I just want to make sure we are not duplicating efforts.

chanwit · 2016-09-13T05:27:42Z

@aluzzardi sorry for being late. I'm eagerly hacking it.
Please name the date and I'll try my best to being on the schedule.

aluzzardi · 2016-09-17T01:19:49Z

@chanwit No worries! Just wanted to avoid duplicating efforts if we got someone to carry over the changes. Take your time :)

We'd like to get support for 1.13

JMesser81 · 2016-10-11T22:35:51Z

Any updates on this PR? would love to get this merged so that Windows can be one step closer for supporting Swarm

chanwit · 2016-10-12T14:33:08Z

@JMesser81 I'm still working on this one. As the milestone stated, it will be available by 1.13.

Integrate xnet package into the modern code xnet package was authored by Chanwit Kaewkasi <chanwit@gmail.com> Integration is largely also based his work PR moby#941. This is a separate commit because a rebase was prohibitively difficult Signed-off-by: Drew Erny <drew.erny@docker.com>

aaronlehmann · 2016-10-27T18:52:44Z

Closing this, as the windows named pipe functionality was implemented by #1654.

TheJayMann · 2016-10-28T03:10:37Z

I really want to be able to try this out with Docker's built in swarm mode. From what I can tell, every so often, the Docker repository does a "revendor." Any chance of knowing that repository is going to do this, or how I can do this myself to test it out?

friism · 2016-10-28T04:34:06Z

@TheJayMann since #1654 was merged, this functionality should now be in the master builds that you can download here: https://master.dockerproject.org/

There probably are some other gotchas, but ping me on michael.friis@docker.com if you get something working!

aaronlehmann · 2016-10-28T04:39:13Z

It will probably be a few days before it shows up in a master build. As @TheJayMann mentioned, we need to revendor.

There is also this PR that needs to be merged in the docker repository: moby/moby#27838

friism · 2016-10-28T05:08:44Z

argh, sorry - didn't realize this was on swarmkit

GordonTheTurtle added status/0-triage group/windows labels Jun 12, 2016

chanwit force-pushed the windows branch 2 times, most recently from caa60d7 to 7d25884 Compare June 12, 2016 16:42

stevvooe reviewed Jun 12, 2016
View reviewed changes

support Windows named pipe

f2cdfa0

Signed-off-by: Chanwit Kaewkasi <chanwit@gmail.com>

chanwit force-pushed the windows branch from 7d25884 to f2cdfa0 Compare June 13, 2016 14:43

tonistiigi mentioned this pull request Jun 29, 2016

Block new unsupported docker commands on Windows moby/moby#23680

Closed

friism mentioned this pull request Jun 30, 2016

Swarm mode on Windows moby/moby#24189

Closed

stevvooe reviewed Aug 4, 2016
View reviewed changes

aluzzardi self-assigned this Aug 25, 2016

dperny mentioned this pull request Oct 17, 2016

Support Windows Named Pipe REDUX #1654

Merged

aaronlehmann closed this Oct 27, 2016

Conversation

chanwit commented Jun 12, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chanwit commented Jun 12, 2016

Uh oh!

icecrime commented Jun 12, 2016

Uh oh!

stevvooe Jun 12, 2016

Choose a reason for hiding this comment

Uh oh!

chanwit commented Jun 13, 2016

Uh oh!

lowenna commented Jun 23, 2016

Uh oh!

chanwit commented Jun 23, 2016

Uh oh!

TheJayMann commented Jul 6, 2016

Uh oh!

mwieczorek commented Aug 4, 2016

Uh oh!

stevvooe Aug 4, 2016

Choose a reason for hiding this comment

Uh oh!

stevvooe commented Aug 4, 2016

Uh oh!

aaronlehmann commented Aug 4, 2016

Uh oh!

stevvooe commented Aug 5, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aluzzardi commented Aug 23, 2016

Uh oh!

lowenna commented Aug 24, 2016

Uh oh!

aluzzardi commented Aug 25, 2016

Uh oh!

aaronlehmann commented Aug 25, 2016

Uh oh!

TheJayMann commented Aug 25, 2016

Uh oh!

chanwit commented Aug 26, 2016

Uh oh!

aluzzardi commented Aug 27, 2016

Uh oh!

aluzzardi commented Sep 9, 2016

Uh oh!

chanwit commented Sep 13, 2016

Uh oh!

aluzzardi commented Sep 17, 2016

Uh oh!

JMesser81 commented Oct 11, 2016

Uh oh!

chanwit commented Oct 12, 2016

Uh oh!

aaronlehmann commented Oct 27, 2016

Uh oh!

TheJayMann commented Oct 28, 2016

Uh oh!

friism commented Oct 28, 2016

Uh oh!

aaronlehmann commented Oct 28, 2016

Uh oh!

friism commented Oct 28, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

chanwit commented Jun 12, 2016 •

edited

Loading

stevvooe commented Aug 5, 2016 •

edited

Loading