From 7c8203ca21cfaaee8af27743a19e7e3a66924e2f Mon Sep 17 00:00:00 2001 From: nb-ohad Date: Sun, 21 Jul 2024 23:18:32 +0300 Subject: [PATCH] Config Controller: Add config deletion and cleanup logic Signed-off-by: nb-ohad --- internal/controller/config_controller.go | 100 ++++++++++++++++------- internal/utils/meta.go | 23 ++++++ 2 files changed, 95 insertions(+), 28 deletions(-) diff --git a/internal/controller/config_controller.go b/internal/controller/config_controller.go index 8527936f..8cbc4a68 100644 --- a/internal/controller/config_controller.go +++ b/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 diff --git a/internal/utils/meta.go b/internal/utils/meta.go index 20ffebfa..3d546459 100644 --- a/internal/utils/meta.go +++ b/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 +}