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

Extract instance limits check into middleware, that runs before any other distributor middlewares #2709

Merged
merged 5 commits into from
Aug 12, 2022

Conversation

pstibrany
Copy link
Member

What this PR does

After merging #2603, instance limits checks are done only after running HA deduplication. That's not correct. This PR moves instance limits checks into its own middleware, and makes it the first middleware to run.

Checklist

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

@pstibrany
Copy link
Member Author

@treid314 since #2603 is in 2.3.0-rc.0, I think this bugfix should go to 2.3.0 branch as well. WDYT?

@pstibrany pstibrany requested a review from replay August 12, 2022 10:34
…ther middlewares (HA dedupe, forwarding).

Added test to check that instance limits are called before HA deduplication.

CHANGELOG.md

Signed-off-by: Peter Štibraný <pstibrany@gmail.com>
@pstibrany pstibrany force-pushed the move-instance-limits-into-middleware branch from 9dc4fee to 5418bd8 Compare August 12, 2022 10:39
@github-actions

This comment has been minimized.

Copy link
Contributor

@replay replay left a comment

Choose a reason for hiding this comment

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

This makes sense to me, thanks for working on it!

@pstibrany
Copy link
Member Author

This makes sense to me, thanks for working on it!

Thanks for your review @replay. I've addressed your feedback, please take a look again.

@github-actions

This comment has been minimized.

Co-authored-by: Mauro Stettler <mauro.stettler@gmail.com>
@github-actions

This comment has been minimized.

Signed-off-by: Peter Štibraný <pstibrany@gmail.com>
@github-actions
Copy link
Contributor

Helm <> Jsonnet Diff

⚠️ A difference was detected between the Helm chart and the Jsonnet library.

  1. Use make check-helm-jsonnet-diff to reproduce the output locally.
  2. This test is experimental while we gather feedback about its usefulness.
  3. It does not block your PR from being merged, but we would appreciate you trying to keep feature parity between the Helm chart and Jsonnet library if possible.

If you get stuck on this step and the Mimir maintainers aren't able to help, feel free to merge without making this step pass and tag @Logiraptor so the Mimir maintainers can gather feedback later.

Please see the contribution docs here for more info.

Expand to see the output

Output of https://github.com/grafana/mimir/actions/runs/2847701525

Warning: policy/v1beta1 PodDisruptionBudget is deprecated in v1.21+, unavailable in v1.25+; use policy/v1 PodDisruptionBudget
diff -r -u -N scratch/./helm/07-config/ingester-MimirConfig.yml scratch/./jsonnet/08-config/ingester-MimirConfig.yml
--- scratch/./helm/07-config/ingester-MimirConfig.yml	2022-08-12 15:25:37.493639489 +0000
+++ scratch/./jsonnet/08-config/ingester-MimirConfig.yml	2022-08-12 15:25:46.961948798 +0000
@@ -553,7 +553,7 @@
     max_fetched_chunks_per_query: 2000000 (default)
     max_fetched_series_per_query: 0 (default)
     max_global_exemplars_per_user: 0 (default)
-    max_global_series_per_metric: 20000 (default)
+    max_global_series_per_metric: 0
     max_global_series_per_user: 150000 (default)
     max_label_name_length: 1024 (default)
     max_label_names_per_series: 30 (default)
diff -r -u -N scratch/./helm/07-config/overrides-exporter-MimirConfig.yml scratch/./jsonnet/08-config/overrides-exporter-MimirConfig.yml
--- scratch/./helm/07-config/overrides-exporter-MimirConfig.yml	2022-08-12 15:25:37.501639753 +0000
+++ scratch/./jsonnet/08-config/overrides-exporter-MimirConfig.yml	2022-08-12 15:25:46.973949184 +0000
@@ -498,7 +498,7 @@
     max_fetched_chunks_per_query: 2000000 (default)
     max_fetched_series_per_query: 0 (default)
     max_global_exemplars_per_user: 0 (default)
-    max_global_series_per_metric: 20000 (default)
+    max_global_series_per_metric: 0
     max_global_series_per_user: 150000 (default)
     max_label_name_length: 1024 (default)
     max_label_names_per_series: 30 (default)

Copy link
Contributor

@replay replay left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks for addressing my feedback

@pstibrany pstibrany merged commit d1953ea into main Aug 12, 2022
@pstibrany pstibrany deleted the move-instance-limits-into-middleware branch August 12, 2022 15:47
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