Merge pull request #1155 from weaveworks/dns-gossip

Make DNS gossip converge correctly

Fixes #1151

nameserver/entry.go

@@ -54,8 +54,9 @@ func (e1 *Entry) merge(e2 *Entry) bool {
 	if e2.Version > e1.Version {
 		e1.Version = e2.Version
 		e1.Tombstone = e2.Tombstone
+		return true
 	}
-	return e2.Version != e1.Version
+	return false
 }
 
 func (e1 *Entry) String() string {
@@ -87,8 +88,10 @@ func (es *Entries) add(hostname, containerid string, origin router.PeerName, add
 		return !(*es)[i].less(&entry)
 	})
 	if i < len(*es) && (*es)[i].equal(&entry) {
-		(*es)[i].Tombstone = 0
-		(*es)[i].Version++
+		if (*es)[i].Tombstone > 0 {
+			(*es)[i].Tombstone = 0
+			(*es)[i].Version++
+		}
 	} else {
 		*es = append(*es, Entry{})
 		copy((*es)[i+1:], (*es)[i:])
@@ -99,24 +102,23 @@ func (es *Entries) add(hostname, containerid string, origin router.PeerName, add
 
 func (es *Entries) merge(incoming Entries) Entries {
 	defer es.checkAndPanic()
-	var (
-		newEntries Entries
-		i          = 0
-	)
+	newEntries := Entries{}
+	i := 0
 
 	for _, entry := range incoming {
 		for i < len(*es) && (*es)[i].less(&entry) {
 			i++
 		}
 		if i < len(*es) && (*es)[i].equal(&entry) {
-			if !(*es)[i].merge(&entry) {
-				continue
+			if (*es)[i].merge(&entry) {
+				newEntries = append(newEntries, entry)
 			}
+		} else {
+			*es = append(*es, Entry{})
+			copy((*es)[i+1:], (*es)[i:])
+			(*es)[i] = entry
+			newEntries = append(newEntries, entry)
 		}
-		*es = append(*es, Entry{})
-		copy((*es)[i+1:], (*es)[i:])
-		(*es)[i] = entry
-		newEntries = append(newEntries, entry)
 	}
 
 	return newEntries

nameserver/entry_test.go

@@ -48,7 +48,13 @@ func TestMerge(t *testing.T) {
 		Entry{Hostname: "F"},
 	}
 
-	e1.merge(e2)
+	diff := e1.merge(e2)
+	expectedDiff := Entries{
+		Entry{Hostname: "B"},
+		Entry{Hostname: "E"},
+	}
+	require.Equal(t, expectedDiff, diff)
+
 	expected := Entries{
 		Entry{Hostname: "A"},
 		Entry{Hostname: "B"},
@@ -57,8 +63,21 @@ func TestMerge(t *testing.T) {
 		Entry{Hostname: "E"},
 		Entry{Hostname: "F"},
 	}
+	require.Equal(t, expected, e1)
+
+	diff = e1.merge(e1)
+	require.Equal(t, Entries{}, diff)
+}
+
+func TestOldMerge(t *testing.T) {
+	e1 := Entries{Entry{Hostname: "A", Version: 0}}
+	diff := e1.merge(Entries{Entry{Hostname: "A", Version: 1}})
+	require.Equal(t, Entries{Entry{Hostname: "A", Version: 1}}, diff)
+	require.Equal(t, Entries{Entry{Hostname: "A", Version: 1}}, e1)
 
-	require.Equal(t, e1, expected)
+	diff = e1.merge(Entries{Entry{Hostname: "A", Version: 0}})
+	require.Equal(t, Entries{}, diff)
+	require.Equal(t, Entries{Entry{Hostname: "A", Version: 1}}, e1)
 }
 
 func TestTombstone(t *testing.T) {

nameserver/nameserver.go

@@ -226,7 +226,10 @@ func (n *Nameserver) receiveGossip(msg []byte) (router.GossipData, router.Gossip
 	}
 
 	newEntries := n.entries.merge(entries)
-	return &newEntries, &entries, nil
+	if len(newEntries) > 0 {
+		return &newEntries, &entries, nil
+	}
+	return nil, &entries, nil
 }
 
 // merge received data into state and return "everything new I've

test/210_dns_cross_hosts_test.sh

@@ -12,14 +12,30 @@ NAME4=seefour.weave.local
 
 start_suite "Resolve names across hosts"
 
-weave_on $HOST1 launch --ipalloc-range $UNIVERSE
-weave_on $HOST2 launch --ipalloc-range $UNIVERSE $HOST1
+for host in $HOSTS; do
+    weave_on $host launch --ipalloc-range $UNIVERSE $HOSTS
+done
 
+# Basic test
 start_container          $HOST2 $C2/24 --name=c2 -h $NAME2
 start_container_with_dns $HOST1 $C1/24 --name=c1
 
 assert_dns_record $HOST1 c1 $NAME2 $C2
 
+# 2 containers on each host, all with the same names
+FOO_IPS=
+for host in $HOSTS; do
+    CID=$(proxy docker_on $host run -dti --name=foo $SMALL_IMAGE /bin/sh)
+    FOO_IPS="$FOO_IPS $(container_ip $host foo)"
+
+    CID=$(proxy docker_on $host run -dti --name=bar $SMALL_IMAGE /bin/sh)
+    BAR_IPS="$BAR_IPS $(container_ip $host bar)"
+done
+
+start_container_with_dns $HOST1 --name=baz
+assert_dns_record $HOST1 baz foo.weave.local $FOO_IPS
+assert_dns_record $HOST1 baz bar.weave.local $BAR_IPS
+
 # resolution for names mapped to multiple addresses
 weave_on $HOST2 dns-add $C2a c2 -h $NAME2
 weave_on $HOST2 dns-add $C2b c2 -h $NAME2

testing/gossip/mocks.go

@@ -92,9 +92,24 @@ func (grouter *TestRouter) run(sender router.PeerName, gossiper router.Gossiper,
 					continue
 				}
 				for _, msg := range message.data.Encode() {
-					if _, err := gossiper.OnGossipBroadcast(message.sender, msg); err != nil {
+					// TODO: this should call OnGossipBroadcast, and we should implement
+					// 'trickle' gossip correctly in this mock.  But no one depends on this
+					// difference for testing right now, so we abuse the interface here.
+					diff, err := gossiper.OnGossip(msg)
+					if err != nil {
 						panic(fmt.Sprintf("Error doing gossip broadcast: %s", err))
 					}
+					if diff == nil {
+						continue
+					}
+					// Sanity check - reconsuming the diff should yield nil
+					for _, diffMsg := range diff.Encode() {
+						if nextDiff, err := gossiper.OnGossip(diffMsg); err != nil {
+							panic(fmt.Sprintf("Error doing gossip broadcast: %s", err))
+						} else if nextDiff != nil {
+							panic(fmt.Sprintf("Breach of gossip interface: %v != nil", nextDiff))
+						}
+					}
 				}
 			}
 		case <-gossipTimer: