rbd: implement CSI-Addons ReclaimSpace operations #2724

nixpanic · 2021-12-20T11:23:12Z

This adds an initial implementation for the ControllerReclaimSpace and NodeReclaimSpace CSI-Addons operations.

There is no csi-addons/kubernetes-csi-addons controller or sidecar available yet, so e2e testing will need to be added later.

Updates: #2672
Closes: #2710

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!)
/retest all: run this in case the CentOS CI failed to start/report any test
progress or results

internal/csi-addons/rbd/reclaimspace.go

humblec · 2021-12-20T16:15:51Z

internal/csi-addons/rbd/reclaimspace.go

+}
+
+// NodeReclaimSpace runs fstrim or blkdiscard on the path where the volume is
+// mounted or attached. When a volume with multi-node permissions is detected,


just to understand this better, if RWX cap is detected, isnt it ? and not applicable for ROX eventhough both are from the multinode world.

read-only volumes should not get this request at all. @Rakshith-R @yati1998 is there a check (planned) for that in the k8s-controller?

internal/csi-addons/rbd/reclaimspace.go

humblec · 2021-12-20T16:18:25Z

internal/csi-addons/rbd/reclaimspace.go

+	}
+
+	// path can either be the staging path on the node, or the volume path
+	// inside an application container


actually iow, it can be a staging path or publish /target path in CSI terms.

@nixpanic did you plan to adjust the path definition here too as publish/target path instead of path inside the application container?

internal/csi-addons/rbd/reclaimspace.go

Madhu-1

mostly LGTM except for @humblec comments. @nixpanic are these changes manually tested?

Yuggupta27

LGTM!

Madhu-1

requesting changes to avoid accidental merging. once we get confirmation that manual testing is working fine. will approve it.

humblec · 2021-12-22T07:42:37Z

also please check the review comments posted a couple of days back on this PR ..

humblec

please check the review comments.

nixpanic · 2021-12-23T10:04:07Z

mostly LGTM except for @humblec comments. @nixpanic are these changes manually tested?

Manually tested with an ugly hack. Now also with minimal testing tool (WIP) at https://github.com/nixpanic/ceph-csi/tree/testing/csi-addons/reclaim-space/tools/csi-addons

nixpanic · 2021-12-23T10:12:47Z

@humblec please check my replies. Let me know if there is anything you are concerned about or do not accept. Thanks!

Will update the PR once you replied.

Pull request has been modified.

Yuggupta27

lgtm!

I tried to address all your questions related to the docs, and fixed the typos.

Pull request has been modified.

nixpanic · 2021-12-23T13:13:20Z

internal/csi-addons/rbd/reclaimspace.go

+		}
+	} else {
+		// append the right staging location used by this CSI-driver
+		path = fmt.Sprintf("%s/%s", path, volumeID)


The globalmount component was included, but it seems that is something CO dependent. Kubernetes includes that in the NodeStageVolume CSI request, and so should any caller of NodeReclaimSpace.

I1223 12:01:43.532455 375675 utils.go:191] ID: 45 Req-ID: 0001-0011-openshift-storage-0000000000000001-96e9a7ce-63c8-11ec-89d0-0a580a83001c GRPC call: /csi.v1.Node/NodeStageVolume I1223 12:01:43.532652 375675 utils.go:195] ID: 45 Req-ID: 0001-0011-openshift-storage-0000000000000001-96e9a7ce-63c8-11ec-89d0-0a580a83001c GRPC request: {"secrets":"***stripped***","staging_target_path":"/var/lib/kubelet/plugins/kubernetes.io/csi/pv/pvc-720d76c5-d994-4eb8-b8c0-d2bc5f493c17/globalmount

diff --git a/internal/csi-addons/rbd/reclaimspace.go b/internal/csi-addons/rbd/reclaimspace.go index 6898fb522..ab6c56c13 100644 --- a/internal/csi-addons/rbd/reclaimspace.go +++ b/internal/csi-addons/rbd/reclaimspace.go @@ -118,7 +118,7 @@ func (rsns *ReclaimSpaceNodeServer) NodeReclaimSpace( } } else { // append the right staging location used by this CSI-driver - path = fmt.Sprintf("%s/globalmount/%s", path, volumeID) + path = fmt.Sprintf("%s/%s", path, volumeID) } // do not allow RWX block-mode volumes, danger of data corruption

Signed-off-by: Niels de Vos <[email protected]>

The CSI Controller (provisioner) can call `rbd sparsify` to reduce the space consumption of the volume. Signed-off-by: Niels de Vos <[email protected]>

By calling fstrim/blkdiscard on the volume, space consumption should get reduced. Signed-off-by: Niels de Vos <[email protected]>

Signed-off-by: Niels de Vos <[email protected]>

humblec · 2021-12-23T15:55:35Z

@humblec please check my replies. Let me know if there is anything you are concerned about or do not accept. Thanks!

Will update the PR once you replied.

@nixpanic I am fine with the current version, only the multinode clarification is pending on Rakshit and Yati. but that should not be a blocker here.

nixpanic force-pushed the csi-addons/reclaim-space branch from ca763d0 to e68e12b Compare December 20, 2021 11:23

nixpanic requested review from Rakshith-R and yati1998 December 20, 2021 11:23

mergify bot added the component/rbd Issues related to RBD label Dec 20, 2021

nixpanic requested a review from Yuggupta27 December 20, 2021 11:24

nixpanic force-pushed the csi-addons/reclaim-space branch from e68e12b to 8104daa Compare December 20, 2021 11:43

yati1998 reviewed Dec 20, 2021

View reviewed changes

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

yati1998 reviewed Dec 20, 2021

View reviewed changes

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

nixpanic force-pushed the csi-addons/reclaim-space branch from 8104daa to 42020f2 Compare December 20, 2021 13:19

nixpanic requested review from yati1998 and a team December 20, 2021 13:19

Madhu-1 reviewed Dec 20, 2021

View reviewed changes

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

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

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

nixpanic force-pushed the csi-addons/reclaim-space branch from 42020f2 to 4475a99 Compare December 20, 2021 16:01

nixpanic requested a review from Madhu-1 December 20, 2021 16:01

humblec reviewed Dec 20, 2021

View reviewed changes

Madhu-1 reviewed Dec 21, 2021

View reviewed changes

Yuggupta27 previously approved these changes Dec 22, 2021

View reviewed changes

Madhu-1 requested changes Dec 22, 2021

View reviewed changes

humblec previously requested changes Dec 22, 2021

View reviewed changes

nixpanic requested a review from humblec December 23, 2021 10:26

nixpanic force-pushed the csi-addons/reclaim-space branch from 4475a99 to f537001 Compare December 23, 2021 12:15

nixpanic requested review from Madhu-1 and Yuggupta27 December 23, 2021 12:16

Madhu-1 previously approved these changes Dec 23, 2021

View reviewed changes

Yuggupta27 previously approved these changes Dec 23, 2021

View reviewed changes

nixpanic force-pushed the csi-addons/reclaim-space branch from f537001 to 54bb4d9 Compare December 23, 2021 13:08

nixpanic commented Dec 23, 2021

View reviewed changes

nixpanic requested review from Madhu-1 and Yuggupta27 December 23, 2021 13:13

Madhu-1 approved these changes Dec 23, 2021

View reviewed changes

Yuggupta27 approved these changes Dec 23, 2021

View reviewed changes

nixpanic added 6 commits December 23, 2021 15:30

rebase: update to latest csi-addons/spec

d6f3693

Signed-off-by: Niels de Vos <[email protected]>

rbd: implement CSI-Addons ControllerReclaimSpace

9c6ff91

The CSI Controller (provisioner) can call `rbd sparsify` to reduce the space consumption of the volume. Signed-off-by: Niels de Vos <[email protected]>

rbd: implement NodeReclaimSpace

70eda9f

By calling fstrim/blkdiscard on the volume, space consumption should get reduced. Signed-off-by: Niels de Vos <[email protected]>

rbd: expose CSI-Addons ReclaimSpace operations

c53ca7b

Signed-off-by: Niels de Vos <[email protected]>

cleanup: refactor gRPC middleware into NewMiddlewareServerOption

30095e9

Signed-off-by: Niels de Vos <[email protected]>

util: add common logging to csi-addons gRPC

a257be7

Signed-off-by: Niels de Vos <[email protected]>

nixpanic force-pushed the csi-addons/reclaim-space branch from 54bb4d9 to a257be7 Compare December 23, 2021 15:30

humblec approved these changes Dec 23, 2021

View reviewed changes

nixpanic mentioned this pull request Dec 23, 2021

build: new csi-addons tool for testing CSI-Addons operations #2745

Closed

mergify bot merged commit 8eaf1ab into ceph:devel Dec 23, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rbd: implement CSI-Addons ReclaimSpace operations #2724

rbd: implement CSI-Addons ReclaimSpace operations #2724

nixpanic commented Dec 20, 2021 •

edited

Loading

humblec Dec 20, 2021 •

edited

Loading

nixpanic Dec 23, 2021

humblec Dec 20, 2021 •

edited

Loading

humblec Dec 23, 2021

Madhu-1 left a comment

Yuggupta27 left a comment

Madhu-1 left a comment

humblec commented Dec 22, 2021

humblec left a comment

nixpanic commented Dec 23, 2021

nixpanic commented Dec 23, 2021

Yuggupta27 left a comment

nixpanic Dec 23, 2021

humblec commented Dec 23, 2021

rbd: implement CSI-Addons ReclaimSpace operations #2724

rbd: implement CSI-Addons ReclaimSpace operations #2724

Conversation

nixpanic commented Dec 20, 2021 • edited Loading

humblec Dec 20, 2021 • edited Loading

Choose a reason for hiding this comment

nixpanic Dec 23, 2021

Choose a reason for hiding this comment

humblec Dec 20, 2021 • edited Loading

Choose a reason for hiding this comment

humblec Dec 23, 2021

Choose a reason for hiding this comment

Madhu-1 left a comment

Choose a reason for hiding this comment

Yuggupta27 left a comment

Choose a reason for hiding this comment

Madhu-1 left a comment

Choose a reason for hiding this comment

humblec commented Dec 22, 2021

humblec left a comment

Choose a reason for hiding this comment

nixpanic commented Dec 23, 2021

nixpanic commented Dec 23, 2021

Yuggupta27 left a comment

Choose a reason for hiding this comment

nixpanic Dec 23, 2021

Choose a reason for hiding this comment

humblec commented Dec 23, 2021

nixpanic commented Dec 20, 2021 •

edited

Loading

humblec Dec 20, 2021 •

edited

Loading

humblec Dec 20, 2021 •

edited

Loading