From 16415404c74ce05856cca4e2aa9eae2cd700b86e Mon Sep 17 00:00:00 2001 From: Marc CARRE Date: Wed, 2 Nov 2016 16:06:41 +0000 Subject: [PATCH 1/3] Added automated test reproducing the issue. test #4 "[ 3e52b548e6df304c61ea5bee0af4b3515e025e7e25152c65ecae3d03c71cf52d == Unable to claim: address 10.32.0.3/12 is already owned by fbe411facc8b657c5850591ab56fc40fb56160d43db0d7363a29ba3edad61134 Unable to claim: address 10.32.0.1/12 is already owned by cf16a2bcdee8dd1ef7e5f0a611127611e67ea734f45d7526678fc3b1fc0c6f5d 3e52b548e6df304c61ea5bee0af4b3515e025e7e25152c65ecae3d03c71cf52d ]" failed: program terminated with code 2 instead of 0 1 of 6 tests failed in 6.499s. --- test/830_cni_plugin_test.sh | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/test/830_cni_plugin_test.sh b/test/830_cni_plugin_test.sh index f042c294..f6eefaf2 100755 --- a/test/830_cni_plugin_test.sh +++ b/test/830_cni_plugin_test.sh @@ -48,12 +48,28 @@ assert_raises "exec_on $HOST1 c2 $PING $C1IP" # Now remove and start a new container to see if IP address re-use breaks things docker_on $HOST1 rm -f c2 -docker_on $HOST1 run --net=none --name=c3 -dt $SMALL_IMAGE /bin/sh +C3=$(docker_on $HOST1 run --net=none --name=c3 -dt $SMALL_IMAGE /bin/sh) cni_connect $HOST1 c3 <&1) +EXPECTED_OUTPUT=$(docker_on $HOST1 inspect --format="{{.Id}}" weave) + +assert_raises "[ $EXPECTED_OUTPUT == $ACTUAL_OUTPUT ]" + +assert "$SSH $HOST1 \"curl -s -X GET 127.0.0.1:6784/ip/$C1 | grep -o -E '[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}'\"" "$C1IP" +assert "$SSH $HOST1 \"curl -s -X GET 127.0.0.1:6784/ip/$C3 | grep -o -E '[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}'\"" "$C3IP" + + end_suite From bc94633180a9b5f75ade7595fe4d5fa36836f81b Mon Sep 17 00:00:00 2001 From: Marc CARRE Date: Wed, 2 Nov 2016 16:07:11 +0000 Subject: [PATCH 2/3] Re-claim IP on start-up when an existing ID has been found. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Rewrote conditions to keep c.ident immutable and hopefully makes things clearer. - Added a case to tolerate reclaim after we have used CNI (passed CID is “_” and existing CID is a real container ID). --- ipam/claim.go | 26 +++++++++++++++++--------- 1 file changed, 17 insertions(+), 9 deletions(-) diff --git a/ipam/claim.go b/ipam/claim.go index fda36f15..f3803feb 100644 --- a/ipam/claim.go +++ b/ipam/claim.go @@ -77,26 +77,34 @@ func (c *claim) Try(alloc *Allocator) bool { return false } - if c.ident == "_" { // Special "I don't have a unique ID" identifier - c.ident = c.cidr.Addr.String() - } // We are the owner, check we haven't given it to another container - switch existingIdent := alloc.findOwner(c.cidr.Addr); existingIdent { - case "": + existingIdent := alloc.findOwner(c.cidr.Addr) + switch { + case existingIdent == "": + // Unused address, we try to claim it: if err := alloc.space.Claim(c.cidr.Addr); err == nil { alloc.debugln("Claimed", c.cidr, "for", c.ident) - alloc.addOwned(c.ident, c.cidr, c.isContainer) + if c.ident == "_" { // Special "I don't have a unique ID" identifier + alloc.addOwned(c.cidr.Addr.String(), c.cidr, c.isContainer) + } else { + alloc.addOwned(c.ident, c.cidr, c.isContainer) + } c.sendResult(nil) } else { c.sendResult(err) } - case c.ident: + case (existingIdent == c.ident) || (c.ident == "_" && existingIdent == c.cidr.Addr.String()): // same identifier is claiming same address; that's OK alloc.debugln("Re-Claimed", c.cidr, "for", c.ident) c.sendResult(nil) - case c.cidr.Addr.String(): - // Address already allocated via "_" name + case existingIdent == c.cidr.Addr.String(): + // Address already allocated via "_" name and current ID is a real container ID: c.sendResult(fmt.Errorf("address %s already in use", c.cidr)) + case c.ident == "_": + // We do not know whether this is the same container or another one, + // but we also cannot prove otherwise, so we let it reclaim the address: + alloc.debugln("Re-Claimed", c.cidr, "for ID", c.ident, "having existing ID as", existingIdent) + c.sendResult(nil) default: // Addr already owned by container on this machine c.sendResult(fmt.Errorf("address %s is already owned by %s", c.cidr.String(), existingIdent)) From 05449013576a33e59a9e71a017fb9efb621a2c0a Mon Sep 17 00:00:00 2001 From: Marc CARRE Date: Wed, 2 Nov 2016 16:07:44 +0000 Subject: [PATCH 3/3] Factorise "_" duplicates as a constant. --- api/ipam.go | 9 +++++++++ ipam/allocate.go | 3 ++- ipam/claim.go | 9 +++++---- plugin/ipam/driver.go | 7 +++---- 4 files changed, 19 insertions(+), 9 deletions(-) diff --git a/api/ipam.go b/api/ipam.go index 3f15af7f..b42e1959 100644 --- a/api/ipam.go +++ b/api/ipam.go @@ -5,6 +5,15 @@ import ( "net" ) +// Special token used in place of a container identifier when: +// - the caller does not know the container ID, or +// - the caller is unsure whether IPAM was previously told the container ID. +// +// While addresses are typically stored under their container IDs, when this +// token is used, the address will be stored under its own string, something +// which should be kept in mind if the entry needs to be removed at any point. +const NoContainerID string = "_" + func (client *Client) ipamOp(ID string, op string) (*net.IPNet, error) { ip, err := client.httpVerb(op, fmt.Sprintf("/ip/%s", ID), nil) if err != nil { diff --git a/ipam/allocate.go b/ipam/allocate.go index c144edfd..ac542aac 100644 --- a/ipam/allocate.go +++ b/ipam/allocate.go @@ -3,6 +3,7 @@ package ipam import ( "fmt" + "github.com/weaveworks/weave/api" "github.com/weaveworks/weave/net/address" ) @@ -43,7 +44,7 @@ func (g *allocate) Try(alloc *Allocator) bool { if ok, addr := alloc.space.Allocate(g.r.HostRange()); ok { // If caller hasn't supplied a unique ID, file it under the IP address // which lets the caller then release the address using DELETE /ip/address - if g.ident == "_" { + if g.ident == api.NoContainerID { g.ident = addr.String() } alloc.debugln("Allocated", addr, "for", g.ident, "in", g.r) diff --git a/ipam/claim.go b/ipam/claim.go index f3803feb..e027f723 100644 --- a/ipam/claim.go +++ b/ipam/claim.go @@ -5,6 +5,7 @@ import ( "github.com/weaveworks/mesh" + "github.com/weaveworks/weave/api" "github.com/weaveworks/weave/common" "github.com/weaveworks/weave/net/address" ) @@ -84,7 +85,7 @@ func (c *claim) Try(alloc *Allocator) bool { // Unused address, we try to claim it: if err := alloc.space.Claim(c.cidr.Addr); err == nil { alloc.debugln("Claimed", c.cidr, "for", c.ident) - if c.ident == "_" { // Special "I don't have a unique ID" identifier + if c.ident == api.NoContainerID { alloc.addOwned(c.cidr.Addr.String(), c.cidr, c.isContainer) } else { alloc.addOwned(c.ident, c.cidr, c.isContainer) @@ -93,14 +94,14 @@ func (c *claim) Try(alloc *Allocator) bool { } else { c.sendResult(err) } - case (existingIdent == c.ident) || (c.ident == "_" && existingIdent == c.cidr.Addr.String()): + case (existingIdent == c.ident) || (c.ident == api.NoContainerID && existingIdent == c.cidr.Addr.String()): // same identifier is claiming same address; that's OK alloc.debugln("Re-Claimed", c.cidr, "for", c.ident) c.sendResult(nil) case existingIdent == c.cidr.Addr.String(): - // Address already allocated via "_" name and current ID is a real container ID: + // Address already allocated via api.NoContainerID name and current ID is a real container ID: c.sendResult(fmt.Errorf("address %s already in use", c.cidr)) - case c.ident == "_": + case c.ident == api.NoContainerID: // We do not know whether this is the same container or another one, // but we also cannot prove otherwise, so we let it reclaim the address: alloc.debugln("Re-Claimed", c.cidr, "for ID", c.ident, "having existing ID as", existingIdent) diff --git a/plugin/ipam/driver.go b/plugin/ipam/driver.go index 31436995..417e468f 100644 --- a/plugin/ipam/driver.go +++ b/plugin/ipam/driver.go @@ -72,9 +72,8 @@ func splitPoolID(poolID string) (subnet, iprange *net.IPNet, err error) { func (i *Ipam) RequestAddress(poolID string, address net.IP, options map[string]string) (ip *net.IPNet, _ map[string]string, err error) { i.logReq("RequestAddress", poolID, address, options) defer func() { i.logRes("RequestAddress", err, ip) }() - // If we pass magic string "_" to weave IPAM it stores the address under its own string if poolID == "weavepool" { // old-style - ip, err = i.weave.AllocateIP("_") + ip, err = i.weave.AllocateIP(api.NoContainerID) return } subnet, iprange, err := splitPoolID(poolID) @@ -83,12 +82,12 @@ func (i *Ipam) RequestAddress(poolID string, address net.IP, options map[string] } if address != nil { // try to claim specific address requested ip = &net.IPNet{IP: address, Mask: subnet.Mask} - if err = i.weave.ClaimIP("_", ip); err != nil { + if err = i.weave.ClaimIP(api.NoContainerID, ip); err != nil { return } } else { // We are lying slightly to IPAM here: the range is not a subnet - if ip, err = i.weave.AllocateIPInSubnet("_", iprange); err != nil { + if ip, err = i.weave.AllocateIPInSubnet(api.NoContainerID, iprange); err != nil { return } ip.Mask = subnet.Mask // fix up the subnet we lied about