openstack: Forcibly detach an attached cinder volume before attaching elsewhere #39055
Conversation
|
Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please follow instructions at https://github.com/kubernetes/kubernetes/wiki/CLA-FAQ to sign the CLA. Once you've signed, please reply here (e.g. "I signed it!") and we'll verify. Thanks.
If you have questions or suggestions related to this bot's behavior, please file an issue against the kubernetes/test-infra repository. |
k8s-ci-robot
added
the
cncf-cla: no
|
This change is |
Fixes kubernetes#33288 Co-Authored-By: @Rotwang
k8s-ci-robot
added
cncf-cla: yes
anguslees
changed the title
k8s-github-robot
added
size/S
|
Jenkins GCI GKE smoke e2e failed for commit 19efce6e3acc3e8c54ff9e9aed9908cede2b257b. Full PR test history. The magic incantation to run this job again is |
thockin
added
lgtm
|
Automatic merge from submit-queue (batch tested with PRs 39152, 39142, 39055) |
Automatic merge from submit-queue (batch tested with PRs 39152, 39142, 39055) openstack: Forcibly detach an attached cinder volume before attaching elsewhere Fixes kubernetes#33288 **What this PR does / why we need it**: Without this fix, we can't preemptively reschedule pods with persistent volumes to other hosts (for rebalancing or hardware failure recovery). **Which issue this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close that issue when PR gets merged)*: fixes kubernetes#33288 **Special notes for your reviewer**: (This is a resurrection/cleanup of PR kubernetes#33734, originally authored by @Rotwang) **Release note**:
|
How will this get moved in to a kubernetes release? I'm not familiar with the release process. @thockin @anguslees |
|
We're finding this issue is a big problem for us on 1.5.2 - any chance of a cherry-pick to the 1.5 series? |
|
Cherry picking this one should probably depend on cherry picking #39791 (comment) as well, as otherwise #39791 might hit you here. |
| } | ||
|
|
||
| glog.V(2).Infof("Disk %q is attached to a different compute (%q), detaching", diskName, disk.Attachments[0]["server_id"]) | ||
| err = os.DetachDisk(fmt.Sprintf("%s", disk.Attachments[0]["server_id"]), diskName) |
There was a problem hiding this comment.
I'm not super familiar with the release process here but 100% agreed with @codablock that this should not be pulled into mainline kube in its current state. For an example, lets say you're doing a kubectl delete on a webserver that's mid transaction. My expected behavior would be for the volume to be available until my pod fully exits, so the server can complete its final set of transactions. With this patch it would lose access to its volumes the instant a new pod gets scheduled which could cause data loss.
I'd imagine there should be a check to the kube API before this detach call to verify that the volume is not in use by any active pods. If this check was added kube would simply keep retrying until the volume isn't in use, at which point it would detach and continue.
|
It seems it was released in v1.6.0-alpha.1 |
Automatic merge from submit-queue (batch tested with PRs 39152, 39142, 39055) openstack: Forcibly detach an attached cinder volume before attaching elsewhere Fixes kubernetes#33288 **What this PR does / why we need it**: Without this fix, we can't preemptively reschedule pods with persistent volumes to other hosts (for rebalancing or hardware failure recovery). **Which issue this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close that issue when PR gets merged)*: fixes kubernetes#33288 **Special notes for your reviewer**: (This is a resurrection/cleanup of PR kubernetes#33734, originally authored by @Rotwang) **Release note**:
Fixes #33288
What this PR does / why we need it:
Without this fix, we can't preemptively reschedule pods with persistent volumes to other hosts (for rebalancing or hardware failure recovery).
Which issue this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close that issue when PR gets merged): fixes #33288Special notes for your reviewer:
(This is a resurrection/cleanup of PR #33734, originally authored by @Rotwang)
Release note: