diff --git a/pkg/apis/cnsoperator/cnsunregistervolume/v1alpha1/cnsunregistervolume_types.go b/pkg/apis/cnsoperator/cnsunregistervolume/v1alpha1/cnsunregistervolume_types.go index fc6fdb7852..69aa1f34ca 100644 --- a/pkg/apis/cnsoperator/cnsunregistervolume/v1alpha1/cnsunregistervolume_types.go +++ b/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. diff --git a/pkg/apis/cnsoperator/config/cnsunregistervolume_crd.yaml b/pkg/apis/cnsoperator/config/cnsunregistervolume_crd.yaml index 3097fb42a6..8015f09fff 100644 --- a/pkg/apis/cnsoperator/config/cnsunregistervolume_crd.yaml +++ b/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: [] \ No newline at end of file + conditions: [ ] + storedVersions: [ ] diff --git a/pkg/common/cns-lib/volume/manager.go b/pkg/common/cns-lib/volume/manager.go index ab6706cfcd..306ec7c06b 100644 --- a/pkg/common/cns-lib/volume/manager.go +++ b/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") } diff --git a/pkg/common/unittestcommon/utils.go b/pkg/common/unittestcommon/utils.go index 174c1dc945..a616a0fd01 100644 --- a/pkg/common/unittestcommon/utils.go +++ b/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 diff --git a/pkg/csi/service/common/commonco/coagnostic.go b/pkg/csi/service/common/commonco/coagnostic.go index fd84a15c87..7faebc764b 100644 --- a/pkg/csi/service/common/commonco/coagnostic.go +++ b/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 diff --git a/pkg/csi/service/common/commonco/k8sorchestrator/k8sorchestrator.go b/pkg/csi/service/common/commonco/k8sorchestrator/k8sorchestrator.go index 04e706cc46..af7782caf6 100644 --- a/pkg/csi/service/common/commonco/k8sorchestrator/k8sorchestrator.go +++ b/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 diff --git a/pkg/kubernetes/kubernetes.go b/pkg/kubernetes/kubernetes.go index c07f9ac13a..f57d0205e6 100644 --- a/pkg/kubernetes/kubernetes.go +++ b/pkg/kubernetes/kubernetes.go @@ -713,85 +713,52 @@ 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) - - if pvName == "" { - log.Debugf("PersistentVolume name is empty. Exiting...") - return nil - } - - log.Debugf("Retaining PersistentVolume %q", pvName) - pv, err := k8sClient.CoreV1().PersistentVolumes().Get(ctx, pvName, metav1.GetOptions{}) - if err != nil { - if apierrors.IsNotFound(err) { - log.Debugf("PersistentVolume %q not found. Exiting...", pvName) - return nil - } - - return logger.LogNewErrorf(log, "Failed to get PersistentVolume %q. Error: %s", pvName, err.Error()) - } - - pv.Spec.PersistentVolumeReclaimPolicy = v1.PersistentVolumeReclaimRetain - _, err = k8sClient.CoreV1().PersistentVolumes().Update(ctx, pv, metav1.UpdateOptions{}) - if err != nil { - return logger.LogNewErrorf(log, "Failed to update PersistentVolume %q to retain policy. Error: %s", - pvName, err.Error()) - } - - log.Debugf("Successfully retained PersistentVolume %q", pvName) - 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) + 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) + log.Debug("Deleting PVC") err := k8sClient.CoreV1().PersistentVolumeClaims(pvcNamespace).Delete(ctx, pvcName, metav1.DeleteOptions{}) 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...") 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 delete PVC. Error: %s", err.Error()) } - log.Debugf("Successfully deleted PersistentVolumeClaim %q in namespace %q", pvcName, pvcNamespace) + 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) + log := logger.GetLogger(ctx).With("name", pvName) if pvName == "" { - log.Debugf("PersistentVolume name is empty. Exiting...") + log.Debug("PersistentVolume name is empty. Exiting...") return nil } - log.Debugf("Deleting PersistentVolume %q", pvName) + log.Debug("Deleting PV") err := k8sClient.CoreV1().PersistentVolumes().Delete(ctx, pvName, metav1.DeleteOptions{}) if err != nil { if apierrors.IsNotFound(err) { - log.Debugf("PersistentVolume %q not found. Exiting...", pvName) + log.Debug("PV not found. Exiting...") return nil } - return logger.LogNewErrorf(log, "Failed to delete PersistentVolume %q. Error: %s", pvName, err.Error()) + return logger.LogNewErrorf(log, "Failed to delete PV. Error: %s", err.Error()) } - log.Debugf("Successfully deleted PersistentVolume %q", pvName) + log.Debug("Successfully deleted PV") return nil } @@ -811,6 +778,80 @@ func UpdateStatus(ctx context.Context, c client.Client, obj client.Object) error 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.Debug("PVC name is empty. Exiting...") + return nil + } + + pvc, err := k8sClient.CoreV1().PersistentVolumeClaims(pvcNamespace).Get(ctx, pvcName, metav1.GetOptions{}) + if err != nil { + if apierrors.IsNotFound(err) { + log.Debug("PVC not found. Exiting...", pvcName, pvcNamespace) + return nil + } + + 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 added finalizer %s to PVC", finalizer) + return nil +} + +// 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 pvcName == "" { + log.Debug("PVC name is empty. Exiting...") + return nil + } + + pvc, err := k8sClient.CoreV1().PersistentVolumeClaims(pvcNamespace).Get(ctx, pvcName, metav1.GetOptions{}) + if err != nil { + if apierrors.IsNotFound(err) { + log.Debug("PVC not found. Exiting...", pvcName, pvcNamespace) + return nil + } + + return logger.LogNewErrorf(log, "Failed to get PVC. Error: %s", err.Error()) + } + + // 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 + } + + // 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.Debugf("Successfully removed finalizer %s from PVC", finalizer) + return nil +} + // AddFinalizer adds the specified finalizer to the given Kubernetes object if it is not already present. // It updates the object in the Kubernetes cluster to persist the change. func AddFinalizer(ctx context.Context, c client.Client, obj client.Object, finalizer string) error { diff --git a/pkg/syncer/admissionhandler/cnscsi_admissionhandler_test.go b/pkg/syncer/admissionhandler/cnscsi_admissionhandler_test.go index b056114293..f9049a97a7 100644 --- a/pkg/syncer/admissionhandler/cnscsi_admissionhandler_test.go +++ b/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) } diff --git a/pkg/syncer/cnsoperator/controller/cnsnodevmbatchattachment/cnsnodevmbatchattachment_helper.go b/pkg/syncer/cnsoperator/controller/cnsnodevmbatchattachment/cnsnodevmbatchattachment_helper.go index d6695c9b59..c5f55f325a 100644 --- a/pkg/syncer/cnsoperator/controller/cnsnodevmbatchattachment/cnsnodevmbatchattachment_helper.go +++ b/pkg/syncer/cnsoperator/controller/cnsnodevmbatchattachment/cnsnodevmbatchattachment_helper.go @@ -33,7 +33,7 @@ import ( vimtypes "github.com/vmware/govmomi/vim25/types" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" - v1alpha1 "sigs.k8s.io/vsphere-csi-driver/v3/pkg/apis/cnsoperator/cnsnodevmbatchattachment/v1alpha1" + "sigs.k8s.io/vsphere-csi-driver/v3/pkg/apis/cnsoperator/cnsnodevmbatchattachment/v1alpha1" volumes "sigs.k8s.io/vsphere-csi-driver/v3/pkg/common/cns-lib/volume" cnsvsphere "sigs.k8s.io/vsphere-csi-driver/v3/pkg/common/cns-lib/vsphere" "sigs.k8s.io/vsphere-csi-driver/v3/pkg/common/config" @@ -71,11 +71,6 @@ func removeFinalizerFromCRDInstance(ctx context.Context, return k8s.PatchFinalizers(ctx, c, instance, finalizersOnInstance) } -// getNamespacedPvcName take namespace and pvcName sends back namespace + "/" + pvcName. -func getNamespacedPvcName(namespace string, pvcName string) string { - return namespace + "/" + pvcName -} - // getVolumesToDetachFromInstance finds out which are the volumes to detach by finding out which are // the volumes present in attachedFCDs but not in spec of the instance. func getVolumesToDetachFromInstance(ctx context.Context, @@ -310,8 +305,8 @@ func getVolumeNameVolumeIdMapsInSpec(ctx context.Context, volumeIdsInSpec = make(map[string]string) volumeNamesInSpec = make(map[string]string) for _, volume := range instance.Spec.Volumes { - namespacedPvcName := getNamespacedPvcName(instance.Namespace, volume.PersistentVolumeClaim.ClaimName) - volumeId, ok := commonco.ContainerOrchestratorUtility.GetVolumeIDFromPVCName(namespacedPvcName) + volumeId, ok := commonco.ContainerOrchestratorUtility.GetVolumeIDFromPVCName( + instance.Namespace, volume.PersistentVolumeClaim.ClaimName) if !ok { msg := fmt.Sprintf("failed to find volumeID for PVC %s", volume.PersistentVolumeClaim.ClaimName) log.Errorf(msg) @@ -328,8 +323,8 @@ func getVolumeNameVolumeIdMapsInSpec(ctx context.Context, func getPvcsInSpec(instance *v1alpha1.CnsNodeVmBatchAttachment) (map[string]string, error) { pvcsInSpec := make(map[string]string) for _, volume := range instance.Spec.Volumes { - namespacedPvcName := getNamespacedPvcName(instance.Namespace, volume.PersistentVolumeClaim.ClaimName) - volumeId, ok := commonco.ContainerOrchestratorUtility.GetVolumeIDFromPVCName(namespacedPvcName) + volumeId, ok := commonco.ContainerOrchestratorUtility.GetVolumeIDFromPVCName( + instance.Namespace, volume.PersistentVolumeClaim.ClaimName) if !ok { return pvcsInSpec, fmt.Errorf("failed to find volumeID for PVC %s", volume.PersistentVolumeClaim.ClaimName) } @@ -398,8 +393,7 @@ func constructBatchAttachRequest(ctx context.Context, volumeName := volume.Name // Find volumeID for PVC. - namespacedPvcName := getNamespacedPvcName(instance.Namespace, pvcName) - attachVolumeId, ok := commonco.ContainerOrchestratorUtility.GetVolumeIDFromPVCName(namespacedPvcName) + attachVolumeId, ok := commonco.ContainerOrchestratorUtility.GetVolumeIDFromPVCName(instance.Namespace, pvcName) if !ok { err := fmt.Errorf("failed to find volumeID for PVC %s", pvcName) log.Error(err) diff --git a/pkg/syncer/cnsoperator/controller/cnsregistervolume/cnsregistervolume_controller_test.go b/pkg/syncer/cnsoperator/controller/cnsregistervolume/cnsregistervolume_controller_test.go index 519dd5142b..14e395bd51 100644 --- a/pkg/syncer/cnsoperator/controller/cnsregistervolume/cnsregistervolume_controller_test.go +++ b/pkg/syncer/cnsoperator/controller/cnsregistervolume/cnsregistervolume_controller_test.go @@ -397,7 +397,7 @@ func (m *mockCOCommon) GetPvcObjectByName(ctx context.Context, pvcName string, return nil, nil } -func (m *mockCOCommon) GetVolumeIDFromPVCName(pvcName string) (string, bool) { +func (m *mockCOCommon) GetVolumeIDFromPVCName(namespace string, pvcName string) (string, bool) { return "vol-1", true } diff --git a/pkg/syncer/cnsoperator/controller/cnsunregistervolume/controller.go b/pkg/syncer/cnsoperator/controller/cnsunregistervolume/controller.go index 3927978537..03e7fcfc56 100644 --- a/pkg/syncer/cnsoperator/controller/cnsunregistervolume/controller.go +++ b/pkg/syncer/cnsoperator/controller/cnsunregistervolume/controller.go @@ -18,6 +18,7 @@ package cnsunregistervolume import ( "context" + "errors" "fmt" "sync" "time" @@ -33,6 +34,7 @@ import ( ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "sigs.k8s.io/controller-runtime/pkg/manager" "sigs.k8s.io/controller-runtime/pkg/predicate" "sigs.k8s.io/controller-runtime/pkg/reconcile" @@ -45,7 +47,7 @@ import ( "sigs.k8s.io/vsphere-csi-driver/v3/pkg/csi/service/logger" k8s "sigs.k8s.io/vsphere-csi-driver/v3/pkg/kubernetes" "sigs.k8s.io/vsphere-csi-driver/v3/pkg/syncer" - cnsoperatortypes "sigs.k8s.io/vsphere-csi-driver/v3/pkg/syncer/cnsoperator/types" + cnsoptypes "sigs.k8s.io/vsphere-csi-driver/v3/pkg/syncer/cnsoperator/types" "sigs.k8s.io/vsphere-csi-driver/v3/pkg/syncer/cnsoperator/util" ) @@ -107,8 +109,13 @@ func Add(mgr manager.Manager, clusterFlavor cnstypes.CnsClusterFlavor, // newReconciler returns a new reconcile.Reconciler. func newReconciler(mgr manager.Manager, configInfo *commonconfig.ConfigurationInfo, volumeManager volumes.Manager, recorder record.EventRecorder) reconcile.Reconciler { - return &Reconciler{client: mgr.GetClient(), scheme: mgr.GetScheme(), - configInfo: configInfo, volumeManager: volumeManager, recorder: recorder} + return &Reconciler{ + client: mgr.GetClient(), + scheme: mgr.GetScheme(), + configInfo: configInfo, + volumeManager: volumeManager, + recorder: recorder, + } } // add adds a new Controller to mgr with r as the reconcile.Reconciler. @@ -121,9 +128,7 @@ func add(mgr manager.Manager, r reconcile.Reconciler) error { err := ctrl.NewControllerManagedBy(mgr).Named("cnsunregistervolume-controller"). For(&v1a1.CnsUnregisterVolume{}). WithEventFilter(predicate.GenerationChangedPredicate{}). - WithOptions(controller.Options{ - MaxConcurrentReconciles: maxWorkerThreads}, - ). + WithOptions(controller.Options{MaxConcurrentReconciles: maxWorkerThreads}). Complete(r) if err != nil { log.Errorf("Failed to build application controller. Err: %v", err) @@ -138,7 +143,7 @@ func add(mgr manager.Manager, r reconcile.Reconciler) error { // reconcile.Reconciler. var _ reconcile.Reconciler = &Reconciler{} -// Reconciler reconciles a CnsUÌ„nregisterVolume object. +// Reconciler reconciles a CnsUnregisterVolume object. type Reconciler struct { // This client, initialized using mgr.Client() above, is a split client // that reads objects from the cache and writes to the apiserver. @@ -150,10 +155,11 @@ type Reconciler struct { } var ( - newK8sClient = k8s.NewClient - retainPV = k8s.RetainPersistentVolume - deletePVC = k8s.DeletePersistentVolumeClaim - deletePV = k8s.DeletePersistentVolume + newK8sClient = k8s.NewClient + protectPVC = k8s.AddFinalizerOnPVC + deletePVC = k8s.DeletePersistentVolumeClaim + deletePV = k8s.DeletePersistentVolume + removeFinalizerFromPVC = k8s.RemoveFinalizerFromPVC ) // Reconcile reads that state of the cluster for a Reconciler object @@ -166,124 +172,217 @@ var ( func (r *Reconciler) Reconcile(ctx context.Context, request reconcile.Request) (reconcile.Result, error) { ctx = logger.NewContextWithLogger(ctx) - log := logger.GetLogger(ctx) + log := logger.GetLogger(ctx).With("name", request.NamespacedName) // Fetch the CnsUnregisterVolume instance. instance := &v1a1.CnsUnregisterVolume{} err := r.client.Get(ctx, request.NamespacedName, instance) if err != nil { if apierrors.IsNotFound(err) { - log.Infof("CnsUnregisterVolume resource not found. Ignoring since object must be deleted.") + log.Info("instance not found. Ignoring since it must be deleted.") return reconcile.Result{}, nil } - log.Errorf("Error reading the CnsUnregisterVolume with name: %q on namespace: %q. Err: %+v", - request.Name, request.Namespace, err) - // Error reading the object - return with err. + log.Error("Error reading the instance. ", err) return reconcile.Result{}, err } - // Initialize backOffDuration for the instance, if required. - backOffDurationMapMutex.Lock() - var timeout time.Duration - if _, exists := backOffDuration[request.NamespacedName]; !exists { - backOffDuration[request.NamespacedName] = time.Second + log.Info("reconciling instance") + defer func() { + log.Info("finished reconciling instance") + }() + + backoff := getBackoffDuration(ctx, request.NamespacedName) + log.Info("backoff duration is ", backoff) + + // 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) + if err != nil { + log.Error("failed to reconcile with error ", err) + setInstanceError(ctx, r, instance, err.Error()) + return reconcile.Result{RequeueAfter: backoff}, nil + } + + log.Info("removing finalizer and allowing deletion of the instance") + err := removeFinalizer(ctx, r.client, instance) + if err != nil { + log.Error("failed to remove finalizer from the instance with error ", err) + setInstanceError(ctx, r, instance, "failed to remove finalizer from the instance") + return reconcile.Result{RequeueAfter: backoff}, nil + } + + deleteBackoffEntry(ctx, request.NamespacedName) + return reconcile.Result{}, nil } - timeout = backOffDuration[request.NamespacedName] - backOffDurationMapMutex.Unlock() // If the volume is already unregistered, remove the instance from the queue. if instance.Status.Unregistered { - backOffDurationMapMutex.Lock() - delete(backOffDuration, request.NamespacedName) - backOffDurationMapMutex.Unlock() + log.Debug("instance is already unregistered") + deleteBackoffEntry(ctx, request.NamespacedName) return reconcile.Result{}, nil } - log.Infof("Reconciling CnsUnregisterVolume instance %q from namespace %q. timeout %q seconds", - instance.Name, request.Namespace, timeout) - pvName, err := getPVName(ctx, instance.Spec.VolumeID) + err = r.reconcile(ctx, *instance, request) if err != nil { + log.Error("failed to reconcile with error ", err) setInstanceError(ctx, r, instance, err.Error()) - return reconcile.Result{RequeueAfter: timeout}, nil + return reconcile.Result{RequeueAfter: backoff}, nil } - pvcName, pvcNamespace, err := getPVCName(ctx, instance.Spec.VolumeID) + msg := "successfully unregistered the volume" + err = setInstanceSuccess(ctx, r, instance, msg) if err != nil { - setInstanceError(ctx, r, instance, err.Error()) - return reconcile.Result{RequeueAfter: timeout}, nil + log.Warn("failed to update status to success with error ", err) + setInstanceError(ctx, r, instance, "failed to update status to success") + return reconcile.Result{RequeueAfter: backoff}, nil } - k8sClient, err := newK8sClient(ctx) + deleteBackoffEntry(ctx, request.NamespacedName) + log.Info(msg) + return reconcile.Result{}, nil +} + +func (r *Reconciler) reconcile(ctx context.Context, + instance v1a1.CnsUnregisterVolume, request reconcile.Request) error { + log := logger.GetLogger(ctx).With("name", request.NamespacedName) + + params, err := getValidatedParams(ctx, instance) if err != nil { - log.Warn("Failed to init K8S client for volume unregistration") - setInstanceError(ctx, r, instance, "Failed to init K8S client for volume unregistration") - return reconcile.Result{RequeueAfter: timeout}, nil + log.Error("invalid input parameters ", err) + return err } - usageInfo, err := getVolumeUsageInfo(ctx, k8sClient, pvcName, pvcNamespace, - instance.Spec.ForceUnregister) + // 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.Warn(err) - setInstanceError(ctx, r, instance, err.Error()) - return reconcile.Result{RequeueAfter: timeout}, nil + log.Error("failed to protect instance with error ", err) + return err + } + + usageInfo, err := getVolumeUsageInfo(ctx, params.pvcName, params.namespace, params.force) + if err != nil { + log.Error("failed to get volume usage info with error ", err) + return err } if usageInfo.isInUse { - msg := fmt.Sprintf("Volume %q cannot be unregistered because %s", instance.Spec.VolumeID, usageInfo) - log.Warn(msg) - setInstanceError(ctx, r, instance, msg) - return reconcile.Result{RequeueAfter: timeout}, nil + msg := fmt.Sprintf("volume %s cannot be unregistered because %s", params.volumeID, usageInfo) + log.Error(msg) + return errors.New(msg) } - err = retainPV(ctx, k8sClient, pvName) + err = unregisterVolume(ctx, r.volumeManager, request, *params) if err != nil { - log.Warn(err) - setInstanceError(ctx, r, instance, err.Error()) - return reconcile.Result{RequeueAfter: timeout}, nil + log.Error("failed to unregister volume with error ", err) + return err } - err = deletePVC(ctx, k8sClient, pvcName, pvcNamespace) + log.Info("successfully unregistered volume") + return nil +} + +// reconcileDelete handles deletion of a CnsUnregisterVolume instance. +// The reconciler tries to keep the system consistent by carefully deciding +// when to continue with volume unregistration. +// The finalizer on the instance will only be removed if the result will keep the system in a consistent state. +func (r *Reconciler) reconcileDelete(ctx context.Context, + instance v1a1.CnsUnregisterVolume, request reconcile.Request) error { + log := logger.GetLogger(ctx).With("name", request.NamespacedName) + + if instance.Status.Unregistered { + // If the volume is already unregistered, the instance can be deleted. + log.Info("volume is already unregistered") + return nil + } + + params, err := getValidatedParams(ctx, instance) if err != nil { - log.Warn(err) - setInstanceError(ctx, r, instance, err.Error()) - return reconcile.Result{RequeueAfter: timeout}, nil + // If input parameters are invalid, the instance can be deleted + // since the volume cannot be unregistered. + log.Info("invalid input parameters ", instance.Spec) + return nil } - err = deletePV(ctx, k8sClient, pvName) + usageInfo, err := getVolumeUsageInfo(ctx, params.pvcName, params.namespace, params.force) if err != nil { - log.Warn(err) - setInstanceError(ctx, r, instance, err.Error()) - return reconcile.Result{RequeueAfter: timeout}, nil + log.Error("failed to get volume usage info with error ", err) + return err } - unregDisk := false - if !instance.Spec.RetainFCD { - unregDisk = true + if usageInfo.isInUse { + // If the volume is in use, the instance can be deleted + // since the volume cannot be unregistered. + log.Info(usageInfo) + return nil } - err = r.volumeManager.UnregisterVolume(ctx, instance.Spec.VolumeID, unregDisk) + + // 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 { - msg := fmt.Sprintf("Failed to unregister volume %q", instance.Spec.VolumeID) - log.Warnf(msg+".Error: %s", err.Error()) - setInstanceError(ctx, r, instance, msg) - return reconcile.Result{RequeueAfter: timeout}, nil + log.Error("failed to unregister volume with error ", err) + return err } - log.Infof("Unregistered CNS volume %q", instance.Spec.VolumeID) - msg := "Successfully unregistered the volume" - err = setInstanceSuccess(ctx, r, instance, msg) + log.Info("successfully unregistered volume") + return nil +} + +var unregisterVolume = _unregisterVolume + +func _unregisterVolume(ctx context.Context, volMgr volumes.Manager, + request reconcile.Request, params params) error { + log := logger.GetLogger(ctx).With("name", request.NamespacedName) + + k8sClient, err := newK8sClient(ctx) if err != nil { - msg := fmt.Sprintf("Failed to update CnsUnregisterVolume instance with error: %s", err) - log.Warn(msg) - setInstanceError(ctx, r, instance, msg) - return reconcile.Result{RequeueAfter: timeout}, nil + log.Error("failed to init K8s client for volume unregistration with error ", err) + return errors.New("failed to init K8s client for volume unregistration") } - backOffDurationMapMutex.Lock() - delete(backOffDuration, request.NamespacedName) - backOffDurationMapMutex.Unlock() - log.Info(msg) - return reconcile.Result{}, nil + err = protectPVC(ctx, k8sClient, params.pvcName, params.namespace, + cnsoptypes.CNSUnregisterProtectionFinalizer) + if err != nil { + log.Error("failed to protect associated PVC with error ", err) + return fmt.Errorf("failed to protect associated PVC %s/%s", params.namespace, params.pvcName) + } + + err = deletePVC(ctx, k8sClient, params.pvcName, params.namespace) + if err != nil { + log.Error("failed to delete associated PVC with error ", err) + return fmt.Errorf("failed to delete associated PVC %s/%s", params.namespace, params.pvcName) + } + + err = deletePV(ctx, k8sClient, params.pvName) + if err != nil { + log.Error("failed to delete associated PV with error ", err) + return fmt.Errorf("failed to delete associated PV %s", params.pvName) + } + + unregDisk := !params.retainFCD // If retainFCD is false, unregister the FCD too. + err = volMgr.UnregisterVolume(ctx, params.volumeID, unregDisk) + if err != nil { + log.Error("failed to unregister associated volume with error ", err) + return fmt.Errorf("failed to unregister associated volume %s", params.volumeID) + } + + err = removeFinalizerFromPVC(ctx, k8sClient, params.pvcName, params.namespace, + cnsoptypes.CNSUnregisterProtectionFinalizer) + if err != nil { + log.Error("failed to remove finalizer from associated PVC with error ", err) + return fmt.Errorf("failed to remove finalizer from associated PVC %s/%s", + params.namespace, params.pvcName) + } + + log.Debug("successfully unregistered CNS volume ", params.volumeID) + return nil } // setInstanceError sets error and records an event on the CnsUnregisterVolume @@ -325,16 +424,120 @@ func recordEvent(ctx context.Context, r *Reconciler, switch eventtype { case v1.EventTypeWarning: // Double backOff duration. - backOffDurationMapMutex.Lock() - backOffDuration[namespacedName] = min(backOffDuration[namespacedName]*2, - cnsoperatortypes.MaxBackOffDurationForReconciler) + doubleBackoffDuration(ctx, namespacedName) r.recorder.Event(instance, v1.EventTypeWarning, "CnsUnregisterVolumeFailed", msg) - backOffDurationMapMutex.Unlock() case v1.EventTypeNormal: // Reset backOff duration to one second. - backOffDurationMapMutex.Lock() - backOffDuration[namespacedName] = time.Second + updateBackoffEntry(ctx, namespacedName, time.Second) r.recorder.Event(instance, v1.EventTypeNormal, "CnsUnregisterVolumeSucceeded", msg) - backOffDurationMapMutex.Unlock() } } + +type params struct { + retainFCD bool + force bool + namespace string + volumeID string + pvcName string + pvName string +} + +func (p *params) String() string { + return fmt.Sprintf("retainFCD: %t, force: %t, namespace: %s, volumeID: %s, pvcName: %s, pvName: %s", + p.retainFCD, p.force, p.namespace, p.volumeID, p.pvcName, p.pvName) +} + +var getValidatedParams = _getValidatedParams + +func _getValidatedParams(ctx context.Context, instance v1a1.CnsUnregisterVolume) (*params, error) { + log := logger.GetLogger(ctx).With("name", instance.Namespace+"/"+instance.Name) + var err error + p := params{ + retainFCD: instance.Spec.RetainFCD, + force: instance.Spec.ForceUnregister, + namespace: instance.Namespace, + } + + if instance.Spec.VolumeID == "" && instance.Spec.PVCName == "" { + return nil, errors.New("either VolumeID or PVCName must be specified") + } + + if instance.Spec.VolumeID != "" && instance.Spec.PVCName != "" { + return nil, errors.New("both VolumeID and PVCName cannot be specified") + } + + if instance.Spec.VolumeID != "" { + p.volumeID = instance.Spec.VolumeID + + p.pvcName, _, err = getPVCName(ctx, instance.Spec.VolumeID) + if err != nil { + log.Info("no PVC found for the Volume ID ", instance.Spec.VolumeID) + } + } else { + p.pvcName = instance.Spec.PVCName + p.volumeID, err = getVolumeID(ctx, p.pvcName, p.namespace) + if err != nil { + log.Info("no Volume found for the PVC ", p.pvcName) + } + } + + p.pvName, err = getPVName(ctx, p.volumeID) + if err != nil { + log.Info("no PV found for the Volume ID ", p.volumeID) + } + + log.Debug("validated input parameters: ", p) + return &p, nil +} + +func getBackoffDuration(ctx context.Context, name types.NamespacedName) time.Duration { + backOffDurationMapMutex.Lock() + defer backOffDurationMapMutex.Unlock() + if _, exists := backOffDuration[name]; !exists { + backOffDuration[name] = time.Second + } + + return backOffDuration[name] +} + +func doubleBackoffDuration(ctx context.Context, name types.NamespacedName) { + d := getBackoffDuration(ctx, name) + d = min(d*2, cnsoptypes.MaxBackOffDurationForReconciler) + updateBackoffEntry(ctx, name, d) +} + +func updateBackoffEntry(ctx context.Context, name types.NamespacedName, duration time.Duration) { + backOffDurationMapMutex.Lock() + defer backOffDurationMapMutex.Unlock() + backOffDuration[name] = duration +} + +func deleteBackoffEntry(ctx context.Context, name types.NamespacedName) { + backOffDurationMapMutex.Lock() + defer backOffDurationMapMutex.Unlock() + delete(backOffDuration, name) +} + +func protectInstance(ctx context.Context, c client.Client, obj *v1a1.CnsUnregisterVolume) error { + log := logger.GetLogger(ctx).With("name", obj.Namespace+"/"+obj.Name) + + if !controllerutil.AddFinalizer(obj, cnsoptypes.CNSUnregisterVolumeFinalizer) { + log.Debugf("finalizer %s already exists on instance", cnsoptypes.CNSUnregisterVolumeFinalizer) + return nil + } + + log.Infof("adding finalizer %s to instance", cnsoptypes.CNSUnregisterVolumeFinalizer) + return c.Update(ctx, obj) +} + +func removeFinalizer(ctx context.Context, c client.Client, obj *v1a1.CnsUnregisterVolume) error { + log := logger.GetLogger(ctx).With("name", obj.Namespace+"/"+obj.Name) + + if controllerutil.RemoveFinalizer(obj, cnsoptypes.CNSUnregisterVolumeFinalizer) { + log.Infof("removing finalizer %s from instance", cnsoptypes.CNSUnregisterVolumeFinalizer) + return c.Update(ctx, obj) + } + + log.Debugf("finalizer %s does not exist on instance", cnsoptypes.CNSUnregisterVolumeFinalizer) + return nil +} diff --git a/pkg/syncer/cnsoperator/controller/cnsunregistervolume/controller_test.go b/pkg/syncer/cnsoperator/controller/cnsunregistervolume/controller_test.go index 3fd5b7e774..1b4190af90 100644 --- a/pkg/syncer/cnsoperator/controller/cnsunregistervolume/controller_test.go +++ b/pkg/syncer/cnsoperator/controller/cnsunregistervolume/controller_test.go @@ -19,6 +19,7 @@ package cnsunregistervolume import ( "context" "errors" + "fmt" "testing" "time" @@ -29,8 +30,7 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" - "k8s.io/client-go/kubernetes" - fakeclientset "k8s.io/client-go/kubernetes/fake" + clientset "k8s.io/client-go/kubernetes" "k8s.io/client-go/tools/record" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" @@ -39,404 +39,777 @@ import ( apis "sigs.k8s.io/vsphere-csi-driver/v3/pkg/apis/cnsoperator" v1a1 "sigs.k8s.io/vsphere-csi-driver/v3/pkg/apis/cnsoperator/cnsunregistervolume/v1alpha1" "sigs.k8s.io/vsphere-csi-driver/v3/pkg/common/cns-lib/volume" - "sigs.k8s.io/vsphere-csi-driver/v3/pkg/common/unittestcommon" - "sigs.k8s.io/vsphere-csi-driver/v3/pkg/csi/service/common/commonco" + cnsoptypes "sigs.k8s.io/vsphere-csi-driver/v3/pkg/syncer/cnsoperator/types" ) func TestReconciler_Reconcile(t *testing.T) { - newK8sClientOriginal := newK8sClient - getPVNameOriginal := getPVName - getPVCNameOriginal := getPVCName + // function overrides + getValidatedParamsOriginal := getValidatedParams getVolumeUsageInfoOriginal := getVolumeUsageInfo - retainPVOriginal := retainPV - deletePVCOriginal := deletePVC - deletePVOriginal := deletePV + unregisterVolumeOriginal := unregisterVolume + backOffDurationOriginal := backOffDuration defer func() { - newK8sClient = newK8sClientOriginal - getPVName = getPVNameOriginal - getPVCName = getPVCNameOriginal + getValidatedParams = getValidatedParamsOriginal getVolumeUsageInfo = getVolumeUsageInfoOriginal - retainPV = retainPVOriginal - deletePVC = deletePVCOriginal - deletePV = deletePVOriginal + unregisterVolume = unregisterVolumeOriginal + backOffDuration = backOffDurationOriginal }() + setup := func(t *testing.T, runtimeObjs []client.Object, interceptorFuncs interceptor.Funcs, + volumeManager volume.Manager) *Reconciler { + t.Helper() + c := newClient(t, runtimeObjs, interceptorFuncs) + backOffDuration = make(map[types.NamespacedName]time.Duration) + return &Reconciler{ + client: c, + recorder: record.NewFakeRecorder(10), + volumeManager: volumeManager, + } + } + assertUtil := func(t *testing.T, r *Reconciler, req reconcile.Request, res reconcile.Result, err error, + expRequeue bool, expRequeueAfter time.Duration, expBackoff bool, expErr, expEvent, expStatus string) { + t.Helper() + + if expErr == "" { + assert.Nil(t, err, "Expected no error") + } else { + assert.NotNil(t, err, "Expected an error") + } + + if expRequeue { + assert.Equal(t, expRequeueAfter, res.RequeueAfter, "Expected requeue after duration") + } else { + assert.True(t, res.IsZero(), "Expected no requeue") + } + + if expBackoff { + assert.Equal(t, expRequeueAfter*2, backOffDuration[req.NamespacedName], "Expected backoff duration to be doubled") + } else { + assert.NotContains(t, backOffDuration, req.NamespacedName, "Expected no backoff duration") + } + + if expEvent != "" { + if r.recorder == nil { + t.Fatal("Expected recorder to be initialized") + } + + event, ok := <-r.recorder.(*record.FakeRecorder).Events + if !ok { + t.Fatal("Expected an event but none found") + } + + assert.Contains(t, event, expEvent) + } + + if expStatus != "" { + var updatedInstance v1a1.CnsUnregisterVolume + err := r.client.Get(context.Background(), req.NamespacedName, &updatedInstance) + if err != nil { + t.Fatalf("Failed to get updated instance: %v", err) + } + + assert.Equal(t, expStatus, updatedInstance.Status.Error, "Expected status error to match") + } + } request := reconcile.Request{ NamespacedName: types.NamespacedName{ Name: "mock-instance", Namespace: "mock-namespace", }, } + t.Run("WhenGettingInstanceFails", func(tt *testing.T) { tt.Run("WhenNotFound", func(tt *testing.T) { - cb := fake.NewClientBuilder().WithInterceptorFuncs( - interceptor.Funcs{ - Get: func(ctx context.Context, client client.WithWatch, key client.ObjectKey, - obj client.Object, opts ...client.GetOption) error { - return apierrors.NewNotFound( - schema.GroupResource{ - Group: "cns.vmware.com", - Resource: "cnsunregistervolume", - }, key.Name) - }, + // Setup + reconciler := setup(tt, nil, interceptor.Funcs{ + Get: func(ctx context.Context, client client.WithWatch, key client.ObjectKey, + obj client.Object, opts ...client.GetOption) error { + return apierrors.NewNotFound( + schema.GroupResource{ + Group: "cns.vmware.com", + Resource: "cnsunregistervolume", + }, key.Name) }, - ) - reconciler := &Reconciler{ - client: cb.Build(), - } - ctx := context.Background() - res, err := reconciler.Reconcile(ctx, request) - assert.Nil(tt, err) - assert.True(tt, res.IsZero(), "Expected no requeue") + }, nil) + + // Execute + res, err := reconciler.Reconcile(context.Background(), request) + + // Assert + assertUtil(tt, reconciler, request, res, err, false, 0, false, "", "", "") }) tt.Run("WhenOtherError", func(tt *testing.T) { - cb := fake.NewClientBuilder().WithInterceptorFuncs( - interceptor.Funcs{ - Get: func(ctx context.Context, client client.WithWatch, key client.ObjectKey, - obj client.Object, opts ...client.GetOption) error { - return apierrors.NewInternalError(errors.New("other error")) - }, + // Setup + reconciler := setup(tt, nil, interceptor.Funcs{ + Get: func(ctx context.Context, client client.WithWatch, key client.ObjectKey, + obj client.Object, opts ...client.GetOption) error { + return apierrors.NewInternalError(errors.New("other error")) + }, + }, nil) + + // Execute + res, err := reconciler.Reconcile(context.Background(), request) + + // Assert + assertUtil(tt, reconciler, request, res, err, false, time.Second, false, "other error", "", "") + }) + }) + + t.Run("WhenInstanceIsBeingDeleted", func(tt *testing.T) { + tt.Run("WhenVolumeIsAlreadyUnregistered", func(tt *testing.T) { + // Setup + instance := newInstance(tt, "mock-instance", "mock-namespace", "mock-volume-id", "", "", + []string{cnsoptypes.CNSUnregisterVolumeFinalizer}, true, false, true, true) + reconciler := setup(tt, []client.Object{instance}, interceptor.Funcs{}, nil) + + // Execute + res, err := reconciler.Reconcile(context.Background(), request) + + // Assert + assertUtil(tt, reconciler, request, res, err, false, 0, false, "", "", "") + }) + tt.Run("WhenParamsAreInvalid", func(tt *testing.T) { + // Setup + errMsg := "invalid parameters" + instance := newInstance(tt, "mock-instance", "mock-namespace", "mock-volume-id", "", "", + []string{cnsoptypes.CNSUnregisterVolumeFinalizer}, true, false, false, true) + getValidatedParams = func(ctx context.Context, instance v1a1.CnsUnregisterVolume) (*params, error) { + return nil, errors.New(errMsg) + } + reconciler := setup(tt, []client.Object{instance}, interceptor.Funcs{}, nil) + + // Execute + res, err := reconciler.Reconcile(context.Background(), request) + + // Assert + assertUtil(tt, reconciler, request, res, err, false, 0, false, "", "", "") + }) + tt.Run("WhenGettingVolumeUsageInfoFails", func(tt *testing.T) { + // Setup + getValidatedParams = func(ctx context.Context, instance v1a1.CnsUnregisterVolume) (*params, error) { + return ¶ms{}, nil + } + getVolumeUsageInfo = func(ctx context.Context, + pvcName, pvcNamespace string, ignoreVMUsage bool) (*volumeUsageInfo, error) { + return nil, errors.New("internal error") + } + instance := newInstance(tt, "mock-instance", "mock-namespace", "mock-volume-id", "", "", + []string{cnsoptypes.CNSUnregisterVolumeFinalizer}, true, false, false, true) + reconciler := setup(tt, []client.Object{instance}, interceptor.Funcs{}, nil) + + // Execute + res, err := reconciler.Reconcile(context.Background(), request) + + // Assert + assertUtil(tt, reconciler, request, res, err, true, + time.Second, true, "", "", "") + }) + tt.Run("WhenVolumeIsInUse", func(tt *testing.T) { + // Setup + usageInfo := &volumeUsageInfo{ + pods: []string{"pod"}, + isInUse: true, + } + getValidatedParams = func(ctx context.Context, instance v1a1.CnsUnregisterVolume) (*params, error) { + return ¶ms{}, nil + } + getVolumeUsageInfo = func(ctx context.Context, + pvcName, pvcNamespace string, ignoreVMUsage bool) (*volumeUsageInfo, error) { + return usageInfo, nil + } + instance := newInstance(tt, "mock-instance", "mock-namespace", "mock-volume-id", "", "", + []string{cnsoptypes.CNSUnregisterVolumeFinalizer}, true, false, false, true) + reconciler := setup(tt, []client.Object{instance}, interceptor.Funcs{}, nil) + + // Execute + res, err := reconciler.Reconcile(context.Background(), request) + + // Assert + assertUtil(tt, reconciler, request, res, err, false, 0, false, "", "", "") + }) + tt.Run("WhenUnregisteringVolumeFails", func(tt *testing.T) { + // Setup + errMsg := "internal server error" + getValidatedParams = func(ctx context.Context, instance v1a1.CnsUnregisterVolume) (*params, error) { + return ¶ms{}, nil + } + getVolumeUsageInfo = func(ctx context.Context, + pvcName, pvcNamespace string, ignoreVMUsage bool) (*volumeUsageInfo, error) { + return &volumeUsageInfo{}, nil + } + instance := newInstance(tt, "mock-instance", "mock-namespace", "mock-volume-id", "", "", + []string{cnsoptypes.CNSUnregisterVolumeFinalizer}, true, false, false, true) + unregisterVolume = func(ctx context.Context, volMgr volume.Manager, request reconcile.Request, params params) error { + return errors.New(errMsg) + } + reconciler := setup(tt, []client.Object{instance}, interceptor.Funcs{}, nil) + + // Execute + res, err := reconciler.Reconcile(context.Background(), request) + + // Assert + assertUtil(tt, reconciler, request, res, err, true, time.Second, true, "", errMsg, errMsg) + }) + tt.Run("WhenRemovingFinalizerFails", func(tt *testing.T) { + // Setup + errMsg := "internal server error" + getValidatedParams = func(ctx context.Context, instance v1a1.CnsUnregisterVolume) (*params, error) { + return ¶ms{}, nil + } + getVolumeUsageInfo = func(ctx context.Context, + pvcName, pvcNamespace string, ignoreVMUsage bool) (*volumeUsageInfo, error) { + return &volumeUsageInfo{}, nil + } + unregisterVolume = func(ctx context.Context, volMgr volume.Manager, request reconcile.Request, params params) error { + return nil + } + instance := newInstance(tt, "mock-instance", "mock-namespace", "mock-volume-id", "", "", + []string{cnsoptypes.CNSUnregisterVolumeFinalizer}, true, false, false, true) + reconciler := setup(tt, []client.Object{instance}, interceptor.Funcs{ + Update: func(ctx context.Context, client client.WithWatch, + obj client.Object, opts ...client.UpdateOption) error { + return apierrors.NewInternalError(errors.New(errMsg)) }, - ) - reconciler := &Reconciler{ - client: cb.Build(), - } - ctx := context.Background() - res, err := reconciler.Reconcile(ctx, request) - assert.NotNil(tt, err) - assert.True(tt, res.IsZero(), "Expected no requeue") + }, nil) + expStatus := "failed to remove finalizer from the instance" + expErrMsg := expStatus + + // Execute + res, err := reconciler.Reconcile(context.Background(), request) + + // Assert + assertUtil(tt, reconciler, request, res, err, true, time.Second, true, + "", expErrMsg, expStatus) + }) + tt.Run("WhenSuccessful", func(tt *testing.T) { + // Setup + getValidatedParams = func(ctx context.Context, instance v1a1.CnsUnregisterVolume) (*params, error) { + return ¶ms{}, nil + } + getVolumeUsageInfo = func(ctx context.Context, + pvcName, pvcNamespace string, ignoreVMUsage bool) (*volumeUsageInfo, error) { + return &volumeUsageInfo{}, nil + } + unregisterVolume = func(ctx context.Context, volMgr volume.Manager, request reconcile.Request, params params) error { + return nil + } + instance := newInstance(tt, "mock-instance", "mock-namespace", "mock-volume-id", "", "", + []string{cnsoptypes.CNSUnregisterVolumeFinalizer}, false, false, false, true) + reconciler := setup(tt, []client.Object{instance}, interceptor.Funcs{}, nil) + + // Execute + res, err := reconciler.Reconcile(context.Background(), request) + + // Assert + assertUtil(tt, reconciler, request, res, err, false, 0, false, "", "", "") }) }) t.Run("WhenInstanceIsUnregistered", func(tt *testing.T) { // Setup - cb := fake.NewClientBuilder() - registerSchemes(tt, cb) - ctx := context.Background() - backOffDuration = make(map[types.NamespacedName]time.Duration) - instance := newInstance(tt, "mock-instance", "mock-namespace", "mock-volume-id", - false, false, true, "") - registerRuntimeObjects(tt, cb, instance) - reconciler := &Reconciler{ - client: cb.Build(), - } + instance := newInstance(tt, "mock-instance", "mock-namespace", "mock-volume-id", "", "", + nil, false, false, true, false) + reconciler := setup(tt, []client.Object{instance}, interceptor.Funcs{}, nil) // Execute - res, err := reconciler.Reconcile(ctx, request) + res, err := reconciler.Reconcile(context.Background(), request) // Assert - assert.Nil(tt, err, "Expected no error") - assert.True(tt, res.IsZero(), "Expected no requeue") - assert.NotContains(tt, backOffDuration, request, "Expected no backoff duration for unregistered instance") + assertUtil(tt, reconciler, request, res, err, false, 0, false, "", "", "") }) - cb := fake.NewClientBuilder() - registerSchemes(t, cb) - ctx := context.Background() - instance := newInstance(t, "mock-instance", "mock-namespace", "mock-volume-id", - false, false, false, "") - registerRuntimeObjects(t, cb, instance) + t.Run("WhenNormalReconcile", func(tt *testing.T) { + tt.Run("WhenParamsAreInvalid", func(tt *testing.T) { + // Setup + errMsg := "invalid parameters" + instance := newInstance(tt, "mock-instance", "mock-namespace", "mock-volume-id", "", "", + []string{cnsoptypes.CNSUnregisterVolumeFinalizer}, true, false, false, false) + getValidatedParams = func(ctx context.Context, instance v1a1.CnsUnregisterVolume) (*params, error) { + return nil, errors.New(errMsg) + } + reconciler := setup(tt, []client.Object{instance}, interceptor.Funcs{}, nil) - t.Run("WhenGettingPVNameFails", func(tt *testing.T) { - // Setup - backOffDuration = make(map[types.NamespacedName]time.Duration) - reconciler := &Reconciler{ - client: cb.Build(), - recorder: record.NewFakeRecorder(10), - } - getPVName = func(ctx context.Context, volumeID string) (string, error) { - return "", errors.New("failed to get PV name") - } + // Execute + res, err := reconciler.Reconcile(context.Background(), request) - // Execute - res, err := reconciler.Reconcile(ctx, request) + // Assert + assertUtil(tt, reconciler, request, res, err, true, time.Second, true, "", errMsg, errMsg) + }) + tt.Run("WhenProtectingInstanceFails", func(tt *testing.T) { + // Setup + errMsg := "failed to add finalizer" + getValidatedParams = func(ctx context.Context, instance v1a1.CnsUnregisterVolume) (*params, error) { + return ¶ms{}, nil + } + instance := newInstance(tt, "mock-instance", "mock-namespace", "mock-volume-id", "", "", + nil, true, false, false, false) + reconciler := setup(tt, []client.Object{instance}, interceptor.Funcs{ + Update: func(ctx context.Context, client client.WithWatch, + obj client.Object, opts ...client.UpdateOption) error { + return errors.New(errMsg) + }, + }, nil) - // Assert - assert.Nil(tt, err, "Expected no error") - assert.Equal(tt, res.RequeueAfter, time.Second, "Expected requeue after 1 second") - assert.Equal(tt, 2*time.Second, backOffDuration[request.NamespacedName], "Expected backoff duration to be 2 seconds") + // Execute + res, err := reconciler.Reconcile(context.Background(), request) + + // Assert + assertUtil(tt, reconciler, request, res, err, true, time.Second, true, "", errMsg, errMsg) + }) + tt.Run("WhenGettingVolumeUsageInfoFails", func(tt *testing.T) { + // Setup + errMsg := "failed to get volume usage info" + getValidatedParams = func(ctx context.Context, instance v1a1.CnsUnregisterVolume) (*params, error) { + return ¶ms{}, nil + } + instance := newInstance(tt, "mock-instance", "mock-namespace", "mock-volume-id", "", "", + []string{cnsoptypes.CNSUnregisterVolumeFinalizer}, true, false, false, false) + getVolumeUsageInfo = func(ctx context.Context, + pvcName, pvcNamespace string, ignoreVMUsage bool) (*volumeUsageInfo, error) { + return nil, errors.New(errMsg) + } + reconciler := setup(tt, []client.Object{instance}, interceptor.Funcs{}, nil) + + // Execute + res, err := reconciler.Reconcile(context.Background(), request) + + // Assert + assertUtil(tt, reconciler, request, res, err, true, time.Second, true, "", errMsg, errMsg) + }) + tt.Run("WhenVolumeIsInUse", func(tt *testing.T) { + // Setup + usageInfo := &volumeUsageInfo{ + pods: []string{"pod"}, + isInUse: true, + } + getValidatedParams = func(ctx context.Context, instance v1a1.CnsUnregisterVolume) (*params, error) { + return ¶ms{volumeID: "mock-volume-id"}, nil + } + getVolumeUsageInfo = func(ctx context.Context, + pvcName, pvcNamespace string, ignoreVMUsage bool) (*volumeUsageInfo, error) { + return usageInfo, nil + } + instance := newInstance(tt, "mock-instance", "mock-namespace", "mock-volume-id", "", "", + []string{cnsoptypes.CNSUnregisterVolumeFinalizer}, true, false, false, false) + reconciler := setup(tt, []client.Object{instance}, interceptor.Funcs{}, nil) + expErr := fmt.Sprintf("volume %s cannot be unregistered because %s", instance.Spec.VolumeID, usageInfo) + // Execute + res, err := reconciler.Reconcile(context.Background(), request) + + // Assert + assertUtil(tt, reconciler, request, res, err, true, time.Second, true, "", expErr, expErr) + }) + tt.Run("WhenUnregisteringVolumeFails", func(tt *testing.T) { + // Setup + errMsg := "internal server error" + getValidatedParams = func(ctx context.Context, instance v1a1.CnsUnregisterVolume) (*params, error) { + return ¶ms{}, nil + } + getVolumeUsageInfo = func(ctx context.Context, + pvcName, pvcNamespace string, ignoreVMUsage bool) (*volumeUsageInfo, error) { + return &volumeUsageInfo{}, nil + } + instance := newInstance(tt, "mock-instance", "mock-namespace", "mock-volume-id", "", "", + []string{cnsoptypes.CNSUnregisterVolumeFinalizer}, true, false, false, false) + unregisterVolume = func(ctx context.Context, volMgr volume.Manager, request reconcile.Request, params params) error { + return errors.New(errMsg) + } + reconciler := setup(tt, []client.Object{instance}, interceptor.Funcs{}, nil) + + // Execute + res, err := reconciler.Reconcile(context.Background(), request) + + // Assert + assertUtil(tt, reconciler, request, res, err, true, time.Second, true, "", errMsg, errMsg) + }) + tt.Run("WhenUpdatingStatusFails", func(tt *testing.T) { + // Setup + errMsg := "internal server error" + getValidatedParams = func(ctx context.Context, instance v1a1.CnsUnregisterVolume) (*params, error) { + return ¶ms{}, nil + } + getVolumeUsageInfo = func(ctx context.Context, + pvcName, pvcNamespace string, ignoreVMUsage bool) (*volumeUsageInfo, error) { + return &volumeUsageInfo{}, nil + } + unregisterVolume = func(ctx context.Context, volMgr volume.Manager, request reconcile.Request, params params) error { + return nil + } + instance := newInstance(tt, "mock-instance", "mock-namespace", "mock-volume-id", "", "", + []string{cnsoptypes.CNSUnregisterVolumeFinalizer}, true, false, false, false) + reconciler := setup(tt, []client.Object{instance}, interceptor.Funcs{ + SubResourceUpdate: func(ctx context.Context, client client.Client, subResourceName string, + obj client.Object, opts ...client.SubResourceUpdateOption) error { + return apierrors.NewInternalError(errors.New(errMsg)) + }, + }, nil) + + // Execute + res, err := reconciler.Reconcile(context.Background(), request) + + // Assert + assertUtil(tt, reconciler, request, res, err, true, time.Second, true, + "", "", "") + }) + tt.Run("WhenSuccessful", func(tt *testing.T) { + // Setup + getValidatedParams = func(ctx context.Context, instance v1a1.CnsUnregisterVolume) (*params, error) { + return ¶ms{}, nil + } + getVolumeUsageInfo = func(ctx context.Context, + pvcName, pvcNamespace string, ignoreVMUsage bool) (*volumeUsageInfo, error) { + return &volumeUsageInfo{}, nil + } + unregisterVolume = func(ctx context.Context, volMgr volume.Manager, request reconcile.Request, params params) error { + return nil + } + instance := newInstance(tt, "mock-instance", "mock-namespace", "mock-volume-id", "", "", + []string{cnsoptypes.CNSUnregisterVolumeFinalizer}, true, false, false, false) + reconciler := setup(tt, []client.Object{instance}, interceptor.Funcs{}, nil) + + // Execute + res, err := reconciler.Reconcile(context.Background(), request) + + // Assert + assertUtil(tt, reconciler, request, res, err, false, 0, false, "", "", "") + }) }) +} - getPVName = func(ctx context.Context, volumeID string) (string, error) { - return "mock-pv-name", nil - } +func TestUnregisterVolume(t *testing.T) { + // function overrides + newK8sClientOriginal := newK8sClient + protectPVCOriginal := protectPVC + deletePVCOriginal := deletePVC + deletePVOriginal := deletePV + removeFinalizerFromPVCOriginal := removeFinalizerFromPVC + defer func() { + newK8sClient = newK8sClientOriginal + protectPVC = protectPVCOriginal + deletePVC = deletePVCOriginal + deletePV = deletePVOriginal + removeFinalizerFromPVC = removeFinalizerFromPVCOriginal + }() - t.Run("WhenGettingPVCNameFails", func(tt *testing.T) { + t.Run("WhenGettingK8sClientFails", func(tt *testing.T) { // Setup - backOffDuration = make(map[types.NamespacedName]time.Duration) - reconciler := &Reconciler{ - client: cb.Build(), - recorder: record.NewFakeRecorder(10), - } - getPVCName = func(ctx context.Context, volumeID string) (string, string, error) { - return "", "", errors.New("failed to get PVC name") + errMsg := "internal server error" + newK8sClient = func(ctx context.Context) (clientset.Interface, error) { + return nil, errors.New(errMsg) } + expErr := "failed to init K8s client for volume unregistration" // Execute - res, err := reconciler.Reconcile(ctx, request) + err := unregisterVolume(context.Background(), nil, reconcile.Request{}, params{}) // Assert - assert.Nil(tt, err, "Expected no error") - assert.Equal(tt, res.RequeueAfter, time.Second, "Expected requeue after 1 second") - assert.Equal(tt, 2*time.Second, backOffDuration[request.NamespacedName], "Expected backoff duration to be 2 seconds") - + assert.Equal(tt, expErr, err.Error(), "Expected error to match") }) - - getPVCName = func(ctx context.Context, volumeID string) (string, string, error) { - return "mock-pvc-name", "mock-pvc-namespace", nil - } - - t.Run("WhenCreatingK8sClientFails", func(tt *testing.T) { + t.Run("WhenProtectingPVCFails", func(tt *testing.T) { // Setup - backOffDuration = make(map[types.NamespacedName]time.Duration) - newK8sClient = func(ctx context.Context) (kubernetes.Interface, error) { - return nil, errors.New("failed to create k8s client") + errMsg := "internal server error" + newK8sClient = func(ctx context.Context) (clientset.Interface, error) { + return &clientset.Clientset{}, nil } - reconciler := &Reconciler{ - client: cb.Build(), - recorder: record.NewFakeRecorder(10), + protectPVC = func(ctx context.Context, k8sClient clientset.Interface, pvcName, pvcNamespace, finalizer string) error { + return errors.New(errMsg) } + params := params{} + expErr := fmt.Sprintf("failed to protect associated PVC %s/%s", params.namespace, params.pvcName) // Execute - res, err := reconciler.Reconcile(ctx, reconcile.Request{ - NamespacedName: types.NamespacedName{ - Name: "mock-instance", - Namespace: "mock-namespace", - }, - }) + err := unregisterVolume(context.Background(), nil, reconcile.Request{}, params) // Assert - assert.Nil(tt, err, "Expected no error") - assert.Equal(tt, res.RequeueAfter, time.Second, "Expected requeue after 1 second") - assert.Equal(tt, 2*time.Second, backOffDuration[types.NamespacedName{ - Name: "mock-instance", - Namespace: "mock-namespace", - }], "Expected backoff duration to be 2 seconds") + assert.Equal(tt, expErr, err.Error(), "Expected error to match") }) - - newK8sClient = func(ctx context.Context) (kubernetes.Interface, error) { - return fakeclientset.NewClientset(), nil - } - - t.Run("WhenGettingVolumeUsageInfoFails", func(tt *testing.T) { + t.Run("WhenDeletingPVCFails", func(tt *testing.T) { // Setup - backOffDuration = make(map[types.NamespacedName]time.Duration) - getVolumeUsageInfo = func(ctx context.Context, k8sClient kubernetes.Interface, pvcName string, - pvcNamespace string, ignoreVMUsage bool) (*volumeUsageInfo, error) { - return nil, errors.New("failed to get volume usage info") + errMsg := "internal server error" + newK8sClient = func(ctx context.Context) (clientset.Interface, error) { + return &clientset.Clientset{}, nil + } + protectPVC = func(ctx context.Context, k8sClient clientset.Interface, pvcName, pvcNamespace, finalizer string) error { + return nil } - reconciler := &Reconciler{ - client: cb.Build(), - recorder: record.NewFakeRecorder(10), + deletePVC = func(ctx context.Context, k8sClient clientset.Interface, pvcName, pvcNamespace string) error { + return errors.New(errMsg) } + params := params{} + expErr := fmt.Sprintf("failed to delete associated PVC %s/%s", params.namespace, params.pvcName) // Execute - res, err := reconciler.Reconcile(ctx, reconcile.Request{ - NamespacedName: types.NamespacedName{ - Name: "mock-instance", - Namespace: "mock-namespace", - }, - }) + err := unregisterVolume(context.Background(), nil, reconcile.Request{}, params) // Assert - assert.Nil(tt, err, "Expected no error") - assert.Equal(tt, res.RequeueAfter, time.Second, "Expected requeue after 1 second") - assert.Equal(tt, 2*time.Second, backOffDuration[types.NamespacedName{ - Name: "mock-instance", - Namespace: "mock-namespace", - }], "Expected backoff duration to be 2 seconds") + assert.Equal(tt, expErr, err.Error(), "Expected error to match") }) - - t.Run("WhenVolumeIsInUse", func(tt *testing.T) { + t.Run("WhenDeletingPVFails", func(tt *testing.T) { // Setup - getVolumeUsageInfo = func(ctx context.Context, k8sClient kubernetes.Interface, pvcName string, - pvcNamespace string, ignoreVMUsage bool) (*volumeUsageInfo, error) { - return &volumeUsageInfo{ - isInUse: true, - pods: []string{"pod1", "pod2"}, - }, nil + errMsg := "internal server error" + newK8sClient = func(ctx context.Context) (clientset.Interface, error) { + return &clientset.Clientset{}, nil } - newK8sClient = func(ctx context.Context) (kubernetes.Interface, error) { - return fakeclientset.NewClientset(), nil + protectPVC = func(ctx context.Context, k8sClient clientset.Interface, pvcName, pvcNamespace, finalizer string) error { + return nil } - reconciler := &Reconciler{ - client: cb.Build(), - recorder: record.NewFakeRecorder(10), + deletePVC = func(ctx context.Context, k8sClient clientset.Interface, pvcName, pvcNamespace string) error { + return nil } - ctx := context.Background() - backOffDuration = make(map[types.NamespacedName]time.Duration) - commonco.ContainerOrchestratorUtility = &unittestcommon.FakeK8SOrchestrator{} + deletePV = func(ctx context.Context, k8sClient clientset.Interface, pvName string) error { + return errors.New(errMsg) + } + params := params{pvName: "mock-pv"} + expErr := fmt.Sprintf("failed to delete associated PV %s", params.pvName) // Execute - res, err := reconciler.Reconcile(ctx, request) + err := unregisterVolume(context.Background(), nil, reconcile.Request{}, params) // Assert - assert.Nil(tt, err, "Expected no error") - assert.Equal(tt, res.RequeueAfter, time.Second, "Expected requeue after 1 second") - assert.Equal(tt, 2*time.Second, backOffDuration[request.NamespacedName], - "Expected backoff duration to be 2 seconds") + assert.Equal(tt, expErr, err.Error(), "Expected error to match") }) - - getVolumeUsageInfo = func(ctx context.Context, k8sClient kubernetes.Interface, pvcName string, - pvcNamespace string, ignoreVMUsage bool) (*volumeUsageInfo, error) { - return &volumeUsageInfo{ - isInUse: false, - }, nil - } - - t.Run("WhenRetainingPVFails", func(tt *testing.T) { + t.Run("WhenUnregisteringVolumeFromCNSFails", func(tt *testing.T) { // Setup - backOffDuration = make(map[types.NamespacedName]time.Duration) - retainPV = func(ctx context.Context, k8sClient kubernetes.Interface, pvName string) error { - return errors.New("failed to retain PV") + errMsg := "internal server error" + newK8sClient = func(ctx context.Context) (clientset.Interface, error) { + return &clientset.Clientset{}, nil + } + protectPVC = func(ctx context.Context, k8sClient clientset.Interface, pvcName, pvcNamespace, finalizer string) error { + return nil } - reconciler := &Reconciler{ - client: cb.Build(), - recorder: record.NewFakeRecorder(10), + deletePVC = func(ctx context.Context, k8sClient clientset.Interface, pvcName, pvcNamespace string) error { + return nil } + deletePV = func(ctx context.Context, k8sClient clientset.Interface, pvName string) error { + return nil + } + mockMgr := volume.NewMockManager(true, errors.New(errMsg)) + params := params{volumeID: "mock-volume-id"} + expErr := fmt.Sprintf("failed to unregister associated volume %s", params.volumeID) // Execute - res, err := reconciler.Reconcile(ctx, request) + err := unregisterVolume(context.Background(), mockMgr, reconcile.Request{}, params) // Assert - assert.Nil(tt, err, "Expected no error") - assert.Equal(tt, res.RequeueAfter, time.Second, "Expected requeue after 1 second") - assert.Equal(tt, 2*time.Second, backOffDuration[request.NamespacedName], - "Expected backoff duration to be 2 seconds") + assert.Equal(tt, expErr, err.Error(), "Expected error to match") }) - - retainPV = func(ctx context.Context, k8sClient kubernetes.Interface, pvName string) error { - return nil - } - - t.Run("WhenDeletingPVCFails", func(tt *testing.T) { + t.Run("WhenRemovingFinalizerFromPVCFails", func(tt *testing.T) { // Setup - backOffDuration = make(map[types.NamespacedName]time.Duration) - deletePVC = func(ctx context.Context, k8sClient kubernetes.Interface, pvcName string, pvcNamespace string) error { - return errors.New("failed to delete PVC") + errMsg := "internal server error" + newK8sClient = func(ctx context.Context) (clientset.Interface, error) { + return &clientset.Clientset{}, nil + } + protectPVC = func(ctx context.Context, k8sClient clientset.Interface, pvcName, pvcNamespace, finalizer string) error { + return nil } - reconciler := &Reconciler{ - client: cb.Build(), - recorder: record.NewFakeRecorder(10), + deletePVC = func(ctx context.Context, k8sClient clientset.Interface, pvcName, pvcNamespace string) error { + return nil } + deletePV = func(ctx context.Context, k8sClient clientset.Interface, pvName string) error { + return nil + } + removeFinalizerFromPVC = func(ctx context.Context, k8sClient clientset.Interface, + pvcName, pvcNamespace, finalizer string) error { + return errors.New(errMsg) + } + mockMgr := volume.NewMockManager(true, nil) + + params := params{pvcName: "mock-pvc", namespace: "mock-namespace"} + expErr := fmt.Sprintf("failed to remove finalizer from associated PVC %s/%s", params.namespace, params.pvcName) // Execute - res, err := reconciler.Reconcile(ctx, request) + err := unregisterVolume(context.Background(), mockMgr, reconcile.Request{}, params) // Assert - assert.Nil(tt, err, "Expected no error") - assert.Equal(tt, res.RequeueAfter, time.Second, "Expected requeue after 1 second") - assert.Equal(tt, 2*time.Second, backOffDuration[request.NamespacedName], - "Expected backoff duration to be 2 seconds") + assert.Equal(tt, expErr, err.Error(), "Expected error to match") }) - - deletePVC = func(ctx context.Context, k8sClient kubernetes.Interface, pvcName string, pvcNamespace string) error { - return nil - } - - t.Run("WhenDeletingPVFails", func(tt *testing.T) { + t.Run("WhenSuccessful", func(tt *testing.T) { // Setup - backOffDuration = make(map[types.NamespacedName]time.Duration) - deletePV = func(ctx context.Context, k8sClient kubernetes.Interface, pvName string) error { - return errors.New("failed to delete PV") + newK8sClient = func(ctx context.Context) (clientset.Interface, error) { + return &clientset.Clientset{}, nil + } + protectPVC = func(ctx context.Context, k8sClient clientset.Interface, pvcName, pvcNamespace, finalizer string) error { + return nil + } + deletePVC = func(ctx context.Context, k8sClient clientset.Interface, pvcName, pvcNamespace string) error { + return nil } - reconciler := &Reconciler{ - client: cb.Build(), - recorder: record.NewFakeRecorder(10), + deletePV = func(ctx context.Context, k8sClient clientset.Interface, pvName string) error { + return nil } + removeFinalizerFromPVC = func(ctx context.Context, k8sClient clientset.Interface, + pvcName, pvcNamespace, finalizer string) error { + return nil + } + mockMgr := volume.NewMockManager(true, nil) // Execute - res, err := reconciler.Reconcile(ctx, request) + err := unregisterVolume(context.Background(), mockMgr, reconcile.Request{}, params{}) // Assert assert.Nil(tt, err, "Expected no error") - assert.Equal(tt, res.RequeueAfter, time.Second, "Expected requeue after 1 second") - assert.Equal(tt, 2*time.Second, backOffDuration[request.NamespacedName], - "Expected backoff duration to be 2 seconds") }) +} - deletePV = func(ctx context.Context, k8sClient kubernetes.Interface, pvName string) error { - return nil - } +func TestGetValidatedParams(t *testing.T) { + // function overrides + getPVCNameOriginal := getPVCName + getPVNameOriginal := getPVName + getVolumeIDOriginal := getVolumeID + defer func() { + getPVCName = getPVCNameOriginal + getPVName = getPVNameOriginal + getVolumeID = getVolumeIDOriginal + }() - t.Run("WhenUnregisteringVolumeFails", func(tt *testing.T) { + t.Run("WhenVolumeIDAndPVCNameAreEmpty", func(t *testing.T) { // Setup - backOffDuration = make(map[types.NamespacedName]time.Duration) - mockVolManager := volume.NewMockManager(true, errors.New("failed to unregister volume")) - reconciler := &Reconciler{ - client: cb.Build(), - recorder: record.NewFakeRecorder(10), - volumeManager: mockVolManager, - } + instance := newInstance(t, "mock-instance", "mock-namespace", "", "", "", + nil, false, false, false, false) // Execute - res, err := reconciler.Reconcile(ctx, request) + params, err := getValidatedParams(context.Background(), *instance) // Assert - assert.Nil(tt, err, "Expected no error") - assert.Equal(tt, res.RequeueAfter, time.Second, "Expected requeue after 1 second") - assert.Equal(tt, 2*time.Second, backOffDuration[request.NamespacedName], - "Expected backoff duration to be 2 seconds") + assert.Nil(t, params, "Expected params to be nil") + assert.NotNil(t, err, "Expected an error") }) - mockVolManager := volume.NewMockManager(false, nil) - - t.Run("WhenUpdatingStatusFails", func(tt *testing.T) { + t.Run("WhenVolumeIDAndPVCNameAreBothSet", func(t *testing.T) { // Setup - backOffDuration = make(map[types.NamespacedName]time.Duration) - cb.WithInterceptorFuncs( - interceptor.Funcs{ - SubResourceUpdate: func(ctx context.Context, client client.Client, subResourceName string, - obj client.Object, opts ...client.SubResourceUpdateOption) error { - return apierrors.NewInternalError(errors.New("failed to update status")) - }, - }, - ) - reconciler := &Reconciler{ - client: cb.Build(), - recorder: record.NewFakeRecorder(10), - volumeManager: mockVolManager, - } + instance := newInstance(t, "mock-instance", "mock-namespace", "mock-volume-id", "", "mock-pvc-name", + nil, false, false, false, false) // Execute - res, err := reconciler.Reconcile(ctx, request) + params, err := getValidatedParams(context.Background(), *instance) // Assert - assert.Nil(tt, err, "Expected no error") - assert.Equal(tt, res.RequeueAfter, time.Second, "Expected requeue after 1 second") - assert.Equal(tt, 2*time.Second, backOffDuration[request.NamespacedName], - "Expected backoff duration to be 2 seconds") + assert.Nil(t, params, "Expected params to be nil") + assert.NotNil(t, err, "Expected an error") }) - cb.WithInterceptorFuncs( - interceptor.Funcs{ - SubResourceUpdate: func(ctx context.Context, client client.Client, subResourceName string, - obj client.Object, opts ...client.SubResourceUpdateOption) error { - return nil // Simulate successful update - }, - }, - ) + t.Run("WhenVolumeIDIsSet", func(t *testing.T) { + t.Run("WhenPVCNotFound", func(t *testing.T) { + // Setup + getPVCName = func(ctx context.Context, volumeID string) (string, string, error) { + return "", "", errors.New("PVC not found") + } + getPVName = func(ctx context.Context, volumeID string) (string, error) { + return "mock-pv", nil + } + instance := newInstance(t, "mock-instance", "mock-namespace", "mock-volume-id", "", "", + nil, false, false, false, false) + expParams := params{ + retainFCD: false, + force: false, + namespace: "mock-namespace", + volumeID: "mock-volume-id", + pvcName: "", + pvName: "mock-pv", + } - t.Run("WhenReconcileSucceeds", func(tt *testing.T) { - // Setup - backOffDuration = make(map[types.NamespacedName]time.Duration) - reconciler := &Reconciler{ - client: cb.Build(), - recorder: record.NewFakeRecorder(10), - volumeManager: mockVolManager, - } + // Execute + params, err := getValidatedParams(context.Background(), *instance) - // Execute - res, err := reconciler.Reconcile(ctx, request) + // Assert + assert.Nil(t, err, "Expected no error") + assert.Equal(t, expParams, *params, "Expected params to match") + }) + t.Run("WhenPVNotFound", func(t *testing.T) { + // Setup + cb := fake.NewClientBuilder() + registerSchemes(t, cb) + getPVCName = func(ctx context.Context, volumeID string) (string, string, error) { + return "mock-pvc", "mock-namespace", nil + } + getPVName = func(ctx context.Context, volumeID string) (string, error) { + return "", errors.New("PV not found") + } + instance := newInstance(t, "mock-instance", "mock-namespace", "mock-volume-id", "", "", + nil, false, false, false, false) + expParams := params{ + retainFCD: false, + force: false, + namespace: "mock-namespace", + volumeID: "mock-volume-id", + pvcName: "mock-pvc", + pvName: "", + } - // Assert - assert.Nil(tt, err, "Expected no error") - assert.True(tt, res.IsZero(), "Expected no requeue") - assert.NotContains(tt, backOffDuration, request.NamespacedName, - "Expected no backoff duration for successful reconciliation") + // Execute + params, err := getValidatedParams(context.Background(), *instance) + + // Assert + assert.Nil(t, err, "Expected no error") + assert.Equal(t, expParams, *params, "Expected params to match") + }) + }) + + t.Run("WhenPVCNameIsSet", func(t *testing.T) { + t.Run("WhenVolumeIDNotFound", func(t *testing.T) { + // Setup + getVolumeID = func(ctx context.Context, pvcName, namespace string) (string, error) { + return "", errors.New("VolumeID not found") + } + getPVName = func(ctx context.Context, volumeID string) (string, error) { + return "mock-pv", nil + } + instance := newInstance(t, "mock-instance", "mock-namespace", "", "", "mock-pvc-name", + nil, false, false, false, false) + expParams := params{ + retainFCD: false, + force: false, + namespace: "mock-namespace", + volumeID: "", + pvcName: "mock-pvc-name", + pvName: "mock-pv", + } + + // Execute + params, err := getValidatedParams(context.Background(), *instance) + + // Assert + assert.Nil(t, err, "Expected no error") + assert.Equal(t, expParams, *params, "Expected params to match") + }) + t.Run("WhenPVNotFound", func(t *testing.T) { + // Setup + getVolumeID = func(ctx context.Context, pvcName, namespace string) (string, error) { + return "mock-volume-id", nil + } + getPVName = func(ctx context.Context, volumeID string) (string, error) { + return "", errors.New("PV not found") + } + instance := newInstance(t, "mock-instance", "mock-namespace", "", "", "mock-pvc-name", + nil, false, false, false, false) + expParams := params{ + retainFCD: false, + force: false, + namespace: "mock-namespace", + volumeID: "mock-volume-id", + pvcName: "mock-pvc-name", + pvName: "", + } + + // Execute + params, err := getValidatedParams(context.Background(), *instance) + + // Assert + assert.Nil(t, err, "Expected no error") + assert.Equal(t, expParams, *params, "Expected params to match") + }) }) } +func newClient(t *testing.T, runtimeObjs []client.Object, interceptorFuncs interceptor.Funcs) client.Client { + t.Helper() + cb := fake.NewClientBuilder() + registerSchemes(t, cb) + registerRuntimeObjects(t, cb, runtimeObjs...) + cb.WithInterceptorFuncs(interceptorFuncs) + return cb.Build() +} + func registerSchemes(t *testing.T, cb *fake.ClientBuilder) { t.Helper() scheme := runtime.NewScheme() @@ -458,22 +831,30 @@ func registerRuntimeObjects(t *testing.T, cb *fake.ClientBuilder, objs ...client cb.WithStatusSubresource(objs...) } -func newInstance(t *testing.T, name, namespace, volumeID string, - retainFCD, forceUnregister, unregistered bool, err string) *v1a1.CnsUnregisterVolume { +func newInstance(t *testing.T, name, namespace, volumeID, errMsg, pvcName string, finalizers []string, + retainFCD, forceUnregister, unregistered, withDeletionTS bool) *v1a1.CnsUnregisterVolume { t.Helper() - return &v1a1.CnsUnregisterVolume{ + instance := &v1a1.CnsUnregisterVolume{ ObjectMeta: metav1.ObjectMeta{ - Name: name, - Namespace: namespace, + Name: name, + Namespace: namespace, + Finalizers: finalizers, }, Spec: v1a1.CnsUnregisterVolumeSpec{ VolumeID: volumeID, + PVCName: pvcName, RetainFCD: retainFCD, ForceUnregister: forceUnregister, }, Status: v1a1.CnsUnregisterVolumeStatus{ Unregistered: unregistered, - Error: err, + Error: errMsg, }, } + + if withDeletionTS { + instance.DeletionTimestamp = &metav1.Time{Time: time.Now()} + } + + return instance } diff --git a/pkg/syncer/cnsoperator/controller/cnsunregistervolume/util.go b/pkg/syncer/cnsoperator/controller/cnsunregistervolume/util.go index 4bd0bc1207..572c975bbe 100644 --- a/pkg/syncer/cnsoperator/controller/cnsunregistervolume/util.go +++ b/pkg/syncer/cnsoperator/controller/cnsunregistervolume/util.go @@ -68,45 +68,57 @@ func (v volumeUsageInfo) String() string { var getVolumeUsageInfo = _getVolumeUsageInfo // getVolumeUsageInfo checks if the PVC is in use by any resources in the specified namespace. +// For the sake of efficiency, the function returns as soon as it finds that the volume is in use by any resource. // If ignoreVMUsage is set to true, the function skips checking if the volume is in use by any virtual machines. -func _getVolumeUsageInfo(ctx context.Context, k8sClient clientset.Interface, pvcName string, pvcNamespace string, +func _getVolumeUsageInfo(ctx context.Context, pvcName string, pvcNamespace string, ignoreVMUsage bool) (*volumeUsageInfo, error) { log := logger.GetLogger(ctx) var volumeUsageInfo volumeUsageInfo if pvcName == "" { - log.Debugf("PVC name is empty. Nothing to do.") + log.Debug("PVC name is empty. Nothing to do.") return &volumeUsageInfo, nil } - pods, isInUse, err := getPodsForPVC(ctx, pvcName, pvcNamespace, k8sClient) + var err error + k8sClient, err := k8s.NewClient(ctx) if err != nil { + log.Errorf("failed to create k8s client. Error: %v", err) return nil, err } - volumeUsageInfo.pods = pods - volumeUsageInfo.isInUse = volumeUsageInfo.isInUse || isInUse + volumeUsageInfo.pods, volumeUsageInfo.isInUse, err = getPodsForPVC(ctx, pvcName, pvcNamespace, k8sClient) + if err != nil { + return nil, err + } + + if volumeUsageInfo.isInUse { + return &volumeUsageInfo, nil + } cfg, err := k8s.GetKubeConfig(ctx) if err != nil { return nil, err } - guestClusters, isInUse, err := getGuestClustersForPVC(ctx, pvcName, pvcNamespace, *cfg) + volumeUsageInfo.guestClusters, volumeUsageInfo.isInUse, err = getGuestClustersForPVC( + ctx, pvcName, pvcNamespace, *cfg) if err != nil { return nil, err } - volumeUsageInfo.guestClusters = guestClusters - volumeUsageInfo.isInUse = volumeUsageInfo.isInUse || isInUse + if volumeUsageInfo.isInUse { + return &volumeUsageInfo, nil + } - snapshots, isInUse, err := getSnapshotsForPVC(ctx, pvcName, pvcNamespace, *cfg) + volumeUsageInfo.snapshots, volumeUsageInfo.isInUse, err = getSnapshotsForPVC(ctx, pvcName, pvcNamespace, *cfg) if err != nil { return nil, err } - volumeUsageInfo.snapshots = snapshots - volumeUsageInfo.isInUse = volumeUsageInfo.isInUse || isInUse + if volumeUsageInfo.isInUse { + return &volumeUsageInfo, nil + } if ignoreVMUsage { log.Debugf("Skipping check for virtual machines using PVC %q in namespace %q as ignoreVMUsage is set to true", @@ -114,13 +126,11 @@ func _getVolumeUsageInfo(ctx context.Context, k8sClient clientset.Interface, pvc return &volumeUsageInfo, nil } - vms, isInUse, err := getVMsForPVC(ctx, pvcName, pvcNamespace, *cfg) + volumeUsageInfo.virtualMachines, volumeUsageInfo.isInUse, err = getVMsForPVC(ctx, pvcName, pvcNamespace, *cfg) if err != nil { return nil, err } - volumeUsageInfo.virtualMachines = vms - volumeUsageInfo.isInUse = volumeUsageInfo.isInUse || isInUse return &volumeUsageInfo, nil } @@ -162,6 +172,25 @@ func _getPVCName(ctx context.Context, volumeID string) (string, string, error) { return pvc, ns, nil } +var getVolumeID = _getVolumeID + +func _getVolumeID(ctx context.Context, pvcName, namespace string) (string, error) { + log := logger.GetLogger(ctx) + if commonco.ContainerOrchestratorUtility == nil { + err := errors.New("ContainerOrchestratorUtility is not initialized") + log.Warn(err) + return "", err + } + + volID, ok := commonco.ContainerOrchestratorUtility.GetVolumeIDFromPVCName(namespace, pvcName) + if !ok { + log.Infof("no volumeID found for PVC %q", pvcName) + } else { + log.Infof("volumeID %q found for PVC %q", volID, pvcName) + } + return volID, nil +} + // getPodsForPVC returns a list of pods that are using the specified PVC. func getPodsForPVC(ctx context.Context, pvcName string, pvcNamespace string, k8sClient clientset.Interface) ([]string, bool, error) { @@ -169,8 +198,9 @@ func getPodsForPVC(ctx context.Context, pvcName string, pvcNamespace string, // TODO: check if we can use informer cache list, err := k8sClient.CoreV1().Pods(pvcNamespace).List(ctx, metav1.ListOptions{}) if err != nil { - return nil, false, logger.LogNewErrorf(log, "Failed to list pods in namespace %q for PVC %q. Error: %q", + log.Warnf("Failed to list pods in namespace %q for PVC %q. Error: %q", pvcNamespace, pvcName, err.Error()) + return nil, false, errors.New("failed to list pods") } var pods []string @@ -197,17 +227,17 @@ func getSnapshotsForPVC(ctx context.Context, pvcName string, pvcNamespace string log := logger.GetLogger(ctx) c, err := snapshotclient.NewForConfig(&cfg) if err != nil { - return nil, false, logger.LogNewErrorf(log, - "Failed to initialize snapshot client for PVC %q in namespace %q. Error: %q", + log.Warnf("Failed to create snapshot client for PVC %q in namespace %q. Error: %q", pvcName, pvcNamespace, err.Error()) + return nil, false, errors.New("failed to create snapshot client") } // TODO: check if we can use informer cache list, err := c.SnapshotV1().VolumeSnapshots(pvcNamespace).List(ctx, metav1.ListOptions{}) if err != nil { - return nil, false, logger.LogNewErrorf(log, - "Failed to list VolumeSnapshots in namespace %q for PVC %q. Error: %q", + log.Warnf("Failed to list VolumeSnapshots in namespace %q for PVC %q. Error: %q", pvcNamespace, pvcName, err.Error()) + return nil, false, errors.New("failed to list VolumeSnapshots") } var snapshots []string @@ -243,9 +273,9 @@ func getGuestClustersForPVC(ctx context.Context, pvcName, pvcNamespace string, return nil, false, nil } - return nil, false, logger.LogNewErrorf(log, - "Failed to get CnsVolumeMetadata %q in namespace %q. Error: %q", + log.Warnf("Failed to get CnsVolumeMetadata %q in namespace %q. Error: %q", pvcName, pvcNamespace, err.Error()) + return nil, false, errors.New("failed to get CnsVolumeMetadata") } var gcs []string @@ -264,13 +294,13 @@ func getVMsForPVC(ctx context.Context, pvcName string, pvcNamespace string, cfg rest.Config) ([]string, bool, error) { c, err := k8s.NewClientForGroup(ctx, &cfg, vmoperatortypes.GroupName) if err != nil { - return nil, false, err + return nil, false, errors.New("failed to create client for virtual machine group") } // TODO: check if we can use informer cache list, err := utils.ListVirtualMachines(ctx, c, pvcNamespace) if err != nil { - return nil, false, err + return nil, false, errors.New("failed to list virtual machines") } var vms []string diff --git a/pkg/syncer/cnsoperator/types/types.go b/pkg/syncer/cnsoperator/types/types.go index 85eb98998c..8f160822f7 100644 --- a/pkg/syncer/cnsoperator/types/types.go +++ b/pkg/syncer/cnsoperator/types/types.go @@ -36,6 +36,14 @@ const ( // This finalizer is added to avoid deletion of such VolumeSnapshots directly from Supervisor. CNSSnapshotFinalizer = "cns.vmware.com/volumesnapshot-protection" + // CNSUnregisterVolumeFinalizer is the finalizer added to CNSUnregisterVolume CRs + // to handle deletion gracefully. + CNSUnregisterVolumeFinalizer = "cns.vmware.com/unregister-volume" + + // CNSUnregisterProtectionFinalizer is the finalizer on Supervisor PVC + // to avoid deletion of PVCs which are in the process of being unregistered. + CNSUnregisterProtectionFinalizer = "cns.vmware.com/unregister-protection" + // VSphereCSIDriverName is the vsphere CSI driver name VSphereCSIDriverName = "csi.vsphere.vmware.com"