fix detach azure disk back off issue which has too big lock in failure retry condition #76573
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
k8s-ci-robot
added
release-note
k8s-ci-robot
added
area/cloudprovider
sig/cloud-provider
andyzhangx
force-pushed
the
disk-backoff-refactor
branch
from
299ef42 to
3772cd8
Compare
k8s-ci-robot
removed
the
approved
feiskyer
reviewed
Apr 15, 2019
fix comments fix import keymux check error add unit test for attach/detach disk funcs
andyzhangx
force-pushed
the
disk-backoff-refactor
branch
from
3772cd8 to
6c70ca6
Compare
Member
Author
|
/test pull-kubernetes-integration |
k8s-ci-robot
added
the
lgtm
Member
|
@andrewsykim Could you help to approve the cloud-provider changes? /assign @andrewsykim |
Member
|
Can we add a bit more details on the release note please? |
andyzhangx
changed the title
Member
Author
|
/test pull-kubernetes-e2e-gce-csi-serial |
Member
Author
|
@andrewsykim thanks. I have changed the release note to: Let me know if you have any question. |
Member
Author
|
@andrewsykim PTAL, thanks. |
Member
|
/approve for |
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: andrewsykim, andyzhangx, feiskyer The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
This was referenced Apr 21, 2019
k8s-ci-robot
added a commit
that referenced
this pull request
Apr 28, 2019
…6573-upstream-release-1.12 Automated cherry pick of #76573: refactor detach azure disk retry operation
k8s-ci-robot
added a commit
that referenced
this pull request
Apr 30, 2019
…6573-upstream-release-1.13 Automated cherry pick of #76573: refactor detach azure disk retry operation
k8s-ci-robot
added a commit
that referenced
this pull request
May 1, 2019
…6573-upstream-release-1.14 Automated cherry pick of #76573: refactor detach azure disk retry operation
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What type of PR is this?
/kind bug
What this PR does / why we need it:
In some error condition when detach azure disk failed, azure cloud provider will retry 6 times at most with exponential backoff, it will hold the data disk list for about 3 minutes with a node level lock, and in that time period, if customer update data disk list manually (e.g. need manual operationto attach/detach another disk since there is attach/detach error, ) , the data disk list will be obselete(dirty data), then weird VM status happens, e.g. attach a non-existing disk, we should split those retry operations, every retry should get a fresh data disk list in the beginning.
kubernetes/pkg/cloudprovider/providers/azure/azure_controller_standard.go
Lines 150 to 153 in cbaaa67
This PR has two commits:
DetachDiskByNametoDetachDiskas.CreateOrUpdateVMWithRetry(nodeResourceGroup, vmName, newVM)which may lead to obsolete data disk listThus there would be separate lock for every detach azure disk operation, we don't need to lock whole disk detach backoff(6 retries) process.
This PR don't change the logic of attach disk since there is no retry in azure cloud provide for attach disk, k8s attach-detach controller will do the attach volume retry.
BTW, I have tested this PR on both vmss and vmas k8s cluster by a stress disk attach/detach test, it works well for a bunch of times.
Which issue(s) this PR fixes:
Fixes #76502
Special notes for your reviewer:
Does this PR introduce a user-facing change?:
/kind bug
/assign @feiskyer
/priority important-soon
/sig azure
cc @khenidak @brendandburns