Enhancement - Introduce pvcName input param in CNSUnregisterVolume (#3658)

* 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.
Updates the enablement logic to use WCP capability supports_mobility_non_disruptive_import
Removes cns-unregister-volume fss from csi-feature-states configmap

* Feature enablement related changes

* Updated reconciler to handle CNS Faults in both delete and normal reconcile workflows

* Updated manager.UnregisterVolume to return fault along with the error

* Updated tests with the new expectations

* Refactorings and comments

Author: kolluria

manifests/supervisorcluster/1.29/cns-csi.yaml

@@ -582,7 +582,6 @@ data:
   "cnsmgr-suspend-create-volume": "true"
   "storage-quota-m2": "true"
   "vdpp-on-stretched-supervisor": "true"
-  "cns-unregister-volume": "false"
   "workload-domain-isolation": "false"
   "sv-pvc-snapshot-protection-finalizer": "true"
 kind: ConfigMap

manifests/supervisorcluster/1.30/cns-csi.yaml

@@ -584,7 +584,6 @@ data:
   "list-volumes": "true"
   "cnsmgr-suspend-create-volume": "true"
   "storage-quota-m2": "true"
-  "cns-unregister-volume": "false"
   "workload-domain-isolation": "false"
   "sv-pvc-snapshot-protection-finalizer": "true"
 kind: ConfigMap

manifests/supervisorcluster/1.31/cns-csi.yaml

@@ -584,7 +584,6 @@ data:
   "list-volumes": "true"
   "cnsmgr-suspend-create-volume": "true"
   "storage-quota-m2": "true"
-  "cns-unregister-volume": "false"
   "workload-domain-isolation": "false"
   "sv-pvc-snapshot-protection-finalizer": "true"
 kind: ConfigMap

manifests/supervisorcluster/1.32/cns-csi.yaml

@@ -584,7 +584,6 @@ data:
   "list-volumes": "true"
   "cnsmgr-suspend-create-volume": "true"
   "storage-quota-m2": "true"
-  "cns-unregister-volume": "false"
   "workload-domain-isolation": "false"
   "sv-pvc-snapshot-protection-finalizer": "true"
 kind: ConfigMap

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,80 @@ 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
+      additionalPrinterColumns:
+        - jsonPath: .status.unregistered
+          name: Unregistered
+          type: boolean
+        - jsonPath: .metadata.creationTimestamp
+          name: Age
+          type: date
+      subresources:
+        status: { }
 status:
   acceptedNames:
     kind: ""
     plural: ""
-  conditions: []
-  storedVersions: []
+  conditions: [ ]
+  storedVersions: [ ]

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

@@ -157,7 +157,7 @@ type Manager interface {
 		vm *cnsvsphere.VirtualMachine, batchAttachRequest []BatchAttachRequest) ([]BatchAttachResult, string, error)
 	// UnregisterVolume unregisters a volume from CNS.
 	// If unregisterDisk is true, it will also unregister the disk from FCD.
-	UnregisterVolume(ctx context.Context, volumeID string, unregisterDisk bool) error
+	UnregisterVolume(ctx context.Context, volumeID string, unregisterDisk bool) (string, error)
 	// SyncVolume returns the aggregated capacity for volumes
 	SyncVolume(ctx context.Context, syncVolumeSpecs []cnstypes.CnsSyncVolumeSpec) (string, error)
 }
@@ -3734,32 +3734,32 @@ func (m *defaultManager) SetListViewNotReady(ctx context.Context) {
 
 // UnregisterVolume unregisters a volume from CNS.
 // If unregisterDisk is true, it will also unregister the disk from FCD.
-func (m *defaultManager) UnregisterVolume(ctx context.Context, volumeID string, unregisterDisk bool) error {
+func (m *defaultManager) UnregisterVolume(ctx context.Context, volumeID string, unregisterDisk bool) (string, error) {
 	ctx, cancelFunc := ensureOperationContextHasATimeout(ctx)
 	defer cancelFunc()
 	start := time.Now()
-	err := m.unregisterVolume(ctx, volumeID, unregisterDisk)
+	faultType, err := m.unregisterVolume(ctx, volumeID, unregisterDisk)
 	if err != nil {
 		prometheus.CnsControlOpsHistVec.WithLabelValues(prometheus.PrometheusUnregisterVolumeOpType,
 			prometheus.PrometheusFailStatus).Observe(time.Since(start).Seconds())
-		return err
+		return faultType, err
 	}
 
 	prometheus.CnsControlOpsHistVec.WithLabelValues(prometheus.PrometheusUnregisterVolumeOpType,
 		prometheus.PrometheusPassStatus).Observe(time.Since(start).Seconds())
-	return nil
+	return "", nil
 }
 
-func (m *defaultManager) unregisterVolume(ctx context.Context, volumeID string, unregisterDisk bool) error {
-	log := logger.GetLogger(ctx)
+func (m *defaultManager) unregisterVolume(ctx context.Context, volumeID string, unregisterDisk bool) (string, error) {
+	log := logger.GetLogger(ctx).With("volumeID", volumeID, "unregisterDisk", unregisterDisk)
 
 	if m.virtualCenter == nil {
-		return errors.New("invalid manager instance")
+		return "", errors.New("virtual Center connection not established")
 	}
 
 	err := m.virtualCenter.ConnectCns(ctx)
 	if err != nil {
-		return errors.New("connecting to CNS failed")
+		return ExtractFaultTypeFromErr(ctx, err), errors.New("connecting to CNS failed")
 	}
 
 	targetVolumeType := "FCD"
@@ -3774,50 +3774,50 @@ func (m *defaultManager) unregisterVolume(ctx context.Context, volumeID string,
 	}
 	task, err := m.virtualCenter.CnsClient.UnregisterVolume(ctx, spec)
 	if err != nil {
-		msg := "failed to invoke UnregisterVolume API"
-		log.Errorf("%s from vCenter %q with err: %v", msg, m.virtualCenter.Config.Host, err)
-		return errors.New(msg)
+		log.Errorf("CNS UnregisterVolume failed from vCenter %q with err: %v",
+			m.virtualCenter.Config.Host, err)
+		return ExtractFaultTypeFromErr(ctx, err), err
 	}
 
 	taskInfo, err := m.waitOnTask(ctx, task.Reference())
 	if err != nil {
-		msg := "failed to get UnregisterVolume taskInfo"
-		log.Errorf("%s from vCenter %q with err: %v",
-			msg, m.virtualCenter.Config.Host, err)
-		return errors.New(msg)
+		log.Errorf("failed to wait for UnregisterVolume task completion from vCenter %q with err: %v",
+			m.virtualCenter.Config.Host, err)
+		return ExtractFaultTypeFromErr(ctx, err), err
 	}
 
 	if taskInfo == nil {
-		msg := "taskInfo is empty for UnregisterVolume task"
-		log.Errorf("%s from vCenter %q. task: %q",
+		msg := "taskInfo is nil for UnregisterVolume task"
+		log.Errorf("%s from vCenter %q. taskID: %q",
 			msg, m.virtualCenter.Config.Host, task.Reference().Value)
-		return errors.New(msg)
+		return csifault.CSITaskInfoEmptyFault, errors.New(msg)
 	}
 
 	log.Infof("processing UnregisterVolume task: %q, opId: %q",
 		taskInfo.Task.Value, taskInfo.ActivationId)
 	res, err := getTaskResultFromTaskInfo(ctx, taskInfo)
 	if err != nil {
-		msg := "failed to get UnregisterVolume task result"
-		log.Errorf("%s with error: %v", msg, err)
-		return errors.New(msg)
+		log.Errorf("failed to get the task result for UnregisterVolume task from vCenter %q with err: %v",
+			m.virtualCenter.Config.Host, err)
+		return ExtractFaultTypeFromErr(ctx, err), err
 	}
 
 	if res == nil {
-		msg := "task result is empty for UnregisterVolume task"
-		log.Errorf("%s from vCenter %q. taskID: %q, opId: %q",
+		msg := "task result is nil for UnregisterVolume task"
+		log.Errorf("%s from vCenter %q. taskID: %q, opId: %q", msg,
 			m.virtualCenter.Config.Host, taskInfo.Task.Value, taskInfo.ActivationId)
-		return errors.New(msg)
+		return csifault.CSITaskResultEmptyFault, errors.New(msg)
 	}
 
 	volOpRes := res.GetCnsVolumeOperationResult()
 	if volOpRes.Fault != nil {
-		msg := "observed a fault in UnregisterVolume result"
-		log.Errorf("%s volume: %q, fault: %q, opID: %q",
-			msg, volumeID, ExtractFaultTypeFromVolumeResponseResult(ctx, volOpRes), taskInfo.ActivationId)
-		return errors.New(msg)
+		msg := "volume operation result contains fault"
+		fault := ExtractFaultTypeFromVolumeResponseResult(ctx, volOpRes)
+		log.Errorf("%s from vCenter %q. fault: %q, opId: %q", msg,
+			m.virtualCenter.Config.Host, fault, taskInfo.ActivationId)
+		return fault, errors.New(msg)
 	}
 
 	log.Infof("volume %q unregistered successfully", volumeID)
-	return nil
+	return "", nil
 }

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

@@ -31,16 +31,19 @@ type MockManager struct {
 	failRequest bool
 	// err is used to store the error that should be returned by the mock manager.
 	err error
+	// faultType is used to store the fault type that should be returned by the mock manager.
+	faultType string
 }
 
-func NewMockManager(failReq bool, err error) *MockManager {
+func NewMockManager(failReq bool, err error, faultType string) *MockManager {
 	if !failReq {
 		return &MockManager{}
 	}
 
 	return &MockManager{
 		failRequest: failReq,
 		err:         err,
+		faultType:   faultType,
 	}
 }
 
@@ -184,12 +187,12 @@ func (m MockManager) BatchAttachVolumes(ctx context.Context, vm *cnsvsphere.Virt
 	panic("implement me")
 }
 
-func (m MockManager) UnregisterVolume(ctx context.Context, volumeID string, unregisterDisk bool) error {
+func (m MockManager) UnregisterVolume(ctx context.Context, volumeID string, unregisterDisk bool) (string, error) {
 	if m.failRequest {
-		return m.err
+		return "", m.err
 	}
 
-	return nil
+	return "", nil
 }
 
 func (m MockManager) SyncVolume(ctx context.Context, syncVolumeSpecs []cnstypes.CnsSyncVolumeSpec) (string, error) {

pkg/common/fault/constants.go

@@ -94,4 +94,6 @@ const (
 	VimFaultInvalidArgument = VimFaultPrefix + "InvalidArgument"
 	// VimFaultCNSFault is the fault returned from CNS when a generic CNS fault occurs.
 	VimFaultCNSFault = VimFaultPrefix + "CnsFault"
+	// VimFaultNotSupported is the fault returned from CNS when the operation is not supported on the object.
+	VimFaultNotSupported = VimFaultPrefix + "NotSupported"
 )

pkg/common/unittestcommon/types.go

@@ -108,9 +108,10 @@ type MockVolumeManager struct {
 		extraParams interface{}) (*cnsvolume.CnsVolumeInfo, string, error)
 }
 
-func (m *MockVolumeManager) UnregisterVolume(ctx context.Context, volumeID string, unregisterDisk bool) error {
+func (m *MockVolumeManager) UnregisterVolume(ctx context.Context, volumeID string,
+	unregisterDisk bool) (string, error) {
 	//TODO implement me
-	return nil
+	return "", nil
 }
 
 func (m *MockVolumeManager) CreateVolume(ctx context.Context, spec *cnstypes.CnsVolumeCreateSpec,