fix race condition issue when detaching azure disk #60183

andyzhangx · 2018-02-22T03:39:59Z

What this PR does / why we need it:
add lock before detaching azure disk, without this PR, there would be lots of Multi-Attach error when scheduling one pod from one node to another.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #60101

Special notes for your reviewer:
@feiskyer @djsly @khenidak
Since we are using getLunMutex.LockKey(instanceid) for both AttachDisk and DetachDisk, there would be only one VM.update operation at a time for both AttachDisk and DetachDisk.

Release note:

fix race condition issue when detaching azure disk

/assign @feiskyer
Could you also mark as v1.10 milestone @feiskyer thanks.
/sig azure

andyzhangx · 2018-02-22T03:44:08Z

@khenidak I think lots of Multi-Attach error of azure disk was due to this bug, it's really bad we do not lock before detaching azure disk...

fix build error

djsly · 2018-02-22T04:05:09Z

Looking good on my side.
I will apply the patch on my local 1.7 source and test it out tomorrow. We have another issue with PV and I want to confirm it is the same source.

andyzhangx · 2018-02-22T04:19:20Z

@djsly just replace the controller manager, thanks.

feiskyer

LGTM with the changes.

Let's wait a while for @djsly's validation.

brendandburns · 2018-02-22T05:08:25Z

pkg/volume/azure_dd/attacher.go

@@ -268,7 +268,7 @@ func (d *azureDiskDetacher) Detach(diskURI string, nodeName types.NodeName) erro
 		return fmt.Errorf("invalid disk to detach: %q", diskURI)
 	}

-	_, err := d.cloud.InstanceID(context.TODO(), nodeName)
+	instanceid, err := d.cloud.InstanceID(context.TODO(), nodeName)


context.TODO() seems like a mistake here? is there a context we can attach to?

@brendanburns it's related to #59287, there is some future work item for context.TODO()

djsly · 2018-02-22T15:39:07Z

@andyzhangx here are the logs for my latest test after I applied the patch on the 1.7 branch.

https://gist.github.com/djsly/d04edbd3239373f345ca27616ddd5132

Things are looking good!

Except the Multi-Attach error. This seems to be caused by the reconciler. It last ~2m30sec until the disks are unmounted and the first detach kicks in.

Could you confirm that those warnings do not involve the ARM api ?

Multi-Attach error for volume "pvc-91d3b0a6-11b9-11e8-82b7-000d3a018ac3" (UniqueName: "kubernetes.io/azure-disk//subscriptions/<sub_id>/resourceGroups/<rg>/providers/Microsoft.Compute/disks/<rg>-dynamic-pvc-91d3b0a6-11b9-11e8-82b7-000d3a018ac3") from node "kn-edge-2" Volume is already exclusively attached to one node and can't be attached to another

djsly · 2018-02-22T15:40:06Z

Also, could we have this PR cherry picked all the way to 1.7 ? thanks!

brendandburns · 2018-02-22T16:26:57Z

/lgtm
/approve

@jdumars can you shepard this into 1.7.x and 1.8.x?

Thanks!

k8s-ci-robot · 2018-02-22T16:27:07Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andyzhangx, brendandburns

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:

~~pkg/volume/azure_dd/OWNERS~~ [andyzhangx,brendandburns]

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

k8s-cherrypick-bot · 2018-02-22T16:29:20Z

Removing label cherrypick-candidate because no release milestone was set. This is an invalid state and thus this PR is not being considered for cherry-pick to any release branch. Please add an appropriate release milestone and then re-add the label.

andyzhangx · 2018-02-23T02:57:02Z

@djsly I checked the code again, current Multi-Attach error from reconciler is expected, it does not come from ARM api, it's a error message generated by reconciler.
When one pod is rescheduled from node#1 to node#2, DisksAreAttached func in azure_controllerCommon.go will still return that the original disk is still in node#1, after around one minute, disk was removed from node#1, and then the Multi-Attach error from reconciler disappears.

I will cherry pick this PR to v1.7-1.9.
@djsly Thanks again for your reporting and verification, finally we resolved this Multi-Attach error issue.

djsly · 2018-02-23T03:11:08Z

My pleasure and thanks for the quick PR.

andyzhangx · 2018-02-23T03:39:47Z

@karataliu @rootfs FYI

k8s-github-robot · 2018-02-23T07:17:34Z

Automatic merge from submit-queue (batch tested with PRs 60208, 60084, 60183, 59713, 60096). If you want to cherry-pick this change to another branch, please follow the instructions here.

jdumars · 2018-02-23T14:57:11Z

@mehdy @jpbetz @wojtek-t this is an important cherry pick

k8s-cherrypick-bot · 2018-02-23T14:57:32Z

Removing label cherrypick-candidate because no release milestone was set. This is an invalid state and thus this PR is not being considered for cherry-pick to any release branch. Please add an appropriate release milestone and then re-add the label.

…0183-upstream-release-1.8 Automatic merge from submit-queue. Automated cherry pick of #60183 Cherry pick of #60183 on release-1.8. #60183: add lock before detaching azure disk

…0183-upstream-release-1.7 Automatic merge from submit-queue. Automated cherry pick of #60183 Cherry pick of #60183 on release-1.7. #60183: add lock before detaching azure disk

…0183-upstream-release-1.9 Automatic merge from submit-queue. Automated cherry pick of #60183: add lock before detaching azure disk Cherry pick of #60183 on release-1.9. #60183: add lock before detaching azure disk

k8s-cherrypick-bot · 2018-03-16T06:18:27Z

Commit found in the "release-1.9" branch appears to be this PR. Removing the "cherrypick-candidate" label. If this is an error find help to get your PR picked.

k8s-ci-robot assigned feiskyer Feb 22, 2018

k8s-ci-robot requested review from jsafrane and khenidak February 22, 2018 03:40

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 22, 2018

feiskyer added this to the v1.10 milestone Feb 22, 2018

feiskyer added status/approved-for-milestone kind/bug Categorizes issue or PR as related to a bug. labels Feb 22, 2018

add lock before detaching azure disk

f3324a6

fix build error

andyzhangx force-pushed the addlock-detach-azuredisk branch from 630269a to f3324a6 Compare February 22, 2018 03:55

feiskyer reviewed Feb 22, 2018

View reviewed changes

brendandburns reviewed Feb 22, 2018

View reviewed changes

k8s-ci-robot assigned brendandburns Feb 22, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 22, 2018

brendandburns added the cherrypick-candidate label Feb 22, 2018

k8s-cherrypick-bot removed the cherrypick-candidate label Feb 22, 2018

This was referenced Feb 23, 2018

Automated cherry pick of #60183 #60271

Merged

Automated cherry pick of #60183 #60272

Merged

Automated cherry pick of #60183: add lock before detaching azure disk #60273

Merged

Automated cherry pick of #60183 #60277

Closed

k8s-github-robot merged commit 8f9e8c0 into kubernetes:master Feb 23, 2018

jdumars added the cherrypick-candidate label Feb 23, 2018

k8s-cherrypick-bot removed the cherrypick-candidate label Feb 23, 2018

jdumars modified the milestones: v1.10, v1.9 Feb 23, 2018

jdumars added the cherrypick-candidate label Feb 23, 2018

This was referenced Feb 25, 2018

Busy azure-disk regularly fail to mount causing K8S Pod deployments to halt. Azure/ACS#12

Open

Since Intel CPU Azure update, new Azure Disks are not mounting, very critical... Azure/acs-engine#2002

Closed

oivindoh mentioned this pull request Mar 13, 2018

Is production ready? Azure/acs-engine#2422

Closed

j03wang mentioned this pull request Mar 14, 2018

Windows support for Kubernetes v1.7.14, v1.8.9, and v1.9.4 Azure/acs-engine#2471

Merged

k8s-cherrypick-bot removed the cherrypick-candidate label Mar 16, 2018

andyzhangx mentioned this pull request Apr 3, 2018

Azure disk fails to attach and mount, causing rescheduled pod to stall following node disruption #46421

Closed

r7vme mentioned this pull request Apr 3, 2018

Bump to 1.9.6 giantswarm/giantnetes-terraform#57

Merged

andyzhangx mentioned this pull request Apr 4, 2018

upgrade k8s process is broke Azure/acs-engine#2022

Closed

andyzhangx mentioned this pull request Apr 20, 2018

Somehow two containers, in different namespaces, have the same AzureDisk mounted. Azure/AKS#110

Closed

andyzhangx mentioned this pull request Jun 22, 2018

Input/output error when accessing PV Azure/AKS#297

Closed

This was referenced Jun 25, 2019

A Tutorial Introduction to Kubernetes guevara/read-it-later#4572

Open

A Tutorial Introduction to Kubernetes guevara/read-it-later#4573

Open

A Tutorial Introduction to Kubernetes guevara/read-it-later#4574

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix race condition issue when detaching azure disk #60183

fix race condition issue when detaching azure disk #60183

andyzhangx commented Feb 22, 2018 •

edited

Loading

andyzhangx commented Feb 22, 2018

djsly commented Feb 22, 2018

andyzhangx commented Feb 22, 2018

feiskyer left a comment

brendandburns Feb 22, 2018

andyzhangx Feb 22, 2018

djsly commented Feb 22, 2018

djsly commented Feb 22, 2018

brendandburns commented Feb 22, 2018

k8s-ci-robot commented Feb 22, 2018

k8s-cherrypick-bot commented Feb 22, 2018

andyzhangx commented Feb 23, 2018

djsly commented Feb 23, 2018

andyzhangx commented Feb 23, 2018

k8s-github-robot commented Feb 23, 2018

jdumars commented Feb 23, 2018

k8s-cherrypick-bot commented Feb 23, 2018

k8s-cherrypick-bot commented Mar 16, 2018

fix race condition issue when detaching azure disk #60183

fix race condition issue when detaching azure disk #60183

Conversation

andyzhangx commented Feb 22, 2018 • edited Loading

andyzhangx commented Feb 22, 2018

djsly commented Feb 22, 2018

andyzhangx commented Feb 22, 2018

feiskyer left a comment

Choose a reason for hiding this comment

brendandburns Feb 22, 2018

Choose a reason for hiding this comment

andyzhangx Feb 22, 2018

Choose a reason for hiding this comment

djsly commented Feb 22, 2018

djsly commented Feb 22, 2018

brendandburns commented Feb 22, 2018

k8s-ci-robot commented Feb 22, 2018

k8s-cherrypick-bot commented Feb 22, 2018

andyzhangx commented Feb 23, 2018

djsly commented Feb 23, 2018

andyzhangx commented Feb 23, 2018

k8s-github-robot commented Feb 23, 2018

jdumars commented Feb 23, 2018

k8s-cherrypick-bot commented Feb 23, 2018

k8s-cherrypick-bot commented Mar 16, 2018

andyzhangx commented Feb 22, 2018 •

edited

Loading