Skip to content

Commit 1c9e46b

Browse files
arnongilboaarnongilboa
committed
Support storageProfile minimumSupportedPVCSize in clone
When the target DataVolume storage requests a size smaller than the source PVC. For target without size it already worked correctly. Signed-off-by: Arnon Gilboa <[email protected]>
1 parent 889dd4f commit 1c9e46b

File tree

8 files changed

+132
-20
lines changed

8 files changed

+132
-20
lines changed

‎pkg/controller/clone/planner.go‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -461,7 +461,13 @@ func (p *Planner) validateSourcePVC(args *ChooseStrategyArgs, sourceClaim *corev
461461
return nil
462462
}
463463

464-
if err := cc.ValidateRequestedCloneSize(sourceClaim.Spec.Resources, args.TargetClaim.Spec.Resources); err != nil {
464+
targetResources, err := cc.GetEffectiveStorageResources(context.TODO(), p.Client, args.TargetClaim.Spec.Resources,
465+
args.TargetClaim.Spec.StorageClassName, cc.GetPVCContentType(args.TargetClaim), args.Log)
466+
if err != nil {
467+
return err
468+
}
469+
470+
if err := cc.ValidateRequestedCloneSize(sourceClaim.Spec.Resources, *targetResources); err != nil {
465471
p.Recorder.Eventf(args.TargetClaim, corev1.EventTypeWarning, cc.ErrIncompatiblePVC, err.Error())
466472
return err
467473
}

‎pkg/controller/common/util.go‎

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1098,6 +1098,48 @@ func AddImportVolumeMounts() []corev1.VolumeMount {
10981098
return volumeMounts
10991099
}
11001100

1101+
// GetEffectiveStorageResources returns the maximum of the passed storageResources and the storageProfile minimumSupportedPVCSize.
1102+
// If the passed storageResources has no size, it is returned as-is.
1103+
func GetEffectiveStorageResources(ctx context.Context, client client.Client, storageResources corev1.VolumeResourceRequirements,
1104+
storageClassName *string, contentType cdiv1.DataVolumeContentType, log logr.Logger) (*corev1.VolumeResourceRequirements, error) {
1105+
sc, err := GetStorageClassByNameWithVirtFallback(ctx, client, storageClassName, contentType)
1106+
if err != nil {
1107+
return nil, err
1108+
}
1109+
if sc == nil {
1110+
return &storageResources, nil
1111+
}
1112+
1113+
storageProfile := &cdiv1.StorageProfile{}
1114+
if err := client.Get(ctx, types.NamespacedName{Name: sc.Name}, storageProfile); err != nil {
1115+
if k8serrors.IsNotFound(err) {
1116+
return &storageResources, nil
1117+
}
1118+
return nil, err
1119+
}
1120+
1121+
requestedSize, hasSize := storageResources.Requests[corev1.ResourceStorage]
1122+
if !hasSize {
1123+
return &storageResources, nil
1124+
}
1125+
1126+
if val, exists := storageProfile.Annotations[AnnMinimumSupportedPVCSize]; exists {
1127+
if minSize, err := resource.ParseQuantity(val); err == nil {
1128+
if requestedSize.Cmp(minSize) == -1 {
1129+
return &corev1.VolumeResourceRequirements{
1130+
Requests: corev1.ResourceList{
1131+
corev1.ResourceStorage: requestedSize,
1132+
},
1133+
}, nil
1134+
}
1135+
} else {
1136+
log.V(1).Info("Invalid minimum PVC size in annotation", "value", val, "error", err)
1137+
}
1138+
}
1139+
1140+
return &storageResources, nil
1141+
}
1142+
11011143
// ValidateRequestedCloneSize validates the clone size requirements on block
11021144
func ValidateRequestedCloneSize(sourceResources, targetResources corev1.VolumeResourceRequirements) error {
11031145
sourceRequest, hasSource := sourceResources.Requests[corev1.ResourceStorage]

‎pkg/controller/datavolume/pvc-clone-controller.go‎

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -445,7 +445,7 @@ func (r *PvcCloneReconciler) validateCloneAndSourcePVC(syncState *dvSyncState, l
445445
return false, err
446446
}
447447

448-
err = validateClone(sourcePvc, &datavolume.Spec)
448+
err = r.validateClone(sourcePvc, &datavolume.Spec)
449449
if err != nil {
450450
syncErr := r.syncDataVolumeStatusPhaseWithEvent(syncState, datavolume.Status.Phase, nil,
451451
Event{
@@ -463,7 +463,7 @@ func (r *PvcCloneReconciler) validateCloneAndSourcePVC(syncState *dvSyncState, l
463463
}
464464

465465
// validateClone compares a clone spec against its source PVC to validate its creation
466-
func validateClone(sourcePVC *corev1.PersistentVolumeClaim, spec *cdiv1.DataVolumeSpec) error {
466+
func (r *PvcCloneReconciler) validateClone(sourcePVC *corev1.PersistentVolumeClaim, spec *cdiv1.DataVolumeSpec) error {
467467
var targetResources corev1.VolumeResourceRequirements
468468

469469
valid, sourceContentType, targetContentType := validateContentTypes(sourcePVC, spec)
@@ -477,12 +477,17 @@ func validateClone(sourcePVC *corev1.PersistentVolumeClaim, spec *cdiv1.DataVolu
477477
if explicitPvcRequest {
478478
targetResources = spec.PVC.Resources
479479
} else {
480-
targetResources = spec.Storage.Resources
481480
// The storage size in the target DV can be empty
482481
// when cloning using the 'Storage' API
483-
if _, ok := targetResources.Requests[corev1.ResourceStorage]; !ok {
482+
if _, ok := spec.Storage.Resources.Requests[corev1.ResourceStorage]; !ok {
484483
isSizelessClone = true
485484
}
485+
486+
res, err := cc.GetEffectiveStorageResources(context.TODO(), r.client, spec.Storage.Resources, spec.Storage.StorageClassName, spec.ContentType, r.log)
487+
if err != nil {
488+
return err
489+
}
490+
targetResources = *res
486491
}
487492

488493
// TODO: Spec.Storage API needs a better more complex check to validate clone size - to account for fsOverhead

‎pkg/controller/datavolume/pvc-clone-controller_test.go‎

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -493,12 +493,13 @@ var _ = Describe("All DataVolume Tests", func() {
493493
sourcePvc := CreatePvc("testPVC", "default", map[string]string{}, nil)
494494
blockVM := corev1.PersistentVolumeBlock
495495
fsVM := corev1.PersistentVolumeFilesystem
496+
r := createCloneReconciler()
496497

497498
It("Should reject the clone if source and target have different content types", func() {
498499
sourcePvc.Annotations[AnnContentType] = string(cdiv1.DataVolumeKubeVirt)
499500
dvSpec := &cdiv1.DataVolumeSpec{ContentType: cdiv1.DataVolumeArchive}
500501

501-
err := validateClone(sourcePvc, dvSpec)
502+
err := r.validateClone(sourcePvc, dvSpec)
502503
Expect(err).To(HaveOccurred())
503504
Expect(err.Error()).To(ContainSubstring(
504505
fmt.Sprintf("Source contentType (%s) and target contentType (%s) do not match", cdiv1.DataVolumeKubeVirt, cdiv1.DataVolumeArchive)))
@@ -516,7 +517,7 @@ var _ = Describe("All DataVolume Tests", func() {
516517
}
517518
dvSpec := &cdiv1.DataVolumeSpec{Storage: storageSpec}
518519

519-
err := validateClone(sourcePvc, dvSpec)
520+
err := r.validateClone(sourcePvc, dvSpec)
520521
Expect(err).To(HaveOccurred())
521522
Expect(err.Error()).To(ContainSubstring("target resources requests storage size is smaller than the source"))
522523
})
@@ -533,7 +534,7 @@ var _ = Describe("All DataVolume Tests", func() {
533534
}
534535
dvSpec := &cdiv1.DataVolumeSpec{Storage: storageSpec}
535536

536-
err := validateClone(sourcePvc, dvSpec)
537+
err := r.validateClone(sourcePvc, dvSpec)
537538
Expect(err).ToNot(HaveOccurred())
538539
})
539540

@@ -543,7 +544,7 @@ var _ = Describe("All DataVolume Tests", func() {
543544
storageSpec := &cdiv1.StorageSpec{}
544545
dvSpec := &cdiv1.DataVolumeSpec{Storage: storageSpec}
545546

546-
err := validateClone(sourcePvc, dvSpec)
547+
err := r.validateClone(sourcePvc, dvSpec)
547548
Expect(err).ToNot(HaveOccurred())
548549
})
549550

@@ -558,7 +559,7 @@ var _ = Describe("All DataVolume Tests", func() {
558559
}
559560
dvSpec := &cdiv1.DataVolumeSpec{PVC: pvcSpec}
560561

561-
err := validateClone(sourcePvc, dvSpec)
562+
err := r.validateClone(sourcePvc, dvSpec)
562563
Expect(err).To(HaveOccurred())
563564
Expect(err.Error()).To(ContainSubstring("target resources requests storage size is smaller than the source"))
564565

@@ -575,7 +576,7 @@ var _ = Describe("All DataVolume Tests", func() {
575576
}
576577
dvSpec := &cdiv1.DataVolumeSpec{PVC: pvcSpec}
577578

578-
err := validateClone(sourcePvc, dvSpec)
579+
err := r.validateClone(sourcePvc, dvSpec)
579580
Expect(err).ToNot(HaveOccurred())
580581
})
581582
})

‎pkg/controller/datavolume/snapshot-clone-controller.go‎

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -340,7 +340,7 @@ func (r *SnapshotCloneReconciler) validateCloneAndSourceSnapshot(syncState *dvSy
340340
}
341341
syncState.snapshot = snapshot
342342

343-
err = validateSnapshotClone(snapshot, &datavolume.Spec)
343+
err = r.validateSnapshotClone(snapshot, &datavolume.Spec)
344344
if err != nil {
345345
syncEventErr := r.syncDataVolumeStatusPhaseWithEvent(syncState, datavolume.Status.Phase, nil,
346346
Event{
@@ -358,7 +358,7 @@ func (r *SnapshotCloneReconciler) validateCloneAndSourceSnapshot(syncState *dvSy
358358
}
359359

360360
// validateSnapshotClone compares a snapshot clone spec against its source snapshot to validate its creation
361-
func validateSnapshotClone(sourceSnapshot *snapshotv1.VolumeSnapshot, spec *cdiv1.DataVolumeSpec) error {
361+
func (r *SnapshotCloneReconciler) validateSnapshotClone(sourceSnapshot *snapshotv1.VolumeSnapshot, spec *cdiv1.DataVolumeSpec) error {
362362
err := cc.IsSnapshotValidForClone(sourceSnapshot)
363363
if err != nil {
364364
return err
@@ -380,10 +380,14 @@ func validateSnapshotClone(sourceSnapshot *snapshotv1.VolumeSnapshot, spec *cdiv
380380
if explicitPvcRequest {
381381
targetResources = spec.PVC.Resources
382382
} else {
383-
targetResources = spec.Storage.Resources
384-
if _, ok := targetResources.Requests["storage"]; !ok {
383+
if _, ok := spec.Storage.Resources.Requests[corev1.ResourceStorage]; !ok {
385384
isSizelessClone = true
386385
}
386+
res, err := cc.GetEffectiveStorageResources(context.TODO(), r.client, spec.Storage.Resources, spec.Storage.StorageClassName, spec.ContentType, r.log)
387+
if err != nil {
388+
return err
389+
}
390+
targetResources = *res
387391
}
388392

389393
if !isSizelessClone && restoreSizeAvailable {

‎pkg/controller/datavolume/snapshot-clone-controller_test.go‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -308,6 +308,7 @@ var _ = Describe("All DataVolume Tests", func() {
308308

309309
var _ = Describe("validateSnapshotClone", func() {
310310
type snapshotModifierFunc func(*snapshotv1.VolumeSnapshot)
311+
r := createSnapshotCloneReconciler()
311312

312313
DescribeTable("Validation mechanism rejects or accepts the clone depending snapshot state",
313314
func(expectedErr string, snapshotModifier snapshotModifierFunc) {
@@ -316,7 +317,7 @@ var _ = Describe("All DataVolume Tests", func() {
316317
snapshot := createSnapshotInVolumeSnapshotClass("test-snap", dv.Namespace, &expectedSnapshotClass, nil, nil, true)
317318
snapshotModifier(snapshot)
318319

319-
err := validateSnapshotClone(snapshot, &dv.Spec)
320+
err := r.validateSnapshotClone(snapshot, &dv.Spec)
320321
if expectedErr != "" {
321322
Expect(err).To(HaveOccurred())
322323
Expect(err.Error()).To(ContainSubstring(expectedErr))
@@ -363,7 +364,7 @@ var _ = Describe("All DataVolume Tests", func() {
363364
restoreSizeParsed := resource.MustParse(restoreSize)
364365
snapshot.Status.RestoreSize = &restoreSizeParsed
365366

366-
err := validateSnapshotClone(snapshot, &dv.Spec)
367+
err := r.validateSnapshotClone(snapshot, &dv.Spec)
367368
if expectedErr != "" {
368369
Expect(err).To(HaveOccurred())
369370
Expect(err.Error()).To(ContainSubstring(expectedErr))

‎tests/cloner_test.go‎

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2215,6 +2215,41 @@ var _ = Describe("all clone tests", func() {
22152215

22162216
})
22172217

2218+
DescribeTable("Clone target datavolume smaller than the source, using storgeProfile with minPvcSize annotation", func(minSize string, shouldSucceed bool) {
2219+
By(fmt.Sprintf("Create source PVC %s", sourcePVCName))
2220+
ns := f.Namespace.Name
2221+
sc := createStorageWithMinimumSupportedPVCSize(f, minSize)
2222+
pvcDef := utils.NewPVCDefinition(sourcePVCName, "1Gi", nil, nil)
2223+
pvcDef.Namespace = ns
2224+
pvcDef.Spec.StorageClassName = &sc
2225+
sourcePvc = f.CreateAndPopulateSourcePVC(pvcDef, sourcePodFillerName, fillCommand+testFile+"; chmod 660 "+testBaseDir+testFile)
2226+
2227+
targetDvName := "small-target-dv"
2228+
By(fmt.Sprintf("Create small target DV %s", targetDvName))
2229+
2230+
targetDV := utils.NewCloningDataVolume(targetDvName, "512Mi", pvcDef)
2231+
targetDV, err := utils.CreateDataVolumeFromDefinition(f.CdiClient, ns, targetDV)
2232+
Expect(err).ToNot(HaveOccurred())
2233+
2234+
if !shouldSucceed {
2235+
By("The clone should fail")
2236+
f.ExpectEvent(ns).Should(ContainSubstring(dvc.CloneValidationFailed))
2237+
return
2238+
}
2239+
2240+
targetPvc, err = utils.WaitForPVC(f.K8sClient, targetDV.Namespace, targetDV.Name)
2241+
Expect(err).ToNot(HaveOccurred())
2242+
f.ForceBindIfWaitForFirstConsumer(targetPvc)
2243+
2244+
By("Wait for target DV succeeded")
2245+
err = utils.WaitForDataVolumePhase(f, ns, cdiv1.Succeeded, targetPvc.Name)
2246+
Expect(err).ToNot(HaveOccurred())
2247+
},
2248+
Entry("[test_id:XXXX] large enough", "1Gi", true),
2249+
Entry("[test_id:XXXX] too small", "256Mi", false),
2250+
Entry("[test_id:XXXX] empty", "", false),
2251+
)
2252+
22182253
It("[test_id:4276] Clone datavolume with short name", Serial, func() {
22192254
shortDvName := "import-long-name-dv"
22202255

@@ -3058,3 +3093,21 @@ func validateCloneType(f *framework.Framework, dv *cdiv1.DataVolume) {
30583093

30593094
Expect(utils.GetCloneType(f.CdiClient, dv)).To(Equal(cloneType))
30603095
}
3096+
3097+
func createStorageWithMinimumSupportedPVCSize(f *framework.Framework, minSize string) string {
3098+
sc, err := f.CreateNonDefaultVariationOfStorageClass(utils.DefaultStorageClass,
3099+
func(sc *storagev1.StorageClass) { sc.UID = "" })
3100+
Expect(err).ToNot(HaveOccurred())
3101+
3102+
var sp *cdiv1.StorageProfile
3103+
Eventually(func() error {
3104+
sp, err = f.CdiClient.CdiV1beta1().StorageProfiles().Get(context.TODO(), sc.Name, metav1.GetOptions{})
3105+
return err
3106+
}, time.Minute, time.Second).Should(Succeed())
3107+
3108+
sp.Annotations = map[string]string{controller.AnnMinimumSupportedPVCSize: minSize}
3109+
_, err = f.CdiClient.CdiV1beta1().StorageProfiles().Update(context.TODO(), sp, metav1.UpdateOptions{})
3110+
Expect(err).ToNot(HaveOccurred())
3111+
3112+
return sc.Name
3113+
}

‎tests/utils/datavolume.go‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -600,7 +600,7 @@ func NewDataVolumeForImageCloning(dataVolumeName, size, namespace, pvcName strin
600600
Name: pvcName,
601601
},
602602
},
603-
PVC: &k8sv1.PersistentVolumeClaimSpec{
603+
Storage: &cdiv1.StorageSpec{
604604
AccessModes: []k8sv1.PersistentVolumeAccessMode{k8sv1.ReadWriteOnce},
605605
Resources: k8sv1.VolumeResourceRequirements{
606606
Requests: k8sv1.ResourceList{
@@ -611,10 +611,10 @@ func NewDataVolumeForImageCloning(dataVolumeName, size, namespace, pvcName strin
611611
},
612612
}
613613
if volumeMode != nil {
614-
dv.Spec.PVC.VolumeMode = volumeMode
614+
dv.Spec.Storage.VolumeMode = volumeMode
615615
}
616616
if storageClassName != nil {
617-
dv.Spec.PVC.StorageClassName = storageClassName
617+
dv.Spec.Storage.StorageClassName = storageClassName
618618
}
619619
return dv
620620
}

0 commit comments

Comments
 (0)