Support filtering snapshots by IDs #299
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -772,93 +772,119 @@ func (d *Driver) DeleteSnapshot(ctx context.Context, req *csi.DeleteSnapshotRequ | |
| // ListSnapshots shold not list a snapshot that is being created but has not | ||
| // been cut successfully yet. | ||
| func (d *Driver) ListSnapshots(ctx context.Context, req *csi.ListSnapshotsRequest) (*csi.ListSnapshotsResponse, error) { | ||
| listResp := &csi.ListSnapshotsResponse{} | ||
| log := d.log.WithFields(logrus.Fields{ | ||
| "snapshot_id": req.SnapshotId, | ||
| "source_volume_id": req.SourceVolumeId, | ||
| "req_starting_token": req.StartingToken, | ||
| "method": "list_snapshots", | ||
| }) | ||
| log.Info("list snapshots is called") | ||
|
|
||
| if req.SnapshotId != "" { | ||
| snapshot, resp, err := d.snapshots.Get(ctx, req.SnapshotId) | ||
| if err != nil { | ||
| if resp == nil || resp.StatusCode != http.StatusNotFound { | ||
| return nil, status.Errorf(codes.Internal, "failed to get snapshot by ID %s: %s", req.SnapshotId, err) | ||
| } | ||
| } else { | ||
| snap, err := toCSISnapshot(snapshot) | ||
| if err != nil { | ||
| return nil, status.Errorf(codes.Internal, | ||
| "failed to convert DO snapshot to CSI snapshot: %s", err) | ||
| } | ||
| listResp = &csi.ListSnapshotsResponse{ | ||
| Entries: []*csi.ListSnapshotsResponse_Entry{ | ||
| { | ||
| Snapshot: snap, | ||
| }, | ||
| }, | ||
| } | ||
| } | ||
| } else { | ||
| // Pagination in the CSI world works different than at DO. CSI sends the | ||
| // `req.MaxEntries` to indicate how much snapshots it wants. The | ||
| // req.StartingToken is returned by us, if we somehow need to indicate that | ||
| // we couldn't fetch and need to fetch again. But it's NOT the page number. | ||
| // I.e: suppose CSI wants us to fetch 50 entries, we only fetch 30, we need to | ||
| // return NextToken as 31 (so req.StartingToken will be set to 31 when CSI | ||
| // calls us again), to indicate that we want to continue returning from the | ||
| // index 31 up to 50. | ||
|
|
||
| var nextToken int | ||
| var err error | ||
| if req.StartingToken != "" { | ||
| nextToken, err = strconv.Atoi(req.StartingToken) | ||
| if err != nil { | ||
| return nil, status.Errorf(codes.Aborted, "ListSnapshots starting token %s is not valid : %s", | ||
| req.StartingToken, err.Error()) | ||
| } | ||
| } | ||
|
|
||
| if nextToken != 0 && req.MaxEntries != 0 { | ||
| return nil, status.Errorf(codes.Aborted, | ||
| "ListSnapshots invalid arguments starting token: %d and max entries: %d can't be non null at the same time", nextToken, req.MaxEntries) | ||
| } | ||
|
|
||
| // fetch all entries | ||
| listOpts := &godo.ListOptions{ | ||
| PerPage: int(req.MaxEntries), | ||
| } | ||
| var snapshots []godo.Snapshot | ||
| for { | ||
| snaps, resp, err := d.snapshots.ListVolume(ctx, listOpts) | ||
|
There was a problem hiding this comment. I'm a little confused with this pagination section below. It seems like we paginate through every entry from the start, ignoring the offset of There was a problem hiding this comment. I think you're right, the logic looks off. I did not pay much attention to it because existing code was just moved into an If you don't mind then I'd like to address this issue in a separate PR. FWIW, the flawed pagination should not have impacted users much since csi-snapshotter always passes in a snapshot ID in the There was a problem hiding this comment. Fix submitted in #300. |
||
| if err != nil { | ||
| return nil, status.Errorf(codes.Aborted, "ListSnapshots listing volume snapshots has failed: %s", err.Error()) | ||
| } | ||
|
|
||
| snapshots = append(snapshots, snaps...) | ||
|
|
||
| if resp.Links == nil || resp.Links.IsLastPage() { | ||
| break | ||
| } | ||
|
|
||
| page, err := resp.Links.CurrentPage() | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| listOpts.Page = page + 1 | ||
| listOpts.PerPage = len(snaps) | ||
| } | ||
|
|
||
| if nextToken > len(snapshots) { | ||
| return nil, status.Error(codes.Aborted, "ListSnapshots starting token is greater than total number of snapshots") | ||
| } | ||
|
|
||
| if nextToken != 0 { | ||
| snapshots = snapshots[nextToken:] | ||
| } | ||
|
|
||
| if req.MaxEntries != 0 { | ||
| nextToken = len(snapshots) - int(req.MaxEntries) - 1 | ||
| snapshots = snapshots[:req.MaxEntries] | ||
| } | ||
|
|
||
| entries := make([]*csi.ListSnapshotsResponse_Entry, 0, len(snapshots)) | ||
| for _, snapshot := range snapshots { | ||
| snap, err := toCSISnapshot(&snapshot) | ||
| if err != nil { | ||
| return nil, status.Errorf(codes.Internal, | ||
| "failed to convert DO snapshot to CSI snapshot: %s", err) | ||
| } | ||
|
|
||
| entries = append(entries, &csi.ListSnapshotsResponse_Entry{ | ||
| Snapshot: snap, | ||
| }) | ||
| } | ||
| listResp = &csi.ListSnapshotsResponse{ | ||
| Entries: entries, | ||
| NextToken: strconv.Itoa(nextToken), | ||
| } | ||
| } | ||
|
|
||
| filterSnapshotEntriesForVolumeID(listResp, req.SourceVolumeId) | ||
|
|
||
| log.WithField("response", listResp).Info("snapshots listed") | ||
| return listResp, nil | ||
| } | ||
|
|
@@ -1176,3 +1202,17 @@ func (d *Driver) tagVolume(parentCtx context.Context, vol *godo.Volume) error { | |
| _, err = d.tags.TagResources(ctx, d.doTag, tagReq) | ||
| return err | ||
| } | ||
|
|
||
| func filterSnapshotEntriesForVolumeID(listResp *csi.ListSnapshotsResponse, sourceVolumeID string) { | ||
| if sourceVolumeID == "" { | ||
| return | ||
| } | ||
|
|
||
| var filteredEntries []*csi.ListSnapshotsResponse_Entry | ||
| for _, entry := range listResp.Entries { | ||
| if entry.Snapshot.SourceVolumeId == sourceVolumeID { | ||
| filteredEntries = append(filteredEntries, entry) | ||
| } | ||
| } | ||
| listResp.Entries = filteredEntries | ||
| } | ||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Why can't you have an offset coupled with the page count in this instance?