Check if ListSnapshot is supported during GetSnapshotStatus #138

ggriffiths · 2019-07-10T23:49:01Z

What type of PR is this?
/kind bug

What this PR does / why we need it:
This PR introduces a check to see if ListSnapshot is supported before calling it GetSnapshotStatus.

If ListSnapshot is not supported, we just return that the snapshot is ReadyToUse and no creation time/restore time.

Which issue(s) this PR fixes:
Fixes #132

Special notes for your reviewer:
n/a

Does this PR introduce a user-facing change?:

Snapshotter will no longer call ListSnapshots if the CSI Driver does not support this operation.

k8s-ci-robot · 2019-07-10T23:49:02Z

Welcome @ggriffiths!

It looks like this is your first PR to kubernetes-csi/external-snapshotter 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-csi/external-snapshotter has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2019-07-10T23:49:14Z

Hi @ggriffiths. Thanks for your PR.

I'm waiting for a kubernetes-csi or kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

ggriffiths · 2019-07-10T23:50:33Z

/assign xing-yang

ggriffiths · 2019-07-10T23:52:27Z

pkg/snapshotter/snapshotter.go

 func (s *snapshot) GetSnapshotStatus(ctx context.Context, snapshotID string) (bool, time.Time, int64, error) {
 	client := csi.NewControllerClient(s.conn)

+	// If the driver does not support ListSnapshots, assume the snapshot ID is valid.
+	listSnapshotsSupported, err := s.isListSnapshotsSupported(ctx)
+	if err != nil {


We could also just return return true, time.Time{}, 0, nil here, but that seems like it could cause other problems.

Also, if the GetCapabilities call fails, I figured the caller should know.

I'm fine with this.

msau42 · 2019-07-10T23:56:30Z

/ok-to-test

xing-yang · 2019-07-11T00:29:40Z

Please check test failures.

xing-yang · 2019-07-11T01:25:29Z

I think we need a release note for this change.

ggriffiths · 2019-07-11T19:51:58Z

Please check test failures.

Fixed test failures and added a new test for when ListSnapshots is not supported.

xing-yang · 2019-07-11T20:20:42Z

@hakanmemisoglu Can you help test this fix? Thanks.

hakanmemisoglu · 2019-07-12T22:09:05Z

Tested with Ceph (RBD) CSI driver (v1.0.0).
It managed to bind previously created VolumeSnapshot and VolumeSnapshotContent.

xing-yang · 2019-07-15T01:56:58Z

Thanks @hakanmemisoglu!

xing-yang · 2019-07-15T01:57:07Z

/lgtm
/approve

k8s-ci-robot · 2019-07-15T01:57:09Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ggriffiths, xing-yang

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [xing-yang]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

1d77cfc Merge pull request kubernetes-csi#138 from pohly/kind-update-0.10 bff2fb7 prow.sh: KinD 0.10.0 git-subtree-dir: release-tools git-subtree-split: 1d77cfc

a1e1127 Merge pull request kubernetes-csi#139 from pohly/kind-for-kubernetes-latest 1c0fb09 prow.sh: use KinD main for latest Kubernetes 1d77cfc Merge pull request kubernetes-csi#138 from pohly/kind-update-0.10 bff2fb7 prow.sh: KinD 0.10.0 git-subtree-dir: release-tools git-subtree-split: a1e1127

bc0504a Merge pull request kubernetes-csi#140 from jsafrane/remove-unused-k8s-libs 5b1de1a go-get-kubernetes.sh: remove unused k8s libs 49b4269 Merge pull request kubernetes-csi#120 from pohly/add-kubernetes-release a1e1127 Merge pull request kubernetes-csi#139 from pohly/kind-for-kubernetes-latest 1c0fb09 prow.sh: use KinD main for latest Kubernetes 1d77cfc Merge pull request kubernetes-csi#138 from pohly/kind-update-0.10 bff2fb7 prow.sh: KinD 0.10.0 95eac33 Merge pull request kubernetes-csi#137 from pohly/fix-go-version-check 437e431 verify-go-version.sh: fix check after removal of travis.yml 1748b16 Merge pull request kubernetes-csi#136 from pohly/go-1.16 ec844ea remove travis.yml, Go 1.16 df76aba Merge pull request kubernetes-csi#134 from andyzhangx/add-build-arg e314a56 add build-arg ARCH for building multi-arch images, e.g. ARG ARCH FROM k8s.gcr.io/build-image/debian-base-${ARCH}:v2.1.3 7bc70e5 Merge pull request kubernetes-csi#129 from pohly/squash-documentation e0b02e7 README.md: document usage of --squash 316cb95 Merge pull request kubernetes-csi#132 from yiyang5055/bugfix/boilerplate 26e2ab1 fix: default boilerplate path 1add8c1 Merge pull request kubernetes-csi#133 from pohly/kubernetes-1.20-tag 3e811d6 prow.sh: fix "on-master" prow jobs 1d60e77 Merge pull request kubernetes-csi#131 from pohly/kubernetes-1.20-tag 9f10459 prow.sh: support building Kubernetes for a specific version f7e7ee4 docs: steps for adding testing against new Kubernetes release fe1f284 Merge pull request kubernetes-csi#121 from kvaps/namespace-check 8fdf0f7 Merge pull request kubernetes-csi#128 from fengzixu/master 1c94220 fix: fix a bug of csi-sanity a4c41e6 Merge pull request kubernetes-csi#127 from pohly/fix-boilerplate ece0f50 check namespace for snapshot-controller dbd8967 verify-boilerplate.sh: fix path to script 9289fd1 Merge pull request kubernetes-csi#125 from sachinkumarsingh092/optional-spelling-boilerplate-checks ad29307 Make the spelling and boilerplate checks optional 5f06d02 Merge pull request kubernetes-csi#124 from sachinkumarsingh092/fix-spellcheck-boilerplate-tests 48186eb Fix spelling and boilerplate errors 71690af Merge pull request kubernetes-csi#122 from sachinkumarsingh092/include-spellcheck-boilerplate-tests 981be3f Adding spelling and boilerplate checks. 2bb7525 Merge pull request kubernetes-csi#117 from fengzixu/master 4ab8b15 use the tag to replace commit of csi-test 5d74e45 change the csi-test import path to v4 7dcd0a9 upgrade csi-test to v4.0.2 git-subtree-dir: release-tools git-subtree-split: bc0504a

prow.sh: KinD 0.10.0

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/bug Categorizes issue or PR as related to a bug. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jul 10, 2019

k8s-ci-robot requested review from jingxu97 and saad-ali July 10, 2019 23:49

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 10, 2019

ggriffiths force-pushed the check_list_snapshots_supported branch from 375a81c to 9c9f046 Compare July 10, 2019 23:49

k8s-ci-robot assigned xing-yang Jul 10, 2019

ggriffiths commented Jul 10, 2019

View reviewed changes

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 10, 2019

ggriffiths force-pushed the check_list_snapshots_supported branch from 9c9f046 to efc4824 Compare July 11, 2019 19:22

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 11, 2019

ggriffiths force-pushed the check_list_snapshots_supported branch from efc4824 to 5db5f5d Compare July 11, 2019 19:23

Check if ListSnapshots is supported during GetSnapshotStatus

ea5d2d6

ggriffiths force-pushed the check_list_snapshots_supported branch from 5db5f5d to ea5d2d6 Compare July 11, 2019 19:24

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Jul 11, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 15, 2019

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 15, 2019

k8s-ci-robot merged commit 7ff6e81 into kubernetes-csi:master Jul 15, 2019

ggriffiths mentioned this pull request Oct 18, 2019

REQUEST: New membership for ggriffiths kubernetes/org#1322

Closed

6 tasks

ggriffiths mentioned this pull request Dec 11, 2019

REQUEST: New membership for ggriffiths kubernetes/org#1480

Closed

6 tasks

pohly mentioned this pull request Mar 23, 2021

master: update release-tools #485

Closed

pohly mentioned this pull request Mar 24, 2021

master: update release-tools #486

Merged

ggriffiths mentioned this pull request Apr 16, 2021

Add latest release-tools to release-4.0 branch #498

Merged

xing-yang pushed a commit to xing-yang/external-snapshotter that referenced this pull request Jul 26, 2021

Merge pull request kubernetes-csi#138 from pohly/kind-update-0.10

1d77cfc

prow.sh: KinD 0.10.0

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check if ListSnapshot is supported during GetSnapshotStatus #138

Check if ListSnapshot is supported during GetSnapshotStatus #138

ggriffiths commented Jul 10, 2019 •

edited

Loading

k8s-ci-robot commented Jul 10, 2019

k8s-ci-robot commented Jul 10, 2019

ggriffiths commented Jul 10, 2019

ggriffiths Jul 10, 2019

xing-yang Jul 11, 2019

msau42 commented Jul 10, 2019

xing-yang commented Jul 11, 2019

xing-yang commented Jul 11, 2019

ggriffiths commented Jul 11, 2019

xing-yang commented Jul 11, 2019

hakanmemisoglu commented Jul 12, 2019

xing-yang commented Jul 15, 2019

xing-yang commented Jul 15, 2019

k8s-ci-robot commented Jul 15, 2019

Check if ListSnapshot is supported during GetSnapshotStatus #138

Check if ListSnapshot is supported during GetSnapshotStatus #138

Conversation

ggriffiths commented Jul 10, 2019 • edited Loading

k8s-ci-robot commented Jul 10, 2019

k8s-ci-robot commented Jul 10, 2019

ggriffiths commented Jul 10, 2019

ggriffiths Jul 10, 2019

Choose a reason for hiding this comment

xing-yang Jul 11, 2019

Choose a reason for hiding this comment

msau42 commented Jul 10, 2019

xing-yang commented Jul 11, 2019

xing-yang commented Jul 11, 2019

ggriffiths commented Jul 11, 2019

xing-yang commented Jul 11, 2019

hakanmemisoglu commented Jul 12, 2019

xing-yang commented Jul 15, 2019

xing-yang commented Jul 15, 2019

k8s-ci-robot commented Jul 15, 2019

ggriffiths commented Jul 10, 2019 •

edited

Loading