Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@sradco: This pull request references CNV-80608 which is a valid jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. WalkthroughThe change adds garbage collection for orphaned plugin-managed AlertRelabelConfigs. Syncs collect live rule IDs and run cleanup for initial and PrometheusRule events. Unit and end-to-end tests cover deletion and retention rules. ChangesAlertRelabelConfig garbage collection
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~30 minutes Change: Feature Sequence Diagram(s)sequenceDiagram
participant PrometheusRule
participant relabeledRulesManager
participant AlertRelabelConfigClient
PrometheusRule->>relabeledRulesManager: trigger PrometheusRule sync
relabeledRulesManager->>relabeledRulesManager: collect live rule IDs
relabeledRulesManager->>AlertRelabelConfigClient: list AlertRelabelConfigs
relabeledRulesManager->>AlertRelabelConfigClient: delete eligible orphaned ARCs
Merge Risk: ⚪ Minimal · up to The added garbage-collection end-to-end coverage has no substantiated merge-blocking issue. 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Some tools did not complete. Review the errors below. 🔧 golangci-lint (2.13.2)level=error msg="[linters_context] typechecking error: build constraints exclude all Go files in /test/e2e" Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: sradco The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
sradco
changed the title
|
@sradco: This pull request references CNV-87535 which is a valid jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
0d89ed9 to
d056eaa
Compare
d056eaa to
63a08fb
Compare
63a08fb to
3c01803
Compare
|
/test ? |
|
/test e2e-management-api |
Detect and remove orphan AlertRelabelConfig resources that no longer have a matching PrometheusRule, preventing stale relabel configs from accumulating. Cover orphan deletion and keeper cases in e2e (live rule, GitOps, unannotated). Signed-off-by: Shirly Radco <sradco@redhat.com> Co-authored-by: AI Assistant <noreply@cursor.com>
3c01803 to
b6230df
Compare
|
/test e2e-management-api |
|
@sradco: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
|
||
| arcs, err := rrm.alertRelabelConfigs.List(ctx, "") | ||
| if err != nil { | ||
| log.Errorf("orphan ARC GC: failed to list ARCs: %v", err) |
There was a problem hiding this comment.
(nit) we shouldn't use acronyms in log messages.
| } | ||
|
|
||
| if err := rrm.sync(ctx); err != nil { | ||
| if err := rrm.sync(ctx, "initial-sync"); err != nil { |
| } | ||
|
|
||
| arcs, err := rrm.alertRelabelConfigs.List(ctx, "") | ||
| if err != nil { |
There was a problem hiding this comment.
how will cluster admins know that something's not going correctly? e.g. can we add metrics + alerting rule?
| continue | ||
| } | ||
|
|
||
| if err := rrm.alertRelabelConfigs.Delete(ctx, arc.Namespace, arc.Name); err != nil { |
There was a problem hiding this comment.
same question here about the ability to know that something failed.
| // | ||
| // Only ARCs carrying the plugin's alertRuleId annotation are considered. | ||
| // GitOps-managed ARCs are never deleted automatically; a warning is logged | ||
| // so that operators can clean them up manually. |
There was a problem hiding this comment.
a log would go unnoticed, there needs to be an alerting rule.
4 participants
Detect and remove orphan
AlertRelabelConfig resources that no
longer have a matching PrometheusRule,
preventing stale relabel configs from
accumulating.
Cover orphan deletion and keeper cases
in e2e (live rule, GitOps, unannotated).
Signed-off-by: Shirly Radco sradco@redhat.com
Co-authored-by: AI Assistant noreply@cursor.com
Summary by CodeRabbit
New Features
Bug Fixes