Minor fixes and enhancements in import/common populator code

* Modify indexes and other related code to support namespaced dataSourceRefs. Cross-namespace population is still not supported as it depends on alpha feature gates.
* Add functional test to cover static binding.
* Fix selected node annotation bug in scratch space PVCs

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

pkg/controller/common/util.go

@@ -939,7 +939,7 @@ func SetRestrictedSecurityContext(podSpec *v1.PodSpec) {
 // SetNodeNameIfPopulator sets NodeName in a pod spec when the PVC is being handled by a CDI volume populator
 func SetNodeNameIfPopulator(pvc *corev1.PersistentVolumeClaim, podSpec *v1.PodSpec) {
 	_, isPopulator := pvc.Annotations[AnnPopulatorKind]
-	nodeName, _ := pvc.Annotations[AnnSelectedNode]
+	nodeName := pvc.Annotations[AnnSelectedNode]
 	if isPopulator && nodeName != "" {
 		podSpec.NodeName = nodeName
 	}

pkg/controller/populators/import-populator.go

@@ -121,7 +121,7 @@ func (r *ImportPopulatorReconciler) getPopulationSource(namespace, name string)
 
 // Import-specific implementation of reconcileTargetPVC
 func (r *ImportPopulatorReconciler) reconcileTargetPVC(pvc, pvcPrime *corev1.PersistentVolumeClaim) (reconcile.Result, error) {
-	phase, _ := pvcPrime.Annotations[cc.AnnPodPhase]
+	phase := pvcPrime.Annotations[cc.AnnPodPhase]
 	if err := r.updateImportProgress(phase, pvc, pvcPrime); err != nil {
 		return reconcile.Result{}, err
 	}

pkg/controller/populators/import-populator_test.go

@@ -73,11 +73,11 @@ var _ = Describe("Import populator tests", func() {
 		AnnDefaultStorageClass: "true",
 	}, map[string]string{}, "csi-plugin")
 
-	getVolumeImportSource := func(preallocation bool) *cdiv1.VolumeImportSource {
+	getVolumeImportSource := func(preallocation bool, namespace string) *cdiv1.VolumeImportSource {
 		return &cdiv1.VolumeImportSource{
 			ObjectMeta: metav1.ObjectMeta{
 				Name:      samplePopulatorName,
-				Namespace: metav1.NamespaceDefault,
+				Namespace: namespace,
 			},
 			Spec: cdiv1.VolumeImportSourceSpec{
 				ContentType:   cdiv1.DataVolumeKubeVirt,
@@ -120,12 +120,19 @@ var _ = Describe("Import populator tests", func() {
 		Kind:     cdiv1.VolumeImportSourceRef,
 		Name:     samplePopulatorName,
 	}
+	nsName := "test-import"
+	namespacedDataSourceRef := &corev1.TypedObjectReference{
+		APIGroup:  &apiGroup,
+		Kind:      cdiv1.VolumeImportSourceRef,
+		Name:      samplePopulatorName,
+		Namespace: &nsName,
+	}
 
 	var _ = Describe("Import populator reconcile", func() {
 		It("should trigger succeeded event when podPhase is succeeded during population", func() {
 			targetPvc := CreatePvcInStorageClass(targetPvcName, metav1.NamespaceDefault, &scName, nil, nil, corev1.ClaimPending)
 			targetPvc.Spec.DataSourceRef = dataSourceRef
-			volumeImportSource := getVolumeImportSource(true)
+			volumeImportSource := getVolumeImportSource(true, metav1.NamespaceDefault)
 			pvcPrime := getPVCPrime(targetPvc, nil)
 			pvcPrime.Annotations = map[string]string{AnnPodPhase: string(corev1.PodSucceeded)}
 
@@ -147,10 +154,35 @@ var _ = Describe("Import populator tests", func() {
 			Expect(found).To(BeTrue())
 		})
 
+		It("should ignore namespaced dataSourceRefs", func() {
+			targetPvc := CreatePvcInStorageClass(targetPvcName, metav1.NamespaceDefault, &scName, 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)
+			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()))
+
+			By("Checking PVC was ignored")
+			close(reconciler.recorder.(*record.FakeRecorder).Events)
+			found := false
+			for event := range reconciler.recorder.(*record.FakeRecorder).Events {
+				if strings.Contains(event, importSucceeded) {
+					found = true
+				}
+			}
+			reconciler.recorder = nil
+			Expect(found).To(BeFalse())
+		})
+
 		It("Should trigger failed import event when pod phase is podfailed", func() {
 			targetPvc := CreatePvcInStorageClass(targetPvcName, metav1.NamespaceDefault, &scName, nil, nil, corev1.ClaimPending)
 			targetPvc.Spec.DataSourceRef = dataSourceRef
-			volumeImportSource := getVolumeImportSource(true)
+			volumeImportSource := getVolumeImportSource(true, metav1.NamespaceDefault)
 			pvcPrime := getPVCPrime(targetPvc, nil)
 			pvcPrime.Annotations = map[string]string{AnnPodPhase: string(corev1.PodFailed)}
 
@@ -175,7 +207,7 @@ var _ = Describe("Import populator tests", func() {
 		It("Should retrigger reconcile while import pod is running", func() {
 			targetPvc := CreatePvcInStorageClass(targetPvcName, metav1.NamespaceDefault, &scName, nil, nil, corev1.ClaimPending)
 			targetPvc.Spec.DataSourceRef = dataSourceRef
-			volumeImportSource := getVolumeImportSource(true)
+			volumeImportSource := getVolumeImportSource(true, metav1.NamespaceDefault)
 			pvcPrime := getPVCPrime(targetPvc, nil)
 			pvcPrime.Annotations = map[string]string{AnnPodPhase: string(corev1.PodRunning)}
 
@@ -190,7 +222,7 @@ var _ = Describe("Import populator tests", func() {
 		It("Should create PVC Prime with proper import annotations", func() {
 			targetPvc := CreatePvcInStorageClass(targetPvcName, metav1.NamespaceDefault, &scName, nil, nil, corev1.ClaimBound)
 			targetPvc.Spec.DataSourceRef = dataSourceRef
-			volumeImportSource := getVolumeImportSource(true)
+			volumeImportSource := getVolumeImportSource(true, metav1.NamespaceDefault)
 
 			By("Reconcile")
 			reconciler = createImportPopulatorReconciler(targetPvc, volumeImportSource, sc)

pkg/controller/populators/populator-base.go

@@ -73,9 +73,10 @@ func CreateCommonPopulatorIndexes(mgr manager.Manager) error {
 	if err := mgr.GetFieldIndexer().IndexField(context.TODO(), &corev1.PersistentVolumeClaim{}, dataSourceRefField, func(obj client.Object) []string {
 		pvc := obj.(*corev1.PersistentVolumeClaim)
 		dataSourceRef := pvc.Spec.DataSourceRef
-		if dataSourceRef != nil && dataSourceRef.APIGroup != nil &&
-			*dataSourceRef.APIGroup == cc.AnnAPIGroup && dataSourceRef.Name != "" {
-			return []string{getPopulatorIndexKey(obj.GetNamespace(), pvc.Spec.DataSourceRef.Kind, pvc.Spec.DataSourceRef.Name)}
+		if isDataSourceRefValid(dataSourceRef) {
+			namespace := getPopulationSourceNamespace(pvc)
+			apiGroup := *dataSourceRef.APIGroup
+			return []string{getPopulatorIndexKey(apiGroup, dataSourceRef.Kind, namespace, dataSourceRef.Name)}
 		}
 		return nil
 	}); err != nil {
@@ -106,7 +107,9 @@ func addCommonPopulatorsWatches(mgr manager.Manager, c controller.Controller, lo
 
 	mapDataSourceRefToPVC := func(obj client.Object) (reqs []reconcile.Request) {
 		var pvcs corev1.PersistentVolumeClaimList
-		matchingFields := client.MatchingFields{dataSourceRefField: getPopulatorIndexKey(obj.GetNamespace(), sourceKind, obj.GetName())}
+		matchingFields := client.MatchingFields{
+			dataSourceRefField: getPopulatorIndexKey(cc.AnnAPIGroup, sourceKind, obj.GetNamespace(), obj.GetName()),
+		}
 		if err := mgr.GetClient().List(context.TODO(), &pvcs, matchingFields); err != nil {
 			log.Error(err, "Unable to list PVCs", "matchingFields", matchingFields)
 			return reqs
@@ -241,8 +244,8 @@ func (r *ReconcilerBase) createPVCPrime(pvc *corev1.PersistentVolumeClaim, sourc
 	}
 	util.SetRecommendedLabels(pvcPrime, r.installerLabels, "cdi-controller")
 
-	requestedSize, _ := pvc.Spec.Resources.Requests[corev1.ResourceStorage]
-	// disk or image size, inflate it with overhead
+	// disk or image size, inflate it with overhead if necessary
+	requestedSize := pvc.Spec.Resources.Requests[corev1.ResourceStorage]
 	requestedSize, err := cc.InflateSizeWithOverhead(r.client, requestedSize.Value(), &pvc.Spec)
 	if err != nil {
 		return nil, err
@@ -297,15 +300,23 @@ func (r *ReconcilerBase) reconcileCommon(pvc *corev1.PersistentVolumeClaim, popu
 
 	// We should ignore PVCs that aren't for this populator to handle
 	dataSourceRef := pvc.Spec.DataSourceRef
-	if dataSourceRef == nil || !IsPVCDataSourceRefKind(pvc, r.sourceKind) || dataSourceRef.Name == "" {
+	if !IsPVCDataSourceRefKind(pvc, r.sourceKind) {
 		log.V(1).Info("reconciled unexpected PVC, ignoring")
 		return nil, nil
 	}
+	// TODO: Remove this check once we support cross-namespace dataSourceRef
+	if dataSourceRef.Namespace != nil {
+		log.V(1).Info("cross-namespace dataSourceRef not supported yet, ignoring")
+		return nil, nil
+	}
+
 	// Wait until dataSourceRef exists
-	populationSource, err := populator.getPopulationSource(pvc.Namespace, dataSourceRef.Name)
+	namespace := getPopulationSourceNamespace(pvc)
+	populationSource, err := populator.getPopulationSource(namespace, dataSourceRef.Name)
 	if populationSource == nil {
 		return nil, err
 	}
+	// Check storage class
 	ready, waitForFirstConsumer, err := r.handleStorageClass(pvc)
 	if !ready || err != nil {
 		return nil, err

pkg/controller/populators/util.go

@@ -46,11 +46,28 @@ const (
 	annMigratedTo = "pv.kubernetes.io/migrated-to"
 )
 
-// IsPVCDataSourceRefKind returns if the PVC has DataSourceRef that
+// IsPVCDataSourceRefKind returns if the PVC has a valid DataSourceRef that
 // is equal to the given kind
 func IsPVCDataSourceRefKind(pvc *corev1.PersistentVolumeClaim, kind string) bool {
 	dataSourceRef := pvc.Spec.DataSourceRef
-	return dataSourceRef != nil && dataSourceRef.APIGroup != nil && *dataSourceRef.APIGroup == cc.AnnAPIGroup && dataSourceRef.Kind == kind
+	return isDataSourceRefValid(dataSourceRef) && dataSourceRef.Kind == kind
+}
+
+func isDataSourceRefValid(dataSourceRef *corev1.TypedObjectReference) bool {
+	return dataSourceRef != nil && dataSourceRef.APIGroup != nil &&
+		*dataSourceRef.APIGroup == cc.AnnAPIGroup && dataSourceRef.Name != ""
+}
+
+func getPopulationSourceNamespace(pvc *corev1.PersistentVolumeClaim) string {
+	namespace := pvc.GetNamespace()
+	// The populator CR can be in a different namespace from the target PVC
+	// if the CrossNamespaceVolumeDataSource feature gate is enabled in the
+	// kube-apiserver and the kube-controller-manager.
+	dataSourceRef := pvc.Spec.DataSourceRef
+	if dataSourceRef != nil && dataSourceRef.Namespace != nil && *dataSourceRef.Namespace != "" {
+		namespace = *pvc.Spec.DataSourceRef.Namespace
+	}
+	return namespace
 }
 
 func isPVCPrimeDataSourceRefKind(pvc *corev1.PersistentVolumeClaim, kind string) bool {
@@ -67,13 +84,8 @@ func PVCPrimeName(targetPVC *corev1.PersistentVolumeClaim) string {
 	return fmt.Sprintf("%s-%s", primePvcPrefix, targetPVC.UID)
 }
 
-func getPopulatorIndexKey(namespace, kind, name string) string {
-	return namespace + "/" + kind + "/" + name
-}
-
-func isPVCOwnedByDataVolume(pvc *corev1.PersistentVolumeClaim) bool {
-	owner := metav1.GetControllerOf(pvc)
-	return (owner != nil && owner.Kind == "DataVolume") || cc.HasAnnOwnedByDataVolume(pvc)
+func getPopulatorIndexKey(apiGroup, kind, namespace, name string) string {
+	return fmt.Sprintf("%s/%s/%s/%s", apiGroup, kind, namespace, name)
 }
 
 func checkIntreeStorageClass(pvc *corev1.PersistentVolumeClaim, sc *storagev1.StorageClass) bool {

pkg/controller/util.go

@@ -137,8 +137,9 @@ func newScratchPersistentVolumeClaimSpec(pvc *v1.PersistentVolumeClaim, pod *v1.
 	}
 	// When the original PVC is being handled by a populator, copy AnnSelectedNode to avoid issues with k8s scheduler
 	_, isPopulator := pvc.Annotations[cc.AnnPopulatorKind]
-	if isPopulator {
-		annotations[cc.AnnSelectedNode] = pvc.Annotations[cc.AnnSelectedNode]
+	selectedNode := pvc.Annotations[cc.AnnSelectedNode]
+	if isPopulator && selectedNode != "" {
+		annotations[cc.AnnSelectedNode] = selectedNode
 	}
 
 	pvcDef := &v1.PersistentVolumeClaim{

tests/import_test.go

@@ -1555,17 +1555,14 @@ var _ = Describe("Import populator", func() {
 
 	BeforeEach(func() {
 		verifyCleanup(pvc)
-		if !f.IsCSIVolumeCloneStorageClassAvailable() {
-			Skip("No CSI drivers available - Population not supported")
-		}
 	})
 
 	AfterEach(func() {
 		By("Deleting verifier pod")
 		err := utils.DeleteVerifierPod(f.K8sClient, f.Namespace.Name)
 		Expect(err).ToNot(HaveOccurred())
 
-		err = f.CdiClient.CdiV1beta1().VolumeImportSources(f.Namespace.Name).Delete(context.TODO(), "import-populator-source", metav1.DeleteOptions{})
+		err = f.CdiClient.CdiV1beta1().VolumeImportSources(f.Namespace.Name).Delete(context.TODO(), "import-populator-test", metav1.DeleteOptions{})
 		if err != nil && !k8serrors.IsNotFound(err) {
 			Expect(err).ToNot(HaveOccurred())
 		}
@@ -1577,7 +1574,6 @@ var _ = Describe("Import populator", func() {
 
 	DescribeTable("should import fileSystem PVC", func(expectedMD5 string, volumeImportSourceFunc func(cdiv1.DataVolumeContentType, bool) error, preallocation bool) {
 		pvc = importPopulationPVCDefinition()
-		pvc.Spec.StorageClassName = &f.CsiCloneSCName
 		pvc = f.CreateScheduledPVCFromDefinition(pvc)
 		err = volumeImportSourceFunc(cdiv1.DataVolumeKubeVirt, preallocation)
 		Expect(err).ToNot(HaveOccurred())
@@ -1647,7 +1643,6 @@ var _ = Describe("Import populator", func() {
 		pvc = importPopulationPVCDefinition()
 		volumeMode := v1.PersistentVolumeBlock
 		pvc.Spec.VolumeMode = &volumeMode
-		pvc.Spec.StorageClassName = &f.CsiCloneSCName
 		pvc = f.CreateScheduledPVCFromDefinition(pvc)
 		err = volumeImportSourceFunc(cdiv1.DataVolumeKubeVirt, true)
 		Expect(err).ToNot(HaveOccurred())
@@ -1689,7 +1684,6 @@ var _ = Describe("Import populator", func() {
 
 	It("should import archive", func() {
 		pvc = importPopulationPVCDefinition()
-		pvc.Spec.StorageClassName = &f.CsiCloneSCName
 		pvc = f.CreateScheduledPVCFromDefinition(pvc)
 		err = createHTTPImportPopulatorCR(cdiv1.DataVolumeArchive, true)
 		Expect(err).ToNot(HaveOccurred())
@@ -1720,6 +1714,92 @@ var _ = Describe("Import populator", func() {
 			return err != nil && k8serrors.IsNotFound(err)
 		}, timeout, pollingInterval).Should(BeTrue())
 	})
+
+	It("Should handle static allocated PVC with import populator", func() {
+		pvc = importPopulationPVCDefinition()
+		pvc = f.CreateScheduledPVCFromDefinition(pvc)
+		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())
+
+		By("Verify target PVC is bound")
+		err = utils.WaitForPersistentVolumeClaimPhase(f.K8sClient, pvc.Namespace, v1.ClaimBound, pvc.Name)
+		Expect(err).ToNot(HaveOccurred())
+		pvc, err := f.K8sClient.CoreV1().PersistentVolumeClaims(pvc.Namespace).Get(context.TODO(), pvc.Name, metav1.GetOptions{})
+		Expect(err).ToNot(HaveOccurred())
+		pvName := pvc.Spec.VolumeName
+
+		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("Verifying the image is sparse")
+		Expect(f.VerifySparse(f.Namespace, pvc, utils.DefaultImagePath)).To(BeTrue())
+		sourceMD5 := md5
+
+		By("Retaining PV")
+		pv, err := f.K8sClient.CoreV1().PersistentVolumes().Get(context.TODO(), pvName, metav1.GetOptions{})
+		Expect(err).ToNot(HaveOccurred())
+		pv.Spec.PersistentVolumeReclaimPolicy = v1.PersistentVolumeReclaimRetain
+		_, err = f.K8sClient.CoreV1().PersistentVolumes().Update(context.TODO(), pv, metav1.UpdateOptions{})
+		Expect(err).ToNot(HaveOccurred())
+
+		By("Forcing cleanup")
+		err = utils.DeleteVerifierPod(f.K8sClient, f.Namespace.Name)
+		Expect(err).ToNot(HaveOccurred())
+		err = f.CdiClient.CdiV1beta1().VolumeImportSources(f.Namespace.Name).Delete(context.TODO(), "import-populator-test", metav1.DeleteOptions{})
+		if err != nil && !k8serrors.IsNotFound(err) {
+			Expect(err).ToNot(HaveOccurred())
+		}
+		Eventually(func() bool {
+			// Make sure pvcPrime was deleted after upload population
+			_, err := f.FindPVC(pvcPrime.Name)
+			return err != nil && k8serrors.IsNotFound(err)
+		}, timeout, pollingInterval).Should(BeTrue())
+
+		err = f.DeletePVC(pvc)
+		Expect(err).ToNot(HaveOccurred())
+		verifyCleanup(pvc)
+
+		By("Making PV available")
+		Eventually(func() bool {
+			pv, err := f.K8sClient.CoreV1().PersistentVolumes().Get(context.TODO(), pvName, metav1.GetOptions{})
+			Expect(err).ToNot(HaveOccurred())
+			Expect(pv.Spec.ClaimRef.Namespace).To(Equal(pvc.Namespace))
+			Expect(pv.Spec.ClaimRef.Name).To(Equal(pvc.Name))
+			if pv.Status.Phase == v1.VolumeAvailable {
+				return true
+			}
+			pv.Spec.ClaimRef.ResourceVersion = ""
+			pv.Spec.ClaimRef.UID = ""
+			_, err = f.K8sClient.CoreV1().PersistentVolumes().Update(context.TODO(), pv, metav1.UpdateOptions{})
+			Expect(err).ToNot(HaveOccurred())
+			return false
+		}, timeout, pollingInterval).Should(BeTrue())
+
+		// Start the whole process again
+
+		pvc = importPopulationPVCDefinition()
+		pvc = f.CreateScheduledPVCFromDefinition(pvc)
+		err = createHTTPImportPopulatorCR(cdiv1.DataVolumeKubeVirt, true)
+		Expect(err).ToNot(HaveOccurred())
+
+		By("Verify PVC prime is not created anymore")
+		_, err = f.FindPVC(populators.PVCPrimeName(pvc))
+		Expect(k8serrors.IsNotFound(err)).To(BeTrue())
+
+		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, sourceMD5, utils.MD5PrefixSize)
+		Expect(err).ToNot(HaveOccurred())
+		Expect(same).To(BeTrue())
+	})
 })
 
 func generateRegistryOnlySidecar() *unstructured.Unstructured {