From 400ca5d6db818b966065001571e59198c6966e2f Mon Sep 17 00:00:00 2001 From: Gorka Eguileor Date: Tue, 4 Apr 2017 12:36:03 +0200 Subject: [PATCH] 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 | 69 +++ os_brick/initiator/__init__.py | 1 - os_brick/initiator/connector.py | 1 - os_brick/initiator/connectors/aoe.py | 3 +- os_brick/initiator/connectors/disco.py | 3 +- os_brick/initiator/connectors/drbd.py | 3 +- os_brick/initiator/connectors/fake.py | 3 +- .../initiator/connectors/fibre_channel.py | 3 +- os_brick/initiator/connectors/hgst.py | 3 +- os_brick/initiator/connectors/huawei.py | 3 +- os_brick/initiator/connectors/iscsi.py | 333 +++++----- os_brick/initiator/connectors/local.py | 3 +- os_brick/initiator/connectors/rbd.py | 3 +- os_brick/initiator/connectors/remotefs.py | 3 +- os_brick/initiator/connectors/scaleio.py | 3 +- os_brick/initiator/connectors/sheepdog.py | 3 +- os_brick/initiator/connectors/vmware.py | 3 +- .../initiator/connectors/vrtshyperscale.py | 3 +- os_brick/initiator/initiator_connector.py | 9 +- os_brick/initiator/linuxscsi.py | 151 +++-- os_brick/initiator/windows/fibre_channel.py | 3 +- os_brick/initiator/windows/iscsi.py | 3 +- os_brick/initiator/windows/smbfs.py | 3 +- os_brick/privileged/rootwrap.py | 117 +++- .../tests/initiator/connectors/test_iscsi.py | 568 ++++++++---------- os_brick/tests/initiator/test_linuxscsi.py | 218 +++++-- os_brick/tests/privileged/test_rootwrap.py | 72 ++- os_brick/tests/windows/fake_win_conn.py | 3 +- ...tor_iscsi_disconnect-557f4173bc1ae4ed.yaml | 12 + 29 files changed, 955 insertions(+), 650 deletions(-) create mode 100644 releasenotes/notes/refactor_iscsi_disconnect-557f4173bc1ae4ed.yaml diff --git a/os_brick/exception.py b/os_brick/exception.py index 754a19047..c047ea1d2 100644 --- a/os_brick/exception.py +++ b/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 diff --git a/os_brick/initiator/__init__.py b/os_brick/initiator/__init__.py index 9f07efcc6..e262f79d2 100644 --- a/os_brick/initiator/__init__.py +++ b/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' diff --git a/os_brick/initiator/connector.py b/os_brick/initiator/connector.py index a56263125..0f2fe41c5 100644 --- a/os_brick/initiator/connector.py +++ b/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' diff --git a/os_brick/initiator/connectors/aoe.py b/os_brick/initiator/connectors/aoe.py index 1496bfc8c..b88bfcea9 100644 --- a/os_brick/initiator/connectors/aoe.py +++ b/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 diff --git a/os_brick/initiator/connectors/disco.py b/os_brick/initiator/connectors/disco.py index a9b7afb8a..8374f81fd 100644 --- a/os_brick/initiator/connectors/disco.py +++ b/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) diff --git a/os_brick/initiator/connectors/drbd.py b/os_brick/initiator/connectors/drbd.py index edd277633..26dbed00c 100644 --- a/os_brick/initiator/connectors/drbd.py +++ b/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, diff --git a/os_brick/initiator/connectors/fake.py b/os_brick/initiator/connectors/fake.py index d564e2978..fac477bc3 100644 --- a/os_brick/initiator/connectors/fake.py +++ b/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): diff --git a/os_brick/initiator/connectors/fibre_channel.py b/os_brick/initiator/connectors/fibre_channel.py index 5cce19464..8bf87ab53 100644 --- a/os_brick/initiator/connectors/fibre_channel.py +++ b/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 diff --git a/os_brick/initiator/connectors/hgst.py b/os_brick/initiator/connectors/hgst.py index d67615137..53653a512 100644 --- a/os_brick/initiator/connectors/hgst.py +++ b/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 diff --git a/os_brick/initiator/connectors/huawei.py b/os_brick/initiator/connectors/huawei.py index 2a4d1ef37..b33131ea8 100644 --- a/os_brick/initiator/connectors/huawei.py +++ b/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 diff --git a/os_brick/initiator/connectors/iscsi.py b/os_brick/initiator/connectors/iscsi.py index b2534e15e..d4e21a557 100644 --- a/os_brick/initiator/connectors/iscsi.py +++ b/os_brick/initiator/connectors/iscsi.py @@ -17,7 +17,6 @@ import collections import copy import glob import os -import re import time from oslo_concurrency import lockutils @@ -106,27 +105,63 @@ class ISCSIConnector(base.BaseLinuxConnector, base_iscsi.BaseISCSIConnector): return volume_paths - def _get_iscsi_sessions(self): + def _get_iscsi_sessions_full(self): + """Get iSCSI session information as a list of tuples. + + Uses iscsiadm -m session and from a command output like + tcp: [1] 192.168.121.250:3260,1 iqn.2010-10.org.openstack:volume- + + This method will drop the node type and return a list like this: + [('tcp:', '1', '192.168.121.250:3260', '1', + 'iqn.2010-10.org.openstack:volume-')] + """ out, err = self._run_iscsi_session() - - iscsi_sessions = [] - if err: LOG.warning("Couldn't find iscsi sessions because " - "iscsiadm err: %s", - err) - else: - # parse the output from iscsiadm - # lines are in the format of - # tcp: [1] 192.168.121.250:3260,1 iqn.2010-10.org.openstack:volume- - lines = out.split('\n') - for line in lines: - if line: - entries = line.split() - portal = entries[2].split(',') - iscsi_sessions.append(portal[0]) + "iscsiadm err: %s", err) + return [] - return iscsi_sessions + # Parse and clean the output from iscsiadm, which is in the form of: + # transport_name: [session_id] ip_address:port,tpgt iqn node_type + lines = [] + for line in out.splitlines(): + if line: + info = line.split() + sid = info[1][1:-1] + portal, tpgt = info[2].split(',') + lines.append((info[0], sid, portal, tpgt, info[3])) + return lines + + def _get_iscsi_nodes(self): + """Get iSCSI node information (portal, iqn) as a list of tuples. + + Uses iscsi_adm -m node and from a command output like + 192.168.121.250:3260,1 iqn.2010-10.org.openstack:volume + + This method will drop the tpgt and return a list like this: + [('192.168.121.250:3260', 'iqn.2010-10.org.openstack:volume')] + """ + out, err = self._execute('iscsiadm', '-m', 'node', run_as_root=True, + root_helper=self._root_helper, + check_exit_code=False) + if err: + LOG.warning("Couldn't find iSCSI nodes because iscsiadm err: %s", + err) + return [] + + # Parse and clean the output from iscsiadm which is in the form of: + # ip_addresss:port,tpgt iqn + lines = [] + for line in out.splitlines(): + if line: + info = line.split() + lines.append((info[0].split(',')[0], info[1])) + return lines + + def _get_iscsi_sessions(self): + """Return portals for all existing sessions.""" + # entry: [tcp, [1], 192.168.121.250:3260,1 ...] + return [entry[2] for entry in self._get_iscsi_sessions_full()] def _get_potential_volume_paths(self, connection_properties, connect_to_portal=True, @@ -471,85 +506,103 @@ class ISCSIConnector(base.BaseLinuxConnector, base_iscsi.BaseISCSIConnector): LOG.debug("connect_volume returning %s", device_info) return device_info + def _get_connection_devices(self, connection_properties): + """Get map of devices by sessions from our connection. + + For each of the TCP sessions that correspond to our connection + properties we generate a map of (ip, iqn) to (belong, other) where + belong is a set of devices in that session that populated our system + when we did a connection using connection properties, and other are + any other devices that share that same session but are the result of + connecting with different connection properties. + + We also include all nodes from our connection that don't have a + session. + """ + ips_iqns_luns = self._get_all_targets(connection_properties) + nodes = self._get_iscsi_nodes() + sessions = self._get_iscsi_sessions_full() + # Use (portal, iqn) to map the session value + sessions_map = {(s[2], s[4]): s[1] for s in sessions if s[0] == 'tcp:'} + # device_map will keep a tuple with devices from the connection and + # others that don't belong to this connection" (belong, others) + device_map = collections.defaultdict(lambda: (set(), set())) + + for ip, iqn, lun in ips_iqns_luns: + session = sessions_map.get((ip, iqn)) + # Our nodes that don't have a session will be returned as empty + if not session: + if (ip, iqn) in nodes: + device_map[(ip, iqn)] = (set(), set()) + continue + + # Get all devices for the session + paths = glob.glob('/sys/class/scsi_host/host*/device/session' + + session + '/target*/*:*:*:*/block/*') + belong, others = device_map[(ip, iqn)] + for path in paths: + __, hctl, __, device = path.rsplit('/', 3) + lun_path = int(hctl.rsplit(':', 1)[-1]) + # For partitions turn them into the whole device: sde1 -> sde + device = device.strip('0123456789') + if lun_path == lun: + belong.add(device) + else: + others.add(device) + + return device_map + @utils.trace @synchronized('connect_volume') - def disconnect_volume(self, connection_properties, device_info): + def disconnect_volume(self, connection_properties, device_info, + force=False, ignore_errors=False): """Detach the volume from instance_name. :param connection_properties: The dictionary that describes all of the target volume attributes. - :type connection_properties: dict + :type connection_properties: dict that must include: + target_portal(s) - IP and optional port + target_iqn(s) - iSCSI Qualified Name + target_lun(s) - LUN id of the volume :param device_info: historical difference, but same as connection_props :type device_info: dict - - connection_properties for iSCSI must include: - target_portal(s) - IP and optional port - target_iqn(s) - iSCSI Qualified Name - target_lun(s) - LUN id of the volume + :param force: Whether to forcefully disconnect even if flush fails. + :type force: bool + :param ignore_errors: When force is True, this will decide whether to + ignore errors or raise an exception once finished + the operation. Default is False. + :type ignore_errors: bool """ - if self.use_multipath: - host_device = multipath_device = None - host_devices = self._get_device_path(connection_properties) - # Choose an accessible host device - for dev in host_devices: - if os.path.exists(dev): - host_device = dev - device_wwn = self._linuxscsi.get_scsi_wwn(dev) - (multipath_device, multipath_id) = (super( - ISCSIConnector, self)._discover_mpath_device( - device_wwn, connection_properties, dev)) - if multipath_device: - break - if not host_device: - LOG.error("No accessible volume device: %(host_devices)s", - {'host_devices': host_devices}) - raise exception.VolumeDeviceNotFound(device=host_devices) + exc = exception.ExceptionChainer() + devices_map = self._get_connection_devices(connection_properties) - if multipath_device: - device_realpath = os.path.realpath(host_device) - self._linuxscsi.remove_multipath_device(device_realpath) - return self._disconnect_volume_multipath_iscsi( - connection_properties, multipath_device) + # Remove devices and multipath from this connection + remove_devices = set() + for remove, __ in devices_map.values(): + remove_devices.update(remove) + multipath_name = self._linuxscsi.remove_connection(remove_devices, + self.use_multipath, + force, exc) - # When multiple portals/iqns/luns are specified, we need to remove - # unused devices created by logging into other LUNs' session. - for props in self._iterate_all_targets(connection_properties): - self._disconnect_volume_iscsi(props) + # Disconnect sessions and remove nodes that are left without devices + disconnect = [conn for conn, (__, keep) in devices_map.items() + if not keep] + self._disconnect_connection(connection_properties, disconnect, force, + exc) - def _disconnect_volume_iscsi(self, connection_properties): - # remove the device from the scsi subsystem - # this eliminates any stale entries until logout - host_devices = self._get_device_path(connection_properties) + # If flushing the multipath failed before, try now after we have + # removed the devices and we may have even logged off (only reaches + # here with multipath_name if force=True). + if multipath_name: + LOG.debug('Flushing again multipath %s now that we removed the ' + 'devices.', multipath_name) + self._linuxscsi.flush_multipath_device(multipath_name) - if host_devices: - host_device = host_devices[0] - else: - return - - dev_name = self._linuxscsi.get_name_from_path(host_device) - if dev_name: - self._linuxscsi.remove_scsi_device(dev_name) - - # NOTE(jdg): On busy systems we can have a race here - # where remove_iscsi_device is called before the device file - # has actually been removed. The result is an orphaned - # iscsi session that never gets logged out. The following - # call to wait addresses that issue. - self._linuxscsi.wait_for_volume_removal(host_device) - - # NOTE(vish): Only disconnect from the target if no luns from the - # target are in use. - device_byname = ("ip-%(portal)s-iscsi-%(iqn)s-lun-" % - {'portal': connection_properties['target_portal'], - 'iqn': connection_properties['target_iqn']}) - devices = self.driver.get_all_block_devices() - devices = [dev for dev in devices if (device_byname in dev - and - dev.startswith( - '/dev/disk/by-path/')) - and os.path.exists(dev)] - if not devices: - self._disconnect_from_iscsi_portal(connection_properties) + if exc: + LOG.warning('There were errors removing %s, leftovers may remain ' + 'in the system', remove_devices) + if not ignore_errors: + raise exc def _munge_portal(self, target): """Remove brackets from portal. @@ -635,65 +688,6 @@ class ISCSIConnector(base.BaseLinuxConnector, base_iscsi.BaseISCSIConnector): iqns.append(data[1]) return ips, iqns - def _disconnect_volume_multipath_iscsi(self, connection_properties, - multipath_name): - """This removes a multipath device and it's LUNs.""" - LOG.debug("Disconnect multipath device %s", multipath_name) - mpath_map = self._get_multipath_device_map() - block_devices = self.driver.get_all_block_devices() - devices = [] - for dev in block_devices: - if os.path.exists(dev): - if "/mapper/" in dev: - devices.append(dev) - else: - mpdev = mpath_map.get(dev) - if mpdev: - devices.append(mpdev) - - # Do a discovery to find all targets. - # Targets for multiple paths for the same multipath device - # may not be the same. - all_ips_iqns_luns = self._discover_iscsi_portals(connection_properties) - - # As discovery result may contain other targets' iqns, extract targets - # to be disconnected whose block devices are already deleted here. - ips_iqns = [] - entries = [device.lstrip('ip-').split('-lun-')[0] - for device in self._get_iscsi_devices()] - for ip, iqn, lun in all_ips_iqns_luns: - ip_iqn = "%s-iscsi-%s" % (ip.split(",")[0], iqn) - if ip_iqn not in entries: - ips_iqns.append([ip, iqn]) - - if not devices: - # disconnect if no other multipath devices - self._disconnect_mpath(connection_properties, ips_iqns) - return - - # Get a target for all other multipath devices - other_iqns = self._get_multipath_iqns(devices, mpath_map) - - # Get all the targets for the current multipath device - current_iqns = [iqn for ip, iqn in ips_iqns] - - in_use = False - for current in current_iqns: - if current in other_iqns: - in_use = True - break - - # If no other multipath device attached has the same iqn - # as the current device - if not in_use: - # disconnect if no other multipath devices with same iqn - self._disconnect_mpath(connection_properties, ips_iqns) - return - - # else do not disconnect iscsi portals, - # as they are used for other luns - return - def _connect_to_iscsi_portal(self, connection_properties): # NOTE(vish): If we are on the same host as nova volume, the # discovery makes the target so we don't need to @@ -774,54 +768,15 @@ class ISCSIConnector(base.BaseLinuxConnector, base_iscsi.BaseISCSIConnector): attempts=5, delay_on_retry=True) - def _get_iscsi_devices(self): - try: - devices = list(os.walk('/dev/disk/by-path'))[0][-1] - except IndexError: - return [] - # For iSCSI HBAs, look at an offset of len('pci-0000:00:00.0') - return [entry for entry in devices if (entry.startswith("ip-") - or (entry.startswith("pci-") - and - entry.find("ip-", 16, 21) - >= 16))] - - def _disconnect_mpath(self, connection_properties, ips_iqns): - for ip, iqn in ips_iqns: - props = copy.deepcopy(connection_properties) + def _disconnect_connection(self, connection_properties, connections, force, + exc): + LOG.debug('Disconnecting from: %s', connections) + props = connection_properties.copy() + for ip, iqn in connections: props['target_portal'] = ip props['target_iqn'] = iqn - self._disconnect_from_iscsi_portal(props) - - def _get_multipath_iqns(self, multipath_devices, mpath_map): - entries = self._get_iscsi_devices() - iqns = [] - for entry in entries: - entry_real_path = os.path.realpath("/dev/disk/by-path/%s" % entry) - entry_multipath = mpath_map.get(entry_real_path) - if entry_multipath and entry_multipath in multipath_devices: - iqns.append(entry.split("iscsi-")[1].split("-lun")[0]) - return iqns - - def _get_multipath_device_map(self): - out = self._run_multipath(['-ll'], check_exit_code=[0, 1])[0] - mpath_line = [line for line in out.splitlines() - if not re.match(initiator.MULTIPATH_ERROR_REGEX, line)] - mpath_dev = None - mpath_map = {} - for line in out.splitlines(): - m = initiator.MULTIPATH_DEV_CHECK_REGEX.split(line) - if len(m) >= 2: - mpath_dev = '/dev/mapper/' + m[0].split(" ")[0] - continue - m = initiator.MULTIPATH_PATH_CHECK_REGEX.split(line) - if len(m) >= 2: - mpath_map['/dev/' + m[1].split(" ")[0]] = mpath_dev - - if mpath_line and not mpath_map: - LOG.warning("Failed to parse the output of multipath -ll. " - "stdout: %s", out) - return mpath_map + with exc.context(force, 'Disconnect from %s %s failed', ip, iqn): + self._disconnect_from_iscsi_portal(props) def _run_iscsi_session(self): (out, err) = self._run_iscsiadm_bare(('-m', 'session'), diff --git a/os_brick/initiator/connectors/local.py b/os_brick/initiator/connectors/local.py index 6b84a8e0c..4741bcd0c 100644 --- a/os_brick/initiator/connectors/local.py +++ b/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 diff --git a/os_brick/initiator/connectors/rbd.py b/os_brick/initiator/connectors/rbd.py index 7c6338f50..9bca3519f 100644 --- a/os_brick/initiator/connectors/rbd.py +++ b/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 diff --git a/os_brick/initiator/connectors/remotefs.py b/os_brick/initiator/connectors/remotefs.py index ad2a19c7b..71e66cbbc 100644 --- a/os_brick/initiator/connectors/remotefs.py +++ b/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 diff --git a/os_brick/initiator/connectors/scaleio.py b/os_brick/initiator/connectors/scaleio.py index 3754dee44..e07911bdd 100644 --- a/os_brick/initiator/connectors/scaleio.py +++ b/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 diff --git a/os_brick/initiator/connectors/sheepdog.py b/os_brick/initiator/connectors/sheepdog.py index 090feb63c..c224b011c 100644 --- a/os_brick/initiator/connectors/sheepdog.py +++ b/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 diff --git a/os_brick/initiator/connectors/vmware.py b/os_brick/initiator/connectors/vmware.py index 0f83593a2..68887eb2d 100644 --- a/os_brick/initiator/connectors/vmware.py +++ b/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 diff --git a/os_brick/initiator/connectors/vrtshyperscale.py b/os_brick/initiator/connectors/vrtshyperscale.py index 3c352e103..b34315d2e 100644 --- a/os_brick/initiator/connectors/vrtshyperscale.py +++ b/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 diff --git a/os_brick/initiator/initiator_connector.py b/os_brick/initiator/initiator_connector.py index 5b360bfc9..34c84d671 100644 --- a/os_brick/initiator/initiator_connector.py +++ b/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 diff --git a/os_brick/initiator/linuxscsi.py b/os_brick/initiator/linuxscsi.py index 6e7b9ce5c..6f22c9f98 100644 --- a/os_brick/initiator/linuxscsi.py +++ b/os_brick/initiator/linuxscsi.py @@ -16,6 +16,7 @@ Note, this is not iSCSI. """ +import glob import os import re import six @@ -56,30 +57,32 @@ class LinuxSCSI(executor.Executor): else: return None - def remove_scsi_device(self, device): + def remove_scsi_device(self, device, force=False, exc=None): """Removes a scsi device based upon /dev/sdX name.""" - path = "/sys/block/%s/device/delete" % device.replace("/dev/", "") if os.path.exists(path): + exc = exception.ExceptionChainer() if exc is None else exc # flush any outstanding IO first - self.flush_device_io(device) + with exc.context(force, 'Flushing %s failed', device): + self.flush_device_io(device) LOG.debug("Remove SCSI device %(device)s with %(path)s", {'device': device, 'path': path}) - self.echo_scsi_command(path, "1") + with exc.context(force, 'Removing %s failed', device): + self.echo_scsi_command(path, "1") - @utils.retry(exceptions=exception.VolumePathNotRemoved, retries=3, - backoff_rate=2) - def wait_for_volume_removal(self, volume_path): - """This is used to ensure that volumes are gone.""" - LOG.debug("Checking to see if SCSI volume %s has been removed.", - volume_path) - if os.path.exists(volume_path): - LOG.debug("%(path)s still exists.", {'path': volume_path}) - raise exception.VolumePathNotRemoved( - volume_path=volume_path) - else: - LOG.debug("SCSI volume %s has been removed.", volume_path) + @utils.retry(exceptions=exception.VolumePathNotRemoved) + def wait_for_volumes_removal(self, volumes_names): + """Wait for device paths to be removed from the system.""" + str_names = ', '.join(volumes_names) + LOG.debug('Checking to see if SCSI volumes %s have been removed.', + str_names) + exist = [volume_name for volume_name in volumes_names + if os.path.exists('/dev/' + volume_name)] + if exist: + LOG.debug('%s still exist.', ', '.join(exist)) + raise exception.VolumePathNotRemoved(volume_path=exist) + LOG.debug("SCSI volumes %s have been removed.", str_names) def get_device_info(self, device): (out, _err) = self._execute('sg_scan', device, run_as_root=True, @@ -125,52 +128,92 @@ class LinuxSCSI(executor.Executor): return True - def remove_multipath_device(self, device): - """Removes related LUNs and multipath device + def get_dm_name(self, dm): + """Get the Device map name given the device name of the dm on sysfs. - This removes LUNs associated with a multipath device - and the multipath device itself. + :param dm: Device map name as seen in sysfs. ie: 'dm-0' + :returns: String with the name, or empty string if not available. + ie: '36e843b658476b7ed5bc1d4d10d9b1fde' """ + try: + with open('/sys/block/' + dm + '/dm/name') as f: + return f.read().strip() + except IOError: + return '' - LOG.debug("remove multipath device %s", device) - mpath_dev = self.find_multipath_device(device) - if mpath_dev: - self.flush_multipath_device(mpath_dev['name']) - devices = mpath_dev['devices'] - LOG.debug("multipath LUNs to remove %s", devices) - for device in devices: - self.remove_scsi_device(device['device']) + def find_sysfs_multipath_dm(self, device_names): + """Find the dm device name given a list of device names + + :param device_names: Iterable with device names, not paths. ie: ['sda'] + :returns: String with the dm name or None if not found. ie: 'dm-0' + """ + glob_str = '/sys/block/%s/holders/dm-*' + for dev_name in device_names: + dms = glob.glob(glob_str % dev_name) + if dms: + __, device_name, __, dm = dms[0].rsplit('/', 3) + return dm + return None + + def remove_connection(self, devices_names, is_multipath, force=False, + exc=None): + """Remove LUNs and multipath associated with devices names. + + :param devices_names: Iterable with real device names ('sda', 'sdb') + :param is_multipath: Whether this is a multipath connection or not + :param force: Whether to forcefully disconnect even if flush fails. + :param exc: ExceptionChainer where to add exceptions if forcing + :returns: Multipath device map name if found and not flushed + """ + if not devices_names: + return + multipath_name = None + exc = exception.ExceptionChainer() if exc is None else exc + LOG.debug('Removing %(type)s devices %(devices)s', + {'type': 'multipathed' if is_multipath else 'single pathed', + 'devices': ', '.join(devices_names)}) + + if is_multipath: + multipath_dm = self.find_sysfs_multipath_dm(devices_names) + multipath_name = multipath_dm and self.get_dm_name(multipath_dm) + if multipath_name: + with exc.context(force, 'Flushing %s failed', multipath_name): + self.flush_multipath_device(multipath_name) + multipath_name = None + + for device_name in devices_names: + self.remove_scsi_device('/dev/' + device_name, force, exc) + + # Wait until the symlinks are removed + with exc.context(force, 'Some devices remain from %s', devices_names): + self.wait_for_volumes_removal(devices_names) + + return multipath_name def flush_device_io(self, device): """This is used to flush any remaining IO in the buffers.""" - try: - LOG.debug("Flushing IO for device %s", device) - self._execute('blockdev', '--flushbufs', device, run_as_root=True, - root_helper=self._root_helper) - except putils.ProcessExecutionError as exc: - LOG.warning("Failed to flush IO buffers prior to removing " - "device: %(code)s", {'code': exc.exit_code}) - - @utils.retry(exceptions=putils.ProcessExecutionError) - def flush_multipath_device(self, device): - try: - LOG.debug("Flush multipath device %s", device) - self._execute('multipath', '-f', device, run_as_root=True, - root_helper=self._root_helper) - except putils.ProcessExecutionError as exc: - if exc.exit_code == 1 and 'map in use' in exc.stdout: - LOG.debug('Multipath is in use, cannot be flushed yet.') + if os.path.exists(device): + try: + # NOTE(geguileo): With 30% connection error rates flush can get + # stuck, set timeout to prevent it from hanging here forever. + # Retry twice after 20 and 40 seconds. + LOG.debug("Flushing IO for device %s", device) + self._execute('blockdev', '--flushbufs', device, + run_as_root=True, attempts=3, timeout=300, + interval=10, root_helper=self._root_helper) + except putils.ProcessExecutionError as exc: + LOG.warning("Failed to flush IO buffers prior to removing " + "device: %(code)s", {'code': exc.exit_code}) raise - LOG.warning("multipath call failed exit %(code)s", - {'code': exc.exit_code}) - def flush_multipath_devices(self): - try: - self._execute('multipath', '-F', run_as_root=True, - root_helper=self._root_helper) - except putils.ProcessExecutionError as exc: - LOG.warning("multipath call failed exit %(code)s", - {'code': exc.exit_code}) + def flush_multipath_device(self, device_map_name): + LOG.debug("Flush multipath device %s", device_map_name) + # NOTE(geguileo): With 30% connection error rates flush can get stuck, + # set timeout to prevent it from hanging here forever. Retry twice + # after 20 and 40 seconds. + self._execute('multipath', '-f', device_map_name, run_as_root=True, + attempts=3, timeout=300, interval=10, + root_helper=self._root_helper) @utils.retry(exceptions=exception.VolumeDeviceNotFound) def wait_for_path(self, volume_path): diff --git a/os_brick/initiator/windows/fibre_channel.py b/os_brick/initiator/windows/fibre_channel.py index 10ef2d213..7995fde96 100644 --- a/os_brick/initiator/windows/fibre_channel.py +++ b/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 diff --git a/os_brick/initiator/windows/iscsi.py b/os_brick/initiator/windows/iscsi.py index 26a341dd2..9a2f9435f 100644 --- a/os_brick/initiator/windows/iscsi.py +++ b/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, diff --git a/os_brick/initiator/windows/smbfs.py b/os_brick/initiator/windows/smbfs.py index 0a8a1f016..4465bbe9a 100644 --- a/os_brick/initiator/windows/smbfs.py +++ b/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) diff --git a/os_brick/privileged/rootwrap.py b/os_brick/privileged/rootwrap.py index 488eb7bb1..ef630e74e 100644 --- a/os_brick/privileged/rootwrap.py +++ b/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) diff --git a/os_brick/tests/initiator/connectors/test_iscsi.py b/os_brick/tests/initiator/connectors/test_iscsi.py index e66be4a30..2b9e65b71 100644 --- a/os_brick/tests/initiator/connectors/test_iscsi.py +++ b/os_brick/tests/initiator/connectors/test_iscsi.py @@ -11,12 +11,14 @@ # WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the # License for the specific language governing permissions and limitations # under the License. +import collections import glob import mock import os import testtools import time +import ddt from oslo_concurrency import processutils as putils from os_brick import exception @@ -28,18 +30,117 @@ from os_brick.privileged import rootwrap as priv_rootwrap from os_brick.tests.initiator import test_connector +@ddt.ddt class ISCSIConnectorTestCase(test_connector.ConnectorTestCase): + CON_PROPS = { + 'volume_id': 'vol_id', + 'target_portal': 'ip1:port1', + 'target_iqn': 'tgt1', + 'target_lun': 4, + 'target_portals': ['ip1:port1', 'ip2:port2', 'ip3:port3', + 'ip4:port4'], + 'target_iqns': ['tgt1', 'tgt2', 'tgt3', 'tgt4'], + 'target_luns': [4, 5, 6, 7], + } def setUp(self): super(ISCSIConnectorTestCase, self).setUp() self.connector = iscsi.ISCSIConnector( None, execute=self.fake_execute, use_multipath=False) + self.connector_with_multipath = iscsi.ISCSIConnector( None, execute=self.fake_execute, use_multipath=True) - self.mock_object(self.connector._linuxscsi, 'get_name_from_path', return_value="/dev/sdb") self._fake_iqn = 'iqn.1234-56.foo.bar:01:23456789abc' + self._name = 'volume-00000001' + self._iqn = 'iqn.2010-10.org.openstack:%s' % self._name + self._location = '10.0.2.15:3260' + self._lun = 1 + + @mock.patch.object(iscsi.ISCSIConnector, '_run_iscsi_session') + def test_get_iscsi_sessions_full(self, sessions_mock): + iscsiadm_result = ('tcp: [session1] ip1:port1,1 tgt1 (non-flash)\n' + 'tcp: [session2] ip2:port2,-1 tgt2 (non-flash)\n' + 'tcp: [session3] ip3:port3,1 tgt3\n') + sessions_mock.return_value = (iscsiadm_result, '') + res = self.connector._get_iscsi_sessions_full() + expected = [('tcp:', 'session1', 'ip1:port1', '1', 'tgt1'), + ('tcp:', 'session2', 'ip2:port2', '-1', 'tgt2'), + ('tcp:', 'session3', 'ip3:port3', '1', 'tgt3')] + self.assertListEqual(expected, res) + + @mock.patch.object(iscsi.ISCSIConnector, '_run_iscsi_session', + return_value=(None, 'error')) + def test_get_iscsi_sessions_full_error(self, sessions_mock): + res = self.connector._get_iscsi_sessions_full() + self.assertEqual([], res) + sessions_mock.assert_called() + + @mock.patch.object(iscsi.ISCSIConnector, '_get_iscsi_sessions_full') + def test_get_iscsi_sessions(self, sessions_mock): + sessions_mock.return_value = [ + ('tcp:', 'session1', 'ip1:port1', '1', 'tgt1'), + ('tcp:', 'session2', 'ip2:port2', '-1', 'tgt2'), + ('tcp:', 'session3', 'ip3:port3', '1', 'tgt3')] + res = self.connector._get_iscsi_sessions() + expected = ['ip1:port1', 'ip2:port2', 'ip3:port3'] + self.assertListEqual(expected, res) + + @mock.patch.object(iscsi.ISCSIConnector, '_get_iscsi_sessions_full', + return_value=[]) + def test_get_iscsi_sessions_no_sessions(self, sessions_mock): + res = self.connector._get_iscsi_sessions() + self.assertListEqual([], res) + sessions_mock.assert_called() + + @mock.patch.object(iscsi.ISCSIConnector, '_execute') + def test_get_iscsi_nodes(self, exec_mock): + iscsiadm_result = ('ip1:port1,1 tgt1\nip2:port2,-1 tgt2\n' + 'ip3:port3,1 tgt3\n') + exec_mock.return_value = (iscsiadm_result, '') + res = self.connector._get_iscsi_nodes() + expected = [('ip1:port1', 'tgt1'), ('ip2:port2', 'tgt2'), + ('ip3:port3', 'tgt3')] + self.assertListEqual(expected, res) + exec_mock.assert_called_once_with( + 'iscsiadm', '-m', 'node', run_as_root=True, + root_helper=self.connector._root_helper, check_exit_code=False) + + @mock.patch.object(iscsi.ISCSIConnector, '_execute') + def test_get_iscsi_nodes_error(self, exec_mock): + exec_mock.return_value = (None, 'error') + res = self.connector._get_iscsi_nodes() + self.assertEqual([], res) + + @mock.patch('glob.glob') + @mock.patch.object(iscsi.ISCSIConnector, '_get_iscsi_sessions_full') + @mock.patch.object(iscsi.ISCSIConnector, '_get_iscsi_nodes') + def test_get_connection_devices(self, nodes_mock, sessions_mock, + glob_mock): + # List sessions from other targets and non tcp sessions + sessions_mock.return_value = [ + ('non-tcp:', '0', 'ip1:port1', '1', 'tgt1'), + ('tcp:', '1', 'ip1:port1', '1', 'tgt1'), + ('tcp:', '2', 'ip2:port2', '-1', 'tgt2'), + ('tcp:', '3', 'ip1:port1', '1', 'tgt4'), + ('tcp:', '4', 'ip2:port2', '-1', 'tgt5')] + # List 1 node without sessions + nodes_mock.return_value = [('ip1:port1', 'tgt1'), + ('ip2:port2', 'tgt2'), + ('ip3:port3', 'tgt3')] + sys_cls = '/sys/class/scsi_host/host' + glob_mock.side_effect = [ + [sys_cls + '1/device/session1/target6/1:2:6:4/block/sda', + sys_cls + '1/device/session1/target6/1:2:6:4/block/sda1'], + [sys_cls + '2/device/session2/target7/2:2:7:5/block/sdb', + sys_cls + '2/device/session2/target7/2:2:7:4/block/sdc'], + ] + res = self.connector._get_connection_devices(self.CON_PROPS) + expected = {('ip1:port1', 'tgt1'): ({'sda'}, set()), + ('ip2:port2', 'tgt2'): ({'sdb'}, {'sdc'}), + ('ip3:port3', 'tgt3'): (set(), set())} + self.assertDictEqual(expected, res) def generate_device(self, location, iqn, transport=None, lun=1): dev_format = "ip-%s-iscsi-%s-lun-%s" % (location, iqn, lun) @@ -236,70 +337,45 @@ class ISCSIConnectorTestCase(test_connector.ConnectorTestCase): self.assertEqual(expected_result, result) @mock.patch('time.sleep', mock.Mock()) + @mock.patch.object(iscsi.ISCSIConnector, 'disconnect_volume') def _test_connect_volume(self, extra_props, additional_commands, - transport=None, disconnect_mock=None): + disconnect_vol_mock, transport=None): # for making sure the /dev/disk/by-path is gone exists_mock = mock.Mock() exists_mock.return_value = True os.path.exists = exists_mock - location = '10.0.2.15:3260' - name = 'volume-00000001' - iqn = 'iqn.2010-10.org.openstack:%s' % name - vol = {'id': 1, 'name': name} - connection_info = self.iscsi_connection(vol, location, iqn) + vol = {'id': 1, 'name': self._name} + connection_info = self.iscsi_connection(vol, self._location, self._iqn) for key, value in extra_props.items(): connection_info['data'][key] = value if transport is not None: - dev_list = self.generate_device(location, iqn, transport) + dev_list = self.generate_device(self._location, self._iqn, + transport) with mock.patch.object(glob, 'glob', return_value=[dev_list]): device = self.connector.connect_volume(connection_info['data']) else: device = self.connector.connect_volume(connection_info['data']) - dev_str = self.generate_device(location, iqn, transport) + dev_str = self.generate_device(self._location, self._iqn, transport) self.assertEqual(device['type'], 'block') self.assertEqual(device['path'], dev_str) self.count = 0 - def mock_exists_effect(*args, **kwargs): - self.count = self.count + 1 - if self.count == 4: - return False - else: - return True + # Disconnect has its own tests, should not be tested here + expected_commands = [ + ('iscsiadm -m node -T %s -p %s' % (self._iqn, self._location)), + ('iscsiadm -m session'), + ('iscsiadm -m node -T %s -p %s --login' % (self._iqn, + self._location)), + ('iscsiadm -m node -T %s -p %s --op update' + ' -n node.startup -v automatic' % (self._iqn, + self._location)), + ('/lib/udev/scsi_id --page 0x83 --whitelisted %s' % dev_str), + ] + additional_commands - if disconnect_mock is None: - disconnect_mock = mock_exists_effect - - with mock.patch.object(os.path, 'exists', - side_effect=disconnect_mock): - if transport is not None: - dev_list = self.generate_device(location, iqn, transport) - with mock.patch.object(glob, 'glob', return_value=[dev_list]): - self.connector.disconnect_volume(connection_info['data'], - device) - else: - self.connector.disconnect_volume(connection_info['data'], - device) - - expected_commands = [ - ('iscsiadm -m node -T %s -p %s' % (iqn, location)), - ('iscsiadm -m session'), - ('iscsiadm -m node -T %s -p %s --login' % (iqn, location)), - ('iscsiadm -m node -T %s -p %s --op update' - ' -n node.startup -v automatic' % (iqn, location)), - ('/lib/udev/scsi_id --page 0x83 --whitelisted %s' % dev_str), - ('blockdev --flushbufs /dev/sdb'), - ('tee -a /sys/block/sdb/device/delete'), - ('iscsiadm -m node -T %s -p %s --op update' - ' -n node.startup -v manual' % (iqn, location)), - ('iscsiadm -m node -T %s -p %s --logout' % (iqn, location)), - ('iscsiadm -m node -T %s -p %s --op delete' % - (iqn, location)), ] + additional_commands - - self.assertEqual(expected_commands, self.cmds) + self.assertEqual(expected_commands, self.cmds) @testtools.skipUnless(os.path.exists('/dev/disk/by-path'), 'Test requires /dev/disk/by-path') @@ -311,63 +387,26 @@ class ISCSIConnectorTestCase(test_connector.ConnectorTestCase): @mock.patch.object(iscsi.ISCSIConnector, '_get_transport') def test_connect_volume_with_transport(self, mock_transport): mock_transport.return_value = 'fake_transport' - self._test_connect_volume({}, [], 'fake_transport') + self._test_connect_volume({}, [], transport='fake_transport') @testtools.skipUnless(os.path.exists('/dev/disk/by-path'), 'Test requires /dev/disk/by-path') - def test_connect_volume_with_alternative_targets(self): - location = '10.0.2.15:3260' - location2 = '[2001:db8::1]:3260' - iqn = 'iqn.2010-10.org.openstack:volume-00000001' - iqn2 = 'iqn.2010-10.org.openstack:volume-00000001-2' - extra_props = {'target_portals': [location, location2], - 'target_iqns': [iqn, iqn2], - 'target_luns': [1, 2]} - additional_commands = [('blockdev --flushbufs /dev/sdb'), - ('tee -a /sys/block/sdb/device/delete'), - ('iscsiadm -m node -T %s -p %s --op update' - ' -n node.startup -v manual' % - (iqn2, location2)), - ('iscsiadm -m node -T %s -p %s --logout' % - (iqn2, location2)), - ('iscsiadm -m node -T %s -p %s --op delete' % - (iqn2, location2))] - - def mock_exists_effect(*args, **kwargs): - self.count = self.count + 1 - # we have 2 targets in this test, so we need - # to make sure we remove and detect removal - # for both. - if (self.count == 4 or - self.count == 8): - return False - else: - return True - - self._test_connect_volume(extra_props, additional_commands, - disconnect_mock=mock_exists_effect) - - @testtools.skipUnless(os.path.exists('/dev/disk/by-path'), - 'Test requires /dev/disk/by-path') - @mock.patch.object(os.path, 'exists') + @mock.patch('os.path.exists', side_effect=(True,) * 4 + (False, False)) @mock.patch.object(iscsi.ISCSIConnector, '_run_iscsiadm') def test_connect_volume_with_alternative_targets_primary_error( self, mock_iscsiadm, mock_exists): - location = '10.0.2.15:3260' location2 = '[2001:db8::1]:3260' dev_loc2 = '2001:db8::1:3260' # udev location2 - name = 'volume-00000001' - iqn = 'iqn.2010-10.org.openstack:%s' % name - iqn2 = 'iqn.2010-10.org.openstack:%s-2' % name - vol = {'id': 1, 'name': name} - connection_info = self.iscsi_connection(vol, location, iqn) - connection_info['data']['target_portals'] = [location, location2] - connection_info['data']['target_iqns'] = [iqn, iqn2] - connection_info['data']['target_luns'] = [1, 2] + iqn2 = 'iqn.2010-10.org.openstack:%s-2' % self._name + vol = {'id': 1, 'name': self._name} + connection_info = self.iscsi_connection(vol, self._location, self._iqn) + connection_info['data']['target_portals'] = [self._location, location2] + connection_info['data']['target_iqns'] = [self._iqn, iqn2] + connection_info['data']['target_luns'] = [self._lun, 2] dev_str2 = '/dev/disk/by-path/ip-%s-iscsi-%s-lun-2' % (dev_loc2, iqn2) def fake_run_iscsiadm(iscsi_properties, iscsi_command, **kwargs): - if iscsi_properties['target_portal'] == location: + if iscsi_properties['target_portal'] == self._location: if iscsi_command == ('--login',): raise putils.ProcessExecutionError(None, None, 21) return mock.DEFAULT @@ -386,21 +425,6 @@ class ISCSIConnectorTestCase(test_connector.ConnectorTestCase): mock_iscsiadm.assert_any_call(props, ('--login',), check_exit_code=[0, 255]) - mock_iscsiadm.reset_mock() - with mock.patch.object(os.path, 'exists', - return_value=False): - self.connector.disconnect_volume(connection_info['data'], device) - props = connection_info['data'].copy() - for key in ('target_portals', 'target_iqns', 'target_luns'): - props.pop(key, None) - mock_iscsiadm.assert_any_call(props, ('--logout',), - check_exit_code=[0, 21, 255]) - props['target_portal'] = location2 - props['target_iqn'] = iqn2 - props['target_lun'] = 2 - mock_iscsiadm.assert_any_call(props, ('--logout',), - check_exit_code=[0, 21, 255]) - @mock.patch.object(linuxscsi.LinuxSCSI, 'get_scsi_wwn') @mock.patch.object(iscsi.ISCSIConnector, '_run_iscsiadm_bare') @mock.patch.object(iscsi.ISCSIConnector, @@ -528,42 +552,36 @@ class ISCSIConnectorTestCase(test_connector.ConnectorTestCase): @mock.patch.object(iscsi.ISCSIConnector, '_get_hosts_channels_targets_luns', return_value=[]) @mock.patch.object(linuxscsi.LinuxSCSI, 'get_scsi_wwn') - @mock.patch.object(os.path, 'exists', return_value=True) + @mock.patch('os.path.exists', side_effect=(True,) * 7 + (False, False)) @mock.patch.object(host_driver.HostDriver, 'get_all_block_devices') - @mock.patch.object(iscsi.ISCSIConnector, '_get_multipath_device_map', - return_value={}) - @mock.patch.object(iscsi.ISCSIConnector, '_get_iscsi_devices') @mock.patch.object(iscsi.ISCSIConnector, '_run_multipath') - @mock.patch.object(iscsi.ISCSIConnector, '_get_multipath_iqns') @mock.patch.object(base.BaseLinuxConnector, '_discover_mpath_device') @mock.patch.object(linuxscsi.LinuxSCSI, 'process_lun_id') def test_connect_volume_with_multiple_portals( self, mock_process_lun_id, mock_discover_mpath_device, - mock_get_iqn, mock_run_multipath, mock_iscsi_devices, - mock_get_device_map, mock_devices, mock_exists, mock_scsi_wwn, + mock_run_multipath, mock_devices, mock_exists, mock_scsi_wwn, mock_get_htcls): mock_scsi_wwn.return_value = test_connector.FAKE_SCSI_WWN - location1 = '10.0.2.15:3260' location2 = '[2001:db8::1]:3260' dev_loc2 = '2001:db8::1:3260' # udev location2 - name1 = 'volume-00000001-1' name2 = 'volume-00000001-2' - iqn1 = 'iqn.2010-10.org.openstack:%s' % name1 iqn2 = 'iqn.2010-10.org.openstack:%s' % name2 - lun1 = 1 lun2 = 2 + fake_multipath_dev = '/dev/mapper/fake-multipath-dev' - vol = {'id': 1, 'name': name1} + vol = {'id': 1, 'name': self._name} connection_properties = self.iscsi_connection_multipath( - vol, [location1, location2], [iqn1, iqn2], [lun1, lun2]) - devs = ['/dev/disk/by-path/ip-%s-iscsi-%s-lun-1' % (location1, iqn1), + vol, [self._location, location2], [self._iqn, iqn2], [self._lun, + lun2]) + devs = ['/dev/disk/by-path/ip-%s-iscsi-%s-lun-1' % (self._location, + self._iqn), '/dev/disk/by-path/ip-%s-iscsi-%s-lun-2' % (dev_loc2, iqn2)] mock_devices.return_value = devs - mock_iscsi_devices.return_value = devs - mock_get_iqn.return_value = [iqn1, iqn2] + # mock_iscsi_devices.return_value = devs + # mock_get_iqn.return_value = [self._iqn, iqn2] mock_discover_mpath_device.return_value = ( fake_multipath_dev, test_connector.FAKE_SCSI_WWN) - mock_process_lun_id.return_value = [lun1, lun2] + mock_process_lun_id.return_value = [self._lun, lun2] result = self.connector_with_multipath.connect_volume( connection_properties['data']) @@ -571,21 +589,14 @@ class ISCSIConnectorTestCase(test_connector.ConnectorTestCase): 'path': fake_multipath_dev, 'type': 'block', 'scsi_wwn': test_connector.FAKE_SCSI_WWN} cmd_format = 'iscsiadm -m node -T %s -p %s --%s' - expected_commands = [cmd_format % (iqn1, location1, 'login'), + expected_commands = [cmd_format % (self._iqn, self._location, 'login'), cmd_format % (iqn2, location2, 'login')] self.assertEqual(expected_result, result) for command in expected_commands: self.assertIn(command, self.cmds) - self.cmds = [] - self.connector_with_multipath.disconnect_volume( - connection_properties['data'], result) - expected_commands = [cmd_format % (iqn1, location1, 'logout'), - cmd_format % (iqn2, location2, 'logout')] - for command in expected_commands: - self.assertIn(command, self.cmds) - - mock_get_htcls.assert_called_once_with([(location1, iqn1, lun1), + mock_get_htcls.assert_called_once_with([(self._location, self._iqn, + self._lun), (location2, iqn2, lun2)]) @mock.patch.object(iscsi.ISCSIConnector, @@ -593,19 +604,14 @@ class ISCSIConnectorTestCase(test_connector.ConnectorTestCase): @mock.patch.object(linuxscsi.LinuxSCSI, 'get_scsi_wwn') @mock.patch.object(os.path, 'exists') @mock.patch.object(host_driver.HostDriver, 'get_all_block_devices') - @mock.patch.object(iscsi.ISCSIConnector, '_get_multipath_device_map', - return_value={}) - @mock.patch.object(iscsi.ISCSIConnector, '_get_iscsi_devices') @mock.patch.object(iscsi.ISCSIConnector, '_run_multipath') - @mock.patch.object(iscsi.ISCSIConnector, '_get_multipath_iqns') @mock.patch.object(iscsi.ISCSIConnector, '_run_iscsiadm') @mock.patch.object(base.BaseLinuxConnector, '_discover_mpath_device') @mock.patch.object(linuxscsi.LinuxSCSI, 'process_lun_id') def test_connect_volume_with_multiple_portals_primary_error( self, mock_process_lun_id, mock_discover_mpath_device, - mock_iscsiadm, mock_get_iqn, mock_run_multipath, - mock_iscsi_devices, mock_get_multipath_device_map, - mock_devices, mock_exists, mock_scsi_wwn, mock_get_htcls): + mock_iscsiadm, mock_run_multipath, mock_devices, mock_exists, + mock_scsi_wwn, mock_get_htcls): mock_scsi_wwn.return_value = test_connector.FAKE_SCSI_WWN location1 = '10.0.2.15:3260' location2 = '[2001:db8::1]:3260' @@ -629,8 +635,6 @@ class ISCSIConnectorTestCase(test_connector.ConnectorTestCase): mock_exists.side_effect = lambda x: x != dev1 mock_devices.return_value = [dev2] - mock_iscsi_devices.return_value = [dev2] - mock_get_iqn.return_value = [iqn2] mock_iscsiadm.side_effect = fake_run_iscsiadm mock_discover_mpath_device.return_value = ( @@ -654,20 +658,6 @@ class ISCSIConnectorTestCase(test_connector.ConnectorTestCase): mock_iscsiadm.assert_any_call(props, ('--login',), check_exit_code=[0, 255]) - mock_iscsiadm.reset_mock() - self.connector_with_multipath.disconnect_volume( - connection_properties['data'], result) - - props = connection_properties['data'].copy() - props['target_portal'] = location1 - props['target_iqn'] = iqn1 - mock_iscsiadm.assert_any_call(props, ('--logout',), - check_exit_code=[0, 21, 255]) - props['target_portal'] = location2 - props['target_iqn'] = iqn2 - mock_iscsiadm.assert_any_call(props, ('--logout',), - check_exit_code=[0, 21, 255]) - lun1, lun2 = connection_properties['data']['target_luns'] mock_get_htcls.assert_called_once_with([(location1, iqn1, lun1), (location2, iqn2, lun2)]) @@ -680,12 +670,11 @@ class ISCSIConnectorTestCase(test_connector.ConnectorTestCase): '_get_target_portals_from_iscsiadm_output') @mock.patch.object(iscsi.ISCSIConnector, '_connect_to_iscsi_portal') @mock.patch.object(host_driver.HostDriver, 'get_all_block_devices') - @mock.patch.object(iscsi.ISCSIConnector, '_get_iscsi_devices') @mock.patch.object(iscsi.ISCSIConnector, '_run_multipath') @mock.patch.object(base.BaseLinuxConnector, '_discover_mpath_device') def test_connect_volume_with_multipath_connecting( self, mock_discover_mpath_device, mock_run_multipath, - mock_iscsi_devices, mock_devices, + mock_devices, mock_connect, mock_portals, mock_exists, mock_scsi_wwn, mock_get_htcls): mock_scsi_wwn.return_value = test_connector.FAKE_SCSI_WWN @@ -702,7 +691,6 @@ class ISCSIConnectorTestCase(test_connector.ConnectorTestCase): devs = ['/dev/disk/by-path/ip-%s-iscsi-%s-lun-1' % (location1, iqn1), '/dev/disk/by-path/ip-%s-iscsi-%s-lun-2' % (dev_loc2, iqn2)] mock_devices.return_value = devs - mock_iscsi_devices.return_value = devs mock_portals.return_value = ([location1, location2, location2], [iqn1, iqn1, iqn2]) mock_discover_mpath_device.return_value = ( @@ -721,7 +709,7 @@ class ISCSIConnectorTestCase(test_connector.ConnectorTestCase): expected_calls = [mock.call(props1), mock.call(props2)] self.assertEqual(expected_result, result) mock_connect.assert_has_calls(expected_calls, any_order=True) - self.assertEqual(expected_calls, mock_connect.call_args_list) + self.assertEqual(len(expected_calls), mock_connect.call_count) lun = connection_properties['data']['target_lun'] self.assertEqual(1, mock_get_htcls.call_count) # Order of elements in the list is randomized because it comes from @@ -735,12 +723,10 @@ class ISCSIConnectorTestCase(test_connector.ConnectorTestCase): '_get_target_portals_from_iscsiadm_output') @mock.patch.object(iscsi.ISCSIConnector, '_connect_to_iscsi_portal') @mock.patch.object(host_driver.HostDriver, 'get_all_block_devices') - @mock.patch.object(iscsi.ISCSIConnector, '_get_iscsi_devices') @mock.patch.object(iscsi.ISCSIConnector, '_run_multipath') def test_connect_volume_multipath_failed_iscsi_login( - self, mock_run_multipath, - mock_iscsi_devices, mock_devices, - mock_connect, mock_portals, mock_exists): + self, mock_run_multipath, mock_devices, mock_connect, mock_portals, + mock_exists): location1 = '10.0.2.15:3260' location2 = '10.0.3.15:3260' name1 = 'volume-00000001-1' @@ -752,7 +738,6 @@ class ISCSIConnectorTestCase(test_connector.ConnectorTestCase): devs = ['/dev/disk/by-path/ip-%s-iscsi-%s-lun-1' % (location1, iqn1), '/dev/disk/by-path/ip-%s-iscsi-%s-lun-2' % (location2, iqn2)] mock_devices.return_value = devs - mock_iscsi_devices.return_value = devs mock_portals.return_value = ([location1, location2, location2], [iqn1, iqn1, iqn2]) @@ -798,168 +783,119 @@ class ISCSIConnectorTestCase(test_connector.ConnectorTestCase): expected = (ips, iqns) self.assertEqual(expected, res) - @mock.patch.object(os, 'walk') - def test_get_iscsi_devices(self, walk_mock): - paths = [('ip-10.0.0.1:3260-iscsi-iqn.2013-01.ro.' - 'com.netapp:node.netapp02-lun-0')] - walk_mock.return_value = [(['.'], ['by-path'], paths)] - self.assertEqual(self.connector._get_iscsi_devices(), paths) + @mock.patch.object(iscsi.ISCSIConnector, '_disconnect_connection') + @mock.patch.object(iscsi.ISCSIConnector, '_get_connection_devices') + @mock.patch.object(linuxscsi.LinuxSCSI, 'flush_multipath_device') + @mock.patch.object(linuxscsi.LinuxSCSI, 'remove_connection', + return_value=None) + def test_disconnect_volume(self, remove_mock, flush_mock, con_devs_mock, + discon_mock): + # Return an ordered dicts instead of normal dict for discon_mock.assert + con_devs_mock.return_value = collections.OrderedDict(( + (('ip1:port1', 'tgt1'), ({'sda'}, set())), + (('ip2:port2', 'tgt2'), ({'sdb'}, {'sdc'})), + (('ip3:port3', 'tgt3'), (set(), set())))) - @mock.patch.object(os, 'walk', return_value=[]) - def test_get_iscsi_devices_with_empty_dir(self, walk_mock): - self.assertEqual(self.connector._get_iscsi_devices(), []) + with mock.patch.object(self.connector, + 'use_multipath') as use_mp_mock: + self.connector.disconnect_volume(self.CON_PROPS, + mock.sentinel.dev_info) - @mock.patch.object(os.path, 'realpath') - @mock.patch.object(iscsi.ISCSIConnector, '_get_iscsi_devices') - def test_get_multipath_iqns(self, get_iscsi_mock, realpath_mock): - paths = [('ip-10.0.0.1:3260-iscsi-iqn.2013-01.ro.' - 'com.netapp:node.netapp02-lun-0')] - devpath = '/dev/disk/by-path/%s' % paths[0] - realpath_mock.return_value = devpath - get_iscsi_mock.return_value = paths - mpath_map = {devpath: paths[0]} - self.assertEqual(self.connector._get_multipath_iqns([paths[0]], - mpath_map), - ['iqn.2013-01.ro.com.netapp:node.netapp02']) + con_devs_mock.assert_called_once_with(self.CON_PROPS) + remove_mock.assert_called_once_with({'sda', 'sdb'}, use_mp_mock, + False, mock.ANY) + discon_mock.assert_called_once_with( + self.CON_PROPS, + [('ip1:port1', 'tgt1'), ('ip3:port3', 'tgt3')], + False, mock.ANY) + flush_mock.assert_not_called() - @mock.patch.object(iscsi.ISCSIConnector, '_run_multipath') - def test_get_multipath_device_map(self, multipath_mock): - multipath_mock.return_value = [ - "Mar 17 14:32:37 | sda: No fc_host device for 'host-1'\n" - "mpathb (36e00000000010001) dm-4 IET ,VIRTUAL-DISK\n" - "size=1.0G features='0' hwhandler='0' wp=rw\n" - "|-+- policy='service-time 0' prio=0 status=active\n" - "| `- 2:0:0:1 sda 8:0 active undef running\n" - "`-+- policy='service-time 0' prio=0 status=enabled\n" - " `- 3:0:0:1 sdb 8:16 active undef running\n"] - expected = {'/dev/sda': '/dev/mapper/mpathb', - '/dev/sdb': '/dev/mapper/mpathb'} - self.assertEqual(expected, self.connector._get_multipath_device_map()) + @mock.patch('os_brick.exception.ExceptionChainer.__nonzero__', + mock.Mock(return_value=True)) + @mock.patch('os_brick.exception.ExceptionChainer.__bool__', + mock.Mock(return_value=True)) + @mock.patch.object(iscsi.ISCSIConnector, '_disconnect_connection') + @mock.patch.object(iscsi.ISCSIConnector, '_get_connection_devices') + @mock.patch.object(linuxscsi.LinuxSCSI, 'flush_multipath_device') + @mock.patch.object(linuxscsi.LinuxSCSI, 'remove_connection', + return_value=mock.sentinel.mp_name) + def test_disconnect_volume_force_failure(self, remove_mock, flush_mock, + con_devs_mock, discon_mock): + # Return an ordered dicts instead of normal dict for discon_mock.assert + con_devs_mock.return_value = collections.OrderedDict(( + (('ip1:port1', 'tgt1'), ({'sda'}, set())), + (('ip2:port2', 'tgt2'), ({'sdb'}, {'sdc'})), + (('ip3:port3', 'tgt3'), (set(), set())))) - @mock.patch.object(iscsi.ISCSIConnector, '_get_multipath_device_map') - @mock.patch.object(iscsi.ISCSIConnector, - '_get_target_portals_from_iscsiadm_output') - @mock.patch.object(iscsi.ISCSIConnector, '_rescan_iscsi') - @mock.patch.object(iscsi.ISCSIConnector, '_get_iscsi_devices') - @mock.patch.object(host_driver.HostDriver, 'get_all_block_devices') - @mock.patch.object(iscsi.ISCSIConnector, - '_disconnect_from_iscsi_portal') - @mock.patch.object(iscsi.ISCSIConnector, '_get_multipath_iqns') - @mock.patch.object(os.path, 'exists', return_value=True) - def test_disconnect_volume_multipath_iscsi( - self, exists_mock, multipath_iqn_mock, disconnect_mock, - get_all_devices_mock, get_iscsi_devices_mock, - rescan_iscsi_mock, get_portals_mock, - get_multipath_device_map_mock): - iqn1 = 'iqn.2013-01.ro.com.netapp:node.netapp01' - iqn2 = 'iqn.2013-01.ro.com.netapp:node.netapp02' - iqns = [iqn1, iqn2] - portal = '10.0.0.1:3260' - dev = ('ip-%s-iscsi-%s-lun-0' % (portal, iqn1)) + with mock.patch.object(self.connector, 'use_multipath', + wraps=True) as use_mp_mock: + self.assertRaises(exception.ExceptionChainer, + self.connector.disconnect_volume, + self.CON_PROPS, mock.sentinel.dev_info, + mock.sentinel.force, ignore_errors=False) - get_portals_mock.return_value = ([portal], [iqn1]) - multipath_iqn_mock.return_value = iqns - get_all_devices_mock.return_value = [dev, '/dev/mapper/md-1'] - get_multipath_device_map_mock.return_value = {dev: '/dev/mapper/md-3'} - get_iscsi_devices_mock.return_value = [] - fake_property = {'target_portal': portal, - 'target_iqn': iqn1} - self.connector._disconnect_volume_multipath_iscsi(fake_property, - 'fake/multipath') - # Target in use by other mp devices, don't disconnect - self.assertFalse(disconnect_mock.called) + con_devs_mock.assert_called_once_with(self.CON_PROPS) + remove_mock.assert_called_once_with({'sda', 'sdb'}, use_mp_mock, + mock.sentinel.force, mock.ANY) + discon_mock.assert_called_once_with( + self.CON_PROPS, + [('ip1:port1', 'tgt1'), ('ip3:port3', 'tgt3')], + mock.sentinel.force, mock.ANY) + flush_mock.assert_called_once_with(mock.sentinel.mp_name) - @mock.patch.object(iscsi.ISCSIConnector, - '_get_target_portals_from_iscsiadm_output') - @mock.patch.object(iscsi.ISCSIConnector, '_rescan_iscsi') - @mock.patch.object(iscsi.ISCSIConnector, '_get_iscsi_devices') - @mock.patch.object(host_driver.HostDriver, 'get_all_block_devices') - @mock.patch.object(iscsi.ISCSIConnector, - '_disconnect_from_iscsi_portal') - @mock.patch.object(iscsi.ISCSIConnector, '_get_multipath_device_map') - @mock.patch.object(iscsi.ISCSIConnector, '_get_multipath_iqns') - @mock.patch.object(os.path, 'exists', return_value=True) - def test_disconnect_volume_multipath_iscsi_other_targets( - self, exists_mock, multipath_iqn_mock, get_multipath_map_mock, - disconnect_mock, get_all_devices_mock, get_iscsi_devices_mock, - rescan_iscsi_mock, get_portals_mock): - iqn1 = 'iqn.2010-10.org.openstack:target-1' - iqn2 = 'iqn.2010-10.org.openstack:target-2' - portal = '10.0.0.1:3260' - dev2 = ('ip-%s-iscsi-%s-lun-0' % (portal, iqn2)) + @ddt.data({'do_raise': False, 'force': False}, + {'do_raise': True, 'force': True}, + {'do_raise': True, 'force': False}) + @ddt.unpack + @mock.patch.object(iscsi.ISCSIConnector, '_disconnect_from_iscsi_portal') + def test_disconnect_connection(self, disconnect_mock, do_raise, force): + will_raise = do_raise and not force + actual_call_args = [] - # Multiple targets are discovered, but only block devices for target-1 - # is deleted and target-2 is in use. - get_portals_mock.return_value = ([portal, portal], [iqn1, iqn2]) - multipath_iqn_mock.return_value = [iqn2, iqn2] - get_all_devices_mock.return_value = [dev2, '/dev/mapper/md-1'] - get_multipath_map_mock.return_value = {dev2: '/dev/mapper/md-3'} - get_iscsi_devices_mock.return_value = [dev2] - fake_property = {'target_portal': portal, - 'target_iqn': iqn1} - self.connector._disconnect_volume_multipath_iscsi(fake_property, - 'fake/multipath') - # Only target-1 should be disconneced. - disconnect_mock.assert_called_once_with(fake_property) + # Since we reuse the copied dictionary on _disconnect_connection + # changing its values we cannot use mock's assert_has_calls + def my_disconnect(con_props): + actual_call_args.append(con_props.copy()) + if do_raise: + raise exception.ExceptionChainer() - @mock.patch.object(iscsi.ISCSIConnector, '_get_multipath_device_map', - return_value={}) - @mock.patch.object(iscsi.ISCSIConnector, - '_get_target_portals_from_iscsiadm_output') - @mock.patch.object(iscsi.ISCSIConnector, '_rescan_iscsi') - @mock.patch.object(iscsi.ISCSIConnector, '_get_iscsi_devices', - return_value=[]) - @mock.patch.object(host_driver.HostDriver, 'get_all_block_devices', - return_value=[]) - @mock.patch.object(iscsi.ISCSIConnector, - '_disconnect_from_iscsi_portal') - @mock.patch.object(os.path, 'exists', return_value=True) - def test_disconnect_volume_multipath_iscsi_without_other_mp_devices( - self, exists_mock, disconnect_mock, get_all_devices_mock, - get_iscsi_devices_mock, rescan_iscsi_mock, - get_portals_mock, get_multipath_device_map_mock): - portal = '10.0.2.15:3260' - name = 'volume-00000001' - iqn = 'iqn.2010-10.org.openstack:%s' % name + disconnect_mock.side_effect = my_disconnect - get_portals_mock.return_value = ([portal], [iqn]) - fake_property = {'target_portal': portal, - 'target_iqn': iqn} - self.connector._disconnect_volume_multipath_iscsi(fake_property, - 'fake/multipath') - # Target not in use by other mp devices, disconnect - disconnect_mock.assert_called_once_with(fake_property) + connections = (('ip1:port1', 'tgt1'), ('ip2:port2', 'tgt2')) + original_props = self.CON_PROPS.copy() + exc = exception.ExceptionChainer() + if will_raise: + self.assertRaises(exception.ExceptionChainer, + self.connector._disconnect_connection, + self.CON_PROPS, connections, + force=force, exc=exc) + else: + self.connector._disconnect_connection(self.CON_PROPS, connections, + force=force, exc=exc) - @mock.patch.object(iscsi.ISCSIConnector, '_get_multipath_device_map', - return_value={}) - @mock.patch.object(iscsi.ISCSIConnector, - '_get_target_portals_from_iscsiadm_output') - @mock.patch.object(iscsi.ISCSIConnector, '_rescan_iscsi') - @mock.patch.object(iscsi.ISCSIConnector, '_get_iscsi_devices') - @mock.patch.object(host_driver.HostDriver, 'get_all_block_devices') - @mock.patch.object(iscsi.ISCSIConnector, - '_disconnect_from_iscsi_portal') - @mock.patch.object(os.path, 'exists', return_value=False) - def test_disconnect_volume_multipath_iscsi_with_invalid_symlink( - self, exists_mock, disconnect_mock, get_all_devices_mock, - get_iscsi_devices_mock, rescan_iscsi_mock, - get_portals_mock, get_multipath_device_map_mock): - # Simulate a broken symlink by returning False for os.path.exists(dev) - portal = '10.0.0.1:3260' - name = 'volume-00000001' - iqn = 'iqn.2010-10.org.openstack:%s' % name - dev = ('ip-%s-iscsi-%s-lun-0' % (portal, iqn)) + # Passed properties should not be altered by the method call + self.assertDictEqual(original_props, self.CON_PROPS) + expected = [original_props.copy(), original_props.copy()] + for i, (ip, iqn) in enumerate(connections): + expected[i].update(target_portal=ip, target_iqn=iqn) + # If we are failing and not forcing we won't make all the alls + if will_raise: + expected = expected[:1] + self.assertListEqual(expected, actual_call_args) + # No exceptions have been caught by ExceptionChainer context manager + self.assertEqual(do_raise, bool(exc)) - get_portals_mock.return_value = ([portal], [iqn]) - get_all_devices_mock.return_value = [dev, '/dev/mapper/md-1'] - get_iscsi_devices_mock.return_value = [] - - fake_property = {'target_portal': portal, - 'target_iqn': iqn} - self.connector._disconnect_volume_multipath_iscsi(fake_property, - 'fake/multipath') - # Target not in use by other mp devices, disconnect - disconnect_mock.assert_called_once_with(fake_property) + def test_disconnect_from_iscsi_portal(self): + self.connector._disconnect_from_iscsi_portal(self.CON_PROPS) + expected_prefix = ('iscsiadm -m node -T %s -p %s ' % + (self.CON_PROPS['target_iqn'], + self.CON_PROPS['target_portal'])) + expected = [ + expected_prefix + '--op update -n node.startup -v manual', + expected_prefix + '--logout', + expected_prefix + '--op delete', + ] + self.assertListEqual(expected, self.cmds) def test_iscsiadm_discover_parsing(self): # Ensure that parsing iscsiadm discover ignores cruft. diff --git a/os_brick/tests/initiator/test_linuxscsi.py b/os_brick/tests/initiator/test_linuxscsi.py index 53f9dbf13..8ed657cfe 100644 --- a/os_brick/tests/initiator/test_linuxscsi.py +++ b/os_brick/tests/initiator/test_linuxscsi.py @@ -17,8 +17,8 @@ import os.path import textwrap import time +import ddt import mock -from oslo_concurrency import processutils as putils from oslo_log import log as logging from os_brick import exception @@ -28,6 +28,7 @@ from os_brick.tests import base LOG = logging.getLogger(__name__) +@ddt.ddt class LinuxSCSITestCase(base.TestCase): def setUp(self): super(LinuxSCSITestCase, self).setUp() @@ -73,51 +74,51 @@ class LinuxSCSITestCase(base.TestCase): ('tee -a /sys/block/sdc/device/delete')] self.assertEqual(expected_commands, self.cmds) - @mock.patch('time.sleep') - def test_wait_for_volume_removal(self, sleep_mock): - fake_path = '/dev/disk/by-path/fake-iscsi-iqn-lun-0' - exists_mock = mock.Mock() - exists_mock.return_value = True - os.path.exists = exists_mock - self.assertRaises(exception.VolumePathNotRemoved, - self.linuxscsi.wait_for_volume_removal, - fake_path) + @mock.patch.object(linuxscsi.LinuxSCSI, 'echo_scsi_command') + @mock.patch.object(linuxscsi.LinuxSCSI, 'flush_device_io') + @mock.patch.object(os.path, 'exists', return_value=True) + def test_remove_scsi_device_force(self, exists_mock, flush_mock, + echo_mock): + """With force we'll always call delete even if flush fails.""" + exc = exception.ExceptionChainer() + flush_mock.side_effect = Exception() + echo_mock.side_effect = Exception() + device = '/dev/sdc' - exists_mock = mock.Mock() - exists_mock.return_value = False - os.path.exists = exists_mock - self.linuxscsi.wait_for_volume_removal(fake_path) - expected_commands = [] - self.assertEqual(expected_commands, self.cmds) - self.assertTrue(sleep_mock.called) + self.linuxscsi.remove_scsi_device(device, force=True, exc=exc) + # The context manager has caught the exceptions + self.assertTrue(exc) + flush_mock.assert_called_once_with(device) + echo_mock.assert_called_once_with('/sys/block/sdc/device/delete', '1') + + @mock.patch('time.sleep') + @mock.patch('os.path.exists', return_value=True) + def test_wait_for_volumes_removal_failure(self, exists_mock, sleep_mock): + retries = 3 + names = ('sda', 'sdb') + self.assertRaises(exception.VolumePathNotRemoved, + self.linuxscsi.wait_for_volumes_removal, names) + exists_mock.assert_has_calls([mock.call('/dev/' + name) + for name in names] * retries) + self.assertEqual(retries - 1, sleep_mock.call_count) + + @mock.patch('time.sleep') + @mock.patch('os.path.exists', side_effect=(True, True, False, False)) + def test_wait_for_volumes_removal_retry(self, exists_mock, sleep_mock): + names = ('sda', 'sdb') + self.linuxscsi.wait_for_volumes_removal(names) + exists_mock.assert_has_calls([mock.call('/dev/' + name) + for name in names] * 2) + self.assertEqual(1, sleep_mock.call_count) def test_flush_multipath_device(self): - self.linuxscsi.flush_multipath_device('/dev/dm-9') - expected_commands = [('multipath -f /dev/dm-9')] - self.assertEqual(expected_commands, self.cmds) + dm_map_name = '3600d0230000000000e13955cc3757800' + with mock.patch.object(self.linuxscsi, '_execute') as exec_mock: + self.linuxscsi.flush_multipath_device(dm_map_name) - @mock.patch('retrying.time.sleep', mock.Mock()) - def test_flush_multipath_device_in_use(self): - side_effect = ( - putils.ProcessExecutionError( - stdout='Feb 09 14:38:02 | mpatha: map in use\n' - 'Feb 09 14:38:02 | failed to remove multipath map ' - 'mpatha\n', - exit_code=1), - ('', '') - ) - - with mock.patch.object(self.linuxscsi, '_execute') as execute_mock: - execute_mock.side_effect = side_effect - self.linuxscsi.flush_multipath_device('mpatha') - execute_mock.assert_has_calls( - [mock.call('multipath', '-f', 'mpatha', run_as_root=True, - root_helper=mock.ANY)] * 2) - - def test_flush_multipath_devices(self): - self.linuxscsi.flush_multipath_devices() - expected_commands = [('multipath -F')] - self.assertEqual(expected_commands, self.cmds) + exec_mock.assert_called_once_with( + 'multipath', '-f', dm_map_name, run_as_root=True, attempts=3, + timeout=300, interval=10, root_helper=self.linuxscsi._root_helper) def test_get_scsi_wwn(self): fake_path = '/dev/disk/by-id/somepath' @@ -130,6 +131,51 @@ class LinuxSCSITestCase(base.TestCase): wwn = self.linuxscsi.get_scsi_wwn(fake_path) self.assertEqual(fake_wwn, wwn) + @mock.patch('six.moves.builtins.open') + def test_get_dm_name(self, open_mock): + dm_map_name = '3600d0230000000000e13955cc3757800' + cm_open = open_mock.return_value.__enter__.return_value + cm_open.read.return_value = dm_map_name + res = self.linuxscsi.get_dm_name('dm-0') + self.assertEqual(dm_map_name, res) + open_mock.assert_called_once_with('/sys/block/dm-0/dm/name') + + @mock.patch('six.moves.builtins.open', side_effect=IOError) + def test_get_dm_name_failure(self, open_mock): + self.assertEqual('', self.linuxscsi.get_dm_name('dm-0')) + + @mock.patch('glob.glob', side_effect=[[], ['/sys/block/sda/holders/dm-9']]) + def test_find_sysfs_multipath_dm(self, glob_mock): + device_names = ('sda', 'sdb') + res = self.linuxscsi.find_sysfs_multipath_dm(device_names) + self.assertEqual('dm-9', res) + glob_mock.assert_has_calls([mock.call('/sys/block/sda/holders/dm-*'), + mock.call('/sys/block/sdb/holders/dm-*')]) + + @mock.patch('glob.glob', return_value=[]) + def test_find_sysfs_multipath_dm_not_found(self, glob_mock): + device_names = ('sda', 'sdb') + res = self.linuxscsi.find_sysfs_multipath_dm(device_names) + self.assertIsNone(res) + glob_mock.assert_has_calls([mock.call('/sys/block/sda/holders/dm-*'), + mock.call('/sys/block/sdb/holders/dm-*')]) + + @mock.patch.object(linuxscsi.LinuxSCSI, '_execute') + @mock.patch('os.path.exists', return_value=True) + def test_flush_device_io(self, exists_mock, exec_mock): + device = '/dev/sda' + self.linuxscsi.flush_device_io(device) + exists_mock.assert_called_once_with(device) + exec_mock.assert_called_once_with( + 'blockdev', '--flushbufs', device, run_as_root=True, attempts=3, + timeout=300, interval=10, root_helper=self.linuxscsi._root_helper) + + @mock.patch('os.path.exists', return_value=False) + def test_flush_device_io_non_existent(self, exists_mock): + device = '/dev/sda' + self.linuxscsi.flush_device_io(device) + exists_mock.assert_called_once_with(device) + @mock.patch.object(os.path, 'exists', return_value=True) def test_find_multipath_device_path(self, exists_mock): fake_wwn = '1234567890' @@ -168,31 +214,73 @@ class LinuxSCSITestCase(base.TestCase): self.linuxscsi.wait_for_path, path) - @mock.patch.object(linuxscsi.LinuxSCSI, 'find_multipath_device') - @mock.patch.object(os.path, 'exists', return_value=True) - def test_remove_multipath_device(self, exists_mock, mock_multipath): - def fake_find_multipath_device(device): - devices = [{'device': '/dev/sde', 'host': 0, - 'channel': 0, 'id': 0, 'lun': 1}, - {'device': '/dev/sdf', 'host': 2, - 'channel': 0, 'id': 0, 'lun': 1}, ] + @ddt.data({'do_raise': False, 'force': False}, + {'do_raise': True, 'force': True}) + @ddt.unpack + @mock.patch.object(linuxscsi.LinuxSCSI, 'flush_multipath_device') + @mock.patch.object(linuxscsi.LinuxSCSI, 'get_dm_name') + @mock.patch.object(linuxscsi.LinuxSCSI, 'find_sysfs_multipath_dm') + @mock.patch.object(linuxscsi.LinuxSCSI, 'wait_for_volumes_removal') + @mock.patch('os_brick.initiator.linuxscsi.LinuxSCSI.remove_scsi_device') + def test_remove_connection_multipath_complete(self, remove_mock, wait_mock, + find_dm_mock, + get_dm_name_mock, + flush_mp_mock, + do_raise, force): + if do_raise: + flush_mp_mock.side_effect = Exception + devices_names = ('sda', 'sdb') + exc = exception.ExceptionChainer() + mp_name = self.linuxscsi.remove_connection(devices_names, + is_multipath=True, + force=mock.sentinel.Force, + exc=exc) + find_dm_mock.assert_called_once_with(devices_names) + get_dm_name_mock.assert_called_once_with(find_dm_mock.return_value) + flush_mp_mock.assert_called_once_with(get_dm_name_mock.return_value) + self.assertEqual(get_dm_name_mock.return_value if do_raise else None, + mp_name) + remove_mock.assert_has_calls([ + mock.call('/dev/sda', mock.sentinel.Force, exc), + mock.call('/dev/sdb', mock.sentinel.Force, exc)]) + wait_mock.assert_called_once_with(devices_names) + self.assertEqual(do_raise, bool(exc)) - info = {"device": "dm-3", - "id": "350002ac20398383d", - "name": "mpatha", - "devices": devices} - return info + @mock.patch.object(linuxscsi.LinuxSCSI, 'flush_multipath_device', + side_effect=Exception) + @mock.patch.object(linuxscsi.LinuxSCSI, 'get_dm_name') + @mock.patch.object(linuxscsi.LinuxSCSI, 'find_sysfs_multipath_dm') + @mock.patch.object(linuxscsi.LinuxSCSI, 'wait_for_volumes_removal') + @mock.patch.object(linuxscsi.LinuxSCSI, 'remove_scsi_device') + def test_remove_connection_multipath_fail(self, remove_mock, wait_mock, + find_dm_mock, get_dm_name_mock, + flush_mp_mock): + flush_mp_mock.side_effect = exception.ExceptionChainer + devices_names = ('sda', 'sdb') + exc = exception.ExceptionChainer() + self.assertRaises(exception.ExceptionChainer, + self.linuxscsi.remove_connection, + devices_names, is_multipath=True, + force=False, exc=exc) + find_dm_mock.assert_called_once_with(devices_names) + get_dm_name_mock.assert_called_once_with(find_dm_mock.return_value) + flush_mp_mock.assert_called_once_with(get_dm_name_mock.return_value) + remove_mock.assert_not_called() + wait_mock.assert_not_called() + self.assertTrue(bool(exc)) - mock_multipath.side_effect = fake_find_multipath_device - - self.linuxscsi.remove_multipath_device('/dev/dm-3') - expected_commands = [ - ('multipath -f mpatha'), - ('blockdev --flushbufs /dev/sde'), - ('tee -a /sys/block/sde/device/delete'), - ('blockdev --flushbufs /dev/sdf'), - ('tee -a /sys/block/sdf/device/delete'), ] - self.assertEqual(expected_commands, self.cmds) + @mock.patch.object(linuxscsi.LinuxSCSI, 'wait_for_volumes_removal') + @mock.patch.object(linuxscsi.LinuxSCSI, 'remove_scsi_device') + def test_remove_connection_singlepath(self, remove_mock, wait_mock): + devices_names = ('sda', 'sdb') + exc = exception.ExceptionChainer() + self.linuxscsi.remove_connection(devices_names, is_multipath=False, + force=mock.sentinel.Force, + exc=exc) + remove_mock.assert_has_calls( + [mock.call('/dev/sda', mock.sentinel.Force, exc), + mock.call('/dev/sdb', mock.sentinel.Force, exc)]) + wait_mock.assert_called_once_with(devices_names) def test_find_multipath_device_3par_ufn(self): def fake_execute(*cmd, **kwargs): diff --git a/os_brick/tests/privileged/test_rootwrap.py b/os_brick/tests/privileged/test_rootwrap.py index a5997b500..4f59fda74 100644 --- a/os_brick/tests/privileged/test_rootwrap.py +++ b/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) diff --git a/os_brick/tests/windows/fake_win_conn.py b/os_brick/tests/windows/fake_win_conn.py index f12887c14..fa5655b1d 100644 --- a/os_brick/tests/windows/fake_win_conn.py +++ b/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): diff --git a/releasenotes/notes/refactor_iscsi_disconnect-557f4173bc1ae4ed.yaml b/releasenotes/notes/refactor_iscsi_disconnect-557f4173bc1ae4ed.yaml new file mode 100644 index 000000000..4edc5fc3d --- /dev/null +++ b/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.