Fix populators not repopulating pvc after it was deleted (kubevirt#3056)

* Add tests to verify pvc is recreated and repopulated after delete Signed-off-by: Shelly Kagan <[email protected]> * Create import/upload source CR in case population pod is not succeeded In case someone deleted the pvc a new pvc is created and the status of the dv is still succeded. when using populators in order for the population to restart and the dv state to change accordingly we need to retrigger the population, this depends on the existing of the source CR. That CR should exist as long as the pvc pod state is not succeeded. When the pvc is recreated the succeded pod state annotation was removed. Adjusted UT accordingly Signed-off-by: Shelly Kagan <[email protected]> * Adjust tests after review Move cloner test to a parallel test suite. Simplify import test. Signed-off-by: Shelly Kagan <[email protected]> --------- Signed-off-by: Shelly Kagan <[email protected]>
deckhouse · Jan 17, 2024 · 40a382d · 40a382d
1 parent 0fb2261
commit 40a382d
Show file tree

Hide file tree

Showing 9 changed files with 95 additions and 6 deletions.
diff --git a/pkg/controller/datavolume/controller-base.go b/pkg/controller/datavolume/controller-base.go
@@ -1154,6 +1154,14 @@ func (r *ReconcilerBase) shouldBeMarkedWaitForFirstConsumer(pvc *corev1.Persiste
 	return res, nil
 }
 
+func (r *ReconcilerBase) shouldReconcileVolumeSourceCR(syncState *dvSyncState) bool {
+	if syncState.pvc == nil {
+		return true
+	}
+	phase := syncState.pvc.Annotations[cc.AnnPodPhase]
+	return phase != string(corev1.PodSucceeded) || syncState.dvMutated.Status.Phase != cdiv1.Succeeded
+}
+
 // shouldBeMarkedPendingPopulation decides whether we should mark DV as PendingPopulation
 func (r *ReconcilerBase) shouldBeMarkedPendingPopulation(pvc *corev1.PersistentVolumeClaim) (bool, error) {
 	wffc, err := r.storageClassWaitForFirstConsumer(pvc.Spec.StorageClassName)

diff --git a/pkg/controller/datavolume/import-controller.go b/pkg/controller/datavolume/import-controller.go
@@ -200,7 +200,7 @@ func (r *ImportReconciler) syncImport(log logr.Logger, req reconcile.Request) (d
 
 	pvcModifier := r.updateAnnotations
 	if syncState.usePopulator {
-		if syncState.dvMutated.Status.Phase != cdiv1.Succeeded {
+		if r.shouldReconcileVolumeSourceCR(&syncState) {
 			err := r.reconcileVolumeImportSourceCR(&syncState)
 			if err != nil {
 				return syncState, err
@@ -221,15 +221,14 @@ func (r *ImportReconciler) syncImport(log logr.Logger, req reconcile.Request) (d
 }
 
 func (r *ImportReconciler) cleanup(syncState *dvSyncState) error {
-	dv := syncState.dvMutated
 	// The cleanup is to delete the volumeImportSourceCR which is used only with populators,
 	// it is owner by the DV so will be deleted when dv is deleted
 	// also we can already delete once dv is succeeded
 	usePopulator, err := checkDVUsingPopulators(syncState.dvMutated)
 	if err != nil {
 		return err
 	}
-	if usePopulator && dv.Status.Phase == cdiv1.Succeeded {
+	if usePopulator && !r.shouldReconcileVolumeSourceCR(syncState) {
 		return r.deleteVolumeImportSourceCR(syncState)
 	}
 

diff --git a/pkg/controller/datavolume/import-controller_test.go b/pkg/controller/datavolume/import-controller_test.go
@@ -159,6 +159,14 @@ var _ = Describe("All DataVolume Tests", func() {
 			Expect(err).ToNot(HaveOccurred())
 			Expect(dv.GetAnnotations()[AnnUsePopulator]).To(Equal("true"))
 
+			pvc := &corev1.PersistentVolumeClaim{}
+			err = reconciler.client.Get(context.TODO(), types.NamespacedName{Name: "test-dv", Namespace: metav1.NamespaceDefault}, pvc)
+			Expect(err).ToNot(HaveOccurred())
+
+			pvc.Annotations[AnnPodPhase] = string(corev1.PodSucceeded)
+			err = reconciler.client.Update(context.TODO(), pvc)
+			Expect(err).ToNot(HaveOccurred())
+
 			dv.Status.Phase = cdiv1.Succeeded
 			err = reconciler.client.Update(context.TODO(), dv)
 			Expect(err).ToNot(HaveOccurred())

diff --git a/pkg/controller/datavolume/upload-controller.go b/pkg/controller/datavolume/upload-controller.go
@@ -161,7 +161,7 @@ func (r *UploadReconciler) syncUpload(log logr.Logger, req reconcile.Request) (d
 
 	pvcModifier := r.updateAnnotations
 	if syncState.usePopulator {
-		if syncState.dvMutated.Status.Phase != cdiv1.Succeeded {
+		if r.shouldReconcileVolumeSourceCR(&syncState) {
 			err := r.createVolumeUploadSourceCR(&syncState)
 			if err != nil {
 				return syncState, err
@@ -177,15 +177,14 @@ func (r *UploadReconciler) syncUpload(log logr.Logger, req reconcile.Request) (d
 }
 
 func (r *UploadReconciler) cleanup(syncState *dvSyncState) error {
-	dv := syncState.dvMutated
 	// The cleanup is to delete the volumeUploadSourceCR which is used only with populators,
 	// it is owner by the DV so will be deleted when dv is deleted
 	// also we can already delete once dv is succeeded
 	usePopulator, err := checkDVUsingPopulators(syncState.dvMutated)
 	if err != nil {
 		return err
 	}
-	if usePopulator && dv.Status.Phase == cdiv1.Succeeded {
+	if usePopulator && !r.shouldReconcileVolumeSourceCR(syncState) {
 		return r.deleteVolumeUploadSourceCR(syncState)
 	}
 

diff --git a/pkg/controller/datavolume/upload-controller_test.go b/pkg/controller/datavolume/upload-controller_test.go
@@ -110,6 +110,14 @@ var _ = Describe("All DataVolume Tests", func() {
 		Expect(err).ToNot(HaveOccurred())
 		Expect(dv.GetAnnotations()[AnnUsePopulator]).To(Equal("true"))
 
+		pvc := &corev1.PersistentVolumeClaim{}
+		err = reconciler.client.Get(context.TODO(), types.NamespacedName{Name: "test-dv", Namespace: metav1.NamespaceDefault}, pvc)
+		Expect(err).ToNot(HaveOccurred())
+
+		pvc.Annotations[AnnPodPhase] = string(corev1.PodSucceeded)
+		err = reconciler.client.Update(context.TODO(), pvc)
+		Expect(err).ToNot(HaveOccurred())
+
 		dv.Status.Phase = cdiv1.Succeeded
 		err = reconciler.client.Update(context.TODO(), dv)
 		Expect(err).ToNot(HaveOccurred())

diff --git a/tests/cloner_test.go b/tests/cloner_test.go
@@ -2503,6 +2503,33 @@ var _ = Describe("all clone tests", func() {
 				Expect(dv.Status.RestartCount).To(BeNumerically("==", 0))
 			}
 		})
+
+		It("should recreate and reclone target pvc if it was deleted", func() {
+			pvcDef := utils.NewPVCDefinition(sourcePVCName, "1Gi", nil, nil)
+			pvcDef.Namespace = f.Namespace.Name
+			sourcePvc = f.CreateAndPopulateSourcePVC(pvcDef, sourcePodFillerName, fillCommand+testFile+"; chmod 660 "+testBaseDir+testFile)
+			dvName := "target-dv"
+			doFileBasedCloneTest(f, pvcDef, f.Namespace, dvName, "1Gi")
+
+			targetPVC, err := f.FindPVC(dvName)
+			Expect(err).ToNot(HaveOccurred())
+
+			By("Delete target PVC")
+			err = utils.DeletePVC(f.K8sClient, f.Namespace.Name, dvName)
+			Expect(err).ToNot(HaveOccurred())
+
+			deleted, err := f.WaitPVCDeletedByUID(targetPVC, time.Minute)
+			Expect(err).ToNot(HaveOccurred())
+			Expect(deleted).To(BeTrue())
+
+			targetPVC, err = utils.WaitForPVC(f.K8sClient, f.Namespace.Name, dvName)
+			Expect(err).ToNot(HaveOccurred())
+			f.ForceBindIfWaitForFirstConsumer(targetPVC)
+
+			By("Verify target PVC is bound again")
+			err = utils.WaitForPersistentVolumeClaimPhase(f.K8sClient, f.Namespace.Name, v1.ClaimBound, dvName)
+			Expect(err).ToNot(HaveOccurred())
+		})
 	})
 
 	var _ = Describe("Preallocation", Serial, func() {

diff --git a/tests/import_test.go b/tests/import_test.go
@@ -2087,6 +2087,40 @@ var _ = Describe("Import populator", func() {
 		err = f.DeletePVC(pvcPrime)
 		Expect(err).ToNot(HaveOccurred())
 	})
+
+	It("should recreate and reimport pvc if it was deleted", func() {
+		dataVolume := utils.NewDataVolumeWithHTTPImport("import-dv", "100Mi", fmt.Sprintf(utils.TinyCoreIsoURL, f.CdiInstallNs))
+		dv, err := utils.CreateDataVolumeFromDefinition(f.CdiClient, f.Namespace.Name, dataVolume)
+		Expect(err).ToNot(HaveOccurred())
+
+		pvc, err = utils.WaitForPVC(f.K8sClient, dv.Namespace, dv.Name)
+		Expect(err).ToNot(HaveOccurred())
+		f.ForceBindIfWaitForFirstConsumer(pvc)
+
+		By("Wait for import to be completed")
+		err = utils.WaitForDataVolumePhase(f, dv.Namespace, cdiv1.Succeeded, dv.Name)
+		Expect(err).ToNot(HaveOccurred(), "Datavolume not in phase succeeded in time")
+
+		By("Delete PVC and wait for it to be deleted")
+		err = f.DeletePVC(pvc)
+		Expect(err).ToNot(HaveOccurred())
+		deleted, err := f.WaitPVCDeletedByUID(pvc, time.Minute)
+		Expect(err).ToNot(HaveOccurred())
+		Expect(deleted).To(BeTrue())
+
+		pvc, err = utils.WaitForPVC(f.K8sClient, dv.Namespace, dv.Name)
+		Expect(err).ToNot(HaveOccurred())
+		f.ForceBindIfWaitForFirstConsumer(pvc)
+
+		By("Verify target PVC is bound again")
+		err = utils.WaitForPersistentVolumeClaimPhase(f.K8sClient, pvc.Namespace, v1.ClaimBound, pvc.Name)
+		Expect(err).ToNot(HaveOccurred())
+
+		By("Verify content")
+		md5, err := f.GetMD5(f.Namespace, pvc, utils.DefaultImagePath, utils.MD5PrefixSize)
+		Expect(err).ToNot(HaveOccurred())
+		Expect(md5).To(Equal(utils.TinyCoreMD5))
+	})
 })
 
 func generateRegistryOnlySidecar() *unstructured.Unstructured {

diff --git a/tests/utils/datavolume.go b/tests/utils/datavolume.go
@@ -895,6 +895,9 @@ func WaitForDataVolumePhaseWithTimeout(ci ClientsIface, namespace string, phase
 	err := wait.PollImmediate(dataVolumePollInterval, timeout, func() (bool, error) {
 		dataVolume, err := ci.Cdi().CdiV1beta1().DataVolumes(namespace).Get(context.TODO(), dataVolumeName, metav1.GetOptions{})
 		if err != nil || dataVolume.Status.Phase != phase {
+			if err != nil {
+				fmt.Fprintf(ginkgo.GinkgoWriter, "Error: failed get datavolume: %s, %s\n", dataVolumeName, err.Error())
+			}
 			actualPhase = dataVolume.Status.Phase
 			return false, err
 		}

diff --git a/tests/utils/pvc.go b/tests/utils/pvc.go
@@ -215,6 +215,9 @@ func WaitForPersistentVolumeClaimPhase(clientSet *kubernetes.Clientset, namespac
 		pvc, err := clientSet.CoreV1().PersistentVolumeClaims(namespace).Get(context.TODO(), pvcName, metav1.GetOptions{})
 		fmt.Fprintf(ginkgo.GinkgoWriter, "INFO: Checking PVC phase: %s, resource version %s\n", string(pvc.Status.Phase), pvc.ResourceVersion)
 		if err != nil || pvc.Status.Phase != phase {
+			if err != nil {
+				fmt.Fprintf(ginkgo.GinkgoWriter, "ERROR: checking pvc %s phase failed: %s\n", pvcName, err)
+			}
 			return false, err
 		}
 		return true, nil