From c48eefa82d67145c0042959dfe4adfefb26cee58 Mon Sep 17 00:00:00 2001 From: Matthias Radestock Date: Fri, 18 Mar 2016 16:47:47 +0000 Subject: [PATCH 1/3] refactor(ish): extract allocator logging commonality and add warnf --- ipam/allocator.go | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/ipam/allocator.go b/ipam/allocator.go index 196bccae..79229c03 100644 --- a/ipam/allocator.go +++ b/ipam/allocator.go @@ -934,14 +934,20 @@ func (alloc *Allocator) syncOwned(ids map[string]struct{}) { // Logging func (alloc *Allocator) fatalf(fmt string, args ...interface{}) { - common.Log.Fatalf("[allocator %s] "+fmt, append([]interface{}{alloc.ourName}, args...)...) + alloc.logf(common.Log.Fatalf, fmt, args...) +} +func (alloc *Allocator) warnf(fmt string, args ...interface{}) { + alloc.logf(common.Log.Warnf, fmt, args...) } func (alloc *Allocator) infof(fmt string, args ...interface{}) { - common.Log.Infof("[allocator %s] "+fmt, append([]interface{}{alloc.ourName}, args...)...) + alloc.logf(common.Log.Infof, fmt, args...) +} +func (alloc *Allocator) debugf(fmt string, args ...interface{}) { + alloc.logf(common.Log.Debugf, fmt, args...) +} +func (alloc *Allocator) logf(f func(string, ...interface{}), fmt string, args ...interface{}) { + f("[allocator %s] "+fmt, append([]interface{}{alloc.ourName}, args...)...) } func (alloc *Allocator) debugln(args ...interface{}) { common.Log.Debugln(append([]interface{}{fmt.Sprintf("[allocator %s]:", alloc.ourName)}, args...)...) } -func (alloc *Allocator) debugf(fmt string, args ...interface{}) { - common.Log.Debugf("[allocator %s] "+fmt, append([]interface{}{alloc.ourName}, args...)...) -} From 62d823850c10decd6b5cbc90762dc61897fba8ce Mon Sep 17 00:00:00 2001 From: Matthias Radestock Date: Sat, 19 Mar 2016 14:40:26 +0000 Subject: [PATCH 2/3] make `rmpeer` report on size of transferred space, and not fail Closes #2058. --- ipam/allocator.go | 31 +++++++++++++++++++---------- ipam/allocator_test.go | 9 +++++++-- ipam/http.go | 8 ++------ ipam/ring/ring.go | 10 ++-------- site/ipam/stop-remove-peers-ipam.md | 3 ++- 5 files changed, 34 insertions(+), 27 deletions(-) diff --git a/ipam/allocator.go b/ipam/allocator.go index 79229c03..ae990444 100644 --- a/ipam/allocator.go +++ b/ipam/allocator.go @@ -376,29 +376,40 @@ func (alloc *Allocator) Shutdown() { <-doneChan } -// AdminTakeoverRanges (Sync) - take over the ranges owned by a given peer. +// AdminTakeoverRanges (Sync) - take over the ranges owned by a given +// peer, and return how much space was transferred in the process. // Only done on adminstrator command. -func (alloc *Allocator) AdminTakeoverRanges(peerNameOrNickname string) error { - resultChan := make(chan error) +func (alloc *Allocator) AdminTakeoverRanges(peerNameOrNickname string) address.Count { + resultChan := make(chan address.Count) alloc.actionChan <- func() { peername, err := alloc.lookupPeername(peerNameOrNickname) if err != nil { - resultChan <- fmt.Errorf("Cannot find peer '%s'", peerNameOrNickname) + alloc.warnf("attempt to take over range from unknown peer '%s'", peerNameOrNickname) + resultChan <- address.Count(0) return } alloc.debugln("AdminTakeoverRanges:", peername) if peername == alloc.ourName { - resultChan <- fmt.Errorf("Cannot take over ranges from yourself!") + alloc.warnf("attempt to take over range from ourself") + resultChan <- address.Count(0) return } - newRanges, err := alloc.ring.Transfer(peername, alloc.ourName) - if err == nil && len(newRanges) > 0 { - alloc.space.AddRanges(newRanges) - alloc.gossip.GossipBroadcast(alloc.Gossip()) + newRanges := alloc.ring.Transfer(peername, alloc.ourName) + + if len(newRanges) == 0 { + resultChan <- address.Count(0) + return } - resultChan <- err + + before := alloc.space.NumFreeAddressesInRange(alloc.universe) + alloc.space.AddRanges(newRanges) + after := alloc.space.NumFreeAddressesInRange(alloc.universe) + + alloc.gossip.GossipBroadcast(alloc.Gossip()) + + resultChan <- after - before } return <-resultChan } diff --git a/ipam/allocator_test.go b/ipam/allocator_test.go index de5f6c62..dd775a6a 100644 --- a/ipam/allocator_test.go +++ b/ipam/allocator_test.go @@ -336,13 +336,18 @@ func TestTransfer(t *testing.T) { router.Flush() alloc2.gossip.GossipBroadcast(alloc3.Gossip()) router.Flush() + + free2 := alloc2.NumFreeAddresses(subnet.Range()) + free3 := alloc3.NumFreeAddresses(subnet.Range()) + router.RemovePeer(alloc2.ourName) router.RemovePeer(alloc3.ourName) alloc2.Stop() alloc3.Stop() router.Flush() - require.NoError(t, alloc1.AdminTakeoverRanges(alloc2.ourName.String())) - require.NoError(t, alloc1.AdminTakeoverRanges(alloc3.ourName.String())) + + require.Equal(t, free2+1, alloc1.AdminTakeoverRanges(alloc2.ourName.String())) + require.Equal(t, free3+1, alloc1.AdminTakeoverRanges(alloc3.ourName.String())) router.Flush() require.Equal(t, address.Count(1024), alloc1.NumFreeAddresses(subnet.Range())) diff --git a/ipam/http.go b/ipam/http.go index 1128f653..7330b96c 100644 --- a/ipam/http.go +++ b/ipam/http.go @@ -151,11 +151,7 @@ func (alloc *Allocator) HandleHTTP(router *mux.Router, defaultSubnet address.CID router.Methods("DELETE").Path("/peer/{id}").HandlerFunc(func(w http.ResponseWriter, r *http.Request) { ident := mux.Vars(r)["id"] - if err := alloc.AdminTakeoverRanges(ident); err != nil { - badRequest(w, err) - return - } - - w.WriteHeader(204) + transferred := alloc.AdminTakeoverRanges(ident) + fmt.Fprintf(w, "%d IPs taken over from %s\n", transferred, ident) }) } diff --git a/ipam/ring/ring.go b/ipam/ring/ring.go index 58e320ba..d7f07906 100644 --- a/ipam/ring/ring.go +++ b/ipam/ring/ring.go @@ -498,28 +498,22 @@ func (r *Ring) PickPeerForTransfer(isValid func(mesh.PeerName) bool) mesh.PeerNa // Transfer will mark all entries associated with 'from' peer as owned by 'to' peer // and return ranges indicating the new space we picked up -func (r *Ring) Transfer(from, to mesh.PeerName) ([]address.Range, error) { +func (r *Ring) Transfer(from, to mesh.PeerName) []address.Range { r.assertInvariants() defer r.assertInvariants() defer r.updateExportedVariables() var newRanges []address.Range - found := false for i, entry := range r.Entries { if entry.Peer == from { - found = true entry.Peer = to entry.Version++ newRanges = append(newRanges, address.Range{Start: entry.Token, End: r.Entries.entry(i + 1).Token}) } } - if !found { - return nil, ErrNotFound - } - - return r.splitRangesOverZero(newRanges), nil + return r.splitRangesOverZero(newRanges) } // Contains returns true if addr is in this ring diff --git a/site/ipam/stop-remove-peers-ipam.md b/site/ipam/stop-remove-peers-ipam.md index 7c07645f..433b8148 100644 --- a/site/ipam/stop-remove-peers-ipam.md +++ b/site/ipam/stop-remove-peers-ipam.md @@ -24,6 +24,7 @@ Assume you had started the three peers in the example earlier, and then host3 caught fire, you can go to one of the other hosts and run: host1$ weave rmpeer host3 + 524288 IPs taken over from host3 Weave Net takes all the IP address ranges owned by host3 and transfers them to be owned by host1. The name "host3" is resolved via the @@ -36,4 +37,4 @@ status`. * [Address Allocation with IP Address Management (IPAM)](/site/ipam/overview-init-ipam.md) * [Automatic Allocation Across Multiple Subnets](/site/ipam/allocation-multi-ipam.md) * [Isolating Applications on a Weave Network](/site/using-weave/isolating-applications.md) - \ No newline at end of file + From 9f10b21c9c36c39bf57a7b3bba840424ce37ec1a Mon Sep 17 00:00:00 2001 From: Matthias Radestock Date: Sat, 19 Mar 2016 14:41:36 +0000 Subject: [PATCH 3/3] use Space.NumFreeAddresses for calculating transferred ip count ...rather than Space.NumFreeAddressesInRange; we don't care about the universe. Alternatively we could make Space.AddRanges return the count. But AddRanges doesn't know whether the ranges overlap with existing free space - that is only guaranteed by the context - so would have to do much the same before/after calculation. Another alternative would be to add the range sizes in AdminTakeoverRanges. But that is more code and less clear. --- ipam/allocator.go | 4 ++-- ipam/space/space.go | 8 ++++++++ ipam/space/space_test.go | 10 ---------- 3 files changed, 10 insertions(+), 12 deletions(-) diff --git a/ipam/allocator.go b/ipam/allocator.go index ae990444..6940bd5a 100644 --- a/ipam/allocator.go +++ b/ipam/allocator.go @@ -403,9 +403,9 @@ func (alloc *Allocator) AdminTakeoverRanges(peerNameOrNickname string) address.C return } - before := alloc.space.NumFreeAddressesInRange(alloc.universe) + before := alloc.space.NumFreeAddresses() alloc.space.AddRanges(newRanges) - after := alloc.space.NumFreeAddressesInRange(alloc.universe) + after := alloc.space.NumFreeAddresses() alloc.gossip.GossipBroadcast(alloc.Gossip()) diff --git a/ipam/space/space.go b/ipam/space/space.go index de7bc36b..80d99c97 100644 --- a/ipam/space/space.go +++ b/ipam/space/space.go @@ -88,6 +88,14 @@ func (s *Space) Claim(addr address.Address) error { return nil } +func (s *Space) NumFreeAddresses() address.Count { + res := address.Count(0) + for i := 0; i < len(s.free); i += 2 { + res += address.Length(s.free[i+1], s.free[i]) + } + return res +} + func (s *Space) NumFreeAddressesInRange(r address.Range) address.Count { res := address.Count(0) s.walkFree(r, func(chunk address.Range) bool { diff --git a/ipam/space/space_test.go b/ipam/space/space_test.go index 3b3290ac..295cfd57 100644 --- a/ipam/space/space_test.go +++ b/ipam/space/space_test.go @@ -19,16 +19,6 @@ func ip(s string) address.Address { return addr } -// Helper function to avoid 'NumFreeAddressesInRange(start, end)' -// dozens of times in tests -func (s *Space) NumFreeAddresses() address.Count { - res := address.Count(0) - for i := 0; i < len(s.free); i += 2 { - res += address.Length(s.free[i+1], s.free[i]) - } - return res -} - func TestLowlevel(t *testing.T) { a := []address.Address{} a = add(a, 100, 200)