Swarm-mode overlay networking support for windows #28182

Merged
merged 2 commits into from Nov 10, 2016

Projects

None yet
@msabansal
Contributor
msabansal commented Nov 9, 2016 edited

- What I did
Enable overlay networking support on windows

- How I did it
The latest libnetwork has windows overlay network support. This PR vendors the latest libnetwork with minor changes that are required to enable overlay networking. For overlay networks there is an additional gateway endpoint attached to the container. We have added support to docker to enable querying this endpoint to pass it to HCS.

- How to verify it
The windows binaries required to enable overlay networking are not yet released. We have validated the support internally and validated the existing scenarios.
- Description for the changelog

  • Added support for overlay networking for windows
  • Vendors latest libnetwork

- A picture of a cute animal (not mandatory but encouraged)

libnetwork vendoring: Fixes #26912

@mavenugo
Contributor
mavenugo commented Nov 9, 2016

@msabansal I pushed docker/swarmkit#1737 to take care of the swarmkit fix. once it is merged, pls vendor swarmkit properly and remove the WIP tag.

daemon/daemon_windows.go
- if err != nil {
- return nil, err
+ // overlay networks are global and should not be deleted by local HNS
+ if v.Type() != "overlay" {
@mavenugo
mavenugo Nov 9, 2016 Contributor

Instead of v.Type() variable, you can use v.Info().Scope() constant and compare it against datastore.GlobalScope or simple "global" string.

@msabansal
msabansal Nov 9, 2016 Contributor

Accepted. Fixing.

daemon/daemon_windows.go
@@ -288,6 +291,11 @@ func (daemon *Daemon) initNetworkController(config *Config, activeSandboxes map[
// discover and add HNS networks to windows
// network that exist are removed and added again
for _, v := range hnsresponse {
+ if strings.ToLower(v.Type) == "overlay" {
@mavenugo
mavenugo Nov 9, 2016 Contributor

same here

@msabansal
msabansal Nov 9, 2016 Contributor

Accepted. Fixing.

@mavenugo mavenugo added this to the 1.13.0 milestone Nov 9, 2016
@msabansal msabansal changed the title from [WIP] Overlay networking support for windows to Swarm-mode overlay networking support for windows Nov 10, 2016
@mavenugo
Contributor
mavenugo commented Nov 10, 2016 edited

@msabansal Thanks for your patience. The docker changes are very minimal and LGTM.

My only comment is that people might get confused due to the lack of overlay support on the released WS2016 software and it requires update in the future. It would be great if we can add a check for this and not show the overlay driver. But given that the current behavior is consistent with other windows drivers such as l2bridge and l2tunnel, i wouldn't make that as a blocker for this PR. But I urge you to consider a follow-up patch for this.

@vieux
Member
vieux commented Nov 10, 2016

LGTM

daemon/daemon_windows.go
- if err != nil {
- return nil, err
+ // global networks should not be deleted by local HNS
+ if v.Info().Scope() == datastore.GlobalScope {
@mavenugo
mavenugo Nov 10, 2016 Contributor

Shouldn't this be != ?

@msabansal
msabansal Nov 10, 2016 Contributor

Accepted. Fixed

@mavenugo
Contributor

@msabansal i know you are blocked by docker/swarmkit#1740. Could you pls validate this fix (after addressing my comment above) along with docker/swarmkit#1740 and make sure things work as expected ?

+ if data["GW_INFO"] != nil {
+ gwInfo := data["GW_INFO"].(map[string]interface{})
+ if gwInfo["hnsid"] != nil {
+ gwHNSID = gwInfo["hnsid"].(string)
@thaJeztah
thaJeztah Nov 10, 2016 Member

Can different networks on the same container have different GW_INFO.hnsid's? Only the last one is added to the endpoint list, correct?

@msabansal
msabansal Nov 10, 2016 Contributor

No. If the container has N endpoints which don't have gateway libnetwork only adds one gateway. This will be common across all endpoints. This is to provide external connectivity.

@mavenugo
mavenugo Nov 10, 2016 Contributor

I agree. this is not different from Linux as well. But windows has a specific requirement for gw-info due to the way HNS works. So am okay with this change.

+
+func (d *driver) network(nid string) *network {
+ d.Lock()
+ networks := d.networks
@cpuguy83
cpuguy83 Nov 10, 2016 Contributor

This is not safe.

+ m["mtu"] = n.mtu
+ b, err = json.Marshal(m)
+ if err != nil {
+ return []byte{}
@cpuguy83
cpuguy83 Nov 10, 2016 Contributor

Why the allocation? Can this be a nil slice?

+
+ data := make(map[string]interface{}, 1)
+ data["hnsid"] = ep.profileId
+ data["AllowUnqualifiedDNSQuery"] = true
@cpuguy83
cpuguy83 Nov 10, 2016 Contributor

Curious why we aren't using a struct for this.

+
+func (d *driver) network(nid string) *network {
+ d.Lock()
+ networks := d.networks
@cpuguy83
cpuguy83 Nov 10, 2016 Contributor

This is not safe

+ }
+
+ d.Lock()
+ networks := d.networks
@cpuguy83
cpuguy83 Nov 10, 2016 Contributor

This is not safe

+
+ for _, n := range networks {
+ n.Lock()
+ endpoints := n.endpoints
@cpuguy83
cpuguy83 Nov 10, 2016 Contributor

This is not safe

@@ -440,6 +448,31 @@ func parseEndpointOptions(epOptions map[string]interface{}) (*endpointConfigurat
return ec, nil
}
+func parseEndpointConnectivity(epOptions map[string]interface{}) (*endpointConnectivity, error) {
@cpuguy83
cpuguy83 Nov 10, 2016 Contributor

This pattern of using map[string]interface{} as a struct in multiple places is worrying.

@mavenugo
mavenugo Nov 10, 2016 Contributor

this is purely the endpoint options that are passed to the driver which is transparent to libnetwork core. It is key-value pair for the drivers per endpoint. So, this is okay (these are essentially map[string]string ... but historically it was interface{} and changing APIs is a pain and hence we left it that way. Again, this is not related to this change per-se.

@cpuguy83

Couple of comments on some of the libnetwork code. I don't think they are blockers for merging, but we probably need to address at least some of them before release.

What's the easiest way to get a box up and running with a build from this PR?

msabansal and others added some commits Nov 10, 2016
@msabansal msabansal Swarm-mode overlay networking support for windows
Signed-off-by: msabansal <sabansal@microsoft.com>
ed8ccc3
@msabansal Sandeep Bansal Vendoring latest swarmkit and libnetwork
Signed-off-by: Sandeep Bansal <msabansal@microsoft.com>
6e95165
@mavenugo
Contributor
mavenugo commented Nov 10, 2016 edited

Thanks @cpuguy83. I agree. those should not block this PR. Also some of these are related to Solaris and I will check with the contributor who contributed the solaris support.

@mavenugo

Thanks @msabansal for addressing the comments.

LGTM

@mavenugo
Contributor

@cpuguy83

What's the easiest way to get a box up and running with a build from this PR?

I will help you with this

@msabansal
Contributor

@mavenugo Validated the basic scenarios with the latest swarmkit vendoring

@cpuguy83
Contributor

LGTM

@thaJeztah
Member

all green! merging 😄

@thaJeztah thaJeztah merged commit 4705728 into docker:master Nov 10, 2016

5 checks passed

dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 26577 has succeeded
Details
janky Jenkins build Docker-PRs 35148 has succeeded
Details
vendor Jenkins build Docker-PRs-vendor 2441 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 6060 has succeeded
Details
@mavenugo mavenugo referenced this pull request Nov 11, 2016
Merged

add 1.13.0 CHANGELOG.md #28275

@git-jiby-me

@marsmensch any ideas when the windows updates needed for overlay network will be released ?

@marsmensch
Contributor

you probably meant to ask @mavenugo ;-)

@thaJeztah
Member

@git-jiby-me I don't think there's a (public) ETA. @msabansal may know, but not sure if that information can be shared

@git-jiby-me

@marsmensch yes exactly, sorry for the confusion there :), @thaJeztah thanks for the info, @msabansal not a problem if the information is private, i was just curious when i can try overlay.

@msabansal
Contributor

Timelines are yet to be worked out for this. Will get back as soon as we have something defined.

@seankel
seankel commented Jan 2, 2017

@msabansal is there anyway to install the driver before the windows update? any update on timelines?

@msabansal
Contributor

@seankel Not that I can think of. I don't have any updates on timelines of the next update aswell. Perhaps @jmesser81 or @PatrickLang can help

@git-jiby-me

@msabansal @jmesser81 @PatrickLang any updates, or any ways to try it out ?

@JMesser81

We're working through this (bureaucratic) process now but unfortunately don't have any timelines we can provide. Feel free to ping me directly (jmesser@microsoft.com) if you have a specific Customer Proof-of-Concept you're interested in running and I'll see what I can do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment