feat: Manage prometheus resources#2117
Conversation
|
Skipping CI for Draft Pull Request. |
Signed-off-by: Anatolii Bazko <abazko@redhat.com>
|
/retest |
| Rules: []rbacv1.PolicyRule{ | ||
| { | ||
| APIGroups: []string{""}, | ||
| Resources: []string{"services", "endpoints", "pods"}, |
There was a problem hiding this comment.
Sorry, I might not have the full context. Why is pods needed?
There was a problem hiding this comment.
I not 100% sure, probably some requirements
https://eclipse.dev/che/docs/stable/administration-guide/monitoring-che/
|
Hi! I'm che-ai-assistant — I help with your pull requests. Available commands:
|
|
While testing it I observe that ServiceMonitor for On installing Eclipse Che based on these changes and creating a DevWorkspace.
These get garbage collected when How to enable Che equivalent resources? I can confirm metrics is enabled in |
Signed-off-by: Anatolii Bazko <abazko@redhat.com>
|
@rohanKanojia
|
|
/che-ai-assistant generate-che-doc Created documentation PR: eclipse-che/che-docs#3106 |
|
/che-ai-assistant ok-pr-review |
tolusha
left a comment
There was a problem hiding this comment.
Review Summary
This PR adds a well-designed metrics reconciler with clean abstractions and good test coverage. The PrometheusResourceProvider interface pattern is well-motivated, and the interval preservation mechanism shows operational awareness.
Key findings:
- 7 inline comments with specific suggestions
- Most are minor improvements (documentation, test coverage, deprecated package usage)
- No blocking issues found
The reconciler design is solid and follows established patterns in the codebase. Great work on the upgrade path with deleteAbandonedResources and graceful degradation for clusters without Prometheus Operator.
|
|
||
| func NewMetricsReconciler() *MetricsReconciler { | ||
| return &MetricsReconciler{} | ||
| } |
There was a problem hiding this comment.
Package-level isAbandonedResourcesDeleted isn't reset between tests. TestReconcileMetrics sets it true, so later tests skip deletion code. Consider a reset function in init_test.go or make this per-DeployContext.
|
I tested it again and now I can confirm both ServiceMonitors are available: |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: rohanKanojia, tolusha 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 |
What does this PR do?
Screenshot/screencast of this PR
N/A
What issues does this PR fix or reference?
https://redhat.atlassian.net/browse/CRW-8629
https://redhat.atlassian.net/browse/CRW-8589
https://redhat.atlassian.net/browse/CRW-8315
How to test this PR?
https://eclipse.dev/che/docs/stable/administration-guide/monitoring-che/
https://eclipse.dev/che/docs/stable/administration-guide/monitoring-the-dev-workspace-operator/
Common Test Scenarios
PR Checklist
As the author of this Pull Request I made sure that:
Reviewers
Reviewers, please comment how you tested the PR when approving it.