diff --git a/CHANGELOG.md b/CHANGELOG.md index 8c59f0b66a..fb3c7d5499 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,7 @@ * [ENHANCEMENT] Memberlist: client can now keep a size-bounded buffer with sent and received messages and display them in the admin UI (/memberlist) for troubleshooting. #3581 * [BUGFIX] Allow `-querier.max-query-lookback` use `y|w|d` suffix like deprecated `-store.max-look-back-period`. #3598 * [BUGFIX] Query-Frontend: `cortex_query_seconds_total` now return seconds not nanoseconds. #3589 +* [BUGFIX] Memberlist: Entry in the ring should now not appear again after using "Forget" feature (unless it's still heartbeating). #3603 ## 1.6.0-rc.0 in progress diff --git a/pkg/ring/merge_test.go b/pkg/ring/merge_test.go index d050deb799..6d5cb2e90c 100644 --- a/pkg/ring/merge_test.go +++ b/pkg/ring/merge_test.go @@ -356,30 +356,30 @@ func TestMergeRemoveMissing(t *testing.T) { Ingesters: map[string]IngesterDesc{ "Ing 1": {Addr: "addr1", Timestamp: now, State: ACTIVE, Tokens: []uint32{30, 40, 50}}, "Ing 2": {Addr: "addr2", Timestamp: now + 5, State: ACTIVE, Tokens: []uint32{5, 10, 20, 100, 200}}, - "Ing 3": {Addr: "addr3", Timestamp: now, State: LEFT}, + "Ing 3": {Addr: "addr3", Timestamp: now + 3, State: LEFT}, // When deleting, time depends on value passed to merge function. }, } } { - our, ch := mergeLocalCAS(firstRing(), secondRing()) + our, ch := mergeLocalCAS(firstRing(), secondRing(), now+3) assert.Equal(t, expectedFirstSecondMerge(), our) assert.Equal(t, &Desc{ Ingesters: map[string]IngesterDesc{ "Ing 2": {Addr: "addr2", Timestamp: now + 5, State: ACTIVE, Tokens: []uint32{5, 10, 20, 100, 200}}, - "Ing 3": {Addr: "addr3", Timestamp: now, State: LEFT}, + "Ing 3": {Addr: "addr3", Timestamp: now + 3, State: LEFT}, // When deleting, time depends on value passed to merge function. }, }, ch) // entire second ring is new } - { // idempotency: (no change after applying same ring again) - our, ch := mergeLocalCAS(expectedFirstSecondMerge(), secondRing()) + { // idempotency: (no change after applying same ring again, even if time has advanced) + our, ch := mergeLocalCAS(expectedFirstSecondMerge(), secondRing(), now+10) assert.Equal(t, expectedFirstSecondMerge(), our) assert.Equal(t, (*Desc)(nil), ch) } { // commutativity is broken when deleting missing entries. But let's make sure we get reasonable results at least. - our, ch := mergeLocalCAS(secondRing(), firstRing()) + our, ch := mergeLocalCAS(secondRing(), firstRing(), now+3) assert.Equal(t, &Desc{ Ingesters: map[string]IngesterDesc{ "Ing 1": {Addr: "addr1", Timestamp: now, State: ACTIVE, Tokens: []uint32{30, 40, 50}}, @@ -419,7 +419,7 @@ func TestMergeMissingIntoLeft(t *testing.T) { } { - our, ch := mergeLocalCAS(ring1(), ring2()) + our, ch := mergeLocalCAS(ring1(), ring2(), now+10) assert.Equal(t, &Desc{ Ingesters: map[string]IngesterDesc{ "Ing 1": {Addr: "addr1", Timestamp: now + 10, State: ACTIVE, Tokens: []uint32{30, 40, 50}}, @@ -438,8 +438,8 @@ func TestMergeMissingIntoLeft(t *testing.T) { } } -func mergeLocalCAS(ring1, ring2 *Desc) (*Desc, *Desc) { - change, err := ring1.Merge(ring2, true) +func mergeLocalCAS(ring1, ring2 *Desc, nowUnixTime int64) (*Desc, *Desc) { + change, err := ring1.mergeWithTime(ring2, true, time.Unix(nowUnixTime, 0)) if err != nil { panic(err) } diff --git a/pkg/ring/model.go b/pkg/ring/model.go index d9ebd78155..e4a2ea45b9 100644 --- a/pkg/ring/model.go +++ b/pkg/ring/model.go @@ -189,6 +189,10 @@ func (i *IngesterDesc) IsHealthy(op Operation, heartbeatTimeout time.Duration) b // // This method is part of memberlist.Mergeable interface, and is only used by gossiping ring. func (d *Desc) Merge(mergeable memberlist.Mergeable, localCAS bool) (memberlist.Mergeable, error) { + return d.mergeWithTime(mergeable, localCAS, time.Now()) +} + +func (d *Desc) mergeWithTime(mergeable memberlist.Mergeable, localCAS bool, now time.Time) (memberlist.Mergeable, error) { if mergeable == nil { return nil, nil } @@ -229,6 +233,10 @@ func (d *Desc) Merge(mergeable memberlist.Mergeable, localCAS bool) (memberlist. // missing, let's mark our ingester as LEFT ting.State = LEFT ting.Tokens = nil + // We are deleting entry "now", and should not keep old timestamp, because there may already be pending + // message in the gossip network with newer timestamp (but still older than "now"). + // Such message would "resurrect" this deleted entry. + ting.Timestamp = now.Unix() thisIngesterMap[name] = ting updated = append(updated, name)