diff --git a/ipam/allocator.go b/ipam/allocator.go index b7aaae1c..f2ca0420 100644 --- a/ipam/allocator.go +++ b/ipam/allocator.go @@ -709,7 +709,6 @@ func (alloc *Allocator) update(sender mesh.PeerName, msg []byte) error { reader := bytes.NewReader(msg) decoder := gob.NewDecoder(reader) var data gossipState - var err error if err := decoder.Decode(&data); err != nil { return err @@ -724,9 +723,10 @@ func (alloc *Allocator) update(sender mesh.PeerName, msg []byte) error { // If someone sent us a ring, merge it into ours. Note this will move us // out of the awaiting-consensus state if we didn't have a ring already. case data.Ring != nil: - switch err = alloc.ring.Merge(*data.Ring); err { + updated, err := alloc.ring.Merge(*data.Ring) + switch err { case nil: - if !alloc.ring.Empty() { + if updated { alloc.pruneNicknames() alloc.ringUpdated() } diff --git a/ipam/allocator_test.go b/ipam/allocator_test.go index c56162e5..eb6dbf77 100644 --- a/ipam/allocator_test.go +++ b/ipam/allocator_test.go @@ -325,6 +325,21 @@ func TestGossipShutdown(t *testing.T) { CheckAllExpectedMessagesSent(alloc) } +func TestNoFrag(t *testing.T) { + const cidr = "10.0.1.7/22" + resultChan := make(chan int) + for i := 0; i < 100; i++ { + allocs, router, subnet := makeNetworkOfAllocators(3, cidr) + allocs[1].Allocate("foo", subnet, returnFalse) + allocs[2].Allocate("bar", subnet, returnFalse) + allocs[2].actionChan <- func() { + resultChan <- len(allocs[2].ring.Entries) + } + require.True(t, <-resultChan < 5, "excessive ring fragmentation") + stopNetworkOfAllocators(allocs, router) + } +} + func TestTransfer(t *testing.T) { const cidr = "10.0.1.7/22" allocs, router, subnet := makeNetworkOfAllocators(3, cidr) diff --git a/ipam/ring/ring.go b/ipam/ring/ring.go index 998c913c..1116a6ac 100644 --- a/ipam/ring/ring.go +++ b/ipam/ring/ring.go @@ -190,35 +190,37 @@ func (r *Ring) GrantRangeToHost(start, end address.Address, peer mesh.PeerName) r.Entries.insert(entry{Token: end, Peer: r.Peer, Free: endFree}) } -// Merge the given ring into this ring and return any new ranges added -func (r *Ring) Merge(gossip Ring) error { +// Merge the given ring into this ring and indicate whether this ring +// got updated as a result. +func (r *Ring) Merge(gossip Ring) (bool, error) { r.assertInvariants() defer r.updateExportedVariables() // Don't panic when checking the gossiped in ring. // In this case just return any error found. if err := gossip.checkInvariants(); err != nil { - return err + return false, err } if len(gossip.Seeds) > 0 && len(r.Seeds) > 0 { if len(gossip.Seeds) != len(r.Seeds) { - return ErrDifferentSeeds + return false, ErrDifferentSeeds } for i, seed := range gossip.Seeds { if seed != r.Seeds[i] { - return ErrDifferentSeeds + return false, ErrDifferentSeeds } } } if r.Start != gossip.Start || r.End != gossip.End { - return ErrDifferentRange + return false, ErrDifferentRange } // Now merge their ring with yours, in a temporary ring. var result entries addToResult := func(e entry) { result = append(result, &e) } + var updated bool var mine, theirs *entry var previousOwner *mesh.PeerName @@ -234,9 +236,10 @@ func (r *Ring) Merge(gossip Ring) error { case mine.Token > theirs.Token: // insert, checking that a range owned by us hasn't been split if previousOwner != nil && *previousOwner == r.Peer && theirs.Peer != r.Peer { - return errEntryInMyRange(theirs) + return false, errEntryInMyRange(theirs) } addToResult(*theirs) + updated = true previousOwner = nil j++ case mine.Token == theirs.Token: @@ -244,15 +247,16 @@ func (r *Ring) Merge(gossip Ring) error { switch { case mine.Version >= theirs.Version: if mine.Version == theirs.Version && !mine.Equal(theirs) { - return errInconsistentEntry(mine, theirs) + return false, errInconsistentEntry(mine, theirs) } addToResult(*mine) previousOwner = &mine.Peer case mine.Version < theirs.Version: if mine.Peer == r.Peer { // We shouldn't receive updates to our own tokens - return errNewerVersion(mine, theirs) + return false, errNewerVersion(mine, theirs) } addToResult(*theirs) + updated = true previousOwner = nil } i++ @@ -271,21 +275,22 @@ func (r *Ring) Merge(gossip Ring) error { for ; j < len(gossip.Entries); j++ { theirs = gossip.Entries[j] if previousOwner != nil && *previousOwner == r.Peer && theirs.Peer != r.Peer { - return errEntryInMyRange(theirs) + return false, errEntryInMyRange(theirs) } addToResult(*theirs) + updated = true previousOwner = nil } if err := r.checkEntries(result); err != nil { - return fmt.Errorf("Merge of incoming data causes: %s", err) + return false, fmt.Errorf("Merge of incoming data causes: %s", err) } if len(r.Seeds) == 0 { r.Seeds = gossip.Seeds } r.Entries = result - return nil + return updated, nil } // Empty returns true if the ring has no entries diff --git a/ipam/ring/ring_test.go b/ipam/ring/ring_test.go index bdf3bc73..2e74f071 100644 --- a/ipam/ring/ring_test.go +++ b/ipam/ring/ring_test.go @@ -30,6 +30,11 @@ func ParseIP(s string) address.Address { return addr } +func merge(r1, r2 *Ring) error { + _, err := r1.Merge(*r2) + return err +} + func TestInvariants(t *testing.T) { ring := New(start, end, peer1name) @@ -158,7 +163,7 @@ func TestGrantSplit(t *testing.T) { // Claim everything for peer1 ring1.Entries = []*entry{{Token: start, Peer: peer1name, Free: 255}} - ring2.Merge(*ring1) + merge(ring2, ring1) require.Equal(t, ring2.Entries, ring1.Entries) // Now grant a split range to peer2 @@ -188,7 +193,7 @@ func TestMergeSimple(t *testing.T) { // Claim everything for peer1 ring1.ClaimItAll() ring1.GrantRangeToHost(middle, end, peer2name) - require.NoError(t, ring2.Merge(*ring1)) + require.NoError(t, merge(ring2, ring1)) require.Equal(t, entries{{Token: start, Peer: peer1name, Free: 128, Version: 1}, {Token: middle, Peer: peer2name, Free: 127}}, ring1.Entries) @@ -200,8 +205,8 @@ func TestMergeSimple(t *testing.T) { ring1.GrantRangeToHost(start, middle, peer2name) ring2.GrantRangeToHost(middle, end, peer1name) - require.NoError(t, ring2.Merge(*ring1)) - require.NoError(t, ring1.Merge(*ring2)) + require.NoError(t, merge(ring2, ring1)) + require.NoError(t, merge(ring1, ring2)) require.Equal(t, entries{{Token: start, Peer: peer2name, Free: 128, Version: 2}, {Token: middle, Peer: peer1name, Free: 127, Version: 1}}, ring1.Entries) @@ -214,29 +219,29 @@ func TestMergeErrors(t *testing.T) { ring1 := New(start, end, peer1name) ring2 := New(start, end, peer2name) ring2.Entries = []*entry{{Token: middle, Peer: peer2name}, {Token: start, Peer: peer2name}} - require.True(t, ring1.Merge(*ring2) == ErrNotSorted, "Expected ErrNotSorted") + require.True(t, merge(ring1, ring2) == ErrNotSorted, "Expected ErrNotSorted") // Should Merge two rings for different ranges ring2 = New(start, middle, peer2name) ring2.Entries = []*entry{} - require.True(t, ring1.Merge(*ring2) == ErrDifferentRange, "Expected ErrDifferentRange") + require.True(t, merge(ring1, ring2) == ErrDifferentRange, "Expected ErrDifferentRange") // Cannot Merge newer version of entry I own ring2 = New(start, end, peer2name) ring1.Entries = []*entry{{Token: start, Peer: peer1name}} ring2.Entries = []*entry{{Token: start, Peer: peer1name, Version: 1}} - fmt.Println(ring1.Merge(*ring2)) - require.Error(t, ring1.Merge(*ring2), "Expected error") + fmt.Println(merge(ring1, ring2)) + require.Error(t, merge(ring1, ring2), "Expected error") // Cannot Merge two entries with same version but different hosts ring1.Entries = []*entry{{Token: start, Peer: peer1name}} ring2.Entries = []*entry{{Token: start, Peer: peer2name}} - require.Error(t, ring1.Merge(*ring2), "Expected error") + require.Error(t, merge(ring1, ring2), "Expected error") // Cannot Merge an entry into a range I own ring1.Entries = []*entry{{Token: start, Peer: peer1name}} ring2.Entries = []*entry{{Token: middle, Peer: peer2name}} - require.Error(t, ring1.Merge(*ring2), "Expected error") + require.Error(t, merge(ring1, ring2), "Expected error") } func TestMergeMore(t *testing.T) { @@ -256,7 +261,7 @@ func TestMergeMore(t *testing.T) { assertRing(ring2, []*entry{}) // Check the Merge sends it to the other ring - require.NoError(t, ring2.Merge(*ring1)) + require.NoError(t, merge(ring2, ring1)) assertRing(ring1, []*entry{{Token: start, Peer: peer1name, Free: 255}}) assertRing(ring2, []*entry{{Token: start, Peer: peer1name, Free: 255}}) @@ -265,7 +270,7 @@ func TestMergeMore(t *testing.T) { assertRing(ring1, []*entry{{Token: start, Peer: peer2name, Free: 255, Version: 1}}) assertRing(ring2, []*entry{{Token: start, Peer: peer1name, Free: 255}}) - require.NoError(t, ring2.Merge(*ring1)) + require.NoError(t, merge(ring2, ring1)) assertRing(ring1, []*entry{{Token: start, Peer: peer2name, Free: 255, Version: 1}}) assertRing(ring2, []*entry{{Token: start, Peer: peer2name, Free: 255, Version: 1}}) @@ -276,14 +281,14 @@ func TestMergeMore(t *testing.T) { assertRing(ring1, []*entry{{Token: start, Peer: peer2name, Free: 255, Version: 1}}) // And Merge back - require.NoError(t, ring1.Merge(*ring2)) + require.NoError(t, merge(ring1, ring2)) assertRing(ring1, []*entry{{Token: start, Peer: peer2name, Free: 128, Version: 2}, {Token: middle, Peer: peer1name, Free: 127}}) assertRing(ring2, []*entry{{Token: start, Peer: peer2name, Free: 128, Version: 2}, {Token: middle, Peer: peer1name, Free: 127}}) // This should be a no-op - require.NoError(t, ring2.Merge(*ring1)) + require.NoError(t, merge(ring2, ring1)) assertRing(ring1, []*entry{{Token: start, Peer: peer2name, Free: 128, Version: 2}, {Token: middle, Peer: peer1name, Free: 127}}) assertRing(ring2, []*entry{{Token: start, Peer: peer2name, Free: 128, Version: 2}, @@ -296,7 +301,7 @@ func TestMergeSplit(t *testing.T) { // Claim everything for peer2 ring1.Entries = []*entry{{Token: start, Peer: peer2name, Free: 255}} - require.NoError(t, ring2.Merge(*ring1)) + require.NoError(t, merge(ring2, ring1)) require.Equal(t, ring2.Entries, ring1.Entries) // Now grant a split range to peer1 @@ -305,7 +310,7 @@ func TestMergeSplit(t *testing.T) { {Token: dot10, Peer: peer1name, Free: 235}, {Token: dot245, Peer: peer2name, Free: 10}}, ring2.Entries) - require.NoError(t, ring1.Merge(*ring2)) + require.NoError(t, merge(ring1, ring2)) require.Equal(t, entries{{Token: start, Peer: peer2name, Free: 10, Version: 1}, {Token: dot10, Peer: peer1name, Free: 235}, {Token: dot245, Peer: peer2name, Free: 10}}, ring1.Entries) @@ -319,7 +324,7 @@ func TestMergeSplit2(t *testing.T) { // Claim everything for peer2 ring1.Entries = []*entry{{Token: start, Peer: peer2name, Free: 250}, {Token: dot250, Peer: peer2name, Free: 5}} - require.NoError(t, ring2.Merge(*ring1)) + require.NoError(t, merge(ring2, ring1)) require.Equal(t, ring2.Entries, ring1.Entries) // Now grant a split range to peer1 @@ -328,7 +333,7 @@ func TestMergeSplit2(t *testing.T) { {Token: dot10, Peer: peer1name, Free: 235}, {Token: dot245, Peer: peer2name, Free: 5}, {Token: dot250, Peer: peer2name, Free: 5}}, ring2.Entries) - require.NoError(t, ring1.Merge(*ring2)) + require.NoError(t, merge(ring1, ring2)) require.Equal(t, entries{{Token: start, Peer: peer2name, Free: 10, Version: 1}, {Token: dot10, Peer: peer1name, Free: 235}, {Token: dot245, Peer: peer2name, Free: 5}, {Token: dot250, Peer: peer2name, Free: 5}}, ring1.Entries) @@ -354,7 +359,7 @@ func TestGossip(t *testing.T) { assertRing(ring2, []*entry{}) // Check the Merge sends it to the other ring - require.NoError(t, ring2.Merge(*ring1)) + require.NoError(t, merge(ring2, ring1)) assertRing(ring1, []*entry{{Token: start, Peer: peer1name, Free: 255}}) assertRing(ring2, []*entry{{Token: start, Peer: peer1name, Free: 255}}) } @@ -407,7 +412,7 @@ func TestReportFree(t *testing.T) { ring1.ClaimItAll() ring1.GrantRangeToHost(middle, end, peer2name) - require.NoError(t, ring2.Merge(*ring1)) + require.NoError(t, merge(ring2, ring1)) freespace := make(map[address.Address]address.Count) for _, r := range ring2.OwnedRanges() { @@ -431,7 +436,7 @@ func TestEmptyGossip(t *testing.T) { ring1.ClaimItAll() // This used to panic, and it shouldn't - require.NoError(t, ring1.Merge(*ring2)) + require.NoError(t, merge(ring1, ring2)) } func TestMergeOldMessage(t *testing.T) { @@ -439,10 +444,10 @@ func TestMergeOldMessage(t *testing.T) { ring2 := New(start, end, peer2name) ring1.ClaimItAll() - require.NoError(t, ring2.Merge(*ring1)) + require.NoError(t, merge(ring2, ring1)) ring1.GrantRangeToHost(middle, end, peer1name) - require.NoError(t, ring1.Merge(*ring2)) + require.NoError(t, merge(ring1, ring2)) } func TestSplitRangeAtBeginning(t *testing.T) { @@ -450,10 +455,10 @@ func TestSplitRangeAtBeginning(t *testing.T) { ring2 := New(start, end, peer2name) ring1.ClaimItAll() - require.NoError(t, ring2.Merge(*ring1)) + require.NoError(t, merge(ring2, ring1)) ring1.GrantRangeToHost(start, middle, peer2name) - require.NoError(t, ring2.Merge(*ring1)) + require.NoError(t, merge(ring2, ring1)) } func TestOwnedRange(t *testing.T) { @@ -466,7 +471,7 @@ func TestOwnedRange(t *testing.T) { require.Equal(t, []address.Range{{Start: start, End: middle}}, ring1.OwnedRanges()) ring2 := New(start, end, peer2name) - ring2.Merge(*ring1) + merge(ring2, ring1) require.Equal(t, []address.Range{{Start: middle, End: end}}, ring2.OwnedRanges()) ring2.Entries = []*entry{{Token: middle, Peer: peer2name}} @@ -586,7 +591,7 @@ func TestFuzzRing(t *testing.T) { // Merge them - this might fail, we don't care // We just want to make sure it doesn't panic - ring1.Merge(*ring2) + merge(ring1, ring2) // Check whats left still passes assertions ring1.assertInvariants() @@ -619,7 +624,7 @@ func TestFuzzRing(t *testing.T) { // Merge them - this might fail, we don't care // We just want to make sure it doesn't panic - ring1.Merge(*ring2) + merge(ring1, ring2) // Check whats left still passes assertions ring1.assertInvariants() @@ -680,7 +685,7 @@ func TestFuzzRingHard(t *testing.T) { // We need to be in a ~converged ring to rmpeer for _, ring := range rings { - require.NoError(t, otherRing.Merge(*ring)) + require.NoError(t, merge(otherRing, ring)) } common.Log.Debugf("%s: transferring from peer %s", otherPeername, peername) @@ -694,7 +699,7 @@ func TestFuzzRingHard(t *testing.T) { // And now tell everyone about the transfer - rmpeer is // not partition safe for i, ring := range rings { - require.NoError(t, ring.Merge(*otherRing)) + require.NoError(t, merge(ring, otherRing)) theRanges[i] = ring.OwnedRanges() } } @@ -723,7 +728,7 @@ func TestFuzzRingHard(t *testing.T) { // Now 'gossip' this to a random host (note, note could be same host as above) otherIndex, _, otherRing := randomPeer(-1) common.Log.Debugf("%s: 'Gossiping' to %s", ring.Peer, otherRing.Peer) - require.NoError(t, otherRing.Merge(*ring)) + require.NoError(t, merge(otherRing, ring)) theRanges[indexWithRanges] = ring.OwnedRanges() theRanges[otherIndex] = otherRing.OwnedRanges() @@ -742,7 +747,7 @@ func TestFuzzRingHard(t *testing.T) { ring1 := ringsWithEntries[rand.Intn(len(ringsWithEntries))] ring2index, _, ring2 := randomPeer(-1) common.Log.Debugf("%s: 'Gossiping' to %s", ring1.Peer, ring2.Peer) - require.NoError(t, ring2.Merge(*ring1)) + require.NoError(t, merge(ring2, ring1)) theRanges[ring2index] = ring2.OwnedRanges() }