From 9992bd23c2aa12db432696fd324e900770251dc0 Mon Sep 17 00:00:00 2001 From: James Ravn Date: Mon, 30 Jan 2017 15:08:43 +0000 Subject: [PATCH] Mark detached only if no pending operations To safely mark a volume detached when the volume controller manager is used. An example of one such problem: 1. pod is created, volume is added to desired state of the world 2. reconciler process starts 3. reconciler starts MountVolume, which is kicked off asynchronously via operation_executor.go 4. MountVolume mounts the volume, but hasn't yet marked it as mounted 5. pod is deleted, volume is removed from desired state of the world 6. reconciler detects volume is no longer in desired state of world, removes it from volumes in use 7. MountVolume tries to mark volume in use, throws an error because volume is no longer in actual state of world list. 8. controller-manager tries to detach the volume, this fails because it is still mounted to the OS. 9. EBS gets stuck indefinitely in busy state trying to detach. --- .../volumemanager/reconciler/reconciler.go | 14 +++++++++----- .../nestedpendingoperations.go | 16 ++++++++-------- .../util/operationexecutor/operation_executor.go | 2 +- 3 files changed, 18 insertions(+), 14 deletions(-) diff --git a/pkg/kubelet/volumemanager/reconciler/reconciler.go b/pkg/kubelet/volumemanager/reconciler/reconciler.go index 4aadcb8d0d4..5a76a4ad8fd 100644 --- a/pkg/kubelet/volumemanager/reconciler/reconciler.go +++ b/pkg/kubelet/volumemanager/reconciler/reconciler.go @@ -314,7 +314,9 @@ func (rc *reconciler) reconcile() { // Ensure devices that should be detached/unmounted are detached/unmounted. for _, attachedVolume := range rc.actualStateOfWorld.GetUnmountedVolumes() { - if !rc.desiredStateOfWorld.VolumeExists(attachedVolume.VolumeName) { + // Check IsOperationPending to avoid marking a volume as detached if it's in the process of mounting. + if !rc.desiredStateOfWorld.VolumeExists(attachedVolume.VolumeName) && + !rc.operationExecutor.IsOperationPending(attachedVolume.VolumeName, nestedpendingoperations.EmptyUniquePodName) { if attachedVolume.GloballyMounted { // Volume is globally mounted to device, unmount it glog.V(12).Infof("Attempting to start UnmountDevice for volume %q (spec.Name: %q)", @@ -341,11 +343,13 @@ func (rc *reconciler) reconcile() { } } else { // Volume is attached to node, detach it + // Kubelet not responsible for detaching or this volume has a non-attachable volume plugin. if rc.controllerAttachDetachEnabled || !attachedVolume.PluginIsAttachable { - // Kubelet not responsible for detaching or this volume has a non-attachable volume plugin, - // so just remove it to actualStateOfWorld without attach. - rc.actualStateOfWorld.MarkVolumeAsDetached( - attachedVolume.VolumeName, rc.nodeName) + rc.actualStateOfWorld.MarkVolumeAsDetached(attachedVolume.VolumeName, attachedVolume.NodeName) + glog.Infof("Detached volume %q (spec.Name: %q) devicePath: %q", + attachedVolume.VolumeName, + attachedVolume.VolumeSpec.Name(), + attachedVolume.DevicePath) } else { // Only detach if kubelet detach is enabled glog.V(12).Infof("Attempting to start DetachVolume for volume %q (spec.Name: %q)", diff --git a/pkg/volume/util/nestedpendingoperations/nestedpendingoperations.go b/pkg/volume/util/nestedpendingoperations/nestedpendingoperations.go index 718caf10de4..64d70900a37 100644 --- a/pkg/volume/util/nestedpendingoperations/nestedpendingoperations.go +++ b/pkg/volume/util/nestedpendingoperations/nestedpendingoperations.go @@ -36,11 +36,11 @@ import ( ) const ( - // emptyUniquePodName is a UniquePodName for empty string. - emptyUniquePodName types.UniquePodName = types.UniquePodName("") + // EmptyUniquePodName is a UniquePodName for empty string. + EmptyUniquePodName types.UniquePodName = types.UniquePodName("") - // emptyUniqueVolumeName is a UniqueVolumeName for empty string - emptyUniqueVolumeName v1.UniqueVolumeName = v1.UniqueVolumeName("") + // EmptyUniqueVolumeName is a UniqueVolumeName for empty string + EmptyUniqueVolumeName v1.UniqueVolumeName = v1.UniqueVolumeName("") ) // NestedPendingOperations defines the supported set of operations. @@ -160,7 +160,7 @@ func (grm *nestedPendingOperations) isOperationExists( podName types.UniquePodName) (bool, int) { // If volumeName is empty, operation can be executed concurrently - if volumeName == emptyUniqueVolumeName { + if volumeName == EmptyUniqueVolumeName { return false, -1 } @@ -170,8 +170,8 @@ func (grm *nestedPendingOperations) isOperationExists( continue } - if previousOp.podName != emptyUniquePodName && - podName != emptyUniquePodName && + if previousOp.podName != EmptyUniquePodName && + podName != EmptyUniquePodName && previousOp.podName != podName { // No match, keep searching continue @@ -274,7 +274,7 @@ func (grm *nestedPendingOperations) Wait() { func getOperationName( volumeName v1.UniqueVolumeName, podName types.UniquePodName) string { podNameStr := "" - if podName != emptyUniquePodName { + if podName != EmptyUniquePodName { podNameStr = fmt.Sprintf(" (%q)", podName) } diff --git a/pkg/volume/util/operationexecutor/operation_executor.go b/pkg/volume/util/operationexecutor/operation_executor.go index 1c9b91bfe3c..b301365df72 100644 --- a/pkg/volume/util/operationexecutor/operation_executor.go +++ b/pkg/volume/util/operationexecutor/operation_executor.go @@ -411,7 +411,7 @@ func (oe *operationExecutor) MountVolume( return err } - podName := volumetypes.UniquePodName("") + podName := nestedpendingoperations.EmptyUniquePodName // TODO: remove this -- not necessary if !volumeToMount.PluginIsAttachable { // Non-attachable volume plugins can execute mount for multiple pods