diff --git a/ipam/allocator.go b/ipam/allocator.go index ba02d82f..875b94f0 100644 --- a/ipam/allocator.go +++ b/ipam/allocator.go @@ -278,7 +278,7 @@ func (alloc *Allocator) Lookup(ident string, r address.Range) ([]address.CIDR, e } // Claim an address that we think we should own (Sync) -func (alloc *Allocator) Claim(ident string, cidr address.CIDR, isContainer, noErrorOnUnknown bool) error { +func (alloc *Allocator) Claim(ident string, cidr address.CIDR, isContainer, noErrorOnUnknown bool, hasBeenCancelled func() bool) error { resultChan := make(chan error) op := &claim{ resultChan: resultChan, @@ -286,6 +286,7 @@ func (alloc *Allocator) Claim(ident string, cidr address.CIDR, isContainer, noEr cidr: cidr, isContainer: isContainer, noErrorOnUnknown: noErrorOnUnknown, + hasBeenCancelled: hasBeenCancelled, } alloc.doOperation(op, &alloc.pendingClaims) return <-resultChan diff --git a/ipam/allocator_test.go b/ipam/allocator_test.go index 84525645..af05530a 100644 --- a/ipam/allocator_test.go +++ b/ipam/allocator_test.go @@ -27,7 +27,7 @@ func (alloc *Allocator) SimplyAllocate(ident string, cidr address.CIDR) (address } func (alloc *Allocator) SimplyClaim(ident string, cidr address.CIDR) error { - return alloc.Claim(ident, cidr, true, true) + return alloc.Claim(ident, cidr, true, true, returnFalse) } func TestAllocFree(t *testing.T) { diff --git a/ipam/claim.go b/ipam/claim.go index 9869bc8a..6ad1b712 100644 --- a/ipam/claim.go +++ b/ipam/claim.go @@ -15,6 +15,7 @@ type claim struct { cidr address.CIDR isContainer bool noErrorOnUnknown bool + hasBeenCancelled func() bool } // Send an error (or nil for success) back to caller listening on resultChan @@ -33,6 +34,11 @@ func (c *claim) sendResult(result error) { // Try returns true for success (or failure), false if we need to try again later func (c *claim) Try(alloc *Allocator) bool { + if c.hasBeenCancelled() { + c.Cancel() + return true + } + if !alloc.ring.Contains(c.cidr.Addr) { // Address not within our universe; assume user knows what they are doing alloc.infof("Address %s claimed by %s - not in our range", c.cidr, c.ident) @@ -51,11 +57,10 @@ func (c *claim) Try(alloc *Allocator) bool { // success case mesh.UnknownPeerName: // If our ring doesn't know, it must be empty. + alloc.infof("Claim %s for %s: is in the range %s, but the allocator is not initialized yet; will try later.", + c.cidr, c.ident, alloc.universe.AsCIDRString()) if c.noErrorOnUnknown { - alloc.infof("Claim %s for %s: address allocator still initializing; will try later.", c.cidr, c.ident) c.sendResult(nil) - } else { - c.sendResult(fmt.Errorf("%s is in the range %s, but the allocator is not initialized yet", c.cidr, alloc.universe.AsCIDRString())) } return false default: diff --git a/ipam/http.go b/ipam/http.go index a4baf30e..6139546c 100644 --- a/ipam/http.go +++ b/ipam/http.go @@ -38,32 +38,52 @@ func writeAddresses(w http.ResponseWriter, cidrs []address.CIDR) { } } +func hasBeenCanceled(dockerCli *docker.Client, closedChan <-chan bool, ident string, checkAlive bool) func() bool { + return func() bool { + select { + case <-closedChan: + return true + default: + res := checkAlive && dockerCli != nil && dockerCli.IsContainerNotRunning(ident) + checkAlive = false // we check only once; if the container dies later we learn about that through events + return res + } + } +} + +func cancellationErr(w http.ResponseWriter, err error) bool { + if _, ok := err.(*errorCancelled); ok { + common.Log.Infoln("[allocator]:", err.Error()) + fmt.Fprint(w, "cancelled") + return true + } + return false +} + 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, checkAlive, - func() bool { - select { - case <-closedChan: - return true - default: - res := checkAlive && dockerCli != nil && dockerCli.IsContainerNotRunning(ident) - checkAlive = false // we check only once; if the container dies later we learn about that through events - return res - } - }) + hasBeenCanceled(dockerCli, w.(http.CloseNotifier).CloseNotify(), ident, checkAlive)) 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 + if !cancellationErr(w, err) { + badRequest(w, err) } - badRequest(w, err) return } - fmt.Fprintf(w, "%s/%d", addr, subnet.PrefixLen) } +func (alloc *Allocator) handleHTTPClaim(dockerCli *docker.Client, w http.ResponseWriter, ident string, cidr address.CIDR, checkAlive, noErrorOnUnknown bool) { + err := alloc.Claim(ident, cidr, checkAlive, noErrorOnUnknown, + hasBeenCanceled(dockerCli, w.(http.CloseNotifier).CloseNotify(), ident, checkAlive)) + if err != nil { + if !cancellationErr(w, err) { + badRequest(w, fmt.Errorf("Unable to claim: %s", err)) + } + return + } + w.WriteHeader(204) +} + // 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("GET").Path("/ipinfo/defaultsubnet").HandlerFunc(func(w http.ResponseWriter, r *http.Request) { @@ -73,13 +93,10 @@ func (alloc *Allocator) HandleHTTP(router *mux.Router, defaultSubnet address.CID router.Methods("PUT").Path("/ip/{id}/{ip}/{prefixlen}").HandlerFunc(func(w http.ResponseWriter, r *http.Request) { vars := mux.Vars(r) if cidr, ok := parseCIDR(w, vars["ip"]+"/"+vars["prefixlen"], false); ok { - noErrorOnUnknown := r.FormValue("noErrorOnUnknown") == "true" + ident := vars["id"] checkAlive := r.FormValue("check-alive") == "true" - if err := alloc.Claim(vars["id"], cidr, checkAlive, noErrorOnUnknown); err != nil { - badRequest(w, fmt.Errorf("Unable to claim: %s", err)) - return - } - w.WriteHeader(204) + noErrorOnUnknown := r.FormValue("noErrorOnUnknown") == "true" + alloc.handleHTTPClaim(dockerCli, w, ident, cidr, checkAlive, noErrorOnUnknown) } }) diff --git a/test/320_claim_3_test.sh b/test/320_claim_3_test.sh index 2e860534..a956cd57 100755 --- a/test/320_claim_3_test.sh +++ b/test/320_claim_3_test.sh @@ -2,6 +2,8 @@ . ./config.sh +UNIVERSE=10.32.0.0/12 + delete_persistence() { for host in "$@" ; do docker_on $host rm -v weavedb >/dev/null @@ -52,4 +54,22 @@ weave_on $HOST3 prime stop_weave_on $HOST2 weave_on $HOST1 launch-router $HOST3 +stop_weave_on $HOST1 +stop_weave_on $HOST2 +stop_weave_on $HOST3 +delete_persistence $HOST1 $HOST2 $HOST3 + +weave_on $HOST1 launch --ipalloc-range $UNIVERSE $HOST2 +# Start another container on host1. The starting should block, because host1 is +# not able to establish the ring due to host2 being offline. +CMD="proxy_start_container $HOST1 --name c4 -e WEAVE_CIDR=10.32.0.4/12" +assert_raises "timeout 5 cat <( $CMD )" 124 +# However, allocation for an external subnet should not block. +assert_raises "proxy_start_container $HOST1 -e WEAVE_CIDR=10.48.0.1/12" + +# Launch host2, so that host1 can establish the ring. +weave_on $HOST2 launch --ipalloc-range $UNIVERSE $HOST1 +weave_on $HOST1 prime +assert_raises "[ $(container_ip $HOST1 c4) == 10.32.0.4 ]" + end_suite