Windows libnetwork integration #20478

msabansal · 2016-02-19T04:52:26Z

This commit brings in the initial version of the windows libnetwork integration for networking.

Most of the code is common between the Linux and windows implementation so that has been pulled into common files.
TP4 builds don't have support for HNS so they will continue to work with the older model (os version check is performed in initNetcontroller in daemon_windows.go)
We don't support hot add or removal of network devices so the implementation is missing with proper error being returned in those cases
The default network mode in Windows is "NAT" and not "bridge". Some of the common libnetwork code relied on the name being "bridge". I have updated that to query the DefaultDaemonNetworkMode
The engine-api changes are still pending to be merged so I have vendored my branch temporarily
HNS (windows networking service) maintains the networks and we query and add the networks from HNS at daemon startup.
Nic addition is not performed by libnetwork as in the case of Linux. This work is done by HNS and that information is passed as part of config to HCS at container start.
I have added a build script to build only windows. It was useful for me to create only windows builds (I can remove that if you think it is not required)

thaJeztah · 2016-02-19T08:28:47Z

Related engine-API PR is here docker/engine-api#88

thaJeztah · 2016-02-23T20:55:49Z

@msabansal looks like you need to git rebase in stead of merge, so that you won't get a merge commit in your PR. If you need more info; have a look at our contributors guide

thaJeztah · 2016-02-23T20:56:55Z

also;

ping @mavenugo if you have time to check if this LGTY

msabansal · 2016-02-24T01:15:30Z

@thaJeztah Thanks. Done

lowenna · 2016-02-24T03:15:34Z

daemon/execdriver/driver_windows.go

@@ -56,6 +56,7 @@ type Command struct {
 	Isolation   string   `json:"isolation"`    // Isolation technology for the container
 	ArgsEscaped bool     `json:"args_escaped"` // True if args are already escaped
 	HvPartition bool     `json:"hv_partition"` // True if it's an hypervisor partition
+	EpList      []string `json:"endpoints"`    // List of network endpoitns for HNS


Typo 'endpoitns'

mavenugo · 2016-02-26T14:57:51Z

@msabansal @thaJeztah will get to this PR soon.

thaJeztah · 2016-02-29T13:10:08Z

@msabansal looks like this needs a rebase 😢

msabansal · 2016-03-01T02:50:55Z

@thaJeztah done.

How can we restart the experimental & windowsTP4 tests? They seem to failing for some configuration issues and look to be transient errors

cpuguy83 · 2016-03-01T03:03:04Z

@msabansal done.

msabansal · 2016-03-01T03:19:57Z

@cpuguy83 :) These tests will take some effort to pass

lowenna · 2016-03-03T05:38:25Z

Screenshot of CI passing, complete with timestamp, included for posterity... Jenkins flakiness has been troublesome for getting a clean run, but that's being worked on independently by multiple people.

@icecrime @calavera @cpuguy83 @tiborvass @thaJeztah @powersplay @friism Can we please get this moved to code review? This is a super critical PR to get merged ASAP in support of the HNS/libnetwork integration for TP5. All issues have been resolved - it LGTM from my review.

@mavenugo @mrjana - Madhu/Jana, please can you code review as a priority?

thaJeztah · 2016-03-03T09:27:37Z

ping @mavenugo PTAL

vdemeester · 2016-03-03T09:51:44Z

It's not completely related to the PR but I feel like container.go and container_operations.go are getting quite big and the changes moved back from container_unix.go could go to a container_network.go (same for container_network_operations.go.

mavenugo · 2016-03-03T13:11:07Z

container/container.go

+	return nil
+}
+
+func getEndpointPortMapInfo(ep libnetwork.Endpoint) (nat.PortMap, error) {


FYI. There is a design change happening on this one via moby/libnetwork#810.
As you can see in the review, we are trying to have no impact to existing functionalities or drivers.
I will let @aboch review this piece of code and confirm.

Just as a note that all these changes to the port-mapping handling are just moving the code
from container_unix.go to container.go so that it is equally applicable to windows platform as well.

So, that makes it a bit more easier.

@mavenugo I am not planning to make big changes to this code anymore, as we need to still send the exp ports and port mapping to the drivers during create endpoint.
And yes this PR's change does not affect that, as it is only moving the code to platform independent files.

mavenugo · 2016-03-03T13:22:06Z

@msabansal could you please confirm that the functions added back to container.go and container_operations.go are simply moving it from its *unix.go counterpart ?
Since that is the bulk of the work, I would like to get that confirmation and that would help a great deal with code-review.

calavera · 2016-03-03T16:53:07Z

bulk of these changes LGTM. I agree with @vdemeester that this is a perfect opportunity to structure those functions a little bit better.

could go to a container_network.go (same for container_network_operations.go)

I like this.

lowenna · 2016-03-03T18:06:31Z

@calavera @vdemeester Can I suggest the restructuring comes in a follow-up PR?

msabansal · 2016-03-03T18:13:34Z

@mavenugo @mrjana We do need libnetwork to be revendored again before this PR is merged(we renamed the windows drivers to be lowercase). Is there a plan to revendor it or should I work on getting that done?

mavenugo · 2016-03-03T18:26:26Z

@msabansal yes. we are planning on the vendoring shortly.

BTW, am +1 with @calavera & @vdemeester as well.

msabansal · 2016-03-04T02:51:14Z

@mavenugo Yes most of the code is just refactoring and moving it to the common location.

I have addressed the one concern we had and renamed Subnet to FixedCIDR

mavenugo · 2016-03-04T03:43:12Z

Thanks @msabansal. I don't have any other comment.

We just have to wait for the next libnetwork vendor in. We are planning on pulling in a bunch of fixes
(including the windows fixes) tomorrow.

BTW, am fine with doing the container_network.go code reshuffling in a follow up PR.

vdemeester · 2016-03-04T08:10:14Z

daemon/container_operations_unix.go

@@ -38,7 +33,7 @@ func (daemon *Daemon) setupLinkedContainers(container *container.Container) ([]s
 	var env []string
 	children := daemon.children(container)

-	bridgeSettings := container.NetworkSettings.Networks["bridge"]
+	bridgeSettings := container.NetworkSettings.Networks[runconfig.DefaultDaemonNetworkMode().NetworkName()]


Why not assigning runconfig.DefaultDaemonNetworkMode().NetworkName() to a var instead of calling it several time 👼

vdemeester · 2016-03-04T08:15:46Z

@jhowardmsft I would think it's not that big a deal to just move the piece of code brought back into non …_unix.go file, as a first simple state. But it could be done in a follow-up PR, that's true (just think it would make sense in this one).

Few nits comments, but overall I'm LGTM 🐰 too .

calavera · 2016-03-08T21:55:53Z

This depends on #21019 Please, do not merge until that PR is merged.

icecrime · 2016-03-10T03:41:07Z

@msabansal #21019 is now merged! Can you please rebase?

GordonTheTurtle · 2016-03-10T04:20:11Z

Please note that concept of execdrivers is being replaced with OCI compliant binaries executed through containerd. There is an ongoing effort for switching to containerd in #20662 . Please consider porting the changes in your PR to this branch instead.

Signed-off-by: msabansal <sabansal@microsoft.com>

lowenna · 2016-03-10T05:20:34Z

@msabansal Ignore the Gordon comment. I've asked Darren to talk to you about that, and will catch up internally if you have more questions. For other reviewers/maintainers, this change is completely independent of the containerd effort and should be merged into master in its own right to support progress on TP5 before the containerd work is complete.

thaJeztah · 2016-03-10T11:52:40Z

ping @calavera @vdemeester @mavenugo please review

mavenugo · 2016-03-10T12:09:05Z

bulk of the changes are simply code re-organizing to make most of the networking code relevant for windows. As agreed earlier in the thread, we can consolidate better, but that doesn't block this PR.

still LGTM.

lowenna · 2016-03-10T19:49:53Z

Is this good now?

icecrime · 2016-03-10T21:33:01Z

All good 🎉 Thanks!

Windows libnetwork integration

msabansal · 2016-03-10T21:54:08Z

Thanks :)

thaJeztah · 2016-03-10T23:51:44Z

Congrats on your first PR merged in Docker @msabansal !

msabansal · 2016-03-10T23:54:12Z

@thaJeztah Thanks :)

GordonTheTurtle added platform/windows status/0-triage labels Feb 19, 2016

calavera added status/1-design-review area/networking and removed status/0-triage labels Feb 19, 2016

GordonTheTurtle added the dco/no Automatically set by a bot when one of the commits lacks proper signature label Feb 23, 2016

GordonTheTurtle removed the dco/no Automatically set by a bot when one of the commits lacks proper signature label Feb 23, 2016

lowenna reviewed Feb 24, 2016
View reviewed changes

msabansal mentioned this pull request Feb 25, 2016

Removing dead code duplicated in docker/docker docker/engine-api#113

Merged

icecrime added the status/failing-ci Indicates that the PR in its current state fails the test suite label Feb 29, 2016

icecrime removed the status/failing-ci Indicates that the PR in its current state fails the test suite label Mar 2, 2016

icecrime added status/2-code-review and removed status/1-design-review labels Mar 3, 2016

mavenugo reviewed Mar 3, 2016
View reviewed changes

vdemeester reviewed Mar 4, 2016
View reviewed changes

Windows libnetwork integration

e8026d8

Signed-off-by: msabansal <sabansal@microsoft.com>

icecrime pushed a commit that referenced this pull request Mar 10, 2016

Merge pull request #20478 from msabansal/HNSIntegration

2b8e7ad

Windows libnetwork integration

icecrime merged commit 2b8e7ad into moby:master Mar 10, 2016

msabansal deleted the HNSIntegration branch March 11, 2016 00:12

thaJeztah mentioned this pull request Jun 23, 2016

Add make target for OS X cross binary #23632

Closed

olljanat mentioned this pull request Jul 19, 2022

Windows: Re-create custom NAT networks after restart if missing from HNS #43836

Merged

Windows libnetwork integration #20478

Windows libnetwork integration #20478

Conversation

msabansal commented Feb 19, 2016

thaJeztah commented Feb 19, 2016

thaJeztah commented Feb 23, 2016

thaJeztah commented Feb 23, 2016

msabansal commented Feb 24, 2016

lowenna Feb 24, 2016

Choose a reason for hiding this comment

msabansal Feb 24, 2016

Choose a reason for hiding this comment

mavenugo commented Feb 26, 2016

thaJeztah commented Feb 29, 2016

msabansal commented Mar 1, 2016

cpuguy83 commented Mar 1, 2016

msabansal commented Mar 1, 2016

lowenna commented Mar 3, 2016

thaJeztah commented Mar 3, 2016

vdemeester commented Mar 3, 2016

mavenugo Mar 3, 2016

Choose a reason for hiding this comment

mavenugo Mar 3, 2016

Choose a reason for hiding this comment

aboch Mar 3, 2016

Choose a reason for hiding this comment

mavenugo commented Mar 3, 2016

calavera commented Mar 3, 2016

lowenna commented Mar 3, 2016

msabansal commented Mar 3, 2016

mavenugo commented Mar 3, 2016

msabansal commented Mar 4, 2016

mavenugo commented Mar 4, 2016

vdemeester Mar 4, 2016

Choose a reason for hiding this comment

vdemeester commented Mar 4, 2016

calavera commented Mar 8, 2016

icecrime commented Mar 10, 2016

GordonTheTurtle commented Mar 10, 2016

lowenna commented Mar 10, 2016

thaJeztah commented Mar 10, 2016

mavenugo commented Mar 10, 2016

lowenna commented Mar 10, 2016

icecrime commented Mar 10, 2016

msabansal commented Mar 10, 2016

thaJeztah commented Mar 10, 2016

msabansal commented Mar 10, 2016