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

Wait for alertmanagers to complete state sync before becoming ACTIVE. #4161

Merged
merged 2 commits into from
May 11, 2021

Conversation

stevesg
Copy link
Contributor

@stevesg stevesg commented May 6, 2021

What this PR does:

Note: Applies to sharding operation only.

When starting up, make sure that the initial state sync is completed for
any users which were initially assigned to the instance. This will reduce
the possibility that we lose state when [cleanly] scaling up or down, by
inadvertently losing the replica which we need to obtain state from.

This change allows the state sync to progress concurrently for all
tenants. The alternative would be to block for the service in New(),
which is a slightly simpler code change, but would mean that start-up
is potentially much slower.

Checklist

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

Note: Applies to sharding operation only.

When starting up, make sure that the initial state sync is completed for
any users which were initially assigned to the instance. This will reduce
the possibility that we lose state when [cleanly] scaling up or down, by
inadvertently losing the replica which we need to obtain state from.

This change allows the state sync to progress concurrently for all
tenants. The alternative would be to block for the service in `New()`,
which is a slightly simpler code change, but would mean that start-up
is potentially much slower.

Signed-off-by: Steve Simpson <[email protected]>
@stevesg stevesg force-pushed the wait-initial-sync branch from c8dc536 to 2a02ffd Compare May 6, 2021 12:45
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.

Makes sense to me! I've added a couple of comments but overall LGTM. Thanks!

Copy link
Contributor

@pstibrany pstibrany left a comment

Choose a reason for hiding this comment

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

LGTM, thanks.

Signed-off-by: Steve Simpson <[email protected]>
@pracucci pracucci merged commit e399c52 into cortexproject:master May 11, 2021
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.

3 participants