OCPBUGS-84872: fix O(n²) metrics collection in Collect() - #101
OCPBUGS-84872: fix O(n²) metrics collection in Collect()#101aswinsuryan wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
jira/severity-moderate
|
@aswinsuryan: This pull request references Jira Issue OCPBUGS-84872, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughIngress metrics collection now caches namespace-qualified ingress management results, performs direct namespace-scoped lookups, and skips only routes with management errors. Stale route metric labels are removed before skipped updates. ChangesIngress metrics collection
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: aswinsuryan 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/route/ingress/metrics.go (1)
89-98: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCache error outcomes for shared owners.
The cache is populated only after
ingressManagedsucceeds. If many routes reference an ingress whose management check errors, each route repeats the ingress lookup and management check. Cache the complete lookup outcome, including errors and not-found status, so each namespace-qualified owner is evaluated once per collection.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/route/ingress/metrics.go` around lines 89 - 98, Update the managedCache flow in the route collection logic to cache the complete ingress lookup and management outcome for each namespace-qualified cacheKey, including errors and not-found results. Ensure shared owners are evaluated only once per collection, while preserving existing error handling and managed-state behavior for subsequent routes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/route/ingress/metrics.go`:
- Around line 91-93: Update the owner lookup in the route handling flow around
ingressManaged to distinguish apierrors.IsNotFound(err) from other
IngressNamespaceLister.Get failures. Treat only NotFound as a missing owner;
handle or propagate other errors so the route is not marked unmanaged and cached
due to the default managed value.
- Around line 94-96: In the ingress metrics collection loop, update the err
branch around ingressManaged to delete the affected route’s labels from
unmanagedRoutes before continuing, preventing stale GaugeVec entries after
collection errors. Extend TestMetrics to cover an errored collect and verify the
route metric is removed.
---
Nitpick comments:
In `@pkg/route/ingress/metrics.go`:
- Around line 89-98: Update the managedCache flow in the route collection logic
to cache the complete ingress lookup and management outcome for each
namespace-qualified cacheKey, including errors and not-found results. Ensure
shared owners are evaluated only once per collection, while preserving existing
error handling and managed-state behavior for subsequent routes.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 521e6564-ded1-4696-8965-7dbaed5dca80
📒 Files selected for processing (1)
pkg/route/ingress/metrics.go
|
/jira refresh |
|
@aswinsuryan: This pull request references Jira Issue OCPBUGS-84872, which is invalid:
Comment 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. |
Replace the nested loop over all ingresses per route with a direct lister lookup using the owner reference name. Add a managedCache to avoid redundant ingressManaged calls for ingresses shared by many routes, reducing complexity from O(routes x ingresses) to O(routes). At 7400 routes x 7500 ingresses this caused the metrics endpoint to exceed the default 10s Prometheus scrape timeout on the leader pod. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
b71f47b to
f6e7f0d
Compare
|
@aswinsuryan: all tests passed! 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. |
|
/assign @rfredette |
|
/jira refresh |
openshift-ci-robot
added
the
jira/valid-bug
|
@aswinsuryan: This pull request references Jira Issue OCPBUGS-84872, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: 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. |
openshift-ci-robot
removed
the
jira/invalid-bug
4 participants
Replace the nested loop over all ingresses per route with a direct lister lookup using the owner reference name. Add a managedCache to avoid redundant ingressManaged calls for ingresses shared by many routes, reducing complexity from O(routes x ingresses) to O(routes).
At 7400 routes x 7500 ingresses this caused the metrics endpoint to exceed the default 10s Prometheus scrape timeout on the leader pod.
Summary by CodeRabbit