From bd24dd7dae237e71519196fb591a983ff5a299f9 Mon Sep 17 00:00:00 2001 From: Bryan Boreham Date: Tue, 26 Mar 2019 17:19:22 +0000 Subject: [PATCH 1/7] Make test harness and comments match Kubernetes behaviour on conflict error Kubernetes always returns nil on error; somehow I had the wrong idea here. --- prog/kube-utils/annotations.go | 6 +++--- testing/kubernetes/testing/fixture.go | 4 ---- 2 files changed, 3 insertions(+), 7 deletions(-) diff --git a/prog/kube-utils/annotations.go b/prog/kube-utils/annotations.go index 1b079365..195273a2 100644 --- a/prog/kube-utils/annotations.go +++ b/prog/kube-utils/annotations.go @@ -100,7 +100,7 @@ func (cml *configMapAnnotations) UpdateAnnotation(key, value string) (err error) return errors.New("endpoint not initialized, call Init first") } // speculatively change the state, then replace with whatever comes back - // from Update(), which will be the latest on the server whatever happened + // from Update(), which will be the latest on the server, or nil if error cml.cm.Annotations[cleanKey(key)] = value cml.cm, err = cml.Client.ConfigMaps(cml.Namespace).Update(cml.cm) return err @@ -111,7 +111,7 @@ func (cml *configMapAnnotations) RemoveAnnotation(key string) (err error) { return errors.New("endpoint not initialized, call Init first") } // speculatively change the state, then replace with whatever comes back - // from Update(), which will be the latest on the server whatever happened + // from Update(), which will be the latest on the server, or nil if error delete(cml.cm.Annotations, cleanKey(key)) cml.cm, err = cml.Client.ConfigMaps(cml.Namespace).Update(cml.cm) return err @@ -122,7 +122,7 @@ func (cml *configMapAnnotations) RemoveAnnotationsWithValue(valueToRemove string return errors.New("endpoint not initialized, call Init first") } // speculatively change the state, then replace with whatever comes back - // from Update(), which will be the latest on the server whatever happened + // from Update(), which will be the latest on the server, or nil if error for key, value := range cml.cm.Annotations { if value == valueToRemove { delete(cml.cm.Annotations, key) // don't need to clean this key as it came from the map diff --git a/testing/kubernetes/testing/fixture.go b/testing/kubernetes/testing/fixture.go index 60965d79..3df53523 100644 --- a/testing/kubernetes/testing/fixture.go +++ b/testing/kubernetes/testing/fixture.go @@ -110,10 +110,6 @@ func ObjectReaction(tracker ObjectTracker) testing.ReactionFunc { } err = tracker.Update(gvr, action.GetObject(), ns) if err != nil { - if errors.IsConflict(err) { // return the currently-stored version of the object - obj, _ := tracker.Get(gvr, ns, objMeta.GetName()) - return true, obj, err - } return true, nil, err } obj, err := tracker.Get(gvr, ns, objMeta.GetName()) From 5024270232ef08d3c7459abc0bf2352131428af7 Mon Sep 17 00:00:00 2001 From: Bryan Boreham Date: Tue, 26 Mar 2019 17:20:24 +0000 Subject: [PATCH 2/7] Avoid panic on nil map It can be set to nil after a failed update; check rather than crashing. --- prog/kube-utils/annotations.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/prog/kube-utils/annotations.go b/prog/kube-utils/annotations.go index 195273a2..f83b676a 100644 --- a/prog/kube-utils/annotations.go +++ b/prog/kube-utils/annotations.go @@ -96,7 +96,7 @@ func (cml *configMapAnnotations) GetAnnotation(key string) (string, bool) { } func (cml *configMapAnnotations) UpdateAnnotation(key, value string) (err error) { - if cml.cm == nil { + if cml.cm == nil || cml.cm.Annotations == nil { return errors.New("endpoint not initialized, call Init first") } // speculatively change the state, then replace with whatever comes back @@ -107,7 +107,7 @@ func (cml *configMapAnnotations) UpdateAnnotation(key, value string) (err error) } func (cml *configMapAnnotations) RemoveAnnotation(key string) (err error) { - if cml.cm == nil { + if cml.cm == nil || cml.cm.Annotations == nil { return errors.New("endpoint not initialized, call Init first") } // speculatively change the state, then replace with whatever comes back @@ -118,7 +118,7 @@ func (cml *configMapAnnotations) RemoveAnnotation(key string) (err error) { } func (cml *configMapAnnotations) RemoveAnnotationsWithValue(valueToRemove string) (err error) { - if cml.cm == nil { + if cml.cm == nil || cml.cm.Annotations == nil { return errors.New("endpoint not initialized, call Init first") } // speculatively change the state, then replace with whatever comes back From 15b9b7416a19e0e9a9a4830901b71f55326c9333 Mon Sep 17 00:00:00 2001 From: Bryan Boreham Date: Tue, 26 Mar 2019 17:25:34 +0000 Subject: [PATCH 3/7] Re-read peers ConfigMap from Kubernetes at start of every reclaim A previous reclaim may have left the map set to nil. --- prog/kube-utils/main.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/prog/kube-utils/main.go b/prog/kube-utils/main.go index e0403f76..691d4c88 100644 --- a/prog/kube-utils/main.go +++ b/prog/kube-utils/main.go @@ -140,6 +140,11 @@ func reclaimPeer(weave weaveClient, cml *configMapAnnotations, storedPeerList *p okToRemove := false nonExistentPeer := false + // Re-read status from Kubernetes; speculative updates may leave it un-set + if err := cml.Init(); err != nil { + return false, err + } + // 3. Check if there is an existing annotation with key X existingAnnotation, found := cml.GetAnnotation(KubePeersPrefix + peerName) if found { From eb7ba691ee5e0be0b8f2da261dc13de63193f6a0 Mon Sep 17 00:00:00 2001 From: Bryan Boreham Date: Tue, 2 Apr 2019 09:11:21 +0000 Subject: [PATCH 4/7] Fetch storedPeerList after read from Kubernetes This avoids a race condition where the peer has added itself to the list and the annotation since we read storedPeerList. --- prog/kube-utils/main.go | 8 ++++++-- prog/kube-utils/peerlist_test.go | 2 +- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/prog/kube-utils/main.go b/prog/kube-utils/main.go index 691d4c88..82b93844 100644 --- a/prog/kube-utils/main.go +++ b/prog/kube-utils/main.go @@ -115,7 +115,7 @@ func reclaimRemovedPeers(kube kubernetes.Interface, cml *configMapAnnotations, m common.Log.Warnln("[kube-peers] not removing myself", peer) continue } - changed, err := reclaimPeer(weave, cml, storedPeerList, peer.PeerName, myPeerName) + changed, err := reclaimPeer(weave, cml, peer.PeerName, myPeerName) if err != nil { return err } @@ -135,7 +135,7 @@ func reclaimRemovedPeers(kube kubernetes.Interface, cml *configMapAnnotations, m // actions the reclaim. // Return a bool to show whether we attempted to change anything, // and an error if something went wrong. -func reclaimPeer(weave weaveClient, cml *configMapAnnotations, storedPeerList *peerList, peerName string, myPeerName string) (changed bool, err error) { +func reclaimPeer(weave weaveClient, cml *configMapAnnotations, peerName string, myPeerName string) (changed bool, err error) { common.Log.Debugln("[kube-peers] Preparing to remove disappeared peer", peerName) okToRemove := false nonExistentPeer := false @@ -153,6 +153,10 @@ func reclaimPeer(weave weaveClient, cml *configMapAnnotations, storedPeerList *p if existingAnnotation == myPeerName { okToRemove = true } else { + storedPeerList, err := cml.GetPeerList() + if err != nil { + return false, err + } // handle an edge case where peer claimed to own the action to reclaim but no longer // exists hence lock persists foever if !storedPeerList.contains(existingAnnotation) { diff --git a/prog/kube-utils/peerlist_test.go b/prog/kube-utils/peerlist_test.go index 77866f69..0a523494 100644 --- a/prog/kube-utils/peerlist_test.go +++ b/prog/kube-utils/peerlist_test.go @@ -137,7 +137,7 @@ func TestPeerListFuzz(t *testing.T) { found := storedPeerList.contains(peerName(i)) require.True(t, found, "peer %d not found in stored list", i) - _, err = reclaimPeer(mockWeave{}, cml, storedPeerList, peerName(i), fmt.Sprintf("deleter-%d", i)) + _, err = reclaimPeer(mockWeave{}, cml, peerName(i), fmt.Sprintf("deleter-%d", i)) require.NoError(t, err) storedPeerList, err = cml.GetPeerList() From 873781ab580773c17c573383bd77fd709207a3e6 Mon Sep 17 00:00:00 2001 From: Murali Reddy Date: Fri, 26 Apr 2019 15:15:47 +0530 Subject: [PATCH 5/7] bump up the version of entry by large value so that transfer of range takes precedence (if there is any conflict in merging entry) over other operations on the entry that increments the version Fixes #3612 --- ipam/ring/ring.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/ipam/ring/ring.go b/ipam/ring/ring.go index 3cd45099..55e139f8 100644 --- a/ipam/ring/ring.go +++ b/ipam/ring/ring.go @@ -565,7 +565,9 @@ func (r *Ring) Transfer(from, to mesh.PeerName) []address.Range { for i, entry := range r.Entries { if entry.Peer == from { entry.Peer = to - entry.Version++ + // bump version by large value so that transfer of range takes precedence (if there is any conflict in merging entry) + // over other opertaions on the entry that increments the version + entry.Version = entry.Version + 100 newRanges = append(newRanges, address.Range{Start: entry.Token, End: r.Entries.entry(i + 1).Token}) } } From 4f8fe2c13ff17e955f56a1c078fadb90c6eea4bd Mon Sep 17 00:00:00 2001 From: Murali Reddy Date: Tue, 9 Apr 2019 13:29:45 +0530 Subject: [PATCH 6/7] gossip broadcast updated rin on donating a range to other peer Fixes #3628 --- ipam/allocator.go | 1 + 1 file changed, 1 insertion(+) diff --git a/ipam/allocator.go b/ipam/allocator.go index bfff91f0..63d0f658 100644 --- a/ipam/allocator.go +++ b/ipam/allocator.go @@ -910,6 +910,7 @@ func (alloc *Allocator) donateSpace(r address.Range, to mesh.PeerName) { alloc.debugln("Giving range", chunk, "to", to) alloc.ring.GrantRangeToHost(chunk.Start, chunk.End, to) alloc.persistRing() + alloc.gossip.GossipBroadcast(alloc.Gossip()) } func (alloc *Allocator) assertInvariants() { From 8a4fc9c02ae52d64d19aa65512e1f078758c70e0 Mon Sep 17 00:00:00 2001 From: Bryan Boreham Date: Fri, 3 May 2019 17:25:36 +0000 Subject: [PATCH 7/7] Fix peer-list fuzz test to re-fetch list when it may be nil --- prog/kube-utils/peerlist_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/prog/kube-utils/peerlist_test.go b/prog/kube-utils/peerlist_test.go index 0a523494..093c575b 100644 --- a/prog/kube-utils/peerlist_test.go +++ b/prog/kube-utils/peerlist_test.go @@ -140,6 +140,7 @@ func TestPeerListFuzz(t *testing.T) { _, err = reclaimPeer(mockWeave{}, cml, peerName(i), fmt.Sprintf("deleter-%d", i)) require.NoError(t, err) + require.NoError(t, cml.Init()) storedPeerList, err = cml.GetPeerList() require.NoError(t, err) stateLock.Lock()