Feature/ub 1941 unmount fix

remote/mounter/block_device_mounter_utils/block_device_mounter_utils.go

@@ -142,7 +142,7 @@ func (b *blockDeviceMounterUtils) UnmountDeviceFlow(devicePath string, volumeWwn
 	defer b.mpathFlock.Unlock()
 	defer b.logger.Debug("Released mpathLock for device", logs.Args{{"device", devicePath}})
 
-	if err := b.blockDeviceUtils.Cleanup(devicePath); err != nil {
+	if err := b.CleanupAll(&resources.VolumeMountProperties{WWN: volumeWwn}); err != nil {
 		return b.logger.ErrorRet(err, "Cleanup failed")
 	}
 

remote/mounter/block_device_mounter_utils/block_device_utils_mounter_test.go

@@ -271,18 +271,18 @@ var _ = Describe("block_device_mounter_utils_test", func() {
 		})
 		It("should fail if Cleanup failed", func() {
 			fakeBlockDeviceUtils.UmountFsReturns(nil)
-			fakeBlockDeviceUtils.CleanupReturns(callErr)
+			fakeBlockDeviceUtils.CleanupDevicesReturns(callErr)
 			err = blockDeviceMounterUtils.UnmountDeviceFlow("fake_device", "6001738CFC9035EA0000000000795164")
 			Expect(err).To(HaveOccurred())
 			Expect(err).To(MatchError(callErr))
-			Expect(fakeBlockDeviceUtils.CleanupCallCount()).To(Equal(1))
+			Expect(fakeBlockDeviceUtils.CleanupDevicesCallCount()).To(Equal(1))
 		})
 		It("should succees if all is cool", func() {
 			fakeBlockDeviceUtils.UmountFsReturns(nil)
-			fakeBlockDeviceUtils.CleanupReturns(nil)
+			fakeBlockDeviceUtils.CleanupDevicesReturns(nil)
 			err = blockDeviceMounterUtils.UnmountDeviceFlow("fake_device", "6001738CFC9035EA0000000000795164")
 			Expect(err).NotTo(HaveOccurred())
-			Expect(fakeBlockDeviceUtils.CleanupCallCount()).To(Equal(1))
+			Expect(fakeBlockDeviceUtils.CleanupDevicesCallCount()).To(Equal(2))
 		})
 	})
 	Context(".CleanupAll", func() {

remote/mounter/block_device_utils/block_device_utils.go

@@ -14,21 +14,22 @@ type blockDeviceUtils struct {
 	exec                utils.Executor
 	regExAlreadyMounted *regexp.Regexp
 	fcConnector         initiator.Connector
+	iscsiConnector      initiator.Connector
 }
 
 func NewBlockDeviceUtils() BlockDeviceUtils {
-	return newBlockDeviceUtils(utils.NewExecutor(), nil)
+	return newBlockDeviceUtils(utils.NewExecutor())
 }
 
 func NewBlockDeviceUtilsWithExecutor(executor utils.Executor) BlockDeviceUtils {
-	return newBlockDeviceUtils(executor, nil)
+	return newBlockDeviceUtils(executor)
 }
 
-func NewBlockDeviceUtilsWithExecutorAndConnector(executor utils.Executor, fcConnector initiator.Connector) BlockDeviceUtils {
-	return newBlockDeviceUtils(executor, fcConnector)
+func NewBlockDeviceUtilsWithExecutorAndConnector(executor utils.Executor, conns ...initiator.Connector) BlockDeviceUtils {
+	return newBlockDeviceUtils(executor, conns...)
 }
 
-func newBlockDeviceUtils(executor utils.Executor, fcConnector initiator.Connector) BlockDeviceUtils {
+func newBlockDeviceUtils(executor utils.Executor, conns ...initiator.Connector) BlockDeviceUtils {
 	logger := logs.GetLogger()
 
 	// Prepare regex that going to be used in unmount interface
@@ -38,8 +39,15 @@ func newBlockDeviceUtils(executor utils.Executor, fcConnector initiator.Connecto
 		panic("failed prepare Already unmount regex")
 	}
 
-	if fcConnector == nil {
+	var fcConnector, iscsiConnector initiator.Connector
+
+	if len(conns) == 0 {
 		fcConnector = connectors.NewFibreChannelConnectorWithExecutor(executor)
+		iscsiConnector = connectors.NewISCSIConnectorWithExecutor(executor)
+	} else {
+		fcConnector = conns[0]
+		iscsiConnector = conns[1]
 	}
-	return &blockDeviceUtils{logger: logger, exec: executor, regExAlreadyMounted: regex, fcConnector: fcConnector}
+
+	return &blockDeviceUtils{logger: logger, exec: executor, regExAlreadyMounted: regex, fcConnector: fcConnector, iscsiConnector: iscsiConnector}
 }

remote/mounter/block_device_utils/block_device_utils_test.go

@@ -38,18 +38,20 @@ import (
 
 var _ = Describe("block_device_utils_test", func() {
 	var (
-		fakeExec        *fakes.FakeExecutor
-		fakeFcConnector *fakeinitiator.FakeConnector
-		bdUtils         block_device_utils.BlockDeviceUtils
-		err             error
-		cmdErr          error = errors.New("command error")
+		fakeExec           *fakes.FakeExecutor
+		fakeFcConnector    *fakeinitiator.FakeConnector
+		fakeISCSIConnector *fakeinitiator.FakeConnector
+		bdUtils            block_device_utils.BlockDeviceUtils
+		err                error
+		cmdErr             error = errors.New("command error")
 	)
 	volumeMountProperties := &resources.VolumeMountProperties{WWN: "wwn", LunNumber: 1}
 
 	BeforeEach(func() {
 		fakeExec = new(fakes.FakeExecutor)
 		fakeFcConnector = new(fakeinitiator.FakeConnector)
-		bdUtils = block_device_utils.NewBlockDeviceUtilsWithExecutorAndConnector(fakeExec, fakeFcConnector)
+		fakeISCSIConnector = new(fakeinitiator.FakeConnector)
+		bdUtils = block_device_utils.NewBlockDeviceUtilsWithExecutorAndConnector(fakeExec, fakeFcConnector, fakeISCSIConnector)
 	})
 
 	Context(".Rescan", func() {
@@ -91,13 +93,15 @@ var _ = Describe("block_device_utils_test", func() {
 		})
 	})
 	Context(".CleanupDevices", func() {
+
 		It("Cleanup ISCSI calls 'sudo iscsiadm -m session --rescan'", func() {
 			err = bdUtils.CleanupDevices(block_device_utils.ISCSI, volumeMountProperties)
 			Expect(err).ToNot(HaveOccurred())
 			Expect(fakeExec.ExecuteWithTimeoutCallCount()).To(Equal(1))
 			_, cmd, args := fakeExec.ExecuteWithTimeoutArgsForCall(0)
 			Expect(cmd).To(Equal("iscsiadm"))
 			Expect(args).To(Equal([]string{"-m", "session", "--rescan"}))
+			Expect(fakeISCSIConnector.DisconnectVolumeCallCount()).To(Equal(1))
 		})
 		It(`Cleanup SCSI calls fcConnector.DisconnectVolume`, func() {
 			err = bdUtils.CleanupDevices(block_device_utils.SCSI, volumeMountProperties)

remote/mounter/block_device_utils/rescan.go

@@ -27,6 +27,7 @@ import (
 
 const rescanIscsiTimeout = 1 * 60 * 1000
 const rescanScsiTimeout = 2 * 60 * 1000
+const ISCSIADM = "iscsiadm"
 
 var FcHostDir = "/sys/class/fc_host/"
 var ScsiHostDir = "/sys/class/scsi_host/"
@@ -51,30 +52,30 @@ func (b *blockDeviceUtils) CleanupDevices(protocol Protocol, volumeMountProperti
 	case SCSI:
 		return b.CleanupSCSIDevices(volumeMountProperties)
 	case ISCSI:
-		return b.CleanupISCSIDevices()
+		return b.CleanupISCSIDevices(volumeMountProperties)
 	default:
 		return b.logger.ErrorRet(&unsupportedProtocolError{protocol}, "failed")
 	}
 }
 
 func (b *blockDeviceUtils) RescanISCSI() error {
 	defer b.logger.Trace(logs.DEBUG)()
-	rescanCmd := "iscsiadm"
-	if err := b.exec.IsExecutable(rescanCmd); err != nil {
+
+	if err := b.exec.IsExecutable(ISCSIADM); err != nil {
 		b.logger.Debug("No iscisadm installed skipping ISCSI rescan")
 		return nil
 	}
 
 	args := []string{"-m", "session", "--rescan"}
 
-	if _, err := b.exec.ExecuteWithTimeout(rescanIscsiTimeout, rescanCmd, args); err != nil {
+	if _, err := b.exec.ExecuteWithTimeout(rescanIscsiTimeout, ISCSIADM, args); err != nil {
 		if b.IsExitStatusCode(err, 21) {
 			// error code 21 : ISCSI_ERR_NO_OBJS_FOUND - no records/targets/sessions/portals found to execute operation on.
-			b.logger.Warning(NoIscsiadmCommnadWarningMessage, logs.Args{{"command", fmt.Sprintf("[%s %s]", rescanCmd, args)}})
+			b.logger.Warning(NoIscsiadmCommnadWarningMessage, logs.Args{{"command", fmt.Sprintf("[%s %s]", ISCSIADM, args)}})
 			return nil
 
 		} else {
-			return b.logger.ErrorRet(&CommandExecuteError{rescanCmd, err}, "failed")
+			return b.logger.ErrorRet(&CommandExecuteError{ISCSIADM, err}, "failed")
 		}
 	}
 	return nil
@@ -98,10 +99,20 @@ func (b *blockDeviceUtils) RescanSCSI(volumeMountProperties *resources.VolumeMou
 }
 
 // TODO: improve it to make it faster, for more details, see os_brick project.
-func (b *blockDeviceUtils) CleanupISCSIDevices() error {
-	return b.RescanISCSI()
+func (b *blockDeviceUtils) CleanupISCSIDevices(volumeMountProperties *resources.VolumeMountProperties) error {
+	defer b.logger.Trace(logs.DEBUG)()
+
+	if err := b.exec.IsExecutable(ISCSIADM); err != nil {
+		b.logger.Debug("No iscisadm installed skipping ISCSI rescan")
+		return nil
+	}
+
+	b.RescanISCSI()
+	return b.iscsiConnector.DisconnectVolume(volumeMountProperties)
 }
 
 func (b *blockDeviceUtils) CleanupSCSIDevices(volumeMountProperties *resources.VolumeMountProperties) error {
+	defer b.logger.Trace(logs.DEBUG)()
+
 	return b.fcConnector.DisconnectVolume(volumeMountProperties)
 }

remote/mounter/initiator/connectors/fibre_channel.go

@@ -1,87 +1,49 @@
 package connectors
 
 import (
-	"fmt"
-
 	"github.com/IBM/ubiquity/remote/mounter/initiator"
 	"github.com/IBM/ubiquity/resources"
 	"github.com/IBM/ubiquity/utils"
 	"github.com/IBM/ubiquity/utils/logs"
 )
 
 type fibreChannelConnector struct {
-	exec    utils.Executor
-	logger  logs.Logger
-	linuxfc initiator.Initiator
+	*scsiConnector
 }
 
 func NewFibreChannelConnector() initiator.Connector {
 	return newFibreChannelConnector()
 }
 
 func NewFibreChannelConnectorWithExecutor(executor utils.Executor) initiator.Connector {
-	return newFibreChannelConnectorWithExecutorAndLogger(executor)
+	logger := logs.GetLogger()
+	return newFibreChannelConnectorWithExecutorAndLogger(executor, logger)
 }
 
-func NewFibreChannelConnectorWithAllFields(executor utils.Executor, linuxfc initiator.Initiator) initiator.Connector {
+func NewFibreChannelConnectorWithAllFields(executor utils.Executor, initi initiator.Initiator) initiator.Connector {
 	logger := logs.GetLogger()
-	return &fibreChannelConnector{logger: logger, exec: executor, linuxfc: linuxfc}
+	return &fibreChannelConnector{&scsiConnector{logger: logger, exec: executor, initi: initi}}
 }
 
 func newFibreChannelConnector() *fibreChannelConnector {
 	executor := utils.NewExecutor()
-	return newFibreChannelConnectorWithExecutorAndLogger(executor)
-}
-
-func newFibreChannelConnectorWithExecutorAndLogger(executor utils.Executor) *fibreChannelConnector {
 	logger := logs.GetLogger()
-	linuxfc := initiator.NewLinuxFibreChannelWithExecutor(executor)
+	return newFibreChannelConnectorWithExecutorAndLogger(executor, logger)
+}
 
-	return &fibreChannelConnector{logger: logger, exec: executor, linuxfc: linuxfc}
+func newFibreChannelConnectorWithExecutorAndLogger(executor utils.Executor, logger logs.Logger) *fibreChannelConnector {
+	initi := initiator.NewLinuxFibreChannelWithExecutor(executor)
+	return &fibreChannelConnector{&scsiConnector{logger: logger, exec: executor, initi: initi}}
 }
 
 // ConnectVolume attach the volume to host by rescaning all the active FC HBAs.
 func (c *fibreChannelConnector) ConnectVolume(volumeMountProperties *resources.VolumeMountProperties) error {
-	hbas := c.linuxfc.GetHBAs()
+	hbas := c.initi.GetHBAs()
 
 	if len(hbas) == 0 {
 		c.logger.Warning("No FC HBA is found.")
 		return nil
 	}
 
-	return c.linuxfc.RescanHosts(hbas, volumeMountProperties)
-}
-
-// DisconnectVolume removes a volume from host by echo "1" to all scsi device's /delete
-func (c *fibreChannelConnector) DisconnectVolume(volumeMountProperties *resources.VolumeMountProperties) error {
-	devices := []string{}
-	_, _, devNames, err := utils.GetMultipathOutputAndDeviceMapperAndDevice(volumeMountProperties.WWN, c.exec)
-	if err != nil {
-		return c.logger.ErrorRet(err, "Failed to get multipath output before disconnecting volume")
-	}
-	for _, devName := range devNames {
-		device := fmt.Sprintf("/dev/%s", devName)
-		devices = append(devices, device)
-	}
-
-	c.logger.Debug("Remove devices", logs.Args{{"names", devices}})
-	err = c.removeDevices(devices)
-	if err != nil {
-		return c.logger.ErrorRet(err, "Failed to remove devices")
-	}
-
-	if _, devMapper, _, _ := utils.GetMultipathOutputAndDeviceMapperAndDevice(volumeMountProperties.WWN, c.exec); devMapper != "" {
-		// flush multipath if device still exists after disconnection
-		c.linuxfc.FlushMultipath(devMapper)
-	}
-	return nil
-}
-
-func (c *fibreChannelConnector) removeDevices(devices []string) error {
-	// Do we need to flush io?
-	var err error
-	for _, device := range devices {
-		err = c.linuxfc.RemoveSCSIDevice(device)
-	}
-	return err
+	return c.initi.RescanHosts(hbas, volumeMountProperties)
 }

remote/mounter/initiator/connectors/fibre_channel_test.go

@@ -60,9 +60,10 @@ var _ = Describe("Test Fibre Channel Connector", func() {
 			fakeExec.ExecuteWithTimeoutReturns([]byte(fakeMultipathOutput), nil)
 		})
 
-		It("should remove all the scsi devices", func() {
+		It("should call multipath and remove all the scsi devices", func() {
 			err := fcConnector.DisconnectVolume(volumeMountProperties)
 			Ω(err).ShouldNot(HaveOccurred())
+			Expect(fakeExec.ExecuteWithTimeoutCallCount()).To(Equal(1))
 			Expect(fakeInitiator.RemoveSCSIDeviceCallCount()).To(Equal(3))
 			var a byte = 97
 			for i := 0; i < 3; i++ {

remote/mounter/initiator/connectors/iscsi.go

@@ -0,0 +1,41 @@
+package connectors
+
+import (
+	"github.com/IBM/ubiquity/remote/mounter/initiator"
+	"github.com/IBM/ubiquity/resources"
+	"github.com/IBM/ubiquity/utils"
+	"github.com/IBM/ubiquity/utils/logs"
+)
+
+type iscsiConnector struct {
+	*scsiConnector
+}
+
+func NewISCSIConnector() initiator.Connector {
+	return newISCSIConnector()
+}
+
+func NewISCSIConnectorWithExecutor(executor utils.Executor) initiator.Connector {
+	logger := logs.GetLogger()
+	return newISCSIConnectorWithExecutorAndLogger(executor, logger)
+}
+
+func NewISCSIConnectorWithAllFields(executor utils.Executor, initi initiator.Initiator) initiator.Connector {
+	logger := logs.GetLogger()
+	return &iscsiConnector{&scsiConnector{logger: logger, exec: executor, initi: initi}}
+}
+
+func newISCSIConnector() *iscsiConnector {
+	executor := utils.NewExecutor()
+	logger := logs.GetLogger()
+	return newISCSIConnectorWithExecutorAndLogger(executor, logger)
+}
+
+func newISCSIConnectorWithExecutorAndLogger(executor utils.Executor, logger logs.Logger) *iscsiConnector {
+	initi := initiator.NewLinuxISCSIWithExecutor(executor)
+	return &iscsiConnector{&scsiConnector{logger: logger, exec: executor, initi: initi}}
+}
+
+func (c *iscsiConnector) ConnectVolume(volumeMountProperties *resources.VolumeMountProperties) error {
+	return nil
+}

remote/mounter/initiator/connectors/iscsi_test.go

@@ -0,0 +1,47 @@
+package connectors_test
+
+import (
+	. "github.com/onsi/ginkgo"
+	. "github.com/onsi/gomega"
+
+	"github.com/IBM/ubiquity/fakes"
+	"github.com/IBM/ubiquity/remote/mounter/initiator"
+	"github.com/IBM/ubiquity/remote/mounter/initiator/connectors"
+	fakeinitiator "github.com/IBM/ubiquity/remote/mounter/initiator/fakes"
+	"github.com/IBM/ubiquity/resources"
+)
+
+var _ = Describe("Test ISCSI Connector", func() {
+	var (
+		fakeExec       *fakes.FakeExecutor
+		fakeInitiator  *fakeinitiator.FakeInitiator
+		iscsiConnector initiator.Connector
+	)
+	volumeMountProperties := &resources.VolumeMountProperties{WWN: fakeWwn, LunNumber: 1}
+
+	BeforeEach(func() {
+		fakeExec = new(fakes.FakeExecutor)
+		fakeInitiator = new(fakeinitiator.FakeInitiator)
+		iscsiConnector = connectors.NewISCSIConnectorWithAllFields(fakeExec, fakeInitiator)
+	})
+
+	Context("DisconnectVolume", func() {
+
+		BeforeEach(func() {
+			fakeExec.ExecuteWithTimeoutReturns([]byte(fakeMultipathOutput), nil)
+		})
+
+		It("should call multipath and remove all the scsi devices", func() {
+			err := iscsiConnector.DisconnectVolume(volumeMountProperties)
+			Ω(err).ShouldNot(HaveOccurred())
+			Expect(fakeExec.ExecuteWithTimeoutCallCount()).To(Equal(1))
+			Expect(fakeInitiator.RemoveSCSIDeviceCallCount()).To(Equal(3))
+			var a byte = 97
+			for i := 0; i < 3; i++ {
+				expectDev := "/dev/sd" + string(a+byte(i))
+				dev := fakeInitiator.RemoveSCSIDeviceArgsForCall(i)
+				Expect(dev).To(Equal(expectDev))
+			}
+		})
+	})
+})