rbd: add support for rbd_diff_iterate3 api #1064

Rakshith-R · 2025-01-29T09:41:08Z

This commit adds support for rbd_diff_iterate3
through DiffIterateByID() which performs
diff interate using snapshot id instead of
snapshot name.

This is useful in cases such as when rbd snapshot is in trash or rbd(group) snapshot in in group namespace.

refer:

https://tracker.ceph.com/issues/65720

Related ceph PR: ceph/ceph#60844

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.

Rakshith-R · 2025-01-29T09:43:41Z

hey,

Please take a look at the function name and the way it is defined.
I have yet to add tests.
I'll add them after we have finalized the structure of the call.

The new api will be added in this pr https://github.com/ceph/ceph/pull/60844/files#diff-b4983d99df0348718f7db5606f7ee3ca00fb0e7a87b4a69740f7ada6dd283856

@nixpanic @ansiwen @phlogistonjohn @anoopcs9

Thanks,

rbd/diff_iterate_by_id.go

phlogistonjohn · 2025-02-05T15:41:41Z

@Rakshith-R please take a look at the test failures as they seem related to the change. Thanks!

Rakshith-R · 2025-02-07T06:56:34Z

@Rakshith-R please take a look at the test failures as they seem related to the change. Thanks!

Yes, the tests are expected to fail for now.
The rbd_diff_iterate3 call needs to be merged upstream ceph first in ceph/ceph#60844 .
I'll rebase the pr once ceph's pr going in.

I Just added the pr early so I can get feedback on naming conventions and format.

phlogistonjohn · 2025-02-07T13:20:29Z

OK, as a general request - not specifically to you but to all contributors that are adding APIs to go-ceph that do not have a functioning versions in ceph - please make it very clear up front that the work is not complete in ceph. (Edit: a sentence or two in the descrition is good, even better if it includes links to Issues, PRs or design docs)

In addition, I would like to see PRs of this sort be in draft status and also consider using the ceph-libs label. Draft tells us that the PR is not in a mergable state - it does not mean that we won't discuss it or provide feedback (although explicit requests for feedback are welcome on draft prs). the ceph-libs label, if you look at the description, indicates that something is needed on the ceph side.

Today, I'll make these changes to the PR after I submit this comment. In the future please consider these suggestions for PRs like this that can't be merged because work in ceph is still pending. Thanks!

Rakshith-R · 2025-02-10T07:09:56Z

OK, as a general request - not specifically to you but to all contributors that are adding APIs to go-ceph that do not have a functioning versions in ceph - please make it very clear up front that the work is not complete in ceph. (Edit: a sentence or two in the descrition is good, even better if it includes links to Issues, PRs or design docs)

In addition, I would like to see PRs of this sort be in draft status and also consider using the ceph-libs label. Draft tells us that the PR is not in a mergable state - it does not mean that we won't discuss it or provide feedback (although explicit requests for feedback are welcome on draft prs). the ceph-libs label, if you look at the description, indicates that something is needed on the ceph side.

Today, I'll make these changes to the PR after I submit this comment. In the future please consider these suggestions for PRs like this that can't be merged because work in ceph is still pending. Thanks!

Noted, I'll keep these points in mind.

mergify · 2025-02-17T13:23:05Z

This pull request now has conflicts with the target branch. Could you please resolve conflicts and force push the corrected changes? 🙏

rbd/diff_iterate_by_id.go

dpulls · 2025-02-27T15:17:41Z

⚠️ Dpulls not installed on repository ceph/ceph. Checkout our quickstart for how to install.

Rakshith-R · 2025-03-05T15:32:30Z

Hey @phlogistonjohn @nixpanic @idryomov ,

The ceph pr has now been merged ceph/ceph#60844 .

I have updated code to include the new flags format.
Please review the new function flow.

The tests passes successfully on main branch
but will fail on release branches with

# github.com/ceph/go-ceph/rbd
# [github.com/ceph/go-ceph/rbd]
rbd/diff_iterate_by_id.go:115:12: could not determine what C.RBD_DIFF_ITERATE_FLAG_INCLUDE_PARENT refers to
rbd/diff_iterate_by_id.go:118:12: could not determine what C.RBD_DIFF_ITERATE_FLAG_WHOLE_OBJECT refers to

I am not sure how to handle fetching these enums without breaking previous release branches.
Please let me know what can be done to remedy this situation 😅

Thanks

docs/api-status.json

rbd/diff_iterate_by_id.go

phlogistonjohn · 2025-03-05T19:11:59Z

Hey @phlogistonjohn @nixpanic @idryomov ,

The ceph pr has now been merged ceph/ceph#60844 .

I have updated code to include the new flags format. Please review the new function flow.

The tests passes successfully on main branch but will fail on release branches with
# github.com/ceph/go-ceph/rbd
# [github.com/ceph/go-ceph/rbd]
rbd/diff_iterate_by_id.go:115:12: could not determine what C.RBD_DIFF_ITERATE_FLAG_INCLUDE_PARENT refers to
rbd/diff_iterate_by_id.go:118:12: could not determine what C.RBD_DIFF_ITERATE_FLAG_WHOLE_OBJECT refers to
I am not sure how to handle fetching these enums without breaking previous release branches. Please let me know what can be done to remedy this situation 😅

Thanks

Right, obviously that's not something we can accept in the current form. Normally, I would be advocating using build tags here, but you're using the new dlsym stuff. I think this is a significant downside to the dlsym appraoch as things like enums and macros don't have symbols to look up. I'm trying to avoid the urge to re-argue the dlsym stuff and just brainstorm a possible way forward.

I see the values you are looking for are enums, which if memory serves won't be something we can if-def. There is a #define LIBRBD_SUPPORTS_DIFF_ITERATE3 1 that you could try keying off of in the embedded C code. IOW (mostly untested pseudo code!):

#ifdef LIBRBD_SUPPORTS_DIFF_ITERATE3
#define _RBD_DIFF_ITERATE_FLAG_INCLUDE_PARENT RBD_DIFF_ITERATE_FLAG_INCLUDE_PARENT
#define _RBD_DIFF_ITERATE_FLAG_WHOLE_OBJECT RBD_DIFF_ITERATE_FLAG_WHOLE_OBJECT
#else
#define _RBD_DIFF_ITERATE_FLAG_INCLUDE_PARENT 0
#define _RBD_DIFF_ITERATE_FLAG_WHOLE_OBJECT 0
#endif

Another, simpler approach would be to simply copy-paste the values from the C header into consts in the Go code. Less elegant perhaps but maybe more robust and understandable. If you do go that route please put a comment explaining the reason for the copy-paste.

Neither of these future proof you for the case of new values appearing in the enum but I'll answer that in the more appropriate location.

Rakshith-R · 2025-03-10T11:16:06Z

Rebased on top of latest master branch
@phlogistonjohn Can you please re review the pr ?
Thanks

dpulls · 2025-03-13T13:40:21Z

⚠️ Dpulls not installed on repository ceph/ceph. Checkout our quickstart for how to install.

dpulls · 2025-03-13T13:40:26Z

⚠️ Dpulls not installed on repository ceph/ceph. Checkout our quickstart for how to install.

dpulls · 2025-03-13T13:40:27Z

⚠️ Dpulls not installed on repository ceph/ceph. Checkout our quickstart for how to install.

dpulls · 2025-03-13T13:40:44Z

⚠️ Dpulls not installed on repository ceph/ceph. Checkout our quickstart for how to install.

dpulls · 2025-03-13T13:40:45Z

⚠️ Dpulls not installed on repository ceph/ceph. Checkout our quickstart for how to install.

phlogistonjohn · 2025-03-13T13:42:05Z

I confirmed that the checklist items are taken care of. In the future, please don't just ignore the checklist - it helps me know that each item was considered and either followed or we had a discussion about it.

phlogistonjohn

looks ok to me.

2nd reviewer please - @anoopcs9 @ansiwen

rbd/diff_iterate_by_id.go

This commit adds support for rbd_diff_iterate3 through DiffIterateByID() which performs diff interate using snapshot id instead of snapshot name. This is useful in cases such as when rbd snapshot is in trash or rbd(group) snapshot in in group namespace. refer: - https://tracker.ceph.com/issues/65720 Signed-off-by: Rakshith R <rar@redhat.com>

Pull request has been modified.

anoopcs9

lgtm, thanks.

ansiwen

LGTM

ansiwen · 2025-03-19T11:45:35Z

@Mergifyio rebase

mergify · 2025-03-19T11:45:39Z

rebase

☑️ Nothing to do

any of:
- #commits > 1 [📌 rebase requirement]
- #commits-behind > 0 [📌 rebase requirement]
- -linear-history [📌 rebase requirement]
-closed [📌 rebase requirement]
-conflict [📌 rebase requirement]
queue-position = -1 [📌 rebase requirement]

phlogistonjohn · 2025-03-19T11:51:56Z

@Mergifyio refresh

mergify · 2025-03-19T11:51:59Z

refresh

✅ Pull request refreshed

ansiwen · 2025-03-19T11:58:57Z

@Mergifyio refresh

@phlogistonjohn I just was researching for this command, but when I came back you were quicker... ;-)

phlogistonjohn · 2025-03-19T13:00:25Z

FWIW it was not just that command I also had to edit the description to remove Depends-on. Mergify rules showed that it thought that the PR dependend on the other PR - and it could not get that PRs status. The Rules checklist is visible by getting the details for the 'mergify' task

Rakshith-R mentioned this pull request Jan 29, 2025

librbd: add rbd_diff_iterate3() API to take source snapshot by ID ceph/ceph#60844

Merged

14 tasks

idryomov reviewed Jan 29, 2025

View reviewed changes

rbd/diff_iterate_by_id.go Outdated Show resolved Hide resolved

Rakshith-R force-pushed the snap-id-diff branch from 888be53 to 402c41b Compare January 29, 2025 10:12

phlogistonjohn marked this pull request as draft February 7, 2025 13:21

phlogistonjohn added ceph-libs Fix or enhancement needed in ceph libraries API This PR includes a change to the public API of a go-ceph package labels Feb 7, 2025

nixpanic reviewed Feb 17, 2025

View reviewed changes

rbd/diff_iterate_by_id.go Outdated Show resolved Hide resolved

Rakshith-R force-pushed the snap-id-diff branch 7 times, most recently from 46ecb45 to 11cba54 Compare February 26, 2025 08:41

Rakshith-R force-pushed the snap-id-diff branch 2 times, most recently from 1df220a to fd4df17 Compare March 5, 2025 15:28

Rakshith-R marked this pull request as ready for review March 5, 2025 15:29

idryomov reviewed Mar 5, 2025

View reviewed changes

Rakshith-R force-pushed the snap-id-diff branch 3 times, most recently from b4d4626 to 6f64f75 Compare March 6, 2025 09:14

Rakshith-R force-pushed the snap-id-diff branch 2 times, most recently from fdb6f8d to 0924902 Compare March 10, 2025 11:15

phlogistonjohn previously approved these changes Mar 13, 2025

View reviewed changes

phlogistonjohn requested review from anoopcs9 and ansiwen March 13, 2025 13:48

anoopcs9 requested changes Mar 17, 2025

View reviewed changes

rbd/diff_iterate_by_id.go Outdated Show resolved Hide resolved

rbd/diff_iterate_by_id.go Outdated Show resolved Hide resolved

Rakshith-R requested a review from anoopcs9 March 17, 2025 10:30

anoopcs9 requested changes Mar 17, 2025

View reviewed changes

rbd/diff_iterate_by_id.go Outdated Show resolved Hide resolved

rbd/diff_iterate_by_id.go Outdated Show resolved Hide resolved

Rakshith-R force-pushed the snap-id-diff branch from 0924902 to 7a572b7 Compare March 18, 2025 08:18

Rakshith-R force-pushed the snap-id-diff branch from 7a572b7 to b5e1ce4 Compare March 18, 2025 08:18

Rakshith-R requested a review from anoopcs9 March 18, 2025 09:33

anoopcs9 approved these changes Mar 18, 2025

View reviewed changes

anoopcs9 requested a review from phlogistonjohn March 18, 2025 09:47

ansiwen approved these changes Mar 19, 2025

View reviewed changes

mergify bot merged commit d6b48f9 into ceph:master Mar 19, 2025
15 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rbd: add support for rbd_diff_iterate3 api #1064

rbd: add support for rbd_diff_iterate3 api #1064

Rakshith-R commented Jan 29, 2025 •

edited by phlogistonjohn

Loading

Rakshith-R commented Jan 29, 2025

phlogistonjohn commented Feb 5, 2025

Rakshith-R commented Feb 7, 2025

phlogistonjohn commented Feb 7, 2025 •

edited

Loading

Rakshith-R commented Feb 10, 2025

mergify bot commented Feb 17, 2025

dpulls bot commented Feb 27, 2025

Rakshith-R commented Mar 5, 2025

phlogistonjohn commented Mar 5, 2025

Rakshith-R commented Mar 10, 2025

dpulls bot commented Mar 13, 2025

dpulls bot commented Mar 13, 2025

dpulls bot commented Mar 13, 2025

dpulls bot commented Mar 13, 2025

dpulls bot commented Mar 13, 2025

phlogistonjohn commented Mar 13, 2025

phlogistonjohn left a comment

anoopcs9 left a comment

ansiwen left a comment

ansiwen commented Mar 19, 2025

mergify bot commented Mar 19, 2025

phlogistonjohn commented Mar 19, 2025

mergify bot commented Mar 19, 2025

ansiwen commented Mar 19, 2025

phlogistonjohn commented Mar 19, 2025

rbd: add support for rbd_diff_iterate3 api #1064

rbd: add support for rbd_diff_iterate3 api #1064

Conversation

Rakshith-R commented Jan 29, 2025 • edited by phlogistonjohn Loading

Checklist

Rakshith-R commented Jan 29, 2025

phlogistonjohn commented Feb 5, 2025

Rakshith-R commented Feb 7, 2025

phlogistonjohn commented Feb 7, 2025 • edited Loading

Rakshith-R commented Feb 10, 2025

mergify bot commented Feb 17, 2025

dpulls bot commented Feb 27, 2025

Rakshith-R commented Mar 5, 2025

phlogistonjohn commented Mar 5, 2025

Rakshith-R commented Mar 10, 2025

dpulls bot commented Mar 13, 2025

dpulls bot commented Mar 13, 2025

dpulls bot commented Mar 13, 2025

dpulls bot commented Mar 13, 2025

dpulls bot commented Mar 13, 2025

phlogistonjohn commented Mar 13, 2025

phlogistonjohn left a comment

Choose a reason for hiding this comment

anoopcs9 left a comment

Choose a reason for hiding this comment

ansiwen left a comment

Choose a reason for hiding this comment

ansiwen commented Mar 19, 2025

mergify bot commented Mar 19, 2025

☑️ Nothing to do

phlogistonjohn commented Mar 19, 2025

mergify bot commented Mar 19, 2025

✅ Pull request refreshed

ansiwen commented Mar 19, 2025

phlogistonjohn commented Mar 19, 2025

Rakshith-R commented Jan 29, 2025 •

edited by phlogistonjohn

Loading

phlogistonjohn commented Feb 7, 2025 •

edited

Loading