Skip to content

Cleanup of bindings for PrometheusRules in prometheus #8024

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
Merged
simonpasquier merged 4 commits into from
Oct 17, 2025
Merged

Cleanup of bindings for PrometheusRules in prometheus #8024

simonpasquier merged 4 commits into from
Oct 17, 2025

Conversation

@yp969803
Copy link
Contributor

@yp969803 yp969803 commented Oct 13, 2025

Description

Cleanup of bindings for PrometheusRules in prometheus

Closes: #7924

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

Verification

Please check the Prometheus-Operator testing guidelines for recommendations about automated tests.

Changelog entry

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

@yp969803 yp969803 requested a review from a team as a code owner October 13, 2025 12:57
@yp969803
Copy link
Contributor Author

yp969803 commented Oct 14, 2025 •
edited
Loading

@simonpasquier review and merge!
Also i don't know why the test

Error Trace:	/home/runner/work/prometheus-operator/prometheus-operator/test/e2e/status_subresource_test.go:222
        	Error:      	Received unexpected error:
        	            	serviceMonitor status gatedfeatures-servicemonitorstatuswithmultipleworkload-3f7286f2/servicemonitor-status-multiple-workloads failed to reach expected condition: context deadline exceeded: resource "prometheuses" gatedfeatures-servicemonitorstatuswithmultipleworkload-3f7286f2/server1: client rate limiter Wait returned an error: context deadline exceeded
        	Test:       	TestGatedFeatures/ServiceMonitorStatusWithMultipleWorkloads

is failing i dont think it belong to my code changes

@simonpasquier
Copy link
Contributor

simonpasquier commented Oct 14, 2025

see #8029 and #8028 for the failing test.

simonpasquier
simonpasquier reviewed Oct 14, 2025
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.

I'm not sure to understand why we need a custom CleanupPrometheusRuleBindings() for rules?

@yp969803
Copy link
Contributor Author

yp969803 commented Oct 14, 2025

@simonpasquier we are using different value of key for promRule k := fmt.Sprintf("%v-%v-%v.yaml", promRule.Namespace, promRule.Name, promRule.UID)

@simonpasquier
Copy link
Contributor

simonpasquier commented Oct 15, 2025

Thanks for the clarification. I'd prefer to avoid a special case for PrometheusRule.
Can we change the current logic to index PrometheusRule resources by namespace/name in the selection map and move the name expansion to the rule syncer (ideally in a separate PR)? In theory the "Selector" shouldn't are about the rule filenames (though I understand why it does now).

@yp969803
Copy link
Contributor Author

yp969803 commented Oct 16, 2025

@simonpasquier PTAL

simonpasquier
simonpasquier approved these changes Oct 17, 2025
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.

LGTM

@simonpasquier simonpasquier merged commit 0ecdf47 into prometheus-operator:main Oct 17, 2025
22 checks passed
tboerger pushed a commit to tboerger/prometheus-operator that referenced this pull request Dec 20, 2025
@yp969803 @tboerger
Cleanup of bindings for PrometheusRules in prometheus (prometheus-ope…
0b985b4 
…rator#8024)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@simonpasquier simonpasquier simonpasquier approved these changes

Assignees

No one assigned

Labels

size/L

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Remove binding from PrometheusRule's status subresource when Prometheus/ThanosRuler gets deleted

2 participants

@yp969803 @simonpasquier