diff --git a/pkg/controller/clone/planner.go b/pkg/controller/clone/planner.go index 9a4d23792..d46f04f99 100644 --- a/pkg/controller/clone/planner.go +++ b/pkg/controller/clone/planner.go @@ -56,6 +56,12 @@ const ( // MessageNoVolumeExpansion reports that no volume expansion is possible (message) MessageNoVolumeExpansion = "No volume expansion is possible" + + // NoProvisionerMatch reports that the storageclass provisioner does not match the volumesnapshotcontent driver (reason) + NoProvisionerMatch = "NoProvisionerMatch" + + // MessageNoProvisionerMatch reports that the storageclass provisioner does not match the volumesnapshotcontent driver (message) + MessageNoProvisionerMatch = "The storageclass provisioner does not match the volumesnapshotcontent driver" ) // Planner plans clone operations @@ -131,8 +137,14 @@ type ChooseStrategyArgs struct { DataSource *cdiv1.VolumeCloneSource } +// ChooseStrategyResult is result returned by ChooseStrategy function +type ChooseStrategyResult struct { + Strategy cdiv1.CDICloneStrategy + FallbackReason *string +} + // ChooseStrategy picks the strategy for a clone op -func (p *Planner) ChooseStrategy(ctx context.Context, args *ChooseStrategyArgs) (*cdiv1.CDICloneStrategy, error) { +func (p *Planner) ChooseStrategy(ctx context.Context, args *ChooseStrategyArgs) (*ChooseStrategyResult, error) { if IsDataSourcePVC(args.DataSource.Spec.Source.Kind) { args.Log.V(3).Info("Getting strategy for PVC source") return p.computeStrategyForSourcePVC(ctx, args) @@ -271,7 +283,9 @@ func (p *Planner) watchOwned(log logr.Logger, obj client.Object) error { return nil } -func (p *Planner) computeStrategyForSourcePVC(ctx context.Context, args *ChooseStrategyArgs) (*cdiv1.CDICloneStrategy, error) { +func (p *Planner) computeStrategyForSourcePVC(ctx context.Context, args *ChooseStrategyArgs) (*ChooseStrategyResult, error) { + res := &ChooseStrategyResult{} + if ok, err := p.validateTargetStorageClassAssignment(ctx, args); !ok || err != nil { return nil, err } @@ -326,29 +340,24 @@ func (p *Planner) computeStrategyForSourcePVC(ctx context.Context, args *ChooseS } if n == nil { - p.Recorder.Event(args.TargetClaim, corev1.EventTypeWarning, NoVolumeSnapshotClass, MessageNoVolumeSnapshotClass) - strategy = cdiv1.CloneStrategyHostAssisted + p.fallbackToHostAssisted(args.TargetClaim, res, NoVolumeSnapshotClass, MessageNoVolumeSnapshotClass) + return res, nil } } + res.Strategy = strategy if strategy == cdiv1.CloneStrategySnapshot || strategy == cdiv1.CloneStrategyCsiClone { - ok, err := p.validateAdvancedClonePVC(ctx, args, sourceClaim) - if err != nil { + if err := p.validateAdvancedClonePVC(ctx, args, res, sourceClaim); err != nil { return nil, err } - - if !ok { - strategy = cdiv1.CloneStrategyHostAssisted - } } - return &strategy, nil + return res, nil } -func (p *Planner) computeStrategyForSourceSnapshot(ctx context.Context, args *ChooseStrategyArgs) (*cdiv1.CDICloneStrategy, error) { - // Defaulting to host-assisted clone since it has fewer requirements - strategy := cdiv1.CloneStrategyHostAssisted +func (p *Planner) computeStrategyForSourceSnapshot(ctx context.Context, args *ChooseStrategyArgs) (*ChooseStrategyResult, error) { + res := &ChooseStrategyResult{} if ok, err := p.validateTargetStorageClassAssignment(ctx, args); !ok || err != nil { return nil, err @@ -380,8 +389,9 @@ func (p *Planner) computeStrategyForSourceSnapshot(ctx context.Context, args *Ch return nil, err } if !valid { + p.fallbackToHostAssisted(args.TargetClaim, res, NoProvisionerMatch, MessageNoProvisionerMatch) args.Log.V(3).Info("Provisioner differs, need to fall back to host assisted") - return &strategy, nil + return res, nil } // Lastly, do size validation to determine whether to use dumb or smart cloning @@ -389,11 +399,12 @@ func (p *Planner) computeStrategyForSourceSnapshot(ctx context.Context, args *Ch if err != nil { return nil, err } - if valid { - strategy = cdiv1.CloneStrategySnapshot + if !valid { + p.fallbackToHostAssisted(args.TargetClaim, res, NoVolumeExpansion, MessageNoVolumeExpansion) + return res, nil } - - return &strategy, nil + res.Strategy = cdiv1.CloneStrategySnapshot + return res, nil } func (p *Planner) validateTargetStorageClassAssignment(ctx context.Context, args *ChooseStrategyArgs) (bool, error) { @@ -435,37 +446,42 @@ func (p *Planner) validateSourcePVC(args *ChooseStrategyArgs, sourceClaim *corev return nil } -func (p *Planner) validateAdvancedClonePVC(ctx context.Context, args *ChooseStrategyArgs, sourceClaim *corev1.PersistentVolumeClaim) (bool, error) { +func (p *Planner) validateAdvancedClonePVC(ctx context.Context, args *ChooseStrategyArgs, res *ChooseStrategyResult, sourceClaim *corev1.PersistentVolumeClaim) error { if !SameVolumeMode(sourceClaim, args.TargetClaim) { - p.Recorder.Event(args.TargetClaim, corev1.EventTypeWarning, IncompatibleVolumeModes, MessageIncompatibleVolumeModes) + p.fallbackToHostAssisted(args.TargetClaim, res, IncompatibleVolumeModes, MessageIncompatibleVolumeModes) args.Log.V(3).Info("volume modes not compatible for advanced clone") - return false, nil + return nil } sc, err := GetStorageClassForClaim(ctx, p.Client, args.TargetClaim) if err != nil { - return false, err + return err } if sc == nil { args.Log.V(3).Info("target storage class not found") - return false, fmt.Errorf("target storage class not found") + return fmt.Errorf("target storage class not found") } srcCapacity, hasSrcCapacity := sourceClaim.Status.Capacity[corev1.ResourceStorage] targetRequest, hasTargetRequest := args.TargetClaim.Spec.Resources.Requests[corev1.ResourceStorage] allowExpansion := sc.AllowVolumeExpansion != nil && *sc.AllowVolumeExpansion if !hasSrcCapacity || !hasTargetRequest { - return false, fmt.Errorf("source/target size info missing") + return fmt.Errorf("source/target size info missing") } if srcCapacity.Cmp(targetRequest) < 0 && !allowExpansion { - p.Recorder.Event(args.TargetClaim, corev1.EventTypeWarning, NoVolumeExpansion, MessageNoVolumeExpansion) + p.fallbackToHostAssisted(args.TargetClaim, res, NoVolumeExpansion, MessageNoVolumeExpansion) args.Log.V(3).Info("advanced clone not possible, no volume expansion") - return false, nil } - return true, nil + return nil +} + +func (p *Planner) fallbackToHostAssisted(targetClaim *corev1.PersistentVolumeClaim, res *ChooseStrategyResult, reason, message string) { + res.Strategy = cdiv1.CloneStrategyHostAssisted + res.FallbackReason = &message + p.Recorder.Event(targetClaim, corev1.EventTypeWarning, reason, message) } func (p *Planner) planHostAssistedFromPVC(ctx context.Context, args *PlanArgs) ([]Phase, error) { diff --git a/pkg/controller/clone/planner_test.go b/pkg/controller/clone/planner_test.go index 5e6125f0f..1cd1300d8 100644 --- a/pkg/controller/clone/planner_test.go +++ b/pkg/controller/clone/planner_test.go @@ -267,9 +267,9 @@ var _ = Describe("Planner test", func() { Log: log, } planner := createPlanner() - strategy, err := planner.ChooseStrategy(context.Background(), args) + csr, err := planner.ChooseStrategy(context.Background(), args) Expect(err).ToNot(HaveOccurred()) - Expect(strategy).To(BeNil()) + Expect(csr).To(BeNil()) }) It("should error if emptystring storageclass name", func() { @@ -281,10 +281,10 @@ var _ = Describe("Planner test", func() { Log: log, } planner := createPlanner() - strategy, err := planner.ChooseStrategy(context.Background(), args) + csr, err := planner.ChooseStrategy(context.Background(), args) Expect(err).To(HaveOccurred()) Expect(err.Error()).To(Equal("claim has emptystring storageclass, will not work")) - Expect(strategy).To(BeNil()) + Expect(csr).To(BeNil()) }) It("should error if no storageclass exists", func() { @@ -294,10 +294,10 @@ var _ = Describe("Planner test", func() { Log: log, } planner := createPlanner() - strategy, err := planner.ChooseStrategy(context.Background(), args) + csr, err := planner.ChooseStrategy(context.Background(), args) Expect(err).To(HaveOccurred()) Expect(err.Error()).To(Equal("target storage class not found")) - Expect(strategy).To(BeNil()) + Expect(csr).To(BeNil()) }) It("should return nil if no source", func() { @@ -307,9 +307,9 @@ var _ = Describe("Planner test", func() { Log: log, } planner := createPlanner(createStorageClass()) - strategy, err := planner.ChooseStrategy(context.Background(), args) + csr, err := planner.ChooseStrategy(context.Background(), args) Expect(err).ToNot(HaveOccurred()) - Expect(strategy).To(BeNil()) + Expect(csr).To(BeNil()) expectEvent(planner, CloneWithoutSource) }) @@ -323,10 +323,10 @@ var _ = Describe("Planner test", func() { Log: log, } planner := createPlanner(createStorageClass(), source) - strategy, err := planner.ChooseStrategy(context.Background(), args) + csr, err := planner.ChooseStrategy(context.Background(), args) Expect(err).To(HaveOccurred()) Expect(err.Error()).To(HavePrefix("target resources requests storage size is smaller than the source")) - Expect(strategy).To(BeNil()) + Expect(csr).To(BeNil()) expectEvent(planner, CloneValidationFailed) }) @@ -337,10 +337,12 @@ var _ = Describe("Planner test", func() { Log: log, } planner := createPlanner(createStorageClass(), createSourceClaim()) - strategy, err := planner.ChooseStrategy(context.Background(), args) + csr, err := planner.ChooseStrategy(context.Background(), args) Expect(err).ToNot(HaveOccurred()) - Expect(strategy).ToNot(BeNil()) - Expect(*strategy).To(Equal(cdiv1.CloneStrategyHostAssisted)) + Expect(csr).ToNot(BeNil()) + Expect(csr.Strategy).To(Equal(cdiv1.CloneStrategyHostAssisted)) + Expect(csr.FallbackReason).ToNot(BeNil()) + Expect(*csr.FallbackReason).To(Equal(MessageNoVolumeSnapshotClass)) expectEvent(planner, NoVolumeSnapshotClass) }) @@ -351,10 +353,10 @@ var _ = Describe("Planner test", func() { Log: log, } planner := createPlanner(createStorageClass(), createSourceClaim(), createVolumeSnapshotClass(), createSourceVolume()) - strategy, err := planner.ChooseStrategy(context.Background(), args) + csr, err := planner.ChooseStrategy(context.Background(), args) Expect(err).ToNot(HaveOccurred()) - Expect(strategy).ToNot(BeNil()) - Expect(*strategy).To(Equal(cdiv1.CloneStrategySnapshot)) + Expect(csr).ToNot(BeNil()) + Expect(csr.Strategy).To(Equal(cdiv1.CloneStrategySnapshot)) }) It("should return snapshot with volumesnapshotclass (no source vol)", func() { @@ -364,10 +366,10 @@ var _ = Describe("Planner test", func() { Log: log, } planner := createPlanner(createStorageClass(), createSourceClaim(), createVolumeSnapshotClass()) - strategy, err := planner.ChooseStrategy(context.Background(), args) + csr, err := planner.ChooseStrategy(context.Background(), args) Expect(err).ToNot(HaveOccurred()) - Expect(strategy).ToNot(BeNil()) - Expect(*strategy).To(Equal(cdiv1.CloneStrategySnapshot)) + Expect(csr).ToNot(BeNil()) + Expect(csr.Strategy).To(Equal(cdiv1.CloneStrategySnapshot)) }) It("should return snapshot with volumesnapshotclass and source storageclass does not exist but same driver", func() { @@ -381,10 +383,10 @@ var _ = Describe("Planner test", func() { Log: log, } planner := createPlanner(createStorageClass(), createVolumeSnapshotClass(), sourceClaim, sourceVolume) - strategy, err := planner.ChooseStrategy(context.Background(), args) + csr, err := planner.ChooseStrategy(context.Background(), args) Expect(err).ToNot(HaveOccurred()) - Expect(strategy).ToNot(BeNil()) - Expect(*strategy).To(Equal(cdiv1.CloneStrategySnapshot)) + Expect(csr).ToNot(BeNil()) + Expect(csr.Strategy).To(Equal(cdiv1.CloneStrategySnapshot)) }) It("should returnsnapshot with bigger target", func() { @@ -396,10 +398,10 @@ var _ = Describe("Planner test", func() { Log: log, } planner := createPlanner(createStorageClass(), createSourceClaim(), createVolumeSnapshotClass()) - strategy, err := planner.ChooseStrategy(context.Background(), args) + csr, err := planner.ChooseStrategy(context.Background(), args) Expect(err).ToNot(HaveOccurred()) - Expect(strategy).ToNot(BeNil()) - Expect(*strategy).To(Equal(cdiv1.CloneStrategySnapshot)) + Expect(csr).ToNot(BeNil()) + Expect(csr.Strategy).To(Equal(cdiv1.CloneStrategySnapshot)) }) It("should return host assisted with bigger target and no volumeexpansion", func() { @@ -413,10 +415,12 @@ var _ = Describe("Planner test", func() { Log: log, } planner := createPlanner(storageClass, createSourceClaim(), createVolumeSnapshotClass()) - strategy, err := planner.ChooseStrategy(context.Background(), args) + csr, err := planner.ChooseStrategy(context.Background(), args) Expect(err).ToNot(HaveOccurred()) - Expect(strategy).ToNot(BeNil()) - Expect(*strategy).To(Equal(cdiv1.CloneStrategyHostAssisted)) + Expect(csr).ToNot(BeNil()) + Expect(csr.Strategy).To(Equal(cdiv1.CloneStrategyHostAssisted)) + Expect(csr.FallbackReason).ToNot(BeNil()) + Expect(*csr.FallbackReason).To(Equal(MessageNoVolumeExpansion)) expectEvent(planner, NoVolumeExpansion) }) @@ -430,10 +434,12 @@ var _ = Describe("Planner test", func() { Log: log, } planner := createPlanner(createStorageClass(), createVolumeSnapshotClass(), source) - strategy, err := planner.ChooseStrategy(context.Background(), args) + csr, err := planner.ChooseStrategy(context.Background(), args) Expect(err).ToNot(HaveOccurred()) - Expect(strategy).ToNot(BeNil()) - Expect(*strategy).To(Equal(cdiv1.CloneStrategyHostAssisted)) + Expect(csr).ToNot(BeNil()) + Expect(csr.Strategy).To(Equal(cdiv1.CloneStrategyHostAssisted)) + Expect(csr.FallbackReason).ToNot(BeNil()) + Expect(*csr.FallbackReason).To(Equal(MessageIncompatibleVolumeModes)) expectEvent(planner, IncompatibleVolumeModes) }) @@ -451,10 +457,10 @@ var _ = Describe("Planner test", func() { cdi.Spec.CloneStrategyOverride = &cs err = planner.Client.Update(context.Background(), cdi) Expect(err).ToNot(HaveOccurred()) - strategy, err := planner.ChooseStrategy(context.Background(), args) + csr, err := planner.ChooseStrategy(context.Background(), args) Expect(err).ToNot(HaveOccurred()) - Expect(strategy).ToNot(BeNil()) - Expect(*strategy).To(Equal(cdiv1.CloneStrategyCsiClone)) + Expect(csr).ToNot(BeNil()) + Expect(csr.Strategy).To(Equal(cdiv1.CloneStrategyCsiClone)) }) It("should return csi-clone if storage profile is set", func() { @@ -473,10 +479,10 @@ var _ = Describe("Planner test", func() { }, } planner := createPlanner(sp, createStorageClass(), createSourceClaim()) - strategy, err := planner.ChooseStrategy(context.Background(), args) + csr, err := planner.ChooseStrategy(context.Background(), args) Expect(err).ToNot(HaveOccurred()) - Expect(strategy).ToNot(BeNil()) - Expect(*strategy).To(Equal(cdiv1.CloneStrategyCsiClone)) + Expect(csr).ToNot(BeNil()) + Expect(csr.Strategy).To(Equal(cdiv1.CloneStrategyCsiClone)) }) }) @@ -499,9 +505,9 @@ var _ = Describe("Planner test", func() { Log: log, } planner := createPlanner(createStorageClass()) - strategy, err := planner.ChooseStrategy(context.Background(), args) + csr, err := planner.ChooseStrategy(context.Background(), args) Expect(err).ToNot(HaveOccurred()) - Expect(strategy).To(BeNil()) + Expect(csr).To(BeNil()) expectEvent(planner, CloneWithoutSource) }) @@ -512,10 +518,10 @@ var _ = Describe("Planner test", func() { Log: log, } planner := createPlanner() - strategy, err := planner.ChooseStrategy(context.Background(), args) + csr, err := planner.ChooseStrategy(context.Background(), args) Expect(err).To(HaveOccurred()) Expect(err.Error()).To(Equal("target storage class not found")) - Expect(strategy).To(BeNil()) + Expect(csr).To(BeNil()) }) It("should fail when snapshot doesn't have populated volumeSnapshotContent name", func() { @@ -528,10 +534,10 @@ var _ = Describe("Planner test", func() { Log: log, } planner := createPlanner(createStorageClass(), source) - strategy, err := planner.ChooseStrategy(context.Background(), args) + csr, err := planner.ChooseStrategy(context.Background(), args) Expect(err).To(HaveOccurred()) Expect(err.Error()).To(Equal("volumeSnapshotContent name not found")) - Expect(strategy).To(BeNil()) + Expect(csr).To(BeNil()) }) It("should fallback to host-assisted when snapshot and storage class provisioners differ", func() { @@ -543,10 +549,13 @@ var _ = Describe("Planner test", func() { Log: log, } planner := createPlanner(createStorageClass(), source, createDefaultVolumeSnapshotContent("test")) - strategy, err := planner.ChooseStrategy(context.Background(), args) + csr, err := planner.ChooseStrategy(context.Background(), args) Expect(err).ToNot(HaveOccurred()) - Expect(strategy).ToNot(BeNil()) - Expect(*strategy).To(Equal(cdiv1.CloneStrategyHostAssisted)) + Expect(csr).ToNot(BeNil()) + Expect(csr.Strategy).To(Equal(cdiv1.CloneStrategyHostAssisted)) + Expect(csr.FallbackReason).ToNot(BeNil()) + Expect(*csr.FallbackReason).To(Equal(MessageNoProvisionerMatch)) + expectEvent(planner, MessageNoProvisionerMatch) }) It("should fail if snapshot doesn't have restore size", func() { @@ -559,10 +568,10 @@ var _ = Describe("Planner test", func() { Log: log, } planner := createPlanner(createStorageClass(), source, createDefaultVolumeSnapshotContent("driver")) - strategy, err := planner.ChooseStrategy(context.Background(), args) + csr, err := planner.ChooseStrategy(context.Background(), args) Expect(err).To(HaveOccurred()) Expect(err.Error()).To(Equal("snapshot has no RestoreSize")) - Expect(strategy).To(BeNil()) + Expect(csr).To(BeNil()) }) It("should fallback to host-assisted when expansion is not supported", func() { @@ -576,10 +585,13 @@ var _ = Describe("Planner test", func() { sc := createStorageClass() sc.AllowVolumeExpansion = nil planner := createPlanner(sc, source, createDefaultVolumeSnapshotContent("driver")) - strategy, err := planner.ChooseStrategy(context.Background(), args) + csr, err := planner.ChooseStrategy(context.Background(), args) Expect(err).ToNot(HaveOccurred()) - Expect(strategy).ToNot(BeNil()) - Expect(*strategy).To(Equal(cdiv1.CloneStrategyHostAssisted)) + Expect(csr).ToNot(BeNil()) + Expect(csr.Strategy).To(Equal(cdiv1.CloneStrategyHostAssisted)) + Expect(csr.FallbackReason).ToNot(BeNil()) + Expect(*csr.FallbackReason).To(Equal(MessageNoVolumeExpansion)) + expectEvent(planner, NoVolumeExpansion) }) It("should do smart clone when meeting all prerequisites", func() { @@ -591,10 +603,10 @@ var _ = Describe("Planner test", func() { Log: log, } planner := createPlanner(createStorageClass(), source, createDefaultVolumeSnapshotContent("driver")) - strategy, err := planner.ChooseStrategy(context.Background(), args) + csr, err := planner.ChooseStrategy(context.Background(), args) Expect(err).ToNot(HaveOccurred()) - Expect(strategy).ToNot(BeNil()) - Expect(*strategy).To(Equal(cdiv1.CloneStrategySnapshot)) + Expect(csr).ToNot(BeNil()) + Expect(csr.Strategy).To(Equal(cdiv1.CloneStrategySnapshot)) }) }) }) diff --git a/pkg/controller/datavolume/clone-controller-base.go b/pkg/controller/datavolume/clone-controller-base.go index daca3adaa..c4718f288 100644 --- a/pkg/controller/datavolume/clone-controller-base.go +++ b/pkg/controller/datavolume/clone-controller-base.go @@ -19,6 +19,7 @@ package datavolume import ( "context" "fmt" + "reflect" "github.com/pkg/errors" corev1 "k8s.io/api/core/v1" @@ -114,6 +115,10 @@ const ( RebindInProgress = "RebindInProgress" // MessageRebindInProgress is a const for reporting target rebind MessageRebindInProgress = "Rebinding PersistentVolumeClaim for DataVolume %s/%s" + // NoPopulator reports CDI populator is not used so we fallback to host-assisted cloning (reason) + NoPopulator = "NoPopulator" + // NoPopulatorMessage reports CDI populator is not used so we fallback to host-assisted cloning (message) + NoPopulatorMessage = "In tree storage class does not support snapshot/clone" // AnnCSICloneRequest annotation associates object with CSI Clone Request AnnCSICloneRequest = "cdi.kubevirt.io/CSICloneRequest" @@ -242,6 +247,22 @@ func (r *CloneReconcilerBase) ensureExtendedTokenDV(dv *cdiv1.DataVolume) (bool, return true, nil } +func (r *CloneReconcilerBase) fallbackToHostAssisted(pvc *corev1.PersistentVolumeClaim) error { + pvcCpy := pvc.DeepCopy() + + cc.AddAnnotation(pvcCpy, cc.AnnCloneType, string(cdiv1.CloneStrategyHostAssisted)) + cc.AddAnnotation(pvcCpy, populators.AnnCloneFallbackReason, NoPopulatorMessage) + + if !reflect.DeepEqual(pvc, pvcCpy) { + r.recorder.Event(pvcCpy, corev1.EventTypeWarning, NoPopulator, NoPopulatorMessage) + if err := r.updatePVC(pvcCpy); err != nil { + return err + } + } + + return nil +} + func (r *CloneReconcilerBase) ensureExtendedTokenPVC(dv *cdiv1.DataVolume, pvc *corev1.PersistentVolumeClaim) error { if !isCrossNamespaceClone(dv) { return nil diff --git a/pkg/controller/datavolume/pvc-clone-controller.go b/pkg/controller/datavolume/pvc-clone-controller.go index 91fd75d89..8d03176ce 100644 --- a/pkg/controller/datavolume/pvc-clone-controller.go +++ b/pkg/controller/datavolume/pvc-clone-controller.go @@ -335,6 +335,9 @@ func (r *PvcCloneReconciler) syncClone(log logr.Logger, req reconcile.Request) ( } } else { cc.AddAnnotation(datavolume, cc.AnnCloneType, string(cdiv1.CloneStrategyHostAssisted)) + if err := r.fallbackToHostAssisted(pvc); err != nil { + return syncRes, err + } } if err := r.ensureExtendedTokenPVC(datavolume, pvc); err != nil { diff --git a/pkg/controller/datavolume/pvc-clone-controller_test.go b/pkg/controller/datavolume/pvc-clone-controller_test.go index 8b9b861e3..2ea8c00d0 100644 --- a/pkg/controller/datavolume/pvc-clone-controller_test.go +++ b/pkg/controller/datavolume/pvc-clone-controller_test.go @@ -20,6 +20,7 @@ import ( "context" "reflect" "strings" + "time" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" @@ -198,6 +199,44 @@ var _ = Describe("All DataVolume Tests", func() { Expect(dv.Annotations[AnnCloneType]).To(Equal(string(cdiv1.CloneStrategySnapshot))) }) + It("Fallback to host-assisted cloning when populator is not used", func() { + dv := newCloneDataVolume("test-dv") + dv.Annotations[AnnUsePopulator] = "false" + + anno := map[string]string{ + AnnExtendedCloneToken: "test-token", + AnnCloneType: string(cdiv1.CloneStrategySnapshot), + } + pvc := CreatePvcInStorageClass("test-dv", metav1.NamespaceDefault, &scName, anno, nil, corev1.ClaimPending) + pvc.OwnerReferences = append(pvc.OwnerReferences, metav1.OwnerReference{ + Kind: "DataVolume", + Controller: pointer.Bool(true), + Name: "test-dv", + UID: dv.UID, + }) + + reconciler = createCloneReconciler(storageClass, csiDriver, dv, pvc /*, vcs*/) + 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(Equal(2 * time.Second)) + + dv = &cdiv1.DataVolume{} + err = reconciler.client.Get(context.TODO(), types.NamespacedName{Name: "test-dv", Namespace: metav1.NamespaceDefault}, dv) + Expect(err).ToNot(HaveOccurred()) + Expect(dv.Annotations[AnnCloneType]).To(Equal(string(cdiv1.CloneStrategyHostAssisted))) + + pvc = &corev1.PersistentVolumeClaim{} + err = reconciler.client.Get(context.TODO(), types.NamespacedName{Name: "test-dv", Namespace: metav1.NamespaceDefault}, pvc) + Expect(err).ToNot(HaveOccurred()) + Expect(pvc.Annotations[AnnCloneType]).To(Equal(string(cdiv1.CloneStrategyHostAssisted))) + Expect(pvc.Annotations[populators.AnnCloneFallbackReason]).To(Equal(NoPopulatorMessage)) + + event := <-reconciler.recorder.(*record.FakeRecorder).Events + Expect(event).To(ContainSubstring(NoPopulator)) + Expect(event).To(ContainSubstring(NoPopulatorMessage)) + }) + DescribeTable("should map phase correctly", func(phaseName string, dvPhase cdiv1.DataVolumePhase, eventReason string) { dv := newCloneDataVolume("test-dv") anno := map[string]string{ diff --git a/pkg/controller/datavolume/snapshot-clone-controller.go b/pkg/controller/datavolume/snapshot-clone-controller.go index b01691269..3870f5398 100644 --- a/pkg/controller/datavolume/snapshot-clone-controller.go +++ b/pkg/controller/datavolume/snapshot-clone-controller.go @@ -261,6 +261,9 @@ func (r *SnapshotCloneReconciler) syncSnapshotClone(log logr.Logger, req reconci } } else { cc.AddAnnotation(datavolume, cc.AnnCloneType, string(cdiv1.CloneStrategyHostAssisted)) + if err := r.fallbackToHostAssisted(pvc); err != nil { + return syncRes, err + } } if err := r.ensureExtendedTokenPVC(datavolume, pvc); err != nil { diff --git a/pkg/controller/datavolume/snapshot-clone-controller_test.go b/pkg/controller/datavolume/snapshot-clone-controller_test.go index b03272c5f..8280332b7 100644 --- a/pkg/controller/datavolume/snapshot-clone-controller_test.go +++ b/pkg/controller/datavolume/snapshot-clone-controller_test.go @@ -20,6 +20,7 @@ import ( "context" "fmt" "strings" + "time" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" @@ -369,6 +370,44 @@ var _ = Describe("All DataVolume Tests", func() { Expect(dv.Annotations[AnnCloneType]).To(Equal(string(cdiv1.CloneStrategySnapshot))) }) + It("Fallback to host-assisted cloning when populator is not used", func() { + dv := newCloneFromSnapshotDataVolume("test-dv") + dv.Annotations[AnnUsePopulator] = "false" + + anno := map[string]string{ + AnnExtendedCloneToken: "test-token", + AnnCloneType: string(cdiv1.CloneStrategySnapshot), + } + pvc := CreatePvcInStorageClass("test-dv", metav1.NamespaceDefault, &scName, anno, nil, corev1.ClaimPending) + pvc.OwnerReferences = append(pvc.OwnerReferences, metav1.OwnerReference{ + Kind: "DataVolume", + Controller: pointer.Bool(true), + Name: "test-dv", + UID: dv.UID, + }) + + reconciler = createSnapshotCloneReconciler(storageClass, csiDriver, dv, pvc) + 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(Equal(2 * time.Second)) + + dv = &cdiv1.DataVolume{} + err = reconciler.client.Get(context.TODO(), types.NamespacedName{Name: "test-dv", Namespace: metav1.NamespaceDefault}, dv) + Expect(err).ToNot(HaveOccurred()) + Expect(dv.Annotations[AnnCloneType]).To(Equal(string(cdiv1.CloneStrategyHostAssisted))) + + pvc = &corev1.PersistentVolumeClaim{} + err = reconciler.client.Get(context.TODO(), types.NamespacedName{Name: "test-dv", Namespace: metav1.NamespaceDefault}, pvc) + Expect(err).ToNot(HaveOccurred()) + Expect(pvc.Annotations[AnnCloneType]).To(Equal(string(cdiv1.CloneStrategyHostAssisted))) + Expect(pvc.Annotations[populators.AnnCloneFallbackReason]).To(Equal(NoPopulatorMessage)) + + event := <-reconciler.recorder.(*record.FakeRecorder).Events + Expect(event).To(ContainSubstring(NoPopulator)) + Expect(event).To(ContainSubstring(NoPopulatorMessage)) + }) + DescribeTable("should map phase correctly", func(phaseName string, dvPhase cdiv1.DataVolumePhase, eventReason string) { dv := newCloneFromSnapshotDataVolume("test-dv") anno := map[string]string{ diff --git a/pkg/controller/populators/clone-populator.go b/pkg/controller/populators/clone-populator.go index efd653139..e62a7d43a 100644 --- a/pkg/controller/populators/clone-populator.go +++ b/pkg/controller/populators/clone-populator.go @@ -48,6 +48,9 @@ const ( // AnnCloneError has the error string for error phase AnnCloneError = "cdi.kubevirt.io/cloneError" + // AnnCloneFallbackReason has the host-assisted clone fallback reason + AnnCloneFallbackReason = "cdi.kubevirt.io/cloneFallbackReason" + // AnnDataSourceNamespace has the namespace of the DataSource // this will be deprecated when cross namespace datasource goes beta AnnDataSourceNamespace = "cdi.kubevirt.io/dataSourceNamespace" @@ -64,7 +67,7 @@ var desiredCloneAnnotations = map[string]struct{}{ // Planner is an interface to mock out planner implementation for testing type Planner interface { - ChooseStrategy(context.Context, *clone.ChooseStrategyArgs) (*cdiv1.CDICloneStrategy, error) + ChooseStrategy(context.Context, *clone.ChooseStrategyArgs) (*clone.ChooseStrategyResult, error) Plan(context.Context, *clone.PlanArgs) ([]clone.Phase, error) Cleanup(context.Context, logr.Logger, client.Object) error } @@ -194,18 +197,18 @@ func (r *ClonePopulatorReconciler) reconcilePending(ctx context.Context, log log return reconcile.Result{}, r.updateClonePhaseError(ctx, log, pvc, err) } - cs, err := r.getCloneStrategy(ctx, log, pvc, vcs) + csr, err := r.getCloneStrategy(ctx, log, pvc, vcs) if err != nil { return reconcile.Result{}, r.updateClonePhaseError(ctx, log, pvc, err) } - if cs == nil { + if csr == nil { log.V(3).Info("unable to choose clone strategy now") // TODO maybe create index/watch to deal with this return reconcile.Result{RequeueAfter: 5 * time.Second}, r.updateClonePhasePending(ctx, log, pvc) } - updated, err := r.initTargetClaim(ctx, log, pvc, vcs, *cs) + updated, err := r.initTargetClaim(ctx, log, pvc, vcs, csr) if err != nil { return reconcile.Result{}, r.updateClonePhaseError(ctx, log, pvc, err) } @@ -220,16 +223,15 @@ func (r *ClonePopulatorReconciler) reconcilePending(ctx context.Context, log log Log: log, TargetClaim: pvc, DataSource: vcs, - Strategy: *cs, + Strategy: csr.Strategy, } return r.planAndExecute(ctx, log, pvc, statusOnly, args) } -func (r *ClonePopulatorReconciler) getCloneStrategy(ctx context.Context, log logr.Logger, pvc *corev1.PersistentVolumeClaim, vcs *cdiv1.VolumeCloneSource) (*cdiv1.CDICloneStrategy, error) { - cs := getSavedCloneStrategy(pvc) - if cs != nil { - return cs, nil +func (r *ClonePopulatorReconciler) getCloneStrategy(ctx context.Context, log logr.Logger, pvc *corev1.PersistentVolumeClaim, vcs *cdiv1.VolumeCloneSource) (*clone.ChooseStrategyResult, error) { + if cs := getSavedCloneStrategy(pvc); cs != nil { + return &clone.ChooseStrategyResult{Strategy: *cs}, nil } args := &clone.ChooseStrategyArgs{ @@ -238,12 +240,7 @@ func (r *ClonePopulatorReconciler) getCloneStrategy(ctx context.Context, log log DataSource: vcs, } - cs, err := r.planner.ChooseStrategy(ctx, args) - if err != nil { - return nil, err - } - - return cs, nil + return r.planner.ChooseStrategy(ctx, args) } func (r *ClonePopulatorReconciler) planAndExecute(ctx context.Context, log logr.Logger, pvc *corev1.PersistentVolumeClaim, statusOnly bool, args *clone.PlanArgs) (reconcile.Result, error) { @@ -315,13 +312,16 @@ func (r *ClonePopulatorReconciler) reconcileDone(ctx context.Context, log logr.L return reconcile.Result{}, r.client.Update(ctx, claimCpy) } -func (r *ClonePopulatorReconciler) initTargetClaim(ctx context.Context, log logr.Logger, pvc *corev1.PersistentVolumeClaim, vcs *cdiv1.VolumeCloneSource, cs cdiv1.CDICloneStrategy) (bool, error) { +func (r *ClonePopulatorReconciler) initTargetClaim(ctx context.Context, log logr.Logger, pvc *corev1.PersistentVolumeClaim, vcs *cdiv1.VolumeCloneSource, csr *clone.ChooseStrategyResult) (bool, error) { claimCpy := pvc.DeepCopy() clone.AddCommonClaimLabels(claimCpy) - setSavedCloneStrategy(claimCpy, cs) + setSavedCloneStrategy(claimCpy, csr.Strategy) if claimCpy.Annotations[AnnClonePhase] == "" { cc.AddAnnotation(claimCpy, AnnClonePhase, clone.PendingPhaseName) } + if claimCpy.Annotations[AnnCloneFallbackReason] == "" && csr.FallbackReason != nil { + cc.AddAnnotation(claimCpy, AnnCloneFallbackReason, *csr.FallbackReason) + } cc.AddFinalizer(claimCpy, cloneFinalizer) if !apiequality.Semantic.DeepEqual(pvc, claimCpy) { diff --git a/pkg/controller/populators/clone-populator_test.go b/pkg/controller/populators/clone-populator_test.go index 988dc3181..33f0f2434 100644 --- a/pkg/controller/populators/clone-populator_test.go +++ b/pkg/controller/populators/clone-populator_test.go @@ -244,7 +244,7 @@ var _ = Describe("Clone populator tests", func() { It("should be pending and initialize target if choosestrategy returns something", func() { target, source := targetAndDataSource() reconciler := createClonePopulatorReconciler(target, storageClass(), source) - csr := cdiv1.CloneStrategySnapshot + csr := clone.ChooseStrategyResult{} reconciler.planner = &fakePlanner{ chooseStrategyResult: &csr, } @@ -252,7 +252,7 @@ var _ = Describe("Clone populator tests", func() { isDefaultResult(result, err) pvc := getTarget(reconciler.client) Expect(pvc.Annotations[AnnClonePhase]).To(Equal(string(clone.PendingPhaseName))) - Expect(pvc.Annotations[cc.AnnCloneType]).To(Equal(string(csr))) + Expect(pvc.Annotations[cc.AnnCloneType]).To(Equal(string(csr.Strategy))) Expect(pvc.Finalizers).To(ContainElement(cloneFinalizer)) }) @@ -395,14 +395,14 @@ var _ = Describe("Clone populator tests", func() { // fakePlanner implements Plan interface type fakePlanner struct { - chooseStrategyResult *cdiv1.CDICloneStrategy + chooseStrategyResult *clone.ChooseStrategyResult chooseStrategyError error planResult []clone.Phase planError error cleanupCalled bool } -func (p *fakePlanner) ChooseStrategy(ctx context.Context, args *clone.ChooseStrategyArgs) (*cdiv1.CDICloneStrategy, error) { +func (p *fakePlanner) ChooseStrategy(ctx context.Context, args *clone.ChooseStrategyArgs) (*clone.ChooseStrategyResult, error) { return p.chooseStrategyResult, p.chooseStrategyError } diff --git a/tests/BUILD.bazel b/tests/BUILD.bazel index ef0f6df46..49ff516a0 100644 --- a/tests/BUILD.bazel +++ b/tests/BUILD.bazel @@ -52,6 +52,7 @@ go_test( "//pkg/client/clientset/versioned:go_default_library", "//pkg/common:go_default_library", "//pkg/controller:go_default_library", + "//pkg/controller/clone:go_default_library", "//pkg/controller/common:go_default_library", "//pkg/controller/datavolume:go_default_library", "//pkg/controller/populators:go_default_library", diff --git a/tests/clone-populator_test.go b/tests/clone-populator_test.go index e88cfdaac..59e0307bf 100644 --- a/tests/clone-populator_test.go +++ b/tests/clone-populator_test.go @@ -18,6 +18,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" cdiv1 "kubevirt.io/containerized-data-importer-api/pkg/apis/core/v1beta1" + "kubevirt.io/containerized-data-importer/pkg/controller/clone" cc "kubevirt.io/containerized-data-importer/pkg/controller/common" "kubevirt.io/containerized-data-importer/tests/framework" "kubevirt.io/containerized-data-importer/tests/utils" @@ -261,6 +262,7 @@ var _ = Describe("Clone Populator tests", func() { sourceHash := getHash(source, 100000) targetHash := getHash(target, 100000) Expect(targetHash).To(Equal(sourceHash)) + f.ExpectCloneFallback(target, clone.IncompatibleVolumeModes, clone.MessageIncompatibleVolumeModes) }) It("should do filesystem to block clone", func() { @@ -276,6 +278,7 @@ var _ = Describe("Clone Populator tests", func() { sourceHash := getHash(source, 100000) targetHash := getHash(target, 100000) Expect(targetHash).To(Equal(sourceHash)) + f.ExpectCloneFallback(target, clone.IncompatibleVolumeModes, clone.MessageIncompatibleVolumeModes) }) DescribeTable("should clone explicit types requested by user", func(cloneType string, canDo func() bool) { @@ -357,6 +360,7 @@ var _ = Describe("Clone Populator tests", func() { By("Check tmp source PVC is deleted") _, err = f.K8sClient.CoreV1().PersistentVolumeClaims(snapshot.Namespace).Get(context.TODO(), tmpSourcePVCforSnapshot, metav1.GetOptions{}) Expect(k8serrors.IsNotFound(err)).To(BeTrue()) + f.ExpectCloneFallback(target, clone.NoVolumeExpansion, clone.MessageNoVolumeExpansion) }) It("should finish the clone after creating the source snapshot", func() { @@ -374,6 +378,7 @@ var _ = Describe("Clone Populator tests", func() { By("Check tmp source PVC is deleted") _, err = f.K8sClient.CoreV1().PersistentVolumeClaims(snapshot.Namespace).Get(context.TODO(), tmpSourcePVCforSnapshot, metav1.GetOptions{}) Expect(k8serrors.IsNotFound(err)).To(BeTrue()) + f.ExpectCloneFallback(target, clone.NoVolumeExpansion, clone.MessageNoVolumeExpansion) }) }) }) diff --git a/tests/cloner_test.go b/tests/cloner_test.go index 0580e6568..f6ee05412 100644 --- a/tests/cloner_test.go +++ b/tests/cloner_test.go @@ -25,6 +25,7 @@ import ( cdiv1 "kubevirt.io/containerized-data-importer-api/pkg/apis/core/v1beta1" "kubevirt.io/containerized-data-importer/pkg/common" + "kubevirt.io/containerized-data-importer/pkg/controller/clone" controller "kubevirt.io/containerized-data-importer/pkg/controller/common" dvc "kubevirt.io/containerized-data-importer/pkg/controller/datavolume" "kubevirt.io/containerized-data-importer/pkg/token" @@ -97,18 +98,15 @@ var _ = Describe("all clone tests", func() { }) It("[test_id:6693]Should clone imported data and retain transfer pods after completion", func() { - smartApplicable := f.IsSnapshotStorageClassAvailable() - sc, err := f.K8sClient.StorageV1().StorageClasses().Get(context.TODO(), f.SnapshotSCName, metav1.GetOptions{}) - if err == nil { - value, ok := sc.Annotations["storageclass.kubernetes.io/is-default-class"] - if smartApplicable && ok && strings.Compare(value, "true") == 0 { - Skip("Cannot test host assisted cloning for within namespace when all pvcs are smart clone capable.") - } + scName := f.GetNoSnapshotStorageClass() + if scName == nil { + Skip("Cannot test host-assisted cloning when all storage classes are smart clone capable") } dataVolume := utils.NewDataVolumeWithHTTPImport(dataVolumeName, "1Gi", fmt.Sprintf(utils.TinyCoreIsoURL, f.CdiInstallNs)) + dataVolume.Spec.PVC.StorageClassName = scName By(fmt.Sprintf("Create new datavolume %s", dataVolume.Name)) - dataVolume, err = utils.CreateDataVolumeFromDefinition(f.CdiClient, f.Namespace.Name, dataVolume) + dataVolume, err := utils.CreateDataVolumeFromDefinition(f.CdiClient, f.Namespace.Name, dataVolume) Expect(err).ToNot(HaveOccurred()) f.ForceBindPvcIfDvIsWaitForFirstConsumer(dataVolume) @@ -116,6 +114,7 @@ var _ = Describe("all clone tests", func() { Expect(err).ToNot(HaveOccurred()) targetDV := utils.NewCloningDataVolume("target-dv", "1Gi", pvc) targetDV.Annotations[controller.AnnPodRetainAfterCompletion] = "true" + targetDV.Spec.PVC.StorageClassName = scName By(fmt.Sprintf("Create new target datavolume %s", targetDV.Name)) targetDataVolume, err := utils.CreateDataVolumeFromDefinition(f.CdiClient, f.Namespace.Name, targetDV) Expect(err).ToNot(HaveOccurred()) @@ -124,6 +123,10 @@ var _ = Describe("all clone tests", func() { By("Wait for target datavolume phase Succeeded") Expect(utils.WaitForDataVolumePhaseWithTimeout(f, targetDataVolume.Namespace, cdiv1.Succeeded, targetDV.Name, cloneCompleteTimeout)).Should(Succeed()) + target, err := f.K8sClient.CoreV1().PersistentVolumeClaims(targetDataVolume.Namespace).Get(context.TODO(), targetDataVolume.Name, metav1.GetOptions{}) + Expect(err).ToNot(HaveOccurred()) + f.ExpectCloneFallback(target, dvc.NoPopulator, dvc.NoPopulatorMessage) + By("Find cloner source pod after completion") cloner, err := utils.FindPodBySuffixOnce(f.K8sClient, targetDataVolume.Namespace, common.ClonerSourcePodNameSuffix, common.CDILabelSelector) Expect(err).ToNot(HaveOccurred()) @@ -2739,6 +2742,8 @@ var _ = Describe("all clone tests", func() { By("Check host assisted clone is taking place") pvc, err := f.K8sClient.CoreV1().PersistentVolumeClaims(targetNs.Name).Get(context.TODO(), dvName, metav1.GetOptions{}) Expect(err).ToNot(HaveOccurred()) + f.ExpectCloneFallback(pvc, clone.NoVolumeExpansion, clone.MessageNoVolumeExpansion) + // non csi if pvc.Spec.DataSourceRef == nil { suffix := "-host-assisted-source-pvc" diff --git a/tests/framework/BUILD.bazel b/tests/framework/BUILD.bazel index 07a79b3ae..603072f90 100644 --- a/tests/framework/BUILD.bazel +++ b/tests/framework/BUILD.bazel @@ -20,6 +20,7 @@ go_library( "//pkg/client/clientset/versioned:go_default_library", "//pkg/common:go_default_library", "//pkg/controller/common:go_default_library", + "//pkg/controller/populators:go_default_library", "//pkg/feature-gates:go_default_library", "//pkg/image:go_default_library", "//pkg/util/naming:go_default_library", diff --git a/tests/framework/framework.go b/tests/framework/framework.go index a06617f33..3df55b444 100644 --- a/tests/framework/framework.go +++ b/tests/framework/framework.go @@ -41,6 +41,8 @@ import ( cdiv1 "kubevirt.io/containerized-data-importer-api/pkg/apis/core/v1beta1" cdiClientset "kubevirt.io/containerized-data-importer/pkg/client/clientset/versioned" "kubevirt.io/containerized-data-importer/pkg/common" + cc "kubevirt.io/containerized-data-importer/pkg/controller/common" + "kubevirt.io/containerized-data-importer/pkg/controller/populators" featuregates "kubevirt.io/containerized-data-importer/pkg/feature-gates" "kubevirt.io/containerized-data-importer/tests/utils" ) @@ -654,6 +656,17 @@ func (f *Framework) ExpectEvent(dataVolumeNamespace string) gomega.AsyncAssertio }, timeout, pollingInterval) } +// ExpectCloneFallback checks PVC annotations and event of clone fallback to host-assisted +func (f *Framework) ExpectCloneFallback(pvc *v1.PersistentVolumeClaim, reason, message string) { + ginkgo.By("Check PVC annotations and event of clone fallback to host-assisted") + gomega.Expect(pvc.Annotations[cc.AnnCloneType]).To(gomega.Equal("copy")) + gomega.Expect(pvc.Annotations[populators.AnnCloneFallbackReason]).To(gomega.Equal(message)) + f.ExpectEvent(pvc.Namespace).Should(gomega.And( + gomega.ContainSubstring(pvc.Name), + gomega.ContainSubstring(reason), + gomega.ContainSubstring(message))) +} + // runKubectlCommand ... func (f *Framework) runKubectlCommand(args ...string) (string, error) { var errb bytes.Buffer @@ -713,6 +726,31 @@ func (f *Framework) IsSnapshotStorageClassAvailable() bool { return false } +// GetNoSnapshotStorageClass gets storage class without snapshot support +func (f *Framework) GetNoSnapshotStorageClass() *string { + scs, err := f.K8sClient.StorageV1().StorageClasses().List(context.TODO(), metav1.ListOptions{}) + if err != nil { + return nil + } + vscs := &snapshotv1.VolumeSnapshotClassList{} + if err = f.CrClient.List(context.TODO(), vscs); err != nil { + return nil + } + for _, sc := range scs.Items { + if sc.Name == "local" || strings.Contains(sc.Provisioner, "no-provisioner") { + continue + } + for _, vsc := range vscs.Items { + if vsc.Driver == sc.Provisioner { + continue + } + } + return &sc.Name + } + + return nil +} + // GetSnapshotClass returns the volume snapshot class. func (f *Framework) GetSnapshotClass() *snapshotv1.VolumeSnapshotClass { // Fetch the storage class