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 be0c2f986..5f811e5de 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.