Refactor iSCSI disconnect

This patch refactors iSCSI disconnect code changing the approach to one
that just uses `iscsiadm -m session` and sysfs to get all the required
information: devices from the connection, multipath system device name,
multipath name, the WWN for the block devices...

By doing so, not only do we fix a good number of bugs, but we also
improve the reliability and speed of the mechanism.

A good example of improvements and benefits achieved by this patch are:

- Common code for multipath and single path disconnects.

- No more querying iSCSI devices for their WWN (page 0x83) removing
  delays and issue on flaky connections.

- All devices are properly cleaned even if they are not part of the
  multipath.

- We wait for device removal and do it in parallel if there are
  multiple.

- Removed usage of `multipath -l` to find devices which is really slow
  with flaky connections and didn't work when called with a device from
  a path that is down.

- Prevent losing data when detaching, currently if the multipath flush
  fails for any other reason than "in use" we silently continue with the
  removal.  That is the case when all paths are momentarily down.

- Adds a new mechanism for the caller of the disconnect to specify that
  it's acceptable to lose data and that it's more important to leave a
  clean system.  That is the case if we are creating a volume from an
  image, since the volume will just be set to error, but we don't want
  leftovers.  Optionally we can tell os-brick to ignore errors and don't
  raise an exception if the flush fails.

- Add a warning when we could be leaving leftovers behind due to
  disconnect issues.

- Action retries (like multipath flush) will now only log the final
  exception instead of logging all the exceptions.

- Flushes of individual paths now use exponential backoff retries
  instead of random retries between 0.2 and 2 seconds (from oslo
  library).

- We no longer use symlinks from `/dev/disk/by-path`, `/dev/disk/by-id`,
  or `/dev/mapper` to find devices or multipaths, as they could be
  leftovers from previous runs.

- With high failure rates (above 30%) some CLI calls will enter into a
  weird state where they wait forever, so we add a timeout mechanism in
  our `execute` method and add it to those specific calls.

Closes-Bug: #1502534
Change-Id: I058ff0a0e5ad517507dc3cda39087c913558561d

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",
+                        "iscsiadm err: %s", err)
-                        err)
+            return []
-        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])
 
-        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
-
+        :param force: Whether to forcefully disconnect even if flush fails.
-        connection_properties for iSCSI must include:
+        :type force: bool
-        target_portal(s) - IP and optional port
+        :param ignore_errors: When force is True, this will decide whether to
-        target_iqn(s) - iSCSI Qualified Name
+                              ignore errors or raise an exception once finished
-        target_lun(s) - LUN id of the volume
+                              the operation.  Default is False.
+        :type ignore_errors: bool
         """
-        if self.use_multipath:
+        exc = exception.ExceptionChainer()
-            host_device = multipath_device = None
+        devices_map = self._get_connection_devices(connection_properties)
-            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)
 
-            if multipath_device:
+        # Remove devices and multipath from this connection
-                device_realpath = os.path.realpath(host_device)
+        remove_devices = set()
-                self._linuxscsi.remove_multipath_device(device_realpath)
+        for remove, __ in devices_map.values():
-                return self._disconnect_volume_multipath_iscsi(
+            remove_devices.update(remove)
-                    connection_properties, multipath_device)
+        multipath_name = self._linuxscsi.remove_connection(remove_devices,
+                                                           self.use_multipath,
+                                                           force, exc)
 
-        # When multiple portals/iqns/luns are specified, we need to remove
+        # Disconnect sessions and remove nodes that are left without devices
-        # unused devices created by logging into other LUNs' session.
+        disconnect = [conn for conn, (__, keep) in devices_map.items()
-        for props in self._iterate_all_targets(connection_properties):
+                      if not keep]
-            self._disconnect_volume_iscsi(props)
+        self._disconnect_connection(connection_properties, disconnect, force,
+                                    exc)
 
-    def _disconnect_volume_iscsi(self, connection_properties):
+        # If flushing the multipath failed before, try now after we have
-        # remove the device from the scsi subsystem
+        # removed the devices and we may have even logged off (only reaches
-        # this eliminates any stale entries until logout
+        # here with multipath_name if force=True).
-        host_devices = self._get_device_path(connection_properties)
+        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:
+        if exc:
-            host_device = host_devices[0]
+            LOG.warning('There were errors removing %s, leftovers may remain '
-        else:
+                        'in the system', remove_devices)
-            return
+            if not ignore_errors:
-
+                raise exc
-        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)
 
     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):
+    def _disconnect_connection(self, connection_properties, connections, force,
-        try:
+                               exc):
-            devices = list(os.walk('/dev/disk/by-path'))[0][-1]
+        LOG.debug('Disconnecting from: %s', connections)
-        except IndexError:
+        props = connection_properties.copy()
-            return []
+        for ip, iqn in connections:
-        # 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)
             props['target_portal'] = ip
             props['target_iqn'] = iqn
-            self._disconnect_from_iscsi_portal(props)
+            with exc.context(force, 'Disconnect from %s %s failed', ip, 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
 
     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,
+    @utils.retry(exceptions=exception.VolumePathNotRemoved)
-                 backoff_rate=2)
+    def wait_for_volumes_removal(self, volumes_names):
-    def wait_for_volume_removal(self, volume_path):
+        """Wait for device paths to be removed from the system."""
-        """This is used to ensure that volumes are gone."""
+        str_names = ', '.join(volumes_names)
-        LOG.debug("Checking to see if SCSI volume %s has been removed.",
+        LOG.debug('Checking to see if SCSI volumes %s have been removed.',
-                  volume_path)
+                  str_names)
-        if os.path.exists(volume_path):
+        exist = [volume_name for volume_name in volumes_names
-            LOG.debug("%(path)s still exists.", {'path': volume_path})
+                 if os.path.exists('/dev/' + volume_name)]
-            raise exception.VolumePathNotRemoved(
+        if exist:
-                volume_path=volume_path)
+            LOG.debug('%s still exist.', ', '.join(exist))
-        else:
+            raise exception.VolumePathNotRemoved(volume_path=exist)
-            LOG.debug("SCSI volume %s has been removed.", volume_path)
+        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):
+    def get_dm_name(self, dm):
-        """Removes related LUNs and multipath device
+        """Get the Device map name given the device name of the dm on sysfs.
 
-        This removes LUNs associated with a multipath device
+        :param dm: Device map name as seen in sysfs. ie: 'dm-0'
-        and the multipath device itself.
+        :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)
+    def find_sysfs_multipath_dm(self, device_names):
-        mpath_dev = self.find_multipath_device(device)
+        """Find the dm device name given a list of device names
-        if mpath_dev:
+
-            self.flush_multipath_device(mpath_dev['name'])
+        :param device_names: Iterable with device names, not paths. ie: ['sda']
-            devices = mpath_dev['devices']
+        :returns: String with the dm name or None if not found. ie: 'dm-0'
-            LOG.debug("multipath LUNs to remove %s", devices)
+        """
-            for device in devices:
+        glob_str = '/sys/block/%s/holders/dm-*'
-                self.remove_scsi_device(device['device'])
+        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:
+        if os.path.exists(device):
-            LOG.debug("Flushing IO for device %s", device)
+            try:
-            self._execute('blockdev', '--flushbufs', device, run_as_root=True,
+                # NOTE(geguileo): With 30% connection error rates flush can get
-                          root_helper=self._root_helper)
+                # stuck, set timeout to prevent it from hanging here forever.
-        except putils.ProcessExecutionError as exc:
+                # Retry twice after 20 and 40 seconds.
-            LOG.warning("Failed to flush IO buffers prior to removing "
+                LOG.debug("Flushing IO for device %s", device)
-                        "device: %(code)s", {'code': exc.exit_code})
+                self._execute('blockdev', '--flushbufs', device,
-
+                              run_as_root=True, attempts=3, timeout=300,
-    @utils.retry(exceptions=putils.ProcessExecutionError)
+                              interval=10, root_helper=self._root_helper)
-    def flush_multipath_device(self, device):
+            except putils.ProcessExecutionError as exc:
-        try:
+                LOG.warning("Failed to flush IO buffers prior to removing "
-            LOG.debug("Flush multipath device %s", device)
+                            "device: %(code)s", {'code': exc.exit_code})
-            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.')
                 raise
-            LOG.warning("multipath call failed exit %(code)s",
-                        {'code': exc.exit_code})
 
-    def flush_multipath_devices(self):
+    def flush_multipath_device(self, device_map_name):
-        try:
+        LOG.debug("Flush multipath device %s", device_map_name)
-            self._execute('multipath', '-F', run_as_root=True,
+        # NOTE(geguileo): With 30% connection error rates flush can get stuck,
-                          root_helper=self._root_helper)
+        # set timeout to prevent it from hanging here forever.  Retry twice
-        except putils.ProcessExecutionError as exc:
+        # after 20 and 40 seconds.
-            LOG.warning("multipath call failed exit %(code)s",
+        self._execute('multipath', '-f', device_map_name, run_as_root=True,
-                        {'code': exc.exit_code})
+                      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'
+        vol = {'id': 1, 'name': self._name}
-        name = 'volume-00000001'
+        connection_info = self.iscsi_connection(vol, self._location, self._iqn)
-        iqn = 'iqn.2010-10.org.openstack:%s' % name
-        vol = {'id': 1, 'name': name}
-        connection_info = self.iscsi_connection(vol, location, 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):
+        # Disconnect has its own tests, should not be tested here
-            self.count = self.count + 1
+        expected_commands = [
-            if self.count == 4:
+            ('iscsiadm -m node -T %s -p %s' % (self._iqn, self._location)),
-                return False
+            ('iscsiadm -m session'),
-            else:
+            ('iscsiadm -m node -T %s -p %s --login' % (self._iqn,
-                return True
+                                                       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:
+        self.assertEqual(expected_commands, self.cmds)
-            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)
 
     @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):
+    @mock.patch('os.path.exists', side_effect=(True,) * 4 + (False, False))
-        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.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'
+        iqn2 = 'iqn.2010-10.org.openstack:%s-2' % self._name
-        iqn = 'iqn.2010-10.org.openstack:%s' % name
+        vol = {'id': 1, 'name': self._name}
-        iqn2 = 'iqn.2010-10.org.openstack:%s-2' % name
+        connection_info = self.iscsi_connection(vol, self._location, self._iqn)
-        vol = {'id': 1, 'name': name}
+        connection_info['data']['target_portals'] = [self._location, location2]
-        connection_info = self.iscsi_connection(vol, location, iqn)
+        connection_info['data']['target_iqns'] = [self._iqn, iqn2]
-        connection_info['data']['target_portals'] = [location, location2]
+        connection_info['data']['target_luns'] = [self._lun, 2]
-        connection_info['data']['target_iqns'] = [iqn, iqn2]
-        connection_info['data']['target_luns'] = [1, 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_run_multipath, mock_devices, mock_exists, mock_scsi_wwn,
-            mock_get_device_map, 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])
+            vol, [self._location, location2], [self._iqn, iqn2], [self._lun,
-        devs = ['/dev/disk/by-path/ip-%s-iscsi-%s-lun-1' % (location1, iqn1),
+                                                                  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_iscsi_devices.return_value = devs
-        mock_get_iqn.return_value = [iqn1, iqn2]
+        # 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 = []
+        mock_get_htcls.assert_called_once_with([(self._location, self._iqn,
-        self.connector_with_multipath.disconnect_volume(
+                                                 self._lun),
-            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),
                                                 (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_iscsiadm, mock_run_multipath, mock_devices, mock_exists,
-            mock_iscsi_devices, mock_get_multipath_device_map,
+            mock_scsi_wwn, mock_get_htcls):
-            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,
+            self, mock_run_multipath, mock_devices, mock_connect, mock_portals,
-            mock_iscsi_devices, mock_devices,
+            mock_exists):
-            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')
+    @mock.patch.object(iscsi.ISCSIConnector, '_disconnect_connection')
-    def test_get_iscsi_devices(self, walk_mock):
+    @mock.patch.object(iscsi.ISCSIConnector, '_get_connection_devices')
-        paths = [('ip-10.0.0.1:3260-iscsi-iqn.2013-01.ro.'
+    @mock.patch.object(linuxscsi.LinuxSCSI, 'flush_multipath_device')
-                 'com.netapp:node.netapp02-lun-0')]
+    @mock.patch.object(linuxscsi.LinuxSCSI, 'remove_connection',
-        walk_mock.return_value = [(['.'], ['by-path'], paths)]
+                       return_value=None)
-        self.assertEqual(self.connector._get_iscsi_devices(), paths)
+    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=[])
+        with mock.patch.object(self.connector,
-    def test_get_iscsi_devices_with_empty_dir(self, walk_mock):
+                               'use_multipath') as use_mp_mock:
-        self.assertEqual(self.connector._get_iscsi_devices(), [])
+            self.connector.disconnect_volume(self.CON_PROPS,
+                                             mock.sentinel.dev_info)
 
-    @mock.patch.object(os.path, 'realpath')
+        con_devs_mock.assert_called_once_with(self.CON_PROPS)
-    @mock.patch.object(iscsi.ISCSIConnector, '_get_iscsi_devices')
+        remove_mock.assert_called_once_with({'sda', 'sdb'}, use_mp_mock,
-    def test_get_multipath_iqns(self, get_iscsi_mock, realpath_mock):
+                                            False, mock.ANY)
-        paths = [('ip-10.0.0.1:3260-iscsi-iqn.2013-01.ro.'
+        discon_mock.assert_called_once_with(
-                  'com.netapp:node.netapp02-lun-0')]
+            self.CON_PROPS,
-        devpath = '/dev/disk/by-path/%s' % paths[0]
+            [('ip1:port1', 'tgt1'), ('ip3:port3', 'tgt3')],
-        realpath_mock.return_value = devpath
+            False, mock.ANY)
-        get_iscsi_mock.return_value = paths
+        flush_mock.assert_not_called()
-        mpath_map = {devpath: paths[0]}
-        self.assertEqual(self.connector._get_multipath_iqns([paths[0]],
-                                                            mpath_map),
-                         ['iqn.2013-01.ro.com.netapp:node.netapp02'])
 
-    @mock.patch.object(iscsi.ISCSIConnector, '_run_multipath')
+    @mock.patch('os_brick.exception.ExceptionChainer.__nonzero__',
-    def test_get_multipath_device_map(self, multipath_mock):
+                mock.Mock(return_value=True))
-        multipath_mock.return_value = [
+    @mock.patch('os_brick.exception.ExceptionChainer.__bool__',
-            "Mar 17 14:32:37 | sda: No fc_host device for 'host-1'\n"
+                mock.Mock(return_value=True))
-            "mpathb (36e00000000010001) dm-4 IET ,VIRTUAL-DISK\n"
+    @mock.patch.object(iscsi.ISCSIConnector, '_disconnect_connection')
-            "size=1.0G features='0' hwhandler='0' wp=rw\n"
+    @mock.patch.object(iscsi.ISCSIConnector, '_get_connection_devices')
-            "|-+- policy='service-time 0' prio=0 status=active\n"
+    @mock.patch.object(linuxscsi.LinuxSCSI, 'flush_multipath_device')
-            "| `- 2:0:0:1 sda 8:0 active undef running\n"
+    @mock.patch.object(linuxscsi.LinuxSCSI, 'remove_connection',
-            "`-+- policy='service-time 0' prio=0 status=enabled\n"
+                       return_value=mock.sentinel.mp_name)
-            "  `- 3:0:0:1 sdb 8:16 active undef running\n"]
+    def test_disconnect_volume_force_failure(self, remove_mock, flush_mock,
-        expected = {'/dev/sda': '/dev/mapper/mpathb',
+                                             con_devs_mock, discon_mock):
-                    '/dev/sdb': '/dev/mapper/mpathb'}
+        # Return an ordered dicts instead of normal dict for discon_mock.assert
-        self.assertEqual(expected, self.connector._get_multipath_device_map())
+        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')
+        with mock.patch.object(self.connector, 'use_multipath',
-    @mock.patch.object(iscsi.ISCSIConnector,
+                               wraps=True) as use_mp_mock:
-                       '_get_target_portals_from_iscsiadm_output')
+            self.assertRaises(exception.ExceptionChainer,
-    @mock.patch.object(iscsi.ISCSIConnector, '_rescan_iscsi')
+                              self.connector.disconnect_volume,
-    @mock.patch.object(iscsi.ISCSIConnector, '_get_iscsi_devices')
+                              self.CON_PROPS, mock.sentinel.dev_info,
-    @mock.patch.object(host_driver.HostDriver, 'get_all_block_devices')
+                              mock.sentinel.force, ignore_errors=False)
-    @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))
 
-        get_portals_mock.return_value = ([portal], [iqn1])
+        con_devs_mock.assert_called_once_with(self.CON_PROPS)
-        multipath_iqn_mock.return_value = iqns
+        remove_mock.assert_called_once_with({'sda', 'sdb'}, use_mp_mock,
-        get_all_devices_mock.return_value = [dev, '/dev/mapper/md-1']
+                                            mock.sentinel.force, mock.ANY)
-        get_multipath_device_map_mock.return_value = {dev: '/dev/mapper/md-3'}
+        discon_mock.assert_called_once_with(
-        get_iscsi_devices_mock.return_value = []
+            self.CON_PROPS,
-        fake_property = {'target_portal': portal,
+            [('ip1:port1', 'tgt1'), ('ip3:port3', 'tgt3')],
-                         'target_iqn': iqn1}
+            mock.sentinel.force, mock.ANY)
-        self.connector._disconnect_volume_multipath_iscsi(fake_property,
+        flush_mock.assert_called_once_with(mock.sentinel.mp_name)
-                                                          'fake/multipath')
-        # Target in use by other mp devices, don't disconnect
-        self.assertFalse(disconnect_mock.called)
 
-    @mock.patch.object(iscsi.ISCSIConnector,
+    @ddt.data({'do_raise': False, 'force': False},
-                       '_get_target_portals_from_iscsiadm_output')
+              {'do_raise': True, 'force': True},
-    @mock.patch.object(iscsi.ISCSIConnector, '_rescan_iscsi')
+              {'do_raise': True, 'force': False})
-    @mock.patch.object(iscsi.ISCSIConnector, '_get_iscsi_devices')
+    @ddt.unpack
-    @mock.patch.object(host_driver.HostDriver, 'get_all_block_devices')
+    @mock.patch.object(iscsi.ISCSIConnector, '_disconnect_from_iscsi_portal')
-    @mock.patch.object(iscsi.ISCSIConnector,
+    def test_disconnect_connection(self, disconnect_mock, do_raise, force):
-                       '_disconnect_from_iscsi_portal')
+        will_raise = do_raise and not force
-    @mock.patch.object(iscsi.ISCSIConnector, '_get_multipath_device_map')
+        actual_call_args = []
-    @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))
 
-        # Multiple targets are discovered, but only block devices for target-1
+        # Since we reuse the copied dictionary on _disconnect_connection
-        # is deleted and target-2 is in use.
+        # changing its values we cannot use mock's assert_has_calls
-        get_portals_mock.return_value = ([portal, portal], [iqn1, iqn2])
+        def my_disconnect(con_props):
-        multipath_iqn_mock.return_value = [iqn2, iqn2]
+            actual_call_args.append(con_props.copy())
-        get_all_devices_mock.return_value = [dev2, '/dev/mapper/md-1']
+            if do_raise:
-        get_multipath_map_mock.return_value = {dev2: '/dev/mapper/md-3'}
+                raise exception.ExceptionChainer()
-        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)
 
-    @mock.patch.object(iscsi.ISCSIConnector, '_get_multipath_device_map',
+        disconnect_mock.side_effect = my_disconnect
-                       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
 
-        get_portals_mock.return_value = ([portal], [iqn])
+        connections = (('ip1:port1', 'tgt1'), ('ip2:port2', 'tgt2'))
-        fake_property = {'target_portal': portal,
+        original_props = self.CON_PROPS.copy()
-                         'target_iqn': iqn}
+        exc = exception.ExceptionChainer()
-        self.connector._disconnect_volume_multipath_iscsi(fake_property,
+        if will_raise:
-                                                          'fake/multipath')
+            self.assertRaises(exception.ExceptionChainer,
-        # Target not in use by other mp devices, disconnect
+                              self.connector._disconnect_connection,
-        disconnect_mock.assert_called_once_with(fake_property)
+                              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',
+        # Passed properties should not be altered by the method call
-                       return_value={})
+        self.assertDictEqual(original_props, self.CON_PROPS)
-    @mock.patch.object(iscsi.ISCSIConnector,
+        expected = [original_props.copy(), original_props.copy()]
-                       '_get_target_portals_from_iscsiadm_output')
+        for i, (ip, iqn) in enumerate(connections):
-    @mock.patch.object(iscsi.ISCSIConnector, '_rescan_iscsi')
+            expected[i].update(target_portal=ip, target_iqn=iqn)
-    @mock.patch.object(iscsi.ISCSIConnector, '_get_iscsi_devices')
+        # If we are failing and not forcing we won't make all the alls
-    @mock.patch.object(host_driver.HostDriver, 'get_all_block_devices')
+        if will_raise:
-    @mock.patch.object(iscsi.ISCSIConnector,
+            expected = expected[:1]
-                       '_disconnect_from_iscsi_portal')
+        self.assertListEqual(expected, actual_call_args)
-    @mock.patch.object(os.path, 'exists', return_value=False)
+        # No exceptions have been caught by ExceptionChainer context manager
-    def test_disconnect_volume_multipath_iscsi_with_invalid_symlink(
+        self.assertEqual(do_raise, bool(exc))
-            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))
 
-        get_portals_mock.return_value = ([portal], [iqn])
+    def test_disconnect_from_iscsi_portal(self):
-        get_all_devices_mock.return_value = [dev, '/dev/mapper/md-1']
+        self.connector._disconnect_from_iscsi_portal(self.CON_PROPS)
-        get_iscsi_devices_mock.return_value = []
+        expected_prefix = ('iscsiadm -m node -T %s -p %s ' %
-
+                           (self.CON_PROPS['target_iqn'],
-        fake_property = {'target_portal': portal,
+                            self.CON_PROPS['target_portal']))
-                         'target_iqn': iqn}
+        expected = [
-        self.connector._disconnect_volume_multipath_iscsi(fake_property,
+            expected_prefix + '--op update -n node.startup -v manual',
-                                                          'fake/multipath')
+            expected_prefix + '--logout',
-        # Target not in use by other mp devices, disconnect
+            expected_prefix + '--op delete',
-        disconnect_mock.assert_called_once_with(fake_property)
+        ]
+        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')
+    @mock.patch.object(linuxscsi.LinuxSCSI, 'echo_scsi_command')
-    def test_wait_for_volume_removal(self, sleep_mock):
+    @mock.patch.object(linuxscsi.LinuxSCSI, 'flush_device_io')
-        fake_path = '/dev/disk/by-path/fake-iscsi-iqn-lun-0'
+    @mock.patch.object(os.path, 'exists', return_value=True)
-        exists_mock = mock.Mock()
+    def test_remove_scsi_device_force(self, exists_mock, flush_mock,
-        exists_mock.return_value = True
+                                      echo_mock):
-        os.path.exists = exists_mock
+        """With force we'll always call delete even if flush fails."""
-        self.assertRaises(exception.VolumePathNotRemoved,
+        exc = exception.ExceptionChainer()
-                          self.linuxscsi.wait_for_volume_removal,
+        flush_mock.side_effect = Exception()
-                          fake_path)
+        echo_mock.side_effect = Exception()
+        device = '/dev/sdc'
 
-        exists_mock = mock.Mock()
+        self.linuxscsi.remove_scsi_device(device, force=True, exc=exc)
-        exists_mock.return_value = False
+        # The context manager has caught the exceptions
-        os.path.exists = exists_mock
+        self.assertTrue(exc)
-        self.linuxscsi.wait_for_volume_removal(fake_path)
+        flush_mock.assert_called_once_with(device)
-        expected_commands = []
+        echo_mock.assert_called_once_with('/sys/block/sdc/device/delete', '1')
-        self.assertEqual(expected_commands, self.cmds)
+
-        self.assertTrue(sleep_mock.called)
+    @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')
+        dm_map_name = '3600d0230000000000e13955cc3757800'
-        expected_commands = [('multipath -f /dev/dm-9')]
+        with mock.patch.object(self.linuxscsi, '_execute') as exec_mock:
-        self.assertEqual(expected_commands, self.cmds)
+            self.linuxscsi.flush_multipath_device(dm_map_name)
 
-    @mock.patch('retrying.time.sleep', mock.Mock())
+        exec_mock.assert_called_once_with(
-    def test_flush_multipath_device_in_use(self):
+            'multipath', '-f', dm_map_name, run_as_root=True, attempts=3,
-        side_effect = (
+            timeout=300, interval=10, root_helper=self.linuxscsi._root_helper)
-            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)
 
     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')
+    @ddt.data({'do_raise': False, 'force': False},
-    @mock.patch.object(os.path, 'exists', return_value=True)
+              {'do_raise': True, 'force': True})
-    def test_remove_multipath_device(self, exists_mock, mock_multipath):
+    @ddt.unpack
-        def fake_find_multipath_device(device):
+    @mock.patch.object(linuxscsi.LinuxSCSI, 'flush_multipath_device')
-            devices = [{'device': '/dev/sde', 'host': 0,
+    @mock.patch.object(linuxscsi.LinuxSCSI, 'get_dm_name')
-                        'channel': 0, 'id': 0, 'lun': 1},
+    @mock.patch.object(linuxscsi.LinuxSCSI, 'find_sysfs_multipath_dm')
-                       {'device': '/dev/sdf', 'host': 2,
+    @mock.patch.object(linuxscsi.LinuxSCSI, 'wait_for_volumes_removal')
-                        'channel': 0, 'id': 0, 'lun': 1}, ]
+    @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",
+    @mock.patch.object(linuxscsi.LinuxSCSI, 'flush_multipath_device',
-                    "id": "350002ac20398383d",
+                       side_effect=Exception)
-                    "name": "mpatha",
+    @mock.patch.object(linuxscsi.LinuxSCSI, 'get_dm_name')
-                    "devices": devices}
+    @mock.patch.object(linuxscsi.LinuxSCSI, 'find_sysfs_multipath_dm')
-            return info
+    @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
+    @mock.patch.object(linuxscsi.LinuxSCSI, 'wait_for_volumes_removal')
-
+    @mock.patch.object(linuxscsi.LinuxSCSI, 'remove_scsi_device')
-        self.linuxscsi.remove_multipath_device('/dev/dm-3')
+    def test_remove_connection_singlepath(self, remove_mock, wait_mock):
-        expected_commands = [
+        devices_names = ('sda', 'sdb')
-            ('multipath -f mpatha'),
+        exc = exception.ExceptionChainer()
-            ('blockdev --flushbufs /dev/sde'),
+        self.linuxscsi.remove_connection(devices_names, is_multipath=False,
-            ('tee -a /sys/block/sde/device/delete'),
+                                         force=mock.sentinel.Force,
-            ('blockdev --flushbufs /dev/sdf'),
+                                         exc=exc)
-            ('tee -a /sys/block/sdf/device/delete'), ]
+        remove_mock.assert_has_calls(
-        self.assertEqual(expected_commands, self.cmds)
+            [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.