From 4c911fed27b62dd6dc01a3069eacabaa40363a37 Mon Sep 17 00:00:00 2001 From: alromeros Date: Thu, 22 Aug 2024 22:22:35 +0200 Subject: [PATCH] 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 * 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 * Add check for lost target PVC in populators Signed-off-by: Alvaro Romero --------- Signed-off-by: Alvaro Romero --- pkg/controller/clone/csi-clone_test.go | 1 + pkg/controller/clone/planner_test.go | 1 + pkg/controller/clone/prep-claim.go | 5 ---- pkg/controller/clone/prep-claim_test.go | 1 + pkg/controller/clone/rebind_test.go | 3 +++ pkg/controller/clone/snap-clone_test.go | 1 + pkg/controller/common/util.go | 7 ++++- .../datavolume/static-volume_test.go | 2 ++ .../populators/forklift-populator_test.go | 2 ++ .../populators/import-populator_test.go | 2 ++ pkg/controller/populators/populator-base.go | 26 ++++++++++--------- .../populators/upload-populator_test.go | 2 ++ pkg/controller/transfer/pvc.go | 2 +- 13 files changed, 36 insertions(+), 19 deletions(-) diff --git a/pkg/controller/clone/csi-clone_test.go b/pkg/controller/clone/csi-clone_test.go index 6ce5e140ec..4708331005 100644 --- 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 index 3aab275538..178dcb69fc 100644 --- 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 index b789053054..9317b74298 100644 --- 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 index 7ae4df30c5..f1e3e247fc 100644 --- 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 index 38acf19289..08b6dae832 100644 --- 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 index f4f2003089..a65bf8bbb5 100644 --- 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 index 86f15eb9f3..1732b330c3 100644 --- a/pkg/controller/common/util.go +++ b/pkg/controller/common/util.go @@ -1525,7 +1525,7 @@ 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 @@ -1533,6 +1533,11 @@ 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 index f11f484990..547618a64e 100644 --- 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 index 60ebe5da62..7b8f198a2f 100644 --- 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 index be24268a39..4403981bd5 100644 --- 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 index f5ba10d0b6..a1b4e6cc89 100644 --- 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 index 22d27b93a1..e31de6c1b3 100644 --- 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 index 34717d30b9..a7b6fd84b3 100644 --- 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 }