Enhancement - Introduce pvcName input param in CNSUnregisterVolume spec

Adds validation rules using x-kubernetes-validations to ensure that only one of the volumeID or pvcName is specified to avoid ambiguity.
Updates the reconciler to use the informer cache to find out the PV and VolumeID when the PVC Name is supplied.
Optimises the reconciler logic to ignore all such events that do not increment the generation.
Optimises the usage calculation logic by failing fast when usages are detected.
Updates the reconciler to add a finalizer to the CR before reconciling to have control over deletion process
Updates the reconciler to process the delete events and remove the finalizer for graceful deletion.
Updates the reconciler to protect the PVC using a finalizer to gracefully reconciler in case of retries
Removes the code that retains the persistent volume as it's no longer required
Updates the reconciler to remove the finalizer on the PVC once unregistration is successful for graceful deletion of the PVC.
Adds/updates unit tests wherever necessary and applicable.

Author: kolluria

pkg/apis/cnsoperator/cnsunregistervolume/v1alpha1/cnsunregistervolume_types.go

@@ -24,7 +24,10 @@ import (
 // +k8s:openapi-gen=true
 type CnsUnregisterVolumeSpec struct {
 	// VolumeID indicates the volume handle of CNS volume to be unregistered
-	VolumeID string `json:"volumeID"`
+	VolumeID string `json:"volumeID,omitempty"`
+
+	// PVCName indicates the name of the PVC to be unregistered.
+	PVCName string `json:"pvcName,omitempty"`
 
 	// RetainFCD indicates if the volume should be retained as an FCD.
 	// If set to false or not specified, the volume will be retained as a VMDK.

pkg/apis/cnsoperator/config/cnsunregistervolume_crd.yaml

@@ -1,4 +1,3 @@
-
 ---
 apiVersion: apiextensions.k8s.io/v1
 kind: CustomResourceDefinition
@@ -16,66 +15,73 @@ spec:
     singular: cnsunregistervolume
   scope: Namespaced
   versions:
-  - name: v1alpha1
-    schema:
-      openAPIV3Schema:
-        description: CnsUnregisterVolume is the Schema for the cnsunregistervolumes API
-        properties:
-          apiVersion:
-            description: 'APIVersion defines the versioned schema of this representation
+    - name: v1alpha1
+      schema:
+        openAPIV3Schema:
+          description: CnsUnregisterVolume is the Schema for the cnsunregistervolumes API
+          properties:
+            apiVersion:
+              description: 'APIVersion defines the versioned schema of this representation
               of an object. Servers should convert recognized schemas to the latest
               internal value, and may reject unrecognized values. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources'
-            type: string
-          kind:
-            description: 'Kind is a string value representing the REST resource this
+              type: string
+            kind:
+              description: 'Kind is a string value representing the REST resource this
               object represents. Servers may infer this from the endpoint the client
               submits requests to. Cannot be updated. In CamelCase. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds'
-            type: string
-          metadata:
-            type: object
-          spec:
-            description: CnsUnregisterVolumeSpec defines the desired state of CnsUnregisterVolume
-            properties:
-              volumeID:
-                description: VolumeID indicates the volume handle of CNS volume to be unregistered.
-                type: string
-                pattern: '^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$'
-              retainFCD:
-                description: RetainFCD indicates if the volume should be retained as an FCD. 
-                  If set to false or not specified, the volume will be retained as a VMDK.
-                type: boolean
-              forceUnregister:
-                description: ForceUnregister indicates if the volume should be forcefully unregistered.
-                  If set to true, the volume will be unregistered even if it is still in use by any VM.
-                  This should be used with caution as it may lead to data loss.
-                type: boolean
-            required:
-            - volumeID
-            type: object
-          status:
-            description: CnsUnregisterVolumeStatus defines the observed state of CnsUnregisterVolume
-            properties:
-              error:
-                description: The last error encountered during export operation, if
-                  any. This field must only be set by the entity completing the export
-                  operation, i.e. the CNS Operator.
-                type: string
-              unregistered:
-                description: Indicates the volume is successfully unregistered. 
-                  This field must only be set by the entity completing the unregister 
-                  operation, i.e. the CNS Operator.
-                type: boolean
-            required:
-            - unregistered
-            type: object
-        type: object
-    served: true
-    storage: true
-    subresources:
-      status: {}
+              type: string
+            metadata:
+              type: object
+            spec:
+              description: CnsUnregisterVolumeSpec defines the desired state of CnsUnregisterVolume
+              properties:
+                volumeID:
+                  description: VolumeID indicates the volume handle of CNS volume to be unregistered.
+                  type: string
+                  pattern: '^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$'
+                pvcName:
+                  description: PVCName indicates the name of the PVC to be unregistered.
+                  type: string
+                  pattern: '^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$'
+                retainFCD:
+                  description: RetainFCD indicates if the volume should be retained as an FCD.
+                    If set to false or not specified, the volume will be retained as a VMDK.
+                  type: boolean
+                forceUnregister:
+                  description: ForceUnregister indicates if the volume should be forcefully unregistered.
+                    If set to true, the volume will be unregistered even if it is still in use by any VM.
+                    This should be used with caution as it may lead to data loss.
+                  type: boolean
+              type: object
+              x-kubernetes-validations:
+                - rule: "has(self.volumeID) || has(self.pvcName)"
+                  message: "Either 'volumeID' or 'pvcName' must be specified, but not both"
+                - rule: "!(has(self.volumeID) && has(self.pvcName))"
+                  message: "Cannot specify both 'volumeID' and 'pvcName' at the same time. Please specify only one of them"
+            status:
+              description: CnsUnregisterVolumeStatus defines the observed state of CnsUnregisterVolume
+              properties:
+                error:
+                  description: The last error encountered during export operation, if
+                    any. This field must only be set by the entity completing the export
+                    operation, i.e. the CNS Operator.
+                  type: string
+                unregistered:
+                  description: Indicates the volume is successfully unregistered.
+                    This field must only be set by the entity completing the unregister
+                    operation, i.e. the CNS Operator.
+                  type: boolean
+              required:
+                - unregistered
+              type: object
+          type: object
+      served: true
+      storage: true
+      subresources:
+        status: { }
 status:
   acceptedNames:
     kind: ""
     plural: ""
-  conditions: []
-  storedVersions: []
+  conditions: [ ]
+  storedVersions: [ ]

pkg/common/cns-lib/volume/manager.go

@@ -3766,6 +3766,11 @@ func (m *defaultManager) UnregisterVolume(ctx context.Context, volumeID string,
 func (m *defaultManager) unregisterVolume(ctx context.Context, volumeID string, unregisterDisk bool) error {
 	log := logger.GetLogger(ctx)
 
+	if volumeID == "" {
+		log.Debugf("UnregisterVolume: volumeID is empty, nothing to unregister")
+		return nil
+	}
+
 	if m.virtualCenter == nil {
 		return errors.New("invalid manager instance")
 	}

pkg/common/unittestcommon/utils.go

@@ -431,7 +431,7 @@ func (c *FakeK8SOrchestrator) GetPVCNameFromCSIVolumeID(volumeID string) (string
 }
 
 // GetVolumeIDFromPVCName simlates an invalid case when pvcName contains "invalid".
-func (c *FakeK8SOrchestrator) GetVolumeIDFromPVCName(pvcName string) (string, bool) {
+func (c *FakeK8SOrchestrator) GetVolumeIDFromPVCName(namespace string, pvcName string) (string, bool) {
 	if strings.Contains(pvcName, "invalid") {
 		// Simulate a case where the volumeID is invalid and does not correspond to any PVC.
 		return "", false

pkg/csi/service/common/commonco/coagnostic.go

@@ -98,9 +98,8 @@ type COCommonInterface interface {
 	GetPVNameFromCSIVolumeID(volumeID string) (string, bool)
 	// GetPVCNameFromCSIVolumeID returns `pvc name` and `pvc namespace` for the given volumeID using volumeIDToPvcMap.
 	GetPVCNameFromCSIVolumeID(volumeID string) (string, string, bool)
-	// GetVolumeIDFromPVCName returns volumeID the given pvcName using pvcToVolumeIDMap.
-	// PVC name is its namespaced name.
-	GetVolumeIDFromPVCName(pvcName string) (string, bool)
+	// GetVolumeIDFromPVCName returns volumeID for the given pvc name and namespace.
+	GetVolumeIDFromPVCName(namespace string, pvcName string) (string, bool)
 	// InitializeCSINodes creates CSINode instances for each K8s node with the appropriate topology keys.
 	InitializeCSINodes(ctx context.Context) error
 	// StartZonesInformer starts a dynamic informer which listens on Zones CR in

pkg/csi/service/common/commonco/k8sorchestrator/k8sorchestrator.go

@@ -2016,8 +2016,8 @@ func (c *K8sOrchestrator) GetPVCNameFromCSIVolumeID(volumeID string) (
 
 // GetVolumeIDFromPVCName returns volumeID the given pvcName using pvcToVolumeIDMap.
 // PVC name is its namespaced name.
-func (c *K8sOrchestrator) GetVolumeIDFromPVCName(pvcName string) (string, bool) {
-	return c.pvcToVolumeIDMap.get(pvcName)
+func (c *K8sOrchestrator) GetVolumeIDFromPVCName(namespace string, pvcName string) (string, bool) {
+	return c.pvcToVolumeIDMap.get(fmt.Sprintf("%s/%s", namespace, pvcName))
 }
 
 // IsLinkedCloneRequest checks if the pvc is a linked clone request

pkg/kubernetes/kubernetes.go

@@ -58,6 +58,7 @@ import (
 	snapshotterClientSet "github.com/kubernetes-csi/external-snapshotter/client/v8/clientset/versioned"
 	storagev1 "k8s.io/api/storage/v1"
 
+	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 	cnsoperatorv1alpha1 "sigs.k8s.io/vsphere-csi-driver/v3/pkg/apis/cnsoperator"
 	migrationv1alpha1 "sigs.k8s.io/vsphere-csi-driver/v3/pkg/apis/migration/v1alpha1"
 	storagepoolAPIs "sigs.k8s.io/vsphere-csi-driver/v3/pkg/apis/storagepool"
@@ -712,100 +713,142 @@ func PatchFinalizers(ctx context.Context, c client.Client, obj client.Object, fi
 	return c.Patch(ctx, obj, patch)
 }
 
-// RetainPersistentVolume updates the PersistentVolume's ReclaimPolicy to Retain.
-// This is useful to preserve the PersistentVolume even if the associated PersistentVolumeClaim is deleted.
-func RetainPersistentVolume(ctx context.Context, k8sClient clientset.Interface, pvName string) error {
-	log := logger.GetLogger(ctx)
+// DeletePersistentVolumeClaim deletes the PersistentVolumeClaim with the given name and namespace.
+func DeletePersistentVolumeClaim(ctx context.Context, k8sClient clientset.Interface,
+	pvcName, pvcNamespace string) error {
+	log := logger.GetLogger(ctx).With("name", fmt.Sprintf("%s/%s", pvcNamespace, pvcName))
 
-	if pvName == "" {
-		log.Debugf("PersistentVolume name is empty. Exiting...")
+	if pvcName == "" {
+		log.Debug("PVC name is empty. Exiting...")
 		return nil
 	}
 
-	log.Debugf("Retaining PersistentVolume %q", pvName)
-	pv, err := k8sClient.CoreV1().PersistentVolumes().Get(ctx, pvName, metav1.GetOptions{})
+	log.Debug("Deleting PVC")
+	err := k8sClient.CoreV1().PersistentVolumeClaims(pvcNamespace).Delete(ctx, pvcName, metav1.DeleteOptions{})
 	if err != nil {
 		if apierrors.IsNotFound(err) {
-			log.Debugf("PersistentVolume %q not found. Exiting...", pvName)
+			log.Debug("PVC not found. Exiting...")
 			return nil
 		}
 
-		return logger.LogNewErrorf(log, "Failed to get PersistentVolume %q. Error: %s", pvName, err.Error())
+		return logger.LogNewErrorf(log, "Failed to delete PVC. Error: %s", err.Error())
+	}
+
+	log.Debug("Successfully deleted PVC")
+	return nil
+}
+
+// DeletePersistentVolume deletes the PersistentVolume with the given name.
+func DeletePersistentVolume(ctx context.Context, k8sClient clientset.Interface, pvName string) error {
+	log := logger.GetLogger(ctx).With("name", pvName)
+
+	if pvName == "" {
+		log.Debug("PersistentVolume name is empty. Exiting...")
+		return nil
 	}
 
-	pv.Spec.PersistentVolumeReclaimPolicy = v1.PersistentVolumeReclaimRetain
-	_, err = k8sClient.CoreV1().PersistentVolumes().Update(ctx, pv, metav1.UpdateOptions{})
+	log.Debug("Deleting PV")
+	err := k8sClient.CoreV1().PersistentVolumes().Delete(ctx, pvName, metav1.DeleteOptions{})
 	if err != nil {
-		return logger.LogNewErrorf(log, "Failed to update PersistentVolume %q to retain policy. Error: %s",
-			pvName, err.Error())
+		if apierrors.IsNotFound(err) {
+			log.Debug("PV not found. Exiting...")
+			return nil
+		}
+
+		return logger.LogNewErrorf(log, "Failed to delete PV. Error: %s", err.Error())
 	}
 
-	log.Debugf("Successfully retained PersistentVolume %q", pvName)
+	log.Debug("Successfully deleted PV")
 	return nil
 }
 
-// DeletePersistentVolumeClaim deletes the PersistentVolumeClaim with the given name and namespace.
-func DeletePersistentVolumeClaim(ctx context.Context, k8sClient clientset.Interface,
-	pvcName, pvcNamespace string) error {
-	log := logger.GetLogger(ctx)
+// UpdateStatus updates the status subresource of the given Kubernetes object.
+// If the object is a Custom Resource, make sure that the `subresources` field in the
+// CustomResourceDefinition includes `status` to enable status subresource updates.
+func UpdateStatus(ctx context.Context, c client.Client, obj client.Object) error {
+	log := logger.GetLogger(ctx).With(
+		"kind", obj.GetObjectKind().GroupVersionKind().Kind,
+		"name", fmt.Sprintf("%s/%s", obj.GetNamespace(), obj.GetName()),
+	)
+	if err := c.Status().Update(ctx, obj); err != nil {
+		log.Errorf("Failed to update status. Error: %s", err)
+		return err
+	}
+
+	log.Info("Successfully updated status")
+	return nil
+}
+
+// AddFinalizerOnPVC adds the specified finalizer to the PersistentVolumeClaim (PVC)
+// if it is not already present.
+func AddFinalizerOnPVC(ctx context.Context, k8sClient clientset.Interface, pvcName, pvcNamespace,
+	finalizer string) error {
+	log := logger.GetLogger(ctx).With("name", fmt.Sprintf("%s/%s", pvcNamespace, pvcName))
 
 	if pvcName == "" {
-		log.Debugf("PVC name is empty. Exiting...")
+		log.Debug("PVC name is empty. Exiting...")
 		return nil
 	}
 
-	log.Debugf("Deleting PersistentVolumeClaim %q in namespace %q", pvcName, pvcNamespace)
-	err := k8sClient.CoreV1().PersistentVolumeClaims(pvcNamespace).Delete(ctx, pvcName, metav1.DeleteOptions{})
+	pvc, err := k8sClient.CoreV1().PersistentVolumeClaims(pvcNamespace).Get(ctx, pvcName, metav1.GetOptions{})
 	if err != nil {
 		if apierrors.IsNotFound(err) {
-			log.Debugf("PersistentVolumeClaim %q in namespace %q not found. Exiting...", pvcName, pvcNamespace)
+			log.Debug("PVC not found. Exiting...", pvcName, pvcNamespace)
 			return nil
 		}
 
-		return logger.LogNewErrorf(log, "Failed to delete PersistentVolumeClaim %q in namespace %q. Error: %s",
-			pvcName, pvcNamespace, err.Error())
+		return logger.LogNewErrorf(log, "Failed to get PVC. Error: %s", err.Error())
+	}
+
+	// If the finalizer is already present, no action is needed
+	if !controllerutil.AddFinalizer(pvc, finalizer) {
+		log.Debugf("Finalizer %s already present on PVC. No action needed.", finalizer)
+		return nil
+	}
+
+	// Update the PVC with the new finalizer
+	_, err = k8sClient.CoreV1().PersistentVolumeClaims(pvcNamespace).Update(ctx, pvc, metav1.UpdateOptions{})
+	if err != nil {
+		return logger.LogNewErrorf(log, "Failed to add finalizer %s to PVC. Error: %s", finalizer, err.Error())
 	}
 
-	log.Debugf("Successfully deleted PersistentVolumeClaim %q in namespace %q", pvcName, pvcNamespace)
+	log.Debugf("Successfully added finalizer %s to PVC", finalizer)
 	return nil
 }
 
-// DeletePersistentVolume deletes the PersistentVolume with the given name.
-func DeletePersistentVolume(ctx context.Context, k8sClient clientset.Interface, pvName string) error {
-	log := logger.GetLogger(ctx)
+// RemoveFinalizerFromPVC removes the specified finalizer from the PersistentVolumeClaim (PVC)
+// if it is present.
+func RemoveFinalizerFromPVC(ctx context.Context, k8sClient clientset.Interface, pvcName, pvcNamespace,
+	finalizer string) error {
+	log := logger.GetLogger(ctx).With("name", fmt.Sprintf("%s/%s", pvcNamespace, pvcName))
 
-	if pvName == "" {
-		log.Debugf("PersistentVolume name is empty. Exiting...")
+	if pvcName == "" {
+		log.Debug("PVC name is empty. Exiting...")
 		return nil
 	}
 
-	log.Debugf("Deleting PersistentVolume %q", pvName)
-	err := k8sClient.CoreV1().PersistentVolumes().Delete(ctx, pvName, metav1.DeleteOptions{})
+	pvc, err := k8sClient.CoreV1().PersistentVolumeClaims(pvcNamespace).Get(ctx, pvcName, metav1.GetOptions{})
 	if err != nil {
 		if apierrors.IsNotFound(err) {
-			log.Debugf("PersistentVolume %q not found. Exiting...", pvName)
+			log.Debug("PVC not found. Exiting...", pvcName, pvcNamespace)
 			return nil
 		}
 
-		return logger.LogNewErrorf(log, "Failed to delete PersistentVolume %q. Error: %s", pvName, err.Error())
+		return logger.LogNewErrorf(log, "Failed to get PVC. Error: %s", err.Error())
 	}
 
-	log.Debugf("Successfully deleted PersistentVolume %q", pvName)
-	return nil
-}
+	// If the finalizer is not present, no action is needed
+	if !controllerutil.RemoveFinalizer(pvc, finalizer) {
+		log.Debugf("Finalizer %s not present on PVC. No action needed.", finalizer)
+		return nil
+	}
 
-// UpdateStatus updates the status subresource of the given Kubernetes object.
-// If the object is a Custom Resource, make sure that the `subresources` field in the
-// CustomResourceDefinition includes `status` to enable status subresource updates.
-func UpdateStatus(ctx context.Context, c client.Client, obj client.Object) error {
-	log := logger.GetLogger(ctx)
-	if err := c.Status().Update(ctx, obj); err != nil {
-		log.Errorf("Failed to update status for %s %s/%s: %v", obj.GetObjectKind().GroupVersionKind().Kind,
-			obj.GetNamespace(), obj.GetName(), err)
-		return err
+	// Update the PVC to remove the finalizer
+	_, err = k8sClient.CoreV1().PersistentVolumeClaims(pvcNamespace).Update(ctx, pvc, metav1.UpdateOptions{})
+	if err != nil {
+		return logger.LogNewErrorf(log, "Failed to remove finalizer %s from PVC. Error: %s", finalizer, err.Error())
 	}
 
-	log.Infof("Successfully updated status for %s %s/%s", obj.GetObjectKind().GroupVersionKind().Kind,
-		obj.GetNamespace(), obj.GetName())
+	log.Debugf("Successfully removed finalizer %s from PVC", finalizer)
 	return nil
 }

pkg/syncer/admissionhandler/cnscsi_admissionhandler_test.go

@@ -188,7 +188,7 @@ func (m *MockCOCommonInterface) GetPVCNameFromCSIVolumeID(volumeID string) (stri
 	return args.String(0), args.String(1), args.Bool(2)
 }
 
-func (m *MockCOCommonInterface) GetVolumeIDFromPVCName(pvcName string) (string, bool) {
+func (m *MockCOCommonInterface) GetVolumeIDFromPVCName(namespace, pvcName string) (string, bool) {
 	args := m.Called(pvcName)
 	return args.String(0), args.Bool(1)
 }