OCPBUGS-39539: clear rogue controller ownerReferences during merge - #1445
OCPBUGS-39539: clear rogue controller ownerReferences during merge#1445savio87 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/valid-reference
|
@savio87: This pull request references Jira Issue OCPBUGS-39539, 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 YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
WalkthroughThe owner-reference merge now clears conflicting existing controller flags when a required controller reference is added. Tests cover both conflicting controller references and preserved non-controller references. ChangesOwner reference reconciliation
Estimated code review effort: 2 (Simple) | ~10 minutes Mergeability Score: ⚪ Minimal · up to The change is localized to owner-reference merging and adds focused unit coverage; no actionable merge-blocking risk remains beyond normal checks and review. 🚥 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 |
openshift-ci-robot
added
jira/valid-bug
|
@savio87: This pull request references Jira Issue OCPBUGS-39539, 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. |
openshift-ci
Bot
added
the
needs-ok-to-test
|
Hi @savio87. Thanks for your PR. I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: savio87 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 |
|
This clears rogue controller ownerReferences that cause CVO to wedge with "only one reference can have Controller set to true" during reconciliation. The rogue ref is preserved — only its controller claim is demoted. Could a maintainer check and run /ok-to-test? Thanks! |
|
Gentle ping @PratikMahajan @hongkailiu , CodeRabbit had no actionable comments. This fixes a bug where CVO wedges on CRDs with rogue controller ownerReferences. Would appreciate /ok-to-test when you get a chance. Thanks! |
|
|
||
| // If a required ref claims Controller=true, clear Controller on any | ||
| // existing refs not in the required set to avoid the API server | ||
| // rejecting the update with "only one reference can have Controller |
There was a problem hiding this comment.
Expanding on my 2024 context, this function is old, dating all the way back to 2018, #7, d9f6718. That commit message doesn't go into detail about why the CVO chose to merge itself into ownerReferences instead of compeltely owning ownerReferences in any resource it managed. But it means that now we're in the sticky position of wondering if we can own more of ownerReferences or not. Why does the old controller: true entry exist? Is it safe to clear controller on those entries? Maybe... something.. should be removing those entries? It's hard to know.
For the bug's original clusterserviceversions.operators.coreos.com with Found "true" in references for ClusterServiceVersion/rhsso-operator.7.6.9-opr-002 and ClusterVersion/version, the issue seems to be the RHSSO operator, right? Should it own that CRD? I'd expect the answer is "no, it does not belong in that CRD's ownerReferences at all, and there should be a bug against the RHSSO operator to get it to remove that entry from the CRD's ownerReferences".
Or maybe we want to tighten the CVO's original weak stance up, and say "look, we're a powerful operator, and if we think that resource is ours, we're erasing your attempt at being in ownerReferences entirely". In that case, #7's original weaker stance was an error.
But keeping ownerRerferences entries we don't understand and just clearing controller on them seems like it's sitting in the middle, and that feels messy to me, and I don't see an upside to sitting on the fence. Both "we own ownerReferences on our resources entirely" and "we complain when there's a conflict, so the cluster-admin can dig in and report a bug against whoever falsely thought they were the controller" make sense to me.
There was a problem hiding this comment.
From the ControllerRef proposal:
The main goal of ControllerRef (controller reference) is to solve the problem of controllers that fight over controlled objects due to overlapping selectors (e.g. a ReplicaSet fighting with a ReplicationController over Pods because both controllers have label selectors that match those Pods). Fighting controllers can destabilize the apiserver, thrash objects back-and-forth, or cause controller operations to hang.
If I understand correctly, in the mentioned bug, the CVO de facto respects the API: once another operator is set as controller, the CVO stops managing the resource. It's de facto because the CVO doesn't check the controllerRef and backs off deliberately - it re-adds its own Controller: true ownerReference on reconcile, the API rejects the update ("only one reference can have Controller set to true"), and the CVO simply can't proceed. It never stomps the rogue ownerReference to force its way through; it just stops and surfaces the failure.
The CVO was initially set as the controller owner of the object and was removed by the rogue operator. Clearing out any such rogue operators as controllers may not stop them from clearing the CVO out again and continuing to fight in an endless loop, which can "destabilize the apiserver, thrash objects back-and-forth, or cause controller operations to hang".
This is an RHSSO operator issue.
However, the bug itself is about what we should do in such cases. Is bricking the CVO the desired outcome? Probably not. However, not respecting the API also does not sound like the ideal approach. Currently, everything works "as expected":
- The CVO is set as an owner and the controller
- Another operator replaces the CVO as the controller and kicks out the CVO as an owner - poor little CVO
- The CVO lost its rights and can't continue to manage the resource. This is propagated by:
The user-facing ClusterVersion conditions:
- lastTransitionTime: "2025-11-18T04:52:21Z"
message: 'Could not update customresourcedefinition "clusterserviceversions.operators.coreos.com"
(649 of 901): the object is invalid, possibly due to local cluster configuration'
reason: UpdatePayloadResourceInvalid
status: "True"
type: Failing
- lastTransitionTime: "2025-06-09T13:07:08Z"
message: 'Error while reconciling 4.18.13: some cluster configuration is invalid'
reason: UpdatePayloadResourceInvalid
status: "False"
type: ProgressingThe CVO logs include more information such as:
2024-09-03T14:11:19.672587865Z I0903 14:11:19.672572 1 sync_worker.go:1171] Update error 649 of 903: UpdatePayloadResourceInvalid Could not update customresourcedefinition "clusterserviceversions.operators.coreos.com" (649 of 903): the object is invalid, possibly due to local cluster configuration (*errors.StatusError: CustomResourceDefinition.apiextensions.k8s.io "clusterserviceversions.operators.coreos.com" is invalid: metadata.ownerReferences: Invalid value: []v1.OwnerReference{v1.OwnerReference{APIVersion:"config.openshift.io/v1", Kind:"ClusterServiceVersion", Name:"rhsso-operator.7.6.9-opr-002", UID:"00f0a902-a305-40bd-b277-2de22dca78ba", Controller:(*bool)(0xc1014fb039), BlockOwnerDeletion:(*bool)(nil)}, v1.OwnerReference{APIVersion:"config.openshift.io/v1", Kind:"ClusterVersion", Name:"version", UID:"6412f9f6-7ecf-4bfc-8277-813c9a4ef48d", Controller:(*bool)(0xc1014fb03a), BlockOwnerDeletion:(*bool)(nil)}}: Only one reference can have Controller set to true. Found "true" in references for ClusterServiceVersion/rhsso-operator.7.6.9-opr-002 and ClusterVersion/version)
We may stomp the rogue operator's changes to proceed, but most likely it won't stop them. A rogue operator may overwrite the CRD on startup or continuously. If continuously, it will only make it harder to detect the introduced situation while introducing a hidden regression in performance and potentially in functionality due to the operators continuously overwriting their changes.
From the proposal:
The Three Laws of Controllers
All controllers that manage collections of objects should obey the following
rules.
Take ownership
A controller should claim ownership of any objects it creates by adding a
ControllerRef, and may also claim ownership of an object it didn't create,
as long as the object has no existing ControllerRef (i.e. it is an orphan).Don't interfere
A controller should not take any action (e.g. edit/scale/delete) on an object
it does not own, except to adopt the object if allowed by the
First Law.Don't share
A controller should not count an object it does not own toward satisfying its
desired state (e.g. a certain number of replicas), although it may include
the object in plans to achieve its desired state (e.g. through adoption)
as long as such plans do not conflict with the First or Second Laws.
The ControllerRef API has its purpose.
The CVO gets attributed the bug because it is simply vocal about the existing issue caused by a local cluster configuration and respects the API to not cause issues.
Removing the controllerRef from a rogue operator, who wants to forcibly manage the object, may introduce issues for that operator of an unknown impact. The CVO fighting with a rogue controller may introduce other issues. Leaving things as-is introduces a blocked CVO as the issue. A serious issue; however, it is communicated to the cluster administrator.
I am inclined towards the "we complain when there's a conflict, so the cluster-admin can dig in and report a bug against whoever falsely thought they were the controller" approach, provided we give actionable enough advice to the cluster administrators where to look next. Maybe Could not update customresourcedefinition "clusterserviceversions.operators.coreos.com" (649 of 901): the object is invalid, possibly due to local cluster configuration is not strong enough?
Alternatively, we may yield being the controller when someone forces us out in a live-cluster and adopt the object when it becomes an orphan; however, that smells like a Pandora's box.
There was a problem hiding this comment.
Thanks @wking and @DavidHurta for the thorough review and the historical context, really appreciate it.
I agree that the current approach (keeping the rogue ref but clearing its controller flag) sits in an awkward middle ground. After reading your feedback, I see three possible directions:
Improve the error message - Keep the current CVO behavior (fail when there's a conflict), but make the error message more actionable so cluster-admins know exactly which operator is the rogue controller and can file a bug against it. This aligns with the "don't interfere" principle from the ControllerRef proposal.
CVO fully owns ownerReferences - When CVO manages a resource, it takes complete ownership of ownerReferences and removes rogue entries entirely rather than trying to merge them.
Close this PR - The root cause belongs in the rogue operator (RHSSO in this case) and this isn't something CVO should work around.
Which direction would you prefer? Happy to update the PR accordingly.
4 participants
Summary
Fixes OCPBUGS-39539: CVO wedges with
Failing=Truewhen reconciling a CRD that has a rogueownerReferencewithcontroller: trueset by another operator (e.g. OLM).The Kubernetes API rejects updates when two ownerReferences both claim
controller: true. ThemergeOwnerRefsfunction was additive-only — it never cleared conflicting controller flags on existing refs, so the rogue ref persisted alongside CVO's own controller ref, causing the update to fail indefinitely.This fix adds a post-merge pass: when a required ownerReference has
Controller=true, any existing ownerReference (not in the required set) that also hasController=truegets itsControllerfield cleared. The rogue reference itself is preserved — only its controller claim is demoted.Test plan
Controller=trueref is cleared when CVO's required ref claims controllerControllerare left untouchedTestMergeOwnerRefstests passSummary by CodeRabbit