Fix PVC deletion logic in populators (#3362)

* Fix PVC' deletion logic in CDI populators This commit ensures that we only proceed with PVC' deletion after confirming that the target PVC is rebound. Signed-off-by: Alvaro Romero <[email protected]> * Refactor IsBound so we check PVC status instead of spec PVC status should be the definitive source to know whether the PVC is bound or not. This commit updates our IsBound function to use status instead of spec. It also updates unit tests and related code to match the new behavior. Signed-off-by: Alvaro Romero <[email protected]> * Add check for lost target PVC in populators Signed-off-by: Alvaro Romero <[email protected]> --------- Signed-off-by: Alvaro Romero <[email protected]>
kubevirt · Aug 22, 2024 · 4c911fe · 4c911fe
1 parent 43f4270
commit 4c911fe
Show file tree

Hide file tree

Showing 13 changed files with 36 additions and 19 deletions.
diff --git a/pkg/controller/clone/csi-clone_test.go b/pkg/controller/clone/csi-clone_test.go
@@ -177,6 +177,7 @@ var _ = Describe("CSIClonePhase test", func() {
 			It("should succeed (immediate bind)", func() {
 				desired := getDesiredClaim()
 				desired.Spec.VolumeName = "vol"
+				desired.Status.Phase = corev1.ClaimBound
 				p := createCSIClonePhase(sourceClaim, getStorageClass(), desired)
 
 				result, err := p.Reconcile(context.Background())

diff --git a/pkg/controller/clone/planner_test.go b/pkg/controller/clone/planner_test.go
@@ -186,6 +186,7 @@ var _ = Describe("Planner test", func() {
 	createSourceClaim := func() *corev1.PersistentVolumeClaim {
 		s := createClaim(sourceName)
 		s.Spec.VolumeName = volumeName
+		s.Status.Phase = corev1.ClaimBound
 		s.Status.Capacity = corev1.ResourceList{
 			corev1.ResourceStorage: s.Spec.Resources.Requests[corev1.ResourceStorage],
 		}

diff --git a/pkg/controller/clone/prep-claim.go b/pkg/controller/clone/prep-claim.go
@@ -72,11 +72,6 @@ func (p *PrepClaimPhase) Reconcile(ctx context.Context) (*reconcile.Result, erro
 
 	if !hasActual {
 		if cc.IsBound(actualClaim) {
-			// PVC is bound but its status hasn't been updated yet.
-			// We'll reconcile again once the status is updated.
-			if actualClaim.Status.Phase == corev1.ClaimPending {
-				return &reconcile.Result{}, nil
-			}
 			return nil, fmt.Errorf("actual PVC size missing")
 		}
 

diff --git a/pkg/controller/clone/prep-claim_test.go b/pkg/controller/clone/prep-claim_test.go
@@ -226,6 +226,7 @@ var _ = Describe("PrepClaimPhase test", func() {
 			claim := getClaim()
 			cc.AddAnnotation(claim, cc.AnnSelectedNode, "node1")
 			claim.Spec.Resources.Requests[corev1.ResourceStorage] = defaultRequestSize
+			claim.Status.Phase = corev1.ClaimPending
 
 			p := createPrepClaimPhase(claim)
 

diff --git a/pkg/controller/clone/rebind_test.go b/pkg/controller/clone/rebind_test.go
@@ -107,6 +107,7 @@ var _ = Describe("RebindPhase test", func() {
 		It("should succeed if target is bound", func() {
 			target := createTarget()
 			target.Spec.VolumeName = pvName
+			target.Status.Phase = corev1.ClaimBound
 
 			p := createRebindPhase(target)
 			result, err := p.Reconcile(context.Background())
@@ -158,6 +159,7 @@ var _ = Describe("RebindPhase test", func() {
 				volume := createVolume()
 				source := createSource()
 				source.Spec.VolumeName = volume.Name
+				source.Status.Phase = corev1.ClaimBound
 				p := createRebindPhase(createTarget(), source, volume)
 				result, err := p.Reconcile(context.Background())
 				Expect(err).ToNot(HaveOccurred())
@@ -178,6 +180,7 @@ var _ = Describe("RebindPhase test", func() {
 				volume.Spec.ClaimRef.Name = "foo"
 				source := createSource()
 				source.Spec.VolumeName = volume.Name
+				source.Status.Phase = corev1.ClaimBound
 				p := createRebindPhase(createTarget(), source, volume)
 				result, err := p.Reconcile(context.Background())
 				Expect(err).To(HaveOccurred())

diff --git a/pkg/controller/clone/snap-clone_test.go b/pkg/controller/clone/snap-clone_test.go
@@ -197,6 +197,7 @@ var _ = Describe("SnapshotClonePhase test", func() {
 			It("should succeed (immediate bind)", func() {
 				desired := getDesiredClaim()
 				desired.Spec.VolumeName = "vol"
+				desired.Status.Phase = corev1.ClaimBound
 				p := createSnapshotClonePhase(getStorageClass(), desired)
 
 				result, err := p.Reconcile(context.Background())

diff --git a/pkg/controller/common/util.go b/pkg/controller/common/util.go
@@ -1525,14 +1525,19 @@ func InflateSizeWithOverhead(ctx context.Context, c client.Client, imgSize int64
 
 // IsBound returns if the pvc is bound
 func IsBound(pvc *corev1.PersistentVolumeClaim) bool {
-	return pvc.Spec.VolumeName != ""
+	return pvc != nil && pvc.Status.Phase == corev1.ClaimBound
 }
 
 // IsUnbound returns if the pvc is not bound yet
 func IsUnbound(pvc *corev1.PersistentVolumeClaim) bool {
 	return !IsBound(pvc)
 }
 
+// IsLost returns if the pvc is lost
+func IsLost(pvc *corev1.PersistentVolumeClaim) bool {
+	return pvc != nil && pvc.Status.Phase == corev1.ClaimLost
+}
+
 // IsImageStream returns true if registry source is ImageStream
 func IsImageStream(pvc *corev1.PersistentVolumeClaim) bool {
 	return pvc.Annotations[AnnRegistryImageStream] == "true"

diff --git a/pkg/controller/datavolume/static-volume_test.go b/pkg/controller/datavolume/static-volume_test.go
@@ -255,6 +255,7 @@ var _ = Describe("checkStaticVolume Tests", func() {
 		pvc := newPendingPVC(dv, pv)
 		pvc.Spec.VolumeName = pv.Name
 		pv.Status.Phase = corev1.VolumeBound
+		pvc.Status.Phase = corev1.ClaimBound
 		reconciler, client := args.newReconciler(dv, pv, pvc)
 		_, err := reconciler.Reconcile(context.TODO(), requestFunc(dv))
 		Expect(err).ToNot(HaveOccurred())
@@ -278,6 +279,7 @@ var _ = Describe("checkStaticVolume Tests", func() {
 		pvc := newPendingPVC(dv, pv)
 		pvc.Spec.VolumeName = "foobar"
 		pv.Status.Phase = corev1.VolumeBound
+		pvc.Status.Phase = corev1.ClaimBound
 		reconciler, client := args.newReconciler(dv, pv, pvc)
 		_, err := reconciler.Reconcile(context.TODO(), requestFunc(dv))
 		Expect(err).To(HaveOccurred())

diff --git a/pkg/controller/populators/forklift-populator_test.go b/pkg/controller/populators/forklift-populator_test.go
@@ -291,6 +291,8 @@ var _ = Describe("Forklift populator tests", func() {
 			AnnPodRetainAfterCompletion: "true",
 			AnnPodPhase:                 string(corev1.PodSucceeded),
 		}
+		pvcPrime.Status = corev1.PersistentVolumeClaimStatus{Phase: corev1.ClaimLost}
+
 		pv := &corev1.PersistentVolume{
 			ObjectMeta: metav1.ObjectMeta{
 				Name: "pv",

diff --git a/pkg/controller/populators/import-populator_test.go b/pkg/controller/populators/import-populator_test.go
@@ -291,6 +291,8 @@ var _ = Describe("Import populator tests", func() {
 			volumeImportSource := getVolumeImportSource(true, metav1.NamespaceDefault)
 			pvcPrime := getPVCPrime(targetPvc, nil)
 			pvcPrime.Annotations = map[string]string{AnnPodRetainAfterCompletion: "true"}
+			targetPvc.Status = corev1.PersistentVolumeClaimStatus{Phase: corev1.ClaimBound}
+			pvcPrime.Status = corev1.PersistentVolumeClaimStatus{Phase: corev1.ClaimLost}
 
 			By("Reconcile")
 			reconciler = createImportPopulatorReconciler(targetPvc, pvcPrime, volumeImportSource, sc)

diff --git a/pkg/controller/populators/populator-base.go b/pkg/controller/populators/populator-base.go
@@ -285,8 +285,8 @@ func (r *ReconcilerBase) reconcile(req reconcile.Request, populator populatorCon
 		return res, err
 	}
 
-	// Making sure to clean PVC once it is complete
-	if cc.IsPVCComplete(pvc) && !cc.IsMultiStageImportInProgress(pvc) {
+	// Making sure to clean PVC' once population is completed
+	if cc.IsPVCComplete(pvc) && cc.IsBound(pvc) && !cc.IsMultiStageImportInProgress(pvc) {
 		res, err = r.reconcileCleanup(pvcPrime)
 	}
 
@@ -332,10 +332,10 @@ func (r *ReconcilerBase) reconcileCommon(pvc *corev1.PersistentVolumeClaim, popu
 		return nil, err
 	}
 
-	// If PVC' doesn't exist and target PVC is not bound, we should create the PVC' to start the population.
+	// If PVC' doesn't exist and target PVC is rebindable, we should create the PVC' to start the population.
 	// We still return nil as we'll get called again once PVC' exists.
 	// If target PVC is bound, we don't really need to populate anything.
-	if cc.IsUnbound(pvc) {
+	if cc.IsUnbound(pvc) && !cc.IsLost(pvc) {
 		_, err := r.createPVCPrime(pvc, populationSource, nodeName != "", populator.updatePVCForPopulation)
 		if err != nil {
 			r.recorder.Eventf(pvc, corev1.EventTypeWarning, errCreatingPVCPrime, err.Error())
@@ -347,14 +347,16 @@ func (r *ReconcilerBase) reconcileCommon(pvc *corev1.PersistentVolumeClaim, popu
 }
 
 func (r *ReconcilerBase) reconcileCleanup(pvcPrime *corev1.PersistentVolumeClaim) (reconcile.Result, error) {
-	if pvcPrime != nil {
-		if pvcPrime.Annotations[cc.AnnPodRetainAfterCompletion] == "true" {
-			// Retaining PVC' in Lost state. We can then keep the pod for debugging purposes.
-			r.recorder.Eventf(pvcPrime, corev1.EventTypeWarning, retainedPVCPrime, messageRetainedPVCPrime)
-		} else {
-			if err := r.client.Delete(context.TODO(), pvcPrime); err != nil {
-				return reconcile.Result{}, err
-			}
+	// If exists, make sure PVC' is rebound before deletion
+	if pvcPrime == nil || pvcPrime.Status.Phase != corev1.ClaimLost {
+		return reconcile.Result{}, nil
+	}
+	if pvcPrime.Annotations[cc.AnnPodRetainAfterCompletion] == "true" {
+		// Retaining PVC' in Lost state. We can then keep the pod for debugging purposes.
+		r.recorder.Eventf(pvcPrime, corev1.EventTypeWarning, retainedPVCPrime, messageRetainedPVCPrime)
+	} else {
+		if err := r.client.Delete(context.TODO(), pvcPrime); err != nil {
+			return reconcile.Result{}, err
 		}
 	}
 	return reconcile.Result{}, nil

diff --git a/pkg/controller/populators/upload-populator_test.go b/pkg/controller/populators/upload-populator_test.go
@@ -169,6 +169,8 @@ var _ = Describe("Datavolume controller reconcile loop", func() {
 		pvc.Spec.VolumeName = "test-pv"
 		cc.AddAnnotation(pvc, cc.AnnPodPhase, string(corev1.PodSucceeded))
 		pvcPrime := newUploadPopulatorPVC(PVCPrimeName(pvc))
+		pvc.Status = corev1.PersistentVolumeClaimStatus{Phase: corev1.ClaimBound}
+		pvcPrime.Status = corev1.PersistentVolumeClaimStatus{Phase: corev1.ClaimLost}
 
 		volumeUploadSourceCR := newUploadPopulatorCR("", false)
 		scName := "test-sc"

diff --git a/pkg/controller/transfer/pvc.go b/pkg/controller/transfer/pvc.go
@@ -43,7 +43,7 @@ func (h *pvcTransferHandler) ReconcilePending(ot *cdiv1.ObjectTransfer) (time.Du
 		return 0, nil
 	}
 
-	if cc.IsUnbound(pvc) || pvc.Status.Phase != corev1.ClaimBound {
+	if cc.IsUnbound(pvc) {
 		if err := h.reconciler.setAndUpdateCompleteCondition(ot, corev1.ConditionFalse, "PVC not bound", ""); err != nil {
 			return 0, err
 		}