Address PR comments #2

- Move the MTU check to CreateDatapath function.

common/net/net.go

@@ -0,0 +1,15 @@
+package net
+
+import (
+	"fmt"
+
+	"github.com/vishvananda/netlink"
+)
+
+func SetMTU(ifaceName string, mtu int) error {
+	link, err := netlink.LinkByName(ifaceName)
+	if err != nil {
+		return fmt.Errorf("Unable to find interface %s: %s", ifaceName, err)
+	}
+	return netlink.LinkSetMTU(link, mtu)
+}

common/odp/odp.go

@@ -6,37 +6,68 @@ import (
 	"syscall"
 
 	"github.com/weaveworks/go-odp/odp"
+
+	wnet "github.com/weaveworks/weave/common/net"
 )
 
 // ODP admin functionality
 
-func CreateDatapath(dpname string) (supported bool, err error) {
+func CreateDatapath(dpname string, mtuToCheck int) (supported bool, validMTU bool, err error) {
+	validMTU = true
+
 	dpif, err := odp.NewDpif()
 	if err != nil {
 		if odp.IsKernelLacksODPError(err) {
-			return false, nil
+			return false, validMTU, nil
 		}
-		return true, err
+		return true, validMTU, err
 	}
 	defer dpif.Close()
 
 	dp, err := dpif.CreateDatapath(dpname)
 	if err != nil && !odp.IsDatapathNameAlreadyExistsError(err) {
-		return true, err
+		return true, validMTU, err
+	}
+
+	var (
+		vpid   odp.VportID
+		vpname string
+	)
+	if vpid, vpname, err = createDummyVxlanVport(dp); err != nil {
+		if nlerr, ok := err.(odp.NetlinkError); ok {
+			if syscall.Errno(nlerr) == syscall.EAFNOSUPPORT {
+				dp.Delete()
+				return false, validMTU, fmt.Errorf("kernel does not have Open vSwitch VXLAN support")
+			}
+		}
 	}
 
-	vpid, _, err := CreateDummyVxlanVport(dpname, true)
-	if nlerr, ok := err.(odp.NetlinkError); ok {
-		if syscall.Errno(nlerr) == syscall.EAFNOSUPPORT {
-			dp.Delete()
-			return false, fmt.Errorf("kernel does not have Open vSwitch VXLAN support")
+	// Check whether the user is exposed to https://github.com/weaveworks/weave/issues/1853
+	if mtuToCheck > 0 {
+		// Create vxlan vport if the previous attempt has failed. Retry a few
+		// times if the creation fails due to the chosen vxlan UDP port being occupied.
+		for i := 0; i < 5 && !(err == nil && vpname == ""); i++ {
+			if vpid, vpname, err = createDummyVxlanVport(dp); err != nil {
+				if errno, ok := err.(syscall.Errno); !(ok && errno == syscall.EADDRINUSE) {
+					// Skip the check if something went wrong
+					return true, validMTU, nil
+				}
+			} else {
+				break
+			}
+		}
+		// Couldn't create the vport, skip the check
+		if err != nil {
+			return true, validMTU, err
 		}
+		validMTU = checkMTU(vpname, mtuToCheck)
 	}
-	if err == nil {
+
+	if err == nil && vpname != "" {
 		dp.DeleteVport(vpid)
 	}
 
-	return true, nil
+	return true, validMTU, nil
 }
 
 func DeleteDatapath(dpname string) error {
@@ -73,50 +104,31 @@ func AddDatapathInterface(dpname string, ifname string) error {
 	return err
 }
 
-func CreateDummyVxlanVport(dpname string, keepUDPSocket bool) (odp.VportID, string, error) {
-	dpif, err := odp.NewDpif()
-	if err != nil {
-		return 0, "", err
-	}
-	defer dpif.Close()
-
-	dp, err := dpif.LookupDatapath(dpname)
-	if err != nil {
-		return 0, "", err
-	}
-
+func createDummyVxlanVport(dp odp.DatapathHandle) (odp.VportID, string, error) {
 	// A dummy way to get an ephemeral port for UDP
 	udpconn, err := net.ListenUDP("udp4", nil)
 	if err != nil {
 		return 0, "", err
 	}
 	portno := uint16(udpconn.LocalAddr().(*net.UDPAddr).Port)
-	if keepUDPSocket {
-		// we leave the UDP socket open, so creating a vxlan vport on
-		// the same port number should fail.  But that's fine: It's
-		// still sufficient to probe for support.
-		defer udpconn.Close()
-	} else {
-		udpconn.Close()
-	}
+	udpconn.Close()
 
 	vpname := fmt.Sprintf("vxlan-%d", portno)
 	vpid, err := dp.CreateVport(odp.NewVxlanVportSpec(vpname, portno))
 
 	return vpid, vpname, err
 }
 
-func DeleteVport(dpname string, vpid odp.VportID) error {
-	dpif, err := odp.NewDpif()
-	if err != nil {
-		return err
-	}
-	defer dpif.Close()
-
-	dp, err := dpif.LookupDatapath(dpname)
-	if err != nil {
-		return err
+func checkMTU(vpname string, mtuToCheck int) bool {
+	// Setting >1500 MTU will fail with EINVAL, if the user is affected by
+	// the kernel issue.
+	if err := wnet.SetMTU(vpname, mtuToCheck); err != nil {
+		if errno, ok := err.(syscall.Errno); ok && errno == syscall.EINVAL {
+			return false
+		}
+		// NB: If no link interface for the vport is found (which
+		// might be a case for SetMTU to fail), the user is probably
+		// running the <= 4.2 kernel, which is fine.
 	}
-
-	return dp.DeleteVport(vpid)
+	return true
 }

prog/weaveutil/datapath.go

@@ -5,11 +5,9 @@ import (
 	"fmt"
 	"os"
 	"strconv"
-	"syscall"
 
-	libodp "github.com/weaveworks/go-odp/odp"
+	wnet "github.com/weaveworks/weave/common/net"
 	"github.com/weaveworks/weave/common/odp"
-	wnet "github.com/weaveworks/weave/net"
 )
 
 func createDatapath(args []string) error {
@@ -23,7 +21,7 @@ func createDatapath(args []string) error {
 		return fmt.Errorf("unable to parse mtu %q: %s", args[1], err)
 	}
 
-	odpSupported, err := odp.CreateDatapath(dpname)
+	odpSupported, validMTU, err := odp.CreateDatapath(dpname, mtu)
 	if !odpSupported {
 		if err != nil {
 			fmt.Fprintln(os.Stderr, err)
@@ -36,13 +34,10 @@ func createDatapath(args []string) error {
 		return err
 	}
 
-	if mtu > 1500 {
-		// Check whether a host is exposed to https://github.com/weaveworks/weave/issues/1853
-		// If yes, fallback to 1500 MTU.
-		if !checkMTU(dpname, mtu) {
-			fmt.Fprintf(os.Stderr, "WARNING: Unable to set fastdp MTU to %d, possibly due to 4.3/4.4 kernel version. Setting MTU to 1500 instead.\n", mtu)
-			mtu = 1500
-		}
+	// Fallback to 1500 MTU if the user is exposed to the issue
+	if mtu > 1500 && !validMTU {
+		fmt.Fprintf(os.Stderr, "WARNING: Unable to set fastdp MTU to %d, possibly due to 4.3/4.4 kernel version. Setting MTU to 1500 instead.\n", mtu)
+		mtu = 1500
 	}
 
 	if err := wnet.SetMTU(dpname, mtu); err != nil {
@@ -67,7 +62,7 @@ func checkDatapath(args []string) error {
 
 	dpname := args[0]
 
-	odpSupported, err := odp.CreateDatapath(dpname)
+	odpSupported, _, err := odp.CreateDatapath(dpname, -1)
 	if err != nil {
 		return err
 	}
@@ -84,46 +79,3 @@ func addDatapathInterface(args []string) error {
 	}
 	return odp.AddDatapathInterface(args[0], args[1])
 }
-
-// Helpers
-
-func checkMTU(dpname string, mtu int) bool {
-	var (
-		vpid   libodp.VportID
-		vpname string
-		err    error
-	)
-
-	// Create a dummy vxlan vport. Retry a few times if the creation fails due
-	// to the chosen vxlan UDP port being occupied.
-	for i := 0; i < 5; i++ {
-		if vpid, vpname, err = odp.CreateDummyVxlanVport(dpname, true); err == nil {
-			defer func() {
-				if err := odp.DeleteVport(dpname, vpid); err != nil {
-					fmt.Fprintf(os.Stderr, "unable to remove unused %q vxlan vport: %s\n", vpname, err)
-				}
-			}()
-			break
-		} else if errno, ok := err.(syscall.Errno); !(ok && errno == syscall.EADDRINUSE) {
-			// Skip the check if something went wrong
-			return true
-		}
-	}
-	// Couldn't create the vport, skip the check
-	if vpname == "" {
-		return true
-	}
-
-	// Setting >1500 MTU will fail with EINVAL, if the user is affected by
-	// the kernel issue.
-	if err := wnet.SetMTU(vpname, mtu); err != nil {
-		if errno, ok := err.(syscall.Errno); ok && errno == syscall.EINVAL {
-			return false
-		}
-		// NB: If no link interface for the vport is found (which
-		// might be a case for SetMTU to fail), the user is probably
-		// running the <= 4.2 kernel, which is fine.
-	}
-
-	return true
-}