Skip to content

OCPBUGS-39539: clear rogue controller ownerReferences during merge #1445

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

Open
savio87 wants to merge 1 commit into
base: main
Choose a base branch
from
+65 −0
Open

OCPBUGS-39539: clear rogue controller ownerReferences during merge #1445

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
19 changes: 19 additions & 0 deletions lib/resourcemerge/meta.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,4 +73,23 @@ func mergeOwnerRefs(modified *bool, existing *[]metav1.OwnerReference, required
*existing = append(*existing, required[ridx])
}
}

// 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

@wking wking Aug 19, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

@DavidHurta DavidHurta Aug 19, 2026 •
edited
Loading

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.

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":

  1. The CVO is set as an owner and the controller
  2. Another operator replaces the CVO as the controller and kicks out the CVO as an owner - poor little CVO
  3. 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: Progressing

The 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.

  1. 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).

  2. 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.

  3. 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.

@savio87 savio87 Aug 20, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

@DavidHurta DavidHurta Aug 21, 2026 •
edited
Loading

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.

I am personally open to improving the error message or closing this PR. Both options work for me.

With respect to closing the PR.

My view may be very biased as someone who is working with the CVO; thus, an outside perspective is especially very desired in such areas.

I agree that the possibly due to local cluster configuration leaves a lot of things implied for the cluster administrator, and I am sure there may be more such messages in the CVO code-base.

Checking the CVO logs is the next best step when troubleshooting any such issues, but that may not be apparent, and I do not blame anyone for that.

One option would be to introduce the improved error propagation. I am happy to review such PRs, but their priority and severity would be somewhat lower at this time, IMO.

// set to true".
for ridx := range required {
if required[ridx].Controller == nil || !*required[ridx].Controller {
continue
}
for eidx := range *existing {
if (*existing)[eidx].UID == required[ridx].UID {
continue
}
if (*existing)[eidx].Controller != nil && *(*existing)[eidx].Controller {
*modified = true
(*existing)[eidx].Controller = nil
}
}
}
}
46 changes: 46 additions & 0 deletions lib/resourcemerge/meta_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,52 @@ func TestMergeOwnerRefs(t *testing.T) {
Controller: ptr.To(true),
UID: types.UID("uid-1"),
}},
}, {
// Rogue controller ref on existing should be cleared when
// a required ref claims Controller=true (OCPBUGS-39539).
existing: []metav1.OwnerReference{{
Kind: "ClusterServiceVersion",
Name: "rogue-operator.1.0.0",
Controller: ptr.To(true),
UID: types.UID("uid-rogue"),
}},
input: []metav1.OwnerReference{{
Kind: "ClusterVersion",
Name: "version",
Controller: ptr.To(true),
UID: types.UID("uid-cv"),
}},

expectedModified: true,
expected: []metav1.OwnerReference{{
Kind: "ClusterServiceVersion",
Name: "rogue-operator.1.0.0",
UID: types.UID("uid-rogue"),
}, {
Kind: "ClusterVersion",
Name: "version",
Controller: ptr.To(true),
UID: types.UID("uid-cv"),
}},
}, {
// Non-controller existing ref should not be modified.
existing: []metav1.OwnerReference{{
Kind: "SomeOther",
UID: types.UID("uid-other"),
}},
input: []metav1.OwnerReference{{
Controller: ptr.To(true),
UID: types.UID("uid-cv"),
}},

expectedModified: true,
expected: []metav1.OwnerReference{{
Kind: "SomeOther",
UID: types.UID("uid-other"),
}, {
Controller: ptr.To(true),
UID: types.UID("uid-cv"),
}},
}}

for idx, test := range tests {
Expand Down