From 8dab1078c715e5e5aa880f5ed98e70eb06cf77ef Mon Sep 17 00:00:00 2001 From: Shelly Kagan Date: Fri, 26 Jul 2024 07:25:09 +0300 Subject: [PATCH] Remove datavolume clone source validation (#3331) * Remove sourceref validation The source can be changed and updated, we want the error to be presented by an event and have the datavolume pending instead of preventing the creation of the datavolume. We are already handling the option of the source not existing yet so lets also handle all the validation in the controller. Signed-off-by: Shelly Kagan * Move pvc clone specific functions to pvc-clone-controller Now that the validation is no longer done in the webhook no need for that code to be in the controller common util file. Moved the UT accordingly. Signed-off-by: Shelly Kagan * Move function to new Describe node Will add in future commits other tests to that section Signed-off-by: Shelly Kagan * pvc-clone-controller: make better event and update dv if validation fails In order to make it more clear that the validation failed we should add the reason of validation failure to the event Added an update watch to the clone controller so if something changes in the source the clone will reconcile. Hence no need to return an error, the reconcile will be triggered if someting will change in the source. Signed-off-by: Shelly Kagan * snap-clone: always validate snapshot source This commit adds missing validation due to the remove validation from the dv webhook, but also fixes an existing missing validation in cases where the clone was created before the source. In such case the webhook would not validate the source since it didnt exist yet, and then if the clone happened with populators then we would not validate the source size at all. We should validate the snapshot source before continuing with the clone whether it is populator/legacy clones. Moved all the validation to one function. Updated the event to include the reason for the validation failure and updated dv status accordingly. Signed-off-by: Shelly Kagan * Cloner_test: remove clone to small size test The test checks the rejection of the webhook which no longer exists then clone to appropriate size dv which should succeed which is done in any other clone test. Im not adding a test to check the reconcile loop since it is covered in the UT. Signed-off-by: Shelly Kagan * tests/datasource: remove wait for pvc Now since we added the validation also in the populators path we are not creating the pvc until the source exists. Signed-off-by: Shelly Kagan --------- Signed-off-by: Shelly Kagan --- pkg/apiserver/webhooks/BUILD.bazel | 1 - pkg/apiserver/webhooks/datavolume-validate.go | 90 ------ .../webhooks/datavolume-validate_test.go | 58 +--- pkg/controller/common/util.go | 84 +----- pkg/controller/common/util_test.go | 30 -- .../datavolume/clone-controller-base.go | 8 +- pkg/controller/datavolume/controller-base.go | 2 + .../datavolume/pvc-clone-controller.go | 58 +++- .../datavolume/pvc-clone-controller_test.go | 266 +++++++++++++++--- .../datavolume/snapshot-clone-controller.go | 144 +++++----- .../snapshot-clone-controller_test.go | 114 ++++++++ pkg/controller/util_test.go | 92 ------ tests/cloner_test.go | 137 --------- tests/datasource_test.go | 14 +- 14 files changed, 500 insertions(+), 598 deletions(-) diff --git a/pkg/apiserver/webhooks/BUILD.bazel b/pkg/apiserver/webhooks/BUILD.bazel index c2d485bba6..a77af35dc2 100644 --- a/pkg/apiserver/webhooks/BUILD.bazel +++ b/pkg/apiserver/webhooks/BUILD.bazel @@ -65,7 +65,6 @@ go_test( "//pkg/controller/common:go_default_library", "//staging/src/kubevirt.io/containerized-data-importer-api/pkg/apis/core/v1beta1:go_default_library", "//vendor/github.com/appscode/jsonpatch:go_default_library", - "//vendor/github.com/kubernetes-csi/external-snapshotter/client/v6/apis/volumesnapshot/v1:go_default_library", "//vendor/github.com/kubernetes-csi/external-snapshotter/client/v6/clientset/versioned/fake:go_default_library", "//vendor/github.com/onsi/ginkgo/v2:go_default_library", "//vendor/github.com/onsi/gomega:go_default_library", diff --git a/pkg/apiserver/webhooks/datavolume-validate.go b/pkg/apiserver/webhooks/datavolume-validate.go index d5eca56bf2..5456d4b09b 100644 --- a/pkg/apiserver/webhooks/datavolume-validate.go +++ b/pkg/apiserver/webhooks/datavolume-validate.go @@ -218,12 +218,6 @@ func (wh *dataVolumeValidatingWebhook) validateDataVolumeSpec(request *admission }) return causes } - if request.Operation == admissionv1.Create { - cause := wh.validateDataVolumeSourcePVC(spec.Source.PVC, field.Child("source", "PVC"), spec) - if cause != nil { - causes = append(causes, *cause) - } - } } if spec.Source.Snapshot != nil { if spec.Source.Snapshot.Namespace == "" || spec.Source.Snapshot.Name == "" { @@ -234,12 +228,6 @@ func (wh *dataVolumeValidatingWebhook) validateDataVolumeSpec(request *admission }) return causes } - if request.Operation == admissionv1.Create { - cause := wh.validateDataVolumeSourceSnapshot(spec.Source.Snapshot, field.Child("source", "Snapshot"), spec) - if cause != nil { - causes = append(causes, *cause) - } - } } return causes @@ -267,61 +255,6 @@ func (wh *dataVolumeValidatingWebhook) validateSourceRef(request *admissionv1.Ad Field: field.Child("sourceRef", "Name").String(), } } - if request.Operation != admissionv1.Create { - return nil - } - ns := namespace - if spec.SourceRef.Namespace != nil && *spec.SourceRef.Namespace != "" { - ns = spec.SourceRef.Namespace - } - dataSource, err := wh.cdiClient.CdiV1beta1().DataSources(*ns).Get(context.TODO(), spec.SourceRef.Name, metav1.GetOptions{}) - if err != nil { - if k8serrors.IsNotFound(err) { - return &metav1.StatusCause{ - Type: metav1.CauseTypeFieldValueNotFound, - Message: fmt.Sprintf("SourceRef %s/%s/%s not found", spec.SourceRef.Kind, *ns, spec.SourceRef.Name), - Field: field.Child("sourceRef").String(), - } - } - return &metav1.StatusCause{ - Message: err.Error(), - Field: field.Child("sourceRef").String(), - } - } - switch { - case dataSource.Spec.Source.PVC != nil: - return wh.validateDataVolumeSourcePVC(dataSource.Spec.Source.PVC, field.Child("sourceRef"), spec) - case dataSource.Spec.Source.Snapshot != nil: - return wh.validateDataVolumeSourceSnapshot(dataSource.Spec.Source.Snapshot, field.Child("sourceRef"), spec) - } - - return &metav1.StatusCause{ - Message: fmt.Sprintf("Empty source field in '%s'. DataSource may not be ready yet", dataSource.Name), - Field: field.Child("sourceRef").String(), - } -} - -func (wh *dataVolumeValidatingWebhook) validateDataVolumeSourcePVC(PVC *cdiv1.DataVolumeSourcePVC, field *k8sfield.Path, spec *cdiv1.DataVolumeSpec) *metav1.StatusCause { - sourcePVC, err := wh.k8sClient.CoreV1().PersistentVolumeClaims(PVC.Namespace).Get(context.TODO(), PVC.Name, metav1.GetOptions{}) - if err != nil { - // We allow the creation of a clone DV even if the source PVC doesn't exist. - // The validation will be completed once the source PVC is created. - if k8serrors.IsNotFound(err) { - return nil - } - return &metav1.StatusCause{ - Message: err.Error(), - Field: field.String(), - } - } - - if err := cc.ValidateClone(sourcePVC, spec); err != nil { - return &metav1.StatusCause{ - Type: metav1.CauseTypeFieldValueInvalid, - Message: err.Error(), - Field: field.String(), - } - } return nil } @@ -400,29 +333,6 @@ func validateDataSourceRef(dataSource *v1.TypedObjectReference, field *k8sfield. return causes } -func (wh *dataVolumeValidatingWebhook) validateDataVolumeSourceSnapshot(snapshot *cdiv1.DataVolumeSourceSnapshot, field *k8sfield.Path, spec *cdiv1.DataVolumeSpec) *metav1.StatusCause { - sourceSnapshot, err := wh.snapClient.SnapshotV1().VolumeSnapshots(snapshot.Namespace).Get(context.TODO(), snapshot.Name, metav1.GetOptions{}) - if err != nil { - if k8serrors.IsNotFound(err) { - return nil - } - return &metav1.StatusCause{ - Message: err.Error(), - Field: field.String(), - } - } - - if err := cc.ValidateSnapshotClone(sourceSnapshot, spec); err != nil { - return &metav1.StatusCause{ - Type: metav1.CauseTypeFieldValueInvalid, - Message: err.Error(), - Field: field.String(), - } - } - - return nil -} - func validateStorageClassName(spec *cdiv1.DataVolumeSpec, field *k8sfield.Path) *metav1.StatusCause { var sc *string diff --git a/pkg/apiserver/webhooks/datavolume-validate_test.go b/pkg/apiserver/webhooks/datavolume-validate_test.go index 6dc284d2d8..d171b76949 100644 --- a/pkg/apiserver/webhooks/datavolume-validate_test.go +++ b/pkg/apiserver/webhooks/datavolume-validate_test.go @@ -30,7 +30,6 @@ import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" - snapshotv1 "github.com/kubernetes-csi/external-snapshotter/client/v6/apis/volumesnapshot/v1" snapclientfake "github.com/kubernetes-csi/external-snapshotter/client/v6/clientset/versioned/fake" admissionv1 "k8s.io/api/admission/v1" @@ -620,55 +619,6 @@ var _ = Describe("Validating Webhook", func() { Expect(resp.Allowed).To(BeTrue()) }) - It("should reject snapshot clone when input size is lower than recommended restore size", func() { - size := resource.MustParse("1G") - snapshot := &snapshotv1.VolumeSnapshot{ - ObjectMeta: metav1.ObjectMeta{ - Name: "testsnap", - Namespace: testNamespace, - }, - Status: &snapshotv1.VolumeSnapshotStatus{ - RestoreSize: &size, - }, - } - storage := &cdiv1.StorageSpec{ - Resources: corev1.VolumeResourceRequirements{ - Requests: corev1.ResourceList{ - corev1.ResourceStorage: resource.MustParse("500Mi"), - }, - }, - } - snapSource := &cdiv1.DataVolumeSource{ - Snapshot: &cdiv1.DataVolumeSourceSnapshot{ - Namespace: snapshot.Namespace, - Name: snapshot.Name, - }, - } - dv := newDataVolumeWithStorageSpec("testDV", snapSource, nil, storage) - resp := validateDataVolumeCreateEx(dv, nil, nil, []runtime.Object{snapshot}, nil) - Expect(resp.Allowed).To(BeFalse()) - }) - - It("should reject snapshot clone when no input size/recommended restore size", func() { - snapshot := &snapshotv1.VolumeSnapshot{ - ObjectMeta: metav1.ObjectMeta{ - Name: "testsnap", - Namespace: testNamespace, - }, - Status: &snapshotv1.VolumeSnapshotStatus{}, - } - storage := &cdiv1.StorageSpec{} - snapSource := &cdiv1.DataVolumeSource{ - Snapshot: &cdiv1.DataVolumeSourceSnapshot{ - Namespace: snapshot.Namespace, - Name: snapshot.Name, - }, - } - dv := newDataVolumeWithStorageSpec("testDV", snapSource, nil, storage) - resp := validateDataVolumeCreateEx(dv, nil, nil, []runtime.Object{snapshot}, nil) - Expect(resp.Allowed).To(BeFalse()) - }) - DescribeTable("should", func(oldFinalCheckpoint bool, oldCheckpoints []string, newFinalCheckpoint bool, newCheckpoints []string, modifyDV func(*cdiv1.DataVolume), expectedSuccess bool, sourceFunc func() *cdiv1.DataVolumeSource) { oldDV := newMultistageDataVolume("multi-stage", oldFinalCheckpoint, oldCheckpoints, sourceFunc) oldBytes, _ := json.Marshal(&oldDV) @@ -844,14 +794,14 @@ var _ = Describe("Validating Webhook", func() { Entry("accept DataVolume with PVC and sourceRef missing namespace on create", &emptyNamespace), ) - It("should reject DataVolume with SourceRef on create if DataSource does not exist", func() { + It("should allow DataVolume with SourceRef on create if DataSource does not exist", func() { ns := "testNamespace" dataVolume := newDataSourceDataVolume("testDV", &ns, "test") resp := validateDataVolumeCreate(dataVolume) - Expect(resp.Allowed).To(BeFalse()) + Expect(resp.Allowed).To(BeTrue()) }) - It("should reject DataVolume with SourceRef on create if DataSource exists but its PVC field is not populated", func() { + It("should allow DataVolume with SourceRef on create if DataSource exists but its PVC field is not populated", func() { dataVolume := newDataSourceDataVolume("testDV", &testNamespace, "test") dataSource := &cdiv1.DataSource{ ObjectMeta: metav1.ObjectMeta{ @@ -865,7 +815,7 @@ var _ = Describe("Validating Webhook", func() { }, } resp := validateDataVolumeCreateEx(dataVolume, nil, []runtime.Object{dataSource}, nil, nil) - Expect(resp.Allowed).To(BeFalse()) + Expect(resp.Allowed).To(BeTrue()) }) It("should accept DataVolume with SourceRef on create if DataSource exists but PVC does not exist", func() { diff --git a/pkg/controller/common/util.go b/pkg/controller/common/util.go index b894526f16..66bd1b4de8 100644 --- a/pkg/controller/common/util.go +++ b/pkg/controller/common/util.go @@ -966,83 +966,25 @@ func validateTokenData(tokenData *token.Payload, srcNamespace, srcName, targetNa return nil } -// validateContentTypes compares the content type of a clone DV against its source PVC's one -func validateContentTypes(sourcePVC *corev1.PersistentVolumeClaim, spec *cdiv1.DataVolumeSpec) (bool, cdiv1.DataVolumeContentType, cdiv1.DataVolumeContentType) { - sourceContentType := GetPVCContentType(sourcePVC) - targetContentType := spec.ContentType - if targetContentType == "" { - targetContentType = cdiv1.DataVolumeKubeVirt - } - return sourceContentType == targetContentType, sourceContentType, targetContentType -} - -// ValidateClone compares a clone spec against its source PVC to validate its creation -func ValidateClone(sourcePVC *corev1.PersistentVolumeClaim, spec *cdiv1.DataVolumeSpec) error { - var targetResources corev1.VolumeResourceRequirements - - valid, sourceContentType, targetContentType := validateContentTypes(sourcePVC, spec) - if !valid { - msg := fmt.Sprintf("Source contentType (%s) and target contentType (%s) do not match", sourceContentType, targetContentType) - return errors.New(msg) - } - - isSizelessClone := false - explicitPvcRequest := spec.PVC != nil - if explicitPvcRequest { - targetResources = spec.PVC.Resources - } else { - targetResources = spec.Storage.Resources - // The storage size in the target DV can be empty - // when cloning using the 'Storage' API - if _, ok := targetResources.Requests[corev1.ResourceStorage]; !ok { - isSizelessClone = true - } - } - - // TODO: Spec.Storage API needs a better more complex check to validate clone size - to account for fsOverhead - // simple size comparison will not work here - if (!isSizelessClone && GetVolumeMode(sourcePVC) == corev1.PersistentVolumeBlock) || explicitPvcRequest { - if err := ValidateRequestedCloneSize(sourcePVC.Spec.Resources, targetResources); err != nil { - return err - } - } - - return nil -} - -// ValidateSnapshotClone compares a snapshot clone spec against its source snapshot to validate its creation -func ValidateSnapshotClone(sourceSnapshot *snapshotv1.VolumeSnapshot, spec *cdiv1.DataVolumeSpec) error { - var sourceResources, targetResources corev1.VolumeResourceRequirements - +// IsSnapshotValidForClone returns an error if the passed snapshot is not valid for cloning +func IsSnapshotValidForClone(sourceSnapshot *snapshotv1.VolumeSnapshot) error { if sourceSnapshot.Status == nil { - return fmt.Errorf("no status on source snapshot, not possible to proceed") + return fmt.Errorf("no status on source snapshot yet") } - size := sourceSnapshot.Status.RestoreSize - restoreSizeAvailable := size != nil && size.Sign() > 0 - if restoreSizeAvailable { - sourceResources.Requests = corev1.ResourceList{corev1.ResourceStorage: *size} + if !IsSnapshotReady(sourceSnapshot) { + klog.V(3).Info("snapshot not ReadyToUse, while we allow this, probably going to be an issue going forward", "namespace", sourceSnapshot.Namespace, "name", sourceSnapshot.Name) } - - isSizelessClone := false - explicitPvcRequest := spec.PVC != nil - if explicitPvcRequest { - targetResources = spec.PVC.Resources - } else { - targetResources = spec.Storage.Resources - if _, ok := targetResources.Requests["storage"]; !ok { - isSizelessClone = true + if sourceSnapshot.Status.Error != nil { + errMessage := "no details" + if msg := sourceSnapshot.Status.Error.Message; msg != nil { + errMessage = *msg } + return fmt.Errorf("snapshot in error state with msg: %s", errMessage) } - - if !isSizelessClone && restoreSizeAvailable { - // Sizes available, make sure user picked something bigger than minimal - if err := ValidateRequestedCloneSize(sourceResources, targetResources); err != nil { - return err - } - } else if isSizelessClone && !restoreSizeAvailable { - return fmt.Errorf("size not specified by user/provisioner, can't tell how much needed for restore") + if sourceSnapshot.Spec.VolumeSnapshotClassName == nil || + *sourceSnapshot.Spec.VolumeSnapshotClassName == "" { + return fmt.Errorf("snapshot %s/%s does not have volume snap class populated, can't clone", sourceSnapshot.Name, sourceSnapshot.Namespace) } - return nil } diff --git a/pkg/controller/common/util_test.go b/pkg/controller/common/util_test.go index a7e3ebe189..f3af6ce5e0 100644 --- a/pkg/controller/common/util_test.go +++ b/pkg/controller/common/util_test.go @@ -30,36 +30,6 @@ var _ = Describe("GetRequestedImageSize", func() { }) }) -var _ = Describe("validateContentTypes", func() { - getContentType := func(contentType string) cdiv1.DataVolumeContentType { - if contentType == "" { - return cdiv1.DataVolumeKubeVirt - } - return cdiv1.DataVolumeContentType(contentType) - } - - DescribeTable("should return", func(sourceContentType, targetContentType string, expectedResult bool) { - sourcePvc := CreatePvc("testPVC", "default", map[string]string{AnnContentType: sourceContentType}, nil) - dvSpec := &cdiv1.DataVolumeSpec{} - dvSpec.ContentType = cdiv1.DataVolumeContentType(targetContentType) - - validated, sourceContent, targetContent := validateContentTypes(sourcePvc, dvSpec) - Expect(validated).To(Equal(expectedResult)) - Expect(sourceContent).To(Equal(getContentType(sourceContentType))) - Expect(targetContent).To(Equal(getContentType(targetContentType))) - }, - Entry("true when using archive in source and target", string(cdiv1.DataVolumeArchive), string(cdiv1.DataVolumeArchive), true), - Entry("false when using archive in source and KubeVirt in target", string(cdiv1.DataVolumeArchive), string(cdiv1.DataVolumeKubeVirt), false), - Entry("false when using KubeVirt in source and archive in target", string(cdiv1.DataVolumeKubeVirt), string(cdiv1.DataVolumeArchive), false), - Entry("true when using KubeVirt in source and target", string(cdiv1.DataVolumeKubeVirt), string(cdiv1.DataVolumeKubeVirt), true), - Entry("true when using default in source and target", "", "", true), - Entry("true when using default in source and KubeVirt (explicit) in target", "", string(cdiv1.DataVolumeKubeVirt), true), - Entry("true when using KubeVirt (explicit) in source and default in target", string(cdiv1.DataVolumeKubeVirt), "", true), - Entry("false when using default in source and archive in target", "", string(cdiv1.DataVolumeArchive), false), - Entry("false when using archive in source and default in target", string(cdiv1.DataVolumeArchive), "", false), - ) -}) - var _ = Describe("GetStorageClassByName", func() { It("Should return the default storage class name", func() { client := CreateClient( diff --git a/pkg/controller/datavolume/clone-controller-base.go b/pkg/controller/datavolume/clone-controller-base.go index 385ae09ab2..d5273eeed1 100644 --- a/pkg/controller/datavolume/clone-controller-base.go +++ b/pkg/controller/datavolume/clone-controller-base.go @@ -104,7 +104,7 @@ const ( // CloneValidationFailed reports that a clone wasn't admitted by our validation mechanism (reason) CloneValidationFailed = "CloneValidationFailed" // MessageCloneValidationFailed reports that a clone wasn't admitted by our validation mechanism (message) - MessageCloneValidationFailed = "The clone doesn't meet the validation requirements" + MessageCloneValidationFailed = "The clone doesn't meet the validation requirements: %s" // CloneWithoutSource reports that the source of a clone doesn't exists (reason) CloneWithoutSource = "CloneWithoutSource" // MessageCloneWithoutSource reports that the source of a clone doesn't exists (message) @@ -519,6 +519,10 @@ func (r *CloneReconcilerBase) populateSourceIfSourceRef(dv *cdiv1.DataVolume) er if err := r.client.Get(context.TODO(), types.NamespacedName{Name: dv.Spec.SourceRef.Name, Namespace: ns}, dataSource); err != nil { return err } + if dataSource.Spec.Source.PVC == nil && dataSource.Spec.Source.Snapshot == nil { + return errors.Errorf("Empty source field in '%s'. DataSource may not be ready yet", dataSource.Name) + } + dv.Spec.Source = &cdiv1.DataVolumeSource{ PVC: dataSource.Spec.Source.PVC, Snapshot: dataSource.Spec.Source.Snapshot, @@ -572,7 +576,7 @@ func addCloneWithoutSourceWatch(mgr manager.Manager, datavolumeController contro predicate.Funcs{ CreateFunc: func(e event.CreateEvent) bool { return true }, DeleteFunc: func(e event.DeleteEvent) bool { return false }, - UpdateFunc: func(e event.UpdateEvent) bool { return false }, + UpdateFunc: func(e event.UpdateEvent) bool { return true }, })); err != nil { return err } diff --git a/pkg/controller/datavolume/controller-base.go b/pkg/controller/datavolume/controller-base.go index d2789a86a3..b8c9f893ee 100644 --- a/pkg/controller/datavolume/controller-base.go +++ b/pkg/controller/datavolume/controller-base.go @@ -27,6 +27,7 @@ import ( "time" "github.com/go-logr/logr" + snapshotv1 "github.com/kubernetes-csi/external-snapshotter/client/v6/apis/volumesnapshot/v1" "github.com/pkg/errors" corev1 "k8s.io/api/core/v1" @@ -111,6 +112,7 @@ type dvSyncState struct { dvMutated *cdiv1.DataVolume pvc *corev1.PersistentVolumeClaim pvcSpec *corev1.PersistentVolumeClaimSpec + snapshot *snapshotv1.VolumeSnapshot dvSyncResult usePopulator bool } diff --git a/pkg/controller/datavolume/pvc-clone-controller.go b/pkg/controller/datavolume/pvc-clone-controller.go index 506bd94816..e9d18ef300 100644 --- a/pkg/controller/datavolume/pvc-clone-controller.go +++ b/pkg/controller/datavolume/pvc-clone-controller.go @@ -422,15 +422,67 @@ func (r *PvcCloneReconciler) validateCloneAndSourcePVC(syncState *dvSyncState, l return false, err } - err = cc.ValidateClone(sourcePvc, &datavolume.Spec) + err = validateClone(sourcePvc, &datavolume.Spec) if err != nil { - r.recorder.Event(datavolume, corev1.EventTypeWarning, CloneValidationFailed, MessageCloneValidationFailed) - return false, err + syncErr := r.syncDataVolumeStatusPhaseWithEvent(syncState, datavolume.Status.Phase, nil, + Event{ + eventType: corev1.EventTypeWarning, + reason: CloneValidationFailed, + message: fmt.Sprintf(MessageCloneValidationFailed, err.Error()), + }) + if syncErr != nil { + log.Error(syncErr, "failed to sync DataVolume status with event") + } + return false, nil } return true, nil } +// validateClone compares a clone spec against its source PVC to validate its creation +func validateClone(sourcePVC *corev1.PersistentVolumeClaim, spec *cdiv1.DataVolumeSpec) error { + var targetResources corev1.VolumeResourceRequirements + + valid, sourceContentType, targetContentType := validateContentTypes(sourcePVC, spec) + if !valid { + msg := fmt.Sprintf("Source contentType (%s) and target contentType (%s) do not match", sourceContentType, targetContentType) + return errors.New(msg) + } + + isSizelessClone := false + explicitPvcRequest := spec.PVC != nil + if explicitPvcRequest { + targetResources = spec.PVC.Resources + } else { + targetResources = spec.Storage.Resources + // The storage size in the target DV can be empty + // when cloning using the 'Storage' API + if _, ok := targetResources.Requests[corev1.ResourceStorage]; !ok { + isSizelessClone = true + } + } + + // TODO: Spec.Storage API needs a better more complex check to validate clone size - to account for fsOverhead + // simple size comparison will not work here + if (!isSizelessClone && cc.GetVolumeMode(sourcePVC) == corev1.PersistentVolumeBlock) || explicitPvcRequest { + if err := cc.ValidateRequestedCloneSize(sourcePVC.Spec.Resources, targetResources); err != nil { + return err + } + } + + return nil +} + +// validateContentTypes compares the content type of a clone DV against its source PVC's one +func validateContentTypes(sourcePVC *corev1.PersistentVolumeClaim, spec *cdiv1.DataVolumeSpec) (bool, cdiv1.DataVolumeContentType, cdiv1.DataVolumeContentType) { + sourceContentType := cc.GetPVCContentType(sourcePVC) + targetContentType := spec.ContentType + if targetContentType == "" { + targetContentType = cdiv1.DataVolumeKubeVirt + } + return sourceContentType == targetContentType, sourceContentType, targetContentType +} + // isSourceReadyToClone handles the reconciling process of a clone when the source PVC is not ready func (r *PvcCloneReconciler) isSourceReadyToClone(datavolume *cdiv1.DataVolume) (bool, error) { // TODO preper const diff --git a/pkg/controller/datavolume/pvc-clone-controller_test.go b/pkg/controller/datavolume/pvc-clone-controller_test.go index 8011967c25..ae06ed0b5c 100644 --- a/pkg/controller/datavolume/pvc-clone-controller_test.go +++ b/pkg/controller/datavolume/pvc-clone-controller_test.go @@ -18,6 +18,7 @@ package datavolume import ( "context" + "fmt" "reflect" "strings" "time" @@ -421,6 +422,235 @@ var _ = Describe("All DataVolume Tests", func() { }) }) + var _ = Describe("validateContentTypes", func() { + getContentType := func(contentType string) cdiv1.DataVolumeContentType { + if contentType == "" { + return cdiv1.DataVolumeKubeVirt + } + return cdiv1.DataVolumeContentType(contentType) + } + + DescribeTable("should return", func(sourceContentType, targetContentType string, expectedResult bool) { + sourcePvc := CreatePvc("testPVC", "default", map[string]string{AnnContentType: sourceContentType}, nil) + dvSpec := &cdiv1.DataVolumeSpec{} + dvSpec.ContentType = cdiv1.DataVolumeContentType(targetContentType) + + validated, sourceContent, targetContent := validateContentTypes(sourcePvc, dvSpec) + Expect(validated).To(Equal(expectedResult)) + Expect(sourceContent).To(Equal(getContentType(sourceContentType))) + Expect(targetContent).To(Equal(getContentType(targetContentType))) + }, + Entry("true when using archive in source and target", string(cdiv1.DataVolumeArchive), string(cdiv1.DataVolumeArchive), true), + Entry("false when using archive in source and KubeVirt in target", string(cdiv1.DataVolumeArchive), string(cdiv1.DataVolumeKubeVirt), false), + Entry("false when using KubeVirt in source and archive in target", string(cdiv1.DataVolumeKubeVirt), string(cdiv1.DataVolumeArchive), false), + Entry("true when using KubeVirt in source and target", string(cdiv1.DataVolumeKubeVirt), string(cdiv1.DataVolumeKubeVirt), true), + Entry("true when using default in source and target", "", "", true), + Entry("true when using default in source and KubeVirt (explicit) in target", "", string(cdiv1.DataVolumeKubeVirt), true), + Entry("true when using KubeVirt (explicit) in source and default in target", string(cdiv1.DataVolumeKubeVirt), "", true), + Entry("false when using default in source and archive in target", "", string(cdiv1.DataVolumeArchive), false), + Entry("false when using archive in source and default in target", string(cdiv1.DataVolumeArchive), "", false), + ) + }) + + var _ = Describe("validateClone", func() { + sourcePvc := CreatePvc("testPVC", "default", map[string]string{}, nil) + blockVM := corev1.PersistentVolumeBlock + fsVM := corev1.PersistentVolumeFilesystem + + It("Should reject the clone if source and target have different content types", func() { + sourcePvc.Annotations[AnnContentType] = string(cdiv1.DataVolumeKubeVirt) + dvSpec := &cdiv1.DataVolumeSpec{ContentType: cdiv1.DataVolumeArchive} + + err := validateClone(sourcePvc, dvSpec) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring( + fmt.Sprintf("Source contentType (%s) and target contentType (%s) do not match", cdiv1.DataVolumeKubeVirt, cdiv1.DataVolumeArchive))) + }) + + It("Should reject the clone if the target has an incompatible size and the source PVC is using block volumeMode (Storage API)", func() { + sourcePvc.Annotations[AnnContentType] = string(cdiv1.DataVolumeKubeVirt) + sourcePvc.Spec.VolumeMode = &blockVM + storageSpec := &cdiv1.StorageSpec{ + Resources: corev1.VolumeResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceStorage: resource.MustParse("1Mi"), // Less than the source's one (1Gi) + }, + }, + } + dvSpec := &cdiv1.DataVolumeSpec{Storage: storageSpec} + + err := validateClone(sourcePvc, dvSpec) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("target resources requests storage size is smaller than the source")) + }) + + It("Should validate the clone when source PVC is using fs volumeMode, even if the target has an incompatible size (Storage API)", func() { + sourcePvc.Annotations[AnnContentType] = string(cdiv1.DataVolumeKubeVirt) + sourcePvc.Spec.VolumeMode = &fsVM + storageSpec := &cdiv1.StorageSpec{ + Resources: corev1.VolumeResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceStorage: resource.MustParse("1Mi"), // Less than the source's one (1Gi) + }, + }, + } + dvSpec := &cdiv1.DataVolumeSpec{Storage: storageSpec} + + err := validateClone(sourcePvc, dvSpec) + Expect(err).ToNot(HaveOccurred()) + }) + + It("Should validate the clone if target's size is empty, even when the source uses block volumeMode (Storage API)", func() { + sourcePvc.Annotations[AnnContentType] = string(cdiv1.DataVolumeKubeVirt) + sourcePvc.Spec.VolumeMode = &blockVM + storageSpec := &cdiv1.StorageSpec{} + dvSpec := &cdiv1.DataVolumeSpec{Storage: storageSpec} + + err := validateClone(sourcePvc, dvSpec) + Expect(err).ToNot(HaveOccurred()) + }) + + It("Should reject the clone when the target has an incompatible size (PVC API)", func() { + sourcePvc.Annotations[AnnContentType] = string(cdiv1.DataVolumeKubeVirt) + pvcSpec := &corev1.PersistentVolumeClaimSpec{ + Resources: corev1.VolumeResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceStorage: resource.MustParse("1Mi"), // Less than the source's one (1Gi) + }, + }, + } + dvSpec := &cdiv1.DataVolumeSpec{PVC: pvcSpec} + + err := validateClone(sourcePvc, dvSpec) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("target resources requests storage size is smaller than the source")) + + }) + + It("Should validate the clone when both sizes are compatible (PVC API)", func() { + sourcePvc.Annotations[AnnContentType] = string(cdiv1.DataVolumeKubeVirt) + pvcSpec := &corev1.PersistentVolumeClaimSpec{ + Resources: corev1.VolumeResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceStorage: resource.MustParse("1Gi"), // Same as the source's + }, + }, + } + dvSpec := &cdiv1.DataVolumeSpec{PVC: pvcSpec} + + err := validateClone(sourcePvc, dvSpec) + Expect(err).ToNot(HaveOccurred()) + }) + }) + + var _ = Describe("validateCloneAndSourcePVC", func() { + scName := "testsc" + sc := CreateStorageClassWithProvisioner(scName, map[string]string{ + AnnDefaultStorageClass: "true", + }, map[string]string{}, "csi-plugin") + + syncState := func(dv *cdiv1.DataVolume) *dvSyncState { + return &dvSyncState{dv: dv, dvMutated: dv.DeepCopy()} + } + + DescribeTable("Validation mechanism rejects or accepts the clone depending on the contentType combination", + func(sourceContentType, targetContentType string, expectedResult bool) { + dv := newCloneDataVolume("test-dv") + dv.Spec.ContentType = cdiv1.DataVolumeContentType(targetContentType) + storageProfile := createStorageProfile(scName, nil, FilesystemMode) + reconciler = createCloneReconciler(dv, storageProfile, sc) + + done, err := reconciler.validateCloneAndSourcePVC(syncState(dv), reconciler.log) + Expect(err).ToNot(HaveOccurred()) + Expect(done).To(BeFalse()) + + // We create the source PVC after creating the clone + pvc := CreatePvcInStorageClass("test", metav1.NamespaceDefault, &scName, map[string]string{ + AnnContentType: sourceContentType}, nil, corev1.ClaimBound) + err = reconciler.client.Create(context.TODO(), pvc) + Expect(err).ToNot(HaveOccurred()) + + syncRes := syncState(dv) + done, err = reconciler.validateCloneAndSourcePVC(syncRes, reconciler.log) + Expect(done).To(Equal(expectedResult)) + Expect(err).ToNot(HaveOccurred()) + if expectedResult == false { + Expect(syncRes.phaseSync.event.reason).To(Equal(CloneValidationFailed)) + } else { + Expect(syncRes.phaseSync).To(BeNil()) + } + }, + Entry("Archive in source and target", string(cdiv1.DataVolumeArchive), string(cdiv1.DataVolumeArchive), true), + Entry("Archive in source and KubeVirt in target", string(cdiv1.DataVolumeArchive), string(cdiv1.DataVolumeKubeVirt), false), + Entry("KubeVirt in source and archive in target", string(cdiv1.DataVolumeKubeVirt), string(cdiv1.DataVolumeArchive), false), + Entry("KubeVirt in source and target", string(cdiv1.DataVolumeKubeVirt), string(cdiv1.DataVolumeKubeVirt), true), + Entry("Empty (KubeVirt by default) in source and target", "", "", true), + Entry("Empty (KubeVirt by default) in source and KubeVirt (explicit) in target", "", string(cdiv1.DataVolumeKubeVirt), true), + Entry("KubeVirt (explicit) in source and empty (KubeVirt by default) in target", string(cdiv1.DataVolumeKubeVirt), "", true), + Entry("Empty (kubeVirt by default) in source and archive in target", "", string(cdiv1.DataVolumeArchive), false), + Entry("Archive in source and empty (KubeVirt by default) in target", string(cdiv1.DataVolumeArchive), "", false), + ) + + DescribeTable("Validation mechanism rejects or accepts the clone depending on the source and target size combination", + func(sourceSize, targetSize, expectedEvent string, explicitPvcRequest bool) { + dv := newCloneDataVolume("test-dv") + if !explicitPvcRequest { + dv.Spec.Storage = &cdiv1.StorageSpec{} + if targetSize != "" { + dv.Spec.Storage.Resources = corev1.VolumeResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceStorage: resource.MustParse(targetSize), + }, + } + } + dv.Spec.PVC = nil + } else { + dv.Spec.PVC.Resources.Requests[corev1.ResourceStorage] = resource.MustParse(targetSize) + } + + srcPvc := CreatePvcInStorageClass("test", "default", &scName, nil, nil, corev1.ClaimBound) + srcPvc.Spec.VolumeMode = &BlockMode + srcPvc.Spec.Resources.Requests[corev1.ResourceStorage] = resource.MustParse(sourceSize) + storageProfile := createStorageProfile(scName, nil, FilesystemMode) + reconciler = createCloneReconciler(dv, srcPvc, storageProfile, sc) + + syncRes := syncState(dv) + done, err := reconciler.validateCloneAndSourcePVC(syncRes, reconciler.log) + Expect(err).ToNot(HaveOccurred()) + if expectedEvent != "" { + Expect(syncRes.phaseSync.event.message).To(ContainSubstring(expectedEvent)) + Expect(done).To(BeFalse()) + } else { + Expect(syncRes.phaseSync).To(BeNil()) + Expect(done).To(BeTrue()) + } + }, + Entry("Explicit PVC request, target equal to source size should accept", "1Gi", "1Gi", "", true), + Entry("Explicit PVC request, target bigger then source size should accept", "1Gi", "2Gi", "", true), + Entry("Explicit PVC request, target smaller then source size should reject", "2Gi", "1Gi", "is smaller than the source", true), + Entry("Storage request, target equal to source size should accept", "1Gi", "1Gi", "", false), + Entry("Storage request, target bigger then source size should accept", "1Gi", "2Gi", "", false), + Entry("Storage request, target smaller then source size should reject", "2Gi", "1Gi", "is smaller than the source", false), + Entry("Storage request, no target size should accept", "1Gi", "", "", false), + ) + + It("should get event when target size is lower than source size", func() { + dv := newCloneDataVolume("test-dv") + storageProfile := createStorageProfile(scName, nil, FilesystemMode) + srcPvc := CreatePvcInStorageClass("test", "default", &scName, nil, nil, corev1.ClaimBound) + srcPvc.Spec.Resources.Requests[corev1.ResourceStorage] = resource.MustParse("2Gi") + reconciler = createCloneReconciler(dv, srcPvc, storageProfile, sc) + _, err := reconciler.Reconcile(context.TODO(), reconcile.Request{NamespacedName: types.NamespacedName{Name: "test-dv", Namespace: metav1.NamespaceDefault}}) + Expect(err).ToNot(HaveOccurred()) + dv = &cdiv1.DataVolume{} + err = reconciler.client.Get(context.TODO(), types.NamespacedName{Name: "test-dv", Namespace: metav1.NamespaceDefault}, dv) + Expect(err).ToNot(HaveOccurred()) + event := <-reconciler.recorder.(*record.FakeRecorder).Events + Expect(event).To(ContainSubstring(CloneValidationFailed)) + }) + + }) + var _ = Describe("Clone without source", func() { scName := "testsc" sc := CreateStorageClassWithProvisioner(scName, map[string]string{ @@ -572,42 +802,6 @@ var _ = Describe("All DataVolume Tests", func() { _, ok := pvc.Annotations[AnnCreatedForDataVolume] Expect(ok).To(BeFalse()) }) - - DescribeTable("Validation mechanism rejects or accepts the clone depending on the contentType combination", - func(sourceContentType, targetContentType string, expectedResult bool) { - dv := newCloneDataVolume("test-dv") - dv.Spec.ContentType = cdiv1.DataVolumeContentType(targetContentType) - storageProfile := createStorageProfile(scName, nil, FilesystemMode) - reconciler = createCloneReconciler(dv, storageProfile, sc) - - done, err := reconciler.validateCloneAndSourcePVC(syncState(dv), reconciler.log) - Expect(err).ToNot(HaveOccurred()) - Expect(done).To(BeFalse()) - - // We create the source PVC after creating the clone - pvc := CreatePvcInStorageClass("test", metav1.NamespaceDefault, &scName, map[string]string{ - AnnContentType: sourceContentType}, nil, corev1.ClaimBound) - err = reconciler.client.Create(context.TODO(), pvc) - Expect(err).ToNot(HaveOccurred()) - - done, err = reconciler.validateCloneAndSourcePVC(syncState(dv), reconciler.log) - Expect(done).To(Equal(expectedResult)) - if expectedResult == false { - Expect(err).To(HaveOccurred()) - } else { - Expect(err).ToNot(HaveOccurred()) - } - }, - Entry("Archive in source and target", string(cdiv1.DataVolumeArchive), string(cdiv1.DataVolumeArchive), true), - Entry("Archive in source and KubeVirt in target", string(cdiv1.DataVolumeArchive), string(cdiv1.DataVolumeKubeVirt), false), - Entry("KubeVirt in source and archive in target", string(cdiv1.DataVolumeKubeVirt), string(cdiv1.DataVolumeArchive), false), - Entry("KubeVirt in source and target", string(cdiv1.DataVolumeKubeVirt), string(cdiv1.DataVolumeKubeVirt), true), - Entry("Empty (KubeVirt by default) in source and target", "", "", true), - Entry("Empty (KubeVirt by default) in source and KubeVirt (explicit) in target", "", string(cdiv1.DataVolumeKubeVirt), true), - Entry("KubeVirt (explicit) in source and empty (KubeVirt by default) in target", string(cdiv1.DataVolumeKubeVirt), "", true), - Entry("Empty (kubeVirt by default) in source and archive in target", "", string(cdiv1.DataVolumeArchive), false), - Entry("Archive in source and empty (KubeVirt by default) in target", string(cdiv1.DataVolumeArchive), "", false), - ) }) var _ = Describe("Clone with empty storage size", func() { diff --git a/pkg/controller/datavolume/snapshot-clone-controller.go b/pkg/controller/datavolume/snapshot-clone-controller.go index be7bee3038..ec5806adee 100644 --- a/pkg/controller/datavolume/snapshot-clone-controller.go +++ b/pkg/controller/datavolume/snapshot-clone-controller.go @@ -206,19 +206,16 @@ func (r *SnapshotCloneReconciler) syncSnapshotClone(log logr.Logger, req reconci } if pvc == nil { + // Check if source snapshot exists and do proper validation before attempting to clone + if done, err := r.validateCloneAndSourceSnapshot(&syncRes); err != nil || !done { + return syncRes, err + } + if datavolume.Spec.Storage != nil { - done, err := r.detectCloneSize(log, &syncRes) + err := r.detectCloneSize(log, &syncRes) if err != nil { return syncRes, err } - if !done { - if syncRes.result == nil { - syncRes.result = &reconcile.Result{} - } - syncRes.result.RequeueAfter = sourceInUseRequeueDuration - // I think pending is more accurate but doing scheduled to be consistent with PVC controller - return syncRes, r.syncCloneStatusPhase(&syncRes, cdiv1.CloneScheduled, nil) - } } pvcModifier := r.updateAnnotations @@ -231,10 +228,8 @@ func (r *SnapshotCloneReconciler) syncSnapshotClone(log logr.Logger, req reconci } pvcModifier = r.updatePVCForPopulation } else { - if done, err := r.validateAndInitLegacyClone(&syncRes); err != nil { + if err := r.initLegacyClone(&syncRes); err != nil { return syncRes, err - } else if !done { - return syncRes, nil } } @@ -279,67 +274,42 @@ func (r *SnapshotCloneReconciler) syncSnapshotClone(log logr.Logger, req reconci return syncRes, syncErr } -func (r *SnapshotCloneReconciler) detectCloneSize(log logr.Logger, syncState *dvSyncState) (bool, error) { +func (r *SnapshotCloneReconciler) detectCloneSize(log logr.Logger, syncState *dvSyncState) error { pvcSpec := syncState.pvcSpec requestedSize := pvcSpec.Resources.Requests[corev1.ResourceStorage] if !requestedSize.IsZero() { log.V(3).Info("requested size is set, skipping size detection", "size", requestedSize) - return true, nil - } - - datavolume := syncState.dvMutated - nn := types.NamespacedName{Namespace: datavolume.Spec.Source.Snapshot.Namespace, Name: datavolume.Spec.Source.Snapshot.Name} - snapshot := &snapshotv1.VolumeSnapshot{} - if err := r.client.Get(context.TODO(), nn, snapshot); err != nil { - if k8serrors.IsNotFound(err) { - log.V(3).Info("snapshot source does not exist", "namespace", nn.Namespace, "name", nn.Name) - return false, nil - } - - return false, err + return nil } - - if snapshot.Status == nil || snapshot.Status.RestoreSize == nil { - log.V(3).Info("snapshot source does not have restoreSize", "namespace", nn.Namespace, "name", nn.Name) - return false, nil + // shouldn't happen after validation of clone source exist + if syncState.snapshot == nil { + return fmt.Errorf("snapshot source does not exist") } if pvcSpec.Resources.Requests == nil { pvcSpec.Resources.Requests = corev1.ResourceList{} } - pvcSpec.Resources.Requests[corev1.ResourceStorage] = *snapshot.Status.RestoreSize + pvcSpec.Resources.Requests[corev1.ResourceStorage] = *syncState.snapshot.Status.RestoreSize log.V(3).Info("set pvc request size", "size", pvcSpec.Resources.Requests[corev1.ResourceStorage]) - return true, nil + return nil } -func (r *SnapshotCloneReconciler) validateAndInitLegacyClone(syncState *dvSyncState) (bool, error) { - // Check if source snapshot exists and do proper validation before attempting to clone - if done, err := r.validateCloneAndSourceSnapshot(syncState); err != nil { - return false, err - } else if !done { - return false, nil - } - +func (r *SnapshotCloneReconciler) initLegacyClone(syncState *dvSyncState) error { datavolume := syncState.dvMutated pvcSpec := syncState.pvcSpec nn := types.NamespacedName{Namespace: datavolume.Spec.Source.Snapshot.Namespace, Name: datavolume.Spec.Source.Snapshot.Name} snapshot := &snapshotv1.VolumeSnapshot{} if err := r.client.Get(context.TODO(), nn, snapshot); err != nil { - return false, err - } - - valid, err := r.isSnapshotValidForClone(snapshot) - if err != nil || !valid { - return false, err + return err } if err := r.createTempHostAssistedSourcePvc(datavolume, snapshot, pvcSpec, syncState); err != nil { - return false, err + return err } - return true, nil + return nil } // validateCloneAndSourceSnapshot checks if the source snapshot of a clone exists and does proper validation @@ -364,16 +334,66 @@ func (r *SnapshotCloneReconciler) validateCloneAndSourceSnapshot(syncState *dvSy } return false, err } + syncState.snapshot = snapshot - err = cc.ValidateSnapshotClone(snapshot, &datavolume.Spec) + err = validateSnapshotClone(snapshot, &datavolume.Spec) if err != nil { - r.recorder.Event(datavolume, corev1.EventTypeWarning, CloneValidationFailed, MessageCloneValidationFailed) - return false, err + syncEventErr := r.syncDataVolumeStatusPhaseWithEvent(syncState, datavolume.Status.Phase, nil, + Event{ + eventType: corev1.EventTypeWarning, + reason: CloneValidationFailed, + message: fmt.Sprintf(MessageCloneValidationFailed, err.Error()), + }) + if syncEventErr != nil { + r.log.Error(syncEventErr, "failed sync status phase") + } + return false, nil } return true, nil } +// validateSnapshotClone compares a snapshot clone spec against its source snapshot to validate its creation +func validateSnapshotClone(sourceSnapshot *snapshotv1.VolumeSnapshot, spec *cdiv1.DataVolumeSpec) error { + err := cc.IsSnapshotValidForClone(sourceSnapshot) + if err != nil { + return err + } + + var sourceResources, targetResources corev1.VolumeResourceRequirements + size := sourceSnapshot.Status.RestoreSize + if size == nil || size.Sign() < 0 { + return fmt.Errorf("snapshot has no restore size") + } + // We allow restoresize to be 0 in case target has a reqested size + restoreSizeAvailable := size != nil && size.Sign() > 0 + if restoreSizeAvailable { + sourceResources.Requests = corev1.ResourceList{corev1.ResourceStorage: *size} + } + + isSizelessClone := false + explicitPvcRequest := spec.PVC != nil + if explicitPvcRequest { + targetResources = spec.PVC.Resources + } else { + targetResources = spec.Storage.Resources + if _, ok := targetResources.Requests["storage"]; !ok { + isSizelessClone = true + } + } + + if !isSizelessClone && restoreSizeAvailable { + // Sizes available, make sure user picked something bigger than minimal + if err := cc.ValidateRequestedCloneSize(sourceResources, targetResources); err != nil { + return err + } + } else if isSizelessClone && !restoreSizeAvailable { + return fmt.Errorf("size not specified by user/provisioner, can't tell how much needed for restore") + } + + return nil +} + func (r *SnapshotCloneReconciler) createTempHostAssistedSourcePvc(dv *cdiv1.DataVolume, snapshot *snapshotv1.VolumeSnapshot, targetPvcSpec *corev1.PersistentVolumeClaimSpec, syncState *dvSyncState) error { tempPvcName := getTempHostAssistedSourcePvcName(dv) tempHostAssistedSourcePvc, err := r.makePvcFromSnapshot(tempPvcName, dv, snapshot, targetPvcSpec) @@ -545,28 +565,6 @@ func (r *SnapshotCloneReconciler) cleanupHostAssistedSnapshotClone(dv *cdiv1.Dat return nil } -// isSnapshotValidForClone returns true if the passed snapshot is valid for cloning -func (r *SnapshotCloneReconciler) isSnapshotValidForClone(snapshot *snapshotv1.VolumeSnapshot) (bool, error) { - if snapshot.Status == nil { - r.log.V(3).Info("Snapshot does not have status populated yet") - return false, nil - } - if !cc.IsSnapshotReady(snapshot) { - r.log.V(3).Info("snapshot not ReadyToUse, while we allow this, probably going to be an issue going forward", "namespace", snapshot.Namespace, "name", snapshot.Name) - } - if snapshot.Status.Error != nil { - errMessage := "no details" - if msg := snapshot.Status.Error.Message; msg != nil { - errMessage = *msg - } - return false, fmt.Errorf("snapshot in error state with msg: %s", errMessage) - } - if snapshot.Spec.VolumeSnapshotClassName == nil || *snapshot.Spec.VolumeSnapshotClassName == "" { - return false, fmt.Errorf("snapshot %s/%s does not have volume snap class populated, can't clone", snapshot.Name, snapshot.Namespace) - } - return true, nil -} - func newPvcFromSnapshot(obj metav1.Object, name string, snapshot *snapshotv1.VolumeSnapshot, targetPvcSpec *corev1.PersistentVolumeClaimSpec) (*corev1.PersistentVolumeClaim, error) { targetPvcSpecCopy := targetPvcSpec.DeepCopy() restoreSize := snapshot.Status.RestoreSize diff --git a/pkg/controller/datavolume/snapshot-clone-controller_test.go b/pkg/controller/datavolume/snapshot-clone-controller_test.go index df854aa29b..c828ba5487 100644 --- a/pkg/controller/datavolume/snapshot-clone-controller_test.go +++ b/pkg/controller/datavolume/snapshot-clone-controller_test.go @@ -306,6 +306,120 @@ var _ = Describe("All DataVolume Tests", func() { Entry("with different namespace", "source-ns"), ) + var _ = Describe("validateSnapshotClone", func() { + type snapshotModifierFunc func(*snapshotv1.VolumeSnapshot) + + DescribeTable("Validation mechanism rejects or accepts the clone depending snapshot state", + func(expectedErr string, snapshotModifier snapshotModifierFunc) { + dv := newCloneFromSnapshotDataVolume("test-dv") + + snapshot := createSnapshotInVolumeSnapshotClass("test-snap", dv.Namespace, &expectedSnapshotClass, nil, nil, true) + snapshotModifier(snapshot) + + err := validateSnapshotClone(snapshot, &dv.Spec) + if expectedErr != "" { + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring(expectedErr)) + } else { + Expect(err).ToNot(HaveOccurred()) + } + }, + Entry("No snapshot status should reject", "no status", func(volumesnapshot *snapshotv1.VolumeSnapshot) { + volumesnapshot.Status = nil + }), + Entry("Snapshot not ready should accept", "", func(volumesnapshot *snapshotv1.VolumeSnapshot) { + volumesnapshot.Status.ReadyToUse = ptr.To[bool](false) + }), + Entry("Snapshot has Error should reject", "snapshot in error", func(volumesnapshot *snapshotv1.VolumeSnapshot) { + volumesnapshot.Status.Error = &snapshotv1.VolumeSnapshotError{} + }), + Entry("Snapshot has no volumesnapshotclassname should reject", "does not have volume snap class", func(volumesnapshot *snapshotv1.VolumeSnapshot) { + volumesnapshot.Spec.VolumeSnapshotClassName = nil + }), + Entry("Snapshot has empty volumesnapshotclassname should reject", "does not have volume snap class", func(volumesnapshot *snapshotv1.VolumeSnapshot) { + volumesnapshot.Spec.VolumeSnapshotClassName = ptr.To[string]("") + }), + Entry("valid snapshot should accept", "", func(volumesnapshot *snapshotv1.VolumeSnapshot) {}), + ) + + DescribeTable("Validation mechanism rejects or accepts the clone depending on the restoresize and target size combination", + func(restoreSize, targetSize, expectedErr string, explicitPvcRequest bool) { + dv := newCloneFromSnapshotDataVolume("test-dv") + if !explicitPvcRequest { + dv.Spec.Storage = &cdiv1.StorageSpec{} + if targetSize != "" { + dv.Spec.Storage.Resources = corev1.VolumeResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceStorage: resource.MustParse(targetSize), + }, + } + } + dv.Spec.PVC = nil + } else { + dv.Spec.PVC.Resources.Requests[corev1.ResourceStorage] = resource.MustParse(targetSize) + } + + snapshot := createSnapshotInVolumeSnapshotClass("test-snap", dv.Namespace, &expectedSnapshotClass, nil, nil, true) + restoreSizeParsed := resource.MustParse(restoreSize) + snapshot.Status.RestoreSize = &restoreSizeParsed + + err := validateSnapshotClone(snapshot, &dv.Spec) + if expectedErr != "" { + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring(expectedErr)) + } else { + Expect(err).ToNot(HaveOccurred()) + } + }, + Entry("Explicit PVC request, target equal to restoreSize should accept", "1Gi", "1Gi", "", true), + Entry("Explicit PVC request, target bigger then restoreSize should accept", "1Gi", "2Gi", "", true), + Entry("Explicit PVC request, target smaller then restoreSize should reject", "2Gi", "1Gi", "is smaller than the source", true), + Entry("Storage request, target equal to restoreSize should accept", "1Gi", "1Gi", "", false), + Entry("Storage request, target bigger then restoreSize should accept", "1Gi", "2Gi", "", false), + Entry("Storage request, target smaller then restoreSize should reject", "2Gi", "1Gi", "is smaller than the source", false), + Entry("Storage request, no target size should accept", "1Gi", "", "", false), + Entry("Storage request, no target size no restoreSize should reject", "0", "", "size not specified", false), + Entry("Storage request, have target size and zero restoreSize should accept", "0", "1Gi", "", false), + Entry("Storage request, have target size negative restoreSize should reject", "-1", "1Gi", "no restore size", false), + ) + }) + + It("should get event when input size is lower than recommended restore size", func() { + dv := newCloneFromSnapshotDataVolume("test-dv") + snapshot := createSnapshotInVolumeSnapshotClass("test-snap", dv.Namespace, &expectedSnapshotClass, nil, nil, true) + bigRestoreSize := resource.MustParse("2G") + snapshot.Status.RestoreSize = &bigRestoreSize + reconciler = createSnapshotCloneReconciler(storageClass, csiDriver, dv, snapshot) + result, err := reconciler.Reconcile(context.TODO(), reconcile.Request{NamespacedName: types.NamespacedName{Name: "test-dv", Namespace: metav1.NamespaceDefault}}) + Expect(err).ToNot(HaveOccurred()) + Expect(result.Requeue).To(BeFalse()) + Expect(result.RequeueAfter).To(BeZero()) + dv = &cdiv1.DataVolume{} + err = reconciler.client.Get(context.TODO(), types.NamespacedName{Name: "test-dv", Namespace: metav1.NamespaceDefault}, dv) + Expect(err).ToNot(HaveOccurred()) + event := <-reconciler.recorder.(*record.FakeRecorder).Events + Expect(event).To(ContainSubstring(CloneValidationFailed)) + }) + + It("should handle zero restore size when requested size exists", func() { + dv := newCloneFromSnapshotDataVolume("test-dv") + snapshot := createSnapshotInVolumeSnapshotClass("test-snap", dv.Namespace, &expectedSnapshotClass, nil, nil, true) + zeroRestoreSize := resource.MustParse("0") + snapshot.Status.RestoreSize = &zeroRestoreSize + reconciler = createSnapshotCloneReconciler(storageClass, csiDriver, dv, snapshot) + result, err := reconciler.Reconcile(context.TODO(), reconcile.Request{NamespacedName: types.NamespacedName{Name: "test-dv", Namespace: metav1.NamespaceDefault}}) + Expect(err).ToNot(HaveOccurred()) + Expect(result.Requeue).To(BeFalse()) + Expect(result.RequeueAfter).To(BeZero()) + dv = &cdiv1.DataVolume{} + err = reconciler.client.Get(context.TODO(), types.NamespacedName{Name: "test-dv", Namespace: metav1.NamespaceDefault}, dv) + Expect(err).ToNot(HaveOccurred()) + pvc := &corev1.PersistentVolumeClaim{} + err = reconciler.client.Get(context.TODO(), types.NamespacedName{Name: "test-dv", Namespace: metav1.NamespaceDefault}, pvc) + Expect(err).ToNot(HaveOccurred()) + Expect(pvc.Spec.Resources.Requests[corev1.ResourceStorage]).To(Equal(dv.Spec.PVC.Resources.Requests[corev1.ResourceStorage])) + }) + It("should handle size omitted", func() { dv := newCloneFromSnapshotDataVolume("test-dv") vm := corev1.PersistentVolumeFilesystem diff --git a/pkg/controller/util_test.go b/pkg/controller/util_test.go index 77fbb4eb10..a6d4f2d4b6 100644 --- a/pkg/controller/util_test.go +++ b/pkg/controller/util_test.go @@ -10,7 +10,6 @@ import ( "github.com/pkg/errors" v1 "k8s.io/api/core/v1" - "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/tools/record" @@ -375,97 +374,6 @@ var _ = Describe("GetPreallocation", func() { }) }) -var _ = Describe("ValidateClone", func() { - sourcePvc := CreatePvc("testPVC", "default", map[string]string{}, nil) - blockVM := v1.PersistentVolumeBlock - fsVM := v1.PersistentVolumeFilesystem - - It("Should reject the clone if source and target have different content types", func() { - sourcePvc.Annotations[AnnContentType] = string(cdiv1.DataVolumeKubeVirt) - dvSpec := &cdiv1.DataVolumeSpec{ContentType: cdiv1.DataVolumeArchive} - - err := ValidateClone(sourcePvc, dvSpec) - Expect(err).To(HaveOccurred()) - Expect(err.Error()).To(ContainSubstring( - fmt.Sprintf("Source contentType (%s) and target contentType (%s) do not match", cdiv1.DataVolumeKubeVirt, cdiv1.DataVolumeArchive))) - }) - - It("Should reject the clone if the target has an incompatible size and the source PVC is using block volumeMode (Storage API)", func() { - sourcePvc.Annotations[AnnContentType] = string(cdiv1.DataVolumeKubeVirt) - sourcePvc.Spec.VolumeMode = &blockVM - storageSpec := &cdiv1.StorageSpec{ - Resources: v1.VolumeResourceRequirements{ - Requests: v1.ResourceList{ - v1.ResourceStorage: resource.MustParse("1Mi"), // Less than the source's one (1Gi) - }, - }, - } - dvSpec := &cdiv1.DataVolumeSpec{Storage: storageSpec} - - err := ValidateClone(sourcePvc, dvSpec) - Expect(err).To(HaveOccurred()) - Expect(err.Error()).To(ContainSubstring("target resources requests storage size is smaller than the source")) - }) - - It("Should validate the clone when source PVC is using fs volumeMode, even if the target has an incompatible size (Storage API)", func() { - sourcePvc.Annotations[AnnContentType] = string(cdiv1.DataVolumeKubeVirt) - sourcePvc.Spec.VolumeMode = &fsVM - storageSpec := &cdiv1.StorageSpec{ - Resources: v1.VolumeResourceRequirements{ - Requests: v1.ResourceList{ - v1.ResourceStorage: resource.MustParse("1Mi"), // Less than the source's one (1Gi) - }, - }, - } - dvSpec := &cdiv1.DataVolumeSpec{Storage: storageSpec} - - err := ValidateClone(sourcePvc, dvSpec) - Expect(err).ToNot(HaveOccurred()) - }) - - It("Should validate the clone if target's size is empty, even when the source uses block volumeMode (Storage API)", func() { - sourcePvc.Annotations[AnnContentType] = string(cdiv1.DataVolumeKubeVirt) - sourcePvc.Spec.VolumeMode = &blockVM - storageSpec := &cdiv1.StorageSpec{} - dvSpec := &cdiv1.DataVolumeSpec{Storage: storageSpec} - - err := ValidateClone(sourcePvc, dvSpec) - Expect(err).ToNot(HaveOccurred()) - }) - - It("Should reject the clone when the target has an incompatible size (PVC API)", func() { - sourcePvc.Annotations[AnnContentType] = string(cdiv1.DataVolumeKubeVirt) - pvcSpec := &v1.PersistentVolumeClaimSpec{ - Resources: v1.VolumeResourceRequirements{ - Requests: v1.ResourceList{ - v1.ResourceStorage: resource.MustParse("1Mi"), // Less than the source's one (1Gi) - }, - }, - } - dvSpec := &cdiv1.DataVolumeSpec{PVC: pvcSpec} - - err := ValidateClone(sourcePvc, dvSpec) - Expect(err).To(HaveOccurred()) - Expect(err.Error()).To(ContainSubstring("target resources requests storage size is smaller than the source")) - - }) - - It("Should validate the clone when both sizes are compatible (PVC API)", func() { - sourcePvc.Annotations[AnnContentType] = string(cdiv1.DataVolumeKubeVirt) - pvcSpec := &v1.PersistentVolumeClaimSpec{ - Resources: v1.VolumeResourceRequirements{ - Requests: v1.ResourceList{ - v1.ResourceStorage: resource.MustParse("1Gi"), // Same as the source's - }, - }, - } - dvSpec := &cdiv1.DataVolumeSpec{PVC: pvcSpec} - - err := ValidateClone(sourcePvc, dvSpec) - Expect(err).ToNot(HaveOccurred()) - }) -}) - var _ = Describe("HandleFailedPod", func() { pvc := CreatePvc("test-pvc", "test-ns", nil, nil) podName := "test-pod" diff --git a/tests/cloner_test.go b/tests/cloner_test.go index 780a7839b5..f611978e03 100644 --- a/tests/cloner_test.go +++ b/tests/cloner_test.go @@ -1850,124 +1850,6 @@ var _ = Describe("all clone tests", func() { }) }) - var _ = Describe("Validate Data Volume clone to smaller size", func() { - f := framework.NewFramework(namespacePrefix) - tinyCoreIsoURL := func() string { return fmt.Sprintf(utils.TinyCoreIsoURL, f.CdiInstallNs) } - - var ( - sourceDv, targetDv *cdiv1.DataVolume - err error - ) - - AfterEach(func() { - if sourceDv != nil { - By("Cleaning up source DV") - err = utils.DeleteDataVolume(f.CdiClient, f.Namespace.Name, sourceDv.Name) - Expect(err).ToNot(HaveOccurred()) - } - if targetDv != nil { - By("Cleaning up target DV") - err = utils.DeleteDataVolume(f.CdiClient, f.Namespace.Name, targetDv.Name) - Expect(err).ToNot(HaveOccurred()) - if targetDv.Status.Phase == cdiv1.Succeeded { - validateCloneType(f, targetDv) - } - } - }) - - It("[rfe_id:1126][test_id:1896][crit:High][vendor:cnv-qe@redhat.com][level:component] Should not allow cloning into a smaller sized data volume", func() { - By("Creating a source from a real image") - sourceDv = utils.NewDataVolumeWithHTTPImport("source-dv", "50Mi", tinyCoreIsoURL()) - filesystem := v1.PersistentVolumeFilesystem - sourceDv.Spec.PVC.VolumeMode = &filesystem - sourceDv, err = utils.CreateDataVolumeFromDefinition(f.CdiClient, f.Namespace.Name, sourceDv) - _, _ = fmt.Fprintf(GinkgoWriter, "sourceDv %v", sourceDv) - - Expect(err).ToNot(HaveOccurred()) - f.ForceBindPvcIfDvIsWaitForFirstConsumer(sourceDv) - - By("Waiting for import to be completed") - Expect( - utils.WaitForDataVolumePhaseWithTimeout(f, f.Namespace.Name, cdiv1.Succeeded, sourceDv.Name, 3*90*time.Second), - ).To(Succeed()) - - By("Calculating the md5sum of the source data volume") - md5sum, err := f.GetMD5(f.Namespace, utils.PersistentVolumeClaimFromDataVolume(sourceDv), utils.DefaultImagePath, 0) - Expect(err).ToNot(HaveOccurred()) - By("Deleting verifier pod") - err = utils.DeleteVerifierPod(f.K8sClient, f.Namespace.Name) - Expect(err).ToNot(HaveOccurred()) - - By("Cloning from the source DataVolume to under sized target") - targetDv = utils.NewDataVolumeForImageCloning("target-dv-too-small", "15Mi", f.Namespace.Name, sourceDv.Name, sourceDv.Spec.PVC.StorageClassName, sourceDv.Spec.PVC.VolumeMode) - _, err = utils.CreateDataVolumeFromDefinition(f.CdiClient, f.Namespace.Name, targetDv) - Expect(err).To(HaveOccurred()) - Expect(strings.Contains(err.Error(), "target resources requests storage size is smaller than the source")).To(BeTrue()) - - By("Cloning from the source DataVolume to properly sized target") - targetDv = utils.NewDataVolumeForImageCloning("target-dv", "50Mi", f.Namespace.Name, sourceDv.Name, sourceDv.Spec.PVC.StorageClassName, sourceDv.Spec.PVC.VolumeMode) - targetDv, err = utils.CreateDataVolumeFromDefinition(f.CdiClient, f.Namespace.Name, targetDv) - Expect(err).ToNot(HaveOccurred()) - f.ForceBindPvcIfDvIsWaitForFirstConsumer(targetDv) - - By("Waiting for clone to be completed") - err = utils.WaitForDataVolumePhaseWithTimeout(f, f.Namespace.Name, cdiv1.Succeeded, targetDv.Name, 3*90*time.Second) - Expect(err).ToNot(HaveOccurred()) - Expect(f.VerifyTargetPVCContentMD5(f.Namespace, utils.PersistentVolumeClaimFromDataVolume(targetDv), utils.DefaultImagePath, md5sum[:32])).To(BeTrue()) - By("Deleting verifier pod") - err = utils.DeleteVerifierPod(f.K8sClient, f.Namespace.Name) - Expect(err).ToNot(HaveOccurred()) - - By("Verifying the image is sparse") - Expect(f.VerifySparse(f.Namespace, utils.PersistentVolumeClaimFromDataVolume(targetDv), utils.DefaultImagePath)).To(BeTrue()) - By("Deleting verifier pod") - err = utils.DeleteVerifierPod(f.K8sClient, f.Namespace.Name) - Expect(err).ToNot(HaveOccurred()) - }) - - It("[rfe_id:1126][test_id:1896][crit:High][vendor:cnv-qe@redhat.com][level:component] Should not allow cloning into a smaller sized data volume in block volume mode", func() { - if !f.IsBlockVolumeStorageClassAvailable() { - Skip("Storage Class for block volume is not available") - } - By("Creating a source from a real image") - sourceDv = utils.NewDataVolumeWithHTTPImportToBlockPV("source-dv", "200Mi", tinyCoreIsoURL(), f.BlockSCName) - sourceDv, err = utils.CreateDataVolumeFromDefinition(f.CdiClient, f.Namespace.Name, sourceDv) - Expect(err).ToNot(HaveOccurred()) - f.ForceBindPvcIfDvIsWaitForFirstConsumer(sourceDv) - - By("Waiting for import to be completed") - Expect( - utils.WaitForDataVolumePhaseWithTimeout(f, f.Namespace.Name, cdiv1.Succeeded, sourceDv.Name, 3*90*time.Second), - ).To(Succeed()) - - By("Calculating the md5sum of the source data volume") - md5sum, err := f.RunCommandAndCaptureOutput(utils.PersistentVolumeClaimFromDataVolume(sourceDv), "md5sum "+testBaseDir, true) - Expect(err).ToNot(HaveOccurred()) - _, _ = fmt.Fprintf(GinkgoWriter, "INFO: MD5SUM for source is: %s\n", md5sum[:32]) - - By("Cloning from the source DataVolume to under sized target") - targetDv = utils.NewDataVolumeForImageCloning("target-dv", "50Mi", f.Namespace.Name, sourceDv.Name, sourceDv.Spec.PVC.StorageClassName, sourceDv.Spec.PVC.VolumeMode) - _, err = utils.CreateDataVolumeFromDefinition(f.CdiClient, f.Namespace.Name, targetDv) - Expect(err).To(HaveOccurred()) - Expect(strings.Contains(err.Error(), "target resources requests storage size is smaller than the source")).To(BeTrue()) - - By("Cloning from the source DataVolume to properly sized target") - targetDv = utils.NewDataVolumeForImageCloning("target-dv", "200Mi", f.Namespace.Name, sourceDv.Name, sourceDv.Spec.PVC.StorageClassName, sourceDv.Spec.PVC.VolumeMode) - targetDv, err = utils.CreateDataVolumeFromDefinition(f.CdiClient, f.Namespace.Name, targetDv) - Expect(err).ToNot(HaveOccurred()) - f.ForceBindPvcIfDvIsWaitForFirstConsumer(targetDv) - - By("Waiting for clone to be completed") - err = utils.WaitForDataVolumePhaseWithTimeout(f, f.Namespace.Name, cdiv1.Succeeded, targetDv.Name, 3*90*time.Second) - Expect(err).ToNot(HaveOccurred()) - Expect(f.VerifyTargetPVCContentMD5(f.Namespace, utils.PersistentVolumeClaimFromDataVolume(targetDv), testBaseDir, md5sum[:32])).To(BeTrue()) - By("Deleting verifier pod") - err = utils.DeleteVerifierPod(f.K8sClient, f.Namespace.Name) - Expect(err).ToNot(HaveOccurred()) - }) - - }) - var _ = Describe("Block PV Cloner Test", func() { f := framework.NewFramework(namespacePrefix) @@ -2919,25 +2801,6 @@ var _ = Describe("all clone tests", func() { }) }) - Context("Validate source snapshot", func() { - It("[test_id:9719] Should reject when input size is lower than recommended restore size", func() { - if !f.IsSnapshotStorageClassAvailable() { - Skip("Clone from volumesnapshot does not work without snapshot capable storage") - } - - recommendedSnapSize := "2Gi" - volumeMode := v1.PersistentVolumeFilesystem - - By("Create source snapshot") - createSnapshot(recommendedSnapSize, &f.SnapshotSCName, volumeMode) - - cloneDV := utils.NewDataVolumeForSnapshotCloningAndStorageSpec("clone-from-snap", "500Mi", f.Namespace.Name, "snap-"+dataVolumeName, &f.SnapshotSCName, &volumeMode) - _, err := utils.CreateDataVolumeFromDefinition(f.CdiClient, f.Namespace.Name, cloneDV) - Expect(err).To(HaveOccurred()) - Expect(err.Error()).To(ContainSubstring("target resources requests storage size is smaller than the source")) - }) - }) - Context("sourceRef support", func() { DescribeTable("[test_id:9758] Should clone data from SourceRef snapshot DataSource", func(sizeless bool) { if !f.IsSnapshotStorageClassAvailable() { diff --git a/tests/datasource_test.go b/tests/datasource_test.go index d7e7bf6d45..ef47fce89c 100644 --- a/tests/datasource_test.go +++ b/tests/datasource_test.go @@ -227,15 +227,11 @@ var _ = Describe("DataSource", func() { dv, err = utils.CreateDataVolumeFromDefinition(f.CdiClient, f.Namespace.Name, dv) Expect(err).ToNot(HaveOccurred()) - pvc, err := utils.WaitForPVC(f.K8sClient, dv.Namespace, dv.Name) - Expect(err).ToNot(HaveOccurred()) - if pvc.Spec.DataSourceRef == nil || pvc.Spec.DataSourceRef.Kind != cdiv1.VolumeCloneSourceRef { - By("Verify DV conditions") - utils.WaitForConditions(f, dv.Name, dv.Namespace, time.Minute, pollingInterval, - &cdiv1.DataVolumeCondition{Type: cdiv1.DataVolumeBound, Status: corev1.ConditionUnknown, Message: "The source snapshot snap1 doesn't exist", Reason: dvc.CloneWithoutSource}, - &cdiv1.DataVolumeCondition{Type: cdiv1.DataVolumeReady, Status: corev1.ConditionFalse, Reason: dvc.CloneWithoutSource}, - &cdiv1.DataVolumeCondition{Type: cdiv1.DataVolumeRunning, Status: corev1.ConditionFalse}) - } + By("Verify DV conditions") + utils.WaitForConditions(f, dv.Name, dv.Namespace, time.Minute, pollingInterval, + &cdiv1.DataVolumeCondition{Type: cdiv1.DataVolumeBound, Status: corev1.ConditionUnknown, Message: "The source snapshot snap1 doesn't exist", Reason: dvc.CloneWithoutSource}, + &cdiv1.DataVolumeCondition{Type: cdiv1.DataVolumeReady, Status: corev1.ConditionFalse, Reason: dvc.CloneWithoutSource}, + &cdiv1.DataVolumeCondition{Type: cdiv1.DataVolumeRunning, Status: corev1.ConditionFalse}) f.ExpectEvent(dv.Namespace).Should(ContainSubstring(dvc.CloneWithoutSource)) By("Create snapshot so the DataSource will be ready")