Config Controller: Add config deletion and cleanup logic

Signed-off-by: nb-ohad <[email protected]>

internal/controller/config_controller.go

@@ -48,6 +48,7 @@ type ConfigReconcile struct {
 	log         logr.Logger
 	config      csiv1a1.Config
 	cephCluster csiv1a1.CephCluster
+	cleanUp     bool
 }
 
 // csiClusterRrcordInfo represent the structure of a serialized csi record
@@ -69,6 +70,10 @@ type csiClusterInfoRecord struct {
 	} `json:"readAffinity,omitempty"`
 }
 
+const (
+	cleanupFinalizer = "csi.ceph.com/cleanup"
+)
+
 var configMapUpdateLock = sync.Mutex{}
 
 //+kubebuilder:rbac:groups=csi.ceph.io,resources=configs,verbs=get;list;watch;create;update;patch;delete
@@ -122,23 +127,29 @@ func (r *ConfigReconcile) reconcile() error {
 		return err
 	}
 
-	// Ensure the ceph cluster resource has an owner reference (not controller reference)
-	// for the current reconciled config resource
-	if !utils.IsOwnedBy(&r.cephCluster, &r.config) {
-		if err := ctrlutil.SetOwnerReference(&r.config, &r.cephCluster, r.Scheme); err != nil {
-			r.log.Error(err, "Failed adding an owner reference on ceph cluster resource")
-			return err
-		}
-		if err := r.Update(r.ctx, &r.cephCluster); err != nil {
-			r.log.Error(err, "Failed to update ceph cluster resource")
+	// Ensure a finalizer on the config resource to allow proper clean up
+	if ctrlutil.AddFinalizer(&r.config, cleanupFinalizer) {
+		if err := r.Update(r.ctx, &r.config); err != nil {
+			r.log.Error(err, "Failed to add a cleanup finalizer on config resource")
 			return err
 		}
 	}
 
+	if err := r.reconcileCephCluster(); err != nil {
+		return err
+	}
 	if err := r.reconcileCephCsiClusterInfo(); err != nil {
 		return err
 	}
 
+	if r.cleanUp {
+		ctrlutil.RemoveFinalizer(&r.config, cleanupFinalizer)
+		if err := r.Update(r.ctx, &r.config); err != nil {
+			r.log.Error(err, "Failed to add a cleanup finalizer on config resource")
+			return err
+		}
+	}
+
 	return nil
 }
 
@@ -148,6 +159,7 @@ func (r *ConfigReconcile) loadAndValidate() error {
 		r.log.Error(err, "Unable to load configs.csi.ceph.io resource")
 		return err
 	}
+	r.cleanUp = r.config.DeletionTimestamp != nil
 
 	// Validate a pointer to a ceph cluster resource
 	if r.config.Spec.CephClusterRef.Name == "" {
@@ -167,6 +179,28 @@ func (r *ConfigReconcile) loadAndValidate() error {
 	return nil
 }
 
+func (r *ConfigReconcile) reconcileCephCluster() error {
+	log := r.log.WithValues("cephClusterName", r.cephCluster.Name)
+	log.Info("Reconciling Ceph Clsuter")
+
+	if needsUpdate, err := utils.ToggleOwnerReference(
+		!r.cleanUp,
+		&r.config,
+		&r.cephCluster,
+		r.Scheme,
+	); err != nil {
+		r.log.Error(err, "Failed to toggle owner reference on ceph cluster resource")
+		return err
+	} else if needsUpdate {
+		if err := r.Update(r.ctx, &r.cephCluster); err != nil {
+			r.log.Error(err, "Failed to update ceph cluster resource")
+			return err
+		}
+	}
+
+	return nil
+}
+
 func (r *ConfigReconcile) reconcileCephCsiClusterInfo() error {
 	csiConfigMap := corev1.ConfigMap{}
 	csiConfigMap.Name = utils.CsiConfigVolume.Name
@@ -175,22 +209,23 @@ func (r *ConfigReconcile) reconcileCephCsiClusterInfo() error {
 	log := r.log.WithValues("csiConfigMapName", csiConfigMap.Name)
 	log.Info("Reconciling Ceph CSI Cluster Info")
 
-	// Creting the desired record in advance to miimize the amount execution time
-	// the code run while serializing access to the configmap
-	record := composeCsiClusterInfoRecord(&r.config, &r.cephCluster)
-
 	// Using a lock to serialized the updating of the config map.
 	// Although the code will run perfetcly fine without the lock, there will be a higher
 	// chance to fail on the create/update operation because another concurrent reconcile loop
 	// updated the config map which will result in stale representation and an update failure.
-	// The locking stategy will sync all update to the shared config file and will prevent such
+	// The locking strategy will sync all update to the shared config file and will prevent such
 	// potential issues without a big impact on preformace as a whole
 	configMapUpdateLock.Lock()
 	defer configMapUpdateLock.Unlock()
 
 	_, err := ctrlutil.CreateOrUpdate(r.ctx, r.Client, &csiConfigMap, func() error {
-		if err := ctrlutil.SetOwnerReference(&r.config, &csiConfigMap, r.Scheme); err != nil {
-			log.Error(err, "Failed setting an owner reference on Ceph CSI config map")
+		if _, err := utils.ToggleOwnerReference(
+			!r.cleanUp,
+			&r.config,
+			&csiConfigMap,
+			r.Scheme,
+		); err != nil {
+			log.Error(err, "Failed togggling owner reference for Ceph CSI config map")
 			return err
 		}
 
@@ -205,29 +240,38 @@ func (r *ConfigReconcile) reconcileCephCsiClusterInfo() error {
 			}
 		}
 
-		// locate an existing entry for the same id if exists
+		// Locate an existing entry for the same config/cluster name if exists
 		index := slices.IndexFunc(clusterInfoList, func(record *csiClusterInfoRecord) bool {
 			return record.ClusterId == r.config.Name
 		})
 
-		// overwrite an existing entry or append a new one
-		if index > -1 {
-			clusterInfoList[index] = record
-		} else {
-			clusterInfoList = append(clusterInfoList, record)
+		if !r.cleanUp {
+			// Overwrite an existing entry or append a new one
+			record := composeCsiClusterInfoRecord(&r.config, &r.cephCluster)
+			if index > -1 {
+				clusterInfoList[index] = record
+			} else {
+				clusterInfoList = append(clusterInfoList, record)
+			}
+		} else if index > -1 {
+			// An O(1) unordered in-place delete of a record
+			// Will not shrink the capacity of the slice
+			length := len(clusterInfoList)
+			clusterInfoList[index] = clusterInfoList[length-1]
+			clusterInfoList = clusterInfoList[:length-1]
 		}
 
-		// serialize the list ba
-		if bytes, err := json.Marshal(clusterInfoList); err == nil {
+		// Serialize the list and store it back into the config map
+		if bytes, err := json.Marshal(clusterInfoList); err != nil {
+			log.Error(err, "Failed to serialize cluster info list")
+			return err
+		} else {
 			if csiConfigMap.Data == nil {
 				csiConfigMap.Data = map[string]string{}
 			}
 			csiConfigMap.Data["config.json"] = string(bytes)
-			return nil
-		} else {
-			log.Error(err, "Failed to serialize cluster info list")
-			return err
 		}
+		return nil
 	})
 
 	return err

internal/utils/meta.go

@@ -18,6 +18,8 @@ package utils
 
 import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime"
+	ctrlutil "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 )
 
 // AddAnnotation adds an annotation to a resource metadata, returns true if added else false
@@ -44,3 +46,24 @@ func IsOwnedBy(obj, owner metav1.Object) bool {
 	}
 	return false
 }
+
+// ToggleOwnerReference adds or remove an owner reference for the given owner based on the first argument.
+// The function return true if the owner reference list had changed and false it it didn't
+func ToggleOwnerReference(on bool, obj, owner metav1.Object, scheme *runtime.Scheme) (bool, error) {
+	ownerRefExists := IsOwnedBy(obj, owner)
+
+	if on {
+		if !ownerRefExists {
+			if err := ctrlutil.SetOwnerReference(obj, owner, scheme); err != nil {
+				return false, err
+			}
+			return true, nil
+		}
+	} else if ownerRefExists {
+		if err := ctrlutil.RemoveOwnerReference(obj, owner, scheme); err != nil {
+			return false, err
+		}
+		return true, nil
+	}
+	return false, nil
+}