Integration of Data volume using CDI populators (kubevirt#2722)

* move cleanup out of dv deletion

It seemed off to call cleanup in the prepare function
just because we don't call cleanup unless the dv is deleting.
Instead we check in the clenup function itself if it should be
done: in this 2 specific cases in case of deletion and in case
the dv succeeded.
The cleanup will be used in future commit also for population cleanup
which we also want to happen not only on deletion.

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

* Use populator if csi storage class exists

Add new datavolume phase PendingPopulation to
indicate wffc when using populators, this new
phase will be used in kubevirt in order to know
that there is no need for dummy pod to pass wffc phase
and that the population will occur once creating the vm.

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

* Update population targetPVC with pvc prime annotations

The annotations will be used to update dv that uses the
populators.

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

* Adjust UT with new behavior

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

* updates after review

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

* Fix import populator report progress

The import pod should be taken from pvcprime

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

* Prevent requeue upload dv when failing to find progress report pod

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

* Remove size inflation in populators

The populators are handling existing PVCs.
The PVC already has a defined requested size,
inflating the PVC' with fsoverhead will only be
on the PVC' spec and will not reflect on the target
PVC, this seems undesired.
Instead if the populators is using by PVC that the
datavolume controller created the inflation will happen
there if needed.

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

* Adjust functional tests to handle dvs using populators

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

* Fix clone test

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

* add shouldUpdateProgress variable to know if need to update progress

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

* Change update of annotation from denied list to allowed list

Instead if checking if the annotation on pvcPrime is not desired
go over desired list and if the annotation exists add it.

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

* fix removing annotations from pv when rebinding

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

* More fixes and UT

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

* a bit more updates and UTs

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

---------

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

pkg/controller/clone/rebind_test.go

@@ -180,7 +180,7 @@ var _ = Describe("RebindPhase test", func() {
 				p := createRebindPhase(createTarget(), source, volume)
 				result, err := p.Reconcile(context.Background())
 				Expect(err).To(HaveOccurred())
-				Expect(err.Error()).To(Equal("PV pv bound to unexpected claim"))
+				Expect(err.Error()).To(Equal("PV pv bound to unexpected claim foo"))
 				Expect(result).To(BeNil())
 			})
 		})

pkg/controller/common/BUILD.bazel

@@ -54,7 +54,9 @@ go_test(
         "//vendor/github.com/onsi/ginkgo/extensions/table:go_default_library",
         "//vendor/github.com/onsi/gomega:go_default_library",
         "//vendor/k8s.io/api/core/v1:go_default_library",
+        "//vendor/k8s.io/apimachinery/pkg/api/errors:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
+        "//vendor/k8s.io/apimachinery/pkg/types:go_default_library",
         "//vendor/sigs.k8s.io/controller-runtime/pkg/log:go_default_library",
         "//vendor/sigs.k8s.io/controller-runtime/pkg/log/zap:go_default_library",
     ],

pkg/controller/common/util.go

@@ -108,8 +108,8 @@ const (
 	// AnnMultiStageImportDone marks a multi-stage import as totally finished
 	AnnMultiStageImportDone = AnnAPIGroup + "/storage.checkpoint.done"
 
-	// AnnImportProgressReporting stores the current progress of the import process as a percetange
-	AnnImportProgressReporting = AnnAPIGroup + "/storage.import.progress"
+	// AnnPopulatorProgress is a standard annotation that can be used progress reporting
+	AnnPopulatorProgress = AnnAPIGroup + "/storage.populator.progress"
 
 	// AnnPreallocationRequested provides a const to indicate whether preallocation should be performed on the PV
 	AnnPreallocationRequested = AnnAPIGroup + "/storage.preallocation.requested"
@@ -204,6 +204,8 @@ const (
 	// AnnPopulatorKind annotation is added to a PVC' to specify the population kind, so it's later
 	// checked by the common populator watches.
 	AnnPopulatorKind = AnnAPIGroup + "/storage.populator.kind"
+	//AnnUsePopulator annotation indicates if the datavolume population will use populators
+	AnnUsePopulator = AnnAPIGroup + "/storage.usePopulator"
 
 	//AnnDefaultStorageClass is the annotation indicating that a storage class is the default one.
 	AnnDefaultStorageClass = "storageclass.kubernetes.io/is-default-class"
@@ -1498,7 +1500,7 @@ func UpdateImageIOAnnotations(annotations map[string]string, imageio *cdiv1.Data
 // IsPVBoundToPVC checks if a PV is bound to a specific PVC
 func IsPVBoundToPVC(pv *corev1.PersistentVolume, pvc *corev1.PersistentVolumeClaim) bool {
 	claimRef := pv.Spec.ClaimRef
-	return claimRef.Name == pvc.Name && claimRef.Namespace == pvc.Namespace && claimRef.UID == pvc.UID
+	return claimRef != nil && claimRef.Name == pvc.Name && claimRef.Namespace == pvc.Namespace && claimRef.UID == pvc.UID
 }
 
 // Rebind binds the PV of source to target
@@ -1514,18 +1516,21 @@ func Rebind(ctx context.Context, c client.Client, source, target *corev1.Persist
 	}
 
 	// Examine the claimref for the PV and see if it's still bound to PVC'
+	if pv.Spec.ClaimRef == nil {
+		return fmt.Errorf("PV %s claimRef is nil", pv.Name)
+	}
+
 	if !IsPVBoundToPVC(pv, source) {
 		// Something is not right if the PV is neither bound to PVC' nor target PVC
 		if !IsPVBoundToPVC(pv, target) {
 			klog.Errorf("PV bound to unexpected PVC: Could not rebind to target PVC '%s'", target.Name)
-			return fmt.Errorf("PV %s bound to unexpected claim", pv.Name)
+			return fmt.Errorf("PV %s bound to unexpected claim %s", pv.Name, pv.Spec.ClaimRef.Name)
 		}
 		// our work is done
 		return nil
 	}
 
 	// Rebind PVC to target PVC
-	pv.Annotations = make(map[string]string)
 	pv.Spec.ClaimRef = &corev1.ObjectReference{
 		Namespace:       target.Namespace,
 		Name:            target.Name,
@@ -1735,3 +1740,20 @@ func isCrdDeployed(c client.Client, name, version string, log logr.Logger) bool
 func IsSnapshotReady(snapshot *snapshotv1.VolumeSnapshot) bool {
 	return snapshot.Status != nil && snapshot.Status.ReadyToUse != nil && *snapshot.Status.ReadyToUse
 }
+
+// GetResource updates given obj with the data of the object with the same name and namespace
+func GetResource(ctx context.Context, c client.Client, namespace, name string, obj client.Object) (bool, error) {
+	obj.SetNamespace(namespace)
+	obj.SetName(name)
+
+	err := c.Get(ctx, client.ObjectKeyFromObject(obj), obj)
+	if err != nil {
+		if k8serrors.IsNotFound(err) {
+			return false, nil
+		}
+
+		return false, err
+	}
+
+	return true, nil
+}

pkg/controller/common/util_test.go

@@ -7,7 +7,9 @@ import (
 	"github.com/onsi/ginkgo/extensions/table"
 	. "github.com/onsi/gomega"
 	v1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/types"
 	cdiv1 "kubevirt.io/containerized-data-importer-api/pkg/apis/core/v1beta1"
 )
 
@@ -77,6 +79,118 @@ var _ = Describe("GetDefaultStorageClass", func() {
 	})
 })
 
+var _ = Describe("Rebind", func() {
+	It("Should return error if PV doesn't exist", func() {
+		client := CreateClient()
+		pvc := &v1.PersistentVolumeClaim{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "testPVC",
+				Namespace: "namespace",
+			},
+			Spec: v1.PersistentVolumeClaimSpec{
+				VolumeName: "testPV",
+			},
+		}
+		err := Rebind(context.Background(), client, pvc, pvc)
+		Expect(err).To(HaveOccurred())
+		Expect(errors.IsNotFound(err)).To(BeTrue())
+	})
+
+	It("Should return error if bound to unexpected claim", func() {
+		pvc := &v1.PersistentVolumeClaim{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "testPVC",
+				Namespace: "namespace",
+			},
+			Spec: v1.PersistentVolumeClaimSpec{
+				VolumeName: "testPV",
+			},
+		}
+		pv := &v1.PersistentVolume{
+			ObjectMeta: metav1.ObjectMeta{
+				Name: "testPV",
+			},
+			Spec: v1.PersistentVolumeSpec{
+				ClaimRef: &v1.ObjectReference{
+					Name:      "anotherPVC",
+					Namespace: "namespace",
+					UID:       "uid",
+				},
+			},
+		}
+		client := CreateClient(pv)
+		err := Rebind(context.Background(), client, pvc, pvc)
+		Expect(err).To(HaveOccurred())
+		Expect(err.Error()).To(Equal("PV testPV bound to unexpected claim anotherPVC"))
+	})
+	It("Should return nil if bound to target claim", func() {
+		pvc := &v1.PersistentVolumeClaim{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "testPVC",
+				Namespace: "namespace",
+			},
+			Spec: v1.PersistentVolumeClaimSpec{
+				VolumeName: "testPV",
+			},
+		}
+		targetPVC := pvc.DeepCopy()
+		targetPVC.Name = "targetPVC"
+		targetPVC.UID = "uid"
+		pv := &v1.PersistentVolume{
+			ObjectMeta: metav1.ObjectMeta{
+				Name: "testPV",
+			},
+			Spec: v1.PersistentVolumeSpec{
+				ClaimRef: &v1.ObjectReference{
+					Name:      "targetPVC",
+					Namespace: "namespace",
+					UID:       "uid",
+				},
+			},
+		}
+		client := CreateClient(pv)
+		err := Rebind(context.Background(), client, pvc, targetPVC)
+		Expect(err).ToNot(HaveOccurred())
+	})
+	It("Should rebind pv to target claim", func() {
+		pvc := &v1.PersistentVolumeClaim{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "testPVC",
+				Namespace: "namespace",
+			},
+			Spec: v1.PersistentVolumeClaimSpec{
+				VolumeName: "testPV",
+			},
+		}
+		targetPVC := pvc.DeepCopy()
+		targetPVC.Name = "targetPVC"
+		pvc.UID = "uid"
+		pv := &v1.PersistentVolume{
+			ObjectMeta: metav1.ObjectMeta{
+				Name: "testPV",
+			},
+			Spec: v1.PersistentVolumeSpec{
+				ClaimRef: &v1.ObjectReference{
+					Name:      "testPVC",
+					Namespace: "namespace",
+					UID:       "uid",
+				},
+			},
+		}
+		AddAnnotation(pv, "someAnno", "somevalue")
+		client := CreateClient(pv)
+		err := Rebind(context.Background(), client, pvc, targetPVC)
+		Expect(err).ToNot(HaveOccurred())
+		updatedPV := &v1.PersistentVolume{}
+		key := types.NamespacedName{Name: pv.Name, Namespace: pv.Namespace}
+		err = client.Get(context.TODO(), key, updatedPV)
+		Expect(err).ToNot(HaveOccurred())
+		Expect(updatedPV.Spec.ClaimRef.Name).To(Equal(targetPVC.Name))
+		//make sure annotations of pv from before rebind dont get deleted
+		Expect(pv.Annotations["someAnno"]).To(Equal("somevalue"))
+	})
+})
+
 func createPvcNoSize(name, ns string, annotations, labels map[string]string) *v1.PersistentVolumeClaim {
 	return &v1.PersistentVolumeClaim{
 		ObjectMeta: metav1.ObjectMeta{

pkg/controller/datavolume/BUILD.bazel

@@ -20,6 +20,7 @@ go_library(
     deps = [
         "//pkg/common:go_default_library",
         "//pkg/controller/common:go_default_library",
+        "//pkg/controller/populators:go_default_library",
         "//pkg/feature-gates:go_default_library",
         "//pkg/token:go_default_library",
         "//pkg/util:go_default_library",