Add VolumeAttachment Lister to CSI Provisioner

[RaunakShah]

This change prevents calling DeleteVolume on the CSI plugin for volumes that are still attached to a kubernetes node.

What type of PR is this?

/kind bug

What this PR does / why we need it:
Currently, external-provisioner does not check if a volume is attached to a node before trying to delete it.
This can happen when there's a race between Pod and PVC deletion (like deleting a ns).
NOTE: PVC protection does not apply here because the Pod is deleted from k8s, but the volume is still attached.

Based on discussion in kubernetes/kubernetes#84226, this change adds a volumeattachment lister to verify if a volume is still attached to a node before attempting to delete it.

Testing:

Deleting a volume that is not attached to a node works as expected:

{"log":"I0513 18:32:21.365637       1 controller.go:1422] delete \"pvc-80f3eaf0-e4f5-487a-a048-1e884a5ee13b\": started\n","stream":"stderr","time":"2020-05-13T18:32:21.372039744Z"}
{"log":"I0513 18:32:23.737215       1 controller.go:1450] delete \"pvc-80f3eaf0-e4f5-487a-a048-1e884a5ee13b\": volume deleted\n","stream":"stderr","time":"2020-05-13T18:32:23.739834928Z"}
{"log":"I0513 18:32:23.758050       1 controller.go:1496] delete \"pvc-80f3eaf0-e4f5-487a-a048-1e884a5ee13b\": persistentvolume deleted\n","stream":"stderr","time":"2020-05-13T18:32:23.758428374Z"}
{"log":"I0513 18:32:23.759194       1 controller.go:1498] delete \"pvc-80f3eaf0-e4f5-487a-a048-1e884a5ee13b\": succeeded\n","stream":"stderr","time":"2020-05-13T18:32:23.759331067Z"}

Deleting the namespace and creating a race between detach and delete:

{"log":"I0513 18:41:53.980570       1 controller.go:1422] delete \"pvc-fce3b92c-093f-4a74-86f7-1fc9cb6d3be0\": started\n","stream":"stderr","time":"2020-05-13T18:41:53.98129317Z"}
{"log":"E0513 18:41:53.986309       1 controller.go:1445] delete \"pvc-fce3b92c-093f-4a74-86f7-1fc9cb6d3be0\": volume deletion failed: persistentvolume pvc-fce3b92c-093f-4a74-86f7-1fc9cb6d3be0 is still attached to node k8s-node-0572\n","stream":"stderr","time":"2020-05-13T18:41:53.986566278Z"}
{"log":"W0513 18:41:53.986780       1 controller.go:965] Retrying syncing volume \"pvc-fce3b92c-093f-4a74-86f7-1fc9cb6d3be0\", failure 0\n","stream":"stderr","time":"2020-05-13T18:41:53.986890274Z"}
{"log":"E0513 18:41:53.987122       1 controller.go:983] error syncing volume \"pvc-fce3b92c-093f-4a74-86f7-1fc9cb6d3be0\": persistentvolume pvc-fce3b92c-093f-4a74-86f7-1fc9cb6d3be0 is still attached to node k8s-node-0572\n","stream":"stderr","time":"2020-05-13T18:41:53.987286288Z"}
{"log":"I0513 18:41:53.987990       1 event.go:281] Event(v1.ObjectReference{Kind:\"PersistentVolume\", Namespace:\"\", Name:\"pvc-fce3b92c-093f-4a74-86f7-1fc9cb6d3be0\", UID:\"79fa52d2-ccdd-49cf-a3ed-e5eb081528af\", APIVersion:\"v1\", ResourceVersion:\"5383\", FieldPath:\"\"}): type: 'Warning' reason: 'VolumeFailedDelete' persistentvolume pvc-fce3b92c-093f-4a74-86f7-1fc9cb6d3be0 is still attached to node k8s-node-0572\n","stream":"stderr","time":"2020-05-13T18:41:53.988209321Z"}
...
{"log":"I0513 18:42:01.006230       1 controller.go:1422] delete \"pvc-fce3b92c-093f-4a74-86f7-1fc9cb6d3be0\": started\n","stream":"stderr","time":"2020-05-13T18:42:01.007199938Z"}
{"log":"I0513 18:42:03.968729       1 controller.go:1450] delete \"pvc-fce3b92c-093f-4a74-86f7-1fc9cb6d3be0\": volume deleted\n","stream":"stderr","time":"2020-05-13T18:42:03.969879653Z"}
{"log":"I0513 18:42:04.025931       1 controller.go:1496] delete \"pvc-fce3b92c-093f-4a74-86f7-1fc9cb6d3be0\": persistentvolume deleted\n","stream":"stderr","time":"2020-05-13T18:42:04.02633573Z"}
{"log":"I0513 18:42:04.025978       1 controller.go:1498] delete \"pvc-fce3b92c-093f-4a74-86f7-1fc9cb6d3be0\": succeeded\n","stream":"stderr","time":"2020-05-13T18:42:04.026375078Z"}

Which issue(s) this PR fixes:

Fixes kubernetes/kubernetes#84226

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

ACTION REQUIRED: Add VolumeAttachment Lister to  prevent calling DeleteVolume on the CSI plugin for volumes that are still attached to a kubernetes node. New RBAC rules for listing VolumeAttachment objects are required.

[k8s-ci-robot]

Welcome @RaunakShah!

It looks like this is your first PR to kubernetes-csi/external-provisioner 🎉. 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-provisioner 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]

Hi @RaunakShah. 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.

[xing-yang]

/ok-to-test

[xing-yang]

/assign @xing-yang

[RaunakShah]

/retest

[msau42]

cc @jsafrane

[xing-yang]

I see a few e2e tests failed complaining about PVs not deleted. We probably need to fix the e2e tests first.

[RaunakShah]

Seeing if the e2e failure was intermittent or because of this change.
/retest

[xing-yang]

/lgtm

[SandeepPissay]

VolumeAttachment objects will be in thousands in a scaled environment. Listing all VA objects in every Delete callback is quite expensive. Moreover, delete gets invoked continuously until the volume is attached thereby causing further degradation. Can we avoid such expensive call to API server?

[jsafrane]

vaLister lists VolumeAttachments from in-memory informer cache. "Thousands" should be ok, "millions" could be problematic.

[SandeepPissay]

Does informer limit how many objects are in the cache? How are we sure that all VolumeAttachment objects will fit into memory? If the lister ends up hitting the API server then this will introduce a performance regression, hence I'm a bit concerned with this code.

[jsafrane]

/lgtm

[jsafrane]

/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jsafrane, RaunakShah

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 [jsafrane]

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

[pohly]

I am seeing tests failures in PMEM-CSI with the new external-provisioner 2.0.0 because of "still attached to node" in a situation where NodeUnpublishVolume and NodeUnstageVolume have completed successfully, i.e. the volume shouldn't be attached anymore. This goes on for 5 minutes until the test gives up and redeploys the driver, which involves restarting the external-provisioner. The new instance then does try to delete. I wonder whether the informer can have an outdated VolumeAttachment object for over five minutes?

[pohly]

I see a few e2e tests failed complaining about PVs not deleted. We probably need to fix the e2e tests first.

Is that problem truly gone? The PMEM-CSI tests are passing locally, but not in the CI, which implies that it depends a bit on timing.

[pohly]

Shouldn't this code be checking that va.Status.Attached is true?

As it stands, it checks for "is there a VolumeAttachment object for the PV", which is a bit broader than "is the PV attached". I don't know whether that makes a difference.

[RaunakShah]

Removed that check based on @msau42 comment - #438 (comment)
IIRC the precedent was another comment in another PR that the existence of volumeattachment was enough to fail the operation..

[pohly]

I think I understand now why: a ControllerPublishVolume call might have failed, in which case the call must be retried or ControllerUnpublishVolume needs to be called (according to the CSI spec). So it's correct to prevent DeleteVolume even when the volume is not really attached.