Skip to content
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

rbd: Adding ListChildrenWithParams Api #1079

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ShravaniVangur
Copy link

@ShravaniVangur ShravaniVangur commented Feb 24, 2025

The existing ListChildren() API provides only basic information, specifically the pool and image names of a given image’s children. However, in many scenarios, additional metadata is required to fully understand the relationships between images.

This new API extends functionality by leveraging rbd_list_children3(rbd_image_t image, rbd_linked_image_spec_t *images, size_t *max_images), which enables the retrieval of comprehensive details, including:

  • Image and pool IDs
  • Image and pool names
  • Pool namespace
  • The trash status of child images

By exposing this additional metadata, the API allows for more informed decision-making when managing child images. Furthermore, modifying the existing ListChildren() API would introduce backward compatibility issues, potentially breaking existing implementations. To avoid such disruptions while still providing enhanced capabilities, this new API has been introduced as a separate function.

Checklist

  • Added tests for features and functional changes
  • Public functions and types are documented
  • Standard formatting is applied to Go code
  • Is this a new API? Added a new file that begins with //go:build ceph_preview
  • Ran make api-update to record new APIs

New or infrequent contributors may want to review the go-ceph Developer's Guide including the section on how we track API Status and the API Stability Plan.

The go-ceph project uses mergify. View the mergify command guide for information on how to interact with mergify. Add a comment with @Mergifyio rebase to rebase your PR when github indicates that the PR is out of date with the base branch.

Copy link
Contributor

@Rakshith-R Rakshith-R left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tiny nit regarding function definition, everything else looks good to me.

corresponding cephcsi issue to include this new API: ceph/ceph-csi#5173

/cc @nixpanic @phlogistonjohn @anoopcs9 @ansiwen

Copy link
Collaborator

@phlogistonjohn phlogistonjohn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, this looks like a very good start. Most of my suggestions are related to the naming conventions we currently use.

//
// int rbd_list_children3(rbd_image_t image, rbd_linked_image_spec_t *images,
// size_t *max_images);
func (image *Image) ListChildrenWithParams() (imgSpec []ImageSpec, err error) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In go-ceph Params usually refers to function parameters (aka arguments). And so calling the function ListChildrenWithParams is a bit confusing. I'd suggest a name more like ListChildrenWithDetails or ListChildrenSpecs or something along those lines to indicate the return value has more information than ListChildren.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ListChildrenWithParams has been renamed to ListChildrenAttributes. Hope this works.

@@ -0,0 +1,188 @@
package rbd
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm trying to discourage the use of file names that include ceph versions/codenames. I won't complain about adding the function to the existing file with that kind of file name but this file is new. So, I suggest renaming it to something like list_children_test.go or something based on what you end up renaming the function to.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noted. Changed the file name to reflect the function with the test cases.

Api retrieves Image,Pool and Trash details of the existing Children.

Signed-off-by: ShravaniVangur <[email protected]>
Copy link
Collaborator

@phlogistonjohn phlogistonjohn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good to me now.
However, you need to update the commit message to match the new name. Also since I'm asking you to change the commit messge title: it should be 'add' not 'adding' to give the commit title an imperative tone

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants