From df39ab2970a55d2648ac14f7059406a45f4063a2 Mon Sep 17 00:00:00 2001 From: Shelly Kagan Date: Sun, 15 May 2022 12:32:12 +0300 Subject: [PATCH 1/4] Fix smart clone request size update In case the pvc got an actual size as the expected size there wasn't an update of the pvc spec with the actual user request size, which left the pvc spec with a smaller size then the user requested(the data size). This caused a discrepancy when trying to restore such pvc, which restored a pvc with the small size instead of the user request size. Signed-off-by: Shelly Kagan --- pkg/controller/datavolume-controller.go | 3 +- pkg/controller/datavolume-controller_test.go | 53 ++++++++++++++++++++ 2 files changed, 55 insertions(+), 1 deletion(-) diff --git a/pkg/controller/datavolume-controller.go b/pkg/controller/datavolume-controller.go index 395da6758e..c98cde906f 100644 --- a/pkg/controller/datavolume-controller.go +++ b/pkg/controller/datavolume-controller.go @@ -1334,10 +1334,11 @@ func (r *DatavolumeReconciler) expand(log logr.Logger, } expansionRequired := actualSize.Cmp(requestedSize) < 0 + updateRequestSizeRequired := actualSize.Cmp(requestedSize) <= 0 && requestedSize.Cmp(currentSize) > 0 log.V(3).Info("Expand sizes", "req", requestedSize, "cur", currentSize, "act", actualSize, "exp", expansionRequired) - if expansionRequired && requestedSize.Cmp(currentSize) != 0 { + if updateRequestSizeRequired { pvc.Spec.Resources.Requests[corev1.ResourceStorage] = requestedSize if err := r.client.Update(context.TODO(), pvc); err != nil { return false, err diff --git a/pkg/controller/datavolume-controller_test.go b/pkg/controller/datavolume-controller_test.go index 0c2f3a86cb..02697c5c83 100644 --- a/pkg/controller/datavolume-controller_test.go +++ b/pkg/controller/datavolume-controller_test.go @@ -1397,6 +1397,59 @@ var _ = Describe("All DataVolume Tests", func() { Entry("snapshot", cdiv1.CloneStrategySnapshot), ) + DescribeTable("After smart clone", func(actualSize resource.Quantity, currentSize resource.Quantity, expectedDvPhase cdiv1.DataVolumePhase) { + strategy := cdiv1.CDICloneStrategy(cdiv1.CloneStrategySnapshot) + controller := true + + dv := newCloneDataVolume("test-dv") + scName := "testsc" + sc := createStorageClassWithProvisioner(scName, map[string]string{ + AnnDefaultStorageClass: "true", + }, map[string]string{}, "csi-plugin") + accessMode := []corev1.PersistentVolumeAccessMode{corev1.ReadOnlyMany} + storageProfile := createStorageProfileWithCloneStrategy(scName, + []cdiv1.ClaimPropertySet{{AccessModes: accessMode, VolumeMode: &blockMode}}, + &strategy) + snapshotClassName := "snap-class" + snapClass := createSnapshotClass(snapshotClassName, nil, "csi-plugin") + + srcPvc := createPvcInStorageClass("test", metav1.NamespaceDefault, &scName, nil, nil, corev1.ClaimBound) + targetPvc := createPvcInStorageClass("test-dv", metav1.NamespaceDefault, &scName, nil, nil, corev1.ClaimBound) + targetPvc.OwnerReferences = append(targetPvc.OwnerReferences, metav1.OwnerReference{ + Kind: "DataVolume", + Controller: &controller, + Name: "test-dv", + UID: dv.UID, + }) + targetPvc.Spec.Resources.Requests[corev1.ResourceStorage] = currentSize + targetPvc.Status.Capacity[corev1.ResourceStorage] = actualSize + targetPvc.SetAnnotations(make(map[string]string)) + targetPvc.GetAnnotations()[AnnCloneOf] = "true" + + reconciler = createDatavolumeReconciler(dv, srcPvc, targetPvc, storageProfile, sc, snapClass, createVolumeSnapshotContentCrd(), createVolumeSnapshotClassCrd(), createVolumeSnapshotCrd()) + + By("Reconcile") + result, err := reconciler.Reconcile(context.TODO(), reconcile.Request{NamespacedName: types.NamespacedName{Name: "test-dv", Namespace: metav1.NamespaceDefault}}) + Expect(err).To(Not(HaveOccurred())) + Expect(result).To(Not(BeNil())) + + By(fmt.Sprintf("Verifying that dv phase is now in %s", expectedDvPhase)) + dv = &cdiv1.DataVolume{} + err = reconciler.client.Get(context.TODO(), types.NamespacedName{Name: "test-dv", Namespace: metav1.NamespaceDefault}, dv) + Expect(err).ToNot(HaveOccurred()) + Expect(dv.Status.Phase).To(Equal(expectedDvPhase)) + + By("Verifying that pvc request size as expected") + 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(resource.MustParse("1G"))) + + }, + Entry("Should expand pvc when actual and current differ then the requested size", resource.MustParse("500M"), resource.MustParse("500M"), cdiv1.ExpansionInProgress), + Entry("Should update request size when current size differ from requested size", resource.MustParse("1G"), resource.MustParse("500M"), cdiv1.ExpansionInProgress), + Entry("Should complete clone in case all sizes match", resource.MustParse("1G"), resource.MustParse("1G"), cdiv1.Succeeded), + ) }) var _ = Describe("CSI clone", func() { From 597866bbbcd8749939feaa81c0c42da761562b1e Mon Sep 17 00:00:00 2001 From: Shelly Kagan Date: Wed, 18 May 2022 13:55:16 +0300 Subject: [PATCH 2/4] review fixes Signed-off-by: Shelly Kagan --- pkg/controller/datavolume-controller.go | 2 +- pkg/controller/datavolume-controller_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/controller/datavolume-controller.go b/pkg/controller/datavolume-controller.go index c98cde906f..c8bdcd79b5 100644 --- a/pkg/controller/datavolume-controller.go +++ b/pkg/controller/datavolume-controller.go @@ -1334,7 +1334,7 @@ func (r *DatavolumeReconciler) expand(log logr.Logger, } expansionRequired := actualSize.Cmp(requestedSize) < 0 - updateRequestSizeRequired := actualSize.Cmp(requestedSize) <= 0 && requestedSize.Cmp(currentSize) > 0 + updateRequestSizeRequired := actualSize.Cmp(requestedSize) <= 0 && currentSize.Cmp(requestedSize) < 0 log.V(3).Info("Expand sizes", "req", requestedSize, "cur", currentSize, "act", actualSize, "exp", expansionRequired) diff --git a/pkg/controller/datavolume-controller_test.go b/pkg/controller/datavolume-controller_test.go index 02697c5c83..6f1263bd2d 100644 --- a/pkg/controller/datavolume-controller_test.go +++ b/pkg/controller/datavolume-controller_test.go @@ -1398,7 +1398,7 @@ var _ = Describe("All DataVolume Tests", func() { ) DescribeTable("After smart clone", func(actualSize resource.Quantity, currentSize resource.Quantity, expectedDvPhase cdiv1.DataVolumePhase) { - strategy := cdiv1.CDICloneStrategy(cdiv1.CloneStrategySnapshot) + strategy := cdiv1.CloneStrategySnapshot controller := true dv := newCloneDataVolume("test-dv") From 95cb489711a7145dce5150549f074f104d800c6a Mon Sep 17 00:00:00 2001 From: Shelly Kagan Date: Mon, 23 May 2022 14:20:14 +0300 Subject: [PATCH 3/4] cherry pick fix Signed-off-by: Shelly Kagan --- pkg/controller/datavolume-controller_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/controller/datavolume-controller_test.go b/pkg/controller/datavolume-controller_test.go index 6f1263bd2d..958f666f9a 100644 --- a/pkg/controller/datavolume-controller_test.go +++ b/pkg/controller/datavolume-controller_test.go @@ -1405,7 +1405,7 @@ var _ = Describe("All DataVolume Tests", func() { scName := "testsc" sc := createStorageClassWithProvisioner(scName, map[string]string{ AnnDefaultStorageClass: "true", - }, map[string]string{}, "csi-plugin") + }, "csi-plugin") accessMode := []corev1.PersistentVolumeAccessMode{corev1.ReadOnlyMany} storageProfile := createStorageProfileWithCloneStrategy(scName, []cdiv1.ClaimPropertySet{{AccessModes: accessMode, VolumeMode: &blockMode}}, From fa8e3d9b859d1eaef7e82e537c8798b0626346e0 Mon Sep 17 00:00:00 2001 From: Shelly Kagan Date: Mon, 23 May 2022 17:06:31 +0300 Subject: [PATCH 4/4] Fix smart clone update request size condition Signed-off-by: Shelly Kagan --- pkg/controller/datavolume-controller.go | 2 +- pkg/controller/datavolume-controller_test.go | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/pkg/controller/datavolume-controller.go b/pkg/controller/datavolume-controller.go index c8bdcd79b5..aafd8d6fbd 100644 --- a/pkg/controller/datavolume-controller.go +++ b/pkg/controller/datavolume-controller.go @@ -1334,7 +1334,7 @@ func (r *DatavolumeReconciler) expand(log logr.Logger, } expansionRequired := actualSize.Cmp(requestedSize) < 0 - updateRequestSizeRequired := actualSize.Cmp(requestedSize) <= 0 && currentSize.Cmp(requestedSize) < 0 + updateRequestSizeRequired := currentSize.Cmp(requestedSize) < 0 log.V(3).Info("Expand sizes", "req", requestedSize, "cur", currentSize, "act", actualSize, "exp", expansionRequired) diff --git a/pkg/controller/datavolume-controller_test.go b/pkg/controller/datavolume-controller_test.go index 958f666f9a..249e2b24b1 100644 --- a/pkg/controller/datavolume-controller_test.go +++ b/pkg/controller/datavolume-controller_test.go @@ -1447,6 +1447,7 @@ var _ = Describe("All DataVolume Tests", func() { }, Entry("Should expand pvc when actual and current differ then the requested size", resource.MustParse("500M"), resource.MustParse("500M"), cdiv1.ExpansionInProgress), + Entry("Should update request size when current size differ then the requested size and actual size is bigger then both", resource.MustParse("2G"), resource.MustParse("500M"), cdiv1.ExpansionInProgress), Entry("Should update request size when current size differ from requested size", resource.MustParse("1G"), resource.MustParse("500M"), cdiv1.ExpansionInProgress), Entry("Should complete clone in case all sizes match", resource.MustParse("1G"), resource.MustParse("1G"), cdiv1.Succeeded), )