-
Notifications
You must be signed in to change notification settings - Fork 271
rbd: add ListChildrenAttributes Api #1079
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
Conversation
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.
Tiny nit regarding function definition, everything else looks good to me.
corresponding cephcsi issue to include this new API: ceph/ceph-csi#5173
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.
Overall, this looks like a very good start. Most of my suggestions are related to the naming conventions we currently use.
c608509
to
7c165db
Compare
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.
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
7c165db
to
1bb42ac
Compare
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.
LGTM, thanks for this, it was a really nice PR!
@anoopcs9 @phlogistonjohn |
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.
@anoopcs9 @phlogistonjohn Would the API Label be required on this PR?
Initially I was wondering why there wasn't any ceph_preview
build tag added to the source files which led me to the realization that the underlying librbd API was not new. Anyway "API" label is needed for automatic merge once required number of approvals are present.
Yeah, I forgot the label. As a reminder: the API vs. non-API label stuff is advisory not a hard rule. Some stuff is obvious like a Makefile fix would be non-API, this is API but like you note it's not adding something net new and the underlying ceph API is supported by all ceph versions that we test. As I already approved it is probably clear I didn't think the preview api status was necessary. It reuses an existing type and an existing api. If I had a time machine it would have been the correct way to implment ListChildren in the first place. But we can discuss it more if you feel strongly about it. |
1bb42ac
to
1812b0c
Compare
Pull request has been modified.
1812b0c
to
041c79a
Compare
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.
Thanks, LGTM
041c79a
to
bf093fa
Compare
@Mergifyio rebase |
Api retrieves Image,Pool and Trash details of the existing Children. Signed-off-by: ShravaniVangur <[email protected]>
✅ Branch has been successfully rebased |
bf093fa
to
188c942
Compare
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.
lgtm, thanks.
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: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
//go:build ceph_preview
make api-update
to record new APIsNew 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.