csi: set ceph cluster as ControllerRef for holder ds

[Madhu-1]

Description of your changes:

Setting the ceph cluster as the ControllerRef for the holder daemonset set so that when a ceph cluster is deleted the holder daemoset will also get deleted.

Which issue is resolved by this Pull Request:
Resolves #12645

Checklist:

• Commit Message Formatting: Commit titles and messages follow guidelines in the developer guide.
• Skip Tests for Docs: If this is only a documentation change, add the label skip-ci on the PR.
• Reviewed the developer guide on Submitting a Pull Request
• Pending release notes updated with breaking and/or notable changes for the next minor release.
• Documentation has been updated, if necessary.
• Unit tests have been added, if necessary.
• Integration tests have been added, if necessary.

[Madhu-1]

as having the ceph cluster as the owner it will not restart the DS.

[travisn]

@BlaineEXE This comment was likely referring to not setting the ownerRef to the operator deployment, right? Setting the cluster CR as the ownerRef makes sense to me though.

[BlaineEXE]

I don't think so. If there are multiple CephClusters, what happens when one of them is deleted? To my logic, the CSI drivers in general shouldn't have any owner set to a CephCluster because they are operator-/cluster-scoped. We don't want to accidentally delete the CSI driver when one of many CephClusters are removed.

[travisn]

Oh of course, the holder daemonset has the same scope as the csi driver, which means it may be serving multiple clusters. So the question is what (if any) resource can the holder set as its owner reference so that it will be cleaned up during uninstall, but only after all cephcluster resources.

[Madhu-1]

@BlaineEXE @travisn These are the special daemonset holder as you know and they get created in the ceph cluster namespace not on the operator namespace like cephcsi plugin daemonset. they are unique per ceph cluster as multiple ceph clusters can be created with different multus networks.

[travisn]

Actually, there is still an issue with this... Even though a separate daemonset is created for each cephcluster, they are still created in the operator namespace. The holder daemonsets are named csi-rbdplugin-holder-<cluster> so they can all be created in the same namespace with the operator. But in this case, the owner references cannot be set on the cephcluster in a different namespace, or else it results in this error:

2023-08-21 16:39:38.454427 E | ceph-csi: failed to reconcile failed to configure ceph csi: failed to start ceph csi drivers: 
failed to configure holder: failed to configure holder "rbd" for "my-cluster"/"other-ceph": 
failed to set owner reference to plugin holder "rook-ceph.rbd.csi.ceph.com": 
cross-namespace owner references are disallowed, owner's namespace other-ceph, obj's namespace rook-ceph

It may be a breaking change at this point, but can the holder daemonsets be created in the same namespace as the cephcluster? That would be necessary for setting the ownerReference like this, except that the CSI driver likely requires the holder pod to be in the same namespace as itself.

[travisn]

For a scoped fix, we could add the ownerReference to the holder daemonset only if is in the same namespace as the operator. This should cover the common scenario anyway.

[Madhu-1]

@travisn Yes you are correct, Thanks for catching it, i don't remember why we choose to create the holder daemonset in the operator namespace other than the cluster namespace, as for the scoped fix am adding a check for it and i think we might need to revisit and check why it's not getting deployed in cluster namespace.

[Madhu-1]

@leseb any idea why we choose to create a holder daemonset set in the operator namespace not in the cluster namespace?

[leseb]

@Madhu-1 Honestly, I don't remember :( Maybe we intended to exercise caution and allow sufficient time for this feature to stabilize before proceeding with a manual deletion? 🤷🏻‍♂️

[travisn]

Actually, there is still an issue with this... Even though a separate daemonset is created for each cephcluster, they are still created in the operator namespace. The holder daemonsets are named csi-rbdplugin-holder-<cluster> so they can all be created in the same namespace with the operator. But in this case, the owner references cannot be set on the cephcluster in a different namespace, or else it results in this error:

2023-08-21 16:39:38.454427 E | ceph-csi: failed to reconcile failed to configure ceph csi: failed to start ceph csi drivers: 
failed to configure holder: failed to configure holder "rbd" for "my-cluster"/"other-ceph": 
failed to set owner reference to plugin holder "rook-ceph.rbd.csi.ceph.com": 
cross-namespace owner references are disallowed, owner's namespace other-ceph, obj's namespace rook-ceph

It may be a breaking change at this point, but can the holder daemonsets be created in the same namespace as the cephcluster? That would be necessary for setting the ownerReference like this, except that the CSI driver likely requires the holder pod to be in the same namespace as itself.