Fix descriptor lock release logic for block volume unmapDevice

pkg/volume/fc/fc_util.go

@@ -30,7 +30,6 @@ import (
 	"k8s.io/kubernetes/pkg/features"
 	"k8s.io/kubernetes/pkg/volume"
 	volumeutil "k8s.io/kubernetes/pkg/volume/util"
-	"k8s.io/kubernetes/pkg/volume/util/volumepathhandler"
 )
 
 type ioHandler interface {
@@ -348,25 +347,11 @@ func (util *FCUtil) DetachBlockFCDisk(c fcDiskUnmapper, mapPath, devicePath stri
 	}
 	glog.V(4).Infof("fc: find destination device path from symlink: %v", dstPath)
 
-	// Get loopback device which takes fd lock for device beofore detaching a volume from node.
-	// TODO: This is a workaround for issue #54108
-	// Currently local attach plugins such as FC, iSCSI, RBD can't obtain devicePath during
-	// GenerateUnmapDeviceFunc() in operation_generator. As a result, these plugins fail to get
-	// and remove loopback device then it will be remained on kubelet node. To avoid the problem,
-	// local attach plugins needs to remove loopback device during TearDownDevice().
 	var devices []string
-	blkUtil := volumepathhandler.NewBlockVolumePathHandler()
 	dm := c.deviceUtil.FindMultipathDeviceForDevice(dstPath)
 	if len(dm) != 0 {
 		dstPath = dm
 	}
-	loop, err := volumepathhandler.BlockVolumePathHandler.GetLoopDevice(blkUtil, dstPath)
-	if err != nil {
-		if err.Error() != volumepathhandler.ErrDeviceNotFound {
-			return fmt.Errorf("fc: failed to get loopback for destination path: %v, err: %v", dstPath, err)
-		}
-		glog.Warningf("fc: loopback for destination path: %s not found", dstPath)
-	}
 
 	// Detach volume from kubelet node
 	if len(dm) != 0 {
@@ -388,13 +373,6 @@ func (util *FCUtil) DetachBlockFCDisk(c fcDiskUnmapper, mapPath, devicePath stri
 		glog.Errorf("fc: last error occurred during detach disk:\n%v", lastErr)
 		return lastErr
 	}
-	if len(loop) != 0 {
-		// The volume was successfully detached from node. We can safely remove the loopback.
-		err = volumepathhandler.BlockVolumePathHandler.RemoveLoopDevice(blkUtil, loop)
-		if err != nil {
-			return fmt.Errorf("fc: failed to remove loopback :%v, err: %v", loop, err)
-		}
-	}
 	return nil
 }
 

pkg/volume/iscsi/iscsi_util.go

@@ -34,7 +34,6 @@ import (
 	"k8s.io/kubernetes/pkg/util/mount"
 	"k8s.io/kubernetes/pkg/volume"
 	volumeutil "k8s.io/kubernetes/pkg/volume/util"
-	"k8s.io/kubernetes/pkg/volume/util/volumepathhandler"
 )
 
 var (
@@ -676,32 +675,11 @@ func (util *ISCSIUtil) DetachBlockISCSIDisk(c iscsiDiskUnmapper, mapPath string)
 	if mappedDevicePath := c.deviceUtil.FindMultipathDeviceForDevice(devicePath); mappedDevicePath != "" {
 		devicePath = mappedDevicePath
 	}
-	// Get loopback device which takes fd lock for devicePath before detaching a volume from node.
-	// TODO: This is a workaround for issue #54108
-	// Currently local attach plugins such as FC, iSCSI, RBD can't obtain devicePath during
-	// GenerateUnmapDeviceFunc() in operation_generator. As a result, these plugins fail to get
-	// and remove loopback device then it will be remained on kubelet node. To avoid the problem,
-	// local attach plugins needs to remove loopback device during TearDownDevice().
-	blkUtil := volumepathhandler.NewBlockVolumePathHandler()
-	loop, err := volumepathhandler.BlockVolumePathHandler.GetLoopDevice(blkUtil, devicePath)
-	if err != nil {
-		if err.Error() != volumepathhandler.ErrDeviceNotFound {
-			return fmt.Errorf("failed to get loopback for device: %v, err: %v", devicePath, err)
-		}
-		glog.Warningf("iscsi: loopback for device: %s not found", device)
-	}
 	// Detach a volume from kubelet node
 	err = util.detachISCSIDisk(c.exec, portals, iqn, iface, volName, initiatorName, found)
 	if err != nil {
 		return fmt.Errorf("failed to finish detachISCSIDisk, err: %v", err)
 	}
-	if len(loop) != 0 {
-		// The volume was successfully detached from node. We can safely remove the loopback.
-		err = volumepathhandler.BlockVolumePathHandler.RemoveLoopDevice(blkUtil, loop)
-		if err != nil {
-			return fmt.Errorf("failed to remove loopback :%v, err: %v", loop, err)
-		}
-	}
 	return nil
 }
 

pkg/volume/rbd/rbd.go

@@ -959,29 +959,6 @@ func (rbd *rbdDiskUnmapper) TearDownDevice(mapPath, _ string) error {
 	if err != nil {
 		return fmt.Errorf("rbd: failed to get loopback for device: %v, err: %v", device, err)
 	}
-	// Get loopback device which takes fd lock for device beofore detaching a volume from node.
-	// TODO: This is a workaround for issue #54108
-	// Currently local attach plugins such as FC, iSCSI, RBD can't obtain devicePath during
-	// GenerateUnmapDeviceFunc() in operation_generator. As a result, these plugins fail to get
-	// and remove loopback device then it will be remained on kubelet node. To avoid the problem,
-	// local attach plugins needs to remove loopback device during TearDownDevice().
-	blkUtil := volumepathhandler.NewBlockVolumePathHandler()
-	loop, err := volumepathhandler.BlockVolumePathHandler.GetLoopDevice(blkUtil, device)
-	if err != nil {
-		if err.Error() != volumepathhandler.ErrDeviceNotFound {
-			return fmt.Errorf("rbd: failed to get loopback for device: %v, err: %v", device, err)
-		}
-		glog.Warning("rbd: loopback for device: % not found", device)
-	} else {
-		if len(loop) != 0 {
-			// Remove loop device before detaching volume since volume detach operation gets busy if volume is opened by loopback.
-			err = volumepathhandler.BlockVolumePathHandler.RemoveLoopDevice(blkUtil, loop)
-			if err != nil {
-				return fmt.Errorf("rbd: failed to remove loopback :%v, err: %v", loop, err)
-			}
-			glog.V(4).Infof("rbd: successfully removed loop device: %s", loop)
-		}
-	}
 
 	err = rbd.manager.DetachBlockDisk(*rbd, mapPath)
 	if err != nil {

pkg/volume/util/operationexecutor/operation_generator.go

@@ -1079,30 +1079,16 @@ func (og *operationGenerator) GenerateUnmapDeviceFunc(
 			return deviceToDetach.GenerateError("UnmapDevice failed", err)
 		}
 
-		// Execute tear down device
-		unmapErr := blockVolumeUnmapper.TearDownDevice(globalMapPath, deviceToDetach.DevicePath)
-		if unmapErr != nil {
-			// On failure, return error. Caller will log and retry.
-			return deviceToDetach.GenerateError("UnmapDevice.TearDownDevice failed", unmapErr)
-		}
-
-		// Plugin finished TearDownDevice(). Now globalMapPath dir and plugin's stored data
-		// on the dir are unnecessary, clean up it.
-		removeMapPathErr := og.blkUtil.RemoveMapPath(globalMapPath)
-		if removeMapPathErr != nil {
-			// On failure, return error. Caller will log and retry.
-			return deviceToDetach.GenerateError("UnmapDevice failed", removeMapPathErr)
-		}
-
 		// The block volume is not referenced from Pods. Release file descriptor lock.
 		glog.V(4).Infof("UnmapDevice: deviceToDetach.DevicePath: %v", deviceToDetach.DevicePath)
 		loopPath, err := og.blkUtil.GetLoopDevice(deviceToDetach.DevicePath)
 		if err != nil {
 			if err.Error() == volumepathhandler.ErrDeviceNotFound {
-				glog.Warningf(deviceToDetach.GenerateMsgDetailed("UnmapDevice: Couldn't find loopback device which takes file descriptor lock", fmt.Sprintf("device path: %q", deviceToDetach.DevicePath)))
+				glog.Warningf(deviceToDetach.GenerateMsgDetailed("UnmapDevice: Couldn't find loopback device which takes file descriptor lock",
+					fmt.Sprintf("device path: %q", deviceToDetach.DevicePath)))
 			} else {
-				errInfo := "UnmapDevice.GetLoopDevice failed to get loopback device, " + fmt.Sprintf("device path: %q", deviceToDetach.DevicePath)
-				return deviceToDetach.GenerateError(errInfo, err)
+				glog.Warningf(deviceToDetach.GenerateMsgDetailed("UnmapDevice: GetLoopDevice failed to get loopback device",
+					fmt.Sprintf("device path: %q", deviceToDetach.DevicePath)))
 			}
 		} else {
 			if len(loopPath) != 0 {
@@ -1113,6 +1099,21 @@ func (og *operationGenerator) GenerateUnmapDeviceFunc(
 			}
 		}
 
+		// Execute tear down device
+		unmapErr := blockVolumeUnmapper.TearDownDevice(globalMapPath, deviceToDetach.DevicePath)
+		if unmapErr != nil {
+			// On failure, return error. Caller will log and retry.
+			return deviceToDetach.GenerateError("UnmapDevice.TearDownDevice failed", unmapErr)
+		}
+
+		// Plugin finished TearDownDevice(). Now globalMapPath dir and plugin's stored data
+		// on the dir are unnecessary, clean up it.
+		removeMapPathErr := og.blkUtil.RemoveMapPath(globalMapPath)
+		if removeMapPathErr != nil {
+			// On failure, return error. Caller will log and retry.
+			return deviceToDetach.GenerateError("UnmapDevice.RemoveMapPath failed", removeMapPathErr)
+		}
+
 		// Before logging that UnmapDevice succeeded and moving on,
 		// use mounter.PathIsDevice to check if the path is a device,
 		// if so use mounter.DeviceOpened to check if the device is in use anywhere