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

Fix ListSnapshots paging #300

Merged
merged 1 commit into from
Apr 22, 2020
Merged

Fix ListSnapshots paging #300

merged 1 commit into from
Apr 22, 2020

Conversation

timoreimann
Copy link
Collaborator

@timoreimann timoreimann commented Apr 20, 2020

This changes provides the following fixes and improvements to ListSnapshots:

  • Handle StartingToken and MaxEntries such that we use paging efficiently and skip initial, unneeded snapshots.
  • Extend fake snapshot driver to support paging.
  • Add tests.

Note that Kubernetes / the csi-snapshotter sidecar currently do not invoke ListSnapshots without the snapshot ID parameter, which means that the fixed code is not executed in production. However, it is used by csi-test / the sanity package, and other COs (Container Orchestrators) may potentially use it as well as Kubernetes going forward.

@timoreimann timoreimann force-pushed the fix-listsnapshots-paging branch 2 times, most recently from 2608e1a to e2bd611 Compare April 21, 2020 09:28
Copy link
Contributor

@nicktate nicktate left a comment

Choose a reason for hiding this comment

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

Nice work on this, lgtm overall. The edge case of remainders between pagination sizes makes this unfortunately complex but I like the solution you provided. 💯

@nicktate nicktate dismissed a stale review April 21, 2020 21:44

invalid

This changes provides the following fixes and improvements to
ListSnapshots:

- Use paging to collect snapshots beyond the first page. Previously, we
  would only return snapshots from the first page.
- Handle StartingToken and MaxEntries such that we use paging
  efficiently and skip initial, unneeded snapshots.
- Extend fake snapshot driver to support paging.
- Add tests.

Note that Kubernetes / the csi-snapshotter sidecar currently do not
invoke ListSnapshots without the snapshot ID parameter, which means that
the fixed code is not executed in production. However, it is used by
csi-test / the sanity package, and other COs (Container Orchestrators)
may potentially use it as well as Kubernetes going forward.
@timoreimann timoreimann merged commit 3eb1779 into master Apr 22, 2020
@timoreimann timoreimann deleted the fix-listsnapshots-paging branch April 22, 2020 05:19
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