Repository: kubernetes-sigs/cloud-provider-azure
Pull request #757 (2021-08-20 13:50:08)

fix: ignore GetDisk throttling in DeleteDisk
<!--  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: ignore GetDisk throttling in DeleteDisk

**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**:
```
 1 controllerserver.go:378] delete azure disk(/subscriptions/xxx/resourceGroups/aks55h93-nodegroup/providers/Microsoft.Compute/disks/pvc-ecfefbb9-102c-4f40-944f-d5d4b6183568) returned with Retriable: true, RetryAfter: 0s, HTTPStatusCode: 0, RawError: azure cloud provider rate limited(read) for operation "GetDisk"
 1 utils.go:100] GRPC error: Retriable: true, RetryAfter: 0s, HTTPStatusCode: 0, RawError: azure cloud provider rate limited(read) for operation "GetDisk"
```

**Release note**:
```
fix: ignore GetDisk throttling in DeleteDisk
```


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/757#" 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":[]} -->
coveralls: 
[![Coverage Status](https://coveralls.io/builds/42298717/badge)](https://coveralls.io/builds/42298717)

Coverage decreased (-0.002%) to 80.584% when pulling **cab0faf33fa707bfda855fdd14712b6378a37aa6 on andyzhangx:deletedisk-throttling** into **eee45743a454b245b781ad11cd86edd903d28238 on kubernetes-sigs:master**.

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

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
pull-cloud-provider-azure-e2e-ccm-capz | cab0faf33fa707bfda855fdd14712b6378a37aa6 | [link](https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_cloud-provider-azure/757/pull-cloud-provider-azure-e2e-ccm-capz/1428715981814894592) | `/test pull-cloud-provider-azure-e2e-ccm-capz`

[Full PR test history](https://prow.k8s.io/pr-history?org=kubernetes-sigs&repo=cloud-provider-azure&pr=757). [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 -->

Patch:
diff --git a/pkg/consts/consts.go b/pkg/consts/consts.go
index 287472e55..20f6b532d 100644
--- a/pkg/consts/consts.go
+++ b/pkg/consts/consts.go
@@ -352,3 +352,6 @@ const (
 	DefaultCloudProviderConfigSecNamespace = "kube-system"
 	DefaultCloudProviderConfigSecKey       = "cloud-config"
 )
+
+// RateLimited error string
+const RateLimited = "rate limited"
diff --git a/pkg/provider/azure_managedDiskController.go b/pkg/provider/azure_managedDiskController.go
index 548c3e09d..a7df010a2 100644
--- a/pkg/provider/azure_managedDiskController.go
+++ b/pkg/provider/azure_managedDiskController.go
@@ -251,35 +251,33 @@ func (c *ManagedDiskController) CreateManagedDisk(options *ManagedDiskOptions) (
 }
 
 //DeleteManagedDisk : delete managed disk
-func (c *ManagedDiskController) DeleteManagedDisk(diskURI string) error {
-	diskName := path.Base(diskURI)
+func (c *ManagedDiskController) DeleteManagedDisk(ctx context.Context, diskURI string) error {
 	resourceGroup, err := getResourceGroupFromDiskURI(diskURI)
 	if err != nil {
 		return err
 	}
 
-	ctx, cancel := getContextWithCancel()
-	defer cancel()
-
 	if _, ok := c.common.diskStateMap.Load(strings.ToLower(diskURI)); ok {
 		return fmt.Errorf("failed to delete disk(%s) since it's in attaching or detaching state", diskURI)
 	}
 
+	diskName := path.Base(diskURI)
 	disk, rerr := c.common.cloud.DisksClient.Get(ctx, resourceGroup, diskName)
 	if rerr != nil {
 		if rerr.HTTPStatusCode == http.StatusNotFound {
 			klog.V(2).Infof("azureDisk - disk(%s) is already deleted", diskURI)
 			return nil
 		}
-		return rerr.Error()
+		// ignore GetDisk throttling
+		if !rerr.IsThrottled() && !strings.Contains(rerr.RawError.Error(), consts.RateLimited) {
+			return rerr.Error()
+		}
 	}
-
 	if disk.ManagedBy != nil {
 		return fmt.Errorf("disk(%s) already attached to node(%s), could not be deleted", diskURI, *disk.ManagedBy)
 	}
 
-	rerr = c.common.cloud.DisksClient.Delete(ctx, resourceGroup, diskName)
-	if rerr != nil {
+	if rerr := c.common.cloud.DisksClient.Delete(ctx, resourceGroup, diskName); rerr != nil {
 		return rerr.Error()
 	}
 	// We don't need poll here, k8s will immediately stop referencing the disk
diff --git a/pkg/retry/azure_error.go b/pkg/retry/azure_error.go
index 3c03c444c..33e935591 100644
--- a/pkg/retry/azure_error.go
+++ b/pkg/retry/azure_error.go
@@ -34,6 +34,9 @@ import (
 	"sigs.k8s.io/cloud-provider-azure/pkg/consts"
 )
 
+// RateLimited error string
+const RateLimited = "rate limited"
+
 var (
 	// The function to get current time.
 	now = time.Now
@@ -134,7 +137,7 @@ func GetRateLimitError(isWrite bool, opName string) *Error {
 	if isWrite {
 		opType = "write"
 	}
-	return GetRetriableError(fmt.Errorf("azure cloud provider rate limited(%s) for operation %q", opType, opName))
+	return GetRetriableError(fmt.Errorf("azure cloud provider %s(%s) for operation %q", RateLimited, opType, opName))
 }
 
 // GetThrottlingError creates a new error for throttling.


Test patch:
diff --git a/pkg/provider/azure_managedDiskController_test.go b/pkg/provider/azure_managedDiskController_test.go
index b00d80622..931f0f831 100644
--- a/pkg/provider/azure_managedDiskController_test.go
+++ b/pkg/provider/azure_managedDiskController_test.go
@@ -314,6 +314,8 @@ func TestDeleteManagedDisk(t *testing.T) {
 			expectedErrMsg: fmt.Errorf("Retriable: false, RetryAfter: 0s, HTTPStatusCode: 0, RawError: Get Disk failed"),
 		},
 	}
+	ctx, cancel := getContextWithCancel()
+	defer cancel()
 
 	for i, test := range testCases {
 		testCloud := GetTestCloud(ctrl)
@@ -332,7 +334,7 @@ func TestDeleteManagedDisk(t *testing.T) {
 		}
 		mockDisksClient.EXPECT().Delete(gomock.Any(), testCloud.ResourceGroup, test.diskName).Return(nil).AnyTimes()
 
-		err := managedDiskController.DeleteManagedDisk(diskURI)
+		err := managedDiskController.DeleteManagedDisk(ctx, diskURI)
 		assert.Equal(t, test.expectedErr, err != nil, "TestCase[%d]: %s, return error: %v", i, test.desc, err)
 		if test.expectedErr {
 			assert.EqualError(t, test.expectedErrMsg, err.Error(), "TestCase[%d]: %s, expected: %v, return: %v", i, test.desc, test.expectedErrMsg, err)
diff --git a/pkg/retry/azure_error_test.go b/pkg/retry/azure_error_test.go
index e1ec91e1c..023366ace 100644
--- a/pkg/retry/azure_error_test.go
+++ b/pkg/retry/azure_error_test.go
@@ -342,6 +342,12 @@ func TestIsThrottled(t *testing.T) {
 			},
 			expected: true,
 		},
+		{
+			err: &Error{
+				RetryAfter: time.Now().Add(0),
+			},
+			expected: true,
+		},
 	}
 
 	for _, test := range tests {