rbd: flatten datasource image before creating volume #2900

Rakshith-R · 2022-02-23T11:14:13Z

This commit encures that parent image is flattened before
creating volume.

If the data source is a PVC, the underlying image's parent
is flattened(which would be a temp clone or snapshot).
hard & soft limit is reduced by 2 to account for depth that
will be added by temp & final clone.
If the data source is a Snapshot, the underlying image is
itself flattened.
hard & soft limit is reduced by 1 to account for depth that
will be added by the clone which will be restored from the
snapshot.

Flattening of resulting PVC image restored from snapshot is removed.
Flattening step for temp clone & final image is removed when pvc clone is
being created.

Fixes: #2190

Signed-off-by: Rakshith R [email protected]

Rakshith-R · 2022-02-23T12:25:27Z

/retest all

Madhu-1

@Rakshith-R have you tested this with hardlimit and softlimit of 1?

Madhu-1 · 2022-03-01T05:42:06Z

internal/rbd/controllerserver.go

+		hardLimit := rbdHardMaxCloneDepth
+		softLimit := rbdSoftMaxCloneDepth


declare this outside of if statements as its common for both snapshot and volume

Madhu-1 · 2022-03-01T05:43:28Z

internal/rbd/controllerserver.go

@@ -335,6 +359,29 @@ func flattenParentImage(ctx context.Context, rbdVol *rbdVolume, cr *util.Credent
 			return err
 		}
 	}
+	if rbdSnap != nil {
+		err := rbdSnap.Connect(cr)


where defer Destroy is called for this one?

Madhu-1 · 2022-03-01T05:44:16Z

internal/rbd/controllerserver.go

+	if rbdSnap != nil {
+		err := rbdSnap.Connect(cr)
+		if err != nil {
+			return err


need to return grpc error?

Rakshith-R · 2022-03-01T06:49:34Z

@Rakshith-R have you tested this with hardlimit and softlimit of 1?

It will not affect the flattening at this stage according to this check, pre existing behaviour will continue.

		const depthToAvoidFlatten = 2 #1 for snapshot
		if rbdHardMaxCloneDepth > depthToAvoidFlatten {
			hardLimit = rbdHardMaxCloneDepth - depthToAvoidFlatten
		}
		if rbdSoftMaxCloneDepth > depthToAvoidFlatten {
			softLimit = rbdSoftMaxCloneDepth - depthToAvoidFlatten
		}

Flattening for the temporary clones and final image is being removed in this pr,
Where the lower limit would have flattened intermediate clones(for clone) and final image(for snapshot) before the pr change.
IMO, the above change(2nd) is fine since cephcsi is the one which decides when depth should be checked and flattening operation performed.

Rakshith-R · 2022-03-10T06:12:05Z

@Mergifyio rebase

mergify · 2022-03-10T06:12:28Z

rebase

✅ Branch has been successfully rebased

Madhu-1

changes LGTM can you please set max limit RbdHardMaxCloneDepth to 2 here https://github.com/ceph/ceph-csi/blob/devel/scripts/install-helm.sh#L182 to test no stale are left behind?

Madhu-1 · 2022-03-11T05:47:37Z

internal/rbd/rbd_util.go

+
+// flattenParentImage flatten the given image's parent if it exists according to hard and soft
+// limits.
+func (ri *rbdImage) flattenParentImage(ctx context.Context, hardLimit, softLimit uint) error {


we already have flattenParentImage image in controllerserver. rename this to flattenParent?

Rakshith-R · 2022-03-11T10:44:46Z

changes LGTM can you please set max limit RbdHardMaxCloneDepth to 2 here https://github.com/ceph/ceph-csi/blob/devel/scripts/install-helm.sh#L182 to test no stale are left behind?

@Madhu-1 , Due to #2327 , setting this lower depth will leave stale images.
That's why this option was not set earlier.

Rakshith-R · 2022-03-14T05:32:57Z

/retest all

Rakshith-R · 2022-03-14T05:33:34Z

/retest all

seems jenkins errored out
https://jenkins-ceph-csi.apps.ocp.ci.centos.org/blue/organizations/jenkins/mini-e2e-helm_k8s-1.21/detail/mini-e2e-helm_k8s-1.21/2982/pipeline
https://jenkins-ceph-csi.apps.ocp.ci.centos.org/blue/organizations/jenkins/mini-e2e-helm_k8s-1.22/detail/mini-e2e-helm_k8s-1.22/2264/pipeline

Rakshith-R · 2022-03-15T09:46:19Z

@Mergifyio rebase
ptal @ceph/ceph-csi-contributors

mergify · 2022-03-15T09:47:07Z

rebase

✅ Branch has been successfully rebased

Madhu-1 · 2022-03-15T10:19:22Z

changes LGTM can you please set max limit RbdHardMaxCloneDepth to 2 here https://github.com/ceph/ceph-csi/blob/devel/scripts/install-helm.sh#L182 to test no stale are left behind?

@Madhu-1 , Due to #2327 , setting this lower depth will leave stale images. That's why this option was not set earlier.

Asked to make sure in any case there wont be any stale resources left behind

Madhu-1

LGTM, small nit

internal/rbd/clone.go

Rakshith-R · 2022-03-17T13:32:58Z

@Mergifyio rebase

mergify · 2022-03-17T13:35:07Z

rebase

✅ Branch has been successfully rebased

mergify · 2022-03-17T13:35:09Z

refresh

✅ Pull request refreshed

github-actions · 2022-03-17T16:42:59Z

/retest ci/centos/mini-e2e/k8s-1.21

github-actions · 2022-03-17T16:43:00Z

@Rakshith-R "ci/centos/mini-e2e/k8s-1.21" test failed. Logs are available at location for debugging

github-actions · 2022-03-17T16:43:01Z

@Mergifyio refresh

mergify · 2022-03-17T16:43:58Z

@github-actions[bot] is not allowed to run commands

Rakshith-R · 2022-03-18T04:39:16Z

/retest ci/centos/upgrade-tests-rbd

Rakshith-R · 2022-03-18T04:40:18Z

/retest ci/centos/k8s-e2e-external-storage/1.23

Rakshith-R · 2022-03-18T04:40:35Z

/retest ci/centos/k8s-e2e-external-storage/1.23
/retest ci/centos/upgrade-tests-rbd

success status was not reflected back to github
https://jenkins-ceph-csi.apps.ocp.ci.centos.org/blue/organizations/jenkins/upgrade-tests-rbd/detail/upgrade-tests-rbd/4869/pipeline
https://jenkins-ceph-csi.apps.ocp.ci.centos.org/blue/organizations/jenkins/k8s-e2e-external-storage-1.23/detail/k8s-e2e-external-storage-1.23/879/pipeline

Rakshith-R · 2022-03-18T05:32:57Z

@Mergifyio rebase

This commit ensures that parent image is flattened before creating volume. - If the data source is a PVC, the underlying image's parent is flattened(which would be a temp clone or snapshot). hard & soft limit is reduced by 2 to account for depth that will be added by temp & final clone. - If the data source is a Snapshot, the underlying image is itself flattened. hard & soft limit is reduced by 1 to account for depth that will be added by the clone which will be restored from the snapshot. Flattening step for resulting PVC image restored from snapshot is removed. Flattening step for temp clone & final image is removed when pvc clone is being created. Fixes: ceph#2190 Signed-off-by: Rakshith R <[email protected]>

mergify · 2022-03-18T05:33:37Z

rebase

✅ Branch has been successfully rebased

ceph-csi-bot · 2022-03-18T06:08:27Z

/retest ci/centos/k8s-e2e-external-storage/1.22

ceph-csi-bot · 2022-03-18T06:08:28Z

@Rakshith-R "ci/centos/k8s-e2e-external-storage/1.22" test failed. Logs are available at location for debugging

ceph-csi-bot · 2022-03-18T06:08:28Z

@Mergifyio refresh

mergify · 2022-03-18T06:08:55Z

refresh

✅ Pull request refreshed

ceph-csi-bot · 2022-03-18T08:35:30Z

/retest ci/centos/mini-e2e/k8s-1.22

ceph-csi-bot · 2022-03-18T08:35:30Z

@Rakshith-R "ci/centos/mini-e2e/k8s-1.22" test failed. Logs are available at location for debugging

ceph-csi-bot · 2022-03-18T08:35:31Z

@Mergifyio refresh

mergify · 2022-03-18T08:36:02Z

refresh

✅ Pull request refreshed

CephCSI should not flatten image that can be mounted for use by the user. `checkFlatten()` was called in a recovery code flow of PVC restored from snapshot and was missed while refractoring in ceph#2900 refer: ceph#2900 Signed-off-by: Rakshith R <[email protected]>

CephCSI should not flatten image that can be mounted for use by the user. `checkFlatten()` was called in a recovery code flow of PVC restored from snapshot and was missed while refractoring in #2900 refer: #2900 Signed-off-by: Rakshith R <[email protected]>

mergify bot added the component/rbd Issues related to RBD label Feb 23, 2022

Rakshith-R force-pushed the fix-parent-flatten branch 4 times, most recently from d3c7ee0 to 5a01cfe Compare February 28, 2022 05:29

Rakshith-R marked this pull request as ready for review February 28, 2022 05:30

Rakshith-R requested review from Madhu-1, nixpanic and a team February 28, 2022 05:30

Madhu-1 reviewed Mar 1, 2022

View reviewed changes

Rakshith-R force-pushed the fix-parent-flatten branch from 5a01cfe to 265e59a Compare March 1, 2022 06:36

Rakshith-R requested a review from Madhu-1 March 1, 2022 06:49

Rakshith-R force-pushed the fix-parent-flatten branch from 265e59a to 13e99e4 Compare March 10, 2022 06:12

Madhu-1 reviewed Mar 11, 2022

View reviewed changes

Rakshith-R force-pushed the fix-parent-flatten branch from 13e99e4 to 145951f Compare March 11, 2022 10:44

Rakshith-R requested a review from Madhu-1 March 11, 2022 10:44

Rakshith-R requested review from a team and removed request for nixpanic March 14, 2022 12:01

ceph-csi-bot force-pushed the fix-parent-flatten branch from 145951f to 1668c68 Compare March 15, 2022 09:47

Madhu-1 reviewed Mar 15, 2022

View reviewed changes

internal/rbd/clone.go Outdated Show resolved Hide resolved

Rakshith-R force-pushed the fix-parent-flatten branch from 75fe110 to 1f1663a Compare March 17, 2022 13:35

Rakshith-R mentioned this pull request Mar 18, 2022

ci: use CEPH_CSI_BOT token for retest action #2951

Merged

Rakshith-R force-pushed the fix-parent-flatten branch from 1f1663a to 0000b26 Compare March 18, 2022 05:33

mergify bot merged commit a56f9a0 into ceph:devel Mar 18, 2022

Rakshith-R mentioned this pull request Nov 14, 2024

Fixes related to flatten operation in PVC restored from snapshot #4960

Merged

6 tasks

		hardLimit := rbdHardMaxCloneDepth
		softLimit := rbdSoftMaxCloneDepth

rbd: flatten datasource image before creating volume #2900

rbd: flatten datasource image before creating volume #2900

Conversation

Rakshith-R commented Feb 23, 2022 • edited Loading

Rakshith-R commented Feb 23, 2022

Madhu-1 left a comment

Choose a reason for hiding this comment

Madhu-1 Mar 1, 2022

Choose a reason for hiding this comment

Madhu-1 Mar 1, 2022

Choose a reason for hiding this comment

Madhu-1 Mar 1, 2022

Choose a reason for hiding this comment

Rakshith-R commented Mar 1, 2022

Rakshith-R commented Mar 10, 2022

mergify bot commented Mar 10, 2022

✅ Branch has been successfully rebased

Madhu-1 left a comment

Choose a reason for hiding this comment

Madhu-1 Mar 11, 2022

Choose a reason for hiding this comment

Rakshith-R commented Mar 11, 2022

Rakshith-R commented Mar 14, 2022

Rakshith-R commented Mar 14, 2022

Rakshith-R commented Mar 15, 2022

mergify bot commented Mar 15, 2022

✅ Branch has been successfully rebased

Madhu-1 commented Mar 15, 2022

Madhu-1 left a comment

Choose a reason for hiding this comment

Rakshith-R commented Mar 17, 2022

mergify bot commented Mar 17, 2022

✅ Branch has been successfully rebased

mergify bot commented Mar 17, 2022

✅ Pull request refreshed

github-actions bot commented Mar 17, 2022

github-actions bot commented Mar 17, 2022

github-actions bot commented Mar 17, 2022

mergify bot commented Mar 17, 2022

Rakshith-R commented Mar 18, 2022

Rakshith-R commented Mar 18, 2022

Rakshith-R commented Mar 18, 2022

Rakshith-R commented Mar 18, 2022

mergify bot commented Mar 18, 2022

✅ Branch has been successfully rebased

ceph-csi-bot commented Mar 18, 2022

ceph-csi-bot commented Mar 18, 2022

ceph-csi-bot commented Mar 18, 2022

mergify bot commented Mar 18, 2022

✅ Pull request refreshed

ceph-csi-bot commented Mar 18, 2022

ceph-csi-bot commented Mar 18, 2022

ceph-csi-bot commented Mar 18, 2022

mergify bot commented Mar 18, 2022

✅ Pull request refreshed

Rakshith-R commented Feb 23, 2022 •

edited

Loading