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

feat: add ListSnapshots RPC #36

Merged
merged 5 commits into from
Dec 4, 2024
Merged

feat: add ListSnapshots RPC #36

merged 5 commits into from
Dec 4, 2024

Conversation

Praveen005
Copy link
Contributor

@Praveen005 Praveen005 commented Nov 28, 2024

This PR implements the ListSnapshots RPC method as part of the CSI Driver's Snapshot & Restore functionality. It handle various cases for listing snapshots,

Case 1: Fetch by Snapshot ID

  1. Retrieves a specific snapshot by its ID.
  2. If the snapshot is not found(a successful search), returns an empty response instead of an error.
if strings.Contains(err.Error(), "DatabaseSnapshotNotFoundError") {
	return &csi.ListSnapshotsResponse{}, nil
}
  1. Returns an error only when, it encounters an internal error while retrieving the snapshot.
return nil, status.Errorf(codes.Internal, "failed to list snapshot %v: %v", snapshotID, err)

Case 2: Fetch by Source Volume ID

  1. Lists snapshots associated with a specific source volume.

Case 3: Fetch All Snapshots

  1. Retrieves all snapshots if no specific filters (snapshot ID or source volume ID) are provided.

The results are sorted and returned.
Reason: To make the order deterministic

Note: for now I have commented out some parts of the code, as it requires some implementations on the client side.

Copy link
Member

@hlts2 hlts2 left a comment

Choose a reason for hiding this comment

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

Thank you for the PR 👍
I think it is good overall!
I have made a few comments, please check them out. 🙏

pkg/driver/controller_server.go Outdated Show resolved Hide resolved
pkg/driver/controller_server.go Outdated Show resolved Hide resolved
pkg/driver/controller_server.go Outdated Show resolved Hide resolved
pkg/driver/controller_server.go Outdated Show resolved Hide resolved
pkg/driver/controller_server.go Outdated Show resolved Hide resolved
pkg/driver/controller_server.go Outdated Show resolved Hide resolved
pkg/driver/controller_server.go Outdated Show resolved Hide resolved
pkg/driver/controller_server.go Outdated Show resolved Hide resolved
@Praveen005 Praveen005 requested a review from hlts2 December 2, 2024 23:50
@Praveen005 Praveen005 requested a review from hlts2 December 3, 2024 06:16
Copy link
Member

@hlts2 hlts2 left a comment

Choose a reason for hiding this comment

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

Thank you! LGTM! 👍

@hlts2
Copy link
Member

hlts2 commented Dec 4, 2024

@Praveen005
I approved this PR 👍
I will merge this PR after all CI have passed 🚀

@hlts2 hlts2 merged commit 0a4e842 into master Dec 4, 2024
6 checks passed
@hlts2 hlts2 deleted the listSnapshotRPC branch December 4, 2024 08:21
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.

2 participants