host-local: Update host-local IPAM to support Windows #77

rakelkar · 2017-10-07T02:23:59Z

Get host-local to work for Windows. This is required for Flannel VXLAN mode on Windows.

squeed · 2017-10-09T11:53:46Z

plugins/ipam/host-local/backend/disk/backend.go

@@ -56,6 +57,10 @@ func New(network, dataDir string) (*Store, error) {

 func (s *Store) Reserve(id string, ip net.IP, rangeID string) (bool, error) {
 	fname := filepath.Join(s.dataDir, ip.String())
+	if runtime.GOOS == "windows" {
+		fname = strings.Replace(fname, ":", "_", -1)


Hah, good catch!

squeed · 2017-10-09T12:04:34Z

pkg/ip/route_unspecified.go

@@ -12,7 +12,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.

-// +build !linux
+// +build !linux,!windows


Is there supposed to be a Windows version of this file? Or am I just missing something?

Cant make a windows specific version of these methods since they reference the netlink package in their method signature "dev netlink.Link" ... Not sure why this file is present in the repo.

squeed · 2017-10-09T12:05:02Z

pkg/ipam/ipam_windows.go

+	"github.com/containernetworking/cni/pkg/types/current"
+)
+
+func ExecAdd(plugin string, netconf []byte) (types.Result, error) {


It looks like these functions are duplicated - Can you just move ConfigureIface to a build-specific file?

Moved the execXX funcs to exec.go - let me know if thats not what you had in mind

squeed · 2017-10-09T12:06:20Z

plugins/ipam/host-local/backend/disk/lock.go

@@ -1,3 +1,5 @@
+// +build linux


Just do !windows here - this "should" work on all Unix-like systems.

squeed · 2017-10-09T12:09:19Z

pkg/ip/addr.go

@@ -1,3 +1,5 @@
+// +build linux


Out of curiosity, does Windows also need to worry about "tentative" addresses?

Not sure, asking around

So it seems Windows doesn't need to worry about it right now since HNS doesn't support IPv6 yet - so ja bitte for now I guess ;-)

squeed · 2017-10-09T12:11:37Z

Seems pretty reasonable to me. I added a first pass of changes - you're right, not too many changes needed.

When doing build-tagged files, it's considered good practice to only duplicate the minimum amount of os-dependent code, and put the rest in non-build-tagged files.

rakelkar · 2017-10-13T22:25:55Z

@squeed Have addressed your comments, could you take a look? The test failure on CI is unrelated to this change (possibly a flaky test?).

squeed · 2017-10-17T12:56:17Z

Kicked that CI job and it's green now, dammit.

Seems reasonable to me. I want to think for one more day if we want to have a specific conditional build policy - the maintainer meeting is tomorrow.

rosenhouse · 2017-10-17T13:33:30Z

pkg/ipam/ipam_windows.go

@@ -0,0 +1,28 @@
+// +build windows


I don't believe this line is necessary, since the filename ends in _windows.go.

[Reference]

rosenhouse · 2017-10-17T13:36:41Z

plugins/ipam/host-local/backend/disk/lock_windows.go

+		lockPath = path.Join(lockPath, "lock")
+	}
+
+	f, err := filemutex.New(lockPath)


this code seems identical to the POSIX version, except it makes use of this nice cross-platform adapter. Could we make this file the one true lock.go and remove the POSIX-only implementation entirely?

rosenhouse · 2017-10-17T13:38:00Z

plugins/ipam/host-local/backend/disk/lock_windows.go

+package disk
+
+import (
+	"github.com/alexflint/go-filemutex"


Is this package added to the vendor directory somewhere? I don't see it.

[meta comment: I'm surprised this passed Travis without doing that. Maybe we should open a separate PR to enforce vendoring at CI time.]

rakelkar · 2017-10-17T14:46:34Z

Travis doesn't build windows... I can add a build.ps1 and an appveyor yml to kick it off? The ps1 would only include plugins that work for Windows - so only host-local for now...

…

On Oct 17, 2017 6:39 AM, "Gabe Rosenhouse" ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In plugins/ipam/host-local/backend/disk/lock_windows.go <#77 (comment)> : > +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package disk + +import ( + "github.com/alexflint/go-filemutex" [meta comment: I'm surprised this passed Travis without doing that. Maybe we should open a separate PR to enforce vendoring at CI time.] — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#77 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADQphave4hcpc8WYnFzV4hBgHG5xqcokks5stK4sgaJpZM4PxM90> .

rosenhouse · 2017-10-18T04:02:11Z

plugins/ipam/host-local/backend/disk/lock.go

 func (l *FileLock) Close() error {
-	return l.f.Close()
+	return nil


why not close the file?

oh, because the library doesn't expose a Close() method at all. uh oh....

Although apparently we're not using Close() anyhow. When I delete the Close() method from the backend.Store interface, the program still compiles. So I guess you could just do that.

Nice catch - will remove, though I must say with a touch of disquiet.

So it seems like main.go:51 creates a Store struct and not the interface and calls defer store.Close so close is called after all...

There is an open PR on the library to add a close method, will create a PR to update this plugin once that has been merged. alexflint/go-filemutex#2

Updated with shiny new close method!

rosenhouse · 2017-10-18T04:05:46Z

plugins/ipam/host-local/backend/disk/lock.go

 }

 // Lock acquires an exclusive lock
 func (l *FileLock) Lock() error {
-	return syscall.Flock(int(l.f.Fd()), syscall.LOCK_EX)
+	l.f.Lock()
+	return nil


I didn't realize before that this library panics on errors rather than returning them.
Effective Go says that "real library functions should avoid panic". I wonder if that library deserves a PR...

rakelkar · 2017-10-28T03:13:37Z

plugins/ipam/host-local/host_local_test.go

+					{"dst": "2001:db8:2::0/64", "gw": "2001:db8:3::1"}
+				]
+			}
+		}`, tmpDir, tmpDir)


I ran gofmt - and it felt indents were in order!

rakelkar · 2017-10-28T03:14:33Z

plugins/ipam/host-local/host_local_test.go

+func getTmpDir() (string, error) {
+	tmpDir, err := ioutil.TempDir("", "host_local_artifacts")
+	if err == nil {
+		tmpDir = filepath.ToSlash(tmpDir)


so without this the un-escaped backslashes in the windows path cause JSON parsing to barf

rakelkar · 2017-10-28T03:15:36Z

plugins/ipam/host-local/backend/disk/backend.go

-	if runtime.GOOS == "windows" {
-		fname = strings.Replace(fname, ":", "_", -1)
-	}
+	fname := GetEscapedPath(s.dataDir, ip.String())


Turns out I missed a bunch, also escaping the colon in the dataDir doesnt work so well when the path includes a drive letter - duh!

rakelkar · 2017-10-28T03:24:00Z

@rosenhouse @squeed Have addressed all comments, got the shiny new changes from alexflint and made fixes to get tests to run on Windows. Let me know if you see something anything don't like. The current build failure is an unrelated flaky test.

rosenhouse · 2017-11-01T15:21:50Z

@rakelkar thanks for all your hard work on this! we're excited to merge this, just as soon as we have Continuous integration set up for Windows in this repo. I've got PR #84 open, but its not yet green. Once that's merged, then we can return to this PR.

rosenhouse · 2017-11-11T03:12:10Z

@rakelkar now that #84 is merged, please rebase and squash this down to a single commit, e.g. something like rosenhouse@799776f

rakelkar · 2017-11-11T21:29:09Z

@rosenhouse rebased and squashed!

rosenhouse · 2017-11-11T21:31:29Z

plugins/ipam/host-local/backend/disk/lock.go

+
+	if fi.IsDir() {
+		lockPath = path.Join(lockPath, "lock")
+	}


is this line necessary for windows? If so, maybe it deserves a unit test?

yes it is required - added unit tests for filepath and folderpath

LMK if you like what you see, will wait for LGTM to squash!

squashed commits

rosenhouse

LGTM

squeed

LGTM, thanks!

rakelkar force-pushed the rakelkar/host-local-windows branch 2 times, most recently from 66e16a4 to 72798b1 Compare October 8, 2017 00:11

squeed reviewed Oct 9, 2017

View reviewed changes

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

rosenhouse reviewed Oct 17, 2017

View reviewed changes

rosenhouse reviewed Oct 18, 2017

View reviewed changes

rosenhouse mentioned this pull request Oct 18, 2017

Returning error instead of panic alexflint/go-filemutex#3

Closed

This was referenced Oct 18, 2017

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

Closed

Add method to close file handle alexflint/go-filemutex#7

Closed

filemutex is missing a Close method alexflint/go-filemutex#8

Closed

rakelkar commented Oct 28, 2017

View reviewed changes

rakelkar mentioned this pull request Nov 1, 2017

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

Closed

4 tasks

This was referenced Nov 1, 2017

Enable Windows CI (Appveyor) #84

Merged

Add a CNI for Windows #80

Closed

rakelkar force-pushed the rakelkar/host-local-windows branch 3 times, most recently from f6d364f to 6816389 Compare November 11, 2017 21:19

rosenhouse reviewed Nov 11, 2017

View reviewed changes

rosenhouse approved these changes Nov 11, 2017

View reviewed changes

host-local: Update host-local IPAM to support Windows

47668f6

rakelkar force-pushed the rakelkar/host-local-windows branch from beb9e1f to 47668f6 Compare November 11, 2017 23:18

squeed approved these changes Nov 13, 2017

View reviewed changes

rosenhouse merged commit 6c2ef73 into containernetworking:master Nov 13, 2017

host-local: Update host-local IPAM to support Windows #77

host-local: Update host-local IPAM to support Windows #77

Conversation

rakelkar commented Oct 7, 2017 • edited Loading

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

squeed commented Oct 9, 2017

rakelkar commented Oct 13, 2017

squeed commented Oct 17, 2017

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 17, 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

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 28, 2017 • edited Loading

rosenhouse commented Nov 1, 2017

rosenhouse commented Nov 11, 2017

rakelkar commented Nov 11, 2017

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

rosenhouse left a comment

Choose a reason for hiding this comment

squeed left a comment

Choose a reason for hiding this comment

rakelkar commented Oct 7, 2017 •

edited

Loading

rakelkar commented Oct 28, 2017 •

edited

Loading