Repository: kubernetes-csi/external-provisioner
Pull request #438 (2020-05-13 20:54:26)

Add VolumeAttachment Lister to CSI Provisioner
This change prevents calling DeleteVolume on the CSI plugin for volumes that are still attached to a kubernetes node.

<!--  Thanks for sending a pull request!  Here are some tips for you:

1. If this is your first time, please read our contributor guidelines: https://git.k8s.io/community/contributors/guide#your-first-contribution and developer guide https://git.k8s.io/community/contributors/devel/development.md#development-guide
2. Follow the instructions for writing a release note: https://git.k8s.io/community/contributors/guide/release-notes.md
3. If the PR is unfinished, see how to mark it: https://git.k8s.io/community/contributors/guide/pull-requests.md#marking-unfinished-pull-requests
-->

**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 https://github.com/kubernetes/kubernetes/issues/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**:
<!--
*Automatically closes linked issue when PR is merged.
Usage: `Fixes #<issue number>`, or `Fixes (paste link of issue)`.
_If PR is about `failing-tests or flakes`, please post the related issues/tests in a comment and do not use `Fixes`_*
-->
Fixes https://github.com/kubernetes/kubernetes/issues/84226

**Special notes for your reviewer**:

**Does this PR introduce a user-facing change?**:
<!--
If no, just write "NONE" in the release-note block below.
If yes, a release note is required:
Enter your extended release note in the block below. If the PR requires additional action from users switching to the new release, include the string "action required".
-->
```release-note
Add VolumeAttachment Lister to  prevent calling DeleteVolume on the CSI plugin for volumes that are still attached to a kubernetes node.
```


k8s-ci-robot: Hi @RaunakShah. Thanks for your PR.

I'm waiting for a [kubernetes-csi](https://github.com/orgs/kubernetes-csi/people) 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](https://git.k8s.io/community/community-membership.md#member) 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](https://go.k8s.io/bot-commands?repo=kubernetes-csi%2Fexternal-provisioner).

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
</details>

k8s-ci-robot: Welcome @RaunakShah! <br><br>It looks like this is your first PR to <a href='https://github.com/kubernetes-csi/external-provisioner'>kubernetes-csi/external-provisioner</a> 🎉. Please refer to our [pull request process documentation](https://git.k8s.io/community/contributors/guide/pull-requests.md) to help your PR have a smooth ride to approval. <br><br>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](https://go.k8s.io/bot-commands). <br><br>You can also check if kubernetes-csi/external-provisioner has [its own contribution guidelines](https://github.com/kubernetes-csi/external-provisioner/tree/master/CONTRIBUTING.md). <br><br>You may want to refer to our [testing guide](https://git.k8s.io/community/contributors/devel/sig-testing/testing.md) if you run into trouble with your tests not passing. <br><br>If you are having difficulty getting your pull request seen, please follow the [recommended escalation practices](https://github.com/kubernetes/community/blob/master/contributors/guide/pull-requests.md#why-is-my-pull-request-not-getting-reviewed). Also, for tips and tricks in the contribution process you may want to read the [Kubernetes contributor cheat sheet](https://git.k8s.io/community/contributors/guide/contributor-cheatsheet/README.md). We want to make sure your contribution gets all the attention it needs! <br><br>Thank you, and welcome to Kubernetes. :smiley:
k8s-ci-robot: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by: *<a href="https://github.com/kubernetes-csi/external-provisioner/pull/438#" title="Author self-approved">RaunakShah</a>*
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **msau42**
You can assign the PR to them by writing `/assign @msau42` in a comment when ready.

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=kubernetes-csi%2Fexternal-provisioner).

<details open>
Needs approval from an approver in each of these files:

- **[OWNERS](https://github.com/kubernetes-csi/external-provisioner/blob/master/OWNERS)**

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":["msau42"]} -->
xing-yang: /ok-to-test
xing-yang: /assign @xing-yang 
k8s-ci-robot: @RaunakShah: The following test **failed**, say `/retest` to rerun all failed tests:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
pull-kubernetes-csi-external-provisioner-unit | 2d1965fe1f452dd7d546db33d94df780ba4d55f6 | [link](https://prow.k8s.io/view/gcs/kubernetes-jenkins/pr-logs/pull/kubernetes-csi_external-provisioner/438/pull-kubernetes-csi-external-provisioner-unit/1260681743933902849) | `/test pull-kubernetes-csi-external-provisioner-unit`

[Full PR test history](https://prow.k8s.io/pr-history?org=kubernetes-csi&repo=external-provisioner&pr=438). [Your PR dashboard](https://prow.k8s.io/pr?query=is%3Apr%20state%3Aopen%20author%3RaunakShah). Please help us cut down on flakes by [linking to](https://git.k8s.io/community/contributors/devel/sig-testing/flaky-tests.md#filing-issues-for-flaky-tests) an [open issue](https://github.com/kubernetes-csi/external-provisioner/issues?q=is:issue+is:open) when you hit one in your PR.

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
RaunakShah: /retest

k8s-ci-robot: @RaunakShah: The following test **failed**, say `/retest` to rerun all failed tests:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
pull-kubernetes-csi-external-provisioner-1-17-on-kubernetes-1-17 | 75c1cabb5b82028d6447475e7434906610f4e682 | [link](https://prow.k8s.io/view/gcs/kubernetes-jenkins/pr-logs/pull/kubernetes-csi_external-provisioner/438/pull-kubernetes-csi-external-provisioner-1-17-on-kubernetes-1-17/1260683883137994760) | `/test pull-kubernetes-csi-external-provisioner-1-17-on-kubernetes-1-17`

[Full PR test history](https://prow.k8s.io/pr-history?org=kubernetes-csi&repo=external-provisioner&pr=438). [Your PR dashboard](https://prow.k8s.io/pr?query=is%3Apr%20state%3Aopen%20author%3RaunakShah). Please help us cut down on flakes by [linking to](https://git.k8s.io/community/contributors/devel/sig-testing/flaky-tests.md#filing-issues-for-flaky-tests) an [open issue](https://github.com/kubernetes-csi/external-provisioner/issues?q=is:issue+is:open) when you hit one in your PR.

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
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

Patch:
diff --git a/cmd/csi-provisioner/csi-provisioner.go b/cmd/csi-provisioner/csi-provisioner.go
index 7bbd8e185..f1a40ca2b 100644
--- a/cmd/csi-provisioner/csi-provisioner.go
+++ b/cmd/csi-provisioner/csi-provisioner.go
@@ -182,6 +182,7 @@ func main() {
 	claimLister := factory.Core().V1().PersistentVolumeClaims().Lister()
 
 	var csiNodeLister storagelistersv1.CSINodeLister
+	vaLister := factory.Storage().V1().VolumeAttachments().Lister()
 	var nodeLister v1.NodeLister
 	if ctrl.SupportsTopology(pluginCapabilities) {
 		csiNodeLister = factory.Storage().V1().CSINodes().Lister()
@@ -237,6 +238,7 @@ func main() {
 		csiNodeLister,
 		nodeLister,
 		claimLister,
+		vaLister,
 		*extraCreateMetadata,
 	)
 
diff --git a/deploy/kubernetes/rbac.yaml b/deploy/kubernetes/rbac.yaml
index 35b688015..8d79962d2 100644
--- a/deploy/kubernetes/rbac.yaml
+++ b/deploy/kubernetes/rbac.yaml
@@ -50,6 +50,9 @@ rules:
   - apiGroups: [""]
     resources: ["nodes"]
     verbs: ["get", "list", "watch"]
+  - apiGroups: ["storage.k8s.io"]
+    resources: ["volumeattachments"]
+    verbs: ["get", "list", "watch"]
 
 ---
 kind: ClusterRoleBinding
diff --git a/pkg/controller/controller.go b/pkg/controller/controller.go
index 417792040..27b2c35af 100644
--- a/pkg/controller/controller.go
+++ b/pkg/controller/controller.go
@@ -38,6 +38,7 @@ import (
 	storagev1 "k8s.io/api/storage/v1"
 	"k8s.io/apimachinery/pkg/api/resource"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/labels"
 	_ "k8s.io/apimachinery/pkg/util/json"
 	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/apimachinery/pkg/util/validation"
@@ -219,6 +220,7 @@ type csiProvisioner struct {
 	csiNodeLister                         storagelistersv1.CSINodeLister
 	nodeLister                            corelisters.NodeLister
 	claimLister                           corelisters.PersistentVolumeClaimLister
+	vaLister                              storagelistersv1.VolumeAttachmentLister
 	extraCreateMetadata                   bool
 }
 
@@ -284,6 +286,7 @@ func NewCSIProvisioner(client kubernetes.Interface,
 	csiNodeLister storagelistersv1.CSINodeLister,
 	nodeLister corelisters.NodeLister,
 	claimLister corelisters.PersistentVolumeClaimLister,
+	vaLister storagelistersv1.VolumeAttachmentLister,
 	extraCreateMetadata bool,
 ) controller.Provisioner {
 
@@ -307,6 +310,7 @@ func NewCSIProvisioner(client kubernetes.Interface,
 		csiNodeLister:                         csiNodeLister,
 		nodeLister:                            nodeLister,
 		claimLister:                           claimLister,
+		vaLister:                              vaLister,
 		extraCreateMetadata:                   extraCreateMetadata,
 	}
 	return provisioner
@@ -1006,7 +1010,6 @@ func (p *csiProvisioner) Delete(volume *v1.PersistentVolume) error {
 	req := csi.DeleteVolumeRequest{
 		VolumeId: volumeId,
 	}
-
 	// get secrets if StorageClass specifies it
 	storageClassName := util.GetPersistentVolumeClass(volume)
 	if len(storageClassName) != 0 {
@@ -1035,6 +1038,18 @@ func (p *csiProvisioner) Delete(volume *v1.PersistentVolume) error {
 	ctx, cancel := context.WithTimeout(context.Background(), p.timeout)
 	defer cancel()
 
+	// Verify if volume is attached to a node before proceeding with deletion
+	vaList, err := p.vaLister.List(labels.Everything())
+	if err != nil {
+		return fmt.Errorf("failed to list volumeattachments: %v", err)
+	}
+
+	for _, va := range vaList {
+		if va.Spec.Source.PersistentVolumeName != nil && *va.Spec.Source.PersistentVolumeName == volume.Name {
+			return fmt.Errorf("persistentvolume %s is still attached to node %s", volume.Name, va.Spec.NodeName)
+		}
+	}
+
 	_, err = p.csiClient.DeleteVolume(ctx, &req)
 
 	return err


Test patch:
diff --git a/pkg/controller/controller_test.go b/pkg/controller/controller_test.go
index f3227b575..835406fd7 100644
--- a/pkg/controller/controller_test.go
+++ b/pkg/controller/controller_test.go
@@ -409,7 +409,7 @@ func TestCreateDriverReturnsInvalidCapacityDuringProvision(t *testing.T) {
 
 	pluginCaps, controllerCaps := provisionCapabilities()
 	csiProvisioner := NewCSIProvisioner(nil, 5*time.Second, "test-provisioner", "test",
-		5, csiConn.conn, nil, driverName, pluginCaps, controllerCaps, "", false, csitrans.New(), nil, nil, nil, nil, false)
+		5, csiConn.conn, nil, driverName, pluginCaps, controllerCaps, "", false, csitrans.New(), nil, nil, nil, nil, nil, false)
 
 	// Requested PVC with requestedBytes storage
 	deletePolicy := v1.PersistentVolumeReclaimDelete
@@ -1674,7 +1674,7 @@ func runProvisionTest(t *testing.T, k string, tc provisioningTestcase, requested
 
 	pluginCaps, controllerCaps := provisionCapabilities()
 	csiProvisioner := NewCSIProvisioner(clientSet, 5*time.Second, "test-provisioner", "test", 5, csiConn.conn,
-		nil, provisionDriverName, pluginCaps, controllerCaps, supportsMigrationFromInTreePluginName, false, csitrans.New(), nil, nil, nil, nil, tc.withExtraMetadata)
+		nil, provisionDriverName, pluginCaps, controllerCaps, supportsMigrationFromInTreePluginName, false, csitrans.New(), nil, nil, nil, nil, nil, tc.withExtraMetadata)
 
 	out := &csi.CreateVolumeResponse{
 		Volume: &csi.Volume{
@@ -2410,7 +2410,7 @@ func TestProvisionFromSnapshot(t *testing.T) {
 
 		pluginCaps, controllerCaps := provisionFromSnapshotCapabilities()
 		csiProvisioner := NewCSIProvisioner(clientSet, 5*time.Second, "test-provisioner", "test", 5, csiConn.conn,
-			client, driverName, pluginCaps, controllerCaps, "", false, csitrans.New(), nil, nil, nil, nil, false)
+			client, driverName, pluginCaps, controllerCaps, "", false, csitrans.New(), nil, nil, nil, nil, nil, false)
 
 		out := &csi.CreateVolumeResponse{
 			Volume: &csi.Volume{
@@ -2580,11 +2580,11 @@ func TestProvisionWithTopologyEnabled(t *testing.T) {
 
 			clientSet := fakeclientset.NewSimpleClientset(nodes, csiNodes)
 
-			scLister, csiNodeLister, nodeLister, claimLister, stopChan := listers(clientSet)
+			scLister, csiNodeLister, nodeLister, claimLister, vaLister, stopChan := listers(clientSet)
 			defer close(stopChan)
 
 			csiProvisioner := NewCSIProvisioner(clientSet, 5*time.Second, "test-provisioner", "test", 5,
-				csiConn.conn, nil, driverName, pluginCaps, controllerCaps, "", false, csitrans.New(), scLister, csiNodeLister, nodeLister, claimLister, false)
+				csiConn.conn, nil, driverName, pluginCaps, controllerCaps, "", false, csitrans.New(), scLister, csiNodeLister, nodeLister, claimLister, vaLister, false)
 
 			pv, err := csiProvisioner.Provision(controller.ProvisionOptions{
 				StorageClass: &storagev1.StorageClass{},
@@ -2674,11 +2674,11 @@ func TestProvisionErrorHandling(t *testing.T) {
 					nodes := buildNodes(nodeLabels, k8sTopologyBetaVersion.String())
 					csiNodes := buildCSINodes(topologyKeys)
 					clientSet := fakeclientset.NewSimpleClientset(nodes, csiNodes)
-					scLister, csiNodeLister, nodeLister, claimLister, stopChan := listers(clientSet)
+					scLister, csiNodeLister, nodeLister, claimLister, vaLister, stopChan := listers(clientSet)
 					defer close(stopChan)
 
 					csiProvisioner := NewCSIProvisioner(clientSet, 5*time.Second, "test-provisioner", "test", 5,
-						csiConn.conn, nil, driverName, pluginCaps, controllerCaps, "", false, csitrans.New(), scLister, csiNodeLister, nodeLister, claimLister, false)
+						csiConn.conn, nil, driverName, pluginCaps, controllerCaps, "", false, csitrans.New(), scLister, csiNodeLister, nodeLister, claimLister, vaLister, false)
 					csiProvisionerExt := csiProvisioner.(controller.ProvisionerExt)
 
 					options := controller.ProvisionOptions{
@@ -2752,7 +2752,7 @@ func TestProvisionWithTopologyDisabled(t *testing.T) {
 	clientSet := fakeclientset.NewSimpleClientset()
 	pluginCaps, controllerCaps := provisionWithTopologyCapabilities()
 	csiProvisioner := NewCSIProvisioner(clientSet, 5*time.Second, "test-provisioner", "test", 5,
-		csiConn.conn, nil, driverName, pluginCaps, controllerCaps, "", false, csitrans.New(), nil, nil, nil, nil, false)
+		csiConn.conn, nil, driverName, pluginCaps, controllerCaps, "", false, csitrans.New(), nil, nil, nil, nil, nil, false)
 
 	out := &csi.CreateVolumeResponse{
 		Volume: &csi.Volume{
@@ -2786,12 +2786,15 @@ func TestProvisionWithTopologyDisabled(t *testing.T) {
 type deleteTestcase struct {
 	persistentVolume *v1.PersistentVolume
 	storageClass     *storagev1.StorageClass
+	volumeAttachment *storagev1.VolumeAttachment
 	mockDelete       bool
 	expectErr        bool
 }
 
 // TestDelete is a test of the delete operation
 func TestDelete(t *testing.T) {
+	pvName := "pv"
+	deletionTimestamp := metav1.NewTime(time.Now())
 	tt := map[string]deleteTestcase{
 		"fail - nil PV": {
 			persistentVolume: nil,
@@ -2838,6 +2841,127 @@ func TestDelete(t *testing.T) {
 			},
 			expectErr: true,
 		},
+		"fail - delete when attached to node": {
+			persistentVolume: &v1.PersistentVolume{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: pvName,
+				},
+				Spec: v1.PersistentVolumeSpec{
+					PersistentVolumeSource: v1.PersistentVolumeSource{
+						CSI: &v1.CSIPersistentVolumeSource{
+							VolumeHandle: "vol-id-1",
+						},
+					},
+					ClaimRef: &v1.ObjectReference{
+						Name: "sc-name",
+					},
+					StorageClassName: "sc-name",
+				},
+			},
+			storageClass: &storagev1.StorageClass{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "sc-name",
+				},
+				Parameters: map[string]string{
+					prefixedProvisionerSecretNameKey: "static-${pv.name}-${pvc.namespace}-${pvc.name}",
+				},
+			},
+			volumeAttachment: &storagev1.VolumeAttachment{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "va",
+				},
+				Spec: storagev1.VolumeAttachmentSpec{
+					Source: storagev1.VolumeAttachmentSource{
+						PersistentVolumeName: &pvName,
+					},
+					NodeName: "node",
+				},
+				Status: storagev1.VolumeAttachmentStatus{
+					Attached: true,
+				},
+			},
+			expectErr: true,
+		},
+		"fail - delete when volumeattachment exists but not attached to node": {
+			persistentVolume: &v1.PersistentVolume{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: pvName,
+				},
+				Spec: v1.PersistentVolumeSpec{
+					PersistentVolumeSource: v1.PersistentVolumeSource{
+						CSI: &v1.CSIPersistentVolumeSource{
+							VolumeHandle: "vol-id-1",
+						},
+					},
+					ClaimRef: &v1.ObjectReference{
+						Name: "sc-name",
+					},
+					StorageClassName: "sc-name",
+				},
+			},
+			storageClass: &storagev1.StorageClass{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "sc-name",
+				},
+				Parameters: map[string]string{
+					prefixedProvisionerSecretNameKey: "static-${pv.name}-${pvc.namespace}-${pvc.name}",
+				},
+			},
+			volumeAttachment: &storagev1.VolumeAttachment{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "va",
+				},
+				Spec: storagev1.VolumeAttachmentSpec{
+					Source: storagev1.VolumeAttachmentSource{
+						PersistentVolumeName: &pvName,
+					},
+					NodeName: "node",
+				},
+				Status: storagev1.VolumeAttachmentStatus{
+					Attached: false,
+				},
+			},
+			expectErr: true,
+		},
+		"fail - delete when volumeattachment exists with deletionTimestamp set": {
+			persistentVolume: &v1.PersistentVolume{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: pvName,
+				},
+				Spec: v1.PersistentVolumeSpec{
+					PersistentVolumeSource: v1.PersistentVolumeSource{
+						CSI: &v1.CSIPersistentVolumeSource{
+							VolumeHandle: "vol-id-1",
+						},
+					},
+					ClaimRef: &v1.ObjectReference{
+						Name: "sc-name",
+					},
+					StorageClassName: "sc-name",
+				},
+			},
+			storageClass: &storagev1.StorageClass{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "sc-name",
+				},
+				Parameters: map[string]string{
+					prefixedProvisionerSecretNameKey: "static-${pv.name}-${pvc.namespace}-${pvc.name}",
+				},
+			},
+			volumeAttachment: &storagev1.VolumeAttachment{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:              "va",
+					DeletionTimestamp: &deletionTimestamp,
+				},
+				Spec: storagev1.VolumeAttachmentSpec{
+					Source: storagev1.VolumeAttachmentSource{
+						PersistentVolumeName: &pvName,
+					},
+					NodeName: "node",
+				},
+			},
+			expectErr: true,
+		},
 		"simple - valid case": {
 			persistentVolume: &v1.PersistentVolume{
 				ObjectMeta: metav1.ObjectMeta{
@@ -2862,6 +2986,47 @@ func TestDelete(t *testing.T) {
 			expectErr:  false,
 			mockDelete: true,
 		},
+		"simple - valid case with existing volumeattachment on different pv": {
+			persistentVolume: &v1.PersistentVolume{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "pv-without-attachment",
+				},
+				Spec: v1.PersistentVolumeSpec{
+					PersistentVolumeSource: v1.PersistentVolumeSource{
+						CSI: &v1.CSIPersistentVolumeSource{
+							VolumeHandle: "vol-id-1",
+						},
+					},
+					ClaimRef: &v1.ObjectReference{
+						Name: "pvc-name",
+					},
+				},
+			},
+			storageClass: &storagev1.StorageClass{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "sc-name",
+				},
+				Parameters: map[string]string{
+					prefixedProvisionerSecretNameKey: "static-${pv.name}-${pvc.namespace}-${pvc.name}",
+				},
+			},
+			volumeAttachment: &storagev1.VolumeAttachment{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "va",
+				},
+				Spec: storagev1.VolumeAttachmentSpec{
+					Source: storagev1.VolumeAttachmentSource{
+						PersistentVolumeName: &pvName,
+					},
+					NodeName: "node",
+				},
+				Status: storagev1.VolumeAttachmentStatus{
+					Attached: true,
+				},
+			},
+			expectErr:  false,
+			mockDelete: true,
+		},
 		"simple - valid case with ClaimRef set": {
 			persistentVolume: &v1.PersistentVolume{
 				ObjectMeta: metav1.ObjectMeta{
@@ -2922,9 +3087,9 @@ func runDeleteTest(t *testing.T, k string, tc deleteTestcase) {
 	}
 
 	pluginCaps, controllerCaps := provisionCapabilities()
-	scLister, _, _, _, _ := listers(clientSet)
+	scLister, _, _, _, vaLister, _ := listers(clientSet)
 	csiProvisioner := NewCSIProvisioner(clientSet, 5*time.Second, "test-provisioner", "test", 5,
-		csiConn.conn, nil, driverName, pluginCaps, controllerCaps, "", false, csitrans.New(), scLister, nil, nil, nil, false)
+		csiConn.conn, nil, driverName, pluginCaps, controllerCaps, "", false, csitrans.New(), scLister, nil, nil, nil, vaLister, false)
 
 	err = csiProvisioner.Delete(tc.persistentVolume)
 	if tc.expectErr && err == nil {
@@ -3341,11 +3506,11 @@ func TestProvisionFromPVC(t *testing.T) {
 				}).Return(&csi.DeleteVolumeResponse{}, nil).Times(1)
 			}
 
-			_, _, _, claimLister, _ := listers(clientSet)
+			_, _, _, claimLister, _, _ := listers(clientSet)
 
 			// Phase: execute the test
 			csiProvisioner := NewCSIProvisioner(clientSet, 5*time.Second, "test-provisioner", "test", 5, csiConn.conn,
-				nil, driverName, pluginCaps, controllerCaps, "", false, csitrans.New(), nil, nil, nil, claimLister, false)
+				nil, driverName, pluginCaps, controllerCaps, "", false, csitrans.New(), nil, nil, nil, claimLister, nil, false)
 
 			pv, err = csiProvisioner.Provision(tc.volOpts)
 			if tc.expectErr && err == nil {
@@ -3463,7 +3628,7 @@ func TestProvisionWithMigration(t *testing.T) {
 			pluginCaps, controllerCaps := provisionCapabilities()
 			csiProvisioner := NewCSIProvisioner(clientSet, 5*time.Second, "test-provisioner",
 				"test", 5, csiConn.conn, nil, driverName, pluginCaps, controllerCaps,
-				inTreePluginName, false, mockTranslator, nil, nil, nil, nil, false)
+				inTreePluginName, false, mockTranslator, nil, nil, nil, nil, nil, false)
 
 			// Set up return values (AnyTimes to avoid overfitting on implementation)
 
@@ -3621,9 +3786,11 @@ func TestDeleteMigration(t *testing.T) {
 			defer driver.Stop()
 			clientSet := fakeclientset.NewSimpleClientset()
 			pluginCaps, controllerCaps := provisionCapabilities()
+			_, _, _, _, vaLister, stopCh := listers(clientSet)
+			defer close(stopCh)
 			csiProvisioner := NewCSIProvisioner(clientSet, 5*time.Second, "test-provisioner",
 				"test", 5, csiConn.conn, nil, driverName, pluginCaps, controllerCaps, "",
-				false, mockTranslator, nil, nil, nil, nil, false)
+				false, mockTranslator, nil, nil, nil, nil, vaLister, false)
 
 			// Set mock return values (AnyTimes to avoid overfitting on implementation details)
 			mockTranslator.EXPECT().IsPVMigratable(gomock.Any()).Return(tc.expectTranslation).AnyTimes()
diff --git a/pkg/controller/topology_test.go b/pkg/controller/topology_test.go
index e218d993c..7fd430294 100644
--- a/pkg/controller/topology_test.go
+++ b/pkg/controller/topology_test.go
@@ -393,7 +393,7 @@ func TestStatefulSetSpreading(t *testing.T) {
 
 	kubeClient := fakeclientset.NewSimpleClientset(nodes, csiNodes)
 
-	_, csiNodeLister, nodeLister, _, stopChan := listers(kubeClient)
+	_, csiNodeLister, nodeLister, _, _, stopChan := listers(kubeClient)
 	defer close(stopChan)
 
 	for name, tc := range testcases {
@@ -1083,7 +1083,7 @@ func TestTopologyAggregation(t *testing.T) {
 
 			kubeClient := fakeclientset.NewSimpleClientset(nodes, csiNodes)
 
-			_, csiNodeLister, nodeLister, _, stopChan := listers(kubeClient)
+			_, csiNodeLister, nodeLister, _, _, stopChan := listers(kubeClient)
 			defer close(stopChan)
 
 			var selectedNode *v1.Node
@@ -1403,7 +1403,7 @@ func TestPreferredTopologies(t *testing.T) {
 			kubeClient := fakeclientset.NewSimpleClientset(nodes, csiNodes)
 			selectedNode := &nodes.Items[0]
 
-			_, csiNodeLister, nodeLister, _, stopChan := listers(kubeClient)
+			_, csiNodeLister, nodeLister, _, _, stopChan := listers(kubeClient)
 			defer close(stopChan)
 
 			requirements, err := GenerateAccessibilityRequirements(
@@ -1622,6 +1622,7 @@ func listers(kubeClient *fakeclientset.Clientset) (
 	storagelistersv1.CSINodeLister,
 	corelisters.NodeLister,
 	corelisters.PersistentVolumeClaimLister,
+	storagelistersv1.VolumeAttachmentLister,
 	chan struct{}) {
 	factory := informers.NewSharedInformerFactory(kubeClient, ResyncPeriodOfCsiNodeInformer)
 	stopChan := make(chan struct{})
@@ -1629,7 +1630,8 @@ func listers(kubeClient *fakeclientset.Clientset) (
 	csiNodeLister := factory.Storage().V1().CSINodes().Lister()
 	nodeLister := factory.Core().V1().Nodes().Lister()
 	claimLister := factory.Core().V1().PersistentVolumeClaims().Lister()
+	vaLister := factory.Storage().V1().VolumeAttachments().Lister()
 	factory.Start(stopChan)
 	factory.WaitForCacheSync(stopChan)
-	return scLister, csiNodeLister, nodeLister, claimLister, stopChan
+	return scLister, csiNodeLister, nodeLister, claimLister, vaLister, stopChan
 }