Improve ListSnapshot filtering tests #259

timoreimann · 2020-04-17T10:00:58Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

We provide tests on ListSnapshots for filtering by snapshot IDs and source volume IDs. However, the tests do not cover the case were snapshot listing functionality exists in general but is missing
filtering capabilities: for this scenario the existing tests yield false positives because the fixtures set up a single snapshot only.

We improve the tests by creating a number of snapshots of which a subset is expected to be found.

Does this PR introduce a user-facing change?:

NONE

k8s-ci-robot · 2020-04-17T10:01:06Z

Hi @timoreimann. Thanks for your PR.

I'm waiting for a kubernetes-csi 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.

Our implementation of ListSnapshots currently returns all snapshots. The request parameter also provides fields to filter by snapshot ID and source volume ID, which we have been ignoring so far. Based on discussions with SIG Storage [1], it became clear that CSI drivers are expected to process present IDs in the ListSnapshots call appropriately. While the spec marks them as OPTIONAL, it turned out that the optionality refers to the CO, not the SP. (A request for clarification has been filed in [2].) As soon as any of IDs are provided, drivers are expected to take them into account. In fact, csi-snapshotter always passes a snapshot ID in and only ever takes the first snapshot item from the list of snapshots returned, which means that our current implementation is buggy for cases where the account holds more than one snapshot (at least on Kubernetes; other COs could potentially behave differently). This change fixes the issue by retrieving the snapshot by direct lookup if a snapshot ID is given. If a source volume ID is given, we do additional filtering by only returning the snapshots that were sourced by the same volume. The existing csi-sanity tests did not catch this bug. A PR to that project was submitted [3] to close the gap. Once it is merged, we should upgrade our version of the test package. [1]: https://kubernetes.slack.com/archives/C8EJ01Z46/p1587038433091900 [2]: container-storage-interface/spec#426 [3]: kubernetes-csi/csi-test#259

xing-yang · 2020-04-17T12:09:15Z

pkg/sanity/controller.go

-		volReq := MakeCreateVolumeReq(sc, "listSnapshots-volume-1")
-		volume, err := c.CreateVolume(context.Background(), volReq)
+		By("creating volumes")
+		volReq := MakeCreateVolumeReq(sc, "listSnapshots-volume-unrelated1")


What does "unrelated" mean? Please add a comment.

xing-yang · 2020-04-17T12:10:05Z

pkg/sanity/controller.go

-		By("creating a snapshot")
-		snapshotReq := MakeCreateSnapshotReq(sc, "listSnapshots-snapshot-1", volume.GetVolume().GetVolumeId())
-		snapshot, err := c.CreateSnapshot(context.Background(), snapshotReq)
+		volReq = MakeCreateVolumeReq(sc, "listSnapshots-volume-target")


Add a comment

xing-yang · 2020-04-17T12:10:22Z

pkg/sanity/controller.go

+		volumeTarget, err := c.CreateVolume(context.Background(), volReq)
+		Expect(err).NotTo(HaveOccurred())
+
+		volReq = MakeCreateVolumeReq(sc, "listSnapshots-volume-unrelated2")


Add comment

xing-yang · 2020-04-17T12:11:08Z

pkg/sanity/controller.go

+		Expect(err).NotTo(HaveOccurred())
+
+		By("creating snapshots")
+		snapshotReq := MakeCreateSnapshotReq(sc, "listSnapshots-snapshot-unrelated1", volumeUnrelated1.GetVolume().GetVolumeId())


Can you move this up to right after create volume.

xing-yang · 2020-04-17T12:11:22Z

pkg/sanity/controller.go

 		Expect(err).NotTo(HaveOccurred())

+		snapshotReq = MakeCreateSnapshotReq(sc, "listSnapshots-snapshot-unrelated2", volumeUnrelated2.GetVolume().GetVolumeId())


xing-yang · 2020-04-17T12:14:34Z

/ok-to-test

timoreimann · 2020-04-17T13:10:26Z

@xing-yang I addressed the comments.

However, the "should return snapshots that match the specified source volume id" test still fails, apparently because the (mock) driver used in the tests does not support multiple snapshots sourced by the same volume ID. I'm trying to understand if the driver needs to be extended or the test reduced.

xing-yang · 2020-04-17T13:15:30Z

However, the "should return snapshots that match the specified source volume id" test still fails, apparently because the (mock) driver used in the tests does not support multiple snapshots sourced by the same volume ID. I'm trying to understand if the driver needs to be extended or the test reduced.

Then the mock driver should be modified to handle that test.

timoreimann · 2020-04-17T13:22:28Z

I simplified the test for now just to see if it passes before I modify the mock driver. Looking into that next.

xing-yang · 2020-04-17T13:50:53Z

pkg/sanity/controller.go

@@ -1340,35 +1340,71 @@ var _ = DescribeSanity("ListSnapshots [Controller Server]", func(sc *TestContext
 	})

 	It("should return snapshots that match the specified snapshot id", func() {
+		// The test creates three snapshots: one that we intend to find by
+		// snapshot ID, and two unrelated ones that must not be returned by


When you say "unrelated", do you mean they won't be returned when filtering by snapshot id or filtering by source volume id? Please clarify in the comments.

As this is the test for snapshot IDs, the "unrelatedness" is with regards to filtering by snapshot IDs. I'll amend the comments.

timoreimann · 2020-04-17T15:21:15Z

@xing-yang I addressed your last comment and brought back the version of the source volume ID test that expects multiple snapshots to be returned. The mock was updated accordingly. I also did a few drive-by improvements in the mock for things I needed to touch anymore. (See the commit description for details.)

Please take another look.

timoreimann · 2020-04-17T16:00:45Z

Uh, I don't understand this: make test runs flawlessly for me locally but in the CI it fails. :(

timoreimann · 2020-04-17T16:03:27Z

I don't think this is flakiness, but anyway:

/retest

We provide tests on ListSnapshots for filtering by snapshot IDs and source volume IDs. However, the tests do not cover the case were snapshot listing functionality exists in general but is missing filtering capabilities: for this scenario the tests yield false positives because the fixtures set up a single snapshot only. We improve the tests by creating a number of snapshots of which a subset is expected to be found.

timoreimann · 2020-04-20T05:43:46Z

I think I understand now why the tests kept failing: I missed to see that the CI runs the tests both against the mocked as all as the hostpath driver, and the latter wasn't updated yet.

I briefly looked at what would need to be changed about the hostpath driver to return multiple snapshots. While the necessary changes are pretty clear, at this point I'd definitely favor getting the PR approved and merged in the simplified version that doesn't require changing any CSI drivers. (I updated the PR to reflect this accordingly.) I'd happy to create an issue to track the extension and work on it afterwards.

@xing-yang what do you think, does that sound like a reasonable approach to you?

xing-yang · 2020-04-20T14:45:27Z

I think I understand now why the tests kept failing: I missed to see that the CI runs the tests both against the mocked as all as the hostpath driver, and the latter wasn't updated yet.

I briefly looked at what would need to be changed about the hostpath driver to return multiple snapshots. While the necessary changes are pretty clear, at this point I'd definitely favor getting the PR approved and merged in the simplified version that doesn't require changing any CSI drivers. (I updated the PR to reflect this accordingly.) I'd happy to create an issue to track the extension and work on it afterwards.

@xing-yang what do you think, does that sound like a reasonable approach to you?

@timoreimann sounds good to me.

xing-yang · 2020-04-20T14:51:25Z

The tests look good to me.
Can you clean up the comments in "Special notes for your reviewer" section?

timoreimann · 2020-04-20T16:11:29Z

@xing-yang 👍 done

xing-yang · 2020-04-20T16:28:07Z

/lgtm

timoreimann · 2020-04-21T22:42:50Z

This one still needs an /approve I suppose?

xing-yang · 2020-04-22T01:02:05Z

/assign @pohly

pohly · 2020-04-22T09:34:33Z

pkg/sanity/controller.go

+		By("creating first unrelated snapshot")
+		// Create volume source and afterwards the first unrelated snapshot.
+		volReq := MakeCreateVolumeReq(sc, "listSnapshots-volume-unrelated1")
+		volumeUnrelated1, err := c.CreateVolume(context.Background(), volReq)


As pointed out elsewhere (issue #260), all created resources like volumes must be registered with the cl instance, otherwise they can leak in case of an assertion failure.

Not sure whether you want to address that already in this PR or do it separately - up to you.

If you don't mind, I'd prefer to merge this one as-is and do the proper registration for resource cleanup in a follow-up PR. As noted in #260, I'd be happy to work on that.

pohly

/approve

k8s-ci-robot · 2020-04-22T10:24:37Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pohly, timoreimann

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~~ [pohly]

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

90efb2c Merge pull request kubernetes-csi#272 from andyzhangx/patch-3 9b616fe Bump golang to 1.23.6 to fix CVE-2024-45336, CVE-2025-22866 0496593 Merge pull request kubernetes-csi#268 from huww98/cloudbuild 119aee1 Merge pull request kubernetes-csi#266 from jsafrane/bump-sanity-5.3.1 0ae5e52 Update cloudbuild image with go 1.21+ 406a79a Merge pull request kubernetes-csi#267 from huww98/gomodcache 9cec273 Set GOMODCACHE to avoid re-download toolchain 98f2307 Merge pull request kubernetes-csi#260 from TerryHowe/update-csi-driver-version e9d8712 Merge pull request kubernetes-csi#259 from stmcginnis/deprecated-kind-kube-root faf79ff Remove --kube-root deprecated kind argument 43bde06 Bump csi-sanity to 5.3.1 18b6ac6 chore: update CSI driver version to 1.15 git-subtree-dir: release-tools git-subtree-split: 90efb2ca59900f19eba05e65da28beda79c5bb28

k8s-ci-robot added the release-note-none Denotes a PR that doesn't merit a release note. label Apr 17, 2020

k8s-ci-robot requested review from jsafrane and msau42 April 17, 2020 10:01

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Apr 17, 2020

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 17, 2020

timoreimann mentioned this pull request Apr 17, 2020

Support filtering snapshots by IDs digitalocean/csi-digitalocean#299

Merged

xing-yang reviewed Apr 17, 2020

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 Apr 17, 2020

xing-yang reviewed Apr 17, 2020

View reviewed changes

timoreimann force-pushed the improve-ListSnapshots-filtering-tests branch 3 times, most recently from a2f85df to 14a3965 Compare April 19, 2020 08:52

timoreimann force-pushed the improve-ListSnapshots-filtering-tests branch from 14a3965 to abd502a Compare April 19, 2020 21:49

k8s-ci-robot assigned xing-yang Apr 20, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 20, 2020

k8s-ci-robot assigned pohly Apr 22, 2020

pohly reviewed Apr 22, 2020

View reviewed changes

pohly approved these changes Apr 22, 2020

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 22, 2020

k8s-ci-robot merged commit b91f254 into kubernetes-csi:master Apr 22, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve ListSnapshot filtering tests #259

Improve ListSnapshot filtering tests #259

timoreimann commented Apr 17, 2020 •

edited

Loading

k8s-ci-robot commented Apr 17, 2020

xing-yang Apr 17, 2020

xing-yang Apr 17, 2020

xing-yang Apr 17, 2020

xing-yang Apr 17, 2020

xing-yang Apr 17, 2020

xing-yang Apr 17, 2020

xing-yang commented Apr 17, 2020

timoreimann commented Apr 17, 2020 •

edited

Loading

xing-yang commented Apr 17, 2020

timoreimann commented Apr 17, 2020

xing-yang Apr 17, 2020

timoreimann Apr 17, 2020 •

edited

Loading

timoreimann commented Apr 17, 2020 •

edited

Loading

timoreimann commented Apr 17, 2020

timoreimann commented Apr 17, 2020

timoreimann commented Apr 20, 2020 •

edited

Loading

xing-yang commented Apr 20, 2020

xing-yang commented Apr 20, 2020

timoreimann commented Apr 20, 2020

xing-yang commented Apr 20, 2020

timoreimann commented Apr 21, 2020

xing-yang commented Apr 22, 2020

pohly Apr 22, 2020

timoreimann Apr 22, 2020

pohly left a comment

k8s-ci-robot commented Apr 22, 2020

		Expect(err).NotTo(HaveOccurred())

		snapshotReq = MakeCreateSnapshotReq(sc, "listSnapshots-snapshot-unrelated2", volumeUnrelated2.GetVolume().GetVolumeId())

Improve ListSnapshot filtering tests #259

Improve ListSnapshot filtering tests #259

Conversation

timoreimann commented Apr 17, 2020 • edited Loading

k8s-ci-robot commented Apr 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xing-yang commented Apr 17, 2020

timoreimann commented Apr 17, 2020 • edited Loading

xing-yang commented Apr 17, 2020

timoreimann commented Apr 17, 2020

Choose a reason for hiding this comment

timoreimann Apr 17, 2020 • edited Loading

Choose a reason for hiding this comment

timoreimann commented Apr 17, 2020 • edited Loading

timoreimann commented Apr 17, 2020

timoreimann commented Apr 17, 2020

timoreimann commented Apr 20, 2020 • edited Loading

xing-yang commented Apr 20, 2020

xing-yang commented Apr 20, 2020

timoreimann commented Apr 20, 2020

xing-yang commented Apr 20, 2020

timoreimann commented Apr 21, 2020

xing-yang commented Apr 22, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pohly left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Apr 22, 2020

timoreimann commented Apr 17, 2020 •

edited

Loading

timoreimann commented Apr 17, 2020 •

edited

Loading

timoreimann Apr 17, 2020 •

edited

Loading

timoreimann commented Apr 17, 2020 •

edited

Loading

timoreimann commented Apr 20, 2020 •

edited

Loading