Skip to content

Conversation

gotjosh
Copy link
Contributor

@gotjosh gotjosh commented Oct 19, 2020

What this PR does:

Limits the number of rules per rule group, total rule groups and rules a particular tenant can create.

Which issue(s) this PR fixes:
Fixes #

Checklist

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

@gotjosh gotjosh force-pushed the per-tenant-ruler-limits branch 2 times, most recently from 3926aee to 71e72e0 Compare October 20, 2020 15:55
@pull-request-size pull-request-size bot added size/L and removed size/M labels Oct 20, 2020
Adds two limits to the rules creation. The number of rules per rule
group and the number of rule groups in total, inherently this creates a
limit of maximum number of rules of `max per rule group` x `max rule groups`.

Signed-off-by: gotjosh <[email protected]>
Signed-off-by: gotjosh <[email protected]>
@gotjosh gotjosh force-pushed the per-tenant-ruler-limits branch from 71e72e0 to cbb9057 Compare October 20, 2020 16:38
@gotjosh gotjosh changed the title WIP: Per tenant ruler rule group limits Per tenant ruler rule group limits Oct 20, 2020
Signed-off-by: gotjosh <[email protected]>
@gotjosh gotjosh marked this pull request as ready for review October 20, 2020 18:29
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.

Pretty straightforward, LGTM, with some nitpicks: we use "tenant" terminology in limits recently instead of "user". Also cli flags and YAML options are not consistent (should be the same word by word except _ / -).

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.

Good job! Left few minor comments and then LGTM.

gotjosh and others added 5 commits October 21, 2020 13:07
Co-authored-by: Marco Pracucci <[email protected]>
Signed-off-by: gotjosh <[email protected]>
Co-authored-by: Marco Pracucci <[email protected]>
Signed-off-by: gotjosh <[email protected]>
Co-authored-by: Marco Pracucci <[email protected]>
Signed-off-by: gotjosh <[email protected]>
Co-authored-by: Marco Pracucci <[email protected]>
Signed-off-by: gotjosh <[email protected]>
@gotjosh gotjosh force-pushed the per-tenant-ruler-limits branch from 5f702ba to ef89c42 Compare October 21, 2020 12:08
Signed-off-by: gotjosh <[email protected]>
Signed-off-by: gotjosh <[email protected]>
@gotjosh gotjosh force-pushed the per-tenant-ruler-limits branch from 986595b to d69047f Compare October 21, 2020 15:17
Signed-off-by: gotjosh <[email protected]>
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.

Thanks for patiently address my comments!

Copy link
Contributor

@jtlisi jtlisi left a comment

Choose a reason for hiding this comment

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

One minor suggestion.

Otherwise, LGTM

@pracucci pracucci merged commit 83db5a9 into cortexproject:master Oct 22, 2020
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