csi: add rbd ls option #333

OdedViner · 2024-10-27T08:25:51Z

Checklist:

Commit Message Formatting: Commit titles and messages follow guidelines in the developer guide.
Reviewed the developer guide on Submitting a Pull Request
Documentation has been updated, if necessary.
Unit tests have been added, if necessary.
Integration tests have been added, if necessary.

subhamkrai

@OdedViner please fix the CI error and add a new CI test for this command thanks.

OdedViner · 2024-10-28T15:18:26Z

@OdedViner please fix the CI error and add a new CI test for this command thanks.

Done

 set -ex
  kubectl rook-ceph ${NS_OPT} rbd ls replicapool
  shell: /usr/bin/bash --noprofile --norc -eo pipefail -x {0}
  env:
    ROOK_PLUGIN_SKIP_PROMPTS: true
    NS_OPT:  
+ set -ex
+ kubectl rook-ceph rbd ls replicapool
poolName     imageName                                     
--------     ---------                                     
builtin-mgr  --                                            
replicapool  csi-vol-df470f06-d699-4b8c-8f5d-b1436c437e12

https://github.com/rook/kubectl-rook-ceph/actions/runs/11556764413/job/32165252505?pr=333

subhamkrai · 2024-10-29T09:36:06Z

.github/workflows/go-test-config/action.yaml

@@ -116,6 +116,12 @@ runs:
        tests/github-action-helper.sh create_stale_subvolume
        subVol=$(kubectl rook-ceph ${NS_OPT} subvolume ls --stale | awk '{print $2}' | grep csi-vol)
        kubectl rook_ceph ${NS_OPT} subvolume delete myfs $subVol
+
+    - name: Get Rbd list


Suggested change

- name: Get Rbd list

- name: Get rbd list

subhamkrai · 2024-10-29T09:36:37Z

cmd/commands/rbd.go

+
+var listCmdRbd = &cobra.Command{
+	Use:   "ls",
+	Short: "Print the list of subvolumes.",


Suggested change

Short: "Print the list of subvolumes.",

Short: "Print the list of rbd volumes.",

subhamkrai · 2024-10-29T09:39:42Z

pkg/rbd/rbd.go

+)
+
+// List retrieves and displays the Ceph block pools and their associated images.
+func List(ctx context.Context, clientsets *k8sutil.Clientsets, clusterNamespace string) {


Suggested change

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

func ListRbdVolumes(ctx context.Context, clientsets *k8sutil.Clientsets, clusterNamespace string) {

subhamkrai · 2024-10-29T09:41:05Z

pkg/rbd/rbd.go

+func List(ctx context.Context, clientsets *k8sutil.Clientsets, clusterNamespace string) {
+	blockPoolList, err := clientsets.Rook.CephV1().CephBlockPools(clusterNamespace).List(ctx, v1.ListOptions{})
+	if err != nil {
+		logging.Fatal(err)


Suggested change

logging.Fatal(err)

logging.Fatal(fmt.Errorf(err, "failed to list CephBlockPools"))

subhamkrai · 2024-10-29T09:41:17Z

pkg/rbd/rbd.go

+	if err != nil {
+		logging.Fatal(err)
+	}
+	// var blockPoolNames []string


Suggested change

// var blockPoolNames []string

?

subhamkrai · 2024-10-29T09:41:54Z

pkg/rbd/rbd.go

+	blockPoolNames = make(map[string]string)
+
+	for _, blockPool := range blockPoolList.Items {
+		// blockPoolNames = append(blockPoolNames, blockPool.Name)


Suggested change

// blockPoolNames = append(blockPoolNames, blockPool.Name)

?

subhamkrai · 2024-10-29T09:42:56Z

pkg/rbd/rbd.go

+	fmt.Fprintln(writer, "poolName\timageName\t")
+	fmt.Fprintln(writer, "--------\t---------\t")


Suggested change

fmt.Fprintln(writer, "poolName\timageName\t")

fmt.Fprintln(writer, "--------\t---------\t")

fmt.Fprintln(writer, "poolName\timageName\t")

fmt.Fprintln(writer, "--------\t---------\t")

can we use logging pkg for logging here and below

$ ./bin/kubectl-rook-ceph rbd ls Info: poolName imageName Info: -------- --------- Info: builtin-mgr -- Info: replicapool csi-vol-f090b7e1-a821-4a71-a7d2-7e3e2aaea8d1

@subhamkrai The output when I use logging is not clear; is it possible to use only fmt.Fprintln ?

hmm can you prefix with \n for new line and see if that make sense? otherwise if not then feel free to use fmt.Fprintln

$ ./bin/kubectl-rook-ceph rbd ls poolName imageName -------- --------- builtin-mgr -- replicapool csi-vol-f090b7e1-a821-4a71-a7d2-7e3e2aaea8d1

output when using fmt.Fprintf

subhamkrai · 2024-10-29T09:43:40Z

pkg/rbd/rbd.go

+	}
+
+	// Flush the writer
+	writer.Flush()


Suggested change

writer.Flush()

defer writer.Flush()

and put this where writer is initialized at L45

subhamkrai · 2024-11-04T02:29:26Z

pkg/rbd/rbd.go

@@ -0,0 +1,52 @@
+package rbd


please add rook copyright comments.

Madhu-1 · 2024-11-04T13:15:59Z

pkg/rbd/rbd.go

+		blockPoolNames[blockPool.Name] = "--"
+	}
+
+	cmd := "rbd"


We also need to take care of listing the images in the radosNamespace inside the pool not just in default rados namespace

@Madhu-1 Are clusterNamespace and radosNamespace different?

yes thats correct, i think we need to accept the --pool --namespace CR names as the arguments or we might end up listing all the pools and all the rados namespaces. @subhamkrai @travisn WDYT?

Correct, we need to use --namespace also to get the image namespace

blockPoolList, err := clientsets.Rook.CephV1().CephBlockPools(clusterNamespace).List(ctx, v1.ListOptions{})

Currently, this code is checking for CephBlockPools CRs only within the specified clusterNamespace (rook-ceph). Would you like to extend the check to see if CephBlockPools CRs exist in other namespaces as well?

If so, we can update it to:

blockPoolList, err := clientsets.Rook.CephV1().CephBlockPools("").List(ctx, v1.ListOptions{})

Using "" as the namespace parameter makes the List call retrieve resources from all namespaces.

Additionally, would you like to include a column for the image namespace? This could help provide more context, as in the following example:

$ ./bin/kubectl-rook-ceph rbd ls poolName imageName namespace -------- --------- --------- builtin-mgr -- rook-ceph replicapool csi-vol-f090b7e1-a821-4a71-a7d2-7e3e2aaea8d1 rook-ceph2

Let me know if you think this approach would be useful!

Yes, better we test that

Do I need to check the metadata.name field, or should I focus on the spec.mirroring.remoteNamespace field instead?"

@Madhu-1 @subhamkrai

spec.name if its set or else metadata.name https://github.com/rook/rook/blob/c5df2fe7214b51b1297b0250d2153970047c6d74/deploy/examples/radosnamespace.yaml#L4-L9

@Madhu-1 @subhamkrai done. I updated github.com/rook/rook/pkg/apis

subhamkrai · 2024-11-11T15:38:31Z

go.mod

@@ -1,16 +1,16 @@
 module github.com/rook/kubectl-rook-ceph

-go 1.22.5
+go 1.23


Suggested change

go 1.23

go 1.23

let''s not change this one for now, we want to keep what it is rook

subhamkrai · 2024-11-12T09:47:35Z

@Madhu-1 ready to review?

Madhu-1 · 2024-11-12T11:21:39Z

cmd/commands/rbd.go

+
+var listCmdRbd = &cobra.Command{
+	Use:   "ls",
+	Short: "Print the list of rbd volumes.",


Suggested change

Short: "Print the list of rbd volumes.",

Print the list of rbd images.

Madhu-1 · 2024-11-12T11:24:30Z

pkg/rbd/rbd.go

+	// Retrieve list of RBD images for each pool
+	cmd := "rbd"
+	for poolName := range blockPoolNames {
+		args := []string{"ls", "--pool=" + poolName}


i dont see we are passing --namespace here? Did we verify this by creating multiple namespaces and see if its working as expected?

this the relevant scenario?

$ kubectl get CephBlockPoolRadosNamespace -A NAMESPACE NAME PHASE BLOCKPOOL AGE rook-ceph namespace-a Ready replicapool 25m rook-ceph namespace-b Ready replicapool 78s

I dont see any output

bash-5.1$ rbd ls --pool=replicapool --namespace=namespace-b bash-5.1$ rbd ls --pool=replicapool --namespace=namespace-a bash-5.1$

bash-5.1$ ceph osd pool ls detail pool 1 '.mgr' replicated size 1 min_size 1 crush_rule 1 object_hash rjenkins pg_num 32 pgp_num 32 autoscale_mode on last_change 22 lfor 0/0/20 flags hashpspool stripe_width 0 application mgr read_balance_score 1.00 pool 2 'replicapool' replicated size 1 min_size 1 crush_rule 2 object_hash rjenkins pg_num 32 pgp_num 32 autoscale_mode on last_change 22 lfor 0/0/20 flags hashpspool,selfmanaged_snaps stripe_width 0 application rbd read_balance_score 1.00 bash-5.1$ ceph osd pool ls detail pool 1 '.mgr' replicated size 1 min_size 1 crush_rule 1 object_hash rjenkins pg_num 32 pgp_num 32 autoscale_mode on last_change 22 lfor 0/0/20 flags hashpspool stripe_width 0 application mgr read_balance_score 1.00 pool 2 'replicapool' replicated size 1 min_size 1 crush_rule 2 object_hash rjenkins pg_num 32 pgp_num 32 autoscale_mode on last_change 22 lfor 0/0/20 flags hashpspool,selfmanaged_snaps stripe_width 0 application rbd read_balance_score 1.00

I don't see namespace-a/b how to configure it?

In my code, I get the namespace via kubectl command and not ceph cli https://github.com/rook/kubectl-rook-ceph/pull/333/files#diff-b4c239a28ac425efccdf14790165fceaf4be580365efb6ccc72068a6800958f8R47

https://rook.io/docs/rook/latest/CRDs/Block-Storage/ceph-block-pool-rados-namespace-crd/#creating-a-storage-class use this to create images and pvc or you can use rbd cli to create images in the namespace. if we dont pass the --namespace to the RBD CLI we wont list images in the namespace

$ cat <<EOF | kubectl apply -f - apiVersion: ceph.rook.io/v1 kind: CephBlockPoolRadosNamespace metadata: name: namespace-a namespace: rook-ceph spec: blockPoolName: replicapool EOF cephblockpoolradosnamespace.ceph.rook.io/namespace-a created $ kubectl -n rook-ceph get cephblockpoolradosnamespace/namespace-a -o jsonpath='{.status.info.clusterID}' 80fc4f4bacc064be641633e6ed25ba7e $ cat <<EOF | kubectl apply -f - apiVersion: storage.k8s.io/v1 kind: StorageClass metadata: name: rook-ceph-block-rados-ns provisioner: rook-ceph.rbd.csi.ceph.com # csi-provisioner-name parameters: clusterID: 80fc4f4bacc064be641633e6ed25ba7e pool: replicapool EOF storageclass.storage.k8s.io/rook-ceph-block-rados-ns created $ cat <<EOF | kubectl apply -f - apiVersion: ceph.rook.io/v1 kind: CephBlockPool metadata: name: replicapool namespace: rook-ceph spec: replicated: size: 3 mirroring: enabled: true mode: image # schedule(s) of snapshot snapshotSchedules: - interval: 24h # daily snapshots startTime: 14:00:00-05:00 EOF cephblockpool.ceph.rook.io/replicapool created $ cat <<EOF | kubectl apply -f - apiVersion: ceph.rook.io/v1 kind: CephBlockPoolRadosNamespace metadata: name: namespace-a namespace: rook-ceph # namespace:cluster spec: # The name of the CephBlockPool CR where the namespace is created. blockPoolName: replicapool mirroring: mode: image remoteNamespace: namespace-a # default is the same as the local rados namespace # schedule(s) of snapshot snapshotSchedules: - interval: 24h # daily snapshots startTime: 14:00:00-05:00 EOF cephblockpoolradosnamespace.ceph.rook.io/namespace-a configured

Working with this procedure and CephBlockPoolRadosNamespace stuck in Progressing Phase and "rbd ls --pool=replicapool --namespace=namespace-a" cli stuck

$ kubectl describe CephBlockPoolRadosNamespace namespace-a -n rook-ceph Name: namespace-a Namespace: rook-ceph Labels: <none> Annotations: <none> API Version: ceph.rook.io/v1 Kind: CephBlockPoolRadosNamespace Metadata: Creation Timestamp: 2024-11-17T18:06:01Z Finalizers: cephblockpoolradosnamespace.ceph.rook.io Generation: 2 Resource Version: 2268 UID: 8078a253-2a23-4787-8b1e-097ba2bdf7c0 Spec: Block Pool Name: replicapool Mirroring: Mode: image Remote Namespace: namespace-a Snapshot Schedules: Interval: 24h Start Time: 14:00:00-05:00 Status: Info: Cluster ID: 80fc4f4bacc064be641633e6ed25ba7e Phase: Progressing Events: <none>

$ kubectl -n rook-ceph exec -it deploy/rook-ceph-tools -- bash bash-5.1$ rbd ls --pool=replicapool --namespace=namespace-a

Please create the pvc from the above storageclass (created for radosnamespace) and then list the images

and also you dont need to enable mirroring mode (like in above examples)

it is not working for me

cat <<EOF | kubectl apply -f - apiVersion: ceph.rook.io/v1 kind: CephBlockPoolRadosNamespace metadata: name: namespace-a namespace: rook-ceph spec: blockPoolName: replicapool EOF $ kubectl -n rook-ceph get cephblockpoolradosnamespace/namespace-a -o jsonpath='{.status.info.clusterID}' 80fc4f4bacc064be641633e6ed25ba7e cat <<EOF | kubectl apply -f - apiVersion: ceph.rook.io/v1 kind: CephBlockPool metadata: name: replicapool namespace: rook-ceph # namespace:cluster spec: failureDomain: osd replicated: size: 1 # Disallow setting pool with replica 1, this could lead to data loss without recovery. # Make sure you're *ABSOLUTELY CERTAIN* that is what you want requireSafeReplicaSize: false EOF cat <<EOF | kubectl apply -f - apiVersion: storage.k8s.io/v1 kind: StorageClass metadata: name: rook-ceph-block-rados-ns provisioner: rook-ceph.rbd.csi.ceph.com # csi-provisioner-name parameters: clusterID: 80fc4f4bacc064be641633e6ed25ba7e pool: replicapool EOF cat <<EOF | kubectl apply -f - apiVersion: v1 kind: PersistentVolumeClaim metadata: name: rbd-pvc spec: accessModes: - ReadWriteOnce resources: requests: storage: 1Gi storageClassName: rook-ceph-block-rados-ns EOF $ kubectl get pvc NAME STATUS VOLUME CAPACITY ACCESS MODES STORAGECLASS VOLUMEATTRIBUTESCLASS AGE rbd-pvc Pending rook-ceph-block-rados-ns <unset> 62s Type Reason Age From Message ---- ------ ---- ---- ------- Normal Provisioning 18s (x7 over 49s) rook-ceph.rbd.csi.ceph.com_csi-rbdplugin-provisioner-7c8b45d6-ghpk8_8c3f9dcf-a8d5-4cf4-b8fa-d7f00fd6e3d3 External provisioner is provisioning volume for claim "default/rbd-pvc" Warning ProvisioningFailed 18s (x7 over 49s) rook-ceph.rbd.csi.ceph.com_csi-rbdplugin-provisioner-7c8b45d6-ghpk8_8c3f9dcf-a8d5-4cf4-b8fa-d7f00fd6e3d3 failed to provision volume with StorageClass "rook-ceph-block-rados-ns": rpc error: code = InvalidArgument desc = provided secret is empty Normal ExternalProvisioning 6s (x5 over 49s) persistentvolume-controller Waiting for a volume to be created either by the external provisioner 'rook-ceph.rbd.csi.ceph.com' or manually by the system administrator. If volume creation is delayed, please verify that the provisioner is running and correctly registered.

Madhu-1 · 2024-11-12T11:25:01Z

pkg/rbd/rbd.go

+	writer := tabwriter.NewWriter(os.Stdout, 0, 0, 2, ' ', 0)
+	defer writer.Flush()
+	// Print header with column names
+	fmt.Fprintln(writer, "poolName\timageName\tnameSpace\t")


Suggested change

fmt.Fprintln(writer, "poolName\timageName\tnameSpace\t")

fmt.Fprintln(writer, "poolName\timageName\tnamespace\t")

Madhu-1 · 2024-11-12T11:25:31Z

pkg/rbd/rbd.go

+		list, err := exec.RunCommandInOperatorPod(ctx, clientsets, cmd, args, operatorNamespace, clusterNamespace, true)
+		if err == nil && len(list) > 0 {
+			blockPoolNames[poolName][0] = strings.ReplaceAll(list, "\n", "")
+		}


what happens if there is an error?

we use the default value -- https://github.com/rook/kubectl-rook-ceph/pull/333/files#diff-b4c239a28ac425efccdf14790165fceaf4be580365efb6ccc72068a6800958f8R43

This is not the expected behavior, user should be able to distinguish between the error or empty images. please return an error message in case of error

What is the expected output? For example, I encounter an error every time I use builtin-mgr

error(*fmt.wrapError) *{msg: "rbd: error opening pool 'builtin-mgr': (2) No such file or directory\nrbd: listing images failed: (2) No such file or directory\n. failed to run command. command terminated with exit code 2", err: error(*fmt.wrapError) *{msg: "failed to run command. command terminated with exit code 2", err: error(k8s.io/client-go/util/exec.CodeExitError) *(*error)(0xc000414fd0)}}``` ![image](https://github.com/user-attachments/assets/3154c5fb-3716-4835-92f7-2fa273da204d)

You need to skip special pools as its not meant for user to create the images.

Madhu-1 · 2024-11-12T11:26:37Z

pkg/rbd/rbd.go

+)
+
+// List retrieves and displays the Ceph block pools and their associated images.
+func ListRbdVolumes(ctx context.Context, clientsets *k8sutil.Clientsets, operatorNamespace string, clusterNamespace string) {


ListRbdVolumes to ListImagesthe packages name is alreadyrbd` we dont need to include rbd in the function name as well.

changed func name to ListVolumes

ListImages makes more sense as in RBD its images and we are listing images

This commit adds the ability to list RBD images. Signed-off-by: Oded Viner <[email protected]>

subhamkrai requested changes Oct 28, 2024

View reviewed changes

OdedViner force-pushed the add_ls_rbd branch 3 times, most recently from 52dad0f to 8744fff Compare October 28, 2024 14:19

OdedViner changed the title ~~Add rbd ls option~~ csi: add rbd ls option Oct 28, 2024

OdedViner force-pushed the add_ls_rbd branch 2 times, most recently from a696b11 to 0e5c199 Compare October 28, 2024 15:02

subhamkrai requested changes Oct 29, 2024

View reviewed changes

OdedViner force-pushed the add_ls_rbd branch 3 times, most recently from 73f2243 to fd23f52 Compare October 29, 2024 13:00

OdedViner requested a review from subhamkrai October 30, 2024 09:13

subhamkrai reviewed Nov 4, 2024

View reviewed changes

OdedViner force-pushed the add_ls_rbd branch 2 times, most recently from a6f54ac to 312966c Compare November 4, 2024 13:05

Madhu-1 reviewed Nov 4, 2024

View reviewed changes

OdedViner force-pushed the add_ls_rbd branch 4 times, most recently from 71428ee to bb7e341 Compare November 11, 2024 12:57

subhamkrai reviewed Nov 11, 2024

View reviewed changes

OdedViner force-pushed the add_ls_rbd branch from bb7e341 to 25326a0 Compare November 11, 2024 16:38

subhamkrai approved these changes Nov 12, 2024

View reviewed changes

subhamkrai requested a review from Madhu-1 November 12, 2024 09:47

Madhu-1 reviewed Nov 12, 2024

View reviewed changes

OdedViner force-pushed the add_ls_rbd branch 2 times, most recently from d917ad6 to 15050f9 Compare November 12, 2024 12:31

csi: add rbd ls option

e158753

This commit adds the ability to list RBD images. Signed-off-by: Oded Viner <[email protected]>

OdedViner force-pushed the add_ls_rbd branch from 15050f9 to e158753 Compare November 12, 2024 13:14

	Short: "Print the list of subvolumes.",
	Short: "Print the list of rbd volumes.",

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

	logging.Fatal(err)
	logging.Fatal(fmt.Errorf(err, "failed to list CephBlockPools"))

		fmt.Fprintln(writer, "poolName\timageName\t")
		fmt.Fprintln(writer, "--------\t---------\t")

	Short: "Print the list of rbd volumes.",
	Print the list of rbd images.

csi: add rbd ls option #333

Are you sure you want to change the base?

csi: add rbd ls option #333

Conversation

OdedViner commented Oct 27, 2024

subhamkrai left a comment

Choose a reason for hiding this comment

OdedViner commented Oct 28, 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

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

subhamkrai Nov 4, 2024 • edited Loading

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

subhamkrai commented Nov 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

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

OdedViner Nov 28, 2024 • edited Loading

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

subhamkrai Nov 4, 2024 •

edited

Loading

OdedViner Nov 28, 2024 •

edited

Loading