Allow to delete subvolume with retain-snapshot feature #233

yati1998 · 2024-02-02T11:20:35Z

This PR introduces the following enhancement to the stale subvolume cleanup process:

Allow to delete the stale subvolumes that have snapshots with retain-snapshot feature
Correct the order of arguments passed while deleting the subvolume
Allow users to delete only one subvolume at a time.

yati1998 · 2024-02-02T11:24:33Z

@Madhu-1 @travisn added a few enhancements. Please do have a look.

cmd/commands/subvolume.go

Madhu-1 · 2024-02-02T11:40:45Z

cmd/commands/subvolume.go

 	},
 }

 func init() {
 	SubvolumeCmd.AddCommand(listCmd)
 	SubvolumeCmd.PersistentFlags().Bool("stale", false, "List only stale subvolumes")
 	SubvolumeCmd.AddCommand(deleteCmd)
+	SubvolumeCmd.PersistentFlags().Bool("retain-snapshots", false, "Delete subvolume but retain the snapshot")


This should be the default behaviour, we dont need to have a new flag from user, because you can never delete a subvolume which is having snapshots without retain-snapshot flag in ceph CLI.

The idea was to list the subvolmes along with the status(if it has snapshots or not), so user can pass this flag and delete the stale subvolumes, even if it has snapshots.

IMHO we dont need any flag this should be the default behaviour

docs/subvolume.md

Madhu-1 · 2024-02-02T11:42:33Z

pkg/filesystem/subvolume.go

@@ -53,7 +53,9 @@ func getK8sRefSubvolume(ctx context.Context, clientsets *k8sutil.Clientsets) map
 	subvolumeNames := make(map[string]subVolumeInfo)
 	for _, pv := range pvList.Items {
 		if pv.Spec.CSI != nil && pv.Spec.CSI.VolumeAttributes["subvolumeName"] != "" {
-			subvolumeNames[pv.Spec.CSI.VolumeAttributes["subvolumeName"]] = subVolumeInfo{}
+			if pv.Spec.CSI.VolumeAttributes["subvolumeName"] != "" {


in which case the subvolumeName can be empty?

@yati1998 this is still not answered

It was added as a check when there will be no CSI subvolumes, but yeah with your last PR to fix this issue, got to understand the correct check.
if there is a CSI volume, don't think the subvolumename will be empty. I will remove the check, it's not required.

pkg/filesystem/subvolume.go

travisn

Please rebase to pick up the recent CI fix.

yati1998 · 2024-02-03T06:07:26Z

@Mergifyio rebase

docs/subvolume.md

pkg/filesystem/subvolume.go

docs/subvolume.md

pkg/filesystem/subvolume.go

travisn

Please note the CI failures

cmd/commands/subvolume.go

yati1998 · 2024-02-05T17:56:50Z

Please note the CI failures

Run set -ex
+ set -ex
+ kubectl rook-ceph rbd ls replicapool
Warning: rook version 'rook: v1.13.0-alpha.0.220.gf5fcf[9](https://github.com/rook/kubectl-rook-ceph/actions/runs/7781414479/job/21215804058?pr=233#step:11:10)6e0' is running a pre-release version of Rook.```

It says failure while listing the rbd pools, doesn't seems to be related to my changes. Any idea on what's the issue?

travisn

Please note the CI failures

Run set -ex
+ set -ex
+ kubectl rook-ceph rbd ls replicapool
Warning: rook version 'rook: v1.13.0-alpha.0.220.gf5fcf[9](https://github.com/rook/kubectl-rook-ceph/actions/runs/7781414479/job/21215804058?pr=233#step:11:10)6e0' is running a pre-release version of Rook.```

It says failure while listing the rbd pools, doesn't seems to be related to my changes. Any idea on what's the issue?

Looks like no OSDs were created, it seems related to other CI issues we are seeing related to #234. You might try a force push to see if a fresh run might succeed.

cmd/commands/subvolume.go

yati1998 · 2024-02-05T18:20:20Z

https://github.com/rook/kubectl-rook-ceph/blob/master/.github/workflows/go-test.yaml#L81
cc @travisn

travisn · 2024-02-05T19:18:46Z

The PR looks good, we should have a separate fix for the CI today, see discussion in rook/rook#13684

pkg/filesystem/subvolume.go

cmd/commands/subvolume.go

pkg/filesystem/subvolume.go

docs/subvolume.md

Madhu-1 · 2024-02-07T07:52:34Z

pkg/filesystem/subvolume.go

@@ -53,7 +53,9 @@ func getK8sRefSubvolume(ctx context.Context, clientsets *k8sutil.Clientsets) map
 	subvolumeNames := make(map[string]subVolumeInfo)
 	for _, pv := range pvList.Items {
 		if pv.Spec.CSI != nil && pv.Spec.CSI.VolumeAttributes["subvolumeName"] != "" {
-			subvolumeNames[pv.Spec.CSI.VolumeAttributes["subvolumeName"]] = subVolumeInfo{}
+			if pv.Spec.CSI.VolumeAttributes["subvolumeName"] != "" {


@yati1998 this is still not answered

travisn · 2024-02-07T20:29:33Z

cmd/commands/subvolume.go

+	Use:     "delete",
+	Short:   "Deletes a stale subvolume.",
+	Args:    cobra.MinimumNArgs(2),
+	Example: "kubectl rook-ceph delete <filesystem> <subvolume> <subvolumegroup>",


Let's use square brackets around optional args

Suggested change

Example: "kubectl rook-ceph delete <filesystem> <subvolume> <subvolumegroup>",

Example: "kubectl rook-ceph delete <filesystem> <subvolume> [subvolumegroup]",

docs/subvolume.md

Madhu-1

small nits, LGTM

Madhu-1 · 2024-02-08T07:06:38Z

docs/subvolume.md


 ```

 ```bash
-kubectl rook-ceph subvolume delete csi-vol-427774b4-340b-11ed-8d66-0242ac110004,csi-vol-427774b4-340b-11ed-8d66-0242ac110005 ocs-storagecluster csi


add a lone about to mention that you are deleting without subvolumegroup or else no one will notice it and we will have two delete commands in the doc

Madhu-1 · 2024-02-08T07:09:15Z

pkg/filesystem/subvolume.go

+	var svg string
+	if len(args) == 2 {
+		svg = "csi"
+	} else {
+		svg = args[2]
+	}


Suggested change

var svg string

if len(args) == 2 {

svg = "csi"

} else {

svg = args[2]

}

svg:="csi"

if len(args) > 2 {

svg = args[2]

}

Madhu-1 · 2024-02-08T07:09:25Z

pkg/filesystem/subvolume.go

+	}
+
+	check := isStaleSubvolume(subvol, k8sSubvolume)
+


remove extra line

Madhu-1 · 2024-02-08T07:11:03Z

pkg/filesystem/subvolume.go

 				}
-				subvolumeNames[sv.Name] = subVolumeInfo{fs.Name, svg.Name}
+				subvolumeNames[sv.Name] = subVolumeInfo{fs.Name, svg.Name, state}
 				fmt.Println(fs.Name, sv.Name, svg.Name, status)


Println is not a formatted output when we have large number of subvolumes it difficult to read, can you please open an issue to print as formatted like kubectl output

#211 , it is already added here, I will check on how we can have a proper format for it.

Madhu-1 · 2024-02-08T07:11:48Z

pkg/filesystem/subvolume.go

+	InUse             = "in-use"
+	Stale             = "stale"
+	StaleWithSnapshot = "stale-with-snapshot"


no need to be exported variables as they are not used anywhere else outside this file?

docs/subvolume.md

pkg/filesystem/subvolume.go

cmd/commands/subvolume.go

pkg/filesystem/subvolume.go

yati1998 · 2024-02-12T06:04:41Z

@travisn @subhamkrai @Madhu-1 I have addressed all the comments, please do review it once you are free.

This commit includes the retain-snapshot flag to delete the subvolume. Signed-off-by: yati1998 <[email protected]>

travisn · 2024-02-12T18:11:36Z

pkg/filesystem/subvolume.go

@@ -166,36 +205,13 @@ func unMarshaljson(list string) []fsStruct {
 	return unmarshal
 }

-func Delete(ctx context.Context, clientsets *k8sutil.Clientsets, OperatorNamespace, CephClusterNamespace, subList, fs, svg string) {
-	subvollist := strings.Split(subList, ",")
+func Delete(ctx context.Context, clientsets *k8sutil.Clientsets, OperatorNamespace, CephClusterNamespace, fs, subvol, svg string) {
 	k8sSubvolume := getK8sRefSubvolume(ctx, clientsets)


This will query all of the PVs in the cluster, just to confirm if it exists. Instead, can we skip this query, and just attempt to delete the subvolume? If it doesn't exist, hopefully an error will already be returned that it doesn't exist, which is anyway what we will print as well.

It's not about the subvolume being present, this is a check added to make sure we are deleting only stale subvolumes. We are listing all the subvolumes with ls command, but still, the user may enter an incorrect subvolume name, which may lead to the deletion of the non-stale subvolume. Hence, it's good to check before directly deleting.

Ok I see now that it is checking to confirm the stale volume. But the perf implication is still concerning. In a test cluster with a handful of PVs it is fine. But what if there are 100 or 1K or 10K PVs in the cluster? How long will this take? Or will these queries for all the PVs even fail if there are too many. I'll open an issue to test it.

travisn · 2024-02-13T17:56:34Z

pkg/filesystem/subvolume.go

@@ -166,36 +205,13 @@ func unMarshaljson(list string) []fsStruct {
 	return unmarshal
 }

-func Delete(ctx context.Context, clientsets *k8sutil.Clientsets, OperatorNamespace, CephClusterNamespace, subList, fs, svg string) {
-	subvollist := strings.Split(subList, ",")
+func Delete(ctx context.Context, clientsets *k8sutil.Clientsets, OperatorNamespace, CephClusterNamespace, fs, subvol, svg string) {
 	k8sSubvolume := getK8sRefSubvolume(ctx, clientsets)


Ok I see now that it is checking to confirm the stale volume. But the perf implication is still concerning. In a test cluster with a handful of PVs it is fine. But what if there are 100 or 1K or 10K PVs in the cluster? How long will this take? Or will these queries for all the PVs even fail if there are too many. I'll open an issue to test it.

feedback addressed

yati1998 force-pushed the enhancement branch from 8ee3e0c to a26611b Compare February 2, 2024 11:23

Madhu-1 reviewed Feb 2, 2024

View reviewed changes

travisn reviewed Feb 2, 2024

View reviewed changes

yati1998 force-pushed the enhancement branch from a26611b to 17e4589 Compare February 3, 2024 07:38

yati1998 requested review from travisn and Madhu-1 February 3, 2024 07:39

subhamkrai requested changes Feb 5, 2024

View reviewed changes

Madhu-1 reviewed Feb 5, 2024

View reviewed changes

docs/subvolume.md Outdated Show resolved Hide resolved

docs/subvolume.md Outdated Show resolved Hide resolved

pkg/filesystem/subvolume.go Outdated Show resolved Hide resolved

yati1998 force-pushed the enhancement branch from 17e4589 to fbb24f3 Compare February 5, 2024 08:13

yati1998 requested review from Madhu-1 and subhamkrai February 5, 2024 14:39

travisn requested changes Feb 5, 2024

View reviewed changes

cmd/commands/subvolume.go Outdated Show resolved Hide resolved

cmd/commands/subvolume.go Show resolved Hide resolved

travisn reviewed Feb 5, 2024

View reviewed changes

cmd/commands/subvolume.go Outdated Show resolved Hide resolved

cmd/commands/subvolume.go Show resolved Hide resolved

Madhu-1 reviewed Feb 6, 2024

View reviewed changes

pkg/filesystem/subvolume.go Outdated Show resolved Hide resolved

pkg/filesystem/subvolume.go Show resolved Hide resolved

pkg/filesystem/subvolume.go Outdated Show resolved Hide resolved

yati1998 force-pushed the enhancement branch from fbb24f3 to 368637f Compare February 6, 2024 14:58

yati1998 requested review from Madhu-1 and travisn February 6, 2024 14:58

travisn reviewed Feb 6, 2024

View reviewed changes

cmd/commands/subvolume.go Outdated Show resolved Hide resolved

pkg/filesystem/subvolume.go Outdated Show resolved Hide resolved

yati1998 force-pushed the enhancement branch from 368637f to a214d41 Compare February 7, 2024 03:43

Madhu-1 reviewed Feb 7, 2024

View reviewed changes

yati1998 force-pushed the enhancement branch 3 times, most recently from c4194a5 to 9b47647 Compare February 7, 2024 13:05

travisn requested changes Feb 7, 2024

View reviewed changes

yati1998 force-pushed the enhancement branch from 9b47647 to d01210c Compare February 8, 2024 04:28

yati1998 requested review from Madhu-1 and travisn February 8, 2024 04:28

yati1998 force-pushed the enhancement branch from d01210c to 3a98ea8 Compare February 8, 2024 04:44

Madhu-1 reviewed Feb 8, 2024

View reviewed changes

yati1998 force-pushed the enhancement branch from 3a98ea8 to 6ca6557 Compare February 8, 2024 08:22

yati1998 requested a review from Madhu-1 February 8, 2024 08:22

travisn reviewed Feb 8, 2024

View reviewed changes

docs/subvolume.md Outdated Show resolved Hide resolved

pkg/filesystem/subvolume.go Outdated Show resolved Hide resolved

yati1998 force-pushed the enhancement branch from 6ca6557 to 66cb8cb Compare February 9, 2024 05:00

yati1998 requested a review from travisn February 9, 2024 05:02

subhamkrai previously requested changes Feb 9, 2024

View reviewed changes

cmd/commands/subvolume.go Outdated Show resolved Hide resolved

pkg/filesystem/subvolume.go Outdated Show resolved Hide resolved

yati1998 force-pushed the enhancement branch from 66cb8cb to cad9411 Compare February 12, 2024 06:03

yati1998 requested a review from subhamkrai February 12, 2024 06:03

yati1998 force-pushed the enhancement branch from cad9411 to 19a7dd3 Compare February 12, 2024 06:20

csi: allow to delete subvolume with retain-snapshot feature

a0359c0

This commit includes the retain-snapshot flag to delete the subvolume. Signed-off-by: yati1998 <[email protected]>

yati1998 force-pushed the enhancement branch from 19a7dd3 to a0359c0 Compare February 12, 2024 06:25

travisn requested changes Feb 12, 2024

View reviewed changes

yati1998 requested a review from travisn February 13, 2024 13:32

travisn approved these changes Feb 13, 2024

View reviewed changes

travisn merged commit f28357c into rook:master Feb 13, 2024
6 checks passed

travisn mentioned this pull request Feb 13, 2024

Test performance of purging a stale subvolume when there are many PVs in the cluster #242

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow to delete subvolume with retain-snapshot feature #233

Allow to delete subvolume with retain-snapshot feature #233

yati1998 commented Feb 2, 2024

yati1998 commented Feb 2, 2024

Madhu-1 Feb 2, 2024

yati1998 Feb 3, 2024

Madhu-1 Feb 5, 2024

Madhu-1 Feb 2, 2024

Madhu-1 Feb 7, 2024

yati1998 Feb 7, 2024

travisn left a comment

yati1998 commented Feb 3, 2024

travisn left a comment

yati1998 commented Feb 5, 2024

travisn left a comment

yati1998 commented Feb 5, 2024

travisn commented Feb 5, 2024

Madhu-1 Feb 7, 2024

travisn Feb 7, 2024

Madhu-1 left a comment

Madhu-1 Feb 8, 2024

Madhu-1 Feb 8, 2024

Madhu-1 Feb 8, 2024

Madhu-1 Feb 8, 2024

yati1998 Feb 8, 2024

Madhu-1 Feb 8, 2024

yati1998 commented Feb 12, 2024

travisn Feb 12, 2024

yati1998 Feb 13, 2024

travisn Feb 13, 2024

travisn Feb 13, 2024

	Example: "kubectl rook-ceph delete <filesystem> <subvolume> <subvolumegroup>",
	Example: "kubectl rook-ceph delete <filesystem> <subvolume> [subvolumegroup]",

Allow to delete subvolume with retain-snapshot feature #233

Allow to delete subvolume with retain-snapshot feature #233

Conversation

yati1998 commented Feb 2, 2024

yati1998 commented Feb 2, 2024

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

travisn left a comment

Choose a reason for hiding this comment

yati1998 commented Feb 3, 2024

travisn left a comment

Choose a reason for hiding this comment

yati1998 commented Feb 5, 2024

travisn left a comment

Choose a reason for hiding this comment

yati1998 commented Feb 5, 2024

travisn commented Feb 5, 2024

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

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

yati1998 commented Feb 12, 2024

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