Retry if response StatusCode 200 and ContentLength -1 #2298

lzhecheng · 2022-09-13T06:49:40Z

Signed-off-by: Zhecheng Li zhechengli@microsoft.com

What type of PR is this?

/kind bug

What this PR does / why we need it:

ARM will possibly return StatusCode 200 and ContentLength -1 which is a bug. However, cloudprovider can handle it better by retrying.

Which issue(s) this PR fixes:

Fixes #2296

Special notes for your reviewer:

Does this PR introduce a user-facing change?

ARM will possibly return StatusCode 200 and ContentLength -1 which is a bug. However, cloudprovider can handle it better by retrying.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

netlify · 2022-09-13T06:49:51Z

✅ Deploy Preview for kubernetes-sigs-cloud-provide-azure canceled.

Name	Link
🔨 Latest commit	`d2a175d`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-cloud-provide-azure/deploys/6323f673300aed0009ee1f03

andyzhangx · 2022-09-13T09:13:55Z

pkg/azureclients/armclient/util.go

 				return response, rerr
 			}
+			if regionalResponse.StatusCode == 200 && (regionalResponse.ContentLength == -1 || regionalResponse.ContentLength == 0) {


it's not response.StatusCode >= 200 && response.StatusCode < 300 ?

let's replace regionalResponse.StatusCode == 200 with response.StatusCode >= 200 && response.StatusCode < 300 to keep align with above condition.

feiskyer · 2022-09-14T04:26:24Z

pkg/azureclients/armclient/util.go

 				return response, rerr
 			}
+			if regionalResponse.StatusCode == 200 && (regionalResponse.ContentLength == -1 || regionalResponse.ContentLength == 0) {
+				contentLengthErrStr := fmt.Sprintf("Send.sendRegionalRequest failed to get response from regional host, error: StatusCode is 200 but ContentLength is %d. Ignoring the result.",


how about changing the error message simpler, e.g. fmt.Sprintf("empty response with ContentLength %d and StatusCode %d", regionalResponse.ContentLength, response.StatusCode)?

feiskyer · 2022-09-14T04:26:42Z

pkg/azureclients/armclient/util.go

@@ -123,7 +123,8 @@ func DoHackRegionalRetryDecorator(c *Client) autorest.SendDecorator {
 			// Hack: retry the regional ARM endpoint in case of ARM traffic split and arm resource group replication is too slow
 			// Empty content and 2xx http status code are returned in this case.
 			// Issue: https://github.com/kubernetes-sigs/cloud-provider-azure/issues/1296
-			emptyResp := (response.ContentLength == 0 || trimmed == "" || trimmed == "{}") && response.StatusCode >= 200 && response.StatusCode < 300
+			// Update: ContentLength == -1 is also considered because ARM can possibly return ContentLength -1 and StatusCode 200.
+			emptyResp := (response.ContentLength == 0 || response.ContentLength == -1 || trimmed == "" || trimmed == "{}") && response.StatusCode >= 200 && response.StatusCode < 300


could you add some unit tests for the new retry logic?

coveralls · 2022-09-14T06:55:13Z

Coverage decreased (-0.004%) to 80.327% when pulling 61983c0 on lzhecheng:statuscode-200-contentlength-minus-1 into 3fb3df7 on kubernetes-sigs:master.

lzhecheng · 2022-09-15T03:04:23Z

/retest

lzhecheng · 2022-09-15T10:07:30Z

/test pull-cloud-provider-azure-e2e-capz

lzhecheng · 2022-09-15T11:13:45Z

/retest

lzhecheng · 2022-09-15T15:41:08Z

ContentLength -1 is kind of different from ContentLength 0. This PR is dealing with a situation satisfying all 3 conditions: ContentLength -1, StatusCode 200 and empty response body. For ContentLength 0, its relation with empty response body is OR.

lzhecheng · 2022-09-15T22:55:22Z

/assign @feiskyer Please take another look.

pkg/azureclients/armclient/util.go

ARM will possibly return StatusCode 200 and ContentLength -1 which is a bug. However, cloudprovider can handle it better by retrying. Signed-off-by: Zhecheng Li <zhechengli@microsoft.com>

feiskyer

/lgtm

k8s-ci-robot · 2022-09-16T05:56:09Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: feiskyer, lzhecheng

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~~ [feiskyer,lzhecheng]

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

lzhecheng · 2022-09-16T06:51:46Z

/cherrypick release-1.24

lzhecheng · 2022-09-16T06:51:50Z

/cherrypick release-1.23

lzhecheng · 2022-09-16T06:51:54Z

/cherrypick release-1.1

lzhecheng · 2022-09-16T06:51:59Z

/cherrypick release-1.25

k8s-infra-cherrypick-robot · 2022-09-16T06:52:22Z

@lzhecheng: #2298 failed to apply on top of branch "release-1.24":

Applying: Retry if response StatusCode 200 and ContentLength -1 ARM will possibly return StatusCode 200 and ContentLength -1 which is a bug. However, cloudprovider can handle it better by retrying.
Using index info to reconstruct a base tree...
M	pkg/azureclients/armclient/azure_armclient_test.go
M	pkg/azureclients/armclient/util.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/azureclients/armclient/util.go
CONFLICT (content): Merge conflict in pkg/azureclients/armclient/util.go
Auto-merging pkg/azureclients/armclient/azure_armclient_test.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Retry if response StatusCode 200 and ContentLength -1 ARM will possibly return StatusCode 200 and ContentLength -1 which is a bug. However, cloudprovider can handle it better by retrying.
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherrypick release-1.24

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.

k8s-infra-cherrypick-robot · 2022-09-16T06:52:27Z

@lzhecheng: #2298 failed to apply on top of branch "release-1.23":

Applying: Retry if response StatusCode 200 and ContentLength -1 ARM will possibly return StatusCode 200 and ContentLength -1 which is a bug. However, cloudprovider can handle it better by retrying.
Using index info to reconstruct a base tree...
M	pkg/azureclients/armclient/azure_armclient_test.go
A	pkg/azureclients/armclient/util.go
Falling back to patching base and 3-way merge...
CONFLICT (modify/delete): pkg/azureclients/armclient/util.go deleted in HEAD and modified in Retry if response StatusCode 200 and ContentLength -1 ARM will possibly return StatusCode 200 and ContentLength -1 which is a bug. However, cloudprovider can handle it better by retrying.. Version Retry if response StatusCode 200 and ContentLength -1 ARM will possibly return StatusCode 200 and ContentLength -1 which is a bug. However, cloudprovider can handle it better by retrying. of pkg/azureclients/armclient/util.go left in tree.
Auto-merging pkg/azureclients/armclient/azure_armclient_test.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Retry if response StatusCode 200 and ContentLength -1 ARM will possibly return StatusCode 200 and ContentLength -1 which is a bug. However, cloudprovider can handle it better by retrying.
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherrypick release-1.23

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.

k8s-infra-cherrypick-robot · 2022-09-16T06:52:31Z

@lzhecheng: #2298 failed to apply on top of branch "release-1.1":

Applying: Retry if response StatusCode 200 and ContentLength -1 ARM will possibly return StatusCode 200 and ContentLength -1 which is a bug. However, cloudprovider can handle it better by retrying.
Using index info to reconstruct a base tree...
M	pkg/azureclients/armclient/azure_armclient_test.go
A	pkg/azureclients/armclient/util.go
Falling back to patching base and 3-way merge...
CONFLICT (modify/delete): pkg/azureclients/armclient/util.go deleted in HEAD and modified in Retry if response StatusCode 200 and ContentLength -1 ARM will possibly return StatusCode 200 and ContentLength -1 which is a bug. However, cloudprovider can handle it better by retrying.. Version Retry if response StatusCode 200 and ContentLength -1 ARM will possibly return StatusCode 200 and ContentLength -1 which is a bug. However, cloudprovider can handle it better by retrying. of pkg/azureclients/armclient/util.go left in tree.
Auto-merging pkg/azureclients/armclient/azure_armclient_test.go
CONFLICT (content): Merge conflict in pkg/azureclients/armclient/azure_armclient_test.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Retry if response StatusCode 200 and ContentLength -1 ARM will possibly return StatusCode 200 and ContentLength -1 which is a bug. However, cloudprovider can handle it better by retrying.
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherrypick release-1.1

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.

k8s-infra-cherrypick-robot · 2022-09-16T06:52:36Z

@lzhecheng: new pull request created: #2315

In response to this:

/cherrypick release-1.25

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.

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/bug Categorizes issue or PR as related to a bug. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Sep 13, 2022

k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Sep 13, 2022

k8s-ci-robot requested review from andyzhangx and feiskyer September 13, 2022 06:50

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 13, 2022

andyzhangx reviewed Sep 13, 2022

View reviewed changes

feiskyer reviewed Sep 14, 2022

View reviewed changes

lzhecheng force-pushed the statuscode-200-contentlength-minus-1 branch from bd2cfbb to 1d505d2 Compare September 14, 2022 06:50

lzhecheng force-pushed the statuscode-200-contentlength-minus-1 branch 3 times, most recently from d7b0041 to 00b0352 Compare September 15, 2022 08:54

lzhecheng force-pushed the statuscode-200-contentlength-minus-1 branch from 00b0352 to 5d9ab56 Compare September 15, 2022 14:17

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Sep 15, 2022

lzhecheng force-pushed the statuscode-200-contentlength-minus-1 branch from 5d9ab56 to 61983c0 Compare September 15, 2022 14:24

feiskyer reviewed Sep 16, 2022

View reviewed changes

pkg/azureclients/armclient/util.go Outdated Show resolved Hide resolved

andyzhangx reviewed Sep 16, 2022

View reviewed changes

pkg/azureclients/armclient/util.go Outdated Show resolved Hide resolved

lzhecheng force-pushed the statuscode-200-contentlength-minus-1 branch from 61983c0 to 38792b7 Compare September 16, 2022 03:08

Retry if response StatusCode 200 and ContentLength -1

d2a175d

ARM will possibly return StatusCode 200 and ContentLength -1 which is a bug. However, cloudprovider can handle it better by retrying. Signed-off-by: Zhecheng Li <zhechengli@microsoft.com>

lzhecheng force-pushed the statuscode-200-contentlength-minus-1 branch from 38792b7 to d2a175d Compare September 16, 2022 04:07

feiskyer approved these changes Sep 16, 2022

View reviewed changes

k8s-ci-robot assigned feiskyer Sep 16, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 16, 2022

k8s-ci-robot merged commit 08d5cd2 into kubernetes-sigs:master Sep 16, 2022

lzhecheng deleted the statuscode-200-contentlength-minus-1 branch September 16, 2022 06:51

k8s-infra-cherrypick-robot mentioned this pull request Sep 16, 2022

[release-1.25] Retry if response StatusCode 200 and ContentLength -1 #2315

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Retry if response StatusCode 200 and ContentLength -1 #2298

Retry if response StatusCode 200 and ContentLength -1 #2298

lzhecheng commented Sep 13, 2022 •

edited

netlify bot commented Sep 13, 2022 •

edited

andyzhangx Sep 13, 2022

feiskyer Sep 14, 2022

lzhecheng Sep 15, 2022

feiskyer Sep 14, 2022

lzhecheng Sep 15, 2022

feiskyer Sep 14, 2022

lzhecheng Sep 15, 2022

coveralls commented Sep 14, 2022 •

edited

lzhecheng commented Sep 15, 2022

lzhecheng commented Sep 15, 2022

lzhecheng commented Sep 15, 2022

lzhecheng commented Sep 15, 2022

lzhecheng commented Sep 15, 2022

feiskyer left a comment

k8s-ci-robot commented Sep 16, 2022

lzhecheng commented Sep 16, 2022

lzhecheng commented Sep 16, 2022

lzhecheng commented Sep 16, 2022

lzhecheng commented Sep 16, 2022

k8s-infra-cherrypick-robot commented Sep 16, 2022

k8s-infra-cherrypick-robot commented Sep 16, 2022

k8s-infra-cherrypick-robot commented Sep 16, 2022

k8s-infra-cherrypick-robot commented Sep 16, 2022

Retry if response StatusCode 200 and ContentLength -1 #2298

Retry if response StatusCode 200 and ContentLength -1 #2298

Conversation

lzhecheng commented Sep 13, 2022 • edited

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

netlify bot commented Sep 13, 2022 • edited

✅ Deploy Preview for kubernetes-sigs-cloud-provide-azure canceled.

andyzhangx Sep 13, 2022

Choose a reason for hiding this comment

feiskyer Sep 14, 2022

Choose a reason for hiding this comment

lzhecheng Sep 15, 2022

Choose a reason for hiding this comment

feiskyer Sep 14, 2022

Choose a reason for hiding this comment

lzhecheng Sep 15, 2022

Choose a reason for hiding this comment

feiskyer Sep 14, 2022

Choose a reason for hiding this comment

lzhecheng Sep 15, 2022

Choose a reason for hiding this comment

coveralls commented Sep 14, 2022 • edited

lzhecheng commented Sep 15, 2022

lzhecheng commented Sep 15, 2022

lzhecheng commented Sep 15, 2022

lzhecheng commented Sep 15, 2022

lzhecheng commented Sep 15, 2022

feiskyer left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Sep 16, 2022

lzhecheng commented Sep 16, 2022

lzhecheng commented Sep 16, 2022

lzhecheng commented Sep 16, 2022

lzhecheng commented Sep 16, 2022

k8s-infra-cherrypick-robot commented Sep 16, 2022

k8s-infra-cherrypick-robot commented Sep 16, 2022

k8s-infra-cherrypick-robot commented Sep 16, 2022

k8s-infra-cherrypick-robot commented Sep 16, 2022

lzhecheng commented Sep 13, 2022 •

edited

netlify bot commented Sep 13, 2022 •

edited

coveralls commented Sep 14, 2022 •

edited