Repository: kubernetes-sigs/cloud-provider-azure
Pull request #780 (2021-08-29 13:00:15)

fix: set back node level lock in disk detach
<!--  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. Please label this pull request according to what type of issue you are addressing, especially if this is a release targeted pull request. For reference on required PR/issue labels, read here:
https://git.k8s.io/community/contributors/devel/release.md#issue-kind-label
3. Ensure you have added or ran the appropriate tests for your PR: https://git.k8s.io/community/contributors/devel/testing.md
4. If you want *faster* PR reviews, read how: https://git.k8s.io/community/contributors/guide/pull-requests.md#best-practices-for-faster-reviews
5. Follow the instructions for writing a release note: https://git.k8s.io/community/contributors/guide/release-notes.md
6. 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**:
fix: set back node level lock in disk detach
attach disk would depend on detach disk in pod failover between different nodes, original async disk detach would make some disk detach completion too soon, that would lead to more disk attach time cost and some internal error msg. This PR set back node level lock **only** in disk detach.

This PR also add `ctx context.Context` parameter in `AttachDisk` & `DetachDisk`

**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 #

**Special notes for your reviewer**:
```console
v1.7.0 with sync detach

Events:
  Type     Reason                  Age                From                     Message
  ----     ------                  ----               ----                     -------
  Normal   Scheduled               2m28s              default-scheduler        Successfully assigned default/statefulset-azuredisk-5-0 to aks-disk-18870927-vmss000001
  Warning  FailedAttachVolume      2m13s              attachdetach-controller  Multi-Attach error for volume "pvc-d133d284-41c9-49db-a404-3daa5be7729d" Volume is already exclusively attached to one node and can't be attached to another
  Normal   SuccessfulAttachVolume  2m2s               attachdetach-controller  AttachVolume.Attach succeeded for volume "pvc-02b4eab3-7ee0-4009-a7d4-025871892c66"
  Warning  FailedAttachVolume      59s (x2 over 60s)  attachdetach-controller  AttachVolume.Attach failed for volume "pvc-64a409c9-a069-4d41-a828-d5df4aaaaa9f" : rpc error: code = Unknown desc = Attach volume "/subscriptions/xxx/resourceGroups/mc_andy-aks1205_andy-aks1205_eastus2/providers/Microsoft.Compute/disks/pvc-64a409c9-a069-4d41-a828-d5df4aaaaa9f" to instance "aks-disk-18870927-vmss000001" failed with disk(/subscriptions/xxx/resourceGroups/mc_andy-aks1205_andy-aks1205_eastus2/providers/Microsoft.Compute/disks/pvc-64a409c9-a069-4d41-a828-d5df4aaaaa9f) already attached to node(/subscriptions/xxx/resourceGroups/MC_andy-aks1205_andy-aks1205_eastus2/providers/Microsoft.Compute/virtualMachineScaleSets/aks-disk-18870927-vmss/virtualMachines/aks-disk-18870927-vmss_0), could not be attached to node(aks-disk-18870927-vmss000001)
  Normal   SuccessfulAttachVolume  58s                attachdetach-controller  AttachVolume.Attach succeeded for volume "pvc-64a409c9-a069-4d41-a828-d5df4aaaaa9f"
  Warning  FailedAttachVolume      52s (x5 over 60s)  attachdetach-controller  AttachVolume.Attach failed for volume "pvc-8e0f8b09-d125-4595-b53b-9abe7c456d7f" : rpc error: code = Unknown desc = Attach volume "/subscriptions/xxx/resourceGroups/mc_andy-aks1205_andy-aks1205_eastus2/providers/Microsoft.Compute/disks/pvc-8e0f8b09-d125-4595-b53b-9abe7c456d7f" to instance "aks-disk-18870927-vmss000001" failed with disk(/subscriptions/xxx/resourceGroups/mc_andy-aks1205_andy-aks1205_eastus2/providers/Microsoft.Compute/disks/pvc-8e0f8b09-d125-4595-b53b-9abe7c456d7f) already attached to node(/subscriptions/xxx/resourceGroups/MC_andy-aks1205_andy-aks1205_eastus2/providers/Microsoft.Compute/virtualMachineScaleSets/aks-disk-18870927-vmss/virtualMachines/aks-disk-18870927-vmss_0), could not be attached to node(aks-disk-18870927-vmss000001)
  Warning  FailedAttachVolume      44s (x6 over 60s)  attachdetach-controller  AttachVolume.Attach failed for volume "pvc-c8a9d621-2548-4cfe-bdac-5016c29e187b" : rpc error: code = Unknown desc = Attach volume "/subscriptions/xxx/resourceGroups/mc_andy-aks1205_andy-aks1205_eastus2/providers/Microsoft.Compute/disks/pvc-c8a9d621-2548-4cfe-bdac-5016c29e187b" to instance "aks-disk-18870927-vmss000001" failed with disk(/subscriptions/xxx/resourceGroups/mc_andy-aks1205_andy-aks1205_eastus2/providers/Microsoft.Compute/disks/pvc-c8a9d621-2548-4cfe-bdac-5016c29e187b) already attached to node(/subscriptions/xxx/resourceGroups/MC_andy-aks1205_andy-aks1205_eastus2/providers/Microsoft.Compute/virtualMachineScaleSets/aks-disk-18870927-vmss/virtualMachines/aks-disk-18870927-vmss_0), could not be attached to node(aks-disk-18870927-vmss000001)
  Normal   SuccessfulAttachVolume  44s                attachdetach-controller  AttachVolume.Attach succeeded for volume "pvc-8e0f8b09-d125-4595-b53b-9abe7c456d7f"
  Normal   SuccessfulAttachVolume  42s                attachdetach-controller  AttachVolume.Attach succeeded for volume "pvc-d133d284-41c9-49db-a404-3daa5be7729d"
  Normal   SuccessfulAttachVolume  28s                attachdetach-controller  AttachVolume.Attach succeeded for volume "pvc-c8a9d621-2548-4cfe-bdac-5016c29e187b"
  Warning  FailedMount             25s                kubelet                  Unable to attach or mount volumes: unmounted volumes=[persistent-storage persistent-storage2 persistent-storage4 persistent-storage5], unattached volumes=[persistent-storage persistent-storage2 persistent-storage3 persistent-storage4 persistent-storage5 default-token-l5k8d]: timed out waiting for the condition
  Normal   Pulled                  11s                kubelet                  Container image "mcr.microsoft.com/oss/nginx/nginx:1.19.5" already present on machine
  Normal   Created                 10s                kubelet                  Created container statefulset-azuredisk
  Normal   Started                 10s                kubelet                  Started container statefulset-azuredisk
  
  
# grep ublishVolume /tmp/csi-azuredisk-controller-684fbb45cc-nm8l6
I0829 12:31:53.545333       1 utils.go:95] GRPC call: /csi.v1.Controller/ControllerUnpublishVolume
I0829 12:31:53.545456       1 utils.go:95] GRPC call: /csi.v1.Controller/ControllerUnpublishVolume
I0829 12:31:53.554228       1 utils.go:95] GRPC call: /csi.v1.Controller/ControllerUnpublishVolume
I0829 12:31:53.565027       1 utils.go:95] GRPC call: /csi.v1.Controller/ControllerUnpublishVolume
I0829 12:31:53.580331       1 utils.go:95] GRPC call: /csi.v1.Controller/ControllerUnpublishVolume
I0829 12:32:09.767903       1 utils.go:95] GRPC call: /csi.v1.Controller/ControllerUnpublishVolume
I0829 12:32:09.823052       1 utils.go:95] GRPC call: /csi.v1.Controller/ControllerUnpublishVolume
I0829 12:32:09.845942       1 utils.go:95] GRPC call: /csi.v1.Controller/ControllerPublishVolume
I0829 12:32:20.247820       1 utils.go:95] GRPC call: /csi.v1.Controller/ControllerPublishVolume
I0829 12:32:21.219278       1 utils.go:95] GRPC call: /csi.v1.Controller/ControllerUnpublishVolume
I0829 12:32:21.242627       1 utils.go:95] GRPC call: /csi.v1.Controller/ControllerPublishVolume
I0829 12:32:36.614630       1 utils.go:95] GRPC call: /csi.v1.Controller/ControllerPublishVolume
I0829 12:32:51.950970       1 utils.go:95] GRPC call: /csi.v1.Controller/ControllerPublishVolume
I0829 12:33:22.594590       1 utils.go:95] GRPC call: /csi.v1.Controller/ControllerPublishVolume
I0829 12:33:22.594590       1 utils.go:95] GRPC call: /csi.v1.Controller/ControllerPublishVolume
I0829 12:33:22.596595       1 utils.go:95] GRPC call: /csi.v1.Controller/ControllerPublishVolume
I0829 12:33:23.953261       1 utils.go:95] GRPC call: /csi.v1.Controller/ControllerPublishVolume
I0829 12:33:29.604210       1 utils.go:95] GRPC call: /csi.v1.Controller/ControllerPublishVolume
I0829 12:33:37.912600       1 utils.go:95] GRPC call: /csi.v1.Controller/ControllerPublishVolume
```

**Release note**:
```
fix: set back node level lock in disk detach

```



Patch:
diff --git a/pkg/provider/azure_controller_common.go b/pkg/provider/azure_controller_common.go
index 8fb8ec7fe..4beb76cca 100644
--- a/pkg/provider/azure_controller_common.go
+++ b/pkg/provider/azure_controller_common.go
@@ -147,7 +147,7 @@ func (c *controllerCommon) getNodeVMSet(nodeName types.NodeName, crt azcache.Azu
 
 // AttachDisk attaches a vhd to vm. The vhd must exist, can be identified by diskName, diskURI.
 // return (lun, error)
-func (c *controllerCommon) AttachDisk(isManagedDisk bool, diskName, diskURI string, nodeName types.NodeName,
+func (c *controllerCommon) AttachDisk(ctx context.Context, isManagedDisk bool, diskName, diskURI string, nodeName types.NodeName,
 	cachingMode compute.CachingTypes, disk *compute.Disk) (int32, error) {
 	diskEncryptionSetID := ""
 	writeAcceleratorEnabled := false
@@ -262,15 +262,10 @@ func (c *controllerCommon) AttachDisk(isManagedDisk bool, diskName, diskURI stri
 	} else {
 		klog.Warningf("azureDisk - switch to batch operation since disk operation is rate limited, current QPS: %f", c.diskOpRateLimiter.QPS())
 	}
-
-	ctx, cancel := getContextWithCancel()
-	defer cancel()
-
 	resourceGroup, err := getResourceGroupFromDiskURI(diskURI)
 	if err != nil {
 		return -1, err
 	}
-
 	return lun, vmset.WaitForUpdateResult(ctx, future, resourceGroup, "attach_disk")
 }
 
@@ -318,9 +313,7 @@ func (c *controllerCommon) cleanAttachDiskRequests(nodeName string) (map[string]
 }
 
 // DetachDisk detaches a disk from VM
-func (c *controllerCommon) DetachDisk(diskName, diskURI string, nodeName types.NodeName) error {
-	ctx, cancel := getContextWithCancel()
-	defer cancel()
+func (c *controllerCommon) DetachDisk(ctx context.Context, diskName, diskURI string, nodeName types.NodeName) error {
 	_, err := c.cloud.InstanceID(ctx, nodeName)
 	if err != nil {
 		if errors.Is(err, cloudprovider.InstanceNotFound) {
@@ -374,14 +367,6 @@ func (c *controllerCommon) DetachDisk(diskName, diskURI string, nodeName types.N
 		if err != nil {
 			return err
 		}
-		if c.diskOpRateLimiter.TryAccept() {
-			// unlock and wait for attach disk complete
-			unlock = true
-			c.lockMap.UnlockEntry(node)
-		} else {
-			klog.Warningf("azureDisk - switch to batch operation since disk operation is rate limited, current QPS: %f", c.diskOpRateLimiter.QPS())
-		}
-
 		if err := vmset.WaitForUpdateResult(ctx, future, resourceGroup, "detach_disk"); err != nil {
 			klog.Errorf("azureDisk - detach disk(%s, %s) failed with error: %v", diskName, diskURI, err)
 			return err


Test patch:
diff --git a/pkg/provider/azure_controller_common_test.go b/pkg/provider/azure_controller_common_test.go
index 8571b9140..9e6f0fa49 100644
--- a/pkg/provider/azure_controller_common_test.go
+++ b/pkg/provider/azure_controller_common_test.go
@@ -47,6 +47,9 @@ func TestCommonAttachDisk(t *testing.T) {
 	ctrl := gomock.NewController(t)
 	defer ctrl.Finish()
 
+	ctx, cancel := getContextWithCancel()
+	defer cancel()
+
 	maxShare := int32(1)
 	goodInstanceID := fmt.Sprintf("/subscriptions/subscription/resourceGroups/rg/providers/Microsoft.Compute/virtualMachines/%s", "vm1")
 	diskEncryptionSetID := fmt.Sprintf("/subscriptions/subscription/resourceGroups/rg/providers/Microsoft.Compute/diskEncryptionSets/%s", "diskEncryptionSet-name")
@@ -174,7 +177,7 @@ func TestCommonAttachDisk(t *testing.T) {
 		mockVMsClient.EXPECT().UpdateAsync(gomock.Any(), testCloud.ResourceGroup, gomock.Any(), gomock.Any(), gomock.Any()).Return(&azure.Future{}, nil).AnyTimes()
 		mockVMsClient.EXPECT().WaitForUpdateResult(gomock.Any(), gomock.Any(), testCloud.ResourceGroup, gomock.Any()).Return(nil).AnyTimes()
 
-		lun, err := common.AttachDisk(true, "", diskURI, test.nodeName, compute.CachingTypesReadOnly, test.existedDisk)
+		lun, err := common.AttachDisk(ctx, true, "", diskURI, test.nodeName, compute.CachingTypesReadOnly, test.existedDisk)
 		assert.Equal(t, test.expectedLun, lun, "TestCase[%d]: %s", i, test.desc)
 		assert.Equal(t, test.expectedErr, err != nil, "TestCase[%d]: %s, return error: %v", i, test.desc, err)
 	}
@@ -184,6 +187,9 @@ func TestCommonAttachDiskWithVMSS(t *testing.T) {
 	ctrl := gomock.NewController(t)
 	defer ctrl.Finish()
 
+	ctx, cancel := getContextWithCancel()
+	defer cancel()
+
 	testCases := []struct {
 		desc            string
 		vmList          map[string]string
@@ -271,7 +277,7 @@ func TestCommonAttachDiskWithVMSS(t *testing.T) {
 			mockVMsClient.EXPECT().Update(gomock.Any(), testCloud.ResourceGroup, gomock.Any(), gomock.Any(), gomock.Any()).Return(nil).AnyTimes()
 		}
 
-		lun, err := common.AttachDisk(test.isManagedDisk, "test", diskURI, test.nodeName, compute.CachingTypesReadOnly, test.existedDisk)
+		lun, err := common.AttachDisk(ctx, test.isManagedDisk, "test", diskURI, test.nodeName, compute.CachingTypesReadOnly, test.existedDisk)
 		assert.Equal(t, test.expectedLun, lun, "TestCase[%d]: %s", i, test.desc)
 		assert.Equal(t, test.expectedErr, err != nil, "TestCase[%d]: %s, return error: %v", i, test.desc, err)
 	}
@@ -281,6 +287,9 @@ func TestCommonDetachDisk(t *testing.T) {
 	ctrl := gomock.NewController(t)
 	defer ctrl.Finish()
 
+	ctx, cancel := getContextWithCancel()
+	defer cancel()
+
 	testCases := []struct {
 		desc        string
 		vmList      map[string]string
@@ -334,7 +343,7 @@ func TestCommonDetachDisk(t *testing.T) {
 		mockVMsClient.EXPECT().UpdateAsync(gomock.Any(), testCloud.ResourceGroup, gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, nil).AnyTimes()
 		mockVMsClient.EXPECT().WaitForUpdateResult(gomock.Any(), gomock.Any(), testCloud.ResourceGroup, gomock.Any()).Return(nil).AnyTimes()
 
-		err := common.DetachDisk(test.diskName, diskURI, test.nodeName)
+		err := common.DetachDisk(ctx, test.diskName, diskURI, test.nodeName)
 		assert.Equal(t, test.expectedErr, err != nil, "TestCase[%d]: %s, err: %v", i, test.desc, err)
 	}
 }