openstack
/
os-brick
Code Issues Proposed changes

Return symlinks for encrypted volumes 

When connecting encrypted volumes we need to return a symbolink link or
we will break all future attachments after detaching the volume.

OS-Brick on 1.14 and 1.15 returns real paths instead of returning symbolic
links, which results in the encryption attach_volume call replacing the
real device with a link to the crypt dm.

The issue comes from the Nova flow when attaching an encrypted volume:

1- Attach volume
2- Generate libvirt configuration with path from step 1
3- Encrypt attach volume

Since step 2 has already generated the config with the path from step 1 then
step 3 must preserve this path.

When step 1 returns a symbolic link we just forcefully replace it with a link
to the crypt dm and everything is OK, but when we return a real path it
does the same thing, which means we'll be replacing for example /dev/sda
with a symlink, which will then break the detach process, and all future
attachments.

Until Nova, Cinder, and OS-Brick are changed to have a different flow
(1, 3, 2) we need a workaround to make it work.

The workaround this patch introduces is to return a symbolic link when
the volume is encrypted.

It will try to return the symlink that always exists, but if it's not
there it will just look for ANY link to the device in '/dev/disk/by-id'.

Related-Bug: #1703954
Change-Id: If4461c3dcd67e5d948be9d9a3643c1eb81aaace9
This commit is contained in:
Gorka Eguileor 2017-07-12 19:55:20 +02:00
parent a1b0dda617
commit f341e9c3ed
2 changed files with 113 additions and 6 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

49
os_brick/initiator/connectors/iscsi.py
View File

@ -506,6 +506,45 @@ class ISCSIConnector(base.BaseLinuxConnector, base_iscsi.BaseISCSIConnector):
with excutils.save_and_reraise_exception():
self._cleanup_connection(connection_properties, force=True)
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'] = mpath
return result
@utils.retry(exceptions=(exception.VolumeDeviceNotFound))
def _connect_single_volume(self, connection_properties):
"""Connect to a volume using a single path."""
@ -520,8 +559,8 @@ class ISCSIConnector(base.BaseLinuxConnector, base_iscsi.BaseISCSIConnector):
for __ in range(10):
wwn = self._linuxscsi.get_sysfs_wwn(found_devs)
if wwn:
return {'type': 'block', 'scsi_wwn': wwn,
'path': '/dev/' + found_devs[0]}
return self._get_connect_result(connection_properties,
wwn, found_devs)
time.sleep(1)
LOG.debug('Could not find the WWN for %s.', found_devs[0])
@ -707,10 +746,8 @@ class ISCSIConnector(base.BaseLinuxConnector, base_iscsi.BaseISCSIConnector):
if not mpath:
LOG.warning('No dm was created, connection to volume is probably '
'bad and will perform poorly.')
return {'type': 'block', 'scsi_wwn': wwn,
'path': '/dev/' + found[0]}
return {'type': 'block', 'scsi_wwn': wwn, 'multipath_id': mpath,
'path': '/dev/' + mpath}
return self._get_connect_result(connection_properties, wwn, found,
mpath)
def _get_connection_devices(self, connection_properties,
ips_iqns_luns=None):

70
os_brick/tests/initiator/connectors/test_iscsi.py
View File

@ -30,6 +30,7 @@ class ISCSIConnectorTestCase(test_connector.ConnectorTestCase):
SINGLE_CON_PROPS = {'volume_id': 'vol_id',
'target_portal': 'ip1:port1',
'target_iqn': 'tgt1',
'encryption': False,
'target_lun': '1'}
CON_PROPS = {
'volume_id': 'vol_id',
@ -1368,3 +1369,72 @@ Setting up iSCSI targets: unused
self.CON_PROPS['target_lun'])
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):
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):
props = self.CON_PROPS.copy()
props['encrypted'] = False
res = self.connector._get_connect_result(props, 'wwn', ['sda', 'sdb'],
'mpath')
expected = {'type': 'block', 'scsi_wwn': 'wwn', 'path': '/dev/mpath',
'multipath_id': 'mpath'}
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.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):
self.assertRaises(exception.VolumeDeviceNotFound,
self.connector._get_device_link,
'wwn', '/dev/sda', None)
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-...')])