Update detach logic for block volume if devicePath is empty

pkg/volume/fc/attacher.go

@@ -200,13 +200,15 @@ func volumeSpecToMounter(spec *volume.Spec, host volume.VolumeHost) (*fcDiskMoun
 			volumeMode: volumeMode,
 			readOnly:   readOnly,
 			mounter:    volumehelper.NewSafeFormatAndMountFromHost(fcPluginName, host),
+			deviceUtil: volumeutil.NewDeviceHandler(volumeutil.NewIOHandler()),
 		}, nil
 	}
 	return &fcDiskMounter{
-		fcDisk:   fcDisk,
-		fsType:   fc.FSType,
-		readOnly: readOnly,
-		mounter:  volumehelper.NewSafeFormatAndMountFromHost(fcPluginName, host),
+		fcDisk:     fcDisk,
+		fsType:     fc.FSType,
+		readOnly:   readOnly,
+		mounter:    volumehelper.NewSafeFormatAndMountFromHost(fcPluginName, host),
+		deviceUtil: volumeutil.NewDeviceHandler(volumeutil.NewIOHandler()),
 	}, nil
 }
 
@@ -215,6 +217,7 @@ func volumeSpecToUnmounter(mounter mount.Interface) *fcDiskUnmounter {
 		fcDisk: &fcDisk{
 			io: &osIOHandler{},
 		},
-		mounter: mounter,
+		mounter:    mounter,
+		deviceUtil: volumeutil.NewDeviceHandler(volumeutil.NewIOHandler()),
 	}
 }

pkg/volume/fc/disk_manager.go

@@ -31,7 +31,9 @@ type diskManager interface {
 	// Attaches the disk to the kubelet's host machine.
 	AttachDisk(b fcDiskMounter) (string, error)
 	// Detaches the disk from the kubelet's host machine.
-	DetachDisk(disk fcDiskUnmounter, devName string) error
+	DetachDisk(disk fcDiskUnmounter, devicePath string) error
+	// Detaches the block disk from the kubelet's host machine.
+	DetachBlockFCDisk(disk fcDiskUnmapper, mntPath, devicePath string) error
 }
 
 // utility to mount a disk based filesystem

pkg/volume/fc/fc.go

@@ -18,6 +18,7 @@ package fc
 
 import (
 	"fmt"
+	"os"
 	"strconv"
 	"strings"
 
@@ -147,13 +148,15 @@ func (plugin *fcPlugin) newMounterInternal(spec *volume.Spec, podUID types.UID,
 			volumeMode: volumeMode,
 			readOnly:   readOnly,
 			mounter:    &mount.SafeFormatAndMount{Interface: mounter, Exec: exec},
+			deviceUtil: util.NewDeviceHandler(util.NewIOHandler()),
 		}, nil
 	}
 	return &fcDiskMounter{
-		fcDisk:   fcDisk,
-		fsType:   fc.FSType,
-		readOnly: readOnly,
-		mounter:  &mount.SafeFormatAndMount{Interface: mounter, Exec: exec},
+		fcDisk:     fcDisk,
+		fsType:     fc.FSType,
+		readOnly:   readOnly,
+		mounter:    &mount.SafeFormatAndMount{Interface: mounter, Exec: exec},
+		deviceUtil: util.NewDeviceHandler(util.NewIOHandler()),
 	}, nil
 
 }
@@ -189,8 +192,9 @@ func (plugin *fcPlugin) newBlockVolumeMapperInternal(spec *volume.Spec, podUID t
 			manager: manager,
 			io:      &osIOHandler{},
 			plugin:  plugin},
-		readOnly: readOnly,
-		mounter:  &mount.SafeFormatAndMount{Interface: mounter, Exec: exec},
+		readOnly:   readOnly,
+		mounter:    &mount.SafeFormatAndMount{Interface: mounter, Exec: exec},
+		deviceUtil: util.NewDeviceHandler(util.NewIOHandler()),
 	}, nil
 }
 
@@ -208,7 +212,8 @@ func (plugin *fcPlugin) newUnmounterInternal(volName string, podUID types.UID, m
 			plugin:  plugin,
 			io:      &osIOHandler{},
 		},
-		mounter: mounter,
+		mounter:    mounter,
+		deviceUtil: util.NewDeviceHandler(util.NewIOHandler()),
 	}, nil
 }
 
@@ -225,6 +230,7 @@ func (plugin *fcPlugin) newUnmapperInternal(volName string, podUID types.UID, ma
 			plugin:  plugin,
 			io:      &osIOHandler{},
 		},
+		deviceUtil: util.NewDeviceHandler(util.NewIOHandler()),
 	}, nil
 }
 
@@ -328,6 +334,7 @@ type fcDiskMounter struct {
 	fsType     string
 	volumeMode v1.PersistentVolumeMode
 	mounter    *mount.SafeFormatAndMount
+	deviceUtil util.DeviceUtil
 }
 
 var _ volume.Mounter = &fcDiskMounter{}
@@ -362,7 +369,8 @@ func (b *fcDiskMounter) SetUpAt(dir string, fsGroup *int64) error {
 
 type fcDiskUnmounter struct {
 	*fcDisk
-	mounter mount.Interface
+	mounter    mount.Interface
+	deviceUtil util.DeviceUtil
 }
 
 var _ volume.Unmounter = &fcDiskUnmounter{}
@@ -380,8 +388,9 @@ func (c *fcDiskUnmounter) TearDownAt(dir string) error {
 // Block Volumes Support
 type fcDiskMapper struct {
 	*fcDisk
-	readOnly bool
-	mounter  mount.Interface
+	readOnly   bool
+	mounter    mount.Interface
+	deviceUtil util.DeviceUtil
 }
 
 var _ volume.BlockVolumeMapper = &fcDiskMapper{}
@@ -392,18 +401,22 @@ func (b *fcDiskMapper) SetUpDevice() (string, error) {
 
 type fcDiskUnmapper struct {
 	*fcDisk
+	deviceUtil util.DeviceUtil
 }
 
 var _ volume.BlockVolumeUnmapper = &fcDiskUnmapper{}
 
-func (c *fcDiskUnmapper) TearDownDevice(_, devicePath string) error {
-	// Remove scsi device from the node.
-	if !strings.HasPrefix(devicePath, "/dev/") {
-		return fmt.Errorf("fc detach disk: invalid device name: %s", devicePath)
+func (c *fcDiskUnmapper) TearDownDevice(mapPath, devicePath string) error {
+	err := c.manager.DetachBlockFCDisk(*c, mapPath, devicePath)
+	if err != nil {
+		return fmt.Errorf("fc: failed to detach disk: %s\nError: %v", mapPath, err)
+	}
+	glog.V(4).Infof("fc: %q is unmounted, deleting the directory", mapPath)
+	err = os.RemoveAll(mapPath)
+	if err != nil {
+		return fmt.Errorf("fc: failed to delete the directory: %s\nError: %v", mapPath, err)
 	}
-	arr := strings.Split(devicePath, "/")
-	dev := arr[len(arr)-1]
-	removeFromScsiSubsystem(dev, c.io)
+	glog.V(4).Infof("fc: successfully detached disk: %s", mapPath)
 	return nil
 }
 

pkg/volume/fc/fc_test.go

@@ -120,6 +120,15 @@ func (fake *fakeDiskManager) DetachDisk(c fcDiskUnmounter, mntPath string) error
 	return nil
 }
 
+func (fake *fakeDiskManager) DetachBlockFCDisk(c fcDiskUnmapper, mapPath, devicePath string) error {
+	err := os.RemoveAll(mapPath)
+	if err != nil {
+		return err
+	}
+	fake.detachCalled = true
+	return nil
+}
+
 func doTestPlugin(t *testing.T, spec *volume.Spec) {
 	tmpDir, err := utiltesting.MkTmpdir("fc_test")
 	if err != nil {

pkg/volume/fc/fc_util.go

@@ -29,6 +29,7 @@ import (
 	utilfeature "k8s.io/apiserver/pkg/util/feature"
 	"k8s.io/kubernetes/pkg/features"
 	"k8s.io/kubernetes/pkg/volume"
+	volumeutil "k8s.io/kubernetes/pkg/volume/util"
 )
 
 type ioHandler interface {
@@ -40,6 +41,11 @@ type ioHandler interface {
 
 type osIOHandler struct{}
 
+const (
+	byPath = "/dev/disk/by-path/"
+	byID   = "/dev/disk/by-id/"
+)
+
 func (handler *osIOHandler) ReadDir(dirname string) ([]os.FileInfo, error) {
 	return ioutil.ReadDir(dirname)
 }
@@ -53,37 +59,17 @@ func (handler *osIOHandler) WriteFile(filename string, data []byte, perm os.File
 	return ioutil.WriteFile(filename, data, perm)
 }
 
-// given a disk path like /dev/sdx, find the devicemapper parent
-// TODO #23192 Convert this code to use the generic code in ../util
-// which is used by the iSCSI implementation
-func findMultipathDeviceMapper(disk string, io ioHandler) string {
-	sys_path := "/sys/block/"
-	if dirs, err := io.ReadDir(sys_path); err == nil {
-		for _, f := range dirs {
-			name := f.Name()
-			if strings.HasPrefix(name, "dm-") {
-				if _, err1 := io.Lstat(sys_path + name + "/slaves/" + disk); err1 == nil {
-					return "/dev/" + name
-				}
-			}
-		}
-	}
-	return ""
-}
-
 // given a wwn and lun, find the device and associated devicemapper parent
-func findDisk(wwn, lun string, io ioHandler) (string, string) {
+func findDisk(wwn, lun string, io ioHandler, deviceUtil volumeutil.DeviceUtil) (string, string) {
 	fc_path := "-fc-0x" + wwn + "-lun-" + lun
-	dev_path := "/dev/disk/by-path/"
+	dev_path := byPath
 	if dirs, err := io.ReadDir(dev_path); err == nil {
 		for _, f := range dirs {
 			name := f.Name()
 			if strings.Contains(name, fc_path) {
 				if disk, err1 := io.EvalSymlinks(dev_path + name); err1 == nil {
-					arr := strings.Split(disk, "/")
-					l := len(arr) - 1
-					dev := arr[l]
-					dm := findMultipathDeviceMapper(dev, io)
+					dm := deviceUtil.FindMultipathDeviceForDevice(disk)
+					glog.Infof("fc: find disk: %v, dm: %v", disk, dm)
 					return disk, dm
 				}
 			}
@@ -93,7 +79,7 @@ func findDisk(wwn, lun string, io ioHandler) (string, string) {
 }
 
 // given a wwid, find the device and associated devicemapper parent
-func findDiskWWIDs(wwid string, io ioHandler) (string, string) {
+func findDiskWWIDs(wwid string, io ioHandler, deviceUtil volumeutil.DeviceUtil) (string, string) {
 	// Example wwid format:
 	//   3600508b400105e210000900000490000
 	//   <VENDOR NAME> <IDENTIFIER NUMBER>
@@ -104,7 +90,7 @@ func findDiskWWIDs(wwid string, io ioHandler) (string, string) {
 	// underscore when wwid is exposed under /dev/by-id.
 
 	fc_path := "scsi-" + wwid
-	dev_id := "/dev/disk/by-id/"
+	dev_id := byID
 	if dirs, err := io.ReadDir(dev_id); err == nil {
 		for _, f := range dirs {
 			name := f.Name()
@@ -114,10 +100,8 @@ func findDiskWWIDs(wwid string, io ioHandler) (string, string) {
 					glog.V(2).Infof("fc: failed to find a corresponding disk from symlink[%s], error %v", dev_id+name, err)
 					return "", ""
 				}
-				arr := strings.Split(disk, "/")
-				l := len(arr) - 1
-				dev := arr[l]
-				dm := findMultipathDeviceMapper(dev, io)
+				dm := deviceUtil.FindMultipathDeviceForDevice(disk)
+				glog.Infof("fc: find disk: %v, dm: %v", disk, dm)
 				return disk, dm
 			}
 		}
@@ -197,9 +181,9 @@ func searchDisk(b fcDiskMounter) (string, error) {
 	for true {
 		for _, diskId := range diskIds {
 			if len(wwns) != 0 {
-				disk, dm = findDisk(diskId, lun, io)
+				disk, dm = findDisk(diskId, lun, io, b.deviceUtil)
 			} else {
-				disk, dm = findDiskWWIDs(diskId, io)
+				disk, dm = findDiskWWIDs(diskId, io, b.deviceUtil)
 			}
 			// if multipath device is found, break
 			if dm != "" {
@@ -265,13 +249,153 @@ func (util *FCUtil) AttachDisk(b fcDiskMounter) (string, error) {
 	return devicePath, err
 }
 
-func (util *FCUtil) DetachDisk(c fcDiskUnmounter, devName string) error {
+// DetachDisk removes scsi device file such as /dev/sdX from the node.
+func (util *FCUtil) DetachDisk(c fcDiskUnmounter, devicePath string) error {
+	var devices []string
+	// devicePath might be like /dev/mapper/mpathX. Find destination.
+	dstPath, err := c.io.EvalSymlinks(devicePath)
+	if err != nil {
+		return err
+	}
+	// Find slave
+	if strings.HasPrefix(dstPath, "/dev/dm-") {
+		devices = c.deviceUtil.FindSlaveDevicesOnMultipath(dstPath)
+	} else {
+		// Add single devicepath to devices
+		devices = append(devices, dstPath)
+	}
+	glog.V(4).Infof("fc: DetachDisk devicePath: %v, dstPath: %v, devices: %v", devicePath, dstPath, devices)
+	var lastErr error
+	for _, device := range devices {
+		err := util.detachFCDisk(c.io, device)
+		if err != nil {
+			glog.Errorf("fc: detachFCDisk failed. device: %v err: %v", device, err)
+			lastErr = fmt.Errorf("fc: detachFCDisk failed. device: %v err: %v", device, err)
+		}
+	}
+	if lastErr != nil {
+		glog.Errorf("fc: last error occurred during detach disk:\n%v", lastErr)
+		return lastErr
+	}
+	return nil
+}
+
+// detachFCDisk removes scsi device file such as /dev/sdX from the node.
+func (util *FCUtil) detachFCDisk(io ioHandler, devicePath string) error {
 	// Remove scsi device from the node.
-	if !strings.HasPrefix(devName, "/dev/") {
-		return fmt.Errorf("fc detach disk: invalid device name: %s", devName)
+	if !strings.HasPrefix(devicePath, "/dev/") {
+		return fmt.Errorf("fc detach disk: invalid device name: %s", devicePath)
 	}
-	arr := strings.Split(devName, "/")
+	arr := strings.Split(devicePath, "/")
 	dev := arr[len(arr)-1]
-	removeFromScsiSubsystem(dev, c.io)
+	removeFromScsiSubsystem(dev, io)
 	return nil
 }
+
+// DetachBlockFCDisk detaches a volume from kubelet node, removes scsi device file
+// such as /dev/sdX from the node, and then removes loopback for the scsi device.
+func (util *FCUtil) DetachBlockFCDisk(c fcDiskUnmapper, mapPath, devicePath string) error {
+	// Check if devicePath is valid
+	if len(devicePath) != 0 {
+		if pathExists, pathErr := checkPathExists(devicePath); !pathExists || pathErr != nil {
+			return pathErr
+		}
+	} else {
+		// TODO: FC plugin can't obtain the devicePath from kubelet becuase devicePath
+		// in volume object isn't updated when volume is attached to kubelet node.
+		glog.Infof("fc: devicePath is empty. Try to retreive FC configuration from global map path: %v", mapPath)
+	}
+
+	// Check if global map path is valid
+	// global map path examples:
+	//   wwn+lun: plugins/kubernetes.io/fc/volumeDevices/50060e801049cfd1-lun-0/
+	//   wwid: plugins/kubernetes.io/fc/volumeDevices/3600508b400105e210000900000490000/
+	if pathExists, pathErr := checkPathExists(mapPath); !pathExists || pathErr != nil {
+		return pathErr
+	}
+
+	// Retreive volume plugin dependent path like '50060e801049cfd1-lun-0' from global map path
+	arr := strings.Split(mapPath, "/")
+	if len(arr) < 1 {
+		return fmt.Errorf("Fail to retreive volume plugin information from global map path: %v", mapPath)
+	}
+	volumeInfo := arr[len(arr)-1]
+
+	// Search symbolick link which matches volumeInfo under /dev/disk/by-path or /dev/disk/by-id
+	// then find destination device path from the link
+	searchPath := byID
+	if strings.Contains(volumeInfo, "-lun-") {
+		searchPath = byPath
+	}
+	fis, err := ioutil.ReadDir(searchPath)
+	if err != nil {
+		return err
+	}
+	for _, fi := range fis {
+		if strings.Contains(fi.Name(), volumeInfo) {
+			devicePath = path.Join(searchPath, fi.Name())
+			glog.V(5).Infof("fc: updated devicePath: %s", devicePath)
+			break
+		}
+	}
+	if len(devicePath) == 0 {
+		return fmt.Errorf("fc: failed to find corresponding device from searchPath: %v", searchPath)
+	}
+	dstPath, err := c.io.EvalSymlinks(devicePath)
+	if err != nil {
+		return err
+	}
+	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.
+	var devices []string
+	blkUtil := volumeutil.NewBlockVolumePathHandler()
+	dm := c.deviceUtil.FindMultipathDeviceForDevice(dstPath)
+	if len(dm) != 0 {
+		dstPath = dm
+	}
+	loop, err := volumeutil.BlockVolumePathHandler.GetLoopDevice(blkUtil, dstPath)
+	if err != nil {
+		glog.Warningf("fc: failed to get loopback for device: %v, err: %v", dstPath, err)
+	} else {
+		glog.V(4).Infof("fc: found loopback: %v", loop)
+	}
+
+	// Detach volume from kubelet node
+	if len(dm) != 0 {
+		// Find all devices which are managed by multipath
+		devices = c.deviceUtil.FindSlaveDevicesOnMultipath(dm)
+	} else {
+		// Add single device path to devices
+		devices = append(devices, dstPath)
+	}
+	var lastErr error
+	for _, device := range devices {
+		err = util.detachFCDisk(c.io, device)
+		if err != nil {
+			glog.Errorf("fc: detachFCDisk failed. device: %v err: %v", device, err)
+			lastErr = fmt.Errorf("fc: detachFCDisk failed. device: %v err: %v", device, err)
+		}
+	}
+	if lastErr != nil {
+		glog.Errorf("fc: last error occurred during detach disk:\n%v", lastErr)
+		return lastErr
+	}
+
+	// The volume was successfully detached from node. We can safely remove the loopback.
+	err = volumeutil.BlockVolumePathHandler.RemoveLoopDevice(blkUtil, loop)
+	if err != nil {
+		return fmt.Errorf("fc: failed to remove loopback :%v, err: %v", loop, err)
+	}
+	return nil
+}
+
+func checkPathExists(path string) (bool, error) {
+	if pathExists, pathErr := volumeutil.PathExists(path); pathErr != nil {
+		return pathExists, fmt.Errorf("Error checking if path exists: %v", pathErr)
+	} else if !pathExists {
+		glog.Warningf("Warning: Unmap skipped because path does not exist: %v", path)
+		return pathExists, nil
+	}
+	return true, nil
+}

pkg/volume/fc/fc_util_test.go

@@ -20,6 +20,8 @@ import (
 	"os"
 	"testing"
 	"time"
+
+	"k8s.io/kubernetes/pkg/volume/util"
 )
 
 type fakeFileInfo struct {
@@ -91,6 +93,7 @@ func TestSearchDisk(t *testing.T) {
 			lun:  "0",
 			io:   &fakeIOHandler{},
 		},
+		deviceUtil: util.NewDeviceHandler(util.NewIOHandler()),
 	}
 	devicePath, error := searchDisk(fakeMounter)
 	// if no disk matches input wwn and lun, exit
@@ -105,6 +108,7 @@ func TestSearchDiskWWID(t *testing.T) {
 			wwids: []string{"3600508b400105e210000900000490000"},
 			io:    &fakeIOHandler{},
 		},
+		deviceUtil: util.NewDeviceHandler(util.NewIOHandler()),
 	}
 	devicePath, error := searchDisk(fakeMounter)
 	// if no disk matches input wwid, exit

pkg/volume/util/device_util.go

@@ -19,6 +19,7 @@ package util
 //DeviceUtil is a util for common device methods
 type DeviceUtil interface {
 	FindMultipathDeviceForDevice(disk string) string
+	FindSlaveDevicesOnMultipath(disk string) []string
 }
 
 type deviceHandler struct {

pkg/volume/util/device_util_linux.go

@@ -20,6 +20,7 @@ package util
 
 import (
 	"errors"
+	"path"
 	"strings"
 )
 
@@ -59,3 +60,23 @@ func findDeviceForPath(path string, io IoUtil) (string, error) {
 	}
 	return "", errors.New("Illegal path for device " + devicePath)
 }
+
+// FindSlaveDevicesOnMultipath given a dm name like /dev/dm-1, find all devices
+// which are managed by the devicemapper dm-1.
+func (handler *deviceHandler) FindSlaveDevicesOnMultipath(dm string) []string {
+	var devices []string
+	io := handler.get_io
+	// Split path /dev/dm-1 into "", "dev", "dm-1"
+	parts := strings.Split(dm, "/")
+	if len(parts) != 3 || !strings.HasPrefix(parts[1], "dev") {
+		return devices
+	}
+	disk := parts[2]
+	slavesPath := path.Join("/sys/block/", disk, "/slaves/")
+	if files, err := io.ReadDir(slavesPath); err == nil {
+		for _, f := range files {
+			devices = append(devices, path.Join("/dev/", f.Name()))
+		}
+	}
+	return devices
+}

pkg/volume/util/device_util_linux_test.go

@@ -21,6 +21,7 @@ package util
 import (
 	"errors"
 	"os"
+	"reflect"
 	"testing"
 	"time"
 )
@@ -29,11 +30,14 @@ type mockOsIOHandler struct{}
 
 func (handler *mockOsIOHandler) ReadDir(dirname string) ([]os.FileInfo, error) {
 	switch dirname {
-	case "/sys/block/dm-2/slaves/":
-		f := &fakeFileInfo{
+	case "/sys/block/dm-1/slaves":
+		f1 := &fakeFileInfo{
 			name: "sda",
 		}
-		return []os.FileInfo{f}, nil
+		f2 := &fakeFileInfo{
+			name: "sdb",
+		}
+		return []os.FileInfo{f1, f2}, nil
 	case "/sys/block/":
 		f1 := &fakeFileInfo{
 			name: "sda",
@@ -62,8 +66,10 @@ func (handler *mockOsIOHandler) EvalSymlinks(path string) (string, error) {
 		"/returns/a/dev":                                              "/dev/sde",
 		"/returns/non/dev":                                            "/sys/block",
 		"/dev/disk/by-path/127.0.0.1:3260-eui.02004567A425678D-lun-0": "/dev/sda",
+		"/dev/disk/by-path/127.0.0.3:3260-eui.03004567A425678D-lun-0": "/dev/sdb",
 		"/dev/dm-2": "/dev/dm-2",
 		"/dev/dm-3": "/dev/dm-3",
+		"/dev/sdc":  "/dev/sdc",
 		"/dev/sde":  "/dev/sde",
 	}
 	return links[path], nil
@@ -140,3 +146,15 @@ func TestFindDeviceForPath(t *testing.T) {
 	}
 
 }
+
+func TestFindSlaveDevicesOnMultipath(t *testing.T) {
+	mockDeviceUtil := NewDeviceHandler(&mockOsIOHandler{})
+	devices := mockDeviceUtil.FindSlaveDevicesOnMultipath("/dev/dm-1")
+	if !reflect.DeepEqual(devices, []string{"/dev/sda", "/dev/sdb"}) {
+		t.Fatalf("failed to find devices managed by mpio device. /dev/sda, /dev/sdb expected got [%s]", devices)
+	}
+	dev := mockDeviceUtil.FindSlaveDevicesOnMultipath("/dev/sdc")
+	if len(dev) != 0 {
+		t.Fatalf("mpio device not found '' expected got [%s]", dev)
+	}
+}

pkg/volume/util/device_util_unsupported.go

@@ -22,3 +22,9 @@ package util
 func (handler *deviceHandler) FindMultipathDeviceForDevice(device string) string {
 	return ""
 }
+
+// FindSlaveDevicesOnMultipath unsupported returns ""
+func (handler *deviceHandler) FindSlaveDevicesOnMultipath(disk string) []string {
+	out := []string{}
+	return out
+}