Overlay fix for transient IP reuse #1935

fcrisciani · 2017-09-07T21:14:25Z

It is possible that for a limited period of time the same IP and Mac address are used at the same time.
The previous logic was not able to handle the condition creating connectivity issues that were lasting up to 5 minutes.

Fixes: #1934

fcrisciani · 2017-09-07T21:15:01Z

drivers/overlay/joinleave.go


-	if err := d.checkEncryption(nid, nil, 0, true, false); err != nil {


checkEncryption is done inside the peerDelete

fcrisciani · 2017-09-07T21:15:56Z

drivers/overlay/ov_network.go

@@ -1090,15 +1060,6 @@ func (n *network) contains(ip net.IP) bool {
 	return false
 }

-func (n *network) getSubnetforIPAddr(ip net.IP) *subnet {


codecov-io · 2017-09-07T23:48:23Z

Codecov Report

❗ No coverage uploaded for pull request base (master@389e1e6). Click here to learn what that means.
The diff coverage is 0.58%.

@@            Coverage Diff            @@
##             master    #1935   +/-   ##
=========================================
  Coverage          ?   37.88%           
=========================================
  Files             ?      137           
  Lines             ?    27325           
  Branches          ?        0           
=========================================
  Hits              ?    10351           
  Misses            ?    15703           
  Partials          ?     1271

Impacted Files	Coverage Δ
drivers/overlay/encryption.go	`0% <ø> (ø)`
drivers/overlay/joinleave.go	`0% <0%> (ø)`
drivers/overlay/peerdb.go	`3.81% <0%> (ø)`
drivers/overlay/ov_serf.go	`30.86% <0%> (ø)`
drivers/overlay/ov_network.go	`0.26% <0%> (ø)`
networkdb/networkdb.go	`60% <0%> (ø)`
osl/neigh_linux.go	`0% <0%> (ø)`
drivers/overlay/overlay.go	`27.52% <100%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 389e1e6...d93b9b0. Read the comment docs.

thaJeztah · 2017-09-18T19:21:54Z

ping @mavenugo @abhinandanpb PTAL; this one should be fixing various stability issues

thaJeztah

Left some nits, and there's some code churn between commits, but would like @mavenugo or @abhinandanpb to have a look as they are probably more familiar with the code 😄

thaJeztah · 2017-09-20T10:51:16Z

drivers/overlay/joinleave.go

-	if err := d.checkEncryption(nid, nil, 0, true, false); err != nil {
-		logrus.Warn(err)
-	}
+	// if err := d.checkEncryption(nid, nil, 0, true, false); err != nil {


can you remove this code instead of commenting it out in this commit?

thaJeztah · 2017-09-20T10:52:58Z

drivers/overlay/joinleave.go

@@ -232,13 +233,9 @@ func (d *driver) Leave(nid, eid string) error {
 		}
 	}

-	n.leaveSandbox()
-
-	// if err := d.checkEncryption(nid, nil, 0, true, false); err != nil {


removing this code should probably be in the previous commit (see my other comment) 😄

thaJeztah · 2017-09-20T10:55:31Z

drivers/overlay/joinleave.go

@@ -68,7 +68,7 @@ func (d *driver) Join(nid, eid string, sboxKey string, jinfo driverapi.JoinInfo,

 	ep.ifName = containerIfName

-	if err := d.writeEndpointToStore(ep); err != nil {
+	if err = d.writeEndpointToStore(ep); err != nil {


Is there a reason you changed these? Overall I think it's clearer to just create a new variable here, as the error is not used outside the if. (It makes the code a bit easier to "grasp", because you know the result is not used outside of the if 😄)

I just changed it as a result of a warning from one of the code checker tools, I can change it back but I also saw that in some other places the proper outer err was never set, so shadowing is risky

thaJeztah · 2017-09-20T10:59:27Z

osl/neigh_linux.go

@@ -188,7 +188,7 @@ func (n *networkNamespace) AddNeighbor(dstIP net.IP, dstMac net.HardwareAddr, fo
 	n.Lock()
 	n.neighbors = append(n.neighbors, nh)
 	n.Unlock()
-	logrus.Debugf("Neighbor entry added for IP %v, mac %v", dstIP, dstMac)
+	logrus.Debugf("Neighbor entry added for IP %v, mac %v on ifc:%s", dstIP, dstMac, nh.linkName)


add a space after ifc:

When I print field especially names or ids, I prefer to not put space between the : in order to make it clear if there is some unprintable or transparent char. IMO the delimiter to help readability is actually the ':'. Does it make sense?

would quoting work? %q? Point is that it's not done like that in many places, so if would still be ambiguous probably

yeah realigning the whole project I think is an endless work, maybe I can update all the fields of this log to be consistent at least

It's not a huge issue, just noticed while looking :)

thaJeztah · 2017-09-20T11:06:29Z

drivers/overlay/joinleave.go

@@ -224,6 +224,7 @@ func (d *driver) Leave(nid, eid string) error {
 		return types.InternalMaskableErrorf("could not find endpoint with id %s", eid)
 	}

+	logrus.Errorf("The channel is valid:%t", d.notifyCh != nil)


This error message is a bit odd "the channel is valid"; should this be "invalid"? Also missing a space after :.

Consider using just Error instead of Errorf, because printing booleans doesn't need formatting;

logrus.Error("The channel is valid: ", d.notifyCh != nil)

sorry my bad, this is an error logs that I put for debug, I will remove it

thaJeztah · 2017-09-20T11:17:00Z

drivers/overlay/ov_network.go

-				// When we get a L3 miss check if its a valid peer and reprogram the neighbor
-				// entry again. Rate limit it to once attempt every 500ms, just in case a faulty
-				// container sends a flood of packets to invalid peers
-				if !l3Miss {


You're removing this check in this commit, but adding it again in the last commit?

thaJeztah · 2017-09-20T11:17:54Z

drivers/overlay/ov_network.go

-					logrus.Warnf("miss notification for peer:%+v l3Miss:%t l2Miss:%t, if the problem persist check the gc_thresholds", neigh, l3Miss, l2Miss)
+					pKey, pEntry, err := n.driver.peerDbSearch(n.id, ip)
+					if !pEntry.isLocal {
+						logrus.Warnf("miss notification for peer:%+v l3Miss:%t l2Miss:%t, if the problem persist check the gc_thresh on the host pKey:%+v pEntry:%+v err:%v",


Can you add spaces after the colons here as well? (peer: %+v etc)

thaJeztah · 2017-09-20T11:18:55Z

drivers/overlay/ov_network.go

-				if time.Since(t) > 500*time.Millisecond {
+				// The time limit here is to guarantee that the dbSearch is not
+				// done too frequently causing a stall of the peerDB operations.
+				if l3Miss && time.Since(t) > 500*time.Millisecond {


The l3Miss check was removed in an earlier commit, and already had a description for the 500ms; perhaps restore that check and description to keep the diff smaller

thaJeztah · 2017-09-20T11:25:02Z

drivers/overlay/peerdb.go

+	if i != 1 {
+		// Transient case, there is more than one endpoint that is using the same IP,MAC pair
+		s, _ := pMap.mp.String(pKey.String())
+		logrus.Warnf("peerDbAdd transient condition - Key:%s cardinality:%d db state:%s", pKey.String(), i, s)


Can you add a space after the colons here (for readability)?

thaJeztah · 2017-09-20T11:25:18Z

drivers/overlay/peerdb.go

+	if i != 0 {
+		// Transient case, there is more than one endpoint that is using the same IP,MAC pair
+		s, _ := pMap.mp.String(pKey.String())
+		logrus.Warnf("peerDbDelete transient condition - Key:%s cardinality:%d db state:%s", pKey.String(), i, s)


Can you add a space after the colons here (for readability)?

(same for other errors)

allencloud · 2017-09-20T12:12:48Z

drivers/overlay/ov_network.go

+				// Entries which are marked as permanent are never deleted by the garbage-collector.
+				// The time limit here is to guarantee that the dbSearch is not
+				// done too frequently causing a stall of the peerDB operations.
+				if l3Miss && time.Since(t) > 500*time.Millisecond {


I recommend that we could combine the else and if into else if.
Tiny point. 😄

sure will do

nishanttotla

Is this PR expected to be part of an upcoming release, or there's no timeline on it yet?

fcrisciani · 2017-09-25T16:13:11Z

@nishanttotla I would like so, but still stuck on code review.
@mavenugo @abhinandanpb @vieux PTAL

abhi · 2017-09-25T16:44:35Z

@fcrisciani will review this today

vieux · 2017-09-26T16:58:00Z

@abhinandanpb ping :)

mavenugo · 2017-09-26T20:22:48Z

drivers/overlay/ov_network.go

-				if time.Since(t) > 500*time.Millisecond {
-					t = time.Now()
-					n.programNeighbor(ip)
+			} else if l3Miss && time.Since(t) > 500*time.Millisecond {


I know this is existing code, I think 500ms is aggressive. we can even double it to 1second. wdyt ?

Now that this is just a warning message without any action, we can even increase this timer to avoid excessive logs.

sure can bump to 1sec

mavenugo · 2017-09-27T01:51:50Z

drivers/overlay/peerdb.go

+
+	// If there is still an entry into the database and the deletion went through without errors means that there is now no
+	// configuration active in the kernel.
+	// Restore one configuration for the <ip,mac> directly from the database, note that is guaranteed that there is one


Its guaranteed to be one or more. Correct ?

yep 4 lines above the if dbEntries == 0 { return nil }

abhi

Overall LGTM. I have one concern about the scenario I have mentioned in the comment.

Alternate thought not necessary to be implemented: Should we treat this problem as a classic networking mac move scenario. If we detect a mac move , unicast arp for it ?

abhi · 2017-09-27T03:41:26Z

drivers/overlay/peerdb.go

+	// If there is still an entry into the database and the deletion went through without errors means that there is now no
+	// configuration active in the kernel.
+	// Restore one configuration for the <ip,mac> directly from the database, note that is guaranteed that there is one
+	peerKey, peerEntry, err := d.peerDbSearch(nid, peerIP)


Will it have more than 1 entry for the same IP if there is continuous container add deletes ? From what I see of the internal map set implementation its a map of maps. https://github.com/deckarep/golang-set/blob/master/threadunsafe.go#L36. If thats the case the order will not be honored. Would that be a problem ? How is the peerDbdelete going to handle that ? For eg lets have a node n100,
C1 is on n1 , fdb on n100 will have c1 against 10.1.1.1, n1
C1 appears on n2 , fdb will have c1 still against 10.1.1.1 on n1 , peerdb mapset cache will have this new entry until the previous entry is deleted
now C1 cppears on n3 , fdb might still have the transient state against 10.1.1.1 on n1 so the peerdb cache will have 2 entries.
At this point we will receive the delete for the entry against n1. Now the peerdb search will fetch one of them ? For our scenario lets say we pick the entry for C1 pointing to n3. Now if we get a delete for the entry C1 pointing to n2 - how would that proceed ?

Of course we are considering a transient period that should converge "pretty quickly" (according to the timing of the networkdb in delivering notification). The idea here is:

push in the kernel the first configuration

if any other notification arrives for the same IP, save it in the Set

once the delete notification arrives if it deletes the current configuration in the kernel, take the next one available and push it to the kernel (note this can still be not the final one)

the expected state at the end is to have only 1 entry in the set and that is the one that is also configured in the kernel.

For your example let's say that you receive the delete for the entry C1, n2, then nothing happens, remove it from the set and contine.
After that you receive the delete for C1, n100, that is the entry configured in the kernel, then the kernel state is going to be purged and the next entry is configured, in this case C1, n3.
The set now contains only 1 element that is actually the one configured and the only real incarnation of the container C1 with the correct peer destination

mavenugo · 2017-09-28T08:54:58Z

drivers/overlay/peerdb.go

+	eid            string
+	vtep           string
+	peerIPMaskOnes int
+	peerIPMaskBits int


Why is peerIPMask, a net.IPMask converted to couple of int, while the vtep net.IP converted to String ?
Isnt it easy to just marshal peerIPMask as a String and that makes it consistent and readable ?

it's only convenience, for the net.IPMask looks like the best way to initialize it back is using that decomposition. Ref: https://golang.org/pkg/net/#IPMask

Checked again and looks like this can be converted to string and use the ParseCIDR that will return the IPNet that contains the IPMask object internally, will change

mavenugo

@fcrisciani am done with my review. Pls address the comments before we can merge this PR.

In case of IP reuse locally there was a race condition that was leaving the overlay namespace with wrong configuration causing connectivity issues. This commit introduces the use of setMatrix to handle the transient state and make sure that the proper configuration is maintained Signed-off-by: Flavio Crisciani <flavio.crisciani@docker.com>

peerDB was never being flushed on network delete leaveing behind stale entries Signed-off-by: Flavio Crisciani <flavio.crisciani@docker.com>

Signed-off-by: Flavio Crisciani <flavio.crisciani@docker.com>

Avoid error logs in case of local peer case, there is no need for deleteNeighbor Avoid the network leave to readvertise already deleted entries to upper layer Signed-off-by: Flavio Crisciani <flavio.crisciani@docker.com>

fcrisciani · 2017-10-03T00:42:45Z

drivers/overlay/peerdb.go

-			// was never been configured into the kernel (we allow only 1 configuration at the time per <ip,mac> mapping)
-			return nil
+	// Local peers do not have any local configuration to delete
+	if !localPeer {


The body internally is only indented in the new if condition

mavenugo · 2017-10-03T14:36:08Z

@fcrisciani thanks for addressing the comments.

mavenugo

LGTM

Nossnevs · 2017-10-04T11:23:42Z

When will this fix be released?
@fcrisciani @mavenugo

kleptog · 2017-10-09T08:30:27Z

And will it be backported to 17.03? We found 17.06 fairly flaky and 17.09 is pretty new.

thaJeztah · 2017-10-09T14:15:00Z

@kleptog Docker CE (Community Edition) 17.03 has reached end of life (Docker CE 17.06 soon); Docker EE (Enterprise Edition) has a longer support duration (12 Months currently), but I don't know if this is planned to be backported. It's also possible that 17.03 does not have the same issue (may have the same effect, but could be a different cause)

fcrisciani added the status/2-needs-code-review label Sep 7, 2017

fcrisciani requested a review from mavenugo September 7, 2017 21:14

fcrisciani commented Sep 7, 2017

View reviewed changes

fcrisciani force-pushed the overlay-setmatrix branch from 5d228bb to 89b266e Compare September 7, 2017 23:07

thaJeztah requested changes Sep 20, 2017

View reviewed changes

allencloud reviewed Sep 20, 2017

View reviewed changes

fcrisciani force-pushed the overlay-setmatrix branch from 57a9315 to dc36fcb Compare September 20, 2017 16:27

nishanttotla reviewed Sep 21, 2017

View reviewed changes

fcrisciani mentioned this pull request Sep 21, 2017

Swarm is having occasional network connection problems between nodes. moby/moby#32195

Closed

andrewhsu mentioned this pull request Sep 25, 2017

Swarm Mode assigns similar IP addresses for different services moby/moby#33402

Open

mavenugo reviewed Sep 27, 2017

View reviewed changes

abhi reviewed Sep 27, 2017

View reviewed changes

mavenugo reviewed Sep 28, 2017

View reviewed changes

mavenugo requested changes Sep 28, 2017

View reviewed changes

Nossnevs mentioned this pull request Sep 29, 2017

One of containers intermittently can NOT access another container in same overlay network #1946

Closed

Flavio Crisciani added 5 commits October 2, 2017 11:12

flush peerdb entries on network delete

2ec096a

peerDB was never being flushed on network delete leaveing behind stale entries Signed-off-by: Flavio Crisciani <flavio.crisciani@docker.com>

log for miss notification

097b363

Signed-off-by: Flavio Crisciani <flavio.crisciani@docker.com>

Addressing code review comments

b12d63c

Signed-off-by: Flavio Crisciani <flavio.crisciani@docker.com>

Changed ipMask to string

d93b9b0

Avoid error logs in case of local peer case, there is no need for deleteNeighbor Avoid the network leave to readvertise already deleted entries to upper layer Signed-off-by: Flavio Crisciani <flavio.crisciani@docker.com>

fcrisciani force-pushed the overlay-setmatrix branch from dc36fcb to d93b9b0 Compare October 3, 2017 00:30

fcrisciani commented Oct 3, 2017

View reviewed changes

mavenugo approved these changes Oct 3, 2017

View reviewed changes

mavenugo merged commit 7447e54 into moby:master Oct 3, 2017

fcrisciani mentioned this pull request Oct 3, 2017

[Backport 17.06] Overlay fix for transient IP reuse #1965

Merged

fcrisciani deleted the overlay-setmatrix branch October 3, 2017 17:22

fcrisciani mentioned this pull request Oct 10, 2017

Docker Swarm Mode service discovery is totally unstable moby/moby#33589

Closed

andrewhsu mentioned this pull request Oct 11, 2017

[17.10] re-vndr libnetwork to 6c51292 and swarmkit to 872861d docker-archive/docker-ce#264

Merged

thaJeztah mentioned this pull request Nov 20, 2017

[17.09] Overlay fix for transient IP reuse #2016

Merged

thaJeztah mentioned this pull request Jan 4, 2018

Wrong behaviour of the DNS resolver within Swarm Mode overlay network moby/moby#30134

Closed


		if err := d.checkEncryption(nid, nil, 0, true, false); err != nil {

Overlay fix for transient IP reuse #1935

Overlay fix for transient IP reuse #1935

Conversation

fcrisciani commented Sep 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Sep 7, 2017 • edited Loading

Codecov Report

thaJeztah commented Sep 18, 2017

thaJeztah left a 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

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

nishanttotla left a comment

Choose a reason for hiding this comment

fcrisciani commented Sep 25, 2017

abhi commented Sep 25, 2017

vieux commented Sep 26, 2017

Choose a reason for hiding this comment

mavenugo Sep 27, 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

abhi left a 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

mavenugo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mavenugo commented Oct 3, 2017

mavenugo left a comment

Choose a reason for hiding this comment

Nossnevs commented Oct 4, 2017 • edited Loading

kleptog commented Oct 9, 2017

thaJeztah commented Oct 9, 2017

codecov-io commented Sep 7, 2017 •

edited

Loading

mavenugo Sep 27, 2017 •

edited

Loading

Nossnevs commented Oct 4, 2017 •

edited

Loading