Skip to content

Add GetSnapshot RPC #586

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

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

Conversation

xing-yang
Copy link
Contributor

What type of PR is this?
feature

What this PR does / why we need it:
This PR adds a GetSnapshot RPC

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce an API-breaking change?:

Adds GetSnapshot RPC.

Copy link
Contributor

@bswartz bswartz left a comment

Choose a reason for hiding this comment

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

/lgtm

Copy link

@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.

Looks good to me, thanks! You probably want to squash the commits into one before it gets merged?

@xing-yang
Copy link
Contributor Author

Squashed the commits.

nixpanic added a commit to nixpanic/ceph-csi that referenced this pull request Apr 28, 2025
Copy link

@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.

Thanks!

It has obviously been an omission, and I can imaging COs would want to use this. Can you leave a link in the comments where this new procedure would be used?

@bswartz
Copy link
Contributor

bswartz commented Apr 28, 2025

Thanks!

It has obviously been an omission, and I can imaging COs would want to use this. Can you leave a link in the comments where this new procedure would be used?

It's expected that statically provisioned group snapshots would use this API to populate information about the individual members of the group snapshot. Today we don't have a good way of getting the necessary information in that case. One hack is to rely on ListSnapshots with a snapshot ID passed in, but many drivers don't support that, due to other challenges related to snapshot listing.

@jdef
Copy link
Member

jdef commented Apr 28, 2025 via email

@xing-yang
Copy link
Contributor Author

Here is the KEP: https://github.com/kubernetes/enhancements/pull/5013/files
ListSnapshots API works as well, however, many storage vendors could not support list snapshots due to the performance overhead.
Usually storage vendors do not have a problem supporting GetSnapshot if they want to support volume snapshot or volume group snapshot.

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.

5 participants