Repository: kubernetes-sigs/cloud-provider-azure
Pull request #678 (2021-06-27 09:47:23)

fix: remove GetDisk operation in AttachDisk
<!--  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: remove GetDisk operation in AttachDisk

This is actually one GetDisk operation in CSI driver AttachDisk interface, this PR removes GetDisk operation to reduce potential GetDisk throttling issue in large amount of disk attachment scenario.

And also retry immediately in DetachDisk is actually not necessary since CSI driver would retry if Detach failed in an outer looper, this could make things worse when there is throttling issue.

**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**:


**Release note**:
```
fix: remove GetDisk operation in AttachDisk
```


k8s-ci-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/kubernetes-sigs/cloud-provider-azure/pull/678#" title="Author self-approved">andyzhangx</a>*

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

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

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

- ~~[OWNERS](https://github.com/kubernetes-sigs/cloud-provider-azure/blob/master/OWNERS)~~ [andyzhangx]

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":[]} -->
k8s-ci-robot: @andyzhangx: The following test **failed**, say `/retest` to rerun all failed tests:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
pull-cloud-provider-azure-check | ce3349f9a494d5850a4ecb659673b93ee357f088 | [link](https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_cloud-provider-azure/678/pull-cloud-provider-azure-check/1409085946506252288) | `/test pull-cloud-provider-azure-check`

[Full PR test history](https://prow.k8s.io/pr-history?org=kubernetes-sigs&repo=cloud-provider-azure&pr=678). [Your PR dashboard](https://prow.k8s.io/pr?query=is%3Apr%20state%3Aopen%20author%3Aandyzhangx). 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-sigs/cloud-provider-azure/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 -->
coveralls: 
[![Coverage Status](https://coveralls.io/builds/40910947/badge)](https://coveralls.io/builds/40910947)

Coverage decreased (-0.0004%) to 79.523% when pulling **ce3349f9a494d5850a4ecb659673b93ee357f088 on andyzhangx:reduce-getdisk** into **97c3f1643349177928353ed8f3c4045c68622a80 on kubernetes-sigs:master**.


Patch:
diff --git a/pkg/provider/azure_controller_common.go b/pkg/provider/azure_controller_common.go
index 8c0142bfc..154dbed1d 100644
--- a/pkg/provider/azure_controller_common.go
+++ b/pkg/provider/azure_controller_common.go
@@ -37,7 +37,6 @@ import (
 
 	azcache "sigs.k8s.io/cloud-provider-azure/pkg/cache"
 	"sigs.k8s.io/cloud-provider-azure/pkg/consts"
-	"sigs.k8s.io/cloud-provider-azure/pkg/retry"
 )
 
 const (
@@ -145,25 +144,14 @@ 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, cachingMode compute.CachingTypes) (int32, error) {
+func (c *controllerCommon) AttachDisk(isManagedDisk bool, diskName, diskURI string, nodeName types.NodeName,
+	cachingMode compute.CachingTypes, disk *compute.Disk) (int32, error) {
 	diskEncryptionSetID := ""
 	writeAcceleratorEnabled := false
 
-	if isManagedDisk {
-		diskName := path.Base(diskURI)
-		resourceGroup, err := getResourceGroupFromDiskURI(diskURI)
-		if err != nil {
-			return -1, err
-		}
-
-		ctx, cancel := getContextWithCancel()
-		defer cancel()
-
-		disk, rerr := c.cloud.DisksClient.Get(ctx, resourceGroup, diskName)
-		if rerr != nil {
-			return -1, rerr.Error()
-		}
-
+	// there is possibility that disk is nil when GetDisk is throttled
+	// don't check disk state when GetDisk is throttled
+	if disk != nil {
 		if disk.ManagedBy != nil && (disk.MaxShares == nil || *disk.MaxShares <= 1) {
 			vmset, err := c.getNodeVMSet(nodeName, azcache.CacheReadTypeUnsafe)
 			if err != nil {
@@ -225,8 +213,8 @@ func (c *controllerCommon) AttachDisk(isManagedDisk bool, diskName, diskURI stri
 		writeAcceleratorEnabled: writeAcceleratorEnabled,
 	}
 	node := strings.ToLower(string(nodeName))
-	disk := strings.ToLower(diskURI)
-	if err := c.insertAttachDiskRequest(disk, node, &options); err != nil {
+	diskuri := strings.ToLower(diskURI)
+	if err := c.insertAttachDiskRequest(diskuri, node, &options); err != nil {
 		return -1, err
 	}
 
@@ -237,7 +225,7 @@ func (c *controllerCommon) AttachDisk(isManagedDisk bool, diskName, diskURI stri
 		return -1, err
 	}
 
-	lun, err := c.SetDiskLun(nodeName, disk, diskMap)
+	lun, err := c.SetDiskLun(nodeName, diskuri, diskMap)
 	if err != nil {
 		return -1, err
 	}
@@ -341,18 +329,11 @@ func (c *controllerCommon) DetachDisk(diskName, diskURI string, nodeName types.N
 					err, diskURI)
 				return nil
 			}
-			if retry.IsErrorRetriable(err) && c.cloud.CloudProviderBackoff {
-				klog.Warningf("azureDisk - update backing off: detach disk(%s, %s), err: %w", diskName, diskURI, err)
-				err = vmset.DetachDisk(nodeName, diskMap)
-			}
+			klog.Errorf("azureDisk - detach disk(%s, %s) failed, err: %v", diskName, diskURI, err)
+			return err
 		}
 	}
 
-	if err != nil {
-		klog.Errorf("azureDisk - detach disk(%s, %s) failed, err: %v", diskName, diskURI, err)
-		return err
-	}
-
 	klog.V(2).Infof("azureDisk - detach disk(%s, %s) succeeded", diskName, diskURI)
 	return nil
 }


Test patch:
diff --git a/pkg/provider/azure_controller_common_test.go b/pkg/provider/azure_controller_common_test.go
index 7344a340d..72f7d4b78 100644
--- a/pkg/provider/azure_controller_common_test.go
+++ b/pkg/provider/azure_controller_common_test.go
@@ -52,7 +52,8 @@ func TestCommonAttachDisk(t *testing.T) {
 	testTags[WriteAcceleratorEnabled] = to.StringPtr("true")
 	testCases := []struct {
 		desc            string
-		existedDisk     compute.Disk
+		diskName        string
+		existedDisk     *compute.Disk
 		nodeName        types.NodeName
 		vmList          map[string]string
 		isDataDisksFull bool
@@ -61,10 +62,20 @@ func TestCommonAttachDisk(t *testing.T) {
 		expectedErr     bool
 		expectedLun     int32
 	}{
+		{
+			desc:        "correct LUN and no error shall be returned if disk is nil",
+			vmList:      map[string]string{"vm1": "PowerState/Running"},
+			nodeName:    "vm1",
+			diskName:    "disk-name",
+			existedDisk: nil,
+			expectedLun: 1,
+			expectedErr: false,
+		},
 		{
 			desc:        "LUN -1 and error shall be returned if there's no such instance corresponding to given nodeName",
 			nodeName:    "vm1",
-			existedDisk: compute.Disk{Name: to.StringPtr("disk-name")},
+			diskName:    "disk-name",
+			existedDisk: &compute.Disk{Name: to.StringPtr("disk-name")},
 			expectedLun: -1,
 			expectedErr: true,
 		},
@@ -73,7 +84,8 @@ func TestCommonAttachDisk(t *testing.T) {
 			vmList:          map[string]string{"vm1": "PowerState/Running"},
 			nodeName:        "vm1",
 			isDataDisksFull: true,
-			existedDisk:     compute.Disk{Name: to.StringPtr("disk-name")},
+			diskName:        "disk-name",
+			existedDisk:     &compute.Disk{Name: to.StringPtr("disk-name")},
 			expectedLun:     -1,
 			expectedErr:     true,
 		},
@@ -81,7 +93,8 @@ func TestCommonAttachDisk(t *testing.T) {
 			desc:     "correct LUN and no error shall be returned if everything is good",
 			vmList:   map[string]string{"vm1": "PowerState/Running"},
 			nodeName: "vm1",
-			existedDisk: compute.Disk{Name: to.StringPtr("disk-name"),
+			diskName: "disk-name",
+			existedDisk: &compute.Disk{Name: to.StringPtr("disk-name"),
 				DiskProperties: &compute.DiskProperties{
 					Encryption: &compute.Encryption{DiskEncryptionSetID: &diskEncryptionSetID, Type: compute.EncryptionTypeEncryptionAtRestWithCustomerKey},
 					DiskSizeGB: to.Int32Ptr(4096),
@@ -95,7 +108,8 @@ func TestCommonAttachDisk(t *testing.T) {
 			desc:     "an error shall be returned if disk state is not Unattached",
 			vmList:   map[string]string{"vm1": "PowerState/Running"},
 			nodeName: "vm1",
-			existedDisk: compute.Disk{Name: to.StringPtr("disk-name"),
+			diskName: "disk-name",
+			existedDisk: &compute.Disk{Name: to.StringPtr("disk-name"),
 				DiskProperties: &compute.DiskProperties{
 					Encryption: &compute.Encryption{DiskEncryptionSetID: &diskEncryptionSetID, Type: compute.EncryptionTypeEncryptionAtRestWithCustomerKey},
 					DiskSizeGB: to.Int32Ptr(4096),
@@ -105,20 +119,12 @@ func TestCommonAttachDisk(t *testing.T) {
 			expectedLun: -1,
 			expectedErr: true,
 		},
-		{
-			desc:         "an error shall be returned if there's invalid disk uri",
-			vmList:       map[string]string{"vm1": "PowerState/Running"},
-			nodeName:     "vm1",
-			isBadDiskURI: true,
-			existedDisk:  compute.Disk{Name: to.StringPtr("disk-name")},
-			expectedLun:  -1,
-			expectedErr:  true,
-		},
 		{
 			desc:        "an error shall be returned if attach an already attached disk with good ManagedBy instance id",
 			vmList:      map[string]string{"vm1": "PowerState/Running"},
 			nodeName:    "vm1",
-			existedDisk: compute.Disk{Name: to.StringPtr("disk-name"), ManagedBy: to.StringPtr(goodInstanceID), DiskProperties: &compute.DiskProperties{MaxShares: &maxShare}},
+			diskName:    "disk-name",
+			existedDisk: &compute.Disk{Name: to.StringPtr("disk-name"), ManagedBy: to.StringPtr(goodInstanceID), DiskProperties: &compute.DiskProperties{MaxShares: &maxShare}},
 			expectedLun: -1,
 			expectedErr: true,
 		},
@@ -126,15 +132,8 @@ func TestCommonAttachDisk(t *testing.T) {
 			desc:        "an error shall be returned if attach an already attached disk with bad ManagedBy instance id",
 			vmList:      map[string]string{"vm1": "PowerState/Running"},
 			nodeName:    "vm1",
-			existedDisk: compute.Disk{Name: to.StringPtr("disk-name"), ManagedBy: to.StringPtr("test"), DiskProperties: &compute.DiskProperties{MaxShares: &maxShare}},
-			expectedLun: -1,
-			expectedErr: true,
-		},
-		{
-			desc:        "an error shall be returned if there's no matching disk",
-			vmList:      map[string]string{"vm1": "PowerState/Running"},
-			nodeName:    "vm1",
-			existedDisk: compute.Disk{Name: to.StringPtr("disk-name-1")},
+			diskName:    "disk-name",
+			existedDisk: &compute.Disk{Name: to.StringPtr("disk-name"), ManagedBy: to.StringPtr("test"), DiskProperties: &compute.DiskProperties{MaxShares: &maxShare}},
 			expectedLun: -1,
 			expectedErr: true,
 		},
@@ -151,10 +150,10 @@ func TestCommonAttachDisk(t *testing.T) {
 			lockMap:               newLockMap(),
 		}
 		diskURI := fmt.Sprintf("/subscriptions/%s/resourceGroups/%s/providers/Microsoft.Compute/disks/%s",
-			testCloud.SubscriptionID, testCloud.ResourceGroup, *test.existedDisk.Name)
+			testCloud.SubscriptionID, testCloud.ResourceGroup, test.diskName)
 		if test.isBadDiskURI {
 			diskURI = fmt.Sprintf("/baduri/subscriptions/%s/resourceGroups/%s/providers/Microsoft.Compute/disks/%s",
-				testCloud.SubscriptionID, testCloud.ResourceGroup, *test.existedDisk.Name)
+				testCloud.SubscriptionID, testCloud.ResourceGroup, test.diskName)
 		}
 		expectedVMs := setTestVirtualMachines(testCloud, test.vmList, test.isDataDisksFull)
 		if test.isDiskUsed {
@@ -170,11 +169,7 @@ func TestCommonAttachDisk(t *testing.T) {
 		}
 		mockVMsClient.EXPECT().Update(gomock.Any(), testCloud.ResourceGroup, gomock.Any(), gomock.Any(), gomock.Any()).Return(nil).AnyTimes()
 
-		mockDisksClient := testCloud.DisksClient.(*mockdiskclient.MockInterface)
-		mockDisksClient.EXPECT().Get(gomock.Any(), testCloud.ResourceGroup, "disk-name").Return(test.existedDisk, nil).AnyTimes()
-		mockDisksClient.EXPECT().Get(gomock.Any(), testCloud.ResourceGroup, gomock.Not("disk-name")).Return(compute.Disk{}, &retry.Error{HTTPStatusCode: http.StatusNotFound, RawError: cloudprovider.InstanceNotFound}).AnyTimes()
-
-		lun, err := common.AttachDisk(true, "", diskURI, test.nodeName, compute.CachingTypesReadOnly)
+		lun, err := common.AttachDisk(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)
 	}
@@ -194,7 +189,8 @@ func TestCommonAttachDiskWithVMSS(t *testing.T) {
 		isManagedDisk   bool
 		isDataDisksFull bool
 		expectedErr     bool
-		existedDisk     compute.Disk
+		diskName        string
+		existedDisk     *compute.Disk
 		expectedLun     int32
 	}{
 		{
@@ -204,7 +200,8 @@ func TestCommonAttachDiskWithVMSS(t *testing.T) {
 			isVMSS:        false,
 			isManagedBy:   false,
 			isManagedDisk: false,
-			existedDisk:   compute.Disk{Name: to.StringPtr("disk-name")},
+			diskName:      "disk-name",
+			existedDisk:   &compute.Disk{Name: to.StringPtr("disk-name")},
 			expectedLun:   -1,
 			expectedErr:   true,
 		},
@@ -215,7 +212,8 @@ func TestCommonAttachDiskWithVMSS(t *testing.T) {
 			isVMSS:        true,
 			isManagedBy:   false,
 			isManagedDisk: false,
-			existedDisk:   compute.Disk{Name: to.StringPtr("disk-name")},
+			diskName:      "disk-name",
+			existedDisk:   &compute.Disk{Name: to.StringPtr("disk-name")},
 			expectedLun:   -1,
 			expectedErr:   true,
 		},
@@ -254,7 +252,7 @@ func TestCommonAttachDiskWithVMSS(t *testing.T) {
 			lockMap:               newLockMap(),
 		}
 		diskURI := fmt.Sprintf("/subscriptions/%s/resourceGroups/%s/providers/Microsoft.Compute/disks/%s",
-			testCloud.SubscriptionID, testCloud.ResourceGroup, *test.existedDisk.Name)
+			testCloud.SubscriptionID, testCloud.ResourceGroup, test.diskName)
 		if !test.isVMSS {
 			expectedVMs := setTestVirtualMachines(testCloud, test.vmList, test.isDataDisksFull)
 			mockVMsClient := testCloud.VirtualMachinesClient.(*mockvmclient.MockInterface)
@@ -265,13 +263,9 @@ func TestCommonAttachDiskWithVMSS(t *testing.T) {
 				mockVMsClient.EXPECT().Get(gomock.Any(), testCloud.ResourceGroup, gomock.Any(), gomock.Any()).Return(compute.VirtualMachine{}, &retry.Error{HTTPStatusCode: http.StatusNotFound, RawError: cloudprovider.InstanceNotFound}).AnyTimes()
 			}
 			mockVMsClient.EXPECT().Update(gomock.Any(), testCloud.ResourceGroup, gomock.Any(), gomock.Any(), gomock.Any()).Return(nil).AnyTimes()
-
-			mockDisksClient := testCloud.DisksClient.(*mockdiskclient.MockInterface)
-			mockDisksClient.EXPECT().Get(gomock.Any(), testCloud.ResourceGroup, "disk-name").Return(test.existedDisk, nil).AnyTimes()
-			mockDisksClient.EXPECT().Get(gomock.Any(), testCloud.ResourceGroup, gomock.Not("disk-name")).Return(compute.Disk{}, &retry.Error{HTTPStatusCode: http.StatusNotFound, RawError: cloudprovider.InstanceNotFound}).AnyTimes()
 		}
 
-		lun, err := common.AttachDisk(test.isManagedDisk, "test", diskURI, test.nodeName, compute.CachingTypesReadOnly)
+		lun, err := common.AttachDisk(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)
 	}