Fix race condition in populators so we clean PVC' after population in…

… all cases (#2833)

This commit fixes a race condition in CDI populators so we attempt to get PVC' even if the population source doesn't exist.

This new behavior allows us to correctly delete the PVC' once the population has succeeded, even if the population source doesn't exist anymore.

Signed-off-by: Alvaro Romero <alromero@redhat.com>

pkg/controller/populators/import-populator_test.go

@@ -174,11 +174,9 @@ var _ = Describe("Import populator tests", func() {
 			targetPvc := CreatePvcInStorageClass(targetPvcName, metav1.NamespaceDefault, &sc.Name, nil, nil, corev1.ClaimPending)
 			targetPvc.Spec.DataSourceRef = namespacedDataSourceRef
 			volumeImportSource := getVolumeImportSource(true, nsName)
-			pvcPrime := getPVCPrime(targetPvc, nil)
-			pvcPrime.Annotations = map[string]string{AnnPodPhase: string(corev1.PodSucceeded)}
 
 			By("Reconcile")
-			reconciler = createImportPopulatorReconciler(targetPvc, pvcPrime, volumeImportSource, sc)
+			reconciler = createImportPopulatorReconciler(targetPvc, volumeImportSource, sc)
 			result, err := reconciler.Reconcile(context.TODO(), reconcile.Request{NamespacedName: types.NamespacedName{Name: targetPvcName, Namespace: metav1.NamespaceDefault}})
 			Expect(err).To(Not(HaveOccurred()))
 			Expect(result).To(Not(BeNil()))

pkg/controller/populators/populator-base.go

@@ -279,6 +279,16 @@ func (r *ReconcilerBase) reconcileCommon(pvc *corev1.PersistentVolumeClaim, popu
 		return nil, nil
 	}
 
+	// Get the PVC'. If it does exist, return it and skip the rest of the checks
+	// If it doesn't, we'll attempt to create it.
+	pvcPrime, err := r.getPVCPrime(pvc)
+	if err != nil {
+		return nil, err
+	}
+	if pvcPrime != nil {
+		return pvcPrime, nil
+	}
+
 	// We should ignore PVCs that aren't for this populator to handle
 	dataSourceRef := pvc.Spec.DataSourceRef
 	if !IsPVCDataSourceRefKind(pvc, r.sourceKind) {
@@ -303,24 +313,18 @@ func (r *ReconcilerBase) reconcileCommon(pvc *corev1.PersistentVolumeClaim, popu
 		return nil, err
 	}
 
-	// Get the PVC'
-	pvcPrime, err := r.getPVCPrime(pvc)
-	if err != nil {
-		return nil, err
-	}
-
 	// If PVC' doesn't exist and target PVC is not bound, we should create the PVC' to start the population.
-	// We still return the nil PVC' as we'll get called again once PVC' exists.
+	// 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 pvcPrime == nil && cc.IsUnbound(pvc) {
+	if cc.IsUnbound(pvc) {
 		_, err := r.createPVCPrime(pvc, populationSource, nodeName != "", populator.updatePVCForPopulation)
 		if err != nil {
 			r.recorder.Eventf(pvc, corev1.EventTypeWarning, errCreatingPVCPrime, err.Error())
 			return nil, err
 		}
 	}
 
-	return pvcPrime, nil
+	return nil, nil
 }
 
 func (r *ReconcilerBase) reconcileCleanup(pvcPrime *corev1.PersistentVolumeClaim) (reconcile.Result, error) {

tests/import_test.go

@@ -1875,6 +1875,44 @@ var _ = Describe("Import populator", func() {
 			return err != nil && k8serrors.IsNotFound(err)
 		}, timeout, pollingInterval).Should(BeTrue())
 	})
+
+	It("should continue normally with the population even if the volueImportSource is deleted", func() {
+		pvc = importPopulationPVCDefinition()
+		controller.AddAnnotation(pvc, controller.AnnImmediateBinding, "")
+		pvc, err = f.CreatePVCFromDefinition(pvc)
+		Expect(err).ToNot(HaveOccurred())
+		err = createHTTPImportPopulatorCR(cdiv1.DataVolumeKubeVirt, true)
+		Expect(err).ToNot(HaveOccurred())
+
+		By("Verify PVC prime was created")
+		pvcPrime, err = utils.WaitForPVC(f.K8sClient, pvc.Namespace, populators.PVCPrimeName(pvc))
+		Expect(err).ToNot(HaveOccurred())
+
+		err = f.CdiClient.CdiV1beta1().VolumeImportSources(f.Namespace.Name).Delete(context.TODO(), "import-populator-test", metav1.DeleteOptions{})
+		Expect(err).ToNot(HaveOccurred())
+
+		By("Verify target PVC is bound")
+		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))
+
+		By("Verify 100.0% annotation")
+		progress, ok, err := utils.WaitForPVCAnnotation(f.K8sClient, f.Namespace.Name, pvc, controller.AnnPopulatorProgress)
+		Expect(err).ToNot(HaveOccurred())
+		Expect(ok).To(BeTrue())
+		Expect(progress).Should(BeEquivalentTo("100.0%"))
+
+		By("Wait for PVC prime to be deleted")
+		Eventually(func() bool {
+			// Make sure pvcPrime was deleted after import population
+			_, err := f.FindPVC(pvcPrime.Name)
+			return err != nil && k8serrors.IsNotFound(err)
+		}, timeout, pollingInterval).Should(BeTrue())
+	})
 })
 
 func generateRegistryOnlySidecar() *unstructured.Unstructured {

tests/upload_test.go

@@ -588,6 +588,56 @@ var _ = Describe("[rfe_id:138][crit:high][vendor:cnv-qe@redhat.com][level:compon
 				Expect(err).ToNot(HaveOccurred())
 				Expect(deleted).To(BeTrue())
 			})
+
+			It("should cleanup appropriately even without volumeUploadSource", func() {
+				pvcDef := utils.UploadPopulationPVCDefinition()
+				controller.AddAnnotation(pvcDef, controller.AnnImmediateBinding, "")
+				pvc, err = f.CreatePVCFromDefinition(pvcDef)
+				Expect(err).ToNot(HaveOccurred())
+
+				By("Verify PVC prime was created")
+				pvcPrime, err = utils.WaitForPVC(f.K8sClient, pvc.Namespace, populators.PVCPrimeName(pvc))
+				Expect(err).ToNot(HaveOccurred())
+
+				err = f.DynamicClient.Resource(uploadSourceGVR).Namespace(f.Namespace.Name).Delete(context.TODO(), "upload-populator-test", metav1.DeleteOptions{})
+				Expect(err).ToNot(HaveOccurred())
+
+				By("Verify PVC prime annotation says ready")
+				found, err := utils.WaitPVCPodStatusReady(f.K8sClient, pvcPrime)
+				Expect(err).ToNot(HaveOccurred())
+				Expect(found).To(BeTrue())
+
+				checkUploadCertSecrets(pvcPrime)
+
+				By("Get an upload token")
+				token, err := utils.RequestUploadToken(f.CdiClient, pvc)
+				Expect(err).ToNot(HaveOccurred())
+				Expect(token).ToNot(BeEmpty())
+
+				By("Do upload")
+				Eventually(func() bool {
+					err = uploadImage(uploadProxyURL, token, http.StatusOK)
+					if err != nil {
+						fmt.Fprintf(GinkgoWriter, "ERROR: %s\n", err.Error())
+						return false
+					}
+					return true
+				}, timeout, 5*time.Second).Should(BeTrue(), "Upload should eventually succeed, even if initially pod is not ready")
+
+				By("Verify target PVC is bound")
+				err = utils.WaitForPersistentVolumeClaimPhase(f.K8sClient, pvc.Namespace, v1.ClaimBound, pvc.Name)
+				Expect(err).ToNot(HaveOccurred())
+
+				By("Verify content")
+				same, err := f.VerifyTargetPVCContentMD5(f.Namespace, pvc, utils.DefaultImagePath, utils.UploadFileMD5100kbytes, 100000)
+				Expect(err).ToNot(HaveOccurred())
+				Expect(same).To(BeTrue())
+
+				By("Wait for upload population pod to be deleted")
+				deleted, err := utils.WaitPodDeleted(f.K8sClient, utils.UploadPodName(pvcPrime), f.Namespace.Name, time.Second*20)
+				Expect(err).ToNot(HaveOccurred())
+				Expect(deleted).To(BeTrue())
+			})
 		})
 
 		Context("archive", func() {