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.

Sign up for GitHub

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

Jump to bottom

Small fix for off-by-one rules limit enforcement #3616

Merged
merged 1 commit into from
Apr 9, 2021
Merged

Small fix for off-by-one rules limit enforcement #3616

merged 1 commit into from
Apr 9, 2021

Conversation

mattmendick
Copy link
Contributor

@mattmendick mattmendick commented Dec 17, 2020 •
edited
Loading

What this PR does:

Fixes an off by one error where rules in a rule group were not able to be equal to the limit.

Which issue(s) this PR fixes:
#3615

Checklist

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

@mattmendick mattmendick force-pushed the off-by-one-ruler-rules-limit branch from 94dc9e7 to a332bd5 Compare December 17, 2020 16:28
pracucci
pracucci reviewed Dec 17, 2020
View reviewed changes
pracucci
pracucci approved these changes Dec 17, 2020
View reviewed changes
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!

@pracucci
Copy link
Contributor

@mattmendick Could you fix the test TestRuler_Limits/when_exceeding_the_rule_group_limit accordingly?

@bboreham
Copy link
Contributor

Will you update the tests?
Looks like someone kindly wrote a test to check the erroneous behaviour.

@mattmendick
Copy link
Contributor Author

Absolutely! Will work on this soon. Thanks for both of you taking a look!

jtlisi
jtlisi approved these changes Jan 5, 2021
View reviewed changes
Copy link
Contributor

@jtlisi jtlisi left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM, module test case and rebase on master

@jtlisi
Copy link
Contributor

jtlisi commented Jan 8, 2021

After a closer look I think I figured out the issue causing tests to fail. When AssertMaxRuleGroups is called, it's only checking against the length of the number of existing rule groups without including the one you are adding. You need to add 1 to the length before passing to the function unless the rule being created already exists. To check if the rule already exists you can loop through the existing groups and see if any groups exist with the same group name and namespace.

bboreham
bboreham reviewed Jan 11, 2021
View reviewed changes
pkg/ruler/api_test.go Outdated
Comment on lines 403 to 406
// define once so the requests build on each other so the number of rules can be tested
router := mux.NewRouter()
router.Path("/api/v1/rules/{namespace}").Methods("POST").HandlerFunc(a.CreateRuleGroup)

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand this change. It's not run "once" - it's done again for each element in tc.
And I can't see what practical difference moving it outside of Run() makes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right you are, definitely not going to work that way :)

pracucci
pracucci reviewed Jan 22, 2021
View reviewed changes
@bboreham
Copy link
Contributor

@mattmendick do you think you will come back to this?

@mattmendick
Copy link
Contributor Author

I'd like to take another shot at this, many apologies for the length of time this has been taking, and I really appreciate all the guidance and help you all have been giving this! 🙏

@pracucci pracucci mentioned this pull request Apr 8, 2021
Closed
@pstibrany pstibrany force-pushed the off-by-one-ruler-rules-limit branch from 2da334e to 987ca67 Compare April 9, 2021 09:38
@pstibrany
Copy link
Contributor

Based on #3615 (comment), rebased on top of master, and squashed to avoid DCO errors.

gotjosh
gotjosh approved these changes Apr 9, 2021
View reviewed changes
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

@pstibrany pstibrany merged commit 68a917d into cortexproject:master Apr 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pstibrany pstibrany pstibrany left review comments

@pracucci pracucci pracucci approved these changes

@bboreham bboreham bboreham left review comments

@gotjosh gotjosh gotjosh approved these changes

@jtlisi jtlisi jtlisi approved these changes

@gouthamve gouthamve gouthamve approved these changes

Assignees
No one assigned
Labels
size/M
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@mattmendick @pracucci @bboreham @jtlisi @pstibrany @gotjosh @gouthamve