Enhancement - Introduce pvcName input param in CNSUnregisterVolume

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)
 }