From ef67399611d9f05a45ad35dfae0c8477d132d01f Mon Sep 17 00:00:00 2001
From: Arik Hadas <ahadas@redhat.com>
Date: Thu, 19 Oct 2023 10:39:55 +0300
Subject: [PATCH] fix error handling in DeletePVCs

To return an error when we can't get a prime PVC rather than continue to
the next PVC.

Also refactored the code a bit to reduce cognitive complexity.

Signed-off-by: Arik Hadas <ahadas@redhat.com>
---
 pkg/controller/plan/kubevirt.go  | 39 ++++++++++-----------
 pkg/controller/plan/migration.go | 59 +++++++++++++++-----------------
 2 files changed, 46 insertions(+), 52 deletions(-)

diff --git a/pkg/controller/plan/kubevirt.go b/pkg/controller/plan/kubevirt.go
index 4dfc6704a..451d39ee4 100644
--- a/pkg/controller/plan/kubevirt.go
+++ b/pkg/controller/plan/kubevirt.go
@@ -932,38 +932,37 @@ func (r *KubeVirt) SetPopulatorPodOwnership(vm *plan.VMStatus) (err error) {
 }
 
 // DeletePVCs deletes PVCs associated with the VM, including prime PVCs (populator flows)
-func (r *KubeVirt) DeletePVCs(vm *plan.VMStatus) (err error) {
+func (r *KubeVirt) DeletePVCs(vm *plan.VMStatus) error {
 	pvcs, err := r.getPVCs(vm.Ref)
+	if err != nil {
+		return err
+	}
 	for _, pvc := range pvcs {
 		primePVC := core.PersistentVolumeClaim{}
 		err = r.Destination.Client.Get(context.TODO(), client.ObjectKey{Namespace: r.Plan.Spec.TargetNamespace, Name: fmt.Sprintf("prime-%s", string(pvc.UID))}, &primePVC)
-		if err != nil {
-			if k8serr.IsNotFound(err) {
-				err = nil
+		switch {
+		case err != nil && !k8serr.IsNotFound(err):
+			return err
+		case err == nil:
+			err = r.DeleteObject(&primePVC, vm, "Deleted prime PVC.", "pvc")
+			if err != nil && !k8serr.IsNotFound(err) {
+				return err
 			}
-			continue
-		}
-		// Best effort deletion
-		err = r.DeleteObject(&primePVC, vm, "Deleted prime PVC.", "pvc")
-		if err != nil && k8serr.IsNotFound(err) {
-			err = nil
-		}
-		if err != nil {
-			return
 		}
+
 		err = r.DeleteObject(&pvc, vm, "Deleted PVC.", "pvc")
-		if err != nil && k8serr.IsNotFound(err) {
-			err = nil
-		}
-		if err != nil {
-			return
+		if err != nil && !k8serr.IsNotFound(err) {
+			return err
 		}
+
 		pvcCopy := pvc.DeepCopy()
 		pvc.Finalizers = nil
 		patch := client.MergeFrom(pvcCopy)
-		err = r.Destination.Client.Patch(context.TODO(), &pvc, patch)
+		if err = r.Destination.Client.Patch(context.TODO(), &pvc, patch); err != nil {
+			return err
+		}
 	}
-	return
+	return nil
 }
 
 // Delete any populator pods that belong to a VM's migration.
diff --git a/pkg/controller/plan/migration.go b/pkg/controller/plan/migration.go
index f674433f0..77ca26a58 100644
--- a/pkg/controller/plan/migration.go
+++ b/pkg/controller/plan/migration.go
@@ -424,68 +424,63 @@ func (r *Migration) Cancel() (err error) {
 }
 
 func (r *Migration) cleanUpPopulatorPVCs(vm *plan.VMStatus) (err error) {
-	err = r.kubevirt.DeletePVCs(vm)
+	if r.builder.SupportsVolumePopulators() {
+		err = r.kubevirt.DeletePVCs(vm)
+	}
 	return
 }
 
 // Delete left over migration resources associated with a VM.
 func (r *Migration) cleanup(vm *plan.VMStatus) (err error) {
 	if !vm.HasCondition(Succeeded) {
-		err = r.kubevirt.DeleteVM(vm)
-		if err != nil {
+		if err = r.kubevirt.DeleteVM(vm); err != nil {
 			return
 		}
-		if r.builder.SupportsVolumePopulators() {
-			err = r.cleanUpPopulatorPVCs(vm)
-			if err != nil {
-				return
-			}
+		if err = r.cleanUpPopulatorPVCs(vm); err != nil {
+			return
 		}
 	}
-	err = r.deleteImporterPods(vm)
-	if err != nil {
+	if err = r.deleteImporterPods(vm); err != nil {
 		return
 	}
-	err = r.kubevirt.DeletePVCConsumerPod(vm)
-	if err != nil {
+	if err = r.kubevirt.DeletePVCConsumerPod(vm); err != nil {
 		return
 	}
-	err = r.kubevirt.DeleteGuestConversionPod(vm)
-	if err != nil {
+	if err = r.kubevirt.DeleteGuestConversionPod(vm); err != nil {
 		return
 	}
-	err = r.kubevirt.DeleteSecret(vm)
-	if err != nil {
+	if err = r.kubevirt.DeleteSecret(vm); err != nil {
 		return
 	}
-	err = r.kubevirt.DeleteConfigMap(vm)
-	if err != nil {
+	if err = r.kubevirt.DeleteConfigMap(vm); err != nil {
 		return
 	}
-	err = r.kubevirt.DeleteHookJobs(vm)
-	if err != nil {
+	if err = r.kubevirt.DeleteHookJobs(vm); err != nil {
 		return
 	}
-	err = r.destinationClient.DeletePopulatorDataSource(vm)
-	if err != nil {
+	if err = r.destinationClient.DeletePopulatorDataSource(vm); err != nil {
 		return
 	}
-	err = r.kubevirt.DeletePopulatorPods(vm)
-	if err != nil {
+	if err = r.kubevirt.DeletePopulatorPods(vm); err != nil {
 		return
 	}
-	if vm.Warm != nil {
-		if errLocal := r.provider.RemoveSnapshots(vm.Ref, vm.Warm.Precopies); errLocal != nil {
-			r.Log.Error(
-				errLocal,
-				"Failed to clean up warm migration snapshots.",
-				"vm", vm)
-		}
-	}
+	r.removeWarmSnapshots(vm)
 
 	return
 }
 
+func (r *Migration) removeWarmSnapshots(vm *plan.VMStatus) {
+	if vm.Warm == nil {
+		return
+	}
+	if err := r.provider.RemoveSnapshots(vm.Ref, vm.Warm.Precopies); err != nil {
+		r.Log.Error(
+			err,
+			"Failed to clean up warm migration snapshots.",
+			"vm", vm)
+	}
+}
+
 func (r *Migration) deleteImporterPods(vm *plan.VMStatus) (err error) {
 	if r.vmMap == nil {
 		r.vmMap, err = r.kubevirt.VirtualMachineMap()