Ensure priority class is assigned to pod populating pvc prime (#2864)

* Ensure priority class is assigned to pod populating pvc prime

Priority class was not being passed to the pvc prime from the PVC
and thus was not being added to the importer pod populating the
pvc prime. There is a list of allowed annotations that can be passed
and the priority class annotation was not in it. This commit adds
the annotation to the allowed list.

Cleaned up unneed log argument to a function related to passing the
annotations.

Signed-off-by: Alexander Wels <awels@redhat.com>

* Restore the logger as it was logging the pvc name as well
as the log message. Modified the name to make it clearer
this is the case.

Signed-off-by: Alexander Wels <awels@redhat.com>

* Have upload also use the populator, can't do clone
because the pod disappears too quickly without retain

Signed-off-by: Alexander Wels <awels@redhat.com>

* Test for priority class in upload as well as fix typo in cloning test.

Signed-off-by: Alexander Wels <awels@redhat.com>

---------

Signed-off-by: Alexander Wels <awels@redhat.com>

pkg/controller/common/util.go

@@ -1941,6 +1941,7 @@ func SetPvcAllowedAnnotations(obj metav1.Object, pvc *corev1.PersistentVolumeCla
 	allowedAnnotations := map[string]string{
 		AnnPodNetwork:              "",
 		AnnPodSidecarInjection:     AnnPodSidecarInjectionDefault,
+		AnnPriorityClassName:       "",
 		AnnPodMultusDefaultNetwork: ""}
 	for ann, def := range allowedAnnotations {
 		val, ok := pvc.Annotations[ann]

pkg/controller/datavolume/controller-base.go

@@ -1038,7 +1038,6 @@ func (r *ReconcilerBase) newPersistentVolumeClaim(dataVolume *cdiv1.DataVolume,
 		annotations[cc.AnnPriorityClassName] = dataVolume.Spec.PriorityClassName
 	}
 	annotations[cc.AnnPreallocationRequested] = strconv.FormatBool(cc.GetPreallocation(context.TODO(), r.client, dataVolume.Spec.Preallocation))
-
 	pvc := &corev1.PersistentVolumeClaim{
 		ObjectMeta: metav1.ObjectMeta{
 			Namespace:   namespace,

pkg/controller/populators/clone-populator_test.go

@@ -114,7 +114,7 @@ var _ = Describe("Clone populator tests", func() {
 		return target, source
 	}
 
-	initinializedTargetAndDataSource := func() (*corev1.PersistentVolumeClaim, *cdiv1.VolumeCloneSource) {
+	initializedTargetAndDataSource := func() (*corev1.PersistentVolumeClaim, *cdiv1.VolumeCloneSource) {
 		target, source := targetAndDataSource()
 		target.Annotations = map[string]string{
 			AnnClonePhase:   clone.PendingPhaseName,
@@ -126,7 +126,7 @@ var _ = Describe("Clone populator tests", func() {
 	}
 
 	succeededTarget := func() *corev1.PersistentVolumeClaim {
-		target, _ := initinializedTargetAndDataSource()
+		target, _ := initializedTargetAndDataSource()
 		target.Annotations[AnnClonePhase] = string(clone.SucceededPhaseName)
 		target.Spec.VolumeName = "volume"
 		return target
@@ -257,7 +257,7 @@ var _ = Describe("Clone populator tests", func() {
 	})
 
 	It("should be in error phase if plan returns an error", func() {
-		target, source := initinializedTargetAndDataSource()
+		target, source := initializedTargetAndDataSource()
 		reconciler := createClonePopulatorReconciler(target, storageClass(), source)
 		reconciler.planner = &fakePlanner{
 			planError: fmt.Errorf("plan error"),
@@ -270,7 +270,7 @@ var _ = Describe("Clone populator tests", func() {
 	})
 
 	It("should be in error phase if phase returns an error", func() {
-		target, source := initinializedTargetAndDataSource()
+		target, source := initializedTargetAndDataSource()
 		reconciler := createClonePopulatorReconciler(target, storageClass(), source)
 		reconciler.planner = &fakePlanner{
 			planResult: []clone.Phase{
@@ -288,7 +288,7 @@ var _ = Describe("Clone populator tests", func() {
 	})
 
 	DescribeTable("should report phase name and progress", func(ownedByDataVolume bool) {
-		target, source := initinializedTargetAndDataSource()
+		target, source := initializedTargetAndDataSource()
 		if ownedByDataVolume {
 			target.OwnerReferences = []metav1.OwnerReference{
 				{
@@ -341,7 +341,7 @@ var _ = Describe("Clone populator tests", func() {
 	)
 
 	It("should be in error phase if progress returns an error", func() {
-		target, source := initinializedTargetAndDataSource()
+		target, source := initializedTargetAndDataSource()
 		reconciler := createClonePopulatorReconciler(target, storageClass(), source)
 		reconciler.planner = &fakePlanner{
 			planResult: []clone.Phase{
@@ -362,7 +362,7 @@ var _ = Describe("Clone populator tests", func() {
 	})
 
 	It("should go to succeeded phase if all phases are done", func() {
-		target, source := initinializedTargetAndDataSource()
+		target, source := initializedTargetAndDataSource()
 		reconciler := createClonePopulatorReconciler(target, storageClass(), source)
 		reconciler.planner = &fakePlanner{
 			planResult: []clone.Phase{

pkg/controller/populators/import-populator_test.go

@@ -233,9 +233,10 @@ var _ = Describe("Import populator tests", func() {
 			Expect(result.RequeueAfter).To(BeNumerically(">", 0))
 		})
 
-		It("Should create PVC Prime with proper import annotations", func() {
-			targetPvc := CreatePvcInStorageClass(targetPvcName, metav1.NamespaceDefault, &sc.Name, nil, nil, corev1.ClaimBound)
+		DescribeTable("Should create PVC Prime with proper import annotations", func(key, value, expectedValue string) {
+			targetPvc := CreatePvcInStorageClass(targetPvcName, metav1.NamespaceDefault, &sc.Name, map[string]string{}, nil, corev1.ClaimBound)
 			targetPvc.Spec.DataSourceRef = dataSourceRef
+			targetPvc.Annotations[key] = value
 			volumeImportSource := getVolumeImportSource(true, metav1.NamespaceDefault)
 
 			By("Reconcile")
@@ -268,7 +269,15 @@ var _ = Describe("Import populator tests", func() {
 			Expect(pvcPrime.GetAnnotations()[AnnPreallocationRequested]).To(Equal("true"))
 			Expect(pvcPrime.GetAnnotations()[AnnEndpoint]).To(Equal("http://example.com/data"))
 			Expect(pvcPrime.GetAnnotations()[AnnSource]).To(Equal(SourceHTTP))
-		})
+			Expect(pvcPrime.Annotations[key]).To(Equal(expectedValue))
+		},
+			Entry("No extra annotations", "", "", ""),
+			Entry("Invalid extra annotation is not passed", "invalid", "test", ""),
+			Entry("Priority class is passed", AnnPriorityClassName, "test", "test"),
+			Entry("pod network is passed", AnnPodNetwork, "test", "test"),
+			Entry("side car injection is passed", AnnPodSidecarInjection, AnnPodSidecarInjectionDefault, AnnPodSidecarInjectionDefault),
+			Entry("multus default network is passed", AnnPodMultusDefaultNetwork, "test", "test"),
+		)
 
 		It("shouldn't error when reconciling PVC with non-import DataSourceRef", func() {
 			targetPvc := CreatePvcInStorageClass(targetPvcName, metav1.NamespaceDefault, &sc.Name, nil, nil, corev1.ClaimBound)

pkg/controller/populators/populator-base.go

@@ -247,7 +247,7 @@ func (r *ReconcilerBase) updatePVCWithPVCPrimeAnnotations(pvc, pvcPrime *corev1.
 
 // reconcile functions
 
-func (r *ReconcilerBase) reconcile(req reconcile.Request, populator populatorController, log logr.Logger) (reconcile.Result, error) {
+func (r *ReconcilerBase) reconcile(req reconcile.Request, populator populatorController, pvcNameLogger logr.Logger) (reconcile.Result, error) {
 	// Get the target PVC
 	pvc := &corev1.PersistentVolumeClaim{}
 	if err := r.client.Get(context.TODO(), req.NamespacedName, pvc); err != nil {
@@ -259,7 +259,7 @@ func (r *ReconcilerBase) reconcile(req reconcile.Request, populator populatorCon
 
 	// We first perform the common reconcile steps.
 	// We should only continue if we get a valid PVC'
-	pvcPrime, err := r.reconcileCommon(pvc, populator, log)
+	pvcPrime, err := r.reconcileCommon(pvc, populator, pvcNameLogger)
 	if err != nil || pvcPrime == nil {
 		return reconcile.Result{}, err
 	}
@@ -273,9 +273,9 @@ func (r *ReconcilerBase) reconcile(req reconcile.Request, populator populatorCon
 	return r.reconcileCleanup(pvcPrime)
 }
 
-func (r *ReconcilerBase) reconcileCommon(pvc *corev1.PersistentVolumeClaim, populator populatorController, log logr.Logger) (*corev1.PersistentVolumeClaim, error) {
+func (r *ReconcilerBase) reconcileCommon(pvc *corev1.PersistentVolumeClaim, populator populatorController, pvcNameLogger logr.Logger) (*corev1.PersistentVolumeClaim, error) {
 	if pvc.DeletionTimestamp != nil {
-		log.V(1).Info("PVC being terminated, ignoring")
+		pvcNameLogger.V(1).Info("PVC being terminated, ignoring")
 		return nil, nil
 	}
 
@@ -292,12 +292,12 @@ 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 !IsPVCDataSourceRefKind(pvc, r.sourceKind) {
-		log.V(1).Info("reconciled unexpected PVC, ignoring")
+		pvcNameLogger.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")
+		pvcNameLogger.V(1).Info("cross-namespace dataSourceRef not supported yet, ignoring")
 		return nil, nil
 	}
 

pkg/controller/populators/upload-populator_test.go

@@ -268,6 +268,53 @@ var _ = Describe("Datavolume controller reconcile loop", func() {
 		Entry("with pod succeded phase", string(corev1.PodFailed)),
 		Entry("with pod succeded phase", string(corev1.PodSucceeded)),
 	)
+
+	DescribeTable("Should create PVC Prime with proper upload annotations", func(key, value, expectedValue string) {
+		pvc := newUploadPopulatorPVC("test-pvc")
+		cc.AddAnnotation(pvc, AnnPVCPrimeName, PVCPrimeName(pvc))
+		uploadPV := uploadPV(pvc)
+
+		volumeUploadSourceCR := newUploadPopulatorCR("", false)
+		scName := "test-sc"
+		sc := cc.CreateStorageClassWithProvisioner(scName, map[string]string{cc.AnnDefaultStorageClass: "true"}, map[string]string{}, "csi-plugin")
+		pvc.Annotations[key] = value
+
+		By("Reconcile")
+		r := createUploadPopulatorReconciler(pvc, volumeUploadSourceCR, sc, uploadPV)
+		result, err := r.Reconcile(context.TODO(), reconcile.Request{NamespacedName: types.NamespacedName{Name: "test-pvc", Namespace: metav1.NamespaceDefault}})
+		Expect(err).To(Not(HaveOccurred()))
+		Expect(result).To(Not(BeNil()))
+
+		By("Checking events recorded")
+		close(r.recorder.(*record.FakeRecorder).Events)
+		found := false
+		for event := range r.recorder.(*record.FakeRecorder).Events {
+			if strings.Contains(event, createdPVCPrimeSuccessfully) {
+				found = true
+			}
+		}
+		r.recorder = nil
+		Expect(found).To(BeTrue())
+
+		By("Checking PVC' annotations")
+		pvcPrime, err := r.getPVCPrime(pvc)
+		Expect(err).ToNot(HaveOccurred())
+		Expect(pvcPrime).ToNot(BeNil())
+		// make sure we didnt inflate size
+		Expect(pvcPrime.Spec.Resources.Requests[corev1.ResourceStorage]).To(Equal(resource.MustParse("1Gi")))
+		Expect(pvcPrime.GetAnnotations()).ToNot(BeNil())
+		Expect(pvcPrime.GetAnnotations()[cc.AnnImmediateBinding]).To(Equal(""))
+		Expect(pvcPrime.GetAnnotations()[cc.AnnUploadRequest]).To(Equal(""))
+		Expect(pvcPrime.GetAnnotations()[cc.AnnPopulatorKind]).To(Equal(cdiv1.VolumeUploadSourceRef))
+		Expect(pvcPrime.Annotations[key]).To(Equal(expectedValue))
+	},
+		Entry("No extra annotations", "", "", ""),
+		Entry("Invalid extra annotation is not passed", "invalid", "test", ""),
+		Entry("Priority class is passed", cc.AnnPriorityClassName, "test", "test"),
+		Entry("pod network is passed", cc.AnnPodNetwork, "test", "test"),
+		Entry("side car injection is passed", cc.AnnPodSidecarInjection, cc.AnnPodSidecarInjectionDefault, cc.AnnPodSidecarInjectionDefault),
+		Entry("multus default network is passed", cc.AnnPodMultusDefaultNetwork, "test", "test"),
+	)
 })
 
 func newUploadPopulatorPVC(name string) *corev1.PersistentVolumeClaim {

tests/datavolume_test.go

@@ -3006,7 +3006,6 @@ var _ = Describe("[vendor:cnv-qe@redhat.com][level:component]DataVolume tests",
 			dataVolume := utils.NewDataVolumeWithHTTPImport(dataVolumeName, "1Gi", fmt.Sprintf(utils.TinyCoreQcow2URLRateLimit, f.CdiInstallNs))
 			By(fmt.Sprintf("creating new datavolume %s with priority class", dataVolume.Name))
 			dataVolume.Spec.PriorityClassName = "system-cluster-critical"
-			dataVolume.Annotations[controller.AnnPodRetainAfterCompletion] = "true"
 			dataVolume, err := utils.CreateDataVolumeFromDefinition(f.CdiClient, f.Namespace.Name, dataVolume)
 			Expect(err).ToNot(HaveOccurred())
 
@@ -3033,7 +3032,6 @@ var _ = Describe("[vendor:cnv-qe@redhat.com][level:component]DataVolume tests",
 			dataVolume := utils.NewDataVolumeForUpload(dataVolumeName, "1Gi")
 			By(fmt.Sprintf("creating new datavolume %s with priority class\"", dataVolume.Name))
 			dataVolume.Spec.PriorityClassName = "system-cluster-critical"
-			dataVolume.Annotations[controller.AnnPodRetainAfterCompletion] = "true"
 			dataVolume, err := utils.CreateDataVolumeFromDefinition(f.CdiClient, f.Namespace.Name, dataVolume)
 			Expect(err).ToNot(HaveOccurred())