Add command to clean the stale subvolume #206

yati1998 · 2023-11-20T05:26:48Z

Includes command to list and delete the stale subvolume
documentation to understand the usage of the command

yati1998 · 2023-11-20T05:29:20Z

@subhamkrai can you please review the PR

sp98 · 2023-11-21T05:47:09Z

docs/stale-subvolume.md

@@ -0,0 +1,36 @@
+# stale subvolume cleanup


Suggested change

# stale subvolume cleanup

# Stale subvolume cleanup

sp98 · 2023-11-21T05:49:39Z

docs/stale-subvolume.md

+## Example
+
+```bash
+kubectl rook-ceph subvolume-cleanup ls


Suggested change

kubectl rook-ceph subvolume-cleanup ls

kubectl rook-ceph subvolume-cleanup ls

Why not just call it subvolume instead of subvolume-cleanup ?
The commands would be
subvolume ls
subvolume clean

subhamkrai

@yati1998 could you follow the following doc former https://github.com/rook/kubectl-rook-ceph/blob/master/docs/rook.md to split up the section based on sub-commands.

docs/stale-subvolume.md

travisn · 2023-11-21T23:11:26Z

docs/stale-subvolume.md

+This would consider all the cases where we can have stale subvolume
+and delete them without impacting other resources and attached volumes.
+
+Few cases are mentioned below:


Why do these cases cause stale subvolumes? If there is a description of stale subvolumes already in another doc such as ceph docs, you could also link it instead of describing all the scenarios.

added the explanation in above comment. that may have these corner cases where we can have stale subvolumes.

The previous paragraph sounds good. How about if we just remove this list (lines 9-13)? Seems like we don't need to list all these scenarios.

subhamkrai · 2023-11-27T14:49:52Z

pkg/subvolume/subvolume.go

+	fsArgs := []string{
+		fmt.Sprintf("fs ls --format=json | jq --raw-output '.[].name'"),
+	}


Suggested change

fsArgs := []string{

fmt.Sprintf("fs ls --format=json | jq --raw-output '.[].name'"),

}

fsArgs := []string{

fmt.Sprintf("fs ls --format=json | jq --raw-output '.[].name'"),

}

This is incorrect, do not use fmt.Sprintf() in []string{} as it will treat it as single string in bash. To achieve the desired output

fsArgs := []string{ "fs", "ls", "--format", "json", }

get output of above and use json UnMarshal to get the name

travisn · 2023-11-27T23:18:05Z

docs/stale-subvolume.md

+This would consider all the cases where we can have stale subvolume
+and delete them without impacting other resources and attached volumes.
+
+Few cases are mentioned below:


The previous paragraph sounds good. How about if we just remove this list (lines 9-13)? Seems like we don't need to list all these scenarios.

travisn · 2023-11-27T23:19:18Z

docs/stale-subvolume.md

+## clean
+
+```bash
+kubectl rook-ceph subvolume clean csi-vol-427774b4-340b-11ed-8d66-0242ac110004


"Cleaning" a volume is just deleting it, right? For clarity, IMO we should call it "delete".

Suggested change

kubectl rook-ceph subvolume clean csi-vol-427774b4-340b-11ed-8d66-0242ac110004

kubectl rook-ceph subvolume delete csi-vol-427774b4-340b-11ed-8d66-0242ac110004

yes, we can do that.

travisn · 2023-11-27T23:20:03Z

pkg/subvolume/subvolume.go

+	// Get all csi reference on k8s side for all cephfs pv
+	k8sRef := exec.ExecuteBashCommand(getpv)
+	if k8sRef == "" {
+		logging.Info("No k8s pvs found. Nothing to do.")


Need to return here?

Suggested change

logging.Info("No k8s pvs found. Nothing to do.")

logging.Info("No k8s pvs found. Nothing to do.")

return

ahh, missed it.

travisn · 2023-11-27T23:24:37Z

pkg/subvolume/subvolume.go

+)
+
+var getpvlist = `
+kubectl get pv -o jsonpath={.items[*].spec.csi.volumeAttributes.subvolumeName}`


This is getting all PVs of all types in the cluster, not just rbd/cephfs PVs? Or does the subvolumeName in the jsonpath effectively help us filter out all others?

@subhamkrai In a separate PR, we need to factor out so kubectl can be replaced by another tool such as oc.

This is getting all PVs of all types in the cluster, not just rbd/cephfs PVs? Or does the subvolumeName in the jsonpath effectively help us filter out all others?

no it will not list all pvs, if you see the subvolumename is under csi, hence it will only list down the cephfs/rbd pvs.

travisn · 2023-11-27T23:26:46Z

pkg/subvolume/subvolume.go

+
+func List(ctx context.Context, clientsets *k8sutil.Clientsets, clusterNamespace string) []string {
+
+	getpv := fmt.Sprintf(getpvlist)


For readability, I'd suggest just putting the command directly here, so the dev doesn't need to look above to find what is executed.

Suggested change

getpv := fmt.Sprintf(getpvlist)

getpv := `kubectl get pv -o jsonpath={.items[*].spec.csi.volumeAttributes.subvolumeName}`

travisn · 2023-11-27T23:29:06Z

pkg/subvolume/subvolume.go

+		fmt.Sprintf("fs ls --format=json | jq --raw-output '.[].name'"),
+	}
+
+	fsList := exec.RunCommandInToolboxPod(ctx, clientsets, "ceph", fsArgs, clusterNamespace, true, false)


Is there a reason to run in the toolbox instead of the operator pod? Where possible, let's use the operator pod instead of the toolbox so it lessens the dependency on the toolbox.

Suggested change

fsList := exec.RunCommandInToolboxPod(ctx, clientsets, "ceph", fsArgs, clusterNamespace, true, false)

fsList := exec.RunCommandInOperatorPod(ctx, clientsets, "ceph", fsArgs, clusterNamespace, true, false)

Testing manually we execute into the toolbox pod, hence thought this function does the same. I will try with operator pod.

travisn · 2023-11-27T23:29:23Z

cmd/commands/root.go

@@ -67,7 +67,7 @@ func init() {

 	RootCmd.PersistentFlags().StringVar(&KubeConfig, "kubeconfig", "", "kubernetes config path")
 	RootCmd.PersistentFlags().StringVar(&OperatorNamespace, "operator-namespace", "", "Kubernetes namespace where rook operator is running")
-	RootCmd.PersistentFlags().StringVarP(&CephClusterNamespace, "namespace", "n", "rook-ceph", "Kubernetes namespace where CephCluster is created")
+	RootCmd.PersistentFlags().StringVarP(&CephClusterNamespace, "namespace", "n", "openshift-storage", "Kubernetes namespace where CephCluster is created")


need to revert

cmd/commands/subvolume.go

docs/stale-subvolume.md

yati1998 · 2023-11-28T06:07:18Z

Addressed all the comments, please do have a review. Meanwhile, I am adding another commit to delete the subvolumes.

cmd/commands/subvolume.go

docs/stale-subvolume.md

pkg/subvolume/subvolume.go

docs/stale-subvolume.md

pkg/subvolume/delete.go

pkg/subvolume/subvolume.go

travisn · 2023-11-28T22:01:53Z

docs/stale-subvolume.md

+## ls
+
+```bash
+kubectl rook-ceph stale-subvolume ls


Is the intention to list stale subvolumes in all filesystems? That seems reasonable, but we might want an optional argument for the name of a specific filesystem. Also, seems like we need to show which filesystem the volume belongs to.

yes the output shows the filesystem and subvolumegroup to which the subvolume belongs. But again giving users to add option to pass filesystem name is valid but would consider to take in as another PR/enhancement?

docs/stale-subvolume.md

subhamkrai

In general, we only need to logs the result no need to show all the info stepswise

pkg/subvolume/subvolume.go

sp98 · 2023-11-30T03:39:07Z

docs/stale-subvolume.md

+## ls
+
+```bash
+kubectl rook-ceph stale-subvolume ls


pkg/subvolume/delete.go

yati1998 · 2023-12-11T14:36:57Z

@subhamkrai @travisn @nixpanic

.github/workflows/go-test.yaml

cmd/commands/subvolume.go

docs/subvolume.md

pkg/filesystem/subvolume.go

travisn · 2023-12-11T16:59:45Z

cmd/commands/subvolume.go

+
+var listCmd = &cobra.Command{
+	Use:   "ls",
+	Short: "Print the list of stale subvolumes no longer in use.",


By default it prints them all, so we can keep this simple

Suggested change

Short: "Print the list of stale subvolumes no longer in use.",

Short: "Print the list of subvolumes.",

travisn · 2023-12-11T17:00:57Z

pkg/filesystem/subvolume.go

+
+	subvolumeNames := getK8sRefSubvolume(ctx, clientsets)
+	listCephFSSubvolumes(ctx, clientsets, operatorNamespace, clusterNamespace, includeStale, subvolumeNames)
+


nit: remove blank line before a closing bracket

Suggested change

travisn · 2023-12-11T17:01:23Z

pkg/filesystem/subvolume.go

+	if includeStale {
+		fmt.Println("Filesystem  Subvolume  SubvolumeGroup")
+	} else {
+		fmt.Println("Filesystem  Subvolume  SubvolumeGroup  State")


Suggested change

fmt.Println("Filesystem Subvolume SubvolumeGroup State")

fmt.Println("Filesystem Subvolume SubvolumeGroup Stale")

I did add it as state, it can be either in-use or stale.

travisn · 2023-12-11T17:01:43Z

pkg/filesystem/subvolume.go

+	staleSubvolumeNames := make(map[string]subVolumeInfo)
+	var svList string
+	if includeStale {
+		fmt.Println("Filesystem  Subvolume  SubvolumeGroup")


We always print the "Stale" column, right? No need for this?

Suggested change

fmt.Println("Filesystem Subvolume SubvolumeGroup")

printing stale cloumn here won't make sense, as we already pass on the --stale flag. if we do this, then there is no use of adding flag to ls. So IMHO, this is fine.

pkg/filesystem/subvolume.go

subhamkrai

In logging, I see fmt.Println is being used in many places, please try to use logging.Info where possible and same for errors and warning. Thank!

yati1998 · 2023-12-11T17:27:18Z

In logging, I see fmt.Println is being used in many places, please try to use logging.Info where possible and same for errors and warning. Thank!

As per @travisn comment's have used it and seems valid to me too. We don't want to log the info and just want to print the output.

docs/subvolume.md

this commit adds a design doc for including cleanup of stale subvolumes. Signed-off-by: yati1998 <ypadia@redhat.com>

subhamkrai

Lgtm, I'll let Travis merge the PR. Thanks for contibuting @yati1998

pkg/filesystem/subvolume.go

this commit adds command and script to cleanup the stale subvolume. Signed-off-by: yati1998 <ypadia@redhat.com>

this commit adds a ci to test the subvolume ls and delete command Signed-off-by: yati1998 <ypadia@redhat.com>

Madhu-1

sorry for leaving a comment on merged PR.

Madhu-1 · 2024-01-11T14:18:32Z

.github/workflows/go-test.yaml

+          kubectl rook-ceph ceph fs subvolume create myfs test-subvol group-a 
+          kubectl rook-ceph subvolume ls
+          kubectl rook-ceph subvolume ls --stale
+          kubectl rook-ceph subvolume delete test-subvol myfs group-a


this should be same as create arg fsname sub-volname groupname not the subvolname fsname and groupname

@yati1998 ^^

I will take care

@Madhu-1 sorry just forgot, this is in subvolname fsname and groupname so that we can add multiple subvolname for same fsname and groupname which will make group deletion easy incase we have many subvols.

doesnt seems right to me, we just stick to how we have for create API call, user can automate the CLI call if he /she thinks they need repetitive operations.

Madhu-1

posted few questions so that i dont forget it. we might need to discuss on those as well when we get some free time.

Madhu-1 · 2024-01-12T10:22:52Z

docs/subvolume.md

+# Subvolume cleanup
+
+The subvolume command is used to clean the stale subvolumes
+which have no parent-pvc attached to them.


Did we consider the external ceph cluster case to identify the stale subvolumes?

No, we need the cluster setup to check it. I did ping ashish , waiting for him on that part.
#211 this is the issue created to complete te remaining scenarios and minor changes .

Madhu-1 · 2024-01-12T10:23:50Z

docs/subvolume.md

+which have no parent-pvc attached to them.
+The command would list out all such subvolumes which needs to be removed.
+This would consider all the cases where we can have stale subvolume
+and delete them without impacting other resources and attached volumes.


I think this tool uses health check user for the external ceph cluster, AFAIK that user will not have all the required access to do subvolume operations.

Madhu-1 · 2024-01-12T10:24:52Z

docs/subvolume.md

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


we do still left with stale rados omap objects created for these subvolumes, how those will be cleaned up?

yes, @riya-singhal31 will be working on that.
The plan is to get the list of objects and delete the omap objects of the respective subvolume. We will take it as enchancement.

Madhu-1 · 2024-01-12T10:25:43Z

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


we will not be able to do the delete operation when the ceph cluster is full, Are we planning to handle stale resources cleanup in that case as well?

This case has not been considered but yeah we can work on it.
cc @riya-singhal31

Madhu-1 · 2024-01-12T10:26:33Z

pkg/filesystem/subvolume.go

+		if pv.Spec.CSI.VolumeAttributes["subvolumeName"] != "" {
+			subvolumeNames[pv.Spec.CSI.VolumeAttributes["subvolumeName"]] = subVolumeInfo{}
+		}


this might panic if we have non CSI PV as CSI is a pointer. please check

Madhu-1 · 2024-01-12T10:28:01Z

pkg/filesystem/subvolume.go

+				if ok || checkSnapshot(ctx, clientsets, operatorNamespace, clusterNamespace, fs.Name, sv.Name, svg.Name) {
+					// The volume is not stale if a PV was found, or it has a snapshot
+					stale = false


we can have a stale subvolume with a snapshot as well. we cannot consider if a subvolume has a snapshot as not a stale.

But deleting the subvolume if it has a snapshot can lead to data loss, hence the plan is to let them know that the subvolume has snapshot so it is good to delete the snapshot manually and then re-try.
What do you think should be an ideal case?

you can delete the subvolume with –retain-snapshots flag, this is take care of cleanup when the snapshot is deleted. this is what we do in cephcsi as well

sure will test and update.

sp98 reviewed Nov 21, 2023

View reviewed changes

yati1998 force-pushed the stale-volume branch from 1e68d4c to 010feba Compare November 21, 2023 08:35

yati1998 requested a review from sp98 November 21, 2023 08:36

subhamkrai requested changes Nov 21, 2023

View reviewed changes

subhamkrai requested review from travisn and Madhu-1 November 21, 2023 09:01

yati1998 force-pushed the stale-volume branch from 010feba to 5d08ded Compare November 21, 2023 09:17

yati1998 requested a review from subhamkrai November 21, 2023 09:17

travisn requested changes Nov 21, 2023

View reviewed changes

docs/stale-subvolume.md Outdated Show resolved Hide resolved

docs/stale-subvolume.md Outdated Show resolved Hide resolved

travisn reviewed Nov 21, 2023

View reviewed changes

yati1998 force-pushed the stale-volume branch from 5d08ded to 0c589a2 Compare November 27, 2023 12:29

yati1998 changed the title ~~doc: design doc for stale volume cleanup~~ [WIP]doc: design doc for stale volume cleanup Nov 27, 2023

subhamkrai requested changes Nov 27, 2023

View reviewed changes

travisn requested changes Nov 27, 2023

View reviewed changes

sp98 requested changes Nov 28, 2023

View reviewed changes

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

docs/stale-subvolume.md Outdated Show resolved Hide resolved

docs/stale-subvolume.md Outdated Show resolved Hide resolved

yati1998 force-pushed the stale-volume branch 3 times, most recently from 8e8d9e6 to 9541514 Compare November 28, 2023 06:06

yati1998 requested review from travisn, subhamkrai and sp98 November 28, 2023 06:06

subhamkrai requested changes Nov 28, 2023

View reviewed changes

yati1998 force-pushed the stale-volume branch 3 times, most recently from f8a578f to aae100f Compare November 28, 2023 12:14

travisn requested changes Nov 28, 2023

View reviewed changes

subhamkrai requested changes Nov 29, 2023

View reviewed changes

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

sp98 requested changes Nov 30, 2023

View reviewed changes

yati1998 requested a review from nixpanic December 11, 2023 12:46

yati1998 force-pushed the stale-volume branch 2 times, most recently from 1f014fe to faba182 Compare December 11, 2023 13:43

travisn requested changes Dec 11, 2023

View reviewed changes

travisn reviewed Dec 11, 2023

View reviewed changes

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

yati1998 force-pushed the stale-volume branch from faba182 to c0df578 Compare December 11, 2023 15:24

yati1998 requested a review from travisn December 11, 2023 15:26

yati1998 force-pushed the stale-volume branch 3 times, most recently from 86a12b2 to 7044dfd Compare December 11, 2023 16:52

travisn requested changes Dec 11, 2023

View reviewed changes

subhamkrai requested changes Dec 11, 2023

View reviewed changes

yati1998 force-pushed the stale-volume branch from 7044dfd to 2ff2517 Compare December 11, 2023 17:43

yati1998 requested review from travisn and subhamkrai December 11, 2023 17:44

travisn requested changes Dec 11, 2023

View reviewed changes

docs/subvolume.md Outdated Show resolved Hide resolved

docs/subvolume.md Outdated Show resolved Hide resolved

doc: design doc for stale volume cleanup

da11ed9

this commit adds a design doc for including cleanup of stale subvolumes. Signed-off-by: yati1998 <ypadia@redhat.com>

yati1998 force-pushed the stale-volume branch from 2ff2517 to 0bf86ab Compare December 11, 2023 17:54

subhamkrai approved these changes Dec 11, 2023

View reviewed changes

travisn reviewed Dec 11, 2023

View reviewed changes

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

pkg/filesystem/subvolume.go Show resolved Hide resolved

yati1998 force-pushed the stale-volume branch from 0bf86ab to 406071b Compare December 11, 2023 18:15

yati1998 added 2 commits December 11, 2023 23:49

add command to clean the subvolume

f134ed4

this commit adds command and script to cleanup the stale subvolume. Signed-off-by: yati1998 <ypadia@redhat.com>

adds ci to test the subvolume command

92810dc

this commit adds a ci to test the subvolume ls and delete command Signed-off-by: yati1998 <ypadia@redhat.com>

yati1998 force-pushed the stale-volume branch from 406071b to 92810dc Compare December 11, 2023 18:19

travisn approved these changes Dec 11, 2023

View reviewed changes

travisn merged commit 70ede27 into rook:master Dec 11, 2023
5 checks passed

Madhu-1 reviewed Jan 11, 2024

View reviewed changes

Madhu-1 reviewed Jan 12, 2024

View reviewed changes

	kubectl rook-ceph subvolume-cleanup ls
	kubectl rook-ceph subvolume-cleanup ls

	kubectl rook-ceph subvolume clean csi-vol-427774b4-340b-11ed-8d66-0242ac110004
	kubectl rook-ceph subvolume delete csi-vol-427774b4-340b-11ed-8d66-0242ac110004

	logging.Info("No k8s pvs found. Nothing to do.")
	logging.Info("No k8s pvs found. Nothing to do.")
	return


		func List(ctx context.Context, clientsets *k8sutil.Clientsets, clusterNamespace string) []string {

		getpv := fmt.Sprintf(getpvlist)

	getpv := fmt.Sprintf(getpvlist)
	getpv := `kubectl get pv -o jsonpath={.items[*].spec.csi.volumeAttributes.subvolumeName}`

	fsList := exec.RunCommandInToolboxPod(ctx, clientsets, "ceph", fsArgs, clusterNamespace, true, false)
	fsList := exec.RunCommandInOperatorPod(ctx, clientsets, "ceph", fsArgs, clusterNamespace, true, false)

	Short: "Print the list of stale subvolumes no longer in use.",
	Short: "Print the list of subvolumes.",


		subvolumeNames := getK8sRefSubvolume(ctx, clientsets)
		listCephFSSubvolumes(ctx, clientsets, operatorNamespace, clusterNamespace, includeStale, subvolumeNames)

	fmt.Println("Filesystem Subvolume SubvolumeGroup State")
	fmt.Println("Filesystem Subvolume SubvolumeGroup Stale")

Add command to clean the stale subvolume #206

Add command to clean the stale subvolume #206

Conversation

yati1998 commented Nov 20, 2023 • edited Loading

yati1998 commented Nov 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

subhamkrai 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

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

yati1998 commented Nov 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

subhamkrai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yati1998 commented Dec 11, 2023

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

subhamkrai left a comment

Choose a reason for hiding this comment

yati1998 commented Dec 11, 2023

subhamkrai left a 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

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

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 Nov 20, 2023 •

edited

Loading