cephfs: use shallow volumes for the ROX accessMode #3651

riya-singhal31 · 2023-02-07T09:14:14Z

this commit makes shallow volume as default feature for ROX volumes.

Signed-off-by: riya-singhal31 rsinghal@redhat.com

Madhu-1 · 2023-02-07T12:08:23Z

internal/cephfs/store/volumeoptions.go

@@ -235,6 +235,7 @@ func NewVolumeOptions(
 	opts.Monitors = strings.Join(clusterData.Monitors, ",")
 	opts.SubvolumeGroup = clusterData.CephFS.SubvolumeGroup
 	opts.Owner = k8s.GetOwner(volOptions)
+	opts.BackingSnapshot = true


This will impact the volume creation for other access modes, the Request is to support backingSnapshot for only ROX modes. For other access modes, we should create a subvolume.

nixpanic · 2023-02-08T14:04:39Z

internal/cephfs/store/volumeoptions.go

@@ -236,6 +236,16 @@ func NewVolumeOptions(
 	opts.SubvolumeGroup = clusterData.CephFS.SubvolumeGroup
 	opts.Owner = k8s.GetOwner(volOptions)

+	// backingSnapshot will be used only with read-only access modes
+	volCaps := req.GetVolumeCapabilities()


Could you make this a helper function so that testing is easier?

// default BackingSnapshot to true only for ReadOnly volume requests opt.BackingSnapshot = isVolumeCreateRO(req) ... } func isVolumeCreateRO(req *csi.CreateVolumeRequest) bool { ... }

That way, you can add a unit test for isVolumeCreateRO(), document it's behaviour and reduce the complexity of NewVolumeOptions().

nixpanic · 2023-02-09T10:23:34Z

internal/cephfs/store/volumeoptions.go

@@ -323,6 +325,19 @@ func NewVolumeOptions(
 	return &opts, nil
 }

+func isVolumeCreateRO(req *csi.CreateVolumeRequest) bool {


can you add a unit-tests in internal/cephfs/store/volumeoptions_test.go for this function?

Yes, sure. I'll update the PR.

nixpanic · 2023-02-13T10:08:04Z

internal/cephfs/store/volumeoptions_test.go

@@ -0,0 +1,82 @@
+package store


please include the header as in other files in this directory

Thanks, updated.

Rakshith-R · 2023-02-14T10:43:01Z

internal/cephfs/store/volumeoptions.go

+	// default BackingSnapshot to true only for ReadOnly volume requests
+	opts.BackingSnapshot = IsVolumeCreateRO(req.VolumeCapabilities)


The datasource needs to be a snapshot for this to work.

Here we are just specifying the value for backing snapshot based on the access mode of volume.
The data source should be handled by user, while creating the yaml of volume. right?

Here we are just specifying the value for backing snapshot based on the access mode of volume. The data source should be handled by user, while creating the yaml of volume. right?

This will cause a error when data source is pvc.

We should default backingSnapshot option to true if the datasource is a snapshot and request is for a RO volume.

Thanks Rakshith, updated.

riya-singhal31 · 2023-02-15T08:04:23Z

/test ci/centos/mini-e2e-helm/k8s-1.26

Rakshith-R · 2023-02-15T09:41:07Z

internal/cephfs/store/volumeoptions.go

+	return false
+}
+
+func IsDataSourceSnapshot(volumeSource *csi.VolumeContentSource) bool {


VolumeSource can itself be nil too,
for a bare createPVC, this can lead to a panic here

Rakshith-R · 2023-02-20T06:21:02Z

e2e/cephfs.go


 				// Deleting snapshot before deleting pvcClone should succeed. It will be
 				// deleted once all volumes that are backed by this snapshot are gone.


Suggested change

// Deleting snapshot before deleting pvcClone should succeed. It will be

// deleted once all volumes that are backed by this snapshot are gone.

check snapshot count right after deletesnapshoy

riya-singhal31 · 2023-02-20T08:07:47Z

/test ci/centos/mini-e2e-helm/k8s-1.26

Rakshith-R

Thanks !

Rakshith-R · 2023-02-20T09:54:18Z

/test ci/centos/mini-e2e-helm/k8s-1.25

yati1998

@riya-singhal31 , just a small nit. else looks good to me.

yati1998 · 2023-02-20T10:14:22Z

e2e/cephfs.go

@@ -1617,8 +1617,8 @@ var _ = Describe(cephfsType, func() {
 						if err != nil {
 							framework.Failf("failed to delete CephFS storageclass: %v", err)
 						}
+


Is this extra line required? IMO, you can remove this.

Rakshith-R · 2023-02-20T11:34:16Z

@Mergifyio rebase

mergify · 2023-02-20T11:34:42Z

⚠️ This pull request got rebased on behalf of a random user of the organization.
This behavior will change on the 1st February 2023, Mergify will pick the author of the pull request instead.

To get the future behavior now, you can configure bot_account options (e.g.: bot_account: { author } or update_bot_account: { author }.

Or you can create a dedicated github account for squash and rebase operations, and use it in different bot_account options.

mergify · 2023-02-20T11:34:43Z

rebase

✅ Branch has been successfully rebased

github-actions · 2023-02-20T12:12:38Z

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

github-actions · 2023-02-21T17:44:23Z

/test ci/centos/k8s-e2e-external-storage/1.26

github-actions · 2023-02-21T17:44:24Z

/test ci/centos/mini-e2e-helm/k8s-1.23

github-actions · 2023-02-21T17:44:25Z

/test ci/centos/mini-e2e-helm/k8s-1.24

github-actions · 2023-02-21T17:44:26Z

/test ci/centos/mini-e2e-helm/k8s-1.25

github-actions · 2023-02-21T17:44:27Z

/test ci/centos/mini-e2e-helm/k8s-1.26

github-actions · 2023-02-21T17:44:29Z

/test ci/centos/mini-e2e/k8s-1.23

github-actions · 2023-02-21T17:44:30Z

/test ci/centos/mini-e2e/k8s-1.24

github-actions · 2023-02-21T17:44:31Z

/test ci/centos/mini-e2e/k8s-1.25

github-actions · 2023-02-21T17:44:33Z

/test ci/centos/mini-e2e/k8s-1.26

github-actions · 2023-02-21T17:44:34Z

/test ci/centos/upgrade-tests-cephfs

github-actions · 2023-02-21T17:44:35Z

/test ci/centos/upgrade-tests-rbd

github-actions · 2023-02-21T20:09:42Z

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

github-actions · 2023-02-21T20:09:43Z

/test ci/centos/k8s-e2e-external-storage/1.24

github-actions · 2023-02-21T20:09:44Z

/test ci/centos/k8s-e2e-external-storage/1.25

github-actions · 2023-02-21T20:09:45Z

/test ci/centos/k8s-e2e-external-storage/1.26

github-actions · 2023-02-21T20:09:47Z

/test ci/centos/mini-e2e-helm/k8s-1.23

github-actions · 2023-02-21T20:09:48Z

/test ci/centos/mini-e2e-helm/k8s-1.24

github-actions · 2023-02-21T20:09:49Z

/test ci/centos/mini-e2e-helm/k8s-1.25

github-actions · 2023-02-21T20:09:51Z

/test ci/centos/mini-e2e-helm/k8s-1.26

github-actions · 2023-02-21T20:09:54Z

/test ci/centos/mini-e2e/k8s-1.23

github-actions · 2023-02-21T20:09:56Z

/test ci/centos/mini-e2e/k8s-1.24

github-actions · 2023-02-21T20:09:57Z

/test ci/centos/mini-e2e/k8s-1.25

github-actions · 2023-02-21T20:09:58Z

/test ci/centos/mini-e2e/k8s-1.26

github-actions · 2023-02-21T20:10:00Z

/test ci/centos/upgrade-tests-cephfs

github-actions · 2023-02-21T20:10:01Z

/test ci/centos/upgrade-tests-rbd

haslersn · 2023-09-14T22:21:26Z

docs/deploy-cephfs.md

@@ -70,7 +70,7 @@ you're running it inside a k8s cluster and find the config itself).
 | `pool`                                                                                              | no             | Ceph pool into which volume data shall be stored                                                                                                                                                                        |
 | `volumeNamePrefix`                                                                                  | no             | Prefix to use for naming subvolumes (defaults to `csi-vol-`).                                                                                                                                                           |
 | `snapshotNamePrefix`                                                                                | no             | Prefix to use for naming snapshots (defaults to `csi-snap-`)                                                                                                                                                            |
-| `backingSnapshot`                                                                                   | no             | Boolean value. The PVC shall be backed by the CephFS snapshot specified in its data source. `pool` parameter must not be specified. (defaults to `false`)                                                               |
+| `backingSnapshot`                                                                                   | no             | Boolean value. The PVC shall be backed by the CephFS snapshot specified in its data source. `pool` parameter must not be specified. (defaults to `true`)                                                               |


@riya-singhal31 Here you say the default is true. However, I looked a bit at the code and it seems there are 3 options:

backingSnapshot is true, then the volume is backed by a snapshot or, if that's not possible, an error is returned.

backingSnapshot is false, then the volume is never backed by a snapshot.

backingSnapshot is not set, then the volume is backed by a snapshot if supported (i.e., IsShallowVolumeSupported(req) returns true.

Is my interpretation of the source code correct? If yes, then you cannot say that the default is true, because the default behavior deviates from the behavior in the true/false cases.

riya-singhal31 changed the title ~~cephfs: use shallow volumes for the ROX accessMode~~ [WIP]cephfs: use shallow volumes for the ROX accessMode Feb 7, 2023

mergify bot added the component/cephfs Issues related to CephFS label Feb 7, 2023

Madhu-1 requested changes Feb 7, 2023

View reviewed changes

nixpanic reviewed Feb 8, 2023

View reviewed changes

riya-singhal31 requested a review from nixpanic February 9, 2023 08:45

nixpanic reviewed Feb 9, 2023

View reviewed changes

nixpanic reviewed Feb 13, 2023

View reviewed changes

riya-singhal31 requested a review from nixpanic February 14, 2023 06:59

Rakshith-R requested changes Feb 14, 2023

View reviewed changes

riya-singhal31 requested review from Rakshith-R and nixpanic and removed request for nixpanic and Rakshith-R February 15, 2023 09:37

Rakshith-R reviewed Feb 15, 2023

View reviewed changes

Rakshith-R reviewed Feb 20, 2023

View reviewed changes

Rakshith-R previously approved these changes Feb 20, 2023

View reviewed changes

Rakshith-R requested a review from a team February 20, 2023 09:54

riya-singhal31 marked this pull request as ready for review February 20, 2023 10:06

yati1998 requested changes Feb 20, 2023

View reviewed changes

riya-singhal31 changed the title ~~[WIP]cephfs: use shallow volumes for the ROX accessMode~~ cephfs: use shallow volumes for the ROX accessMode Feb 20, 2023

Madhu-1 previously approved these changes Feb 20, 2023

View reviewed changes

Rakshith-R requested a review from yati1998 February 20, 2023 11:33

Rakshith-R added the ok-to-test Label to trigger E2E tests label Feb 20, 2023

github-actions bot removed the ok-to-test Label to trigger E2E tests label Feb 21, 2023

mergify bot added the ok-to-test Label to trigger E2E tests label Feb 21, 2023

mergify bot merged commit b28b5e6 into ceph:devel Feb 21, 2023
3 checks passed

github-actions bot removed the ok-to-test Label to trigger E2E tests label Feb 21, 2023

haslersn reviewed Sep 14, 2023

View reviewed changes

		// default BackingSnapshot to true only for ReadOnly volume requests
		opts.BackingSnapshot = IsVolumeCreateRO(req.VolumeCapabilities)


		// Deleting snapshot before deleting pvcClone should succeed. It will be
		// deleted once all volumes that are backed by this snapshot are gone.

cephfs: use shallow volumes for the ROX accessMode #3651

cephfs: use shallow volumes for the ROX accessMode #3651

Conversation

riya-singhal31 commented Feb 7, 2023 • edited by Madhu-1

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

riya-singhal31 commented Feb 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

riya-singhal31 commented Feb 20, 2023

Rakshith-R left a comment

Choose a reason for hiding this comment

Rakshith-R commented Feb 20, 2023

yati1998 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rakshith-R commented Feb 20, 2023

mergify bot commented Feb 20, 2023

mergify bot commented Feb 20, 2023

✅ Branch has been successfully rebased

github-actions bot commented Feb 20, 2023

github-actions bot commented Feb 21, 2023

github-actions bot commented Feb 21, 2023

github-actions bot commented Feb 21, 2023

github-actions bot commented Feb 21, 2023

github-actions bot commented Feb 21, 2023

github-actions bot commented Feb 21, 2023

github-actions bot commented Feb 21, 2023

github-actions bot commented Feb 21, 2023

github-actions bot commented Feb 21, 2023

github-actions bot commented Feb 21, 2023

github-actions bot commented Feb 21, 2023

github-actions bot commented Feb 21, 2023

github-actions bot commented Feb 21, 2023

github-actions bot commented Feb 21, 2023

github-actions bot commented Feb 21, 2023

github-actions bot commented Feb 21, 2023

github-actions bot commented Feb 21, 2023

github-actions bot commented Feb 21, 2023

github-actions bot commented Feb 21, 2023

github-actions bot commented Feb 21, 2023

github-actions bot commented Feb 21, 2023

github-actions bot commented Feb 21, 2023

github-actions bot commented Feb 21, 2023

github-actions bot commented Feb 21, 2023

github-actions bot commented Feb 21, 2023

Choose a reason for hiding this comment

riya-singhal31 commented Feb 7, 2023 •

edited by Madhu-1