remove extra deep copy of object from cache

[leelavg]

controller-runtime accepts the pointer to the memory which is reserved by application and runtime deepcopies the object in it's cache to the memory provided by application.

this commit removes extra copy of driver spec which isn't required.

fixes: #58

[leelavg]

Testing:

diff --cc internal/controller/driver_controller.go
index 75a82b8,ce77af7..0000000
--- a/internal/controller/driver_controller.go
+++ b/internal/controller/driver_controller.go
@@@ -312,10 -312,18 +312,16 @@@ func (r *driverReconcile) LoadAndValida
        r.images = maps.Clone(imageDefaults)
  
        if opConfig.Spec.DriverSpecDefaults != nil {
--              // Creating a copy of the driver spec, making sure any local changes will not effect the object residing
--              // in the client's cache
-               r.driver.Spec = *r.driver.Spec.DeepCopy()
+               r.log.Info("got from cache before merge", "driver", r.driver.Spec)
                mergeDriverSpecs(&r.driver.Spec, opConfig.Spec.DriverSpecDefaults)
+               r.log.Info("merged from defaults", "driver", r.driver.Spec)
+               driver := &csiv1a1.Driver{}
+               driver.Name = r.driver.Name
+               driver.Namespace = r.driver.Namespace
+               if err := r.Get(r.ctx, client.ObjectKeyFromObject(driver), driver); err != nil {
+                       r.log.Error(err, "failed to get driver", "name", driver.Name)
+               }
+               r.log.Info("got from cache after merge", "driver", driver.Spec)
  
                // If provided, load an imageset from configmap to overwrite default images
                imageSetSpec := opConfig.Spec.DriverSpecDefaults.ImageSet

---
apiVersion: csi.ceph.io/v1alpha1
kind: OperatorConfig
metadata:
  name: ceph-csi-operator-config
spec:
  driverSpecDefaults:
    attachRequired: true
    clusterName: d4ff6939-86ef-4677-9e64-fee0f622e4da
    nodePlugin:
      tolerations:
      - effect: NoSchedule
        key: node-role.kubernetes.io/master
        operator: Exists
---
apiVersion: csi.ceph.io/v1alpha1
kind: Driver
metadata:
  name: rbd.csi.ceph.com
spec:
  attachRequired: false
  clusterName: dummy-id
---

Can be seen in below that cache isn't touched w/o deepcopy, as mentioned in #24 (comment), deepcopy of cached items is enabled by default and disabling that functionality is an opt in.

2024-10-15T07:05:06Z    INFO    got from cache before merge     {"controller": "driver", "controllerGroup": "csi.ceph.io", "controllerKind": "Driver", "Driver": {"name":"rbd.csi.ceph.com","namespace":"csi-op"}, "namespace": "csi-op", "name": "rbd.csi.ceph.com", "reconcileID": "d4400238-4319-4d94-9176-a1b8c7c4fb6a", "driver": {"clusterName":"dummy-id","attachRequired":false}}
2024-10-15T07:05:06Z    INFO    merged from defaults    {"controller": "driver", "controllerGroup": "csi.ceph.io", "controllerKind": "Driver", "Driver": {"name":"rbd.csi.ceph.com","namespace":"csi-op"}, "namespace": "csi-op", "name": "rbd.csi.ceph.com", "reconcileID": "d4400238-4319-4d94-9176-a1b8c7c4fb6a", "driver": {"clusterName":"dummy-id","nodePlugin":{"tolerations":[{"key":"node-role.kubernetes.io/master","operator":"Exists","effect":"NoSchedule"}],"imagePullPolicy":"","resources":{}},"attachRequired":false}}
2024-10-15T07:05:06Z    INFO    got from cache after merge      {"controller": "driver", "controllerGroup": "csi.ceph.io", "controllerKind": "Driver", "Driver": {"name":"rbd.csi.ceph.com","namespace":"csi-op"}, "namespace": "csi-op", "name": "rbd.csi.ceph.com", "reconcileID": "d4400238-4319-4d94-9176-a1b8c7c4fb6a", "driver": {"clusterName":"dummy-id","attachRequired":false}}

[nb-ohad]

@leelavg I am not sure the above test you present tests deep nested pointer structures.
I would ask to have a link to the code inside the controller runtime that implements the deep copy of the nested structures as proof that our deep copy is redundant

[leelavg]

I would ask to have a link to the code inside the controller runtime that implements the deep copy of the structures as proof that our deep copy is redundant

• already presented the link in Implement Driver API reconciliation  #24 (comment), it's https://github.com/kubernetes-sigs/controller-runtime/blob/1ed345090869/pkg/cache/internal/cache_reader.go#L84-L90

[leelavg]

Tested for below config as well, 2 level nested pointer on driver spec

apiVersion: csi.ceph.io/v1alpha1
kind: OperatorConfig
metadata:
  name: ceph-csi-operator-config
spec:
  driverSpecDefaults:
    attachRequired: true
    clusterName: d4ff6939-86ef-4677-9e64-fee0f622e4da
    nodePlugin:
      tolerations:
      - effect: NoSchedule
        key: node-role.kubernetes.io/master
        operator: Exists
---
apiVersion: csi.ceph.io/v1alpha1
kind: Driver
metadata:
  name: rbd.csi.ceph.com
spec:
  attachRequired: false
  clusterName: dummy-id
  log:
    rotation:
      logHostPath: /var/lib/cephcsi
      maxFiles: 7
      maxLogSize: 500M
      periodicity: daily
---

2024-10-15T11:58:03Z    INFO    got from cache before merge     {"controller": "driver", "controllerGroup": "csi.ceph.io", "controllerKind": "Driver", "Driver": {"name":"rbd.csi.ceph.com","namespace":"csi-op"}, "namespace": "csi-op", "name": "rbd.csi.ceph.com", "reconcileID": "3591e675-dde0-4052-8f1f-957352e0291f", "driver": {"log":{"rotation":{"maxFiles":7,"maxLogSize":"500M","periodicity":"daily","logHostPath":"/var/lib/cephcsi"}},"clusterName":"dummy-id","attachRequired":false}}
2024-10-15T11:58:03Z    INFO    merged from operator defaults   {"controller": "driver", "controllerGroup": "csi.ceph.io", "controllerKind": "Driver", "Driver": {"name":"rbd.csi.ceph.com","namespace":"csi-op"}, "namespace": "csi-op", "name": "rbd.csi.ceph.com", "reconcileID": "3591e675-dde0-4052-8f1f-957352e0291f", "driver": {"log":{"rotation":{"maxFiles":7,"maxLogSize":"500M","periodicity":"daily","logHostPath":"/var/lib/cephcsi"}},"clusterName":"dummy-id","nodePlugin":{"tolerations":[{"key":"node-role.kubernetes.io/master","operator":"Exists","effect":"NoSchedule"}],"imagePullPolicy":"","resources":{}},"attachRequired":false}}
2024-10-15T11:58:03Z    INFO    got from cache after merge      {"controller": "driver", "controllerGroup": "csi.ceph.io", "controllerKind": "Driver", "Driver": {"name":"rbd.csi.ceph.com","namespace":"csi-op"}, "namespace": "csi-op", "name": "rbd.csi.ceph.com", "reconcileID": "3591e675-dde0-4052-8f1f-957352e0291f", "driver": {"log":{"rotation":{"maxFiles":7,"maxLogSize":"500M","periodicity":"daily","logHostPath":"/var/lib/cephcsi"}},"clusterName":"dummy-id","attachRequired":false}}

[nb-ohad]

I would ask to have a link to the code inside the controller runtime that implements the deep copy of the structures as proof that our deep copy is redundant

• already presented the link in Implement Driver API reconciliation  #24 (comment), it's https://github.com/kubernetes-sigs/controller-runtime/blob/1ed345090869/pkg/cache/internal/cache_reader.go#L84-L90

@leelavg
According to the code under the link, this is a configurable feature controlled by a disable flag. Can we make sure that the flag is set to false? something like asserting the value/behavior at the start of the reconcile?

[leelavg]

something like asserting the value/behavior at the start of the reconcile?

• what's the expectation? Set the flag to deep copy always or fail if it isn't possible or always deep copy in operator if deepcopy in lib can't be changed?
• we never needed to change it upto now and we always mutate the object returned from cache (basically copied from cache)