Merge pull request #1426 from weaveworks/1418-clean-cancel

Handle cancelled allocation in 'weave attach'

Fixes #1418.

ipam/allocate.go

@@ -59,7 +59,7 @@ func (g *allocate) Try(alloc *Allocator) bool {
 }
 
 func (g *allocate) Cancel() {
-	g.resultChan <- allocateResult{0, fmt.Errorf("Allocate request for %s cancelled", g.ident)}
+	g.resultChan <- allocateResult{0, &errorCancelled{"Allocate", g.ident}}
 }
 
 func (g *allocate) ForContainer(ident string) bool {

ipam/allocator.go

@@ -186,6 +186,15 @@ func hasBeenCancelled(cancelChan <-chan bool) func() bool {
 	}
 }
 
+type errorCancelled struct {
+	kind  string
+	ident string
+}
+
+func (e *errorCancelled) Error() string {
+	return fmt.Sprintf("%s request for %s cancelled", e.kind, e.ident)
+}
+
 // Actor client API
 
 // Allocate (Sync) - get new IP address for container with given name in range

ipam/claim.go

@@ -95,7 +95,7 @@ func (c *claim) deniedBy(alloc *Allocator, owner router.PeerName) {
 }
 
 func (c *claim) Cancel() {
-	c.sendResult(fmt.Errorf("Operation cancelled."))
+	c.sendResult(&errorCancelled{"Claim", c.ident})
 }
 
 func (c *claim) ForContainer(ident string) bool {

ipam/http.go

@@ -16,6 +16,41 @@ func badRequest(w http.ResponseWriter, err error) {
 	common.Log.Warningln("[allocator]:", err.Error())
 }
 
+func parseCIDR(w http.ResponseWriter, cidrStr string) (address.CIDR, bool) {
+	subnetAddr, cidr, err := address.ParseCIDR(cidrStr)
+	if err != nil {
+		badRequest(w, err)
+		return address.CIDR{}, false
+	}
+	if cidr.Start != subnetAddr {
+		badRequest(w, fmt.Errorf("Invalid subnet %s - bits after network prefix are not all zero", cidrStr))
+		return address.CIDR{}, false
+	}
+	return cidr, true
+}
+
+func (alloc *Allocator) handleHTTPAllocate(dockerCli *docker.Client, w http.ResponseWriter, ident string, checkAlive bool, subnet address.CIDR) {
+	closedChan := w.(http.CloseNotifier).CloseNotify()
+	addr, err := alloc.Allocate(ident, subnet.HostRange(), closedChan)
+	if err != nil {
+		if _, ok := err.(*errorCancelled); ok { // cancellation is not really an error
+			common.Log.Infoln("[allocator]:", err.Error())
+			fmt.Fprint(w, "cancelled")
+			return
+		}
+		badRequest(w, err)
+		return
+	}
+	if checkAlive && dockerCli != nil && dockerCli.IsContainerNotRunning(ident) {
+		common.Log.Infof("[allocator] '%s' is not running: freeing %s", ident, addr)
+		alloc.Free(ident, addr)
+		fmt.Fprint(w, "cancelled")
+		return
+	}
+
+	fmt.Fprintf(w, "%s/%d", addr, subnet.PrefixLen)
+}
+
 // HandleHTTP wires up ipams HTTP endpoints to the provided mux.
 func (alloc *Allocator) HandleHTTP(router *mux.Router, defaultSubnet address.CIDR, dockerCli *docker.Client) {
 	router.Methods("PUT").Path("/ip/{id}/{ip}").HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
@@ -36,18 +71,14 @@ func (alloc *Allocator) HandleHTTP(router *mux.Router, defaultSubnet address.CID
 
 	router.Methods("GET").Path("/ip/{id}/{ip}/{prefixlen}").HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		vars := mux.Vars(r)
-		cidr := vars["ip"] + "/" + vars["prefixlen"]
-		_, subnet, err := address.ParseCIDR(cidr)
-		if err != nil {
-			badRequest(w, err)
-			return
+		if subnet, ok := parseCIDR(w, vars["ip"]+"/"+vars["prefixlen"]); ok {
+			addr, err := alloc.Lookup(vars["id"], subnet.HostRange())
+			if err != nil {
+				http.NotFound(w, r)
+				return
+			}
+			fmt.Fprintf(w, "%s/%d", addr, subnet.PrefixLen)
 		}
-		addr, err := alloc.Lookup(vars["id"], subnet.HostRange())
-		if err != nil {
-			http.NotFound(w, r)
-			return
-		}
-		fmt.Fprintf(w, "%s/%d", addr, subnet.PrefixLen)
 	})
 
 	router.Methods("GET").Path("/ip/{id}").HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
@@ -60,48 +91,15 @@ func (alloc *Allocator) HandleHTTP(router *mux.Router, defaultSubnet address.CID
 	})
 
 	router.Methods("POST").Path("/ip/{id}/{ip}/{prefixlen}").HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		closedChan := w.(http.CloseNotifier).CloseNotify()
 		vars := mux.Vars(r)
-		ident := vars["id"]
-		cidrStr := vars["ip"] + "/" + vars["prefixlen"]
-		subnetAddr, cidr, err := address.ParseCIDR(cidrStr)
-		if err != nil {
-			badRequest(w, err)
-			return
+		if subnet, ok := parseCIDR(w, vars["ip"]+"/"+vars["prefixlen"]); ok {
+			alloc.handleHTTPAllocate(dockerCli, w, vars["id"], r.FormValue("check-alive") == "true", subnet)
 		}
-		if cidr.Start != subnetAddr {
-			badRequest(w, fmt.Errorf("Invalid subnet %s - bits after network prefix are not all zero", cidrStr))
-			return
-		}
-		addr, err := alloc.Allocate(ident, cidr.HostRange(), closedChan)
-		if err != nil {
-			badRequest(w, fmt.Errorf("Unable to allocate: %s", err))
-			return
-		}
-		if r.FormValue("check-alive") == "true" && dockerCli != nil && dockerCli.IsContainerNotRunning(ident) {
-			common.Log.Infof("[allocator] '%s' is not running: freeing %s", ident, addr)
-			alloc.Free(ident, addr)
-			return
-		}
-
-		fmt.Fprintf(w, "%s/%d", addr, cidr.PrefixLen)
 	})
 
 	router.Methods("POST").Path("/ip/{id}").HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		closedChan := w.(http.CloseNotifier).CloseNotify()
-		ident := mux.Vars(r)["id"]
-		newAddr, err := alloc.Allocate(ident, defaultSubnet.HostRange(), closedChan)
-		if err != nil {
-			badRequest(w, err)
-			return
-		}
-		if r.FormValue("check-alive") == "true" && dockerCli != nil && dockerCli.IsContainerNotRunning(ident) {
-			common.Log.Infof("[allocator] '%s' is not running: freeing %s", ident, newAddr)
-			alloc.Free(ident, newAddr)
-			return
-		}
-
-		fmt.Fprintf(w, "%s/%d", newAddr, defaultSubnet.PrefixLen)
+		vars := mux.Vars(r)
+		alloc.handleHTTPAllocate(dockerCli, w, vars["id"], r.FormValue("check-alive") == "true", defaultSubnet)
 	})
 
 	router.Methods("DELETE").Path("/ip/{id}/{ip}").HandlerFunc(func(w http.ResponseWriter, r *http.Request) {

weave

@@ -915,6 +915,9 @@ ipam_cidrs() {
         if [ "$IPAM_CIDRS" = "404 page not found" ] ; then
             echo "No IP address supplied and IP address allocation is disabled" >&2
             return 1
+        elif ! is_cidr "$IPAM_CIDRS" ; then
+            echo "$IPAM_CIDRS" >&2
+            return 1
         fi
         ALL_CIDRS="$IPAM_CIDRS"
     fi
@@ -929,8 +932,7 @@ ipam_cidrs() {
             if [ "$CIDR" = "404 page not found" ] ; then
                 echo "IP address allocation must be enabled to use 'net:'" >&2
                 return 1
-            fi
-            if ! is_cidr "$CIDR" ; then
+            elif ! is_cidr "$CIDR" ; then
                 echo "$CIDR" >&2
                 return 1
             fi