Skip to content

Generators: Drop out of the ring before stopping ingestion#4101

Merged
joe-elliott merged 3 commits intografana:mainfrom
joe-elliott:generator-shutdown
Sep 20, 2024
Merged

Generators: Drop out of the ring before stopping ingestion#4101
joe-elliott merged 3 commits intografana:mainfrom
joe-elliott:generator-shutdown

Conversation

@joe-elliott
Copy link
Copy Markdown
Collaborator

What this PR does:
Currently on shutdown the generators set a flag to stop receiving traffic and then remove themselves from the ring. This causes distributors to continue to send to a generator that is refusing traffic for a few seconds.

This PR:

  1. Correctly drops out of the ring first and then sets the flag to stop ingestion
  2. Adds a sleep in between these steps. Tempo by default uses memberlist for ring propagation which often requires a few seconds to propagate state from one component to another. Without this sleep the change was ineffective. I do not like the sleep but I'm not sure there's a better solution.

To the left of the red line is what a generator rollout looks like today. To the right is what this change looks like. Note that there are still a few drops but it's significantly reduced.

image

Checklist

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

Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Copy link
Copy Markdown
Contributor

@mdisibio mdisibio left a comment

Choose a reason for hiding this comment

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

This lgtm. I don't have any qualms about a hard-coded 5s sleep when it helps that much. If it had to be something much longer like 60s to help, then yes agree we would want to look at something else like ring.WaitRingStability.

@joe-elliott joe-elliott merged commit 64fd38e into grafana:main Sep 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants