From bd24dd7dae237e71519196fb591a983ff5a299f9 Mon Sep 17 00:00:00 2001 From: Bryan Boreham Date: Tue, 26 Mar 2019 17:19:22 +0000 Subject: [PATCH 1/5] 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/5] 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/5] 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/5] 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 8a4fc9c02ae52d64d19aa65512e1f078758c70e0 Mon Sep 17 00:00:00 2001 From: Bryan Boreham Date: Fri, 3 May 2019 17:25:36 +0000 Subject: [PATCH 5/5] 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()