OCPBUGS-77042: use NewFromConfig instead of NewFromConfigOrDie in builder - #1426
OCPBUGS-77042: use NewFromConfig instead of NewFromConfigOrDie in builder#1426Cali0707 wants to merge 2 commits into
Conversation
openshift-ci-robot
added
jira/severity-important
|
@Cali0707: This pull request references Jira Issue OCPBUGS-77042, 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. |
|
Warning Review limit reached
Next review available in: 40 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
WalkthroughResource builder factories now return initialization errors. Typed clients use ChangesResource builder initialization
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant newBuilder
participant getOrCreateClientSet
participant newClientSet
participant KubernetesTypedClients
newBuilder->>getOrCreateClientSet: request clientSet for config
getOrCreateClientSet->>newClientSet: create clientSet on cache miss
newClientSet->>KubernetesTypedClients: call NewForConfig
KubernetesTypedClients-->>newClientSet: typed clients or error
newClientSet-->>getOrCreateClientSet: clientSet or error
getOrCreateClientSet-->>newBuilder: cached clientSet or error
🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 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: Cali0707 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.
🧹 Nitpick comments (1)
hack/generate-lib-resources.py (1)
172-177: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFix unused loop control variable.
The loop variable
datais not used in the loop body. Consider replacing it with_to suppress static analysis warnings and clarify intent.♻️ Proposed refactor
- for prop_name, data in sorted(client_properties.items()): + for prop_name, _ in sorted(client_properties.items()): var_name = prop_name[0].lower() + prop_name[1:] lines.append('\t\t{:{width}} {},'.format(prop_name + ':', var_name, width=longest_property+1))🤖 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 `@hack/generate-lib-resources.py` around lines 172 - 177, Replace the unused data variable in the sorted client_properties loop with the conventional underscore placeholder, while preserving prop_name and the generated output unchanged.Source: Linters/SAST tools
🤖 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.
Nitpick comments:
In `@hack/generate-lib-resources.py`:
- Around line 172-177: Replace the unused data variable in the sorted
client_properties loop with the conventional underscore placeholder, while
preserving prop_name and the generated output unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: db0b4910-c64f-4e74-bd27-615336b66875
📒 Files selected for processing (5)
hack/generate-lib-resources.pylib/resourcebuilder/interface.golib/resourcebuilder/resourcebuilder.gopkg/cvo/internal/operatorstatus.gopkg/cvo/sync_test.go
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@Cali0707: This pull request references Jira Issue OCPBUGS-77042, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
No GitHub users were found matching the public email listed for the QA contact in Jira (jiajliu@redhat.com), skipping review request. 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. |
| if err != nil { | ||
| return nil, err | ||
| } | ||
| actual, _ := clientSetCache.LoadOrStore(config, cs) |
There was a problem hiding this comment.
we'll want to clear the map here too, right? To avoid a slow memory leak as the REST config slowly rotates over time?
There was a problem hiding this comment.
yes, just forgot about that initially - will add
There was a problem hiding this comment.
Looking through this, we would not in practice get a memory leak here, as:
- cache is keyed by the pointer, not the value
- There are only two rest.Config pointers, which are only ever created at startup
So in practice this map will only ever have two entries
There was a problem hiding this comment.
- cache is keyed by the pointer, not the value
- There are only two rest.Config pointers, which are only ever created at startup
This makes me a bit nervous. Say the value changes (cred rotation, whatever) but the pointer does not. Will the old client automatically update to use the new creds? Or do we need to create a new client in that situation?
If we are confident we'll only ever have two entries, maybe we can simplify by dropping the map, and instead tracking the two clients with per-client properties? That way it's very clear that there are only two slots, and it will also be very clear if someone makes a future change that adds an additional slot.
There was a problem hiding this comment.
The two rest.Config pointers are created once at startup in pkg/start/start.go via cb.RestConfig(defaultQPS) and cb.RestConfig(highQPS). They are never recreated, the same two pointers are held by the resourceBuilder in pkg/cvo/cvo.go for the lifetime of the process..
Credential rotation is handled transparently by client-go, so we do not need to worry about recreating the rest.Config` or the clients built from it.
That said, I like your suggestion to replace the map with a fixed size array so that the two slot constraint is clear
11c826c to
0419cf7
Compare
|
@Cali0707: This pull request references Jira Issue OCPBUGS-77042, which is valid. 3 validation(s) were run on this bug
No GitHub users were found matching the public email listed for the QA contact in Jira (jiajliu@redhat.com), skipping review request. 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. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
hack/generate-lib-resources.py (1)
164-199: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winCodegen root cause for unbounded
clientSetCache(see consolidated comment).This template emits the unbounded, pointer-keyed
clientSetCachewith no eviction — same concern flagged on the generatedlib/resourcebuilder/resourcebuilder.go. See consolidated comment for the shared fix.🤖 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 `@hack/generate-lib-resources.py` around lines 164 - 199, Update the generation template around getOrCreateClientSet and clientSetCache to stop emitting an unbounded pointer-keyed sync.Map. Apply the same bounded cache strategy required for the generated resourcebuilder client cache, including eviction and safe reuse of client sets, while preserving newClientSet error handling and client creation behavior.
🤖 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 `@lib/resourcebuilder/resourcebuilder.go`:
- Around line 135-147: Update getOrCreateClientSet and clientSetCache to prevent
unbounded retention of client sets when new *rest.Config instances replace older
ones. Implement bounded or insertion-order eviction while preserving
concurrently live distinct configurations; do not clear all entries on every
cache miss unless the surrounding usage guarantees only one live configuration
at a time.
---
Outside diff comments:
In `@hack/generate-lib-resources.py`:
- Around line 164-199: Update the generation template around
getOrCreateClientSet and clientSetCache to stop emitting an unbounded
pointer-keyed sync.Map. Apply the same bounded cache strategy required for the
generated resourcebuilder client cache, including eviction and safe reuse of
client sets, while preserving newClientSet error handling and client creation
behavior.
🪄 Autofix (Beta)
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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: aa68bc00-8436-422c-89cf-2268e8fd0eae
📒 Files selected for processing (2)
hack/generate-lib-resources.pylib/resourcebuilder/resourcebuilder.go
The generated newBuilder function previously used NewForConfigOrDie for every typed client, which would panic if any client construction failed. Switch to NewForConfig so errors propagate instead. On failure, getOrCreateClientSet returns the error without caching, so the next reconciliation attempt retries client creation from scratch. The exponential backoff in Task.Run handles transient failures automatically. If retries exhaust, the error surfaces as a sync failure on the ClusterVersion status rather than crashing the process. Signed-off-by: Calum Murray <cmurray@redhat.com>
0419cf7 to
b7bc9ad
Compare
|
/retest-required |
|
/retest |
1 similar comment
|
/retest |
b7bc9ad to
f184e09
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@hack/generate-lib-resources.py`:
- Around line 202-221: Add an early nil-config guard to the generated
getOrCreateClientSet template in hack/generate-lib-resources.py, returning the
specified error before cache lookup; then regenerate
lib/resourcebuilder/resourcebuilder.go so its generated getOrCreateClientSet
implementation matches. Do not hand-edit lib/resourcebuilder/resourcebuilder.go;
the sibling site is corrected by regeneration.
- Around line 178-183: Update the loop over sorted client_properties in the
resource-generation code to use an intentionally ignored variable instead of the
unused data binding, while preserving prop_name, var_name, and the generated
output unchanged.
- Around line 223-236: Update the sorted client_properties loop in the
resource-generation code to avoid binding the unused data value, while
preserving prop_name iteration and generated builder field output.
🪄 Autofix (Beta)
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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 9c6c015b-505d-45e9-89e5-3884bf5dbf8c
📒 Files selected for processing (5)
hack/generate-lib-resources.pylib/resourcebuilder/interface.golib/resourcebuilder/resourcebuilder.gopkg/cvo/internal/operatorstatus.gopkg/cvo/sync_test.go
The CVO creates exactly two rest.Config pointers at startup (default-QPS and burst-QPS). Cache the client sets built from each using a fixed-size [2]clientSetSlot array so the two-slot constraint is explicit in the type system. A third config will produce a clear error rather than silently growing. Signed-off-by: Calum Murray <cmurray@redhat.com>
f184e09 to
6b0ac61
Compare
|
@Cali0707: 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. |
4 participants
This should fix the bug we are seeing with panics when there is a missing CA cert temporarily. By moving from
NewFromConfigOrDietoNewFromConfigwe remove the possibility for a panic and instead this will enter an exponential backoff retry before surfacing as a status errorSummary by CodeRabbit