Duplicate owner reference in packageserver service

[bryanl]

Bug Report

What did you do?

Installed OLM using kubectl

What did you expect to see?

The service created should have a single owner reference to the CSV.

What did you see instead? Under which circumstances?

The service has multiple owner references to the same CSV.

---
apiVersion: v1
kind: Service
metadata:
  creationTimestamp: "2020-01-15T13:49:15Z"
  name: v1-packages-operators-coreos-com
  namespace: olm
  ownerReferences:
  - apiVersion: operators.coreos.com/v1alpha1
    blockOwnerDeletion: false
    controller: false
    kind: ClusterServiceVersion
    name: packageserver
    uid: e2bdc207-8f58-4ef8-afe4-212704dbe060
  - apiVersion: operators.coreos.com/v1alpha1
    blockOwnerDeletion: false
    controller: false
    kind: ClusterServiceVersion
    name: packageserver
    uid: e2bdc207-8f58-4ef8-afe4-212704dbe060
  resourceVersion: "201176"
  selfLink: /api/v1/namespaces/olm/services/v1-packages-operators-coreos-com
  uid: 7a1f2c82-e2ae-4adb-a10d-dee40c8a76ae
spec:
  clusterIP: 10.96.68.225
  ports:
  - port: 443
    protocol: TCP
    targetPort: 5443
  selector:
    app: packageserver
  sessionAffinity: None
  type: ClusterIP
status:
  loadBalancer: {}

Environment

• operator-lifecycle-manager version:

https://github.com/operator-framework/operator-lifecycle-manager/tree/976f2b981c326ee51748045f6df20317d6d95bd5

• Kubernetes version information:

Client Version: version.Info{Major:"1", Minor:"17", GitVersion:"v1.17.0", GitCommit:"70132b0f130acc0bed193d9ba59dd186f0e634cf", GitTreeState:"clean", BuildDate:"2019-12-07T21:20:10Z", GoVersion:"go1.13.4", Compiler:"gc", Platform:"darwin/amd64"}
Server Version: version.Info{Major:"1", Minor:"17", GitVersion:"v1.17.0", GitCommit:"70132b0f130acc0bed193d9ba59dd186f0e634cf", GitTreeState:"clean", BuildDate:"2019-12-07T21:12:17Z", GoVersion:"go1.13.4", Compiler:"gc", Platform:"linux/amd64"}

• Kubernetes cluster kind:

Minikube

[exdx]

Hey @bryanl, thanks for filing this bug. We think it stems from the way we set owner references without checking the existence of previous references. Will take a look at this one.

[exdx]

Looking into this a bit, I see we do check for existing owner references when assigning a new owner to an object, and return early instead of assigning the same reference again. https://github.com/operator-framework/operator-lifecycle-manager/blob/master/pkg/lib/ownerutil/util.go#L158

That being said this bug shows that the duplicate owernerref is being set again. This suggests it could be a race condition. The call that sets the ownerref for the packageserver service is here

operator-lifecycle-manager/pkg/controller/operators/olm/apiservices.go

Line 366 in 366ed26

ownerutil.AddNonBlockingOwner(service, csv)

. This runs inside of OLM operator and is triggered every time a sync is being done.

One thing I noticed is that the function that actually sets the ownerref has the following signature.

func AddNonBlockingOwner(object metav1.Object, owner Owner)

It takes copies to objects instead of pointers to the objects themselves. Therefore in the case where one sync runs, and calls this function, it sees no references and appends them, and if another sync is running and calls this function with the same object, it will not see the updated references yet and set it again. Then as the sync updates the object on the api server it sets duplicate references. I played around with refactoring the signature to take in pointers to the object and owner args instead, but this causes issues like Cannot use type *Service as type *metav1.Object as the pointer type fulfills a different interface.

All that being said if this was on on minikube then the sync should run really fast, and assuming no errors were present I'm a little doubtful its a race. Any thoughts @ecordell @njhale?

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[timflannagan]

Closing as this appears to have already been resolved but automation didn't close this out immediately. Try using a more up-to-date OLM release that contains the fixes linked in this issue. Feel free to re-open if those fixes don't address completely address your issue.