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

Ruler: add configuration option to disable event-based sync #5053

Merged
merged 2 commits into from
May 24, 2023

Conversation

pracucci
Copy link
Collaborator

What this PR does

The event-based rule groups re-sync introduced in #4975 is expected to benefit Mimir clusters with a large number of tenants, not updating rule groups frequently. However, there are some cases where a single-tenant Mimir cluster may get rule groups re-uploaded very frequently and so the event-based re-sync may actually increase the cost instead of decreasing it.

In this PR I'm simply introducing an answer to disable it, while keeping it enabled by default. To do it, I've took the simplest path: do not put the request to the outbound queue when the event-based sync is disabled. The queue and queue processor are started anyway, but they're not expected to make any dent in terms of resources utilization given they're not going to be used.

Which issue(s) this PR fixes or relates to

Part of #4941.

Checklist

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

@pracucci pracucci requested a review from pstibrany May 23, 2023 08:43
@pracucci pracucci marked this pull request as ready for review May 23, 2023 08:43
@pracucci pracucci requested review from a team as code owners May 23, 2023 08:43
@pstibrany
Copy link
Member

Would it make sense to have this flag per tenant, so that we can avoid syncing rules when a tenant performs API change and only do it via regular (time-based) sync?

Signed-off-by: Marco Pracucci <marco@pracucci.com>
@pracucci pracucci force-pushed the ruler-option-to-disable-event-based-sync branch from e76ee79 to ca53d31 Compare May 23, 2023 12:42
@pracucci
Copy link
Collaborator Author

Would it make sense to have this flag per tenant, so that we can avoid syncing rules when a tenant performs API change and only do it via regular (time-based) sync?

Makes sense. Done in ca53d31.

@pracucci pracucci force-pushed the ruler-option-to-disable-event-based-sync branch from ca53d31 to 7005002 Compare May 23, 2023 12:44
Signed-off-by: Marco Pracucci <marco@pracucci.com>
@pracucci pracucci force-pushed the ruler-option-to-disable-event-based-sync branch from 7005002 to edd5196 Compare May 23, 2023 12:44
Copy link
Member

@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.

Thank you!

Copy link
Contributor

@gotjosh gotjosh left a comment

Choose a reason for hiding this comment

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

LGTM, I was going to suggest the same as peter - if we're initializing all the components anyways might as well just make it a runtime configuration via limits. Great job 🙏

CHANGELOG.md Show resolved Hide resolved
@pracucci pracucci merged commit b800803 into main May 24, 2023
@pracucci pracucci deleted the ruler-option-to-disable-event-based-sync branch May 24, 2023 09:46
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.

3 participants