Skip to content

Use most recent time to remove from ring #4501

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

Use most recent time to remove from ring #4501

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@
* [BUGFIX] Querier: fixed panic when querying exemplars and using `-distributor.shard-by-all-labels=false`. #4473
* [BUGFIX] Querier: honor querier minT,maxT if `nil` SelectHints are passed to Select(). #4413
* [BUGFIX] Compactor: fixed panic while collecting Prometheus metrics. #4483
* [BUGFIX] Memberlist: Remove entry from the ring even if the timestamp is in the future #4501

Comment on lines +63 to 64
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This entry should be moved to master / unreleased section now.


## 1.10.0 / 2021-08-03
Expand Down
35 changes: 33 additions & 2 deletions pkg/ring/merge_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -356,7 +356,27 @@ func TestMergeRemoveMissing(t *testing.T) {
Ingesters: map[string]InstanceDesc{
"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 + 3, State: LEFT}, // When deleting, time depends on value passed to merge function.
"Ing 3": {Addr: "addr3", Timestamp: now + 3, State: LEFT},
},
}
}

thirdRing := func() *Desc {
return &Desc{
Ingesters: map[string]InstanceDesc{
"Ing 1": {Addr: "addr1", Timestamp: now, State: ACTIVE, Tokens: []uint32{30, 40, 50}},
"Ing 2": {Addr: "addr2", Timestamp: now, State: JOINING, Tokens: []uint32{5, 10, 20, 100, 200}},
"Ing 3": {Addr: "addr3", Timestamp: now + 50, State: LEAVING, Tokens: []uint32{5, 10, 20, 100, 200}},
},
}
}

expectedThirdSecondMerge := func() *Desc {
return &Desc{
Ingesters: map[string]InstanceDesc{
"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 + 50, State: LEFT},
},
}
}
Expand All @@ -367,7 +387,18 @@ func TestMergeRemoveMissing(t *testing.T) {
assert.Equal(t, &Desc{
Ingesters: map[string]InstanceDesc{
"Ing 2": {Addr: "addr2", Timestamp: now + 5, State: ACTIVE, Tokens: []uint32{5, 10, 20, 100, 200}},
"Ing 3": {Addr: "addr3", Timestamp: now + 3, State: LEFT}, // When deleting, time depends on value passed to merge function.
"Ing 3": {Addr: "addr3", Timestamp: now + 3, State: LEFT}, // When deleting, time is recent between now and current timestamp
},
}, ch) // entire second ring is new
}

{
our, ch := mergeLocalCAS(thirdRing(), secondRing(), now+10)
assert.Equal(t, expectedThirdSecondMerge(), our)
assert.Equal(t, &Desc{
Ingesters: map[string]InstanceDesc{
"Ing 2": {Addr: "addr2", Timestamp: now + 5, State: ACTIVE, Tokens: []uint32{5, 10, 20, 100, 200}},
"Ing 3": {Addr: "addr3", Timestamp: now + 50, State: LEFT}, // When deleting, time is recent between now and current timestamp
},
}, ch) // entire second ring is new
}
Expand Down
7 changes: 6 additions & 1 deletion pkg/ring/model.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,7 +210,12 @@ func (d *Desc) mergeWithTime(mergeable memberlist.Mergeable, localCAS bool, now
// 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()
// If timestamp on the ring is in "the future" we need to use the timestamp of the ring
// otherwise the gossip message will be ignored by the peers.
// see https://github.com/cortexproject/cortex/blob/d3f46c53616dcdf5c670f3e3a4d371e44b10683e/pkg/ring/model.go#L192-L200
if ting.Timestamp < now.Unix() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure this fixes your issue. We must update the timestamp, otherwise the change is not propagated via gossip, so I think that what you want is something more like this:

if ting.Timestamp < now.Unix() {
    ting.Timestamp = now.Unix()
} else {
    ting.Timestamp += 1
}

Copy link
Contributor Author

@danielblando danielblando Oct 6, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pracucci thanks for looking into this.

We do add the LEFT change right after this line in the updates list.

cortex/pkg/ring/model.go

Line 221 in 5e7f104

updated = append(updated, name)

which will go onto the changes returned

cortex/pkg/ring/model.go

Lines 236 to 241 in 5e7f104

// Let's build a "change" for returning
out := NewDesc()
for _, u := range updated {
ing := thisIngesterMap[u]
out.Ingesters[u] = ing
}

Form what i can tell we don't depend on timestamp

I think what we need to make sure is that the misconfigured ingester be shut down before forgetting it, which will make sure it does not rejoin the ring.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO it's best to put a newer timestamp on newer data.
That way everyone in the gossip will treat it as new, not be uncertain whether they should use the other info.

ting.Timestamp = now.Unix()
}
thisIngesterMap[name] = ting

updated = append(updated, name)
Expand Down