From fc1d83750150c617114c3c059f01ffccfb9dbd77 Mon Sep 17 00:00:00 2001 From: Bryan Boreham Date: Thu, 29 Oct 2015 15:47:11 +0000 Subject: [PATCH 1/8] Extend unit test to cover mixed-case hostnames and daisy-chained gossip; reduce mock gossip timer to 2 seconds to trigger more often during tests. --- nameserver/nameserver_test.go | 23 +++++++++++++++++++++-- testing/gossip/mocks.go | 2 +- 2 files changed, 22 insertions(+), 3 deletions(-) diff --git a/nameserver/nameserver_test.go b/nameserver/nameserver_test.go index 581fe321..5b518ad8 100644 --- a/nameserver/nameserver_test.go +++ b/nameserver/nameserver_test.go @@ -83,6 +83,10 @@ func testNameservers(t *testing.T) { lookupTimeout := 10 // ms nameservers, grouter := makeNetwork(50) defer stopNetwork(nameservers, grouter) + // This subset will sometimes lose touch with some of the others + badNameservers := nameservers[45:] + // This subset will remain well-connected, and we will deal mainly with them + nameservers = nameservers[:45] nameserversByName := map[router.PeerName]*Nameserver{} for _, n := range nameservers { nameserversByName[n.ourName] = n @@ -106,7 +110,13 @@ func testNameservers(t *testing.T) { addMapping := func() { nameserver := nameservers[rand.Intn(len(nameservers))] addr := address.Address(rand.Int31()) - hostname := fmt.Sprintf("hostname%d", rand.Int63()) + // Create a hostname which has some upper and lowercase letters, + // and a unique number so we don't have to check if we allocated it already + randomBits := rand.Int63() + firstLetter := 'H' + (randomBits&1)*32 + secondLetter := 'O' + (randomBits&2)*16 + randomBits = randomBits >> 2 + hostname := fmt.Sprintf("%c%cstname%d", firstLetter, secondLetter, randomBits) mapping := mapping{hostname, []pair{{nameserver.ourName, addr}}} mappings = append(mappings, mapping) @@ -129,6 +139,12 @@ func testNameservers(t *testing.T) { check(nameserver, mapping) } + loseConnection := func() { + nameserver1 := badNameservers[rand.Intn(len(badNameservers))] + nameserver2 := nameservers[rand.Intn(len(nameservers))] + nameserver1.PeerGone(&router.Peer{Name: nameserver2.ourName}) + } + deleteMapping := func() { if len(mappings) <= 0 { return @@ -191,7 +207,10 @@ func testNameservers(t *testing.T) { case 0.2 <= r && r < 0.3: deleteMapping() - case 0.3 <= r && r < 0.9: + case 0.3 <= r && r < 0.35: + loseConnection() + + case 0.35 <= r && r < 0.9: doLookup() case 0.9 <= r: diff --git a/testing/gossip/mocks.go b/testing/gossip/mocks.go index ebc63242..ae5791f1 100644 --- a/testing/gossip/mocks.go +++ b/testing/gossip/mocks.go @@ -97,7 +97,7 @@ type TestRouterClient struct { } func (grouter *TestRouter) run(sender router.PeerName, gossiper router.Gossiper, gossipChan chan interface{}) { - gossipTimer := time.Tick(10 * time.Second) + gossipTimer := time.Tick(2 * time.Second) for { select { case gossip := <-gossipChan: From 64f7953423f7e6b980bd243f548b6b05941aff23 Mon Sep 17 00:00:00 2001 From: Bryan Boreham Date: Thu, 29 Oct 2015 17:23:11 +0000 Subject: [PATCH 2/8] Add unit test to check outbound mixed-case merging --- nameserver/entry_test.go | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/nameserver/entry_test.go b/nameserver/entry_test.go index 96d2b883..4a84d143 100644 --- a/nameserver/entry_test.go +++ b/nameserver/entry_test.go @@ -130,3 +130,31 @@ func TestLookup(t *testing.T) { } require.Equal(t, have, want) } + +func TestGossipDataMerge(t *testing.T) { + g1 := GossipData{Entries: Entries{ + Entry{Hostname: "A"}, + Entry{Hostname: "c"}, + Entry{Hostname: "D"}, + Entry{Hostname: "f"}, + }} + + g2 := GossipData{Entries: Entries{ + Entry{Hostname: "B"}, + Entry{Hostname: "E"}, + Entry{Hostname: "f"}, + }} + + g1.Merge(&g2) + + expected := GossipData{Entries: Entries{ + Entry{Hostname: "A"}, + Entry{Hostname: "B"}, + Entry{Hostname: "c"}, + Entry{Hostname: "D"}, + Entry{Hostname: "E"}, + Entry{Hostname: "f"}, + }} + + require.Equal(t, expected, g1) +} From f485b729bce77946079a0103105c552c7dc62618 Mon Sep 17 00:00:00 2001 From: Bryan Boreham Date: Thu, 29 Oct 2015 17:57:24 +0000 Subject: [PATCH 3/8] Hold all entries sorted case-insensitively until just before sending, when we re-sort case-sensitively in Encode() --- nameserver/entry.go | 11 +++++++---- nameserver/nameserver.go | 1 - 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/nameserver/entry.go b/nameserver/entry.go index 52e901a5..3b45fd0a 100644 --- a/nameserver/entry.go +++ b/nameserver/entry.go @@ -233,8 +233,8 @@ type GossipData struct { } func (g *GossipData) Merge(o router.GossipData) { - checkAndPanic(CaseSensitive(g.Entries)) - defer func() { checkAndPanic(CaseSensitive(g.Entries)) }() + checkAndPanic(CaseInsensitive(g.Entries)) + defer func() { checkAndPanic(CaseInsensitive(g.Entries)) }() other := o.(*GossipData) g.Entries.merge(other.Entries) if g.Timestamp < other.Timestamp { @@ -243,9 +243,12 @@ func (g *GossipData) Merge(o router.GossipData) { } func (g *GossipData) Encode() [][]byte { - checkAndPanic(CaseSensitive(g.Entries)) + // Make a copy so we can sort: all outgoing data is sent in case-sensitive order + g2 := GossipData{Timestamp: g.Timestamp, Entries: make(Entries, len(g.Entries))} + copy(g2.Entries, g.Entries) + sort.Sort(CaseSensitive(g2.Entries)) buf := &bytes.Buffer{} - if err := gob.NewEncoder(buf).Encode(g); err != nil { + if err := gob.NewEncoder(buf).Encode(g2); err != nil { panic(err) } return [][]byte{buf.Bytes()} diff --git a/nameserver/nameserver.go b/nameserver/nameserver.go index 19037804..a2e9f238 100644 --- a/nameserver/nameserver.go +++ b/nameserver/nameserver.go @@ -196,7 +196,6 @@ func (n *Nameserver) Gossip() router.GossipData { Timestamp: now(), } copy(gossip.Entries, n.entries) - sort.Sort(CaseSensitive(gossip.Entries)) return gossip } From ed9ade05ea296ca853b888dea2b98a1005a760e1 Mon Sep 17 00:00:00 2001 From: Bryan Boreham Date: Fri, 30 Oct 2015 09:58:14 +0000 Subject: [PATCH 4/8] Improve merge asserts --- nameserver/entry.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/nameserver/entry.go b/nameserver/entry.go index 3b45fd0a..dd11f5b5 100644 --- a/nameserver/entry.go +++ b/nameserver/entry.go @@ -145,6 +145,7 @@ func (es *Entries) add(hostname, containerid string, origin router.PeerName, add func (es *Entries) merge(incoming Entries) Entries { defer es.checkAndPanic().checkAndPanic() + incoming.checkAndPanic() newEntries := Entries{} i := 0 @@ -233,8 +234,6 @@ type GossipData struct { } func (g *GossipData) Merge(o router.GossipData) { - checkAndPanic(CaseInsensitive(g.Entries)) - defer func() { checkAndPanic(CaseInsensitive(g.Entries)) }() other := o.(*GossipData) g.Entries.merge(other.Entries) if g.Timestamp < other.Timestamp { From 35f57b3ed26e617c557288b448f183bf08623a50 Mon Sep 17 00:00:00 2001 From: Bryan Boreham Date: Fri, 30 Oct 2015 10:12:40 +0000 Subject: [PATCH 5/8] Move decoding code next to encoding code --- nameserver/entry.go | 9 +++++++++ nameserver/nameserver.go | 8 +------- 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/nameserver/entry.go b/nameserver/entry.go index dd11f5b5..312bcd4e 100644 --- a/nameserver/entry.go +++ b/nameserver/entry.go @@ -241,6 +241,15 @@ func (g *GossipData) Merge(o router.GossipData) { } } +func (g *GossipData) Decode(msg []byte) error { + if err := gob.NewDecoder(bytes.NewReader(msg)).Decode(g); err != nil { + return err + } + + sort.Sort(CaseInsensitive(g.Entries)) + return nil +} + func (g *GossipData) Encode() [][]byte { // Make a copy so we can sort: all outgoing data is sent in case-sensitive order g2 := GossipData{Timestamp: g.Timestamp, Entries: make(Entries, len(g.Entries))} diff --git a/nameserver/nameserver.go b/nameserver/nameserver.go index a2e9f238..f85e5482 100644 --- a/nameserver/nameserver.go +++ b/nameserver/nameserver.go @@ -1,10 +1,7 @@ package nameserver import ( - "bytes" - "encoding/gob" "fmt" - "sort" "sync" "time" @@ -205,16 +202,13 @@ func (n *Nameserver) OnGossipUnicast(sender router.PeerName, msg []byte) error { func (n *Nameserver) receiveGossip(msg []byte) (router.GossipData, router.GossipData, error) { var gossip GossipData - if err := gob.NewDecoder(bytes.NewReader(msg)).Decode(&gossip); err != nil { + if err := gossip.Decode(msg); err != nil { return nil, nil, err } - if delta := gossip.Timestamp - now(); delta > gossipWindow || delta < -gossipWindow { return nil, nil, fmt.Errorf("clock skew of %d detected", delta) } - sort.Sort(CaseInsensitive(gossip.Entries)) - n.Lock() defer n.Unlock() From fc7d04ce73ca8729b21a64076cd4a150a0f9ea2e Mon Sep 17 00:00:00 2001 From: Bryan Boreham Date: Fri, 30 Oct 2015 17:48:22 +0000 Subject: [PATCH 6/8] Condense Entries unit tests with helper function --- nameserver/entry_test.go | 78 ++++++++++------------------------------ 1 file changed, 18 insertions(+), 60 deletions(-) diff --git a/nameserver/entry_test.go b/nameserver/entry_test.go index 4a84d143..e0640c1f 100644 --- a/nameserver/entry_test.go +++ b/nameserver/entry_test.go @@ -9,6 +9,14 @@ import ( "github.com/weaveworks/weave/router" ) +func makeEntries(values string) Entries { + entries := make(Entries, len(values)) + for i, c := range values { + entries[i] = Entry{Hostname: string(c)} + } + return entries +} + func TestAdd(t *testing.T) { oldNow := now defer func() { now = oldNow }() @@ -35,35 +43,13 @@ func TestAdd(t *testing.T) { } func TestMerge(t *testing.T) { - e1 := Entries{ - Entry{Hostname: "A"}, - Entry{Hostname: "C"}, - Entry{Hostname: "D"}, - Entry{Hostname: "F"}, - } - - e2 := Entries{ - Entry{Hostname: "B"}, - Entry{Hostname: "E"}, - Entry{Hostname: "F"}, - } + e1 := makeEntries("ACDF") + e2 := makeEntries("BEF") 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"}, - Entry{Hostname: "C"}, - Entry{Hostname: "D"}, - Entry{Hostname: "E"}, - Entry{Hostname: "F"}, - } - require.Equal(t, expected, e1) + require.Equal(t, makeEntries("BE"), diff) + require.Equal(t, makeEntries("ABCDEF"), e1) diff = e1.merge(e1) require.Equal(t, Entries{}, diff) @@ -85,10 +71,7 @@ func TestTombstone(t *testing.T) { defer func() { now = oldNow }() now = func() int64 { return 1234 } - es := Entries{ - Entry{Hostname: "A"}, - Entry{Hostname: "B"}, - } + es := makeEntries("AB") es.tombstone(router.UnknownPeerName, func(e *Entry) bool { return e.Hostname == "B" @@ -101,18 +84,12 @@ func TestTombstone(t *testing.T) { } func TestDelete(t *testing.T) { - es := Entries{ - Entry{Hostname: "A"}, - Entry{Hostname: "B"}, - } + es := makeEntries("AB") es.filter(func(e *Entry) bool { return e.Hostname != "A" }) - expected := Entries{ - Entry{Hostname: "B"}, - } - require.Equal(t, expected, es) + require.Equal(t, makeEntries("B"), es) } func TestLookup(t *testing.T) { @@ -132,29 +109,10 @@ func TestLookup(t *testing.T) { } func TestGossipDataMerge(t *testing.T) { - g1 := GossipData{Entries: Entries{ - Entry{Hostname: "A"}, - Entry{Hostname: "c"}, - Entry{Hostname: "D"}, - Entry{Hostname: "f"}, - }} - - g2 := GossipData{Entries: Entries{ - Entry{Hostname: "B"}, - Entry{Hostname: "E"}, - Entry{Hostname: "f"}, - }} + g1 := GossipData{Entries: makeEntries("AcDf")} + g2 := GossipData{Entries: makeEntries("BEf")} g1.Merge(&g2) - expected := GossipData{Entries: Entries{ - Entry{Hostname: "A"}, - Entry{Hostname: "B"}, - Entry{Hostname: "c"}, - Entry{Hostname: "D"}, - Entry{Hostname: "E"}, - Entry{Hostname: "f"}, - }} - - require.Equal(t, expected, g1) + require.Equal(t, GossipData{Entries: makeEntries("ABcDEf")}, g1) } From e5c5fbcea346f5d7be893f0404df3d91ad270607 Mon Sep 17 00:00:00 2001 From: Bryan Boreham Date: Mon, 2 Nov 2015 14:10:47 +0000 Subject: [PATCH 7/8] Hold on to the lower-case version of a hostname, rather than re-computing it every time --- nameserver/entry.go | 25 +++++++++++++++----- nameserver/entry_test.go | 43 +++++++++++++++++++---------------- nameserver/nameserver_test.go | 4 ++-- 3 files changed, 45 insertions(+), 27 deletions(-) diff --git a/nameserver/entry.go b/nameserver/entry.go index 312bcd4e..a3310423 100644 --- a/nameserver/entry.go +++ b/nameserver/entry.go @@ -18,7 +18,8 @@ type Entry struct { ContainerID string Origin router.PeerName Addr address.Address - Hostname string + Hostname string // as supplied + lHostname string // lowercased (not exported, so not encoded by gob) Version int Tombstone int64 // timestamp of when it was deleted } @@ -69,7 +70,7 @@ func (e1 *Entry) less(e2 *Entry) bool { func (e1 *Entry) insensitiveLess(e2 *Entry) bool { // Entries are kept sorted by Hostname, Origin, ContainerID then address - e1Hostname, e2Hostname := strings.ToLower(e1.Hostname), strings.ToLower(e2.Hostname) + e1Hostname, e2Hostname := e1.lHostname, e2.lHostname switch { case e1Hostname != e2Hostname: return e1Hostname < e2Hostname @@ -100,6 +101,10 @@ func (e1 *Entry) String() string { return fmt.Sprintf("%s -> %s", e1.Hostname, e1.Addr.String()) } +func (e1 *Entry) addLowercase() { + e1.lHostname = strings.ToLower(e1.Hostname) +} + func check(es SortableEntries) error { if !sort.IsSorted(es) { return fmt.Errorf("Not sorted!") @@ -126,7 +131,8 @@ func (es *Entries) checkAndPanic() *Entries { func (es *Entries) add(hostname, containerid string, origin router.PeerName, addr address.Address) Entry { defer es.checkAndPanic().checkAndPanic() - entry := Entry{Hostname: hostname, Origin: origin, ContainerID: containerid, Addr: addr} + entry := Entry{Hostname: hostname, lHostname: strings.ToLower(hostname), + Origin: origin, ContainerID: containerid, Addr: addr} i := sort.Search(len(*es), func(i int) bool { return !(*es)[i].insensitiveLess(&entry) }) @@ -204,14 +210,14 @@ func (es Entries) lookup(hostname string) Entries { lowerHostname := strings.ToLower(hostname) i := sort.Search(len(es), func(i int) bool { - return strings.ToLower(es[i].Hostname) >= lowerHostname + return es[i].lHostname >= lowerHostname }) - if i >= len(es) || strings.ToLower(es[i].Hostname) != lowerHostname { + if i >= len(es) || es[i].lHostname != lowerHostname { return Entries{} } j := sort.Search(len(es)-i, func(j int) bool { - return strings.ToLower(es[i+j].Hostname) > lowerHostname + return es[i+j].lHostname > lowerHostname }) return es[i : i+j] @@ -228,6 +234,12 @@ func (es Entries) first(f func(*Entry) bool) (*Entry, error) { return nil, fmt.Errorf("Not found") } +func (es Entries) addLowercase() { + for i := range es { + es[i].addLowercase() + } +} + type GossipData struct { Timestamp int64 Entries @@ -246,6 +258,7 @@ func (g *GossipData) Decode(msg []byte) error { return err } + g.Entries.addLowercase() // lowercase strings not sent on the wire sort.Sort(CaseInsensitive(g.Entries)) return nil } diff --git a/nameserver/entry_test.go b/nameserver/entry_test.go index e0640c1f..1e350e64 100644 --- a/nameserver/entry_test.go +++ b/nameserver/entry_test.go @@ -9,12 +9,17 @@ import ( "github.com/weaveworks/weave/router" ) +func l(es Entries) Entries { + es.addLowercase() + return es +} + func makeEntries(values string) Entries { entries := make(Entries, len(values)) for i, c := range values { entries[i] = Entry{Hostname: string(c)} } - return entries + return l(entries) } func TestAdd(t *testing.T) { @@ -24,21 +29,21 @@ func TestAdd(t *testing.T) { entries := Entries{} entries.add("A", "", router.UnknownPeerName, address.Address(0)) - expected := Entries{ + expected := l(Entries{ Entry{Hostname: "A", Origin: router.UnknownPeerName, Addr: address.Address(0)}, - } + }) require.Equal(t, entries, expected) entries.tombstone(router.UnknownPeerName, func(e *Entry) bool { return e.Hostname == "A" }) - expected = Entries{ + expected = l(Entries{ Entry{Hostname: "A", Origin: router.UnknownPeerName, Addr: address.Address(0), Version: 1, Tombstone: 1234}, - } + }) require.Equal(t, entries, expected) entries.add("A", "", router.UnknownPeerName, address.Address(0)) - expected = Entries{ + expected = l(Entries{ Entry{Hostname: "A", Origin: router.UnknownPeerName, Addr: address.Address(0), Version: 2}, - } + }) require.Equal(t, entries, expected) } @@ -56,14 +61,14 @@ func TestMerge(t *testing.T) { } 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) + e1 := l(Entries{Entry{Hostname: "A", Version: 0}}) + diff := e1.merge(l(Entries{Entry{Hostname: "A", Version: 1}})) + require.Equal(t, l(Entries{Entry{Hostname: "A", Version: 1}}), diff) + require.Equal(t, l(Entries{Entry{Hostname: "A", Version: 1}}), e1) - diff = e1.merge(Entries{Entry{Hostname: "A", Version: 0}}) + diff = e1.merge(l(Entries{Entry{Hostname: "A", Version: 0}})) require.Equal(t, Entries{}, diff) - require.Equal(t, Entries{Entry{Hostname: "A", Version: 1}}, e1) + require.Equal(t, l(Entries{Entry{Hostname: "A", Version: 1}}), e1) } func TestTombstone(t *testing.T) { @@ -76,10 +81,10 @@ func TestTombstone(t *testing.T) { es.tombstone(router.UnknownPeerName, func(e *Entry) bool { return e.Hostname == "B" }) - expected := Entries{ + expected := l(Entries{ Entry{Hostname: "A"}, Entry{Hostname: "B", Version: 1, Tombstone: 1234}, - } + }) require.Equal(t, expected, es) } @@ -93,18 +98,18 @@ func TestDelete(t *testing.T) { } func TestLookup(t *testing.T) { - es := Entries{ + es := l(Entries{ Entry{Hostname: "A"}, Entry{Hostname: "B", ContainerID: "bar"}, Entry{Hostname: "B", ContainerID: "foo"}, Entry{Hostname: "C"}, - } + }) have := es.lookup("B") - want := Entries{ + want := l(Entries{ Entry{Hostname: "B", ContainerID: "bar"}, Entry{Hostname: "B", ContainerID: "foo"}, - } + }) require.Equal(t, have, want) } diff --git a/nameserver/nameserver_test.go b/nameserver/nameserver_test.go index 5b518ad8..77b39b84 100644 --- a/nameserver/nameserver_test.go +++ b/nameserver/nameserver_test.go @@ -260,14 +260,14 @@ func TestTombstoneDeletion(t *testing.T) { err = nameserver.Delete("hostname", "containerid", "", address.Address(0)) require.Nil(t, err) require.Equal(t, []address.Address{}, nameserver.Lookup("hostname")) - require.Equal(t, Entries{{ + require.Equal(t, l(Entries{Entry{ ContainerID: "containerid", Origin: peername, Addr: address.Address(0), Hostname: "hostname", Version: 1, Tombstone: 1234, - }}, nameserver.entries) + }}), nameserver.entries) now = func() int64 { return 1234 + int64(tombstoneTimeout/time.Second) + 1 } nameserver.deleteTombstones() From b80c1343c3a8ff60bf6ca61d9061959115bef697 Mon Sep 17 00:00:00 2001 From: Bryan Boreham Date: Mon, 2 Nov 2015 16:12:11 +0000 Subject: [PATCH 8/8] Take steps to avoid the test timing out --- nameserver/nameserver_test.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/nameserver/nameserver_test.go b/nameserver/nameserver_test.go index 77b39b84..f396303f 100644 --- a/nameserver/nameserver_test.go +++ b/nameserver/nameserver_test.go @@ -72,7 +72,7 @@ func (m mapping) Addrs() []address.Address { } func TestNameservers(t *testing.T) { - wt.RunWithTimeout(t, time.Minute, func() { + wt.RunWithTimeout(t, 2*time.Minute, func() { testNameservers(t) }) } @@ -81,12 +81,12 @@ func testNameservers(t *testing.T) { //common.SetLogLevel("debug") lookupTimeout := 10 // ms - nameservers, grouter := makeNetwork(50) + nameservers, grouter := makeNetwork(30) defer stopNetwork(nameservers, grouter) // This subset will sometimes lose touch with some of the others - badNameservers := nameservers[45:] + badNameservers := nameservers[25:] // This subset will remain well-connected, and we will deal mainly with them - nameservers = nameservers[:45] + nameservers = nameservers[:25] nameserversByName := map[router.PeerName]*Nameserver{} for _, n := range nameservers { nameserversByName[n.ourName] = n @@ -195,7 +195,7 @@ func testNameservers(t *testing.T) { require.Equal(t, mapping.hostname, hostname) } - for i := 0; i < 1000; i++ { + for i := 0; i < 800; i++ { r := rand.Float32() switch { case r < 0.1: