[release-v1.58] Fix populators not repopulating pvc after it was dele…

…ted (kubevirt#3071)

* Add tests to verify pvc is recreated and repopulated after delete

Signed-off-by: Shelly Kagan <skagan@redhat.com>

* 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 <skagan@redhat.com>

* Adjust tests after review

Move cloner test to a parallel test suite.
Simplify import test.

Signed-off-by: Shelly Kagan <skagan@redhat.com>

---------

Signed-off-by: Shelly Kagan <skagan@redhat.com>
Co-authored-by: Shelly Kagan <skagan@redhat.com>

pkg/controller/datavolume/controller-base.go

@@ -1163,6 +1163,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)

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)
  }
 

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())

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)
  }
 

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())

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", func() {

tests/import_test.go

@@ -2082,6 +2082,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 {

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
  }

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