OCPBUGS-65687: fix(controlplane-component): prevent informer creation for unused platform resources - #7417
OCPBUGS-65687: fix(controlplane-component): prevent informer creation for unused platform resources#7417muraee wants to merge 1 commit into
Conversation
…tform resources When components use .WithPredicate() (e.g., azure-cloud-controller-manager), non-matching platforms trigger cleanup mode which calls Client.Get() to check for existing resources before deletion. For platform-specific resource types like SecretProviderClass, this causes controller-runtime to create informers that continuously LIST/WATCH resources that will never exist on non-Azure platforms. Track whether resources have been successfully applied and skip delete operations entirely for resources that were never created, avoiding the Client.Get() call that triggers unnecessary informer creation.
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
openshift-ci-robot
added
the
jira/severity-critical
|
Skipping CI for Draft Pull Request. |
openshift-ci-robot
added
jira/valid-reference
|
@muraee: This pull request references Jira Issue OCPBUGS-65687, 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. |
WalkthroughBoth control plane components now track an internal Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: muraee The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
approved
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@muraee: This pull request references Jira Issue OCPBUGS-65687, which is valid. 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. |
|
/test unit |
|
@muraee: you cannot LGTM your own PR. 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 kubernetes-sigs/prow repository. |
openshift-ci-robot
added
the
verified
|
@muraee: This PR has been marked as verified by 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
Bot
removed
the
do-not-merge/work-in-progress
|
@muraee: This pull request references Jira Issue OCPBUGS-65687, which is valid. 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. |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
support/controlplane-component/generic-adapter.go (1)
16-23:⚠️ Potential issue | 🔴 CriticalFix lost state mutation: store
genericAdapterpointers or write back to map afterreconcile().
genericAdapteris stored by value inmanifestsAdaptersmap. Whenadapter.reconcile()is called (line 269 of controlplane-component.go), the pointer receiver implicitly takes the address of the temporary copy retrieved from the map. Mutations likega.hasBeenApplied = true(line 96 of generic-adapter.go) are written to that temporary and discarded; the map entry is never updated. On the next reconcile,hasBeenAppliedremains false, causing the cleanup guard at line 54 to never trigger correctly.Recommended fix: either store
*genericAdapterin the map, or reassign the adapter back to the map after reconcile completes.Example fix (write-back approach)
adapter, exist := c.manifestsAdapters[manifestName] if exist { - return adapter.reconcile(cpContext, obj) + if err := adapter.reconcile(cpContext, obj); err != nil { + return err + } + c.manifestsAdapters[manifestName] = adapter + return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@support/controlplane-component/generic-adapter.go` around lines 16 - 23, The map manifestsAdapters stores genericAdapter values so pointer-receiver mutations inside genericAdapter.reconcile() (which sets genericAdapter.hasBeenApplied) are lost; fix by either changing the map to store *genericAdapter pointers or by writing the mutated adapter back into manifestsAdapters after calling reconcile(). Locate the manifestsAdapters map usage (where reconcile() is invoked) and implement one of: 1) change its value type to *genericAdapter and update creation sites to take addresses, or 2) after calling adapter.reconcile(...), assign the possibly-modified adapter back into manifestsAdapters (e.g., manifestsAdapters[key] = adapter) so hasBeenApplied mutations persist.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@support/controlplane-component/controlplane-component.go`:
- Around line 197-201: The delete method on controlPlaneWorkload[T] currently
early-returns when the in-memory flag hasBeenApplied is false, which prevents
cleanup after process restarts; change the logic in
controlPlaneWorkload[T].delete to derive persisted applied state (e.g., read
hyperv1.ControlPlaneComponent status or annotation via the
ControlPlaneContext/k8s client) or rehydrate hasBeenApplied at startup so delete
does not rely solely on the volatile hasBeenApplied boolean; specifically, add a
lookup (using ControlPlaneContext) for the corresponding
hyperv1.ControlPlaneComponent status/annotation to decide whether resources
exist and only skip deletion when that persisted marker indicates not applied,
and update any rehydration initialization path so hasBeenApplied reflects
persisted state.
- Around line 176-180: The code sets c.hasBeenApplied = true unconditionally
after calling c.update(cpContext), which can mark the component applied even if
c.update returned an error; change this so reconcilationError =
c.update(cpContext) is evaluated first and only set c.hasBeenApplied = true when
reconcilationError == nil (i.e., update succeeded), leaving the flag unchanged
on error; locate the block that checks unavailableDependencies and adjust the
order/guard around c.update, c.hasBeenApplied and reconcilationError
accordingly.
---
Outside diff comments:
In `@support/controlplane-component/generic-adapter.go`:
- Around line 16-23: The map manifestsAdapters stores genericAdapter values so
pointer-receiver mutations inside genericAdapter.reconcile() (which sets
genericAdapter.hasBeenApplied) are lost; fix by either changing the map to store
*genericAdapter pointers or by writing the mutated adapter back into
manifestsAdapters after calling reconcile(). Locate the manifestsAdapters map
usage (where reconcile() is invoked) and implement one of: 1) change its value
type to *genericAdapter and update creation sites to take addresses, or 2) after
calling adapter.reconcile(...), assign the possibly-modified adapter back into
manifestsAdapters (e.g., manifestsAdapters[key] = adapter) so hasBeenApplied
mutations persist.
ℹ️ Review info
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (2)
support/controlplane-component/controlplane-component.gosupport/controlplane-component/generic-adapter.go
| if len(unavailableDependencies) == 0 { | ||
| // reconcile only when all dependencies are available, and don't return error immediately so it can be included in the status condition first. | ||
| reconcilationError = c.update(cpContext) | ||
| c.hasBeenApplied = true | ||
| } |
There was a problem hiding this comment.
Set hasBeenApplied only after a successful update.
c.hasBeenApplied = true runs even when c.update(...) returns an error. That can mark components as applied even when nothing was created, undermining the “skip delete if never applied” guard. Gate this on reconcilationError == nil.
✅ Suggested fix
- reconcilationError = c.update(cpContext)
- c.hasBeenApplied = true
+ reconcilationError = c.update(cpContext)
+ if reconcilationError == nil {
+ c.hasBeenApplied = true
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if len(unavailableDependencies) == 0 { | |
| // reconcile only when all dependencies are available, and don't return error immediately so it can be included in the status condition first. | |
| reconcilationError = c.update(cpContext) | |
| c.hasBeenApplied = true | |
| } | |
| if len(unavailableDependencies) == 0 { | |
| // reconcile only when all dependencies are available, and don't return error immediately so it can be included in the status condition first. | |
| reconcilationError = c.update(cpContext) | |
| if reconcilationError == nil { | |
| c.hasBeenApplied = true | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@support/controlplane-component/controlplane-component.go` around lines 176 -
180, The code sets c.hasBeenApplied = true unconditionally after calling
c.update(cpContext), which can mark the component applied even if c.update
returned an error; change this so reconcilationError = c.update(cpContext) is
evaluated first and only set c.hasBeenApplied = true when reconcilationError ==
nil (i.e., update succeeded), leaving the flag unchanged on error; locate the
block that checks unavailableDependencies and adjust the order/guard around
c.update, c.hasBeenApplied and reconcilationError accordingly.
| func (c *controlPlaneWorkload[T]) delete(cpContext ControlPlaneContext) error { | ||
| if !c.hasBeenApplied { | ||
| // if the component has not been applied, it doesn't exist, so there's nothing to delete. | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Guard can skip cleanup after operator restart.
hasBeenApplied is in-memory only. After a restart it resets to false, so if the component is disabled at startup, delete becomes a no-op even when resources exist from prior runs. Consider persisting applied state (e.g., via hyperv1.ControlPlaneComponent status/annotation) or rehydrating it once per process to allow cleanup after restarts.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@support/controlplane-component/controlplane-component.go` around lines 197 -
201, The delete method on controlPlaneWorkload[T] currently early-returns when
the in-memory flag hasBeenApplied is false, which prevents cleanup after process
restarts; change the logic in controlPlaneWorkload[T].delete to derive persisted
applied state (e.g., read hyperv1.ControlPlaneComponent status or annotation via
the ControlPlaneContext/k8s client) or rehydrate hasBeenApplied at startup so
delete does not rely solely on the volatile hasBeenApplied boolean;
specifically, add a lookup (using ControlPlaneContext) for the corresponding
hyperv1.ControlPlaneComponent status/annotation to decide whether resources
exist and only skip deletion when that persisted marker indicates not applied,
and update any rehydration initialization path so hasBeenApplied reflects
persisted state.
|
@muraee: 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. |
|
superseded by #7819 |
|
@muraee: This pull request references Jira Issue OCPBUGS-65687. The bug has been updated to no longer 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. |
2 participants
What this PR does / why we need it:
When components use .WithPredicate() (e.g., azure-cloud-controller-manager), non-matching platforms trigger cleanup mode which calls Client.Get() to check for existing resources before deletion. For platform-specific resource types like SecretProviderClass, this causes controller-runtime to create informers that continuously LIST/WATCH resources that will never exist on non-Azure platforms.
Track whether resources have been successfully applied and skip delete operations entirely for resources that were never created, avoiding the Client.Get() call that triggers unnecessary informer creation.
Which issue(s) this PR fixes:
Fixes
Special notes for your reviewer:
Checklist:
Summary by CodeRabbit