Added comments and TODOs wherever applicable

Author: kolluria

pkg/syncer/cnsoperator/controller/cnsunregistervolume/controller.go

@@ -164,6 +164,8 @@ var (
 	removeFinalizerFromPVC = k8s.RemoveFinalizerFromPVC
 )
 
+// TODO: find a way to optimise the logger usage in this file.
+
 // Reconcile reads that state of the cluster for a Reconciler object
 // and makes changes based on the state read and what is in the
 // Reconciler.Spec.
@@ -194,6 +196,9 @@ func (r *Reconciler) Reconcile(ctx context.Context,
 		log.Info("finished reconciling instance")
 	}()
 
+	// Handle deletion of the instance.
+	// The finalizer will be removed only if the volume is unregistered
+	// or if the input parameters are invalid or if the volume is in use.
 	if instance.DeletionTimestamp != nil {
 		log.Info("instance is marked for deletion")
 		err = r.reconcileDelete(ctx, *instance, request)
@@ -220,12 +225,6 @@ func (r *Reconciler) Reconcile(ctx context.Context,
 		return reconcile.Result{}, nil
 	}
 
-	err = protectInstance(ctx, r.client, instance)
-	if err != nil {
-		log.Error("failed to protect instance with error ", err)
-		return reconcile.Result{}, err
-	}
-
 	err = r.reconcile(ctx, instance, request)
 	if err != nil {
 		log.Error("failed to reconcile with error ", err)
@@ -256,6 +255,14 @@ func (r *Reconciler) reconcile(ctx context.Context,
 		return err
 	}
 
+	// Only protect the instance if input parameters are valid.
+	// This ensures faster deletion of instances with invalid parameters.
+	err = protectInstance(ctx, r.client, instance)
+	if err != nil {
+		log.Error("failed to protect instance with error ", err)
+		return err
+	}
+
 	usageInfo, err := getVolumeUsageInfo(ctx, params.pvcName, params.namespace,
 		instance.Spec.ForceUnregister)
 	if err != nil {
@@ -269,7 +276,7 @@ func (r *Reconciler) reconcile(ctx context.Context,
 		return errors.New(msg)
 	}
 
-	err = unregisterVolume(ctx, *r, request, *params)
+	err = unregisterVolume(ctx, r.volumeManager, request, *params)
 	if err != nil {
 		log.Error("failed to unregister volume with error ", err)
 		return err
@@ -315,7 +322,11 @@ func (r *Reconciler) reconcileDelete(ctx context.Context,
 		return nil
 	}
 
-	err = unregisterVolume(ctx, *r, request, *params)
+	// Try to unregister the volume. This ensures that the system remains
+	// consistent and the volume is not left in an unusable state.
+	// If unregistration fails, the instance will be re-queued for
+	// reconciliation.
+	err = unregisterVolume(ctx, r.volumeManager, request, *params)
 	if err != nil {
 		log.Error("failed to unregister volume with error ", err)
 		return err
@@ -325,7 +336,7 @@ func (r *Reconciler) reconcileDelete(ctx context.Context,
 	return nil
 }
 
-func unregisterVolume(ctx context.Context, r Reconciler,
+func unregisterVolume(ctx context.Context, volMgr volumes.Manager,
 	request reconcile.Request, params params) error {
 	log := logger.GetLogger(ctx).With("name", request.NamespacedName)
 
@@ -355,7 +366,7 @@ func unregisterVolume(ctx context.Context, r Reconciler,
 	}
 
 	unregDisk := !params.retainFCD // If retainFCD is false, unregister the FCD too.
-	err = r.volumeManager.UnregisterVolume(ctx, params.volumeID, unregDisk)
+	err = volMgr.UnregisterVolume(ctx, params.volumeID, unregDisk)
 	if err != nil {
 		log.Error("failed to unregister volume with error ", err)
 		return errors.New("failed to unregister volume")

pkg/syncer/cnsoperator/controller/cnsunregistervolume/controller_test.go

@@ -43,6 +43,8 @@ import (
 	cnsoptypes "sigs.k8s.io/vsphere-csi-driver/v3/pkg/syncer/cnsoperator/types"
 )
 
+// TODO: update tests
+
 func TestReconciler_Reconcile(t *testing.T) {
 	// function overrides
 	newK8sClientOriginal := newK8sClient
@@ -280,7 +282,7 @@ func TestReconciler_Reconcile(t *testing.T) {
 		errMsg := "failed to get volume usage info"
 		instance := newInstance(tt, "mock-instance", "mock-namespace", "mock-volume-id", "", "",
 			nil, false, false, false, false)
-		getVolumeUsageInfo = func(ctx context.Context, k8sClient kubernetes.Interface, pvcName string,
+		getVolumeUsageInfo = func(ctx context.Context, pvcName string,
 			pvcNamespace string, ignoreVMUsage bool) (*volumeUsageInfo, error) {
 			return nil, errors.New(errMsg)
 		}
@@ -306,7 +308,7 @@ func TestReconciler_Reconcile(t *testing.T) {
 		}
 		instance := newInstance(tt, "mock-instance", "mock-namespace", "mock-volume-id", "", "",
 			nil, false, false, false, false)
-		getVolumeUsageInfo = func(ctx context.Context, k8sClient kubernetes.Interface, pvcName string,
+		getVolumeUsageInfo = func(ctx context.Context, pvcName string,
 			pvcNamespace string, ignoreVMUsage bool) (*volumeUsageInfo, error) {
 			return usageInfo, nil
 		}
@@ -323,7 +325,7 @@ func TestReconciler_Reconcile(t *testing.T) {
 		assertUtil(tt, reconciler, request, res, err, true, time.Second, true, "", usageInfo.String(), expStatus)
 	})
 
-	getVolumeUsageInfo = func(ctx context.Context, k8sClient kubernetes.Interface, pvcName string,
+	getVolumeUsageInfo = func(ctx context.Context, pvcName string,
 		pvcNamespace string, ignoreVMUsage bool) (*volumeUsageInfo, error) {
 		return &volumeUsageInfo{}, nil
 	}