-
Notifications
You must be signed in to change notification settings - Fork 13
Enhance rbd ls Command to List Images Without Requiring an Image Name #117
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
cmd/odf/ceph/rbd.go
Outdated
// If the user is trying to run "rbd ls" (without possible extra arguments) | ||
if args[0] == "ls" && len(args) == 1 { | ||
if len(args) > 1 { | ||
logging.Warning("Ignoring extra arguments for 'rbd ls'; running only 'rbd ls'.") | ||
} | ||
// Call the vendor ListImages function to do the listing | ||
rbd.ListImages(cmd.Context(), root.ClientSets, root.OperatorNamespace, root.StorageClusterNamespace) | ||
return | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// If the user is trying to run "rbd ls" (without possible extra arguments) | |
if args[0] == "ls" && len(args) == 1 { | |
if len(args) > 1 { | |
logging.Warning("Ignoring extra arguments for 'rbd ls'; running only 'rbd ls'.") | |
} | |
// Call the vendor ListImages function to do the listing | |
rbd.ListImages(cmd.Context(), root.ClientSets, root.OperatorNamespace, root.StorageClusterNamespace) | |
return | |
} | |
// If the user is trying to run "rbd ls" (without possible extra arguments) | |
if args[0] == "ls" && len(args) == 1 { | |
if len(args) > 1 { | |
logging.Warning("Ignoring extra arguments for 'rbd ls'; running only 'rbd ls'.") | |
} | |
// Call the vendor ListImages function to do the listing | |
rbd.ListImages(cmd.Context(), root.ClientSets, root.OperatorNamespace, root.StorageClusterNamespace) | |
return | |
} |
instead of this can we do similar to https://github.com/rook/kubectl-rook-ceph/pull/333/files#diff-9b7d33b239e9bb3a80d5cda418273877bb1398a5a0f928fc478ef98b37901c5dR44-R55 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@subhamkrai I changed the code based on the upstream kubectl-rook-ceph
project, but I didn't add an option to list images for a specific pool only rbd ls
. Do you want me to add rbd ls <pool name>
?
9ca7a0e
to
f884809
Compare
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: OdedViner The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
cmd/odf/ceph/rbd.go
Outdated
Run: func(cmd *cobra.Command, args []string) { | ||
logging.Info("running 'rbd' command with args: %v", args) | ||
// verify operator pod is running | ||
_, err := k8sutil.WaitForPodToRun(cmd.Context(), root.ClientSets.Kube, root.OperatorNamespace, "app=rook-ceph-operator") | ||
if err != nil { | ||
logging.Fatal(err) | ||
} | ||
} | ||
|
||
_, err = exec.RunCommandInOperatorPod(cmd.Context(), root.ClientSets, cmd.Use, args, root.OperatorNamespace, root.StorageClusterNamespace, false) | ||
if err != nil { | ||
logging.Fatal(err) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We still need these for running ceph rbd commands
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@subhamkrai I called a function to verify that the rook-ceph-operator
pod is running.
My question is whether we want to support only rbd ls
or both rbd ls
and rbd ls <pool-name>
.
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's support what is it in upstream kubectl-rook-ceph is supporting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@subhamkrai
Upstream kubectl-rook-ceph supports only rbs ls
and does not support rbd ls <pool-name>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are you sure @OdedViner I think it will support as rbd ls <poolname>
is rbd ceph command. can you verify that?
Signed-off-by: Oded Viner <[email protected]>
f884809
to
063ef00
Compare
This looks useful, but on a real system we can have large number of images and namespaces and listing everything may be too much.
Changing existing command semantics make the command harder to use for people that already know it. It seems that the right place to make |
This PR introduces support for running the
rbd ls
command without having to specify an image name.When a user executes odf
rbd ls
with no additional arguments, the command now directly invokes the vendor’s ListImages function (rook-kubectl upstream) to display a detailed table listing of available images (including pool name, image name, and namespace).If an image name (or extra arguments) is provided (e.g., odf rbd ls ), the command continues to run in the operator pod as before, ensuring backward compatibility while improving the user experience for simple image listing.