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

pkg/apis: Support Prometheus RuleGroup Limit #4999

Merged
merged 1 commit into from
Apr 27, 2023
Merged

pkg/apis: Support Prometheus RuleGroup Limit #4999

merged 1 commit into from
Apr 27, 2023

Conversation

slashpai
Copy link
Contributor

@slashpai slashpai commented Sep 7, 2022 •
edited

Fixes #4998

Signed-off-by: Jayapriya Pai slashpai9@gmail.com

Description

Support Prometheus RuleGroup Limit

Type of change

What type of changes does your code introduce to the Prometheus operator? Put an x in the box that apply.

  • CHANGE (fix or feature that would cause existing functionality to not work as expected)
  • FEATURE (non-breaking change which adds functionality)
  • BUGFIX (non-breaking change which fixes an issue)
  • ENHANCEMENT (non-breaking change which improves existing functionality)
  • NONE (if none of the other choices apply. Example, tooling, build system, CI, docs, etc.)

Changelog entry

Please put a one-line changelog entry below. This will be copied to the changelog file during the release process.

Support Prometheus RuleGroup Limit

@slashpai slashpai force-pushed the am_limit branch 4 times, most recently from 601ea3b to fa7bc2a Compare October 14, 2022 14:16
@slashpai slashpai changed the title pkg/apis: Support Alertmanager RuleGroup Limit pkg/apis: Support Prometheus RuleGroup Limit Oct 14, 2022
@slashpai slashpai marked this pull request as ready for review October 14, 2022 14:17
@slashpai slashpai requested a review from a team as a code owner October 14, 2022 14:17
@slashpai
Copy link
Contributor Author

@simonpasquier PTAL

@simonpasquier
Copy link
Contributor

We need a version check when generating the rules configmaps because when Prometheus < 2.31 or Thanos < 0.24, the field should be omitted.
I suggest to take this as an opportunity to rework GenerateContent(): it should take into account the flavor (Prometheus vs. Thanos) + whether RuleGroup's limit is supported or not.

@slashpai slashpai mentioned this pull request Oct 28, 2022
Closed
@cuizhaoyue
Copy link

@simonpasquier when can this PR merge to the main branch? I am looking for the function that support prometheus RuleGroup Limit.

@slashpai
Copy link
Contributor Author

#5124 this needs to be merged so that I can update it further to do version check

@simonpasquier
Copy link
Contributor

@slashpai I guess you meant #5177 which I've just merged :)

Something that would be a natural follow-up is to add a spec.enforcedGroupLimit field to the Prometheus and ThanosRuler CRDs (e.g. a way for admins to ensure that user rules don't overload the system).

@slashpai
Copy link
Contributor Author

I meant #5124 itself since that was modifying generate function and ya ofcourse the #5177 was follow up after that :)

I will update this PR soon

@slashpai slashpai force-pushed the am_limit branch 3 times, most recently from 54a08d4 to 00d3368 Compare December 12, 2022 06:28
@github-actions github-actions bot added the stale label Feb 11, 2023
@slashpai slashpai removed the stale label Mar 13, 2023
@slashpai slashpai force-pushed the am_limit branch 2 times, most recently from 5ed52cd to 9b20045 Compare March 14, 2023 12:11
@slashpai slashpai marked this pull request as draft March 14, 2023 12:45
@slashpai slashpai force-pushed the am_limit branch 2 times, most recently from f0660f4 to b3245a2 Compare March 14, 2023 14:27
@slashpai slashpai marked this pull request as ready for review March 15, 2023 02:33
JoaoBraveCoding
JoaoBraveCoding reviewed Mar 15, 2023
View reviewed changes
pkg/operator/rules.go Outdated Show resolved Hide resolved
pkg/operator/rules.go Outdated Show resolved Hide resolved
@JoaoBraveCoding
Copy link
Contributor

Also, it seems you might have forgotten to run make format generate

@slashpai
Copy link
Contributor Author

Also, it seems you might have forgotten to run make format generate

No I did run that. Looks like go version issue since I am using 1.19. I will update to 1.20 and format again

@slashpai slashpai force-pushed the am_limit branch 3 times, most recently from cf712b9 to 4814dda Compare March 16, 2023 12:49
@slashpai
Copy link
Contributor Author

@simonpasquier @JoaoBraveCoding Addressed review comments

JoaoBraveCoding
JoaoBraveCoding approved these changes Mar 17, 2023
View reviewed changes
Copy link
Contributor

@JoaoBraveCoding JoaoBraveCoding left a comment

Choose a reason for hiding this comment

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

One small non-blocking nit, apart from that it's a /lgtm from my side

Good job JP! 💪

pkg/operator/rules_test.go Outdated Show resolved Hide resolved
simonpasquier
simonpasquier reviewed Mar 20, 2023
View reviewed changes
pkg/operator/rules.go Outdated Show resolved Hide resolved
pkg/operator/rules.go Outdated Show resolved Hide resolved
pkg/operator/rules.go Outdated Show resolved Hide resolved
pkg/operator/rules.go Outdated Show resolved Hide resolved
pkg/operator/rules.go Outdated Show resolved Hide resolved
@slashpai slashpai force-pushed the am_limit branch 2 times, most recently from 3b465a9 to 4f8aeb5 Compare April 19, 2023 06:23
@slashpai
Copy link
Contributor Author

This is ready for review again :)
cc: @simonpasquier

simonpasquier
simonpasquier reviewed Apr 24, 2023
View reviewed changes
pkg/apis/monitoring/v1/prometheusrule_types.go Outdated Show resolved Hide resolved
pkg/operator/rules.go Outdated Show resolved Hide resolved
pkg/operator/rules.go Outdated Show resolved Hide resolved
pkg/operator/rules.go Outdated Show resolved Hide resolved
@slashpai
Copy link
Contributor Author

@simonpasquier addressed comments

@slashpai slashpai force-pushed the am_limit branch 2 times, most recently from b576352 to 5cfa200 Compare April 26, 2023 10:09
JoaoBraveCoding
JoaoBraveCoding reviewed Apr 26, 2023
View reviewed changes
Copy link
Contributor

@JoaoBraveCoding JoaoBraveCoding left a comment

Choose a reason for hiding this comment

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

one small nit

pkg/apis/monitoring/v1/prometheusrule_types.go Outdated Show resolved Hide resolved
@slashpai
pkg/apis: Support Prometheus RuleGroup Limit
b221227 
Fixes prometheus-operator#4998

Signed-off-by: Jayapriya Pai <slashpai9@gmail.com>
@slashpai
Copy link
Contributor Author

should be good now

simonpasquier
simonpasquier approved these changes Apr 27, 2023
View reviewed changes
Copy link
Contributor

@simonpasquier simonpasquier left a comment

Choose a reason for hiding this comment

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

thanks!

@simonpasquier simonpasquier merged commit 0b0de7e into prometheus-operator:main Apr 27, 2023
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JoaoBraveCoding JoaoBraveCoding JoaoBraveCoding approved these changes

@simonpasquier simonpasquier simonpasquier approved these changes

@cuizhaoyue cuizhaoyue cuizhaoyue approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

The "GroupRule" struct is missing the "limit" field.
4 participants
@slashpai @simonpasquier @cuizhaoyue @JoaoBraveCoding