diff --git a/os_brick/initiator/connectors/fibre_channel.py b/os_brick/initiator/connectors/fibre_channel.py index 36aef3dad..8e8533837 100644 --- a/os_brick/initiator/connectors/fibre_channel.py +++ b/os_brick/initiator/connectors/fibre_channel.py @@ -169,6 +169,7 @@ class FibreChannelConnector(base.BaseLinuxConnector): @utils.trace @synchronized('extend_volume', external=True) + @utils.connect_volume_undo_prepare_result def extend_volume(self, connection_properties): """Update the local kernel's size information. @@ -189,6 +190,7 @@ class FibreChannelConnector(base.BaseLinuxConnector): raise exception.VolumePathsNotFound() @utils.trace + @utils.connect_volume_prepare_result @synchronized('connect_volume', external=True) def connect_volume(self, connection_properties): """Attach the volume to instance_name. @@ -315,6 +317,7 @@ class FibreChannelConnector(base.BaseLinuxConnector): @utils.trace @synchronized('connect_volume', external=True) + @utils.connect_volume_undo_prepare_result(unlink_after=True) def disconnect_volume(self, connection_properties, device_info, force=False, ignore_errors=False): """Detach the volume from instance_name. @@ -356,8 +359,7 @@ class FibreChannelConnector(base.BaseLinuxConnector): # There may have been more than 1 device mounted # by the kernel for this volume. We have to remove # all of them - path_used = self._linuxscsi.get_dev_path(connection_properties, - device_info) + path_used = utils.get_dev_path(connection_properties, device_info) # NOTE: Due to bug #1897787 device_info may have a real path for some # single paths instead of a symlink as it should have, so it'll only # be a multipath if it was a symlink (not real path) and it wasn't a diff --git a/os_brick/initiator/connectors/iscsi.py b/os_brick/initiator/connectors/iscsi.py index bcdfe44ba..41af074c1 100644 --- a/os_brick/initiator/connectors/iscsi.py +++ b/os_brick/initiator/connectors/iscsi.py @@ -475,6 +475,7 @@ class ISCSIConnector(base.BaseLinuxConnector, base_iscsi.BaseISCSIConnector): @utils.trace @synchronized('extend_volume', external=True) + @utils.connect_volume_undo_prepare_result def extend_volume(self, connection_properties: dict): """Update the local kernel's size information. @@ -497,6 +498,7 @@ class ISCSIConnector(base.BaseLinuxConnector, base_iscsi.BaseISCSIConnector): raise exception.VolumePathsNotFound() @utils.trace + @utils.connect_volume_prepare_result @synchronized('connect_volume', external=True) def connect_volume(self, connection_properties: dict): """Attach the volume to instance_name. @@ -523,41 +525,8 @@ class ISCSIConnector(base.BaseLinuxConnector, base_iscsi.BaseISCSIConnector): with excutils.save_and_reraise_exception(): self._cleanup_connection(connection_properties, force=True) - @utils.retry((exception.VolumeDeviceNotFound)) - def _get_device_link(self, wwn, device, mpath): - # These are the default symlinks that should always be there - if mpath: - symlink = '/dev/disk/by-id/dm-uuid-mpath-' + mpath - else: - symlink = '/dev/disk/by-id/scsi-' + wwn - - # If default symlinks are not there just search for anything that links - # to our device. In my experience this will return the last added link - # first, so if we are going to succeed this should be fast. - if not os.path.realpath(symlink) == device: - links_path = '/dev/disk/by-id/' - for symlink in os.listdir(links_path): - symlink = links_path + symlink - if os.path.realpath(symlink) == device: - break - else: - # Raising this will trigger the next retry - raise exception.VolumeDeviceNotFound(device='/dev/disk/by-id') - return symlink - def _get_connect_result(self, con_props, wwn, devices_names, mpath=None): device = '/dev/' + (mpath or devices_names[0]) - - # NOTE(geguileo): This is only necessary because of the current - # encryption flow that requires that connect_volume returns a symlink - # because first we do the volume attach, then the libvirt config is - # generated using the path returned by the atach, and then we do the - # encryption attach, which is forced to preserve the path that was used - # in the libvirt config. If we fix that flow in OS-brick, Nova, and - # Cinder we can remove this and just return the real path. - if con_props.get('encrypted'): - device = self._get_device_link(wwn, device, mpath) - result = {'type': 'block', 'scsi_wwn': wwn, 'path': device} if mpath: result['multipath_id'] = wwn @@ -863,6 +832,7 @@ class ISCSIConnector(base.BaseLinuxConnector, base_iscsi.BaseISCSIConnector): @utils.trace @synchronized('connect_volume', external=True) + @utils.connect_volume_undo_prepare_result(unlink_after=True) def disconnect_volume(self, connection_properties, device_info, force=False, ignore_errors=False): """Detach the volume from instance_name. @@ -928,8 +898,7 @@ class ISCSIConnector(base.BaseLinuxConnector, base_iscsi.BaseISCSIConnector): for remove, __ in devices_map.values(): remove_devices.update(remove) - path_used = self._linuxscsi.get_dev_path(connection_properties, - device_info) + path_used = utils.get_dev_path(connection_properties, device_info) was_multipath = (path_used.startswith('/dev/dm-') or 'mpath' in path_used) multipath_name = self._linuxscsi.remove_connection( diff --git a/os_brick/initiator/connectors/lightos.py b/os_brick/initiator/connectors/lightos.py index 5b744fa11..0f5750174 100644 --- a/os_brick/initiator/connectors/lightos.py +++ b/os_brick/initiator/connectors/lightos.py @@ -256,6 +256,7 @@ class LightOSConnector(base.BaseLinuxConnector): return None @utils.trace + @utils.connect_volume_prepare_result @synchronized('volume_op') def connect_volume(self, connection_properties): """Discover and attach the volume. @@ -292,6 +293,7 @@ class LightOSConnector(base.BaseLinuxConnector): @utils.trace @synchronized('volume_op') + @utils.connect_volume_undo_prepare_result(unlink_after=True) def disconnect_volume(self, connection_properties, device_info, force=False, ignore_errors=False): """Disconnect a volume from the local host. @@ -334,6 +336,7 @@ class LightOSConnector(base.BaseLinuxConnector): @utils.trace @synchronized('volume_op') + @utils.connect_volume_undo_prepare_result def extend_volume(self, connection_properties): uuid = connection_properties['uuid'] new_size = self._get_size_by_uuid(uuid) diff --git a/os_brick/initiator/connectors/nvmeof.py b/os_brick/initiator/connectors/nvmeof.py index 4835ae1c4..cbc572529 100644 --- a/os_brick/initiator/connectors/nvmeof.py +++ b/os_brick/initiator/connectors/nvmeof.py @@ -307,6 +307,7 @@ class NVMeOFConnector(base.BaseLinuxConnector): return self._is_nvme_available(nvme_name) @utils.trace + @utils.connect_volume_prepare_result @synchronized('connect_volume', external=True) def connect_volume(self, connection_properties): """Discover and attach the volume. @@ -359,6 +360,7 @@ class NVMeOFConnector(base.BaseLinuxConnector): @utils.trace @synchronized('connect_volume', external=True) + @utils.connect_volume_undo_prepare_result(unlink_after=True) def disconnect_volume(self, connection_properties, device_info, force=False, ignore_errors=False): """Flush the volume. @@ -403,6 +405,7 @@ class NVMeOFConnector(base.BaseLinuxConnector): @utils.trace @synchronized('extend_volume', external=True) + @utils.connect_volume_undo_prepare_result def extend_volume(self, connection_properties): """Update the local kernel's size information. diff --git a/os_brick/initiator/connectors/rbd.py b/os_brick/initiator/connectors/rbd.py index 2fbec2e18..980987926 100644 --- a/os_brick/initiator/connectors/rbd.py +++ b/os_brick/initiator/connectors/rbd.py @@ -227,6 +227,7 @@ class RBDConnector(base_rbd.RBDConnectorMixin, base.BaseLinuxConnector): return res @utils.trace + @utils.connect_volume_prepare_result def connect_volume(self, connection_properties): """Connect to a volume. @@ -305,6 +306,7 @@ class RBDConnector(base_rbd.RBDConnectorMixin, base.BaseLinuxConnector): return None @utils.trace + @utils.connect_volume_undo_prepare_result(unlink_after=True) def disconnect_volume(self, connection_properties, device_info, force=False, ignore_errors=False): """Disconnect a volume. @@ -368,6 +370,7 @@ class RBDConnector(base_rbd.RBDConnectorMixin, base.BaseLinuxConnector): # For backward compatibility ignore run_as_root param with handles return self._check_valid_device(path) + @utils.connect_volume_undo_prepare_result def extend_volume(self, connection_properties): """Refresh local volume view and return current size in bytes.""" # Nothing to do, RBD attached volumes are automatically refreshed, but diff --git a/os_brick/initiator/connectors/scaleio.py b/os_brick/initiator/connectors/scaleio.py index ea605a424..8fe5acccf 100644 --- a/os_brick/initiator/connectors/scaleio.py +++ b/os_brick/initiator/connectors/scaleio.py @@ -348,6 +348,7 @@ class ScaleIOConnector(base.BaseLinuxConnector): return device_info @utils.trace + @utils.connect_volume_prepare_result @lockutils.synchronized('scaleio', 'scaleio-', external=True) def connect_volume(self, connection_properties): """Connect the volume. @@ -463,6 +464,7 @@ class ScaleIOConnector(base.BaseLinuxConnector): @utils.trace @lockutils.synchronized('scaleio', 'scaleio-', external=True) + @utils.connect_volume_undo_prepare_result(unlink_after=True) def disconnect_volume(self, connection_properties, device_info, force=False, ignore_errors=False): """Disconnect the ScaleIO volume. @@ -526,6 +528,7 @@ class ScaleIOConnector(base.BaseLinuxConnector): LOG.error(msg) raise exception.BrickException(message=msg) + @utils.connect_volume_undo_prepare_result def extend_volume(self, connection_properties): """Update the local kernel's size information. diff --git a/os_brick/initiator/connectors/storpool.py b/os_brick/initiator/connectors/storpool.py index a84e41257..99989729c 100644 --- a/os_brick/initiator/connectors/storpool.py +++ b/os_brick/initiator/connectors/storpool.py @@ -21,6 +21,7 @@ from oslo_utils import importutils from os_brick import exception from os_brick.initiator.connectors import base +from os_brick import utils LOG = logging.getLogger(__name__) @@ -50,6 +51,7 @@ class StorPoolConnector(base.BaseLinuxConnector): """The StorPool connector properties.""" return {} + @utils.connect_volume_prepare_result def connect_volume(self, connection_properties): """Connect to a volume. @@ -85,6 +87,7 @@ class StorPoolConnector(base.BaseLinuxConnector): self._attach.sync(req_id, None) return {'type': 'block', 'path': '/dev/storpool/' + volume} + @utils.connect_volume_undo_prepare_result(unlink_after=True) def disconnect_volume(self, connection_properties, device_info, force=False, ignore_errors=False): """Disconnect a volume from the local host. @@ -195,6 +198,7 @@ class StorPoolConnector(base.BaseLinuxConnector): else: return None + @utils.connect_volume_undo_prepare_result def extend_volume(self, connection_properties): """Update the attached volume's size. diff --git a/os_brick/initiator/initiator_connector.py b/os_brick/initiator/initiator_connector.py index b91e013e4..f2eb82b0a 100644 --- a/os_brick/initiator/initiator_connector.py +++ b/os_brick/initiator/initiator_connector.py @@ -126,6 +126,11 @@ class InitiatorConnector(executor.Executor, metaclass=abc.ABCMeta): whenever there's a connection between the host's HBA and the storage array's target ports. + Encrypted volumes have some peculiar requirements on the path that must + be returned, so it is recommended to decorate the method with the + os_brick.utils.connect_volume_prepare_result to ensure that the right + device path is returned to the caller. + :param connection_properties: The dictionary that describes all of the target volume attributes. :type connection_properties: dict @@ -141,6 +146,15 @@ class InitiatorConnector(executor.Executor, metaclass=abc.ABCMeta): The connection_properties are the same as from connect_volume. The device_info is returned from connect_volume. + If the connector's connect_volume is decorated with + os_brick.utils.connect_volume_prepare_result then the path will + have been changed by the decorator if the volume was encrypted, so if + we need to have the original path that the connector returned instead + of the modified one (for example to identify the WWN from the symlink) + then we should use the + os_brick.utils.connect_volume_undo_prepare_result decorator with the + unlink_after=True parameter. + :param connection_properties: The dictionary that describes all of the target volume attributes. :type connection_properties: dict @@ -187,6 +201,15 @@ class InitiatorConnector(executor.Executor, metaclass=abc.ABCMeta): system. The new volume size in bytes will be returned. If there is a failure to update, then None will be returned. + If the connector's connect_volume is decorated with + os_brick.utils.connect_volume_prepare_result then the path will + have been changed by the decorator if the volume was encrypted, so if + this method uses the original path as a shortcut to know which device + to extend (instead of using the other connection information) then it + should use the os_brick.utils.connect_volume_undo_prepare_result + decorator on this method so that it gets the original path instead of + the modified symlink one. + :param connection_properties: The volume connection properties. :returns: new size of the volume. """ diff --git a/os_brick/initiator/linuxscsi.py b/os_brick/initiator/linuxscsi.py index ccfb8b0b0..ddbaf02d9 100644 --- a/os_brick/initiator/linuxscsi.py +++ b/os_brick/initiator/linuxscsi.py @@ -254,14 +254,6 @@ class LinuxSCSI(executor.Executor): return dm return None - @staticmethod - def get_dev_path(connection_properties, device_info): - """Determine what path was used by Nova/Cinder to access volume.""" - if device_info and device_info.get('path'): - return device_info.get('path') - - return connection_properties.get('device_path') or '' - @staticmethod def requires_flush(path, path_used, was_multipath): """Check if a device needs to be flushed when detaching. diff --git a/os_brick/privileged/rootwrap.py b/os_brick/privileged/rootwrap.py index 145c97c73..fa13237e5 100644 --- a/os_brick/privileged/rootwrap.py +++ b/os_brick/privileged/rootwrap.py @@ -216,8 +216,25 @@ def unlink_root(*links, **kwargs): raise_at_end = kwargs.get('raise_at_end', False) exc = exception.ExceptionChainer() catch_exception = no_errors or raise_at_end + LOG.debug('Unlinking %s', links) for link in links: with exc.context(catch_exception, 'Unlink failed for %s', link): os.unlink(link) if not no_errors and raise_at_end and exc: raise exc + + +@privileged.default.entrypoint +def link_root(target, link_name, force=True): + """Create a symbolic link with sys admin privileges. + + This method behaves like the "ln -s" command, including the force parameter + where it will replace the link_name file even if it's not a symlink. + """ + LOG.debug('Linking %s -> %s', link_name, target) + if force: + try: + os.remove(link_name) + except FileNotFoundError: + pass + os.symlink(target, link_name) diff --git a/os_brick/tests/initiator/connectors/test_fibre_channel.py b/os_brick/tests/initiator/connectors/test_fibre_channel.py index e54d3181b..063058eee 100644 --- a/os_brick/tests/initiator/connectors/test_fibre_channel.py +++ b/os_brick/tests/initiator/connectors/test_fibre_channel.py @@ -740,7 +740,7 @@ class FibreChannelConnectorTestCase(test_connector.ConnectorTestCase): @ddt.unpack @mock.patch('os_brick.initiator.linuxscsi.LinuxSCSI.remove_scsi_device') @mock.patch('os_brick.initiator.linuxscsi.LinuxSCSI.requires_flush') - @mock.patch('os_brick.initiator.linuxscsi.LinuxSCSI.get_dev_path') + @mock.patch('os_brick.utils.get_dev_path') def test__remove_devices(self, path_used, was_multipath, get_dev_path_mock, flush_mock, remove_mock): get_dev_path_mock.return_value = path_used diff --git a/os_brick/tests/initiator/connectors/test_iscsi.py b/os_brick/tests/initiator/connectors/test_iscsi.py index ca33a2d84..0327b0dde 100644 --- a/os_brick/tests/initiator/connectors/test_iscsi.py +++ b/os_brick/tests/initiator/connectors/test_iscsi.py @@ -551,13 +551,17 @@ class ISCSIConnectorTestCase(test_connector.ConnectorTestCase): @mock.patch.object(iscsi.ISCSIConnector, '_cleanup_connection') def test_disconnect_volume(self, cleanup_mock): - res = self.connector.disconnect_volume(mock.sentinel.con_props, + conn_props = {'target_portal': '198.72.124.185:3260', + 'target_iqn': 'iqn.2010-10.org.openstack:volume-uuid', + 'target_lun': 0, + 'device_path': '/dev/sda'} + res = self.connector.disconnect_volume(conn_props, mock.sentinel.dev_info, mock.sentinel.Force, mock.sentinel.ignore_errors) self.assertEqual(cleanup_mock.return_value, res) cleanup_mock.assert_called_once_with( - mock.sentinel.con_props, + conn_props, force=mock.sentinel.Force, ignore_errors=mock.sentinel.ignore_errors, device_info=mock.sentinel.dev_info, @@ -677,7 +681,7 @@ class ISCSIConnectorTestCase(test_connector.ConnectorTestCase): ('/dev/dm-11', True), ('/dev/disk/by-id/dm-uuid-mpath-MPATH', True)) @ddt.unpack - @mock.patch.object(linuxscsi.LinuxSCSI, 'get_dev_path') + @mock.patch('os_brick.utils.get_dev_path') @mock.patch.object(iscsi.ISCSIConnector, '_disconnect_connection') @mock.patch.object(iscsi.ISCSIConnector, '_get_connection_devices') @mock.patch.object(linuxscsi.LinuxSCSI, 'flush_multipath_device') @@ -1641,17 +1645,14 @@ Setting up iSCSI targets: unused scan_mock.assert_not_called() dev_name_mock.assert_called_once_with(mock.sentinel.session, hctl) - @mock.patch.object(iscsi.ISCSIConnector, '_get_device_link') - def test__get_connect_result(self, get_link_mock): + def test__get_connect_result(self): props = self.CON_PROPS.copy() props['encrypted'] = False res = self.connector._get_connect_result(props, 'wwn', ['sda', 'sdb']) expected = {'type': 'block', 'scsi_wwn': 'wwn', 'path': '/dev/sda'} self.assertDictEqual(expected, res) - get_link_mock.assert_not_called() - @mock.patch.object(iscsi.ISCSIConnector, '_get_device_link') - def test__get_connect_result_mpath(self, get_link_mock): + def test__get_connect_result_mpath(self): props = self.CON_PROPS.copy() props['encrypted'] = False res = self.connector._get_connect_result(props, 'wwn', ['sda', 'sdb'], @@ -1659,106 +1660,6 @@ Setting up iSCSI targets: unused expected = {'type': 'block', 'scsi_wwn': 'wwn', 'path': '/dev/mpath', 'multipath_id': 'wwn'} self.assertDictEqual(expected, res) - get_link_mock.assert_not_called() - - @mock.patch.object(iscsi.ISCSIConnector, '_get_device_link', - return_value='/dev/disk/by-id/scsi-wwn') - def test__get_connect_result_encrypted(self, get_link_mock): - props = self.CON_PROPS.copy() - props['encrypted'] = True - res = self.connector._get_connect_result(props, 'wwn', ['sda', 'sdb']) - expected = {'type': 'block', 'scsi_wwn': 'wwn', - 'path': get_link_mock.return_value} - self.assertDictEqual(expected, res) - get_link_mock.assert_called_once_with('wwn', '/dev/sda', None) - - @mock.patch('os.path.realpath', return_value='/dev/sda') - def test__get_device_link(self, realpath_mock): - symlink = '/dev/disk/by-id/scsi-wwn' - res = self.connector._get_device_link('wwn', '/dev/sda', None) - self.assertEqual(symlink, res) - realpath_mock.assert_called_once_with(symlink) - - @mock.patch('os.path.realpath', return_value='/dev/dm-0') - def test__get_device_link_multipath(self, realpath_mock): - symlink = '/dev/disk/by-id/dm-uuid-mpath-wwn' - res = self.connector._get_device_link('wwn', '/dev/dm-0', 'wwn') - self.assertEqual(symlink, res) - realpath_mock.assert_called_once_with(symlink) - - @mock.patch('os.path.realpath', side_effect=('/dev/sdz', '/dev/sdy', - '/dev/sda', '/dev/sdx')) - @mock.patch('os.listdir', return_value=['dm-...', 'scsi-wwn', 'scsi-...']) - def test__get_device_link_check_links(self, listdir_mock, realpath_mock): - res = self.connector._get_device_link('wwn', '/dev/sda', None) - self.assertEqual(res, '/dev/disk/by-id/scsi-wwn') - listdir_mock.assert_called_once_with('/dev/disk/by-id/') - realpath_mock.assert_has_calls([ - mock.call('/dev/disk/by-id/scsi-wwn'), - mock.call('/dev/disk/by-id/dm-...'), - mock.call('/dev/disk/by-id/scsi-wwn')]) - - @mock.patch('os_brick.utils._time_sleep') - @mock.patch('os.path.realpath', return_value='/dev/sdz') - @mock.patch('os.listdir', return_value=['dm-...', 'scsi-...']) - def test__get_device_link_not_found(self, listdir_mock, realpath_mock, - mock_time): - self.assertRaises(exception.VolumeDeviceNotFound, - self.connector._get_device_link, - 'wwn', '/dev/sda', None) - listdir_mock.assert_has_calls(3 * [mock.call('/dev/disk/by-id/')]) - self.assertEqual(3, listdir_mock.call_count) - realpath_mock.assert_has_calls( - 3 * [mock.call('/dev/disk/by-id/scsi-wwn'), - mock.call('/dev/disk/by-id/dm-...'), - mock.call('/dev/disk/by-id/scsi-...')]) - self.assertEqual(9, realpath_mock.call_count) - - @mock.patch('os_brick.utils._time_sleep') - @mock.patch('os.path.realpath') - @mock.patch('os.listdir', return_value=['dm-...', 'scsi-...']) - def test__get_device_link_symlink_found_after_retry(self, mock_listdir, - mock_realpath, - mock_time): - # Return the expected realpath on the third retry - mock_realpath.side_effect = [ - None, None, None, None, None, None, '/dev/sda'] - - # Assert that VolumeDeviceNotFound isn't raised - self.connector._get_device_link('wwn', '/dev/sda', None) - - # Assert that listdir and realpath have been called correctly - mock_listdir.assert_has_calls(2 * [mock.call('/dev/disk/by-id/')]) - self.assertEqual(2, mock_listdir.call_count) - mock_realpath.assert_has_calls( - 2 * [mock.call('/dev/disk/by-id/scsi-wwn'), - mock.call('/dev/disk/by-id/dm-...'), - mock.call('/dev/disk/by-id/scsi-...')] - + [mock.call('/dev/disk/by-id/scsi-wwn')]) - self.assertEqual(7, mock_realpath.call_count) - - @mock.patch('os_brick.utils._time_sleep') - @mock.patch('os.path.realpath') - @mock.patch('os.listdir', return_value=['dm-...', 'scsi-...']) - def test__get_device_link_symlink_found_after_retry_by_listdir( - self, mock_listdir, mock_realpath, mock_time): - - # Return the expected realpath on the second retry while looping over - # the devices returned by listdir - mock_realpath.side_effect = [ - None, None, None, None, None, '/dev/sda'] - - # Assert that VolumeDeviceNotFound isn't raised - self.connector._get_device_link('wwn', '/dev/sda', None) - - # Assert that listdir and realpath have been called correctly - mock_listdir.assert_has_calls(2 * [mock.call('/dev/disk/by-id/')]) - self.assertEqual(2, mock_listdir.call_count) - mock_realpath.assert_has_calls( - 2 * [mock.call('/dev/disk/by-id/scsi-wwn'), - mock.call('/dev/disk/by-id/dm-...'), - mock.call('/dev/disk/by-id/scsi-...')]) - self.assertEqual(6, mock_realpath.call_count) @mock.patch.object(iscsi.ISCSIConnector, '_run_iscsiadm_bare') def test_get_node_startup_values(self, run_iscsiadm_bare_mock): diff --git a/os_brick/tests/initiator/test_linuxscsi.py b/os_brick/tests/initiator/test_linuxscsi.py index 6ebcddda7..87aacc511 100644 --- a/os_brick/tests/initiator/test_linuxscsi.py +++ b/os_brick/tests/initiator/test_linuxscsi.py @@ -1231,36 +1231,6 @@ loop0 0""" self.linuxscsi.multipath_del_path('/dev/sda') self.assertEqual(['multipathd del path /dev/sda'], self.cmds) - @ddt.data({'con_props': {}, 'dev_info': {'path': mock.sentinel.path}}, - {'con_props': None, 'dev_info': {'path': mock.sentinel.path}}, - {'con_props': {'device_path': mock.sentinel.device_path}, - 'dev_info': {'path': mock.sentinel.path}}) - @ddt.unpack - def test_get_dev_path_device_info(self, con_props, dev_info): - self.assertEqual(mock.sentinel.path, - self.linuxscsi.get_dev_path(con_props, dev_info)) - - @ddt.data({'con_props': {'device_path': mock.sentinel.device_path}, - 'dev_info': {'path': None}}, - {'con_props': {'device_path': mock.sentinel.device_path}, - 'dev_info': {'path': ''}}, - {'con_props': {'device_path': mock.sentinel.device_path}, - 'dev_info': {}}, - {'con_props': {'device_path': mock.sentinel.device_path}, - 'dev_info': None}) - @ddt.unpack - def test_get_dev_path_conn_props(self, con_props, dev_info): - self.assertEqual(mock.sentinel.device_path, - self.linuxscsi.get_dev_path(con_props, dev_info)) - - @ddt.data({'con_props': {'device_path': ''}, 'dev_info': {'path': None}}, - {'con_props': {'device_path': None}, 'dev_info': {'path': ''}}, - {'con_props': {}, 'dev_info': {}}, - {'con_props': {}, 'dev_info': None}) - @ddt.unpack - def test_get_dev_path_no_path(self, con_props, dev_info): - self.assertEqual('', self.linuxscsi.get_dev_path(con_props, dev_info)) - @ddt.data(('/dev/sda', '/dev/sda', False, True, None), # This checks that we ignore the was_multipath parameter if it # doesn't make sense (because the used path is the one we are diff --git a/os_brick/tests/privileged/test_rootwrap.py b/os_brick/tests/privileged/test_rootwrap.py index b4c1db08f..47f21d574 100644 --- a/os_brick/tests/privileged/test_rootwrap.py +++ b/os_brick/tests/privileged/test_rootwrap.py @@ -12,6 +12,7 @@ from unittest import mock +import ddt from oslo_concurrency import processutils as putils from os_brick import exception @@ -20,6 +21,7 @@ from os_brick.privileged import rootwrap as priv_rootwrap from os_brick.tests import base +@ddt.ddt class PrivRootwrapTestCase(base.TestCase): def setUp(self): super(PrivRootwrapTestCase, self).setUp() @@ -155,3 +157,37 @@ class PrivRootwrapTestCase(base.TestCase): *links, raise_at_end=True) unlink_mock.assert_has_calls([mock.call(links[0]), mock.call(links[1])]) + + @mock.patch.object(priv_rootwrap.unlink_root.privsep_entrypoint, + 'client_mode', False) + @mock.patch('os.symlink') + @mock.patch('os.remove') + def test_link_root_no_force(self, mock_remove, mock_link): + priv_rootwrap.link_root(mock.sentinel.target, mock.sentinel.link_name, + force=False) + mock_remove.assert_not_called() + mock_link.assert_called_once_with(mock.sentinel.target, + mock.sentinel.link_name) + + @ddt.data(None, FileNotFoundError) + @mock.patch.object(priv_rootwrap.unlink_root.privsep_entrypoint, + 'client_mode', False) + @mock.patch('os.symlink') + @mock.patch('os.remove') + def test_link_root_force(self, remove_effect, mock_remove, mock_link): + mock_remove.side_effect = remove_effect + priv_rootwrap.link_root(mock.sentinel.target, mock.sentinel.link_name) + mock_remove.assert_called_once_with(mock.sentinel.link_name) + mock_link.assert_called_once_with(mock.sentinel.target, + mock.sentinel.link_name) + + @mock.patch.object(priv_rootwrap.unlink_root.privsep_entrypoint, + 'client_mode', False) + @mock.patch('os.symlink') + @mock.patch('os.remove', side_effect=IndexError) # Non not found error + def test_link_root_force_fail(self, mock_remove, mock_link): + self.assertRaises(IndexError, + priv_rootwrap.link_root, + mock.sentinel.target, mock.sentinel.link_name) + mock_remove.assert_called_once_with(mock.sentinel.link_name) + mock_link.assert_not_called() diff --git a/os_brick/tests/test_utils.py b/os_brick/tests/test_utils.py index f1366c21e..3b1aa442b 100644 --- a/os_brick/tests/test_utils.py +++ b/os_brick/tests/test_utils.py @@ -13,9 +13,12 @@ # under the License. import functools +import io import time from unittest import mock +import ddt + from os_brick import exception from os_brick.tests import base from os_brick import utils @@ -313,3 +316,277 @@ class LogTracingTestCase(base.TestCase): self.assertEqual(2, mock_log.debug.call_count) self.assertIn("'auth_password': '***'", str(mock_log.debug.call_args_list[0])) + + +@ddt.ddt +class GetDevPathTestCase(base.TestCase): + """Test the get_dev_path method.""" + @ddt.data({'con_props': {}, 'dev_info': {'path': '/dev/sda'}}, + {'con_props': {}, 'dev_info': {'path': b'/dev/sda'}}, + {'con_props': None, 'dev_info': {'path': '/dev/sda'}}, + {'con_props': None, 'dev_info': {'path': b'/dev/sda'}}, + {'con_props': {'device_path': b'/dev/sdb'}, + 'dev_info': {'path': '/dev/sda'}}, + {'con_props': {'device_path': '/dev/sdb'}, + 'dev_info': {'path': b'/dev/sda'}}) + @ddt.unpack + def test_get_dev_path_device_info(self, con_props, dev_info): + self.assertEqual('/dev/sda', utils.get_dev_path(con_props, dev_info)) + + @ddt.data({'con_props': {'device_path': '/dev/sda'}, + 'dev_info': {'path': None}}, + {'con_props': {'device_path': b'/dev/sda'}, + 'dev_info': {'path': None}}, + {'con_props': {'device_path': '/dev/sda'}, + 'dev_info': {'path': ''}}, + {'con_props': {'device_path': b'/dev/sda'}, + 'dev_info': {'path': ''}}, + {'con_props': {'device_path': '/dev/sda'}, + 'dev_info': {}}, + {'con_props': {'device_path': b'/dev/sda'}, + 'dev_info': {}}, + {'con_props': {'device_path': '/dev/sda'}, + 'dev_info': None}, + {'con_props': {'device_path': b'/dev/sda'}, + 'dev_info': None}) + @ddt.unpack + def test_get_dev_path_conn_props(self, con_props, dev_info): + self.assertEqual('/dev/sda', utils.get_dev_path(con_props, dev_info)) + + @ddt.data({'con_props': {'device_path': ''}, 'dev_info': {'path': None}}, + {'con_props': {'device_path': None}, 'dev_info': {'path': ''}}, + {'con_props': {}, 'dev_info': {}}, + {'con_props': {}, 'dev_info': None}) + @ddt.unpack + def test_get_dev_path_no_path(self, con_props, dev_info): + self.assertEqual('', utils.get_dev_path(con_props, dev_info)) + + +@ddt.ddt +class ConnectionPropertiesDecoratorsTestCase(base.TestCase): + def test__symlink_name_from_device_path(self): + """Get symlink for non replicated device.""" + dev_name = '/dev/nvme0n1' + res = utils._symlink_name_from_device_path(dev_name) + self.assertEqual('/dev/disk/by-id/os-brick+dev+nvme0n1', res) + + def test__symlink_name_from_device_path_raid(self): + """Get symlink for replicated device.""" + dev_name = '/dev/md/alias' + res = utils._symlink_name_from_device_path(dev_name) + self.assertEqual('/dev/disk/by-id/os-brick+dev+md+alias', res) + + def test__device_path_from_symlink(self): + """Get device name for non replicated symlink.""" + symlink = '/dev/disk/by-id/os-brick+dev+nvme0n1' + res = utils._device_path_from_symlink(symlink) + self.assertEqual('/dev/nvme0n1', res) + + def test__device_path_from_symlink_raid(self): + """Get device name for replicated symlink.""" + symlink = '/dev/disk/by-id/os-brick+dev+md+alias' + res = utils._device_path_from_symlink(symlink) + self.assertEqual('/dev/md/alias', res) + + @ddt.data(({}, {'type': 'block', 'path': '/dev/sda'}), + ({'encrypted': False}, {'type': 'block', 'path': '/dev/sda'}), + ({'encrypted': False}, {'type': 'block', 'path': b'/dev/sda'}), + ({'encrypted': True}, {'type': 'block', 'path': io.StringIO()})) + @ddt.unpack + @mock.patch('os_brick.utils._symlink_name_from_device_path') + @mock.patch('os.path.realpath') + @mock.patch('os_brick.privileged.rootwrap.link_root') + def test_connect_volume_prepare_result_non_encrypted( + self, conn_props, result, mock_link, mock_path, mock_get_symlink): + """Test decorator for non encrypted devices or non host devices.""" + testing_self = mock.Mock() + testing_self.connect_volume.return_value = result + func = utils.connect_volume_prepare_result(testing_self.connect_volume) + + res = func(testing_self, conn_props) + self.assertEqual(testing_self.connect_volume.return_value, res) + + testing_self.connect_volume.assert_called_once_with(testing_self, + conn_props) + mock_path.assert_not_called() + mock_get_symlink.assert_not_called() + mock_link.assert_not_called() + + @ddt.data('/dev/md/alias', b'/dev/md/alias') + @mock.patch('os_brick.utils._symlink_name_from_device_path') + @mock.patch('os.path.realpath') + @mock.patch('os_brick.privileged.rootwrap.link_root') + def test_connect_volume_prepare_result_encrypted( + self, connector_path, mock_link, mock_path, mock_get_symlink): + """Test decorator for encrypted device.""" + real_device = '/dev/md-6' + expected_symlink = '/dev/disk/by-id/os-brick_dev_md_alias' + mock_path.return_value = real_device + mock_get_symlink.return_value = expected_symlink + testing_self = mock.Mock() + testing_self.connect_volume.return_value = {'type': 'block', + 'path': connector_path} + conn_props = {'encrypted': True} + func = utils.connect_volume_prepare_result(testing_self.connect_volume) + + res = func(testing_self, conn_props) + self.assertEqual({'type': 'block', 'path': expected_symlink}, res) + + testing_self.connect_volume.assert_called_once_with(testing_self, + conn_props) + expected_connector_path = utils.convert_str(connector_path) + mock_get_symlink.assert_called_once_with(expected_connector_path) + mock_link.assert_called_once_with(real_device, expected_symlink, + force=True) + + @ddt.data({}, {'encrypted': False}, {'encrypted': True}) + @mock.patch('os_brick.utils._symlink_name_from_device_path') + @mock.patch('os.path.realpath') + @mock.patch('os_brick.privileged.rootwrap.link_root') + def test_connect_volume_prepare_result_connect_fail( + self, conn_props, mock_link, mock_path, mock_get_symlink): + """Test decorator when decorated function fails.""" + testing_self = mock.Mock() + testing_self.connect_volume.side_effect = ValueError + + func = utils.connect_volume_prepare_result(testing_self.connect_volume) + self.assertRaises(ValueError, func, testing_self, conn_props) + mock_link.assert_not_called() + mock_path.assert_not_called() + mock_get_symlink.assert_not_called() + + @mock.patch('os_brick.utils._symlink_name_from_device_path') + @mock.patch('os.path.realpath') + @mock.patch('os_brick.privileged.rootwrap.link_root') + def test_connect_volume_prepare_result_symlink_fail( + self, mock_link, mock_path, mock_get_symlink): + """Test decorator for encrypted device failing on the symlink.""" + real_device = '/dev/md-6' + connector_path = '/dev/md/alias' + expected_symlink = '/dev/disk/by-id/os-brick_dev_md_alias' + mock_path.return_value = real_device + mock_get_symlink.return_value = expected_symlink + testing_self = mock.Mock() + connect_result = {'type': 'block', 'path': connector_path} + mock_link.side_effect = ValueError + + testing_self.connect_volume.return_value = connect_result + conn_props = {'encrypted': True} + func = utils.connect_volume_prepare_result(testing_self.connect_volume) + + self.assertRaises(ValueError, func, testing_self, conn_props) + + testing_self.connect_volume.assert_called_once_with(testing_self, + conn_props) + mock_get_symlink.assert_called_once_with(connector_path) + mock_link.assert_called_once_with(real_device, expected_symlink, + force=True) + testing_self.disconnect_volume.assert_called_once_with( + connect_result, force=True, ignore_errors=True) + + @ddt.data(({'device_path': '/dev/md/alias'}, {}), + ({'device_path': '/dev/md/alias', 'encrypted': False}, None), + ({'device_path': '/dev/md/alias'}, {'path': '/dev/md/alias'}), + ({'device_path': '/dev/md/alias', 'encrypted': False}, + {'path': '/dev/md/alias'}), + ({'device_path': io.StringIO(), 'encrypted': True}, None), + ({'device_path': '/dev/disk/by-id/wwn-...', 'encrypted': True}, + None)) + @ddt.unpack + @mock.patch('os_brick.utils._device_path_from_symlink') + @mock.patch('os_brick.privileged.rootwrap.unlink_root') + def test_connect_volume_undo_prepare_result_non_custom_link( + outer_self, conn_props, dev_info, mock_unlink, mock_dev_path): + + class Test(object): + @utils.connect_volume_undo_prepare_result(unlink_after=True) + def disconnect_volume(self, connection_properties, device_info, + force=False, ignore_errors=False): + outer_self.assertEqual(conn_props, connection_properties) + outer_self.assertEqual(dev_info, device_info) + return 'disconnect_volume' + + @utils.connect_volume_undo_prepare_result + def extend_volume(self, connection_properties): + outer_self.assertEqual(conn_props, connection_properties) + return 'extend_volume' + + path = conn_props['device_path'] + mock_dev_path.return_value = path + + t = Test() + + res = t.disconnect_volume(conn_props, dev_info) + outer_self.assertEqual('disconnect_volume', res) + + res = t.extend_volume(conn_props) + outer_self.assertEqual('extend_volume', res) + + if conn_props.get('encrypted'): + outer_self.assertEqual(2, mock_dev_path.call_count) + mock_dev_path.assert_has_calls((mock.call(path), mock.call(path))) + else: + mock_dev_path.assert_not_called() + mock_unlink.assert_not_called() + + @mock.patch('os_brick.utils._device_path_from_symlink') + @mock.patch('os_brick.privileged.rootwrap.unlink_root') + def test_connect_volume_undo_prepare_result_encrypted_disconnect( + outer_self, mock_unlink, mock_dev_path): + connector_path = '/dev/md/alias' + mock_dev_path.return_value = connector_path + symlink_path = '/dev/disk/by-id/os-brick_dev_md_alias' + mock_unlink.side_effect = ValueError + + class Test(object): + @utils.connect_volume_undo_prepare_result(unlink_after=True) + def disconnect_volume(self, connection_properties, device_info, + force=False, ignore_errors=False): + outer_self.assertEqual(connector_path, + connection_properties['device_path']) + outer_self.assertEqual(connector_path, + device_info['path']) + return 'disconnect_volume' + + conn_props = {'target_portal': '198.72.124.185:3260', + 'target_iqn': 'iqn.2010-10.org.openstack:volume-uuid', + 'target_lun': 0, + 'encrypted': True, + 'device_path': symlink_path} + dev_info = {'type': 'block', 'path': symlink_path} + + t = Test() + res = t.disconnect_volume(conn_props, dev_info) + + outer_self.assertEqual('disconnect_volume', res) + mock_dev_path.assert_called_once_with(symlink_path) + mock_unlink.assert_called_once_with(symlink_path) + + @mock.patch('os_brick.utils._device_path_from_symlink') + @mock.patch('os_brick.privileged.rootwrap.unlink_root') + def test_connect_volume_undo_prepare_result_encrypted_extend( + outer_self, mock_unlink, mock_dev_path): + connector_path = '/dev/md/alias' + mock_dev_path.return_value = connector_path + symlink_path = '/dev/disk/by-id/os-brick_dev_md_alias' + mock_unlink.side_effect = ValueError + + class Test(object): + @utils.connect_volume_undo_prepare_result + def extend_volume(self, connection_properties): + outer_self.assertEqual(connector_path, + connection_properties['device_path']) + return 'extend_volume' + + conn_props = {'target_portal': '198.72.124.185:3260', + 'target_iqn': 'iqn.2010-10.org.openstack:volume-uuid', + 'target_lun': 0, + 'encrypted': True, + 'device_path': symlink_path} + + t = Test() + res = t.extend_volume(conn_props) + + outer_self.assertEqual('extend_volume', res) + mock_dev_path.assert_called_once_with(symlink_path) + mock_unlink.assert_not_called() diff --git a/os_brick/utils.py b/os_brick/utils.py index 5fc0797f8..c0fd69503 100644 --- a/os_brick/utils.py +++ b/os_brick/utils.py @@ -15,8 +15,9 @@ import functools import inspect import logging as py_logging +import os import time -from typing import Callable, Tuple, Type, Union # noqa: H301 +from typing import Any, Callable, Tuple, Type, Union # noqa: H301 from oslo_concurrency import processutils from oslo_log import log as logging @@ -24,6 +25,10 @@ from oslo_utils import strutils from os_brick.i18n import _ from os_brick.privileged import nvmeof as priv_nvme +from os_brick.privileged import rootwrap as priv_rootwrap + + +CUSTOM_LINK_PREFIX = '/dev/disk/by-id/os-brick' _time_sleep = time.sleep @@ -221,3 +226,170 @@ def get_host_nqn(): except Exception: host_nqn = None return host_nqn + + +def _symlink_name_from_device_path(device_path): + """Generate symlink absolute path for encrypted devices. + + The symlink's basename will contain the original device name so we can + reconstruct it afterwards on disconnect. + + Being able to restore the original device name may be important for some + connectors, because the system may have multiple devices for the same + connection information (for example if a controller came back to life after + having network issues and an auto scan presented the device) and if we + reuse an existing symlink created by udev we wouldn't know which one was + actually used. + + The symlink will be created under the /dev/disk/by-id directory and will + prefix the name with os-brick- and then continue with the full device path + that was passed (replacing '/' with '+') + """ + # Convert / into + that is unlikely used by devices or symlinks (cryptsetup + # is not happy if we use ยท in the symlink) + encoded_device = device_path.replace('/', '+') + return CUSTOM_LINK_PREFIX + encoded_device + + +def _device_path_from_symlink(symlink): + """Get the original encrypted device path from the device symlink. + + This is the reverse operation of the one performed by the + _symlink_name_from_device_path method. + """ + if symlink and symlink.startswith(CUSTOM_LINK_PREFIX): + ending = symlink[len(CUSTOM_LINK_PREFIX):] + return ending.replace('+', '/') + return symlink + + +def connect_volume_prepare_result( + func: Callable[[Any, dict], dict]) -> Callable[[Any, dict], dict]: + """Decorator to prepare the result of connect_volume for encrypted volumes. + + WARNING: This decorator must be **before** any connect_volume locking + because it may call disconnect_volume. + + Encryptor drivers expect a symlink that they "own", so that they can modify + it as they want. + + The current flow is like this: + + - connect_volume connector call + - libvirt config is generated by Nova using returned path + - connect_volume encryptor call => Replaces the original path + + For encrypted volumes the decorator modifies the "path" value for the + returned dictionary. + + Unencrypted volumes will be left unchanged. + + There are special connectors that return a file descriptor instead of a + path depending on the parameters. In those cases the result will also be + left untouched. + + If a connector relies on the path that has been used they can use the + connect_volume_undo_prepare_result decorator to get the value changed back + the original path. + """ + @functools.wraps(func) + def change_encrypted(self, connection_properties): + res = func(self, connection_properties) + # Decode if path is bytes, otherwise leave it as it is + device_path = convert_str(res['path']) + # There are connectors that sometimes return file descriptors (rbd) + if (connection_properties.get('encrypted') and + isinstance(device_path, str)): + symlink = _symlink_name_from_device_path(device_path) + try: + priv_rootwrap.link_root(os.path.realpath(device_path), + symlink, + force=True) + res['path'] = symlink + except Exception as exc: + LOG.debug('Failed to create symlink, cleaning connection: %s', + exc) + self.disconnect_volume(res, force=True, ignore_errors=True) + raise + + return res + return change_encrypted + + +def get_dev_path(connection_properties, device_info): + """Return the device that was returned when connecting a volume.""" + if device_info and device_info.get('path'): + res = device_info['path'] + else: + res = connection_properties.get('device_path') or '' + + # Decode if path is bytes, otherwise leave it as it is + return convert_str(res) + + +def connect_volume_undo_prepare_result(f=None, unlink_after=False): + """Decorator that returns the device path to how it was originally. + + WARNING: This decorator must be **the first** decorator of the method to + get the actual method signature during introspection. + + Undo changes made to the device path of encrypted volumes done by the + connect_volume_prepare_result decorator. + + That way the connector will always get back the same device path that it + returned. + + Examples of connector methods that may want to use this are + disconnect_volume and extend_volume. + + It can optionally delete the symlink on successful completion, required for + disconnect_volume method. + + @connect_volume_undo_prepare_result(unlink_after=True) + def disconnect_volume(...): + + @connect_volume_undo_prepare_result + def extend_volume(...): + + """ + def decorator(func): + @functools.wraps(func) + def change_encrypted(*args, **kwargs): + # May receive only connection_properties or also device_info params + call_args = inspect.getcallargs(func, *args, **kwargs) + conn_props = call_args['connection_properties'] + + custom_symlink = False + if conn_props.get('encrypted'): + dev_info = call_args.get('device_info') + symlink = get_dev_path(conn_props, dev_info) + devpath = _device_path_from_symlink(symlink) + + # Symlink can be a file descriptor, which we don't touch, same + # for old symlinks where the path is the same + if isinstance(symlink, str) and symlink != devpath: + custom_symlink = True + # Don't modify the caller's dictionaries + call_args['connection_properties'] = conn_props.copy() + call_args['connection_properties']['device_path'] = devpath + + # Same for the device info dictionary + if dev_info: + dev_info = call_args['device_info'] = dev_info.copy() + dev_info['path'] = devpath + + res = func(**call_args) + + # Clean symlink only when asked (usually on disconnect) + if custom_symlink and unlink_after: + try: + priv_rootwrap.unlink_root(symlink) + except Exception: + LOG.warning('Failed to remove encrypted custom symlink %s', + symlink) + return res + return change_encrypted + + if f: + return decorator(f) + return decorator diff --git a/releasenotes/notes/encryption-a642889a82ff9207.yaml b/releasenotes/notes/encryption-a642889a82ff9207.yaml new file mode 100644 index 000000000..eeaa68fc9 --- /dev/null +++ b/releasenotes/notes/encryption-a642889a82ff9207.yaml @@ -0,0 +1,12 @@ +--- +fixes: + - | + NVMe-oF connector `bug #1964379 + `_: Fixed using non LUKS + v1 encrypted volumes, as once one of such volumes is disconnected from a + host all successive NVMe-oF attachments would fail. + - | + `Bug #1967790 + `_: Fixed encryptor's + connect_volume returns and the symlink is pointing to the raw block device + instead of to the decrypted device mapper device.