rbd: add volume locks for reclaimspace operations #4641

iPraveenParihar · 2024-05-27T06:26:39Z

Describe what this PR does

This commit adds VolumeLocks on reclaimspace operations to
prevent multiple process executing rbd sparsify/fstrim on
same volume.

Related issues

Closes: #4637, #4648

Checklist:

Commit Message Formatting: Commit titles and messages follow guidelines in the developer guide.
Reviewed the developer guide on Submitting a Pull Request
Pending release notes updated with breaking and/or notable changes for the next major release.
Documentation has been updated, if necessary.
Unit tests have been added, if necessary.
Integration tests have been added, if necessary.

Show available bot commands

These commands are normally not required, but in case of issues, leave any of
the following bot commands in an otherwise empty comment in this PR:

/retest ci/centos/<job-name>: retest the <job-name> after unrelated
failure (please report the failure too!)

iPraveenParihar · 2024-05-27T07:09:35Z

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

internal/csi-addons/rbd/reclaimspace.go

Rakshith-R

Can we use already created controller and node servers ?

Rakshith-R · 2024-05-27T11:43:52Z

internal/rbd/driver/driver.go

@@ -213,7 +210,7 @@ func (r *Driver) setupCSIAddonsServer(conf *util.Config) error {
 	r.cas.RegisterService(is)

 	if conf.IsControllerServer {
-		rs := casrbd.NewReclaimSpaceControllerServer()
+		rs := casrbd.NewReclaimSpaceControllerServer(NewControllerServer(r.cd))


Suggested change

rs := casrbd.NewReclaimSpaceControllerServer(NewControllerServer(r.cd))

rs := casrbd.NewReclaimSpaceControllerServer(r.cs)

Yes, we should use the same already created Controller and Node Server struct, as they will share the same sets of locks. Since the ReplicationServer uses a new ControllerServer struct, I decided to follow the same for ReclaimSpace ControllerServer/NodeServer.

I forgot about the sharing locks with cephcsi controllerserver and nodeserver.

Let's just add new volume locks for both reclaimspace node&controller server in this pr and open a new issue to discuss whether we should share it with the default servers or not.

Both sparsify and fstrim are blocking operation, that's why we may need further discussion.

cc @Madhu-1

lets discuss on this one and we need to see can what will happen when we try to do nodeserver operations like (expand/umount) etc when the fstrim is going on can it leads to corruption etc. based on that we can have central/shared locking. Is it allowed to expand or remove the image when the ControllerReclaimOperation is going on etc.

internal/rbd/driver/driver.go

Madhu-1 · 2024-05-28T09:55:47Z

internal/rbd/driver/driver.go

@@ -224,7 +224,9 @@ func (r *Driver) setupCSIAddonsServer(conf *util.Config) error {
 	}

 	if conf.IsNodeServer {
-		rs := casrbd.NewReclaimSpaceNodeServer()
+		rs := casrbd.NewReclaimSpaceNodeServer(&rbd.NodeServer{
+			VolumeLocks: util.NewVolumeLocks(),


same here as well can you please check?

you might need to pass NewNodeServer(r.cd)

NewNodeServer() needs other params to be passed. So instead created NodeServer struct with volumeLocks

ceph-csi/internal/rbd/driver/driver.go

Lines 69 to 81 in 9574fa8

func NewNodeServer(

d *csicommon.CSIDriver,

t string,

nodeLabels, topology, crushLocationMap map[string]string,

) (*rbd.NodeServer, error) {

cliReadAffinityMapOptions := util.ConstructReadAffinityMapOption(crushLocationMap)

ns := rbd.NodeServer{

DefaultNodeServer: csicommon.NewDefaultNodeServer(d, t, cliReadAffinityMapOptions, topology, nodeLabels),

VolumeLocks: util.NewVolumeLocks(),

}

return &ns, nil

}

got it. the aim looks to be new locks instead of sharing as per #4641 (comment), lets do this one.

Madhu-1

LGTM, please test this change manually and share the result to ensure nothing breaks as we don't have any E2E tests for this one.

iPraveenParihar · 2024-05-29T07:36:39Z

Created a PVC of 1Gi and wrote dummy data of 500M and deleted it.

root@rbd-pod-test:/# dd if=/dev/zero of=/var/lib/www/html/dummy-file bs=1M count=500
500+0 records in
500+0 records out
524288000 bytes (524 MB, 500 MiB) copied, 1.6972 s, 309 MB/s
root@rbd-pod-test:/# sync
root@rbd-pod-test:/# rm /var/lib/www/html/dummy-file
root@rbd-pod-test:/# ls -l /var/lib/www/html/
total 16
drwx------ 2 root root 16384 May 29 07:23 lost+found

before ReclaimSpace operation -

[root@c1 /]# rbd du -p replicapool
warning: fast-diff map is not enabled for csi-vol-58a49f13-ff2e-4415-b1a8-21ec59d95017. operation may be slow.
NAME                                          PROVISIONED  USED
csi-vol-58a49f13-ff2e-4415-b1a8-21ec59d95017        1 GiB  528 MiB

created ReclaimSpaceJob for the PVC

pm@dhcp53-176:~$ cat <<EOF | k create -f -
> apiVersion: csiaddons.openshift.io/v1alpha1
kind: ReclaimSpaceJob
metadata:
  name: sample-1
spec:
  target:
    persistentVolumeClaim: rbd-pvc-test
  backOffLimit: 10
  retryDeadlineSeconds: 900
  timeout: 600
> EOF
reclaimspacejob.csiaddons.openshift.io/sample-1 created

/NodeReclaimSpace - fstrim operation succeed

I0529 07:29:28.134875 2566810 utils.go:198] ID: 23 GRPC call: /reclaimspace.ReclaimSpaceNode/NodeReclaimSpace
I0529 07:29:28.138386 2566810 utils.go:199] ID: 23 GRPC request: {"secrets":"***stripped***","staging_target_path":"/var/lib/kubelet/plugins/kubernetes.io/csi/rook-ceph.rbd.csi.ceph.com/cff6345ad1aff4cb25770de65b8012a934530bfd9d29bc77c557332df5607126/globalmount","volume_capability":{"AccessType":{"Mount":{}},"access_mode":{"mode":7}},"volume_id":"0001-0009-rook-ceph-0000000000000008-58a49f13-ff2e-4415-b1a8-21ec59d95017"}
I0529 07:29:28.439310 2566810 cephcmds.go:105] ID: 23 command succeeded: fstrim [/var/lib/kubelet/plugins/kubernetes.io/csi/rook-ceph.rbd.csi.ceph.com/cff6345ad1aff4cb25770de65b8012a934530bfd9d29bc77c557332df5607126/globalmount/0001-0009-rook-ceph-0000000000000008-58a49f13-ff2e-4415-b1a8-21ec59d95017]

[root@c1 /]# rbd du -p replicapool
warning: fast-diff map is not enabled for csi-vol-58a49f13-ff2e-4415-b1a8-21ec59d95017. operation may be slow.
NAME                                          PROVISIONED  USED
csi-vol-58a49f13-ff2e-4415-b1a8-21ec59d95017        1 GiB  304 MiB

/ControllerReclaimSpace - sparsify command succeed

I0529 07:29:28.589924       1 utils.go:198] ID: 26 GRPC call: /reclaimspace.ReclaimSpaceController/ControllerReclaimSpace
I0529 07:29:28.595724       1 utils.go:199] ID: 26 GRPC request: {"parameters":{"clusterID":"rook-ceph","encryptionKMSID":"azure-test","imageFeatures":"layering","imageFormat":"2","imageName":"csi-vol-58a49f13-ff2e-4415-b1a8-21ec59d95017","journalPool":"replicapool","pool":"replicapool","storage.kubernetes.io/csiProvisionerIdentity":"1716966822191-8666-rook-ceph.rbd.csi.ceph.com"},"secrets":"***stripped***","volume_id":"0001-0009-rook-ceph-0000000000000008-58a49f13-ff2e-4415-b1a8-21ec59d95017"}
I0529 07:29:28.690999       1 omap.go:89] ID: 26 got omap values: (pool="replicapool", namespace="", name="csi.volume.58a49f13-ff2e-4415-b1a8-21ec59d95017"): map[csi.imageid:73c2aff800e79 csi.imagename:csi-vol-58a49f13-ff2e-4415-b1a8-21ec59d95017 csi.volname:pvc-ffd7ee84-6ae9-47fc-8eb0-32d9c255b92b csi.volume.owner:reclaim-ns]
I0529 07:29:28.761741       1 reclaimspace.go:77] ID: 26 volume with ID "0001-0009-rook-ceph-0000000000000008-58a49f13-ff2e-4415-b1a8-21ec59d95017" is in use, skipping sparsify operation
I0529 07:29:28.762193       1 utils.go:205] ID: 26 GRPC response: {}
I0529 07:31:26.161922       1 utils.go:198] ID: 27 GRPC call: /reclaimspace.ReclaimSpaceController/ControllerReclaimSpace
I0529 07:31:26.163979       1 utils.go:199] ID: 27 GRPC request: {"parameters":{"clusterID":"rook-ceph","encryptionKMSID":"azure-test","imageFeatures":"layering","imageFormat":"2","imageName":"csi-vol-58a49f13-ff2e-4415-b1a8-21ec59d95017","journalPool":"replicapool","pool":"replicapool","storage.kubernetes.io/csiProvisionerIdentity":"1716966822191-8666-rook-ceph.rbd.csi.ceph.com"},"secrets":"***stripped***","volume_id":"0001-0009-rook-ceph-0000000000000008-58a49f13-ff2e-4415-b1a8-21ec59d95017"}
I0529 07:31:26.171744       1 omap.go:89] ID: 27 got omap values: (pool="replicapool", namespace="", name="csi.volume.58a49f13-ff2e-4415-b1a8-21ec59d95017"): map[csi.imageid:73c2aff800e79 csi.imagename:csi-vol-58a49f13-ff2e-4415-b1a8-21ec59d95017 csi.volname:pvc-ffd7ee84-6ae9-47fc-8eb0-32d9c255b92b csi.volume.owner:reclaim-ns]
I0529 07:31:26.697576       1 utils.go:205] ID: 27 GRPC response: {}

[root@c1 /]# rbd du -p replicapool
warning: fast-diff map is not enabled for csi-vol-58a49f13-ff2e-4415-b1a8-21ec59d95017. operation may be slow.
NAME                                          PROVISIONED  USED
csi-vol-58a49f13-ff2e-4415-b1a8-21ec59d95017        1 GiB  28 MiB

@Madhu-1, will this be enough for the test?

iPraveenParihar · 2024-05-29T07:57:26Z

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

Madhu-1 · 2024-05-29T09:25:17Z

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

Madhu-1 · 2024-05-29T09:30:12Z

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

Madhu-1 · 2024-05-29T09:33:22Z

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

nixpanic · 2024-05-29T13:27:13Z

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

nixpanic · 2024-05-29T13:56:07Z

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

nixpanic · 2024-05-29T14:12:00Z

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

ceph-csi-bot · 2024-05-29T14:57:32Z

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

ceph-csi-bot · 2024-05-29T14:57:33Z

/test ci/centos/upgrade-tests-cephfs

ceph-csi-bot · 2024-05-29T14:57:33Z

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

ceph-csi-bot · 2024-05-29T14:57:33Z

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

ceph-csi-bot · 2024-05-29T14:57:33Z

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

ceph-csi-bot · 2024-07-30T15:37:10Z

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

ceph-csi-bot · 2024-07-30T15:37:10Z

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

ceph-csi-bot · 2024-07-30T15:37:12Z

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

ceph-csi-bot · 2024-07-30T15:37:13Z

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

ceph-csi-bot · 2024-07-30T15:37:14Z

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

nixpanic · 2024-07-30T15:43:53Z

@Mergifyio requeue

multi-arch-build failed to download some Ceph packages

mergify · 2024-07-30T15:44:04Z

requeue

✅ The queue state of this pull request has been cleaned. It can be re-embarked automatically

This commit adds locks on reclaimspace operations to prevent multiple process executing rbd sparsify/fstrim on same volume. Signed-off-by: Praveen M <m.praveen@ibm.com>

Signed-off-by: Praveen M <m.praveen@ibm.com>

ceph-csi-bot · 2024-07-30T19:46:09Z

/test ci/centos/upgrade-tests-cephfs

ceph-csi-bot · 2024-07-30T19:46:10Z

/test ci/centos/upgrade-tests-rbd

ceph-csi-bot · 2024-07-30T19:46:10Z

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

ceph-csi-bot · 2024-07-30T19:46:10Z

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

ceph-csi-bot · 2024-07-30T19:46:10Z

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

ceph-csi-bot · 2024-07-30T19:46:11Z

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

ceph-csi-bot · 2024-07-30T19:46:11Z

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

ceph-csi-bot · 2024-07-30T19:46:11Z

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

ceph-csi-bot · 2024-07-30T19:46:11Z

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

ceph-csi-bot · 2024-07-30T19:46:11Z

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

ceph-csi-bot · 2024-07-30T19:46:11Z

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

ceph-csi-bot · 2024-07-30T19:46:12Z

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

ceph-csi-bot · 2024-07-30T19:46:12Z

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

ceph-csi-bot · 2024-07-30T19:46:12Z

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

nixpanic · 2024-07-31T06:29:42Z

@Mergifyio requeue

mergify · 2024-07-31T06:29:45Z

requeue

✅ The queue state of this pull request has been cleaned. It can be re-embarked automatically

mergify bot added the component/rbd Issues related to RBD label May 27, 2024

iPraveenParihar force-pushed the rbd/add-locks-for-reclaimspace branch from 0d019f8 to 88f490b Compare May 27, 2024 06:48

iPraveenParihar marked this pull request as ready for review May 27, 2024 07:21

iPraveenParihar requested a review from Rakshith-R May 27, 2024 07:23

Madhu-1 requested changes May 27, 2024

View reviewed changes

internal/csi-addons/rbd/reclaimspace.go Outdated Show resolved Hide resolved

iPraveenParihar force-pushed the rbd/add-locks-for-reclaimspace branch from 88f490b to 993a7c2 Compare May 27, 2024 10:18

iPraveenParihar requested a review from Madhu-1 May 27, 2024 10:32

Rakshith-R reviewed May 27, 2024

View reviewed changes

Rakshith-R reviewed May 28, 2024

View reviewed changes

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

iPraveenParihar force-pushed the rbd/add-locks-for-reclaimspace branch from 993a7c2 to 5fd00ce Compare May 28, 2024 09:45

Madhu-1 reviewed May 28, 2024

View reviewed changes

Madhu-1 previously approved these changes May 29, 2024

View reviewed changes

iPraveenParihar requested a review from Rakshith-R May 29, 2024 07:57

iPraveenParihar mentioned this pull request May 29, 2024

Can we share VolumeLocks for NodeServer/ControllerServer? #4648

Closed

nixpanic added the ok-to-test Label to trigger E2E tests label May 29, 2024

ceph-csi-bot removed the ok-to-test Label to trigger E2E tests label Jul 30, 2024

iPraveenParihar added 2 commits July 30, 2024 19:43

rbd: add volume locks for reclaimspace operations

e794e28

This commit adds locks on reclaimspace operations to prevent multiple process executing rbd sparsify/fstrim on same volume. Signed-off-by: Praveen M <m.praveen@ibm.com>

cleanup: remove unnecessary error return type

960e6c8

Signed-off-by: Praveen M <m.praveen@ibm.com>

iPraveenParihar force-pushed the rbd/add-locks-for-reclaimspace branch from fbd2de5 to 960e6c8 Compare July 30, 2024 19:43

mergify bot added the ok-to-test Label to trigger E2E tests label Jul 30, 2024

ceph-csi-bot removed the ok-to-test Label to trigger E2E tests label Jul 30, 2024

mergify bot merged commit 8fa3ac9 into ceph:devel Jul 31, 2024
40 checks passed

	rs := casrbd.NewReclaimSpaceControllerServer(NewControllerServer(r.cd))
	rs := casrbd.NewReclaimSpaceControllerServer(r.cs)

	func NewNodeServer(
	d *csicommon.CSIDriver,
	t string,
	nodeLabels, topology, crushLocationMap map[string]string,
	) (*rbd.NodeServer, error) {
	cliReadAffinityMapOptions := util.ConstructReadAffinityMapOption(crushLocationMap)
	ns := rbd.NodeServer{
	DefaultNodeServer: csicommon.NewDefaultNodeServer(d, t, cliReadAffinityMapOptions, topology, nodeLabels),
	VolumeLocks: util.NewVolumeLocks(),
	}

	return &ns, nil
	}

rbd: add volume locks for reclaimspace operations #4641

rbd: add volume locks for reclaimspace operations #4641

Conversation

iPraveenParihar commented May 27, 2024 • edited Loading

Describe what this PR does

Related issues

iPraveenParihar commented May 27, 2024

Rakshith-R left a 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

Madhu-1 left a comment

Choose a reason for hiding this comment

iPraveenParihar commented May 29, 2024 • edited Loading

iPraveenParihar commented May 29, 2024

Madhu-1 commented May 29, 2024

Madhu-1 commented May 29, 2024

Madhu-1 commented May 29, 2024

nixpanic commented May 29, 2024

nixpanic commented May 29, 2024

nixpanic commented May 29, 2024

ceph-csi-bot commented May 29, 2024

ceph-csi-bot commented May 29, 2024

ceph-csi-bot commented May 29, 2024

ceph-csi-bot commented May 29, 2024

ceph-csi-bot commented May 29, 2024

ceph-csi-bot commented Jul 30, 2024

ceph-csi-bot commented Jul 30, 2024

ceph-csi-bot commented Jul 30, 2024

ceph-csi-bot commented Jul 30, 2024

ceph-csi-bot commented Jul 30, 2024

nixpanic commented Jul 30, 2024

mergify bot commented Jul 30, 2024

✅ The queue state of this pull request has been cleaned. It can be re-embarked automatically

ceph-csi-bot commented Jul 30, 2024

ceph-csi-bot commented Jul 30, 2024

ceph-csi-bot commented Jul 30, 2024

ceph-csi-bot commented Jul 30, 2024

ceph-csi-bot commented Jul 30, 2024

ceph-csi-bot commented Jul 30, 2024

ceph-csi-bot commented Jul 30, 2024

ceph-csi-bot commented Jul 30, 2024

ceph-csi-bot commented Jul 30, 2024

ceph-csi-bot commented Jul 30, 2024

ceph-csi-bot commented Jul 30, 2024

ceph-csi-bot commented Jul 30, 2024

ceph-csi-bot commented Jul 30, 2024

ceph-csi-bot commented Jul 30, 2024

nixpanic commented Jul 31, 2024

mergify bot commented Jul 31, 2024

✅ The queue state of this pull request has been cleaned. It can be re-embarked automatically

iPraveenParihar commented May 27, 2024 •

edited

Loading

iPraveenParihar commented May 29, 2024 •

edited

Loading