Support delegating to Windows CNI from Flannel CNI #76

rakelkar · 2017-10-04T05:21:17Z

Support delegating to Windows CNI for Flannel host-gw and vxlan modes.

This PR is related to the change proposed to Flannel to support Windows:
flannel-io/flannel#832

Support delegating to Windows CNI - use a separate WindowsDelegate field as the format of Windows properties is different from the schema supported by Linux under the delegate field. This will users to configure both Linux and Windows together and also to use Windows specific configuration for Windows clusters.

rosenhouse

Thanks for getting this work kicked off. I left a few comments on the code itself.

I'd also like to see this new WindowsDelegate field described in the README. In particular, please include an explanation of why the schema needs to be different from that for Linux.

rosenhouse · 2017-10-04T14:20:55Z

plugins/meta/flannel/flannel.go

+func cmdAddWindows(containerID string, n *NetConf, fenv *subnetEnv) error {
+	if n.WindowsDelegate == nil {
+		n.WindowsDelegate = make(map[string]interface{})
+	} else {


these checks appear identical to those in the other cmdAdd function, with only a difference in the error string. Could you extract a function with the common code?

Since I'm removing the "WindowsDelegate" attribute, I'll just move the call to cmdAddWindows down after the checks and remove the dups.

rosenhouse · 2017-10-04T14:24:46Z

plugins/meta/flannel/flannel.go

+
+	updateOutboundNat(&n.WindowsDelegate, fenv)
+
+	n.WindowsDelegate["cniVersion"] = "0.2.0"


Why not n.CNIVersion or version.Current()?

rosenhouse · 2017-10-04T14:26:43Z

plugins/meta/flannel/flannel.go

+		n.WindowsDelegate["type"] = "wincni.exe"
+	}
+
+	updateOutboundNat(&n.WindowsDelegate, fenv)


In Go, a map is already a reference type, there's rarely a reason to pass around a pointer to a map.

rosenhouse · 2017-10-04T14:38:15Z

plugins/meta/flannel/flannel.go

+	// TODO: rakesh: file a bug somewhere to remove this when HNS the HNS limitation is fixed
+	nid := ipn.IP.Mask(ipn.Mask)
+	i := ipToInt(nid)
+	return intToIP(i.Add(i, big.NewInt(2)))


why not just

nid[len(nid)-1] += 2 return nid

and skip the big.Int conversions?

rosenhouse · 2017-10-04T14:39:38Z

plugins/meta/flannel/flannel_windows_test.go

+import (
+	"testing"
+	"net"
+	"github.com/stretchr/testify/assert"


Almost all of CNI, including the other tests of the flannel package, are using Ginkgo. Is there a reason that won't work here?

Moved to tests to ginkgo.

rosenhouse · 2017-10-04T14:53:09Z

plugins/meta/flannel/flannel.go

+							return
+						}
+
+						nets := pv["ExceptionList"].([]interface{})


Tabs are a scarce resource 😄
I would suggest that the if branch be the negative case, in order to reduce indenting and improve readability. You can also replace the single-case switch statement with a simple if. For example:

pt := policy.(map[string]interface{}) if !hasKey(pt, "Value") { continue } pv, ok := pt["Value"].(map[string]interface{}) if !ok || !hasKey(pv, "Type") { continue } if !strings.EqualFold(pv["Type"].(string), "OutBoundNAT") { continue } // now to the real suff if !hasKey(pv, "ExceptionList") { ...

They are scarce indeed - have made the changes and saved them for future generations :-)

rosenhouse · 2017-10-04T14:56:29Z

plugins/meta/flannel/flannel_windows_test.go

+	exceptionList := value["ExceptionList"].([]interface{})
+	assert.Equal(t, nw.String(), exceptionList[0].(string))
+	assert.Equal(t, nw2.String(), exceptionList[1].(string))
+}


Can we get a real integration test of the Windows behavior? In other words, a test case that calls cmdAddWindows directly or indirectly?

The other flannel test cases aren't great -- they don't have particularly strong assertions. But they do integrate the full program. I'd like to see that level of test coverage for the Windows features.

rakelkar · 2017-10-05T00:58:25Z

@rosenhouse Thanks much for taking a look. Your comments are very reasonable, a bunch have to do with my go-naivety that I will fix. One worth discussing a bit is the "windowsDelegate". In general the "delegate" field seems to have the schema of the delegate CNI - so a new attribute is not required if we assume a single CNI type works for all nodes.

This assumption is invalid if we assume a mixed cluster that has Windows and Linux nodes. In this case we need one type of delegate for the Linux nodes and a different delegate for Windows nodes. A mixed cluster is desirable since it is quite likely you want to run a few Linux containers alongside the Windows containers e.g. nginx for ingress.

A more generalized solution would allow 1..n delegates and some mechanism to select on some basis (not just os) - possibly based on node tags. Another option is to have an additional windows specific section in the flannel config map YAML and provision windows nodes based on that?

To avoid premature over-design, I decided to start with a simple separate attribute that allowed me to clearly differentiate Windows and Linux.

What do you think?

squeed · 2017-10-05T11:06:08Z

I'm not sure I like windowsDelegate. We've talked about "branching" configuration before, specifically in the context of hypervisors, and my personal preference is against them.

The one thing this enables is deploying the same CNI configuration file to Windows vs. Linux hosts. I'm not entirely convinced of the advantage to adding this complexity. It seems natural that different OSs have different configurations. The presence of the windows-specific additional NAT arguments seems to support this.

rosenhouse · 2017-10-05T15:08:30Z

I agree with @squeed. At this point, with recommendations like portMappings in our conventions, there's an expectation that the container runtime is doing some processing on the config file before sending it to the plugin.

The single-host runtime certainly knows which OS it is running on, and so could decide which OS-specific config to pass to the plugin. I believe the same goes for an orchestrator utilizing a mixture of Linux and Windows hosts -- it likely is aware of OS-based placement constraints. Although the single-host vs multi-host distinction is outside the scope of CNI.

squeed · 2017-10-05T15:13:49Z

That's not to say that I disagree with most of this PR. The windows-specific code changes for generating a delegate config seem more-or-less fine. It's just the separate config key.

rakelkar · 2017-10-05T16:21:37Z

Makes sense. I agree different OSes can have different configurations. There is probably more downside than upside to a single mega configuration that spans the variations. I'll update the PR.

…

On Thu, Oct 5, 2017 at 8:13 AM, Casey Callendrello ***@***.*** > wrote: That's not to say that I disagree with most of this PR. The windows-specific code changes for generating a delegate config seem more-or-less fine. It's just the separate config key. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#76 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADQphSJvVR_ju2xRMpD3tdlqYoUkadqDks5spPIvgaJpZM4PtG8d> .

-- Rakesh Kelkar

Resolves PR review comments by: removing the WindowsDelegate attribute, fix coditions to avoid tabs remove duplicate checks remove redundant map pointer remove redundant ip fns

squeed · 2017-10-17T10:56:12Z

plugins/meta/flannel/flannel.go

@@ -202,6 +204,10 @@ func cmdAdd(args *skel.CmdArgs) error {
 		}
 	}

+	if runtime.GOOS == "windows" {


Suggestion: both of these functions generate some kind of transformed configuration. So, can you structure the function something like:

func cmdAdd() { // preamble switch runtime.GOOS { case `"windows":` buildWindowsDelegate() case "linux": buildLinuxDelegate() default: return err.. } return delegateAdd() }

This would be easier to test on the Linux side too.

Yup - I made precisely this change to address the test comment :) will commit after adding a test

squeed · 2017-10-17T10:56:48Z

plugins/meta/flannel/flannel.go

+
+	updateOutboundNat(n.Delegate, fenv)
+
+	n.Delegate["cniVersion"] = "0.2.0"


If possible, can you use 0.3.1? It supports a more advanced return type.

Updated to use existing (linux) logic:
n.Delegate["cniVersion"] = n.CNIVersion

Flannel: Add support for Windows Overlay (vxlan) mode Windows Overlay mode requires the network to be set to the cluster CIDR. So for overlay setup host-local as IPAM with the pod CIDR as the ip range

rakelkar · 2017-10-18T08:02:50Z

CI failure appears to be a flake.

rakelkar · 2017-10-18T08:07:45Z

@rosenhouse Have added a section to the flannel CNI README to describe Windows support. Per your contributing guide 3rd party plugins should go into their own repo... so going to work with the folks that own WINCNI to find a home for it somewhere nice and warm.

rakelkar · 2017-11-03T04:54:23Z

Refactored most of the windows specific logic into new windows CNIs in #85. Closing this PR as it is superseded by changes in #85.

rosenhouse suggested changes Oct 4, 2017

View reviewed changes

Ubuntu and others added 3 commits October 5, 2017 23:39

flannel: Remove WindowsDelegate in favor of OS specific config files

5aa91ea

Resolves PR review comments by: removing the WindowsDelegate attribute, fix coditions to avoid tabs remove duplicate checks remove redundant map pointer remove redundant ip fns

flannel: Format windows test file

575e7ff

flannel: move Windows tests to Ginkgo

a2c0697

rakelkar mentioned this pull request Oct 14, 2017

Enable Flannel to support Windows for host-gw and VXLAN flannel-io/flannel#832

Closed

2 tasks

squeed reviewed Oct 17, 2017

View reviewed changes

rakelkar mentioned this pull request Oct 18, 2017

Support Windows in host-gw and VXLAN modes flannel-io/flannel#833

Closed

rakelkar added 2 commits October 17, 2017 23:30

Add logic to support vxlan on Windows (#2)

780e0bd

Flannel: Add support for Windows Overlay (vxlan) mode Windows Overlay mode requires the network to be set to the cluster CIDR. So for overlay setup host-local as IPAM with the pod CIDR as the ip range

flannel: refactor windows support for testability and add tests

c024a03

rakelkar mentioned this pull request Nov 1, 2017

Windows CNI for overlay (vxlan) and host-gw (l2bridge) modes #85

Closed

4 tasks

rakelkar closed this Nov 3, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support delegating to Windows CNI from Flannel CNI #76

Support delegating to Windows CNI from Flannel CNI #76

rakelkar commented Oct 4, 2017 •

edited

Loading

rosenhouse left a comment

rosenhouse Oct 4, 2017

rakelkar Oct 5, 2017

rosenhouse Oct 4, 2017

rakelkar Oct 13, 2017

rosenhouse Oct 4, 2017

rakelkar Oct 5, 2017

rosenhouse Oct 4, 2017

rosenhouse Oct 4, 2017

rakelkar Oct 14, 2017

rosenhouse Oct 4, 2017

rakelkar Oct 5, 2017

rosenhouse Oct 4, 2017

rakelkar commented Oct 5, 2017

squeed commented Oct 5, 2017

rosenhouse commented Oct 5, 2017 •

edited

Loading

squeed commented Oct 5, 2017

rakelkar commented Oct 5, 2017 via email

squeed Oct 17, 2017

rakelkar Oct 18, 2017

squeed Oct 17, 2017

rakelkar Oct 18, 2017

rakelkar commented Oct 18, 2017

rakelkar commented Oct 18, 2017

rakelkar commented Nov 3, 2017


		updateOutboundNat(&n.WindowsDelegate, fenv)

		n.WindowsDelegate["cniVersion"] = "0.2.0"


		updateOutboundNat(n.Delegate, fenv)

		n.Delegate["cniVersion"] = "0.2.0"

Support delegating to Windows CNI from Flannel CNI #76

Support delegating to Windows CNI from Flannel CNI #76

Conversation

rakelkar commented Oct 4, 2017 • edited Loading

rosenhouse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rakelkar commented Oct 5, 2017

squeed commented Oct 5, 2017

rosenhouse commented Oct 5, 2017 • edited Loading

squeed commented Oct 5, 2017

rakelkar commented Oct 5, 2017 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rakelkar commented Oct 18, 2017

rakelkar commented Oct 18, 2017

rakelkar commented Nov 3, 2017

rakelkar commented Oct 4, 2017 •

edited

Loading

rosenhouse commented Oct 5, 2017 •

edited

Loading