Skip to content
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.

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

Memberlist ignore old tombstones #4420

Merged

Conversation

pstibrany
Copy link
Contributor

@pstibrany pstibrany commented Aug 13, 2021

What this PR does: This PR modifies memberlist KV store to avoid "accepting" old tombstones as a change, and avoid forwarding them to other gossip members. Before this PR, if there was a message with old tombstone in a gossip network, such message would basically never "die".

This PR builds on top of #4419, and will be rebased once #4419 is merged. Update: Rebased.

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

Copy link
Contributor

@joe-elliott joe-elliott left a comment

Choose a reason for hiding this comment

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

I'm not as knowledgeable as @pstibrany in memberlist ring propagation but this change as well as #4419 are consistent with my understanding of how this works.

Additionally, I can confirm that deploying these changes to an environment with "unforgettable" ring members prevents the propagation of messages that were keeping these members around.

@pstibrany
Copy link
Contributor Author

@joe-elliott these two PRs wouldn't be possible without your huge help with troubleshooting this and insights about NotifyMsg!

(I still think there may be another bug lurking as I cannot yet explain why did nodes keep gossiping about those old tombstones, and why didn't such messages die during LeftIngestersTimeout period.)

…d will not gossip about them.

Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
@pstibrany pstibrany force-pushed the memberlist-ignore-old-tombstones branch from e908472 to 6573606 Compare August 16, 2021 07:32
Copy link
Contributor

@pracucci pracucci left a comment

Choose a reason for hiding this comment

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

The change makes sense to me.

@pstibrany pstibrany enabled auto-merge (squash) August 16, 2021 07:47
@pstibrany pstibrany merged commit 090988c into cortexproject:master Aug 16, 2021
@pstibrany pstibrany deleted the memberlist-ignore-old-tombstones branch August 16, 2021 08:13
@@ -21,6 +21,7 @@
* [CHANGE] Some files and directories created by Mimir components on local disk now have stricter permissions, and are only readable by owner, but not group or others. #4394
* [CHANGE] Compactor: compactor will no longer try to compact blocks that are already marked for deletion. Previously compactor would consider blocks marked for deletion within `-compactor.deletion-delay / 2` period as eligible for compaction. #4328
* [CHANGE] Memberlist: forward only changes, not entire original message. #4419
* [CHANGE] Memberlist: don't accept old tombstones as incoming change, and don't forward such messages to other gossip members. #4420
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this listed as a breaking change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good shout. Should be bugfix as well.

alvinlin123 pushed a commit to ac1214/cortex that referenced this pull request Jan 14, 2022
* Memberlist KV will no longer consider old tombstones as a "change" and will not gossip about them.

Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>

* CHANGELOG.md

Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>
Signed-off-by: Alvin Lin <alvinlin@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants