Skip to content

OCPBUGS-65687: fix(controlplane-component): prevent informer creation for unused platform resources #7417

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
muraee wants to merge 1 commit into from
Closed

OCPBUGS-65687: fix(controlplane-component): prevent informer creation for unused platform resources #7417

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 15 additions & 2 deletions support/controlplane-component/controlplane-component.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,9 @@ type controlPlaneWorkload[T client.Object] struct {

customOperandsRolloutCheck func(cpContext WorkloadContext) (bool, error)
monitorOperandsRolloutStatus bool

// tracks whether the component has been applied successfully.
hasBeenApplied bool
}

// Name implements ControlPlaneComponent.
Expand Down Expand Up @@ -173,6 +176,7 @@ func (c *controlPlaneWorkload[T]) Reconcile(cpContext ControlPlaneContext) error
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
}
Comment on lines 176 to 180

@coderabbitai coderabbitai Bot Feb 23, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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.

Suggested change
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.


component := &hyperv1.ControlPlaneComponent{
Expand All @@ -191,6 +195,11 @@ func (c *controlPlaneWorkload[T]) Reconcile(cpContext ControlPlaneContext) error
}

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
}
Comment on lines 197 to +201

@coderabbitai coderabbitai Bot Feb 23, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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.


workloadObj := c.workloadProvider.NewObject()
// make sure that the Deployment/Statefulset name matches the component name.
workloadObj.SetName(c.Name())
Expand Down Expand Up @@ -221,8 +230,12 @@ func (c *controlPlaneWorkload[T]) delete(cpContext ControlPlaneContext) error {
Namespace: cpContext.HCP.Namespace,
},
}
_, err = util.DeleteIfNeeded(cpContext, cpContext.Client, component)
return err
if _, err := util.DeleteIfNeeded(cpContext, cpContext.Client, component); err != nil {
return err
}

c.hasBeenApplied = false
return nil
}

// update reconciles component workload and related manifests
Expand Down
11 changes: 11 additions & 0 deletions support/controlplane-component/generic-adapter.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,9 @@ type genericAdapter struct {
adapt func(cpContext WorkloadContext, resource client.Object) error
predicate Predicate
reconcileExisting bool // if true, causes the existing resource to be fetched before adapting

// tracks whether the resource has been applied successfully.
hasBeenApplied bool
}

type option func(*genericAdapter)
Expand Down Expand Up @@ -48,6 +51,11 @@ func (ga *genericAdapter) reconcile(cpContext ControlPlaneContext, obj client.Ob
workloadContext := cpContext.workloadContext()

if ga.predicate != nil && !ga.predicate(workloadContext) {
if !ga.hasBeenApplied {
// if the resource has not been applied, it doesn't exist, so there's nothing to delete.
return nil
}

// get the existing object to read its ownerRefs
existing := obj.DeepCopyObject().(client.Object)
err := cpContext.Client.Get(cpContext, client.ObjectKeyFromObject(obj), existing)
Expand All @@ -64,6 +72,8 @@ func (ga *genericAdapter) reconcile(cpContext ControlPlaneContext, obj client.Ob
} else if !apierrors.IsNotFound(err) && !meta.IsNoMatchError(err) {
return err
}

ga.hasBeenApplied = false
return nil
}

Expand All @@ -83,5 +93,6 @@ func (ga *genericAdapter) reconcile(cpContext ControlPlaneContext, obj client.Ob
return err
}

ga.hasBeenApplied = true
return nil
}