/retest
/approve

I do not fully agree with this PR. We had lots of problems without this detachdisk. If you delete this detachdisk from code, it means that if you have pod running in server1, server1 is shutdowned -> the pod cannot move to server2 without someone going and release the drive manually

please test it carefully before removing things like this. In bad case you are breaking someone production because the failover does not work anymore like it worked before.

/approve cancel

Let's get @anguslees 's opinion here too

@zetaab shouldn't the attach/detach controller in the controller manager be handling detach logic? I'm confused why you have to manually detach. Are you running an older version of the control plane?

well we have 1.7, but i have not tested it without that detachdisk line. We have another environment which is using openshift 3.5 (kubernetes 1.5) and we have to detach drives manually once in a week. Anyways I would say that another pr made by @jamiehannaford looks better than just removing this line(how well it is tested?).

The logic of AttachDisk in all other attachable volume plugins (e.g., GCE PD, AWS EBS) is that if it is already attached to a different instance, it just returns error. Kubernetes attach_detach controller decides when is safe to detach and takes the action. Before detaching, we have to make sure the the volume is already unmounted, otherwise it will cause data corruption.
It often happens that a pod is killed due to some reason and gets recreated in a different node. The sequence should be unmount volume, detach volume from the old node, and then attach to the new node. But it is very possible that attach requests comes in first, and if the attach function call detach directly, it will cause problem as described above.
We have the tests for GCE PD for this workflow. If there is no such test cases for Cinder, then we need add one.

I agree with @jingxu97. I didn't know about this PR when I submitted #50089, so I can close it in favour of this one.

BTW I've tested my PR on a live Cinder cluster and it fixes the bug we were seeing. Since our 2 PRs do the same thing, I'm pretty sure this will work too.

/approve
/lgtm

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dims, jingxu97

Associated issue: 50038

The full list of commands accepted by this bot can be found here.

/release-note-none

Dang it! we need someone to clear the "do-not-merge" label :(

@dims Just wondering, how does that usually work? Do we need to ping someone?

/retest

Automatic merge from submit-queue (batch tested with PRs 50087, 39587, 50042, 50241, 49914)

@wojtek-t can this be cherry-picked for v1.7.4?

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.

@jingxu97 @jamiehannaford - if you want this to be cherrypicked, please provide a release note

@jingxu97 @wojtek-t Ah okay. How about:

Updates Cinder AttachDisk operation to be more reliable by delegating Detaches to volume manager.

SGTM

When do the volume attach and find the volume had been attached to another node, is it better to to check whether the volume should be detached or not? then we can decide to detach the volume or just return error. The previous code is just do detach in this situation and now this PR changed to not do the detach. If we can check the volume should be detached or not, that would be better and can solve many issues like volume not get detached

@wenlxie What do you mean? Usually this is handled by the attach/detach controller, right?

Cherrypick approved and created.

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