Merge "Refactor iSCSI disconnect"

os_brick/exception.py

@@ -14,7 +14,9 @@
 
 """Exceptions for the Brick library."""
 
+from oslo_concurrency import processutils as putils
 import six
+import traceback
 
 from os_brick.i18n import _
 from oslo_log import log as logging
@@ -167,3 +169,70 @@ class HostChannelsTargetsNotFound(BrickException):
     def __init__(self, message=None, iqns=None, found=None):
         super(HostChannelsTargetsNotFound, self).__init__(message, iqns=iqns)
         self.found = found
+
+
+class ExceptionChainer(BrickException):
+    """A Exception that can contain a group of exceptions.
+
+    This exception serves as a container for exceptions, useful when we want to
+    store all exceptions that happened during a series of steps and then raise
+    them all together as one.
+
+    The representation of the exception will include all exceptions and their
+    tracebacks.
+
+    This class also includes a context manager for convenience, one that will
+    support both swallowing the exception as if nothing had happened and
+    raising the exception.  In both cases the exception will be stored.
+
+    If a message is provided to the context manager it will be formatted and
+    logged with warning level.
+    """
+    def __init__(self, *args, **kwargs):
+        self._exceptions = []
+        self._repr = None
+        super(ExceptionChainer, self).__init__(*args, **kwargs)
+
+    def __repr__(self):
+        # Since generating the representation can be slow we cache it
+        if not self._repr:
+            tracebacks = (
+                ''.join(traceback.format_exception(*e)).replace('\n', '\n\t')
+                for e in self._exceptions)
+            self._repr = '\n'.join('\nChained Exception #%s\n\t%s' % (i + 1, t)
+                                   for i, t in enumerate(tracebacks))
+        return self._repr
+
+    __str__ = __unicode__ = __repr__
+
+    def __nonzero__(self):
+        # We want to be able to do boolean checks on the exception
+        return bool(self._exceptions)
+
+    __bool__ = __nonzero__  # For Python 3
+
+    def add_exception(self, exc_type, exc_val, exc_tb):
+        # Clear the representation cache
+        self._repr = None
+        self._exceptions.append((exc_type, exc_val, exc_tb))
+
+    def context(self, catch_exception, msg='', *msg_args):
+        self._catch_exception = catch_exception
+        self._exc_msg = msg
+        self._exc_msg_args = msg_args
+        return self
+
+    def __enter__(self):
+        return self
+
+    def __exit__(self, exc_type, exc_val, exc_tb):
+        if exc_type:
+            self.add_exception(exc_type, exc_val, exc_tb)
+            if self._exc_msg:
+                LOG.warning(self._exc_msg, *self._exc_msg_args)
+            if self._catch_exception:
+                return True
+
+
+class ExecutionTimeout(putils.ProcessExecutionError):
+    pass

os_brick/initiator/__init__.py

@@ -25,7 +25,6 @@ import re
 
 DEVICE_SCAN_ATTEMPTS_DEFAULT = 3
 MULTIPATH_ERROR_REGEX = re.compile("\w{3} \d+ \d\d:\d\d:\d\d \|.*$")
-MULTIPATH_DEV_CHECK_REGEX = re.compile("\s+dm-\d+\s+")
 MULTIPATH_PATH_CHECK_REGEX = re.compile("\s+\d+:\d+:\d+:\d+\s+")
 
 PLATFORM_ALL = 'ALL'

os_brick/initiator/connector.py

@@ -43,7 +43,6 @@ synchronized = lockutils.synchronized_with_prefix('os-brick-')
 
 DEVICE_SCAN_ATTEMPTS_DEFAULT = 3
 MULTIPATH_ERROR_REGEX = re.compile("\w{3} \d+ \d\d:\d\d:\d\d \|.*$")
-MULTIPATH_DEV_CHECK_REGEX = re.compile("\s+dm-\d+\s+")
 MULTIPATH_PATH_CHECK_REGEX = re.compile("\s+\d+:\d+:\d+:\d+\s+")
 
 PLATFORM_ALL = 'ALL'

os_brick/initiator/connectors/aoe.py

@@ -124,7 +124,8 @@ class AoEConnector(base.BaseLinuxConnector):
 
     @utils.trace
     @lockutils.synchronized('aoe_control', 'aoe-')
-    def disconnect_volume(self, connection_properties, device_info):
+    def disconnect_volume(self, connection_properties, device_info,
+                          force=False, ignore_errors=False):
         """Detach and flush the volume.
 
         :param connection_properties: The dictionary that describes all

os_brick/initiator/connectors/disco.py

@@ -113,7 +113,8 @@ class DISCOConnector(base.BaseLinuxConnector):
 
     @utils.trace
     @synchronized('connect_volume')
-    def disconnect_volume(self, connection_properties, device_info):
+    def disconnect_volume(self, connection_properties, device_info,
+                          force=False, ignore_errors=False):
         """Detach the volume from instance."""
         disco_id = connection_properties['disco_id']
         disco_dev = '/dev/dms%s' % (disco_id)

os_brick/initiator/connectors/drbd.py

@@ -90,7 +90,8 @@ class DRBDConnector(base.BaseLinuxConnector):
         return device_info
 
     @utils.trace
-    def disconnect_volume(self, connection_properties, device_info):
+    def disconnect_volume(self, connection_properties, device_info,
+                          force=False, ignore_errors=False):
         """Detach the volume."""
 
         self._drbdadm_command("down", connection_properties,

os_brick/initiator/connectors/fake.py

@@ -27,7 +27,8 @@ class FakeConnector(base.BaseLinuxConnector):
                             'path': self.fake_path}
         return fake_device_info
 
-    def disconnect_volume(self, connection_properties, device_info):
+    def disconnect_volume(self, connection_properties, device_info,
+                          force=False, ignore_errors=False):
         pass
 
     def get_volume_paths(self, connection_properties):

os_brick/initiator/connectors/fibre_channel.py

@@ -243,7 +243,8 @@ class FibreChannelConnector(base.BaseLinuxConnector):
 
     @utils.trace
     @synchronized('connect_volume')
-    def disconnect_volume(self, connection_properties, device_info):
+    def disconnect_volume(self, connection_properties, device_info,
+                          force=False, ignore_errors=False):
         """Detach the volume from instance_name.
 
         :param connection_properties: The dictionary that describes all

os_brick/initiator/connectors/hgst.py

@@ -142,7 +142,8 @@ class HGSTConnector(base.BaseLinuxConnector):
         return device_info
 
     @utils.trace
-    def disconnect_volume(self, connection_properties, device_info):
+    def disconnect_volume(self, connection_properties, device_info,
+                          force=False, ignore_errors=False):
         """Detach and flush the volume.
 
         :param connection_properties: The dictionary that describes all

os_brick/initiator/connectors/huawei.py

@@ -117,7 +117,8 @@ class HuaweiStorHyperConnector(base.BaseLinuxConnector):
 
     @utils.trace
     @synchronized('connect_volume')
-    def disconnect_volume(self, connection_properties, device_info):
+    def disconnect_volume(self, connection_properties, device_info,
+                          force=False, ignore_errors=False):
         """Disconnect a volume from the local host.
 
         :param connection_properties: The dictionary that describes all

os_brick/initiator/connectors/iscsi.py

@@ -17,7 +17,6 @@ import collections
 import copy
 import glob
 import os
-import re
 import time
 
 from oslo_concurrency import lockutils
@@ -106,27 +105,63 @@ class ISCSIConnector(base.BaseLinuxConnector, base_iscsi.BaseISCSIConnector):
 
         return volume_paths
 
-    def _get_iscsi_sessions(self):
+    def _get_iscsi_sessions_full(self):
+        """Get iSCSI session information as a list of tuples.
+
+        Uses iscsiadm -m session and from a command output like
+            tcp: [1] 192.168.121.250:3260,1 iqn.2010-10.org.openstack:volume-
+
+        This method will drop the node type and return a list like this:
+            [('tcp:', '1', '192.168.121.250:3260', '1',
+              'iqn.2010-10.org.openstack:volume-')]
+        """
         out, err = self._run_iscsi_session()
-
-        iscsi_sessions = []
-
         if err:
             LOG.warning("Couldn't find iscsi sessions because "
-                        "iscsiadm err: %s",
-                        err)
-        else:
-            # parse the output from iscsiadm
-            # lines are in the format of
-            # tcp: [1] 192.168.121.250:3260,1 iqn.2010-10.org.openstack:volume-
-            lines = out.split('\n')
-            for line in lines:
-                if line:
-                    entries = line.split()
-                    portal = entries[2].split(',')
-                    iscsi_sessions.append(portal[0])
+                        "iscsiadm err: %s", err)
+            return []
 
-        return iscsi_sessions
+        # Parse and clean the output from iscsiadm, which is in the form of:
+        # transport_name: [session_id] ip_address:port,tpgt iqn node_type
+        lines = []
+        for line in out.splitlines():
+            if line:
+                info = line.split()
+                sid = info[1][1:-1]
+                portal, tpgt = info[2].split(',')
+                lines.append((info[0], sid, portal, tpgt, info[3]))
+        return lines
+
+    def _get_iscsi_nodes(self):
+        """Get iSCSI node information (portal, iqn) as a list of tuples.
+
+        Uses iscsi_adm -m node and from a command output like
+            192.168.121.250:3260,1 iqn.2010-10.org.openstack:volume
+
+        This method will drop the tpgt and return a list like this:
+            [('192.168.121.250:3260', 'iqn.2010-10.org.openstack:volume')]
+        """
+        out, err = self._execute('iscsiadm', '-m', 'node', run_as_root=True,
+                                 root_helper=self._root_helper,
+                                 check_exit_code=False)
+        if err:
+            LOG.warning("Couldn't find iSCSI nodes because iscsiadm err: %s",
+                        err)
+            return []
+
+        # Parse and clean the output from iscsiadm which is in the form of:
+        # ip_addresss:port,tpgt iqn
+        lines = []
+        for line in out.splitlines():
+            if line:
+                info = line.split()
+                lines.append((info[0].split(',')[0], info[1]))
+        return lines
+
+    def _get_iscsi_sessions(self):
+        """Return portals for all existing sessions."""
+        # entry: [tcp, [1], 192.168.121.250:3260,1 ...]
+        return [entry[2] for entry in self._get_iscsi_sessions_full()]
 
     def _get_potential_volume_paths(self, connection_properties,
                                     connect_to_portal=True,
@@ -471,85 +506,103 @@ class ISCSIConnector(base.BaseLinuxConnector, base_iscsi.BaseISCSIConnector):
         LOG.debug("connect_volume returning %s", device_info)
         return device_info
 
+    def _get_connection_devices(self, connection_properties):
+        """Get map of devices by sessions from our connection.
+
+        For each of the TCP sessions that correspond to our connection
+        properties we generate a map of (ip, iqn) to (belong, other) where
+        belong is a set of devices in that session that populated our system
+        when we did a connection using connection properties, and other are
+        any other devices that share that same session but are the result of
+        connecting with different connection properties.
+
+        We also include all nodes from our connection that don't have a
+        session.
+        """
+        ips_iqns_luns = self._get_all_targets(connection_properties)
+        nodes = self._get_iscsi_nodes()
+        sessions = self._get_iscsi_sessions_full()
+        # Use (portal, iqn) to map the session value
+        sessions_map = {(s[2], s[4]): s[1] for s in sessions if s[0] == 'tcp:'}
+        # device_map will keep a tuple with devices from the connection and
+        # others that don't belong to this connection" (belong, others)
+        device_map = collections.defaultdict(lambda: (set(), set()))
+
+        for ip, iqn, lun in ips_iqns_luns:
+            session = sessions_map.get((ip, iqn))
+            # Our nodes that don't have a session will be returned as empty
+            if not session:
+                if (ip, iqn) in nodes:
+                    device_map[(ip, iqn)] = (set(), set())
+                continue
+
+            # Get all devices for the session
+            paths = glob.glob('/sys/class/scsi_host/host*/device/session' +
+                              session + '/target*/*:*:*:*/block/*')
+            belong, others = device_map[(ip, iqn)]
+            for path in paths:
+                __, hctl, __, device = path.rsplit('/', 3)
+                lun_path = int(hctl.rsplit(':', 1)[-1])
+                # For partitions turn them into the whole device: sde1 -> sde
+                device = device.strip('0123456789')
+                if lun_path == lun:
+                    belong.add(device)
+                else:
+                    others.add(device)
+
+        return device_map
+
     @utils.trace
     @synchronized('connect_volume')
-    def disconnect_volume(self, connection_properties, device_info):
+    def disconnect_volume(self, connection_properties, device_info,
+                          force=False, ignore_errors=False):
         """Detach the volume from instance_name.
 
         :param connection_properties: The dictionary that describes all
                                       of the target volume attributes.
-        :type connection_properties: dict
+        :type connection_properties: dict that must include:
+                                     target_portal(s) - IP and optional port
+                                     target_iqn(s) - iSCSI Qualified Name
+                                     target_lun(s) - LUN id of the volume
         :param device_info: historical difference, but same as connection_props
         :type device_info: dict
-
-        connection_properties for iSCSI must include:
-        target_portal(s) - IP and optional port
-        target_iqn(s) - iSCSI Qualified Name
-        target_lun(s) - LUN id of the volume
+        :param force: Whether to forcefully disconnect even if flush fails.
+        :type force: bool
+        :param ignore_errors: When force is True, this will decide whether to
+                              ignore errors or raise an exception once finished
+                              the operation.  Default is False.
+        :type ignore_errors: bool
         """
-        if self.use_multipath:
-            host_device = multipath_device = None
-            host_devices = self._get_device_path(connection_properties)
-            # Choose an accessible host device
-            for dev in host_devices:
-                if os.path.exists(dev):
-                    host_device = dev
-                    device_wwn = self._linuxscsi.get_scsi_wwn(dev)
-                    (multipath_device, multipath_id) = (super(
-                        ISCSIConnector, self)._discover_mpath_device(
-                            device_wwn, connection_properties, dev))
-                    if multipath_device:
-                        break
-            if not host_device:
-                LOG.error("No accessible volume device: %(host_devices)s",
-                          {'host_devices': host_devices})
-                raise exception.VolumeDeviceNotFound(device=host_devices)
+        exc = exception.ExceptionChainer()
+        devices_map = self._get_connection_devices(connection_properties)
 
-            if multipath_device:
-                device_realpath = os.path.realpath(host_device)
-                self._linuxscsi.remove_multipath_device(device_realpath)
-                return self._disconnect_volume_multipath_iscsi(
-                    connection_properties, multipath_device)
+        # Remove devices and multipath from this connection
+        remove_devices = set()
+        for remove, __ in devices_map.values():
+            remove_devices.update(remove)
+        multipath_name = self._linuxscsi.remove_connection(remove_devices,
+                                                           self.use_multipath,
+                                                           force, exc)
 
-        # When multiple portals/iqns/luns are specified, we need to remove
-        # unused devices created by logging into other LUNs' session.
-        for props in self._iterate_all_targets(connection_properties):
-            self._disconnect_volume_iscsi(props)
+        # Disconnect sessions and remove nodes that are left without devices
+        disconnect = [conn for conn, (__, keep) in devices_map.items()
+                      if not keep]
+        self._disconnect_connection(connection_properties, disconnect, force,
+                                    exc)
 
-    def _disconnect_volume_iscsi(self, connection_properties):
-        # remove the device from the scsi subsystem
-        # this eliminates any stale entries until logout
-        host_devices = self._get_device_path(connection_properties)
+        # If flushing the multipath failed before, try now after we have
+        # removed the devices and we may have even logged off (only reaches
+        # here with multipath_name if force=True).
+        if multipath_name:
+            LOG.debug('Flushing again multipath %s now that we removed the '
+                      'devices.', multipath_name)
+            self._linuxscsi.flush_multipath_device(multipath_name)
 
-        if host_devices:
-            host_device = host_devices[0]
-        else:
-            return
-
-        dev_name = self._linuxscsi.get_name_from_path(host_device)
-        if dev_name:
-            self._linuxscsi.remove_scsi_device(dev_name)
-
-            # NOTE(jdg): On busy systems we can have a race here
-            # where remove_iscsi_device is called before the device file
-            # has actually been removed.   The result is an orphaned
-            # iscsi session that never gets logged out.  The following
-            # call to wait addresses that issue.
-            self._linuxscsi.wait_for_volume_removal(host_device)
-
-        # NOTE(vish): Only disconnect from the target if no luns from the
-        #             target are in use.
-        device_byname = ("ip-%(portal)s-iscsi-%(iqn)s-lun-" %
-                         {'portal': connection_properties['target_portal'],
-                          'iqn': connection_properties['target_iqn']})
-        devices = self.driver.get_all_block_devices()
-        devices = [dev for dev in devices if (device_byname in dev
-                                              and
-                                              dev.startswith(
-                                                  '/dev/disk/by-path/'))
-                   and os.path.exists(dev)]
-        if not devices:
-            self._disconnect_from_iscsi_portal(connection_properties)
+        if exc:
+            LOG.warning('There were errors removing %s, leftovers may remain '
+                        'in the system', remove_devices)
+            if not ignore_errors:
+                raise exc
 
     def _munge_portal(self, target):
         """Remove brackets from portal.
@@ -635,65 +688,6 @@ class ISCSIConnector(base.BaseLinuxConnector, base_iscsi.BaseISCSIConnector):
                 iqns.append(data[1])
         return ips, iqns
 
-    def _disconnect_volume_multipath_iscsi(self, connection_properties,
-                                           multipath_name):
-        """This removes a multipath device and it's LUNs."""
-        LOG.debug("Disconnect multipath device %s", multipath_name)
-        mpath_map = self._get_multipath_device_map()
-        block_devices = self.driver.get_all_block_devices()
-        devices = []
-        for dev in block_devices:
-            if os.path.exists(dev):
-                if "/mapper/" in dev:
-                    devices.append(dev)
-                else:
-                    mpdev = mpath_map.get(dev)
-                    if mpdev:
-                        devices.append(mpdev)
-
-        # Do a discovery to find all targets.
-        # Targets for multiple paths for the same multipath device
-        # may not be the same.
-        all_ips_iqns_luns = self._discover_iscsi_portals(connection_properties)
-
-        # As discovery result may contain other targets' iqns, extract targets
-        # to be disconnected whose block devices are already deleted here.
-        ips_iqns = []
-        entries = [device.lstrip('ip-').split('-lun-')[0]
-                   for device in self._get_iscsi_devices()]
-        for ip, iqn, lun in all_ips_iqns_luns:
-            ip_iqn = "%s-iscsi-%s" % (ip.split(",")[0], iqn)
-            if ip_iqn not in entries:
-                ips_iqns.append([ip, iqn])
-
-        if not devices:
-            # disconnect if no other multipath devices
-            self._disconnect_mpath(connection_properties, ips_iqns)
-            return
-
-        # Get a target for all other multipath devices
-        other_iqns = self._get_multipath_iqns(devices, mpath_map)
-
-        # Get all the targets for the current multipath device
-        current_iqns = [iqn for ip, iqn in ips_iqns]
-
-        in_use = False
-        for current in current_iqns:
-            if current in other_iqns:
-                in_use = True
-                break
-
-        # If no other multipath device attached has the same iqn
-        # as the current device
-        if not in_use:
-            # disconnect if no other multipath devices with same iqn
-            self._disconnect_mpath(connection_properties, ips_iqns)
-            return
-
-        # else do not disconnect iscsi portals,
-        # as they are used for other luns
-        return
-
     def _connect_to_iscsi_portal(self, connection_properties):
         # NOTE(vish): If we are on the same host as nova volume, the
         #             discovery makes the target so we don't need to
@@ -774,54 +768,15 @@ class ISCSIConnector(base.BaseLinuxConnector, base_iscsi.BaseISCSIConnector):
                            attempts=5,
                            delay_on_retry=True)
 
-    def _get_iscsi_devices(self):
-        try:
-            devices = list(os.walk('/dev/disk/by-path'))[0][-1]
-        except IndexError:
-            return []
-        # For iSCSI HBAs, look at an offset of len('pci-0000:00:00.0')
-        return [entry for entry in devices if (entry.startswith("ip-")
-                                               or (entry.startswith("pci-")
-                                                   and
-                                                   entry.find("ip-", 16, 21)
-                                                   >= 16))]
-
-    def _disconnect_mpath(self, connection_properties, ips_iqns):
-        for ip, iqn in ips_iqns:
-            props = copy.deepcopy(connection_properties)
+    def _disconnect_connection(self, connection_properties, connections, force,
+                               exc):
+        LOG.debug('Disconnecting from: %s', connections)
+        props = connection_properties.copy()
+        for ip, iqn in connections:
             props['target_portal'] = ip
             props['target_iqn'] = iqn
-            self._disconnect_from_iscsi_portal(props)
-
-    def _get_multipath_iqns(self, multipath_devices, mpath_map):
-        entries = self._get_iscsi_devices()
-        iqns = []
-        for entry in entries:
-            entry_real_path = os.path.realpath("/dev/disk/by-path/%s" % entry)
-            entry_multipath = mpath_map.get(entry_real_path)
-            if entry_multipath and entry_multipath in multipath_devices:
-                iqns.append(entry.split("iscsi-")[1].split("-lun")[0])
-        return iqns
-
-    def _get_multipath_device_map(self):
-        out = self._run_multipath(['-ll'], check_exit_code=[0, 1])[0]
-        mpath_line = [line for line in out.splitlines()
-                      if not re.match(initiator.MULTIPATH_ERROR_REGEX, line)]
-        mpath_dev = None
-        mpath_map = {}
-        for line in out.splitlines():
-            m = initiator.MULTIPATH_DEV_CHECK_REGEX.split(line)
-            if len(m) >= 2:
-                mpath_dev = '/dev/mapper/' + m[0].split(" ")[0]
-                continue
-            m = initiator.MULTIPATH_PATH_CHECK_REGEX.split(line)
-            if len(m) >= 2:
-                mpath_map['/dev/' + m[1].split(" ")[0]] = mpath_dev
-
-        if mpath_line and not mpath_map:
-            LOG.warning("Failed to parse the output of multipath -ll. "
-                        "stdout: %s", out)
-        return mpath_map
+            with exc.context(force, 'Disconnect from %s %s failed', ip, iqn):
+                self._disconnect_from_iscsi_portal(props)
 
     def _run_iscsi_session(self):
         (out, err) = self._run_iscsiadm_bare(('-m', 'session'),

os_brick/initiator/connectors/local.py

@@ -62,7 +62,8 @@ class LocalConnector(base.BaseLinuxConnector):
         return device_info
 
     @utils.trace
-    def disconnect_volume(self, connection_properties, device_info):
+    def disconnect_volume(self, connection_properties, device_info,
+                          force=False, ignore_errors=False):
         """Disconnect a volume from the local host.
 
         :param connection_properties: The dictionary that describes all

os_brick/initiator/connectors/rbd.py

@@ -192,7 +192,8 @@ class RBDConnector(base.BaseLinuxConnector):
         return {'path': rbd_handle}
 
     @utils.trace
-    def disconnect_volume(self, connection_properties, device_info):
+    def disconnect_volume(self, connection_properties, device_info,
+                          force=False, ignore_errors=False):
         """Disconnect a volume.
 
         :param connection_properties: The dictionary that describes all

os_brick/initiator/connectors/remotefs.py

@@ -105,7 +105,8 @@ class RemoteFsConnector(base.BaseLinuxConnector):
         return {'path': path}
 
     @utils.trace
-    def disconnect_volume(self, connection_properties, device_info):
+    def disconnect_volume(self, connection_properties, device_info,
+                          force=False, ignore_errors=False):
         """No need to do anything to disconnect a volume in a filesystem.
 
         :param connection_properties: The dictionary that describes all

os_brick/initiator/connectors/scaleio.py

@@ -410,7 +410,8 @@ class ScaleIOConnector(base.BaseLinuxConnector):
 
     @utils.trace
     @lockutils.synchronized('scaleio', 'scaleio-')
-    def disconnect_volume(self, connection_properties, device_info):
+    def disconnect_volume(self, connection_properties, device_info,
+                          force=False, ignore_errors=False):
         """Disconnect the ScaleIO volume.
 
         :param connection_properties: The dictionary that describes all

os_brick/initiator/connectors/sheepdog.py

@@ -83,7 +83,8 @@ class SheepdogConnector(base.BaseLinuxConnector):
         return {'path': sheepdog_handle}
 
     @utils.trace
-    def disconnect_volume(self, connection_properties, device_info):
+    def disconnect_volume(self, connection_properties, device_info,
+                          force=False, ignore_errors=False):
         """Disconnect a volume.
 
         :param connection_properties: The dictionary that describes all

os_brick/initiator/connectors/vmware.py

@@ -237,7 +237,8 @@ class VmdkConnector(initiator_connector.InitiatorConnector):
                                   datacenter=dc_ref)
         session.wait_for_task(task)
 
-    def disconnect_volume(self, connection_properties, device_info):
+    def disconnect_volume(self, connection_properties, device_info,
+                          force=False, ignore_errors=False):
         tmp_file_path = device_info['path']
         if not os.path.exists(tmp_file_path):
             msg = _("Vmdk: %s not found.") % tmp_file_path

os_brick/initiator/connectors/vrtshyperscale.py

@@ -127,7 +127,8 @@ class HyperScaleConnector(base.BaseLinuxConnector):
 
     @utils.trace
     @synchronized('connect_volume')
-    def disconnect_volume(self, connection_properties, device_info):
+    def disconnect_volume(self, connection_properties, device_info,
+                          force=False, ignore_errors=False):
         """Disconnect a volume from an instance."""
         volume_name = None
 

os_brick/initiator/initiator_connector.py

@@ -112,7 +112,8 @@ class InitiatorConnector(executor.Executor):
         pass
 
     @abc.abstractmethod
-    def disconnect_volume(self, connection_properties, device_info):
+    def disconnect_volume(self, connection_properties, device_info,
+                          force=False, ignore_errors=False):
         """Disconnect a volume from the local host.
 
         The connection_properties are the same as from connect_volume.
@@ -123,6 +124,12 @@ class InitiatorConnector(executor.Executor):
         :type connection_properties: dict
         :param device_info: historical difference, but same as connection_props
         :type device_info: dict
+        :param force: Whether to forcefully disconnect even if flush fails.
+        :type force: bool
+        :param ignore_errors: When force is True, this will decide whether to
+                              ignore errors or raise an exception once finished
+                              the operation.  Default is False.
+        :type ignore_errors: bool
         """
         pass
 

os_brick/initiator/linuxscsi.py

@@ -16,6 +16,7 @@
 
    Note, this is not iSCSI.
 """
+import glob
 import os
 import re
 import six
@@ -56,30 +57,32 @@ class LinuxSCSI(executor.Executor):
         else:
             return None
 
-    def remove_scsi_device(self, device):
+    def remove_scsi_device(self, device, force=False, exc=None):
         """Removes a scsi device based upon /dev/sdX name."""
-
         path = "/sys/block/%s/device/delete" % device.replace("/dev/", "")
         if os.path.exists(path):
+            exc = exception.ExceptionChainer() if exc is None else exc
             # flush any outstanding IO first
-            self.flush_device_io(device)
+            with exc.context(force, 'Flushing %s failed', device):
+                self.flush_device_io(device)
 
             LOG.debug("Remove SCSI device %(device)s with %(path)s",
                       {'device': device, 'path': path})
-            self.echo_scsi_command(path, "1")
+            with exc.context(force, 'Removing %s failed', device):
+                self.echo_scsi_command(path, "1")
 
-    @utils.retry(exceptions=exception.VolumePathNotRemoved, retries=3,
-                 backoff_rate=2)
-    def wait_for_volume_removal(self, volume_path):
-        """This is used to ensure that volumes are gone."""
-        LOG.debug("Checking to see if SCSI volume %s has been removed.",
-                  volume_path)
-        if os.path.exists(volume_path):
-            LOG.debug("%(path)s still exists.", {'path': volume_path})
-            raise exception.VolumePathNotRemoved(
-                volume_path=volume_path)
-        else:
-            LOG.debug("SCSI volume %s has been removed.", volume_path)
+    @utils.retry(exceptions=exception.VolumePathNotRemoved)
+    def wait_for_volumes_removal(self, volumes_names):
+        """Wait for device paths to be removed from the system."""
+        str_names = ', '.join(volumes_names)
+        LOG.debug('Checking to see if SCSI volumes %s have been removed.',
+                  str_names)
+        exist = [volume_name for volume_name in volumes_names
+                 if os.path.exists('/dev/' + volume_name)]
+        if exist:
+            LOG.debug('%s still exist.', ', '.join(exist))
+            raise exception.VolumePathNotRemoved(volume_path=exist)
+        LOG.debug("SCSI volumes %s have been removed.", str_names)
 
     def get_device_info(self, device):
         (out, _err) = self._execute('sg_scan', device, run_as_root=True,
@@ -125,52 +128,92 @@ class LinuxSCSI(executor.Executor):
 
         return True
 
-    def remove_multipath_device(self, device):
-        """Removes related LUNs and multipath device
+    def get_dm_name(self, dm):
+        """Get the Device map name given the device name of the dm on sysfs.
 
-        This removes LUNs associated with a multipath device
-        and the multipath device itself.
+        :param dm: Device map name as seen in sysfs. ie: 'dm-0'
+        :returns: String with the name, or empty string if not available.
+                  ie: '36e843b658476b7ed5bc1d4d10d9b1fde'
         """
+        try:
+            with open('/sys/block/' + dm + '/dm/name') as f:
+                return f.read().strip()
+        except IOError:
+            return ''
 
-        LOG.debug("remove multipath device %s", device)
-        mpath_dev = self.find_multipath_device(device)
-        if mpath_dev:
-            self.flush_multipath_device(mpath_dev['name'])
-            devices = mpath_dev['devices']
-            LOG.debug("multipath LUNs to remove %s", devices)
-            for device in devices:
-                self.remove_scsi_device(device['device'])
+    def find_sysfs_multipath_dm(self, device_names):
+        """Find the dm device name given a list of device names
+
+        :param device_names: Iterable with device names, not paths. ie: ['sda']
+        :returns: String with the dm name or None if not found. ie: 'dm-0'
+        """
+        glob_str = '/sys/block/%s/holders/dm-*'
+        for dev_name in device_names:
+            dms = glob.glob(glob_str % dev_name)
+            if dms:
+                __, device_name, __, dm = dms[0].rsplit('/', 3)
+                return dm
+        return None
+
+    def remove_connection(self, devices_names, is_multipath, force=False,
+                          exc=None):
+        """Remove LUNs and multipath associated with devices names.
+
+        :param devices_names: Iterable with real device names ('sda', 'sdb')
+        :param is_multipath: Whether this is a multipath connection or not
+        :param force: Whether to forcefully disconnect even if flush fails.
+        :param exc: ExceptionChainer where to add exceptions if forcing
+        :returns: Multipath device map name if found and not flushed
+        """
+        if not devices_names:
+            return
+        multipath_name = None
+        exc = exception.ExceptionChainer() if exc is None else exc
+        LOG.debug('Removing %(type)s devices %(devices)s',
+                  {'type': 'multipathed' if is_multipath else 'single pathed',
+                   'devices': ', '.join(devices_names)})
+
+        if is_multipath:
+            multipath_dm = self.find_sysfs_multipath_dm(devices_names)
+            multipath_name = multipath_dm and self.get_dm_name(multipath_dm)
+            if multipath_name:
+                with exc.context(force, 'Flushing %s failed', multipath_name):
+                    self.flush_multipath_device(multipath_name)
+                    multipath_name = None
+
+        for device_name in devices_names:
+            self.remove_scsi_device('/dev/' + device_name, force, exc)
+
+        # Wait until the symlinks are removed
+        with exc.context(force, 'Some devices remain from %s', devices_names):
+            self.wait_for_volumes_removal(devices_names)
+
+        return multipath_name
 
     def flush_device_io(self, device):
         """This is used to flush any remaining IO in the buffers."""
-        try:
-            LOG.debug("Flushing IO for device %s", device)
-            self._execute('blockdev', '--flushbufs', device, run_as_root=True,
-                          root_helper=self._root_helper)
-        except putils.ProcessExecutionError as exc:
-            LOG.warning("Failed to flush IO buffers prior to removing "
-                        "device: %(code)s", {'code': exc.exit_code})
-
-    @utils.retry(exceptions=putils.ProcessExecutionError)
-    def flush_multipath_device(self, device):
-        try:
-            LOG.debug("Flush multipath device %s", device)
-            self._execute('multipath', '-f', device, run_as_root=True,
-                          root_helper=self._root_helper)
-        except putils.ProcessExecutionError as exc:
-            if exc.exit_code == 1 and 'map in use' in exc.stdout:
-                LOG.debug('Multipath is in use, cannot be flushed yet.')
+        if os.path.exists(device):
+            try:
+                # NOTE(geguileo): With 30% connection error rates flush can get
+                # stuck, set timeout to prevent it from hanging here forever.
+                # Retry twice after 20 and 40 seconds.
+                LOG.debug("Flushing IO for device %s", device)
+                self._execute('blockdev', '--flushbufs', device,
+                              run_as_root=True, attempts=3, timeout=300,
+                              interval=10, root_helper=self._root_helper)
+            except putils.ProcessExecutionError as exc:
+                LOG.warning("Failed to flush IO buffers prior to removing "
+                            "device: %(code)s", {'code': exc.exit_code})
                 raise
-            LOG.warning("multipath call failed exit %(code)s",
-                        {'code': exc.exit_code})
 
-    def flush_multipath_devices(self):
-        try:
-            self._execute('multipath', '-F', run_as_root=True,
-                          root_helper=self._root_helper)
-        except putils.ProcessExecutionError as exc:
-            LOG.warning("multipath call failed exit %(code)s",
-                        {'code': exc.exit_code})
+    def flush_multipath_device(self, device_map_name):
+        LOG.debug("Flush multipath device %s", device_map_name)
+        # NOTE(geguileo): With 30% connection error rates flush can get stuck,
+        # set timeout to prevent it from hanging here forever.  Retry twice
+        # after 20 and 40 seconds.
+        self._execute('multipath', '-f', device_map_name, run_as_root=True,
+                      attempts=3, timeout=300, interval=10,
+                      root_helper=self._root_helper)
 
     @utils.retry(exceptions=exception.VolumeDeviceNotFound)
     def wait_for_path(self, volume_path):

os_brick/initiator/windows/fibre_channel.py

@@ -126,5 +126,6 @@ class WindowsFCConnector(win_conn_base.BaseWindowsConnector):
         return mappings
 
     @utils.trace
-    def disconnect_volume(self, connection_properties):
+    def disconnect_volume(self, connection_properties,
+                          force=False, ignore_errors=False):
         pass

os_brick/initiator/windows/iscsi.py

@@ -134,7 +134,8 @@ class WindowsISCSIConnector(win_conn_base.BaseWindowsConnector,
         return device_info
 
     @utils.trace
-    def disconnect_volume(self, connection_properties):
+    def disconnect_volume(self, connection_properties,
+                          force=False, ignore_errors=False):
         # We want to refresh the cached information first.
         self._diskutils.rescan_disks()
         for (target_portal,

os_brick/initiator/windows/smbfs.py

@@ -49,7 +49,8 @@ class WindowsSMBFSConnector(win_conn_base.BaseWindowsConnector):
         return device_info
 
     @utils.trace
-    def disconnect_volume(self, connection_properties):
+    def disconnect_volume(self, connection_properties,
+                          force=False, ignore_errors=False):
         export_path = self._get_export_path(connection_properties)
         self._remotefsclient.unmount(export_path)
 

os_brick/privileged/rootwrap.py

@@ -36,14 +36,126 @@ the urgency of (1)), then work on the larger refactor that addresses
 
 """
 
+import signal
 import six
+import threading
+import time
 
 from oslo_concurrency import processutils as putils
+from oslo_log import log as logging
 from oslo_utils import strutils
 
+from os_brick import exception
 from os_brick import privileged
 
 
+LOG = logging.getLogger(__name__)
+
+
+def custom_execute(*cmd, **kwargs):
+    """Custom execute with additional functionality on top of Oslo's.
+
+    Additional features are timeouts and exponential backoff retries.
+
+    The exponential backoff retries replaces standard Oslo random sleep times
+    that range from 200ms to 2seconds when attempts is greater than 1, but it
+    is disabled if delay_on_retry is passed as a parameter.
+
+    Exponential backoff is controlled via interval and backoff_rate parameters,
+    just like the os_brick.utils.retry decorator.
+
+    To use the timeout mechanism to stop the subprocess with a specific signal
+    after a number of seconds we must pass a non-zero timeout value in the
+    call.
+
+    When using multiple attempts and timeout at the same time the method will
+    only raise the timeout exception to the caller if the last try timeouts.
+
+    Timeout mechanism is controlled with timeout, signal, and raise_timeout
+    parameters.
+
+    :param interval: The multiplier
+    :param backoff_rate: Base used for the exponential backoff
+    :param timeout: Timeout defined in seconds
+    :param signal: Signal to use to stop the process on timeout
+    :param raise_timeout: Raise and exception on timeout or return error as
+                          stderr.  Defaults to raising if check_exit_code is
+                          not False.
+    :returns: Tuple with stdout and stderr
+    """
+    # Since python 2 doesn't have nonlocal we use a mutable variable to store
+    # the previous attempt number, the timeout handler, and the process that
+    # timed out
+    shared_data = [0, None, None]
+
+    def on_timeout(proc):
+        sanitized_cmd = strutils.mask_password(' '.join(cmd))
+        LOG.warning('Stopping %(cmd)s with signal %(signal)s after %(time)ss.',
+                    {'signal': sig_end, 'cmd': sanitized_cmd, 'time': timeout})
+        shared_data[2] = proc
+        proc.send_signal(sig_end)
+
+    def on_execute(proc):
+        # Call user's on_execute method
+        if on_execute_call:
+            on_execute_call(proc)
+        # Sleep if this is not the first try and we have a timeout interval
+        if shared_data[0] and interval:
+            exp = backoff_rate ** shared_data[0]
+            wait_for = max(0, interval * exp)
+            LOG.debug('Sleeping for %s seconds', wait_for)
+            time.sleep(wait_for)
+        # Increase the number of tries and start the timeout timer
+        shared_data[0] += 1
+        if timeout:
+            shared_data[2] = None
+            shared_data[1] = threading.Timer(timeout, on_timeout, (proc,))
+            shared_data[1].start()
+
+    def on_completion(proc):
+        # This is always called regardless of success or failure
+        # Cancel the timeout timer
+        if shared_data[1]:
+            shared_data[1].cancel()
+        # Call user's on_completion method
+        if on_completion_call:
+            on_completion_call(proc)
+
+    # We will be doing the wait ourselves in on_execute
+    if 'delay_on_retry' in kwargs:
+        interval = None
+    else:
+        kwargs['delay_on_retry'] = False
+        interval = kwargs.pop('interval', 1)
+        backoff_rate = kwargs.pop('backoff_rate', 2)
+
+    timeout = kwargs.pop('timeout', None)
+    sig_end = kwargs.pop('signal', signal.SIGTERM)
+    default_raise_timeout = kwargs.get('check_exit_code', True)
+    raise_timeout = kwargs.pop('raise_timeout', default_raise_timeout)
+
+    on_execute_call = kwargs.pop('on_execute', None)
+    on_completion_call = kwargs.pop('on_completion', None)
+
+    try:
+        return putils.execute(on_execute=on_execute,
+                              on_completion=on_completion, *cmd, **kwargs)
+    except putils.ProcessExecutionError:
+        # proc is only stored if a timeout happened
+        proc = shared_data[2]
+        if proc:
+            sanitized_cmd = strutils.mask_password(' '.join(cmd))
+            msg = ('Time out on proc %(pid)s after waiting %(time)s seconds '
+                   'when running %(cmd)s' %
+                   {'pid': proc.pid, 'time': timeout, 'cmd': sanitized_cmd})
+            LOG.debug(msg)
+            if raise_timeout:
+                raise exception.ExecutionTimeout(stdout='', stderr=msg,
+                                                 cmd=sanitized_cmd)
+            return '', msg
+        raise
+
+
 # Entrypoint used for rootwrap.py transition code.  Don't use this for
 # other purposes, since it will be removed when we think the
 # transition is finished.
@@ -51,12 +163,11 @@ def execute(*cmd, **kwargs):
     """NB: Raises processutils.ProcessExecutionError on failure."""
     run_as_root = kwargs.pop('run_as_root', False)
     kwargs.pop('root_helper', None)
-
     try:
         if run_as_root:
             return execute_root(*cmd, **kwargs)
         else:
-            return putils.execute(*cmd, **kwargs)
+            return custom_execute(*cmd, **kwargs)
     except OSError as e:
         # Note:
         #  putils.execute('bogus', run_as_root=True)
@@ -79,4 +190,4 @@ def execute(*cmd, **kwargs):
 @privileged.default.entrypoint
 def execute_root(*cmd, **kwargs):
     """NB: Raises processutils.ProcessExecutionError/OSError on failure."""
-    return putils.execute(*cmd, shell=False, run_as_root=False, **kwargs)
+    return custom_execute(*cmd, shell=False, run_as_root=False, **kwargs)

os_brick/tests/initiator/connectors/test_iscsi.py

@@ -11,12 +11,14 @@
 #    WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
 #    License for the specific language governing permissions and limitations
 #    under the License.
+import collections
 import glob
 import mock
 import os
 import testtools
 import time
 
+import ddt
 from oslo_concurrency import processutils as putils
 
 from os_brick import exception
@@ -28,18 +30,117 @@ from os_brick.privileged import rootwrap as priv_rootwrap
 from os_brick.tests.initiator import test_connector
 
 
+@ddt.ddt
 class ISCSIConnectorTestCase(test_connector.ConnectorTestCase):
+    CON_PROPS = {
+        'volume_id': 'vol_id',
+        'target_portal': 'ip1:port1',
+        'target_iqn': 'tgt1',
+        'target_lun': 4,
+        'target_portals': ['ip1:port1', 'ip2:port2', 'ip3:port3',
+                           'ip4:port4'],
+        'target_iqns': ['tgt1', 'tgt2', 'tgt3', 'tgt4'],
+        'target_luns': [4, 5, 6, 7],
+    }
 
     def setUp(self):
         super(ISCSIConnectorTestCase, self).setUp()
         self.connector = iscsi.ISCSIConnector(
             None, execute=self.fake_execute, use_multipath=False)
+
         self.connector_with_multipath = iscsi.ISCSIConnector(
             None, execute=self.fake_execute, use_multipath=True)
-
         self.mock_object(self.connector._linuxscsi, 'get_name_from_path',
                          return_value="/dev/sdb")
         self._fake_iqn = 'iqn.1234-56.foo.bar:01:23456789abc'
+        self._name = 'volume-00000001'
+        self._iqn = 'iqn.2010-10.org.openstack:%s' % self._name
+        self._location = '10.0.2.15:3260'
+        self._lun = 1
+
+    @mock.patch.object(iscsi.ISCSIConnector, '_run_iscsi_session')
+    def test_get_iscsi_sessions_full(self, sessions_mock):
+        iscsiadm_result = ('tcp: [session1] ip1:port1,1 tgt1 (non-flash)\n'
+                           'tcp: [session2] ip2:port2,-1 tgt2 (non-flash)\n'
+                           'tcp: [session3] ip3:port3,1 tgt3\n')
+        sessions_mock.return_value = (iscsiadm_result, '')
+        res = self.connector._get_iscsi_sessions_full()
+        expected = [('tcp:', 'session1', 'ip1:port1', '1', 'tgt1'),
+                    ('tcp:', 'session2', 'ip2:port2', '-1', 'tgt2'),
+                    ('tcp:', 'session3', 'ip3:port3', '1', 'tgt3')]
+        self.assertListEqual(expected, res)
+
+    @mock.patch.object(iscsi.ISCSIConnector, '_run_iscsi_session',
+                       return_value=(None, 'error'))
+    def test_get_iscsi_sessions_full_error(self, sessions_mock):
+        res = self.connector._get_iscsi_sessions_full()
+        self.assertEqual([], res)
+        sessions_mock.assert_called()
+
+    @mock.patch.object(iscsi.ISCSIConnector, '_get_iscsi_sessions_full')
+    def test_get_iscsi_sessions(self, sessions_mock):
+        sessions_mock.return_value = [
+            ('tcp:', 'session1', 'ip1:port1', '1', 'tgt1'),
+            ('tcp:', 'session2', 'ip2:port2', '-1', 'tgt2'),
+            ('tcp:', 'session3', 'ip3:port3', '1', 'tgt3')]
+        res = self.connector._get_iscsi_sessions()
+        expected = ['ip1:port1', 'ip2:port2', 'ip3:port3']
+        self.assertListEqual(expected, res)
+
+    @mock.patch.object(iscsi.ISCSIConnector, '_get_iscsi_sessions_full',
+                       return_value=[])
+    def test_get_iscsi_sessions_no_sessions(self, sessions_mock):
+        res = self.connector._get_iscsi_sessions()
+        self.assertListEqual([], res)
+        sessions_mock.assert_called()
+
+    @mock.patch.object(iscsi.ISCSIConnector, '_execute')
+    def test_get_iscsi_nodes(self, exec_mock):
+        iscsiadm_result = ('ip1:port1,1 tgt1\nip2:port2,-1 tgt2\n'
+                           'ip3:port3,1 tgt3\n')
+        exec_mock.return_value = (iscsiadm_result, '')
+        res = self.connector._get_iscsi_nodes()
+        expected = [('ip1:port1', 'tgt1'), ('ip2:port2', 'tgt2'),
+                    ('ip3:port3', 'tgt3')]
+        self.assertListEqual(expected, res)
+        exec_mock.assert_called_once_with(
+            'iscsiadm', '-m', 'node', run_as_root=True,
+            root_helper=self.connector._root_helper, check_exit_code=False)
+
+    @mock.patch.object(iscsi.ISCSIConnector, '_execute')
+    def test_get_iscsi_nodes_error(self, exec_mock):
+        exec_mock.return_value = (None, 'error')
+        res = self.connector._get_iscsi_nodes()
+        self.assertEqual([], res)
+
+    @mock.patch('glob.glob')
+    @mock.patch.object(iscsi.ISCSIConnector, '_get_iscsi_sessions_full')
+    @mock.patch.object(iscsi.ISCSIConnector, '_get_iscsi_nodes')
+    def test_get_connection_devices(self, nodes_mock, sessions_mock,
+                                    glob_mock):
+        # List sessions from other targets and non tcp sessions
+        sessions_mock.return_value = [
+            ('non-tcp:', '0', 'ip1:port1', '1', 'tgt1'),
+            ('tcp:', '1', 'ip1:port1', '1', 'tgt1'),
+            ('tcp:', '2', 'ip2:port2', '-1', 'tgt2'),
+            ('tcp:', '3', 'ip1:port1', '1', 'tgt4'),
+            ('tcp:', '4', 'ip2:port2', '-1', 'tgt5')]
+        # List 1 node without sessions
+        nodes_mock.return_value = [('ip1:port1', 'tgt1'),
+                                   ('ip2:port2', 'tgt2'),
+                                   ('ip3:port3', 'tgt3')]
+        sys_cls = '/sys/class/scsi_host/host'
+        glob_mock.side_effect = [
+            [sys_cls + '1/device/session1/target6/1:2:6:4/block/sda',
+             sys_cls + '1/device/session1/target6/1:2:6:4/block/sda1'],
+            [sys_cls + '2/device/session2/target7/2:2:7:5/block/sdb',
+             sys_cls + '2/device/session2/target7/2:2:7:4/block/sdc'],
+        ]
+        res = self.connector._get_connection_devices(self.CON_PROPS)
+        expected = {('ip1:port1', 'tgt1'): ({'sda'}, set()),
+                    ('ip2:port2', 'tgt2'): ({'sdb'}, {'sdc'}),
+                    ('ip3:port3', 'tgt3'): (set(), set())}
+        self.assertDictEqual(expected, res)
 
     def generate_device(self, location, iqn, transport=None, lun=1):
         dev_format = "ip-%s-iscsi-%s-lun-%s" % (location, iqn, lun)
@@ -236,70 +337,45 @@ class ISCSIConnectorTestCase(test_connector.ConnectorTestCase):
         self.assertEqual(expected_result, result)
 
     @mock.patch('time.sleep', mock.Mock())
+    @mock.patch.object(iscsi.ISCSIConnector, 'disconnect_volume')
     def _test_connect_volume(self, extra_props, additional_commands,
-                             transport=None, disconnect_mock=None):
+                             disconnect_vol_mock, transport=None):
         # for making sure the /dev/disk/by-path is gone
         exists_mock = mock.Mock()
         exists_mock.return_value = True
         os.path.exists = exists_mock
 
-        location = '10.0.2.15:3260'
-        name = 'volume-00000001'
-        iqn = 'iqn.2010-10.org.openstack:%s' % name
-        vol = {'id': 1, 'name': name}
-        connection_info = self.iscsi_connection(vol, location, iqn)
+        vol = {'id': 1, 'name': self._name}
+        connection_info = self.iscsi_connection(vol, self._location, self._iqn)
         for key, value in extra_props.items():
             connection_info['data'][key] = value
         if transport is not None:
-            dev_list = self.generate_device(location, iqn, transport)
+            dev_list = self.generate_device(self._location, self._iqn,
+                                            transport)
             with mock.patch.object(glob, 'glob', return_value=[dev_list]):
                 device = self.connector.connect_volume(connection_info['data'])
         else:
             device = self.connector.connect_volume(connection_info['data'])
 
-        dev_str = self.generate_device(location, iqn, transport)
+        dev_str = self.generate_device(self._location, self._iqn, transport)
         self.assertEqual(device['type'], 'block')
         self.assertEqual(device['path'], dev_str)
 
         self.count = 0
 
-        def mock_exists_effect(*args, **kwargs):
-            self.count = self.count + 1
-            if self.count == 4:
-                return False
-            else:
-                return True
+        # Disconnect has its own tests, should not be tested here
+        expected_commands = [
+            ('iscsiadm -m node -T %s -p %s' % (self._iqn, self._location)),
+            ('iscsiadm -m session'),
+            ('iscsiadm -m node -T %s -p %s --login' % (self._iqn,
+                                                       self._location)),
+            ('iscsiadm -m node -T %s -p %s --op update'
+             ' -n node.startup -v automatic' % (self._iqn,
+                                                self._location)),
+            ('/lib/udev/scsi_id --page 0x83 --whitelisted %s' % dev_str),
+        ] + additional_commands
 
-        if disconnect_mock is None:
-            disconnect_mock = mock_exists_effect
-
-        with mock.patch.object(os.path, 'exists',
-                               side_effect=disconnect_mock):
-            if transport is not None:
-                dev_list = self.generate_device(location, iqn, transport)
-                with mock.patch.object(glob, 'glob', return_value=[dev_list]):
-                    self.connector.disconnect_volume(connection_info['data'],
-                                                     device)
-            else:
-                self.connector.disconnect_volume(connection_info['data'],
-                                                 device)
-
-            expected_commands = [
-                ('iscsiadm -m node -T %s -p %s' % (iqn, location)),
-                ('iscsiadm -m session'),
-                ('iscsiadm -m node -T %s -p %s --login' % (iqn, location)),
-                ('iscsiadm -m node -T %s -p %s --op update'
-                 ' -n node.startup -v automatic' % (iqn, location)),
-                ('/lib/udev/scsi_id --page 0x83 --whitelisted %s' % dev_str),
-                ('blockdev --flushbufs /dev/sdb'),
-                ('tee -a /sys/block/sdb/device/delete'),
-                ('iscsiadm -m node -T %s -p %s --op update'
-                 ' -n node.startup -v manual' % (iqn, location)),
-                ('iscsiadm -m node -T %s -p %s --logout' % (iqn, location)),
-                ('iscsiadm -m node -T %s -p %s --op delete' %
-                 (iqn, location)), ] + additional_commands
-
-            self.assertEqual(expected_commands, self.cmds)
+        self.assertEqual(expected_commands, self.cmds)
 
     @testtools.skipUnless(os.path.exists('/dev/disk/by-path'),
                           'Test requires /dev/disk/by-path')
@@ -311,63 +387,26 @@ class ISCSIConnectorTestCase(test_connector.ConnectorTestCase):
     @mock.patch.object(iscsi.ISCSIConnector, '_get_transport')
     def test_connect_volume_with_transport(self, mock_transport):
         mock_transport.return_value = 'fake_transport'
-        self._test_connect_volume({}, [], 'fake_transport')
+        self._test_connect_volume({}, [], transport='fake_transport')
 
     @testtools.skipUnless(os.path.exists('/dev/disk/by-path'),
                           'Test requires /dev/disk/by-path')
-    def test_connect_volume_with_alternative_targets(self):
-        location = '10.0.2.15:3260'
-        location2 = '[2001:db8::1]:3260'
-        iqn = 'iqn.2010-10.org.openstack:volume-00000001'
-        iqn2 = 'iqn.2010-10.org.openstack:volume-00000001-2'
-        extra_props = {'target_portals': [location, location2],
-                       'target_iqns': [iqn, iqn2],
-                       'target_luns': [1, 2]}
-        additional_commands = [('blockdev --flushbufs /dev/sdb'),
-                               ('tee -a /sys/block/sdb/device/delete'),
-                               ('iscsiadm -m node -T %s -p %s --op update'
-                                ' -n node.startup -v manual' %
-                                (iqn2, location2)),
-                               ('iscsiadm -m node -T %s -p %s --logout' %
-                                (iqn2, location2)),
-                               ('iscsiadm -m node -T %s -p %s --op delete' %
-                                (iqn2, location2))]
-
-        def mock_exists_effect(*args, **kwargs):
-            self.count = self.count + 1
-            # we have 2 targets in this test, so we need
-            # to make sure we remove and detect removal
-            # for both.
-            if (self.count == 4 or
-               self.count == 8):
-                return False
-            else:
-                return True
-
-        self._test_connect_volume(extra_props, additional_commands,
-                                  disconnect_mock=mock_exists_effect)
-
-    @testtools.skipUnless(os.path.exists('/dev/disk/by-path'),
-                          'Test requires /dev/disk/by-path')
-    @mock.patch.object(os.path, 'exists')
+    @mock.patch('os.path.exists', side_effect=(True,) * 4 + (False, False))
     @mock.patch.object(iscsi.ISCSIConnector, '_run_iscsiadm')
     def test_connect_volume_with_alternative_targets_primary_error(
             self, mock_iscsiadm, mock_exists):
-        location = '10.0.2.15:3260'
         location2 = '[2001:db8::1]:3260'
         dev_loc2 = '2001:db8::1:3260'  # udev location2
-        name = 'volume-00000001'
-        iqn = 'iqn.2010-10.org.openstack:%s' % name
-        iqn2 = 'iqn.2010-10.org.openstack:%s-2' % name
-        vol = {'id': 1, 'name': name}
-        connection_info = self.iscsi_connection(vol, location, iqn)
-        connection_info['data']['target_portals'] = [location, location2]
-        connection_info['data']['target_iqns'] = [iqn, iqn2]
-        connection_info['data']['target_luns'] = [1, 2]
+        iqn2 = 'iqn.2010-10.org.openstack:%s-2' % self._name
+        vol = {'id': 1, 'name': self._name}
+        connection_info = self.iscsi_connection(vol, self._location, self._iqn)
+        connection_info['data']['target_portals'] = [self._location, location2]
+        connection_info['data']['target_iqns'] = [self._iqn, iqn2]
+        connection_info['data']['target_luns'] = [self._lun, 2]
         dev_str2 = '/dev/disk/by-path/ip-%s-iscsi-%s-lun-2' % (dev_loc2, iqn2)
 
         def fake_run_iscsiadm(iscsi_properties, iscsi_command, **kwargs):
-            if iscsi_properties['target_portal'] == location:
+            if iscsi_properties['target_portal'] == self._location:
                 if iscsi_command == ('--login',):
                     raise putils.ProcessExecutionError(None, None, 21)
             return mock.DEFAULT
@@ -386,21 +425,6 @@ class ISCSIConnectorTestCase(test_connector.ConnectorTestCase):
         mock_iscsiadm.assert_any_call(props, ('--login',),
                                       check_exit_code=[0, 255])
 
-        mock_iscsiadm.reset_mock()
-        with mock.patch.object(os.path, 'exists',
-                               return_value=False):
-            self.connector.disconnect_volume(connection_info['data'], device)
-            props = connection_info['data'].copy()
-            for key in ('target_portals', 'target_iqns', 'target_luns'):
-                props.pop(key, None)
-            mock_iscsiadm.assert_any_call(props, ('--logout',),
-                                          check_exit_code=[0, 21, 255])
-            props['target_portal'] = location2
-            props['target_iqn'] = iqn2
-            props['target_lun'] = 2
-            mock_iscsiadm.assert_any_call(props, ('--logout',),
-                                          check_exit_code=[0, 21, 255])
-
     @mock.patch.object(linuxscsi.LinuxSCSI, 'get_scsi_wwn')
     @mock.patch.object(iscsi.ISCSIConnector, '_run_iscsiadm_bare')
     @mock.patch.object(iscsi.ISCSIConnector,
@@ -528,42 +552,36 @@ class ISCSIConnectorTestCase(test_connector.ConnectorTestCase):
     @mock.patch.object(iscsi.ISCSIConnector,
                        '_get_hosts_channels_targets_luns', return_value=[])
     @mock.patch.object(linuxscsi.LinuxSCSI, 'get_scsi_wwn')
-    @mock.patch.object(os.path, 'exists', return_value=True)
+    @mock.patch('os.path.exists', side_effect=(True,) * 7 + (False, False))
     @mock.patch.object(host_driver.HostDriver, 'get_all_block_devices')
-    @mock.patch.object(iscsi.ISCSIConnector, '_get_multipath_device_map',
-                       return_value={})
-    @mock.patch.object(iscsi.ISCSIConnector, '_get_iscsi_devices')
     @mock.patch.object(iscsi.ISCSIConnector, '_run_multipath')
-    @mock.patch.object(iscsi.ISCSIConnector, '_get_multipath_iqns')
     @mock.patch.object(base.BaseLinuxConnector, '_discover_mpath_device')
     @mock.patch.object(linuxscsi.LinuxSCSI, 'process_lun_id')
     def test_connect_volume_with_multiple_portals(
             self, mock_process_lun_id, mock_discover_mpath_device,
-            mock_get_iqn, mock_run_multipath, mock_iscsi_devices,
-            mock_get_device_map, mock_devices, mock_exists, mock_scsi_wwn,
+            mock_run_multipath, mock_devices, mock_exists, mock_scsi_wwn,
             mock_get_htcls):
         mock_scsi_wwn.return_value = test_connector.FAKE_SCSI_WWN
-        location1 = '10.0.2.15:3260'
         location2 = '[2001:db8::1]:3260'
         dev_loc2 = '2001:db8::1:3260'  # udev location2
-        name1 = 'volume-00000001-1'
         name2 = 'volume-00000001-2'
-        iqn1 = 'iqn.2010-10.org.openstack:%s' % name1
         iqn2 = 'iqn.2010-10.org.openstack:%s' % name2
-        lun1 = 1
         lun2 = 2
+
         fake_multipath_dev = '/dev/mapper/fake-multipath-dev'
-        vol = {'id': 1, 'name': name1}
+        vol = {'id': 1, 'name': self._name}
         connection_properties = self.iscsi_connection_multipath(
-            vol, [location1, location2], [iqn1, iqn2], [lun1, lun2])
-        devs = ['/dev/disk/by-path/ip-%s-iscsi-%s-lun-1' % (location1, iqn1),
+            vol, [self._location, location2], [self._iqn, iqn2], [self._lun,
+                                                                  lun2])
+        devs = ['/dev/disk/by-path/ip-%s-iscsi-%s-lun-1' % (self._location,
+                                                            self._iqn),
                 '/dev/disk/by-path/ip-%s-iscsi-%s-lun-2' % (dev_loc2, iqn2)]
         mock_devices.return_value = devs
-        mock_iscsi_devices.return_value = devs
-        mock_get_iqn.return_value = [iqn1, iqn2]
+        # mock_iscsi_devices.return_value = devs
+        # mock_get_iqn.return_value = [self._iqn, iqn2]
         mock_discover_mpath_device.return_value = (
             fake_multipath_dev, test_connector.FAKE_SCSI_WWN)
-        mock_process_lun_id.return_value = [lun1, lun2]
+        mock_process_lun_id.return_value = [self._lun, lun2]
 
         result = self.connector_with_multipath.connect_volume(
             connection_properties['data'])
@@ -571,21 +589,14 @@ class ISCSIConnectorTestCase(test_connector.ConnectorTestCase):
                            'path': fake_multipath_dev, 'type': 'block',
                            'scsi_wwn': test_connector.FAKE_SCSI_WWN}
         cmd_format = 'iscsiadm -m node -T %s -p %s --%s'
-        expected_commands = [cmd_format % (iqn1, location1, 'login'),
+        expected_commands = [cmd_format % (self._iqn, self._location, 'login'),
                              cmd_format % (iqn2, location2, 'login')]
         self.assertEqual(expected_result, result)
         for command in expected_commands:
             self.assertIn(command, self.cmds)
 
-        self.cmds = []
-        self.connector_with_multipath.disconnect_volume(
-            connection_properties['data'], result)
-        expected_commands = [cmd_format % (iqn1, location1, 'logout'),
-                             cmd_format % (iqn2, location2, 'logout')]
-        for command in expected_commands:
-            self.assertIn(command, self.cmds)
-
-        mock_get_htcls.assert_called_once_with([(location1, iqn1, lun1),
+        mock_get_htcls.assert_called_once_with([(self._location, self._iqn,
+                                                 self._lun),
                                                 (location2, iqn2, lun2)])
 
     @mock.patch.object(iscsi.ISCSIConnector,
@@ -593,19 +604,14 @@ class ISCSIConnectorTestCase(test_connector.ConnectorTestCase):
     @mock.patch.object(linuxscsi.LinuxSCSI, 'get_scsi_wwn')
     @mock.patch.object(os.path, 'exists')
     @mock.patch.object(host_driver.HostDriver, 'get_all_block_devices')
-    @mock.patch.object(iscsi.ISCSIConnector, '_get_multipath_device_map',
-                       return_value={})
-    @mock.patch.object(iscsi.ISCSIConnector, '_get_iscsi_devices')
     @mock.patch.object(iscsi.ISCSIConnector, '_run_multipath')
-    @mock.patch.object(iscsi.ISCSIConnector, '_get_multipath_iqns')
     @mock.patch.object(iscsi.ISCSIConnector, '_run_iscsiadm')
     @mock.patch.object(base.BaseLinuxConnector, '_discover_mpath_device')
     @mock.patch.object(linuxscsi.LinuxSCSI, 'process_lun_id')
     def test_connect_volume_with_multiple_portals_primary_error(
             self, mock_process_lun_id, mock_discover_mpath_device,
-            mock_iscsiadm, mock_get_iqn, mock_run_multipath,
-            mock_iscsi_devices, mock_get_multipath_device_map,
-            mock_devices, mock_exists, mock_scsi_wwn, mock_get_htcls):
+            mock_iscsiadm, mock_run_multipath, mock_devices, mock_exists,
+            mock_scsi_wwn, mock_get_htcls):
         mock_scsi_wwn.return_value = test_connector.FAKE_SCSI_WWN
         location1 = '10.0.2.15:3260'
         location2 = '[2001:db8::1]:3260'
@@ -629,8 +635,6 @@ class ISCSIConnectorTestCase(test_connector.ConnectorTestCase):
 
         mock_exists.side_effect = lambda x: x != dev1
         mock_devices.return_value = [dev2]
-        mock_iscsi_devices.return_value = [dev2]
-        mock_get_iqn.return_value = [iqn2]
         mock_iscsiadm.side_effect = fake_run_iscsiadm
 
         mock_discover_mpath_device.return_value = (
@@ -654,20 +658,6 @@ class ISCSIConnectorTestCase(test_connector.ConnectorTestCase):
         mock_iscsiadm.assert_any_call(props, ('--login',),
                                       check_exit_code=[0, 255])
 
-        mock_iscsiadm.reset_mock()
-        self.connector_with_multipath.disconnect_volume(
-            connection_properties['data'], result)
-
-        props = connection_properties['data'].copy()
-        props['target_portal'] = location1
-        props['target_iqn'] = iqn1
-        mock_iscsiadm.assert_any_call(props, ('--logout',),
-                                      check_exit_code=[0, 21, 255])
-        props['target_portal'] = location2
-        props['target_iqn'] = iqn2
-        mock_iscsiadm.assert_any_call(props, ('--logout',),
-                                      check_exit_code=[0, 21, 255])
-
         lun1, lun2 = connection_properties['data']['target_luns']
         mock_get_htcls.assert_called_once_with([(location1, iqn1, lun1),
                                                (location2, iqn2, lun2)])
@@ -680,12 +670,11 @@ class ISCSIConnectorTestCase(test_connector.ConnectorTestCase):
                        '_get_target_portals_from_iscsiadm_output')
     @mock.patch.object(iscsi.ISCSIConnector, '_connect_to_iscsi_portal')
     @mock.patch.object(host_driver.HostDriver, 'get_all_block_devices')
-    @mock.patch.object(iscsi.ISCSIConnector, '_get_iscsi_devices')
     @mock.patch.object(iscsi.ISCSIConnector, '_run_multipath')
     @mock.patch.object(base.BaseLinuxConnector, '_discover_mpath_device')
     def test_connect_volume_with_multipath_connecting(
             self, mock_discover_mpath_device, mock_run_multipath,
-            mock_iscsi_devices, mock_devices,
+            mock_devices,
             mock_connect, mock_portals, mock_exists, mock_scsi_wwn,
             mock_get_htcls):
         mock_scsi_wwn.return_value = test_connector.FAKE_SCSI_WWN
@@ -702,7 +691,6 @@ class ISCSIConnectorTestCase(test_connector.ConnectorTestCase):
         devs = ['/dev/disk/by-path/ip-%s-iscsi-%s-lun-1' % (location1, iqn1),
                 '/dev/disk/by-path/ip-%s-iscsi-%s-lun-2' % (dev_loc2, iqn2)]
         mock_devices.return_value = devs
-        mock_iscsi_devices.return_value = devs
         mock_portals.return_value = ([location1, location2, location2],
                                      [iqn1, iqn1, iqn2])
         mock_discover_mpath_device.return_value = (
@@ -721,7 +709,7 @@ class ISCSIConnectorTestCase(test_connector.ConnectorTestCase):
         expected_calls = [mock.call(props1), mock.call(props2)]
         self.assertEqual(expected_result, result)
         mock_connect.assert_has_calls(expected_calls, any_order=True)
-        self.assertEqual(expected_calls, mock_connect.call_args_list)
+        self.assertEqual(len(expected_calls), mock_connect.call_count)
         lun = connection_properties['data']['target_lun']
         self.assertEqual(1, mock_get_htcls.call_count)
         # Order of elements in the list is randomized because it comes from
@@ -735,12 +723,10 @@ class ISCSIConnectorTestCase(test_connector.ConnectorTestCase):
                        '_get_target_portals_from_iscsiadm_output')
     @mock.patch.object(iscsi.ISCSIConnector, '_connect_to_iscsi_portal')
     @mock.patch.object(host_driver.HostDriver, 'get_all_block_devices')
-    @mock.patch.object(iscsi.ISCSIConnector, '_get_iscsi_devices')
     @mock.patch.object(iscsi.ISCSIConnector, '_run_multipath')
     def test_connect_volume_multipath_failed_iscsi_login(
-            self, mock_run_multipath,
-            mock_iscsi_devices, mock_devices,
-            mock_connect, mock_portals, mock_exists):
+            self, mock_run_multipath, mock_devices, mock_connect, mock_portals,
+            mock_exists):
         location1 = '10.0.2.15:3260'
         location2 = '10.0.3.15:3260'
         name1 = 'volume-00000001-1'
@@ -752,7 +738,6 @@ class ISCSIConnectorTestCase(test_connector.ConnectorTestCase):
         devs = ['/dev/disk/by-path/ip-%s-iscsi-%s-lun-1' % (location1, iqn1),
                 '/dev/disk/by-path/ip-%s-iscsi-%s-lun-2' % (location2, iqn2)]
         mock_devices.return_value = devs
-        mock_iscsi_devices.return_value = devs
         mock_portals.return_value = ([location1, location2, location2],
                                      [iqn1, iqn1, iqn2])
 
@@ -798,168 +783,119 @@ class ISCSIConnectorTestCase(test_connector.ConnectorTestCase):
         expected = (ips, iqns)
         self.assertEqual(expected, res)
 
-    @mock.patch.object(os, 'walk')
-    def test_get_iscsi_devices(self, walk_mock):
-        paths = [('ip-10.0.0.1:3260-iscsi-iqn.2013-01.ro.'
-                 'com.netapp:node.netapp02-lun-0')]
-        walk_mock.return_value = [(['.'], ['by-path'], paths)]
-        self.assertEqual(self.connector._get_iscsi_devices(), paths)
+    @mock.patch.object(iscsi.ISCSIConnector, '_disconnect_connection')
+    @mock.patch.object(iscsi.ISCSIConnector, '_get_connection_devices')
+    @mock.patch.object(linuxscsi.LinuxSCSI, 'flush_multipath_device')
+    @mock.patch.object(linuxscsi.LinuxSCSI, 'remove_connection',
+                       return_value=None)
+    def test_disconnect_volume(self, remove_mock, flush_mock, con_devs_mock,
+                               discon_mock):
+        # Return an ordered dicts instead of normal dict for discon_mock.assert
+        con_devs_mock.return_value = collections.OrderedDict((
+            (('ip1:port1', 'tgt1'), ({'sda'}, set())),
+            (('ip2:port2', 'tgt2'), ({'sdb'}, {'sdc'})),
+            (('ip3:port3', 'tgt3'), (set(), set()))))
 
-    @mock.patch.object(os, 'walk', return_value=[])
-    def test_get_iscsi_devices_with_empty_dir(self, walk_mock):
-        self.assertEqual(self.connector._get_iscsi_devices(), [])
+        with mock.patch.object(self.connector,
+                               'use_multipath') as use_mp_mock:
+            self.connector.disconnect_volume(self.CON_PROPS,
+                                             mock.sentinel.dev_info)
 
-    @mock.patch.object(os.path, 'realpath')
-    @mock.patch.object(iscsi.ISCSIConnector, '_get_iscsi_devices')
-    def test_get_multipath_iqns(self, get_iscsi_mock, realpath_mock):
-        paths = [('ip-10.0.0.1:3260-iscsi-iqn.2013-01.ro.'
-                  'com.netapp:node.netapp02-lun-0')]
-        devpath = '/dev/disk/by-path/%s' % paths[0]
-        realpath_mock.return_value = devpath
-        get_iscsi_mock.return_value = paths
-        mpath_map = {devpath: paths[0]}
-        self.assertEqual(self.connector._get_multipath_iqns([paths[0]],
-                                                            mpath_map),
-                         ['iqn.2013-01.ro.com.netapp:node.netapp02'])
+        con_devs_mock.assert_called_once_with(self.CON_PROPS)
+        remove_mock.assert_called_once_with({'sda', 'sdb'}, use_mp_mock,
+                                            False, mock.ANY)
+        discon_mock.assert_called_once_with(
+            self.CON_PROPS,
+            [('ip1:port1', 'tgt1'), ('ip3:port3', 'tgt3')],
+            False, mock.ANY)
+        flush_mock.assert_not_called()
 
-    @mock.patch.object(iscsi.ISCSIConnector, '_run_multipath')
-    def test_get_multipath_device_map(self, multipath_mock):
-        multipath_mock.return_value = [
-            "Mar 17 14:32:37 | sda: No fc_host device for 'host-1'\n"
-            "mpathb (36e00000000010001) dm-4 IET ,VIRTUAL-DISK\n"
-            "size=1.0G features='0' hwhandler='0' wp=rw\n"
-            "|-+- policy='service-time 0' prio=0 status=active\n"
-            "| `- 2:0:0:1 sda 8:0 active undef running\n"
-            "`-+- policy='service-time 0' prio=0 status=enabled\n"
-            "  `- 3:0:0:1 sdb 8:16 active undef running\n"]
-        expected = {'/dev/sda': '/dev/mapper/mpathb',
-                    '/dev/sdb': '/dev/mapper/mpathb'}
-        self.assertEqual(expected, self.connector._get_multipath_device_map())
+    @mock.patch('os_brick.exception.ExceptionChainer.__nonzero__',
+                mock.Mock(return_value=True))
+    @mock.patch('os_brick.exception.ExceptionChainer.__bool__',
+                mock.Mock(return_value=True))
+    @mock.patch.object(iscsi.ISCSIConnector, '_disconnect_connection')
+    @mock.patch.object(iscsi.ISCSIConnector, '_get_connection_devices')
+    @mock.patch.object(linuxscsi.LinuxSCSI, 'flush_multipath_device')
+    @mock.patch.object(linuxscsi.LinuxSCSI, 'remove_connection',
+                       return_value=mock.sentinel.mp_name)
+    def test_disconnect_volume_force_failure(self, remove_mock, flush_mock,
+                                             con_devs_mock, discon_mock):
+        # Return an ordered dicts instead of normal dict for discon_mock.assert
+        con_devs_mock.return_value = collections.OrderedDict((
+            (('ip1:port1', 'tgt1'), ({'sda'}, set())),
+            (('ip2:port2', 'tgt2'), ({'sdb'}, {'sdc'})),
+            (('ip3:port3', 'tgt3'), (set(), set()))))
 
-    @mock.patch.object(iscsi.ISCSIConnector, '_get_multipath_device_map')
-    @mock.patch.object(iscsi.ISCSIConnector,
-                       '_get_target_portals_from_iscsiadm_output')
-    @mock.patch.object(iscsi.ISCSIConnector, '_rescan_iscsi')
-    @mock.patch.object(iscsi.ISCSIConnector, '_get_iscsi_devices')
-    @mock.patch.object(host_driver.HostDriver, 'get_all_block_devices')
-    @mock.patch.object(iscsi.ISCSIConnector,
-                       '_disconnect_from_iscsi_portal')
-    @mock.patch.object(iscsi.ISCSIConnector, '_get_multipath_iqns')
-    @mock.patch.object(os.path, 'exists', return_value=True)
-    def test_disconnect_volume_multipath_iscsi(
-            self, exists_mock, multipath_iqn_mock, disconnect_mock,
-            get_all_devices_mock, get_iscsi_devices_mock,
-            rescan_iscsi_mock, get_portals_mock,
-            get_multipath_device_map_mock):
-        iqn1 = 'iqn.2013-01.ro.com.netapp:node.netapp01'
-        iqn2 = 'iqn.2013-01.ro.com.netapp:node.netapp02'
-        iqns = [iqn1, iqn2]
-        portal = '10.0.0.1:3260'
-        dev = ('ip-%s-iscsi-%s-lun-0' % (portal, iqn1))
+        with mock.patch.object(self.connector, 'use_multipath',
+                               wraps=True) as use_mp_mock:
+            self.assertRaises(exception.ExceptionChainer,
+                              self.connector.disconnect_volume,
+                              self.CON_PROPS, mock.sentinel.dev_info,
+                              mock.sentinel.force, ignore_errors=False)
 
-        get_portals_mock.return_value = ([portal], [iqn1])
-        multipath_iqn_mock.return_value = iqns
-        get_all_devices_mock.return_value = [dev, '/dev/mapper/md-1']
-        get_multipath_device_map_mock.return_value = {dev: '/dev/mapper/md-3'}
-        get_iscsi_devices_mock.return_value = []
-        fake_property = {'target_portal': portal,
-                         'target_iqn': iqn1}
-        self.connector._disconnect_volume_multipath_iscsi(fake_property,
-                                                          'fake/multipath')
-        # Target in use by other mp devices, don't disconnect
-        self.assertFalse(disconnect_mock.called)
+        con_devs_mock.assert_called_once_with(self.CON_PROPS)
+        remove_mock.assert_called_once_with({'sda', 'sdb'}, use_mp_mock,
+                                            mock.sentinel.force, mock.ANY)
+        discon_mock.assert_called_once_with(
+            self.CON_PROPS,
+            [('ip1:port1', 'tgt1'), ('ip3:port3', 'tgt3')],
+            mock.sentinel.force, mock.ANY)
+        flush_mock.assert_called_once_with(mock.sentinel.mp_name)
 
-    @mock.patch.object(iscsi.ISCSIConnector,
-                       '_get_target_portals_from_iscsiadm_output')
-    @mock.patch.object(iscsi.ISCSIConnector, '_rescan_iscsi')
-    @mock.patch.object(iscsi.ISCSIConnector, '_get_iscsi_devices')
-    @mock.patch.object(host_driver.HostDriver, 'get_all_block_devices')
-    @mock.patch.object(iscsi.ISCSIConnector,
-                       '_disconnect_from_iscsi_portal')
-    @mock.patch.object(iscsi.ISCSIConnector, '_get_multipath_device_map')
-    @mock.patch.object(iscsi.ISCSIConnector, '_get_multipath_iqns')
-    @mock.patch.object(os.path, 'exists', return_value=True)
-    def test_disconnect_volume_multipath_iscsi_other_targets(
-            self, exists_mock, multipath_iqn_mock, get_multipath_map_mock,
-            disconnect_mock, get_all_devices_mock, get_iscsi_devices_mock,
-            rescan_iscsi_mock, get_portals_mock):
-        iqn1 = 'iqn.2010-10.org.openstack:target-1'
-        iqn2 = 'iqn.2010-10.org.openstack:target-2'
-        portal = '10.0.0.1:3260'
-        dev2 = ('ip-%s-iscsi-%s-lun-0' % (portal, iqn2))
+    @ddt.data({'do_raise': False, 'force': False},
+              {'do_raise': True, 'force': True},
+              {'do_raise': True, 'force': False})
+    @ddt.unpack
+    @mock.patch.object(iscsi.ISCSIConnector, '_disconnect_from_iscsi_portal')
+    def test_disconnect_connection(self, disconnect_mock, do_raise, force):
+        will_raise = do_raise and not force
+        actual_call_args = []
 
-        # Multiple targets are discovered, but only block devices for target-1
-        # is deleted and target-2 is in use.
-        get_portals_mock.return_value = ([portal, portal], [iqn1, iqn2])
-        multipath_iqn_mock.return_value = [iqn2, iqn2]
-        get_all_devices_mock.return_value = [dev2, '/dev/mapper/md-1']
-        get_multipath_map_mock.return_value = {dev2: '/dev/mapper/md-3'}
-        get_iscsi_devices_mock.return_value = [dev2]
-        fake_property = {'target_portal': portal,
-                         'target_iqn': iqn1}
-        self.connector._disconnect_volume_multipath_iscsi(fake_property,
-                                                          'fake/multipath')
-        # Only target-1 should be disconneced.
-        disconnect_mock.assert_called_once_with(fake_property)
+        # Since we reuse the copied dictionary on _disconnect_connection
+        # changing its values we cannot use mock's assert_has_calls
+        def my_disconnect(con_props):
+            actual_call_args.append(con_props.copy())
+            if do_raise:
+                raise exception.ExceptionChainer()
 
-    @mock.patch.object(iscsi.ISCSIConnector, '_get_multipath_device_map',
-                       return_value={})
-    @mock.patch.object(iscsi.ISCSIConnector,
-                       '_get_target_portals_from_iscsiadm_output')
-    @mock.patch.object(iscsi.ISCSIConnector, '_rescan_iscsi')
-    @mock.patch.object(iscsi.ISCSIConnector, '_get_iscsi_devices',
-                       return_value=[])
-    @mock.patch.object(host_driver.HostDriver, 'get_all_block_devices',
-                       return_value=[])
-    @mock.patch.object(iscsi.ISCSIConnector,
-                       '_disconnect_from_iscsi_portal')
-    @mock.patch.object(os.path, 'exists', return_value=True)
-    def test_disconnect_volume_multipath_iscsi_without_other_mp_devices(
-            self, exists_mock, disconnect_mock, get_all_devices_mock,
-            get_iscsi_devices_mock, rescan_iscsi_mock,
-            get_portals_mock, get_multipath_device_map_mock):
-        portal = '10.0.2.15:3260'
-        name = 'volume-00000001'
-        iqn = 'iqn.2010-10.org.openstack:%s' % name
+        disconnect_mock.side_effect = my_disconnect
 
-        get_portals_mock.return_value = ([portal], [iqn])
-        fake_property = {'target_portal': portal,
-                         'target_iqn': iqn}
-        self.connector._disconnect_volume_multipath_iscsi(fake_property,
-                                                          'fake/multipath')
-        # Target not in use by other mp devices, disconnect
-        disconnect_mock.assert_called_once_with(fake_property)
+        connections = (('ip1:port1', 'tgt1'), ('ip2:port2', 'tgt2'))
+        original_props = self.CON_PROPS.copy()
+        exc = exception.ExceptionChainer()
+        if will_raise:
+            self.assertRaises(exception.ExceptionChainer,
+                              self.connector._disconnect_connection,
+                              self.CON_PROPS, connections,
+                              force=force, exc=exc)
+        else:
+            self.connector._disconnect_connection(self.CON_PROPS, connections,
+                                                  force=force, exc=exc)
 
-    @mock.patch.object(iscsi.ISCSIConnector, '_get_multipath_device_map',
-                       return_value={})
-    @mock.patch.object(iscsi.ISCSIConnector,
-                       '_get_target_portals_from_iscsiadm_output')
-    @mock.patch.object(iscsi.ISCSIConnector, '_rescan_iscsi')
-    @mock.patch.object(iscsi.ISCSIConnector, '_get_iscsi_devices')
-    @mock.patch.object(host_driver.HostDriver, 'get_all_block_devices')
-    @mock.patch.object(iscsi.ISCSIConnector,
-                       '_disconnect_from_iscsi_portal')
-    @mock.patch.object(os.path, 'exists', return_value=False)
-    def test_disconnect_volume_multipath_iscsi_with_invalid_symlink(
-            self, exists_mock, disconnect_mock, get_all_devices_mock,
-            get_iscsi_devices_mock, rescan_iscsi_mock,
-            get_portals_mock, get_multipath_device_map_mock):
-        # Simulate a broken symlink by returning False for os.path.exists(dev)
-        portal = '10.0.0.1:3260'
-        name = 'volume-00000001'
-        iqn = 'iqn.2010-10.org.openstack:%s' % name
-        dev = ('ip-%s-iscsi-%s-lun-0' % (portal, iqn))
+        # Passed properties should not be altered by the method call
+        self.assertDictEqual(original_props, self.CON_PROPS)
+        expected = [original_props.copy(), original_props.copy()]
+        for i, (ip, iqn) in enumerate(connections):
+            expected[i].update(target_portal=ip, target_iqn=iqn)
+        # If we are failing and not forcing we won't make all the alls
+        if will_raise:
+            expected = expected[:1]
+        self.assertListEqual(expected, actual_call_args)
+        # No exceptions have been caught by ExceptionChainer context manager
+        self.assertEqual(do_raise, bool(exc))
 
-        get_portals_mock.return_value = ([portal], [iqn])
-        get_all_devices_mock.return_value = [dev, '/dev/mapper/md-1']
-        get_iscsi_devices_mock.return_value = []
-
-        fake_property = {'target_portal': portal,
-                         'target_iqn': iqn}
-        self.connector._disconnect_volume_multipath_iscsi(fake_property,
-                                                          'fake/multipath')
-        # Target not in use by other mp devices, disconnect
-        disconnect_mock.assert_called_once_with(fake_property)
+    def test_disconnect_from_iscsi_portal(self):
+        self.connector._disconnect_from_iscsi_portal(self.CON_PROPS)
+        expected_prefix = ('iscsiadm -m node -T %s -p %s ' %
+                           (self.CON_PROPS['target_iqn'],
+                            self.CON_PROPS['target_portal']))
+        expected = [
+            expected_prefix + '--op update -n node.startup -v manual',
+            expected_prefix + '--logout',
+            expected_prefix + '--op delete',
+        ]
+        self.assertListEqual(expected, self.cmds)
 
     def test_iscsiadm_discover_parsing(self):
         # Ensure that parsing iscsiadm discover ignores cruft.

os_brick/tests/initiator/test_linuxscsi.py

@@ -17,8 +17,8 @@ import os.path
 import textwrap
 import time
 
+import ddt
 import mock
-from oslo_concurrency import processutils as putils
 from oslo_log import log as logging
 
 from os_brick import exception
@@ -28,6 +28,7 @@ from os_brick.tests import base
 LOG = logging.getLogger(__name__)
 
 
+@ddt.ddt
 class LinuxSCSITestCase(base.TestCase):
     def setUp(self):
         super(LinuxSCSITestCase, self).setUp()
@@ -73,51 +74,51 @@ class LinuxSCSITestCase(base.TestCase):
             ('tee -a /sys/block/sdc/device/delete')]
         self.assertEqual(expected_commands, self.cmds)
 
-    @mock.patch('time.sleep')
-    def test_wait_for_volume_removal(self, sleep_mock):
-        fake_path = '/dev/disk/by-path/fake-iscsi-iqn-lun-0'
-        exists_mock = mock.Mock()
-        exists_mock.return_value = True
-        os.path.exists = exists_mock
-        self.assertRaises(exception.VolumePathNotRemoved,
-                          self.linuxscsi.wait_for_volume_removal,
-                          fake_path)
+    @mock.patch.object(linuxscsi.LinuxSCSI, 'echo_scsi_command')
+    @mock.patch.object(linuxscsi.LinuxSCSI, 'flush_device_io')
+    @mock.patch.object(os.path, 'exists', return_value=True)
+    def test_remove_scsi_device_force(self, exists_mock, flush_mock,
+                                      echo_mock):
+        """With force we'll always call delete even if flush fails."""
+        exc = exception.ExceptionChainer()
+        flush_mock.side_effect = Exception()
+        echo_mock.side_effect = Exception()
+        device = '/dev/sdc'
 
-        exists_mock = mock.Mock()
-        exists_mock.return_value = False
-        os.path.exists = exists_mock
-        self.linuxscsi.wait_for_volume_removal(fake_path)
-        expected_commands = []
-        self.assertEqual(expected_commands, self.cmds)
-        self.assertTrue(sleep_mock.called)
+        self.linuxscsi.remove_scsi_device(device, force=True, exc=exc)
+        # The context manager has caught the exceptions
+        self.assertTrue(exc)
+        flush_mock.assert_called_once_with(device)
+        echo_mock.assert_called_once_with('/sys/block/sdc/device/delete', '1')
+
+    @mock.patch('time.sleep')
+    @mock.patch('os.path.exists', return_value=True)
+    def test_wait_for_volumes_removal_failure(self, exists_mock, sleep_mock):
+        retries = 3
+        names = ('sda', 'sdb')
+        self.assertRaises(exception.VolumePathNotRemoved,
+                          self.linuxscsi.wait_for_volumes_removal, names)
+        exists_mock.assert_has_calls([mock.call('/dev/' + name)
+                                      for name in names] * retries)
+        self.assertEqual(retries - 1, sleep_mock.call_count)
+
+    @mock.patch('time.sleep')
+    @mock.patch('os.path.exists', side_effect=(True, True, False, False))
+    def test_wait_for_volumes_removal_retry(self, exists_mock, sleep_mock):
+        names = ('sda', 'sdb')
+        self.linuxscsi.wait_for_volumes_removal(names)
+        exists_mock.assert_has_calls([mock.call('/dev/' + name)
+                                      for name in names] * 2)
+        self.assertEqual(1, sleep_mock.call_count)
 
     def test_flush_multipath_device(self):
-        self.linuxscsi.flush_multipath_device('/dev/dm-9')
-        expected_commands = [('multipath -f /dev/dm-9')]
-        self.assertEqual(expected_commands, self.cmds)
+        dm_map_name = '3600d0230000000000e13955cc3757800'
+        with mock.patch.object(self.linuxscsi, '_execute') as exec_mock:
+            self.linuxscsi.flush_multipath_device(dm_map_name)
 
-    @mock.patch('retrying.time.sleep', mock.Mock())
-    def test_flush_multipath_device_in_use(self):
-        side_effect = (
-            putils.ProcessExecutionError(
-                stdout='Feb 09 14:38:02 | mpatha: map in use\n'
-                       'Feb 09 14:38:02 | failed to remove multipath map '
-                       'mpatha\n',
-                exit_code=1),
-            ('', '')
-        )
-
-        with mock.patch.object(self.linuxscsi, '_execute') as execute_mock:
-            execute_mock.side_effect = side_effect
-            self.linuxscsi.flush_multipath_device('mpatha')
-            execute_mock.assert_has_calls(
-                [mock.call('multipath', '-f', 'mpatha', run_as_root=True,
-                           root_helper=mock.ANY)] * 2)
-
-    def test_flush_multipath_devices(self):
-        self.linuxscsi.flush_multipath_devices()
-        expected_commands = [('multipath -F')]
-        self.assertEqual(expected_commands, self.cmds)
+        exec_mock.assert_called_once_with(
+            'multipath', '-f', dm_map_name, run_as_root=True, attempts=3,
+            timeout=300, interval=10, root_helper=self.linuxscsi._root_helper)
 
     def test_get_scsi_wwn(self):
         fake_path = '/dev/disk/by-id/somepath'
@@ -130,6 +131,51 @@ class LinuxSCSITestCase(base.TestCase):
         wwn = self.linuxscsi.get_scsi_wwn(fake_path)
         self.assertEqual(fake_wwn, wwn)
 
+    @mock.patch('six.moves.builtins.open')
+    def test_get_dm_name(self, open_mock):
+        dm_map_name = '3600d0230000000000e13955cc3757800'
+        cm_open = open_mock.return_value.__enter__.return_value
+        cm_open.read.return_value = dm_map_name
+        res = self.linuxscsi.get_dm_name('dm-0')
+        self.assertEqual(dm_map_name, res)
+        open_mock.assert_called_once_with('/sys/block/dm-0/dm/name')
+
+    @mock.patch('six.moves.builtins.open', side_effect=IOError)
+    def test_get_dm_name_failure(self, open_mock):
+        self.assertEqual('', self.linuxscsi.get_dm_name('dm-0'))
+
+    @mock.patch('glob.glob', side_effect=[[], ['/sys/block/sda/holders/dm-9']])
+    def test_find_sysfs_multipath_dm(self, glob_mock):
+        device_names = ('sda', 'sdb')
+        res = self.linuxscsi.find_sysfs_multipath_dm(device_names)
+        self.assertEqual('dm-9', res)
+        glob_mock.assert_has_calls([mock.call('/sys/block/sda/holders/dm-*'),
+                                    mock.call('/sys/block/sdb/holders/dm-*')])
+
+    @mock.patch('glob.glob', return_value=[])
+    def test_find_sysfs_multipath_dm_not_found(self, glob_mock):
+        device_names = ('sda', 'sdb')
+        res = self.linuxscsi.find_sysfs_multipath_dm(device_names)
+        self.assertIsNone(res)
+        glob_mock.assert_has_calls([mock.call('/sys/block/sda/holders/dm-*'),
+                                    mock.call('/sys/block/sdb/holders/dm-*')])
+
+    @mock.patch.object(linuxscsi.LinuxSCSI, '_execute')
+    @mock.patch('os.path.exists', return_value=True)
+    def test_flush_device_io(self, exists_mock, exec_mock):
+        device = '/dev/sda'
+        self.linuxscsi.flush_device_io(device)
+        exists_mock.assert_called_once_with(device)
+        exec_mock.assert_called_once_with(
+            'blockdev', '--flushbufs', device, run_as_root=True, attempts=3,
+            timeout=300, interval=10, root_helper=self.linuxscsi._root_helper)
+
+    @mock.patch('os.path.exists', return_value=False)
+    def test_flush_device_io_non_existent(self, exists_mock):
+        device = '/dev/sda'
+        self.linuxscsi.flush_device_io(device)
+        exists_mock.assert_called_once_with(device)
+
     @mock.patch.object(os.path, 'exists', return_value=True)
     def test_find_multipath_device_path(self, exists_mock):
         fake_wwn = '1234567890'
@@ -168,31 +214,73 @@ class LinuxSCSITestCase(base.TestCase):
                                 self.linuxscsi.wait_for_path,
                                 path)
 
-    @mock.patch.object(linuxscsi.LinuxSCSI, 'find_multipath_device')
-    @mock.patch.object(os.path, 'exists', return_value=True)
-    def test_remove_multipath_device(self, exists_mock, mock_multipath):
-        def fake_find_multipath_device(device):
-            devices = [{'device': '/dev/sde', 'host': 0,
-                        'channel': 0, 'id': 0, 'lun': 1},
-                       {'device': '/dev/sdf', 'host': 2,
-                        'channel': 0, 'id': 0, 'lun': 1}, ]
+    @ddt.data({'do_raise': False, 'force': False},
+              {'do_raise': True, 'force': True})
+    @ddt.unpack
+    @mock.patch.object(linuxscsi.LinuxSCSI, 'flush_multipath_device')
+    @mock.patch.object(linuxscsi.LinuxSCSI, 'get_dm_name')
+    @mock.patch.object(linuxscsi.LinuxSCSI, 'find_sysfs_multipath_dm')
+    @mock.patch.object(linuxscsi.LinuxSCSI, 'wait_for_volumes_removal')
+    @mock.patch('os_brick.initiator.linuxscsi.LinuxSCSI.remove_scsi_device')
+    def test_remove_connection_multipath_complete(self, remove_mock, wait_mock,
+                                                  find_dm_mock,
+                                                  get_dm_name_mock,
+                                                  flush_mp_mock,
+                                                  do_raise, force):
+        if do_raise:
+            flush_mp_mock.side_effect = Exception
+        devices_names = ('sda', 'sdb')
+        exc = exception.ExceptionChainer()
+        mp_name = self.linuxscsi.remove_connection(devices_names,
+                                                   is_multipath=True,
+                                                   force=mock.sentinel.Force,
+                                                   exc=exc)
+        find_dm_mock.assert_called_once_with(devices_names)
+        get_dm_name_mock.assert_called_once_with(find_dm_mock.return_value)
+        flush_mp_mock.assert_called_once_with(get_dm_name_mock.return_value)
+        self.assertEqual(get_dm_name_mock.return_value if do_raise else None,
+                         mp_name)
+        remove_mock.assert_has_calls([
+            mock.call('/dev/sda', mock.sentinel.Force, exc),
+            mock.call('/dev/sdb', mock.sentinel.Force, exc)])
+        wait_mock.assert_called_once_with(devices_names)
+        self.assertEqual(do_raise, bool(exc))
 
-            info = {"device": "dm-3",
-                    "id": "350002ac20398383d",
-                    "name": "mpatha",
-                    "devices": devices}
-            return info
+    @mock.patch.object(linuxscsi.LinuxSCSI, 'flush_multipath_device',
+                       side_effect=Exception)
+    @mock.patch.object(linuxscsi.LinuxSCSI, 'get_dm_name')
+    @mock.patch.object(linuxscsi.LinuxSCSI, 'find_sysfs_multipath_dm')
+    @mock.patch.object(linuxscsi.LinuxSCSI, 'wait_for_volumes_removal')
+    @mock.patch.object(linuxscsi.LinuxSCSI, 'remove_scsi_device')
+    def test_remove_connection_multipath_fail(self, remove_mock, wait_mock,
+                                              find_dm_mock, get_dm_name_mock,
+                                              flush_mp_mock):
+        flush_mp_mock.side_effect = exception.ExceptionChainer
+        devices_names = ('sda', 'sdb')
+        exc = exception.ExceptionChainer()
+        self.assertRaises(exception.ExceptionChainer,
+                          self.linuxscsi.remove_connection,
+                          devices_names, is_multipath=True,
+                          force=False, exc=exc)
+        find_dm_mock.assert_called_once_with(devices_names)
+        get_dm_name_mock.assert_called_once_with(find_dm_mock.return_value)
+        flush_mp_mock.assert_called_once_with(get_dm_name_mock.return_value)
+        remove_mock.assert_not_called()
+        wait_mock.assert_not_called()
+        self.assertTrue(bool(exc))
 
-        mock_multipath.side_effect = fake_find_multipath_device
-
-        self.linuxscsi.remove_multipath_device('/dev/dm-3')
-        expected_commands = [
-            ('multipath -f mpatha'),
-            ('blockdev --flushbufs /dev/sde'),
-            ('tee -a /sys/block/sde/device/delete'),
-            ('blockdev --flushbufs /dev/sdf'),
-            ('tee -a /sys/block/sdf/device/delete'), ]
-        self.assertEqual(expected_commands, self.cmds)
+    @mock.patch.object(linuxscsi.LinuxSCSI, 'wait_for_volumes_removal')
+    @mock.patch.object(linuxscsi.LinuxSCSI, 'remove_scsi_device')
+    def test_remove_connection_singlepath(self, remove_mock, wait_mock):
+        devices_names = ('sda', 'sdb')
+        exc = exception.ExceptionChainer()
+        self.linuxscsi.remove_connection(devices_names, is_multipath=False,
+                                         force=mock.sentinel.Force,
+                                         exc=exc)
+        remove_mock.assert_has_calls(
+            [mock.call('/dev/sda', mock.sentinel.Force, exc),
+             mock.call('/dev/sdb', mock.sentinel.Force, exc)])
+        wait_mock.assert_called_once_with(devices_names)
 
     def test_find_multipath_device_3par_ufn(self):
         def fake_execute(*cmd, **kwargs):

os_brick/tests/privileged/test_rootwrap.py

@@ -10,10 +10,13 @@
 #    License for the specific language governing permissions and limitations
 #    under the License.
 
+import time
+
 import mock
-
 from oslo_concurrency import processutils as putils
+import six
 
+from os_brick import exception
 from os_brick import privileged
 from os_brick.privileged import rootwrap as priv_rootwrap
 from os_brick.tests import base
@@ -43,7 +46,8 @@ class PrivRootwrapTestCase(base.TestCase):
     def test_execute_root(self, mock_putils_exec):
         priv_rootwrap.execute_root('echo', 'foo', check_exit_code=0)
         mock_putils_exec.assert_called_once_with(
-            'echo', 'foo', check_exit_code=0, shell=False, run_as_root=False)
+            'echo', 'foo', check_exit_code=0, shell=False, run_as_root=False,
+            delay_on_retry=False, on_completion=mock.ANY, on_execute=mock.ANY)
 
         # Exact exception isn't particularly important, but these
         # should be errors:
@@ -57,3 +61,67 @@ class PrivRootwrapTestCase(base.TestCase):
     def test_oserror_raise(self, mock_putils_exec):
         self.assertRaises(putils.ProcessExecutionError,
                           priv_rootwrap.execute, 'foo')
+
+    @mock.patch.object(priv_rootwrap.execute_root.privsep_entrypoint,
+                       'client_mode', False)
+    @mock.patch.object(priv_rootwrap, 'custom_execute')
+    def test_execute_as_root(self, exec_mock):
+        res = priv_rootwrap.execute(mock.sentinel.cmds, run_as_root=True,
+                                    root_helper=mock.sentinel.root_helper,
+                                    keyword_arg=mock.sentinel.kwarg)
+        self.assertEqual(exec_mock.return_value, res)
+        exec_mock.assert_called_once_with(mock.sentinel.cmds, shell=False,
+                                          run_as_root=False,
+                                          keyword_arg=mock.sentinel.kwarg)
+
+    def test_custom_execute(self):
+        on_execute = mock.Mock()
+        on_completion = mock.Mock()
+        msg = 'hola'
+        out, err = priv_rootwrap.custom_execute('echo', msg,
+                                                on_execute=on_execute,
+                                                on_completion=on_completion)
+        self.assertEqual(msg + '\n', out)
+        self.assertEqual('', err)
+        on_execute.assert_called_once_with(mock.ANY)
+        proc = on_execute.call_args[0][0]
+        on_completion.assert_called_once_with(proc)
+
+    @mock.patch('time.sleep')
+    def test_custom_execute_timeout_raises_with_retries(self, sleep_mock):
+        on_execute = mock.Mock()
+        on_completion = mock.Mock()
+        t0 = time.time()
+        self.assertRaises(exception.ExecutionTimeout,
+                          priv_rootwrap.custom_execute,
+                          'sleep', '2', timeout=0.05, raise_timeout=True,
+                          interval=2, backoff_rate=3, attempts=3,
+                          on_execute=on_execute, on_completion=on_completion)
+        t1 = time.time()
+        self.assertLess(t1 - t0, 0.3)
+        sleep_mock.assert_has_calls([mock.call(0), mock.call(6), mock.call(0),
+                                     mock.call(18), mock.call(0)])
+        expected_calls = [mock.call(args[0][0])
+                          for args in on_execute.call_args_list]
+        on_execute.assert_has_calls(expected_calls)
+        on_completion.assert_has_calls(expected_calls)
+
+    def test_custom_execute_timeout_no_raise(self):
+        t0 = time.time()
+        out, err = priv_rootwrap.custom_execute('sleep', '2', timeout=0.05,
+                                                raise_timeout=False)
+        t1 = time.time()
+        self.assertEqual('', out)
+        self.assertIsInstance(err, six.string_types)
+        self.assertLess(t1 - t0, 0.3)
+
+    def test_custom_execute_check_exit_code(self):
+        self.assertRaises(putils.ProcessExecutionError,
+                          priv_rootwrap.custom_execute,
+                          'ls', '-y', check_exit_code=True)
+
+    def test_custom_execute_no_check_exit_code(self):
+        out, err = priv_rootwrap.custom_execute('ls', '-y',
+                                                check_exit_code=False)
+        self.assertEqual('', out)
+        self.assertIsInstance(err, six.string_types)

os_brick/tests/windows/fake_win_conn.py

@@ -20,7 +20,8 @@ class FakeWindowsConnector(win_conn_base.BaseWindowsConnector):
     def connect_volume(self, connection_properties):
         return {}
 
-    def disconnect_volume(self, connection_properties, device_info):
+    def disconnect_volume(self, connection_properties, device_info,
+                          force=False, ignore_errors=False):
         pass
 
     def get_volume_paths(self, connection_properties):

releasenotes/notes/refactor_iscsi_disconnect-557f4173bc1ae4ed.yaml

@@ -0,0 +1,12 @@
+---
+features:
+  - |
+    New parameters on `disconnect_volume` named `force` and `ignore_errors` can
+    be used to let OS-Brick know that data loss is secondary to leaving a clean
+    system with no leftover devices.  If `force` is not set, or set to False,
+    preventing data loss will take priority.  Currently only iSCSI implements
+    these new parameters.
+fixes:
+  - |
+    iSCSI disconnect refactoring improves reliability, speed, and thoroughness
+    on leaving a cleaner system after disconnection.