From 27632b118f90748a74c3a88dcf47c9709a05a0c2 Mon Sep 17 00:00:00 2001 From: Matthias Radestock Date: Sun, 17 Apr 2016 09:35:21 +0100 Subject: [PATCH 1/4] prevent excessive ring fragmentation due to repeated space requests Merging a ring now indicates whether the ring has been updated. This prevents excessive allocation retries, which in turn can result in repeated space requests - i.e. if an allocation cannot proceed due to lack of space - which in turn can result in excessive ring fragmentation since many of the requests will result in the creation of a new ring entries, due to the need to split ranges. Fixes #2181. --- ipam/allocator.go | 6 ++-- ipam/allocator_test.go | 15 +++++++++ ipam/ring/ring.go | 29 ++++++++++-------- ipam/ring/ring_test.go | 69 ++++++++++++++++++++++-------------------- 4 files changed, 72 insertions(+), 47 deletions(-) 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() } From 4a01d23025f53e4a178246df99d5735d7272c248 Mon Sep 17 00:00:00 2001 From: Matthias Radestock Date: Sun, 17 Apr 2016 10:18:24 +0100 Subject: [PATCH 2/4] refactor: extract entry merging from ring merging --- ipam/ring/ring.go | 137 +++++++++++++++++++++++++--------------------- 1 file changed, 75 insertions(+), 62 deletions(-) diff --git a/ipam/ring/ring.go b/ipam/ring/ring.go index 1116a6ac..faefa957 100644 --- a/ipam/ring/ring.go +++ b/ipam/ring/ring.go @@ -217,69 +217,10 @@ func (r *Ring) Merge(gossip Ring) (bool, error) { 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 + result, updated, err := r.Entries.merge(gossip.Entries, r.Peer) - var mine, theirs *entry - var previousOwner *mesh.PeerName - // i is index into r.Entries; j is index into gossip.Entries - var i, j int - for i < len(r.Entries) && j < len(gossip.Entries) { - mine, theirs = r.Entries[i], gossip.Entries[j] - switch { - case mine.Token < theirs.Token: - addToResult(*mine) - previousOwner = &mine.Peer - i++ - 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 false, errEntryInMyRange(theirs) - } - addToResult(*theirs) - updated = true - previousOwner = nil - j++ - case mine.Token == theirs.Token: - // merge - switch { - case mine.Version >= theirs.Version: - if mine.Version == theirs.Version && !mine.Equal(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 false, errNewerVersion(mine, theirs) - } - addToResult(*theirs) - updated = true - previousOwner = nil - } - i++ - j++ - } - } - - // At this point, either i is at the end of r or j is at the end - // of gossip, so copy over the remaining entries. - - for ; i < len(r.Entries); i++ { - mine = r.Entries[i] - addToResult(*mine) - } - - for ; j < len(gossip.Entries); j++ { - theirs = gossip.Entries[j] - if previousOwner != nil && *previousOwner == r.Peer && theirs.Peer != r.Peer { - return false, errEntryInMyRange(theirs) - } - addToResult(*theirs) - updated = true - previousOwner = nil + if err != nil { + return false, err } if err := r.checkEntries(result); err != nil { @@ -293,6 +234,78 @@ func (r *Ring) Merge(gossip Ring) (bool, error) { return updated, nil } +// Merge other entries into ours, and complain when that stomps on +// entries belonging to ourPeer. Returns the merged entries and an +// indication whether the merge resulted in any changes, i.e. the +// result differs from the original. +func (es entries) merge(other entries, ourPeer mesh.PeerName) (entries, bool, error) { + var result entries + addToResult := func(e entry) { result = append(result, &e) } + var updated bool + + var mine, theirs *entry + var previousOwner *mesh.PeerName + // i is index into es; j is index into other + var i, j int + for i < len(es) && j < len(other) { + mine, theirs = es[i], other[j] + switch { + case mine.Token < theirs.Token: + addToResult(*mine) + previousOwner = &mine.Peer + i++ + case mine.Token > theirs.Token: + // insert, checking that a range owned by us hasn't been split + if previousOwner != nil && *previousOwner == ourPeer && theirs.Peer != ourPeer { + return result, false, errEntryInMyRange(theirs) + } + addToResult(*theirs) + updated = true + previousOwner = nil + j++ + case mine.Token == theirs.Token: + // merge + switch { + case mine.Version >= theirs.Version: + if mine.Version == theirs.Version && !mine.Equal(theirs) { + return result, false, errInconsistentEntry(mine, theirs) + } + addToResult(*mine) + previousOwner = &mine.Peer + case mine.Version < theirs.Version: + if mine.Peer == ourPeer { // We shouldn't receive updates to our own tokens + return result, false, errNewerVersion(mine, theirs) + } + addToResult(*theirs) + updated = true + previousOwner = nil + } + i++ + j++ + } + } + + // At this point, either i is at the end of es or j is at the end + // of other, so copy over the remaining entries. + + for ; i < len(es); i++ { + mine = es[i] + addToResult(*mine) + } + + for ; j < len(other); j++ { + theirs = other[j] + if previousOwner != nil && *previousOwner == ourPeer && theirs.Peer != ourPeer { + return result, false, errEntryInMyRange(theirs) + } + addToResult(*theirs) + updated = true + previousOwner = nil + } + + return result, updated, nil +} + // Empty returns true if the ring has no entries func (r *Ring) Empty() bool { return len(r.Entries) == 0 From a1823af745fe7ca49dfbb9c6430c9d03b372c537 Mon Sep 17 00:00:00 2001 From: Matthias Radestock Date: Sun, 17 Apr 2016 10:21:00 +0100 Subject: [PATCH 3/4] refactor: make error returns less onerous --- ipam/ring/ring.go | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/ipam/ring/ring.go b/ipam/ring/ring.go index faefa957..857dd033 100644 --- a/ipam/ring/ring.go +++ b/ipam/ring/ring.go @@ -238,10 +238,8 @@ func (r *Ring) Merge(gossip Ring) (bool, error) { // entries belonging to ourPeer. Returns the merged entries and an // indication whether the merge resulted in any changes, i.e. the // result differs from the original. -func (es entries) merge(other entries, ourPeer mesh.PeerName) (entries, bool, error) { - var result entries +func (es entries) merge(other entries, ourPeer mesh.PeerName) (result entries, updated bool, err error) { addToResult := func(e entry) { result = append(result, &e) } - var updated bool var mine, theirs *entry var previousOwner *mesh.PeerName @@ -257,7 +255,8 @@ func (es entries) merge(other entries, ourPeer mesh.PeerName) (entries, bool, er case mine.Token > theirs.Token: // insert, checking that a range owned by us hasn't been split if previousOwner != nil && *previousOwner == ourPeer && theirs.Peer != ourPeer { - return result, false, errEntryInMyRange(theirs) + err = errEntryInMyRange(theirs) + return } addToResult(*theirs) updated = true @@ -268,13 +267,15 @@ func (es entries) merge(other entries, ourPeer mesh.PeerName) (entries, bool, er switch { case mine.Version >= theirs.Version: if mine.Version == theirs.Version && !mine.Equal(theirs) { - return result, false, errInconsistentEntry(mine, theirs) + err = errInconsistentEntry(mine, theirs) + return } addToResult(*mine) previousOwner = &mine.Peer case mine.Version < theirs.Version: if mine.Peer == ourPeer { // We shouldn't receive updates to our own tokens - return result, false, errNewerVersion(mine, theirs) + err = errNewerVersion(mine, theirs) + return } addToResult(*theirs) updated = true @@ -296,14 +297,15 @@ func (es entries) merge(other entries, ourPeer mesh.PeerName) (entries, bool, er for ; j < len(other); j++ { theirs = other[j] if previousOwner != nil && *previousOwner == ourPeer && theirs.Peer != ourPeer { - return result, false, errEntryInMyRange(theirs) + err = errEntryInMyRange(theirs) + return } addToResult(*theirs) updated = true previousOwner = nil } - return result, updated, nil + return } // Empty returns true if the ring has no entries From c1d98ba59cd3d237fb7ca83a62d9820b71cf76c4 Mon Sep 17 00:00:00 2001 From: Matthias Radestock Date: Sun, 17 Apr 2016 10:28:21 +0100 Subject: [PATCH 4/4] refactor: extract a helper --- ipam/ring/ring.go | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/ipam/ring/ring.go b/ipam/ring/ring.go index 857dd033..aadd30f0 100644 --- a/ipam/ring/ring.go +++ b/ipam/ring/ring.go @@ -239,10 +239,15 @@ func (r *Ring) Merge(gossip Ring) (bool, error) { // indication whether the merge resulted in any changes, i.e. the // result differs from the original. func (es entries) merge(other entries, ourPeer mesh.PeerName) (result entries, updated bool, err error) { - addToResult := func(e entry) { result = append(result, &e) } - var mine, theirs *entry var previousOwner *mesh.PeerName + addToResult := func(e entry) { result = append(result, &e) } + addTheirs := func(e entry) { + addToResult(e) + updated = true + previousOwner = nil + } + // i is index into es; j is index into other var i, j int for i < len(es) && j < len(other) { @@ -258,9 +263,7 @@ func (es entries) merge(other entries, ourPeer mesh.PeerName) (result entries, u err = errEntryInMyRange(theirs) return } - addToResult(*theirs) - updated = true - previousOwner = nil + addTheirs(*theirs) j++ case mine.Token == theirs.Token: // merge @@ -277,9 +280,7 @@ func (es entries) merge(other entries, ourPeer mesh.PeerName) (result entries, u err = errNewerVersion(mine, theirs) return } - addToResult(*theirs) - updated = true - previousOwner = nil + addTheirs(*theirs) } i++ j++ @@ -300,9 +301,7 @@ func (es entries) merge(other entries, ourPeer mesh.PeerName) (result entries, u err = errEntryInMyRange(theirs) return } - addToResult(*theirs) - updated = true - previousOwner = nil + addTheirs(*theirs) } return