Merge pull request #1957 from /issues/1949-ring-errors

Make error messages on ring merge clearer

Author: awh

ipam/ring/ring.go

@@ -40,12 +40,19 @@ var (
 	ErrTokenOutOfRange = errors.New("Token is out of range")
 	ErrDifferentSeeds  = errors.New("Received ring was seeded differently from ours!")
 	ErrDifferentRange  = errors.New("Received range differs from ours!")
-	ErrNewerVersion    = errors.New("Received new version for entry I own!")
-	ErrInvalidEntry    = errors.New("Received invalid state update!")
-	ErrEntryInMyRange  = errors.New("Received new entry in my range!")
 	ErrNotFound        = errors.New("No entries for peer found")
 )
 
+func errInconsistentEntry(mine, theirs *entry) error {
+	return fmt.Errorf("Inconsistent entries for %s: owned by %s but incoming message says %s", mine.Token, mine.Peer, theirs.Peer)
+}
+func errEntryInMyRange(theirs *entry) error {
+	return fmt.Errorf("Peer %s says it owns the IP range from %s, which I think I own", theirs.Peer, theirs.Token)
+}
+func errNewerVersion(mine, theirs *entry) error {
+	return fmt.Errorf("Received update for IP range I own at %s v%d: incoming message says owner %s v%d", mine.Token, mine.Version, theirs.Peer, theirs.Version)
+}
+
 func (r *Ring) checkInvariants() error {
 	if !sort.IsSorted(r.Entries) {
 		return ErrNotSorted
@@ -224,7 +231,7 @@ 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
+				return errEntryInMyRange(theirs)
 			}
 			addToResult(*theirs)
 			previousOwner = nil
@@ -234,14 +241,13 @@ func (r *Ring) Merge(gossip Ring) error {
 			switch {
 			case mine.Version >= theirs.Version:
 				if mine.Version == theirs.Version && !mine.Equal(theirs) {
-					common.Log.Debugf("Error merging entries at %s - %v != %v", mine.Token, mine, theirs)
-					return ErrInvalidEntry
+					return 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
+					return errNewerVersion(mine, theirs)
 				}
 				addToResult(*theirs)
 				previousOwner = nil
@@ -262,7 +268,7 @@ 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
+			return errEntryInMyRange(theirs)
 		}
 		addToResult(*theirs)
 		previousOwner = nil

ipam/ring/ring_test.go

@@ -225,17 +225,18 @@ func TestMergeErrors(t *testing.T) {
 	ring2 = New(start, end, peer2name)
 	ring1.Entries = []*entry{{Token: start, Peer: peer1name}}
 	ring2.Entries = []*entry{{Token: start, Peer: peer1name, Version: 1}}
-	require.True(t, ring1.Merge(*ring2) == ErrNewerVersion, "Expected ErrNewerVersion")
+	fmt.Println(ring1.Merge(*ring2))
+	require.Error(t, ring1.Merge(*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.True(t, ring1.Merge(*ring2) == ErrInvalidEntry, "Expected ErrInvalidEntry")
+	require.Error(t, ring1.Merge(*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.True(t, ring1.Merge(*ring2) == ErrEntryInMyRange, "Expected ErrEntryInMyRange")
+	require.Error(t, ring1.Merge(*ring2), "Expected error")
 }
 
 func TestMergeMore(t *testing.T) {