Skip to content

cephfs: add support for cephfs snapdiff api #1186

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.

Sign up for GitHub

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

Jump to bottom
Merged
mergify merged 1 commit into from
Oct 21, 2025
Merged

cephfs: add support for cephfs snapdiff api #1186

mergify merged 1 commit into from
Oct 21, 2025

Conversation

@Rakshith-R
Copy link
Contributor

@Rakshith-R Rakshith-R commented Oct 10, 2025 •
edited by anoopcs9
Loading

This commit adds support for cephfs snap diff
APIs which gives a list of changed files between
two snapshots.

Refer:

fixes #1087

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
Copy link
Contributor Author

Rakshith-R commented Oct 10, 2025

/cc @ansiwen @phlogistonjohn @nixpanic @anoopcs9
PTAL

@phlogistonjohn phlogistonjohn added the API This PR includes a change to the public API of a go-ceph package label Oct 10, 2025
phlogistonjohn
phlogistonjohn requested changes Oct 10, 2025
View reviewed changes
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.

Pretty interesting, thanks!
I have a few thoughts and thoughts about how to make the API a bit more idiomatic.

@Rakshith-R Rakshith-R force-pushed the cephfs-diff branch 6 times, most recently from 97cc2d3 to b5d8835 Compare October 13, 2025 09:16
@Rakshith-R
Copy link
Contributor Author

Rakshith-R commented Oct 13, 2025

Pretty interesting, thanks! I have a few thoughts and thoughts about how to make the API a bit more idiomatic.

Thanks for the quick review !
I've made changes as requested. PTAL.

@Rakshith-R
Copy link
Contributor Author

Rakshith-R commented Oct 13, 2025

Rebased on top of latest branch commits

phlogistonjohn
phlogistonjohn requested changes Oct 13, 2025
View reviewed changes
@Rakshith-R Rakshith-R force-pushed the cephfs-diff branch 2 times, most recently from b45a8bb to 6dc77ae Compare October 13, 2025 15:49
@Rakshith-R
Copy link
Contributor Author

Rakshith-R commented Oct 13, 2025

If you are going to document each field, put the comment above each field item. For example: https://tip.golang.org/doc/comment (search for per-field comments).

modified it.

I have documented all the fields, it just needs two snapshot names to diff between without any order. That's why its just named snap1 and snap2.
This explanation is what should be in the doc comment!

added a line in doc comment for this :)

PTAL

phlogistonjohn
phlogistonjohn requested changes Oct 13, 2025
View reviewed changes
phlogistonjohn
phlogistonjohn reviewed Oct 13, 2025
View reviewed changes
anoopcs9
anoopcs9 requested changes Oct 14, 2025
View reviewed changes
Copy link
Collaborator

@anoopcs9 anoopcs9 left a comment

Choose a reason for hiding this comment

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

Please fix the commit message rbd: add support for cephfs snapdiff api and PR title to replace rbd with cephfs.

@Rakshith-R Rakshith-R force-pushed the cephfs-diff branch 3 times, most recently from aa838cb to 1a83013 Compare October 14, 2025 10:06
@Rakshith-R
Copy link
Contributor Author

Rakshith-R commented Oct 14, 2025

Please fix the commit message rbd: add support for cephfs snapdiff api and PR title to replace rbd with cephfs.

Fixed this and renamed functions as suggested(ceph prefix is indeed redundant ) 👍 :)
PTAL again.

@anoopcs9 anoopcs9 changed the title rbd: add support for cephfs snapdiff api cephfs: add support for cephfs snapdiff api Oct 14, 2025
anoopcs9
anoopcs9 reviewed Oct 14, 2025
View reviewed changes
@Rakshith-R Rakshith-R force-pushed the cephfs-diff branch 3 times, most recently from 0a166e6 to d0dff26 Compare October 14, 2025 13:28
@Rakshith-R Rakshith-R requested a review from anoopcs9 October 14, 2025 13:32
@Rakshith-R Rakshith-R force-pushed the cephfs-diff branch 2 times, most recently from d0d30bd to c087686 Compare October 14, 2025 14:25
@Rakshith-R
Copy link
Contributor Author

Rakshith-R commented Oct 14, 2025

rebased on top of master branch

anoopcs9
anoopcs9 previously approved these changes Oct 14, 2025
View reviewed changes
Copy link
Collaborator

@anoopcs9 anoopcs9 left a comment

Choose a reason for hiding this comment

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

lgtm, thanks.

@mergify mergify bot dismissed anoopcs9’s stale review October 14, 2025 15:09

Pull request has been modified.

@Rakshith-R
Copy link
Contributor Author

Rakshith-R commented Oct 14, 2025

Well, we also have path.Join 😉.

I fixed this too now 😅
rebased it again on top of master too.

Please provide ✅ again

anoopcs9
anoopcs9 previously approved these changes Oct 14, 2025
View reviewed changes
@Rakshith-R
Copy link
Contributor Author

Rakshith-R commented Oct 17, 2025

Hey @phlogistonjohn ,

Can you please review this pr again ?
Thanks

nixpanic
nixpanic reviewed Oct 17, 2025
View reviewed changes
Copy link
Member

@nixpanic nixpanic left a comment

Choose a reason for hiding this comment

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

just a few nits to improve stability and maintenance a little

cephfs/snap_diff.go Outdated
func OpenSnapDiff(config SnapDiffConfig) (*SnapDiffInfo, error) {
if config.CMount == nil || config.RootPath == "" || config.RelPath == "" ||
config.Snap1 == "" || config.Snap2 == "" {
return nil, fmt.Errorf("invalid arguments")
Copy link
Member

@nixpanic nixpanic Oct 17, 2025

Choose a reason for hiding this comment

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

consider returning errInvalid

@Rakshith-R
cephfs: add support for cephfs snapdiff api
937a849 
This commit adds support for cephfs snap diff
APIs which gives a list of changed files between
two snapshots.

Signed-off-by: Rakshith R <rar@redhat.com>
@mergify mergify bot dismissed anoopcs9’s stale review October 17, 2025 11:01

Pull request has been modified.

@Rakshith-R
Copy link
Contributor Author

Rakshith-R commented Oct 17, 2025

just a few nits to improve stability and maintenance a little

done, ptal.

anoopcs9
anoopcs9 approved these changes Oct 17, 2025
View reviewed changes
@Rakshith-R
Copy link
Contributor Author

Rakshith-R commented Oct 21, 2025 •
edited
Loading

Hey @phlogistonjohn ,

Can you please review this pr again ? Thanks

The pr is revised, and rebased.
PTAL again
😅

phlogistonjohn
phlogistonjohn approved these changes Oct 21, 2025
View reviewed changes
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.

lgtm, thanks

@mergify mergify bot added the queued label Oct 21, 2025
@mergify mergify bot merged commit cdc15f4 into ceph:master Oct 21, 2025
16 checks passed
@mergify mergify bot removed the queued label Oct 21, 2025
@Rakshith-R Rakshith-R mentioned this pull request Nov 13, 2025
Merged
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@anoopcs9 anoopcs9 anoopcs9 approved these changes

@phlogistonjohn phlogistonjohn phlogistonjohn approved these changes

@nixpanic nixpanic Awaiting requested review from nixpanic

Assignees

No one assigned

Labels

API This PR includes a change to the public API of a go-ceph package

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

support cephfs snapdiffs

4 participants

@Rakshith-R @nixpanic @anoopcs9 @phlogistonjohn