openstack
/
nova
Code Issues Proposed changes

HyperV: Fix vm disk path issue 

As the disk number of iSCSI attached disks can change after host
reboot, passthrough attached volumes can get attached in this case.

This bug was partially fixed during Icehouse by this patch:
https://review.openstack.org/95356

One of the issues with this patch is that it only handles SCSI
attached disks, for which reason this issue continues to occur when
having generation 1 VMs booted from volume, in which case the disk
will be placed on the IDE controller.

In this case, one instance may end up booting from another tenant's
volume, which is a critical security issue.

Also, it assumes that the block device info volume order matches
the according disk controller slot order, which is wrong.

This patch fixes this issue, looping through the block device
info and fixing the disk path for all the passthrough attached
disks, regardless of the disk controller.

This requires the virtual disk resources attached to the instance
to be tagged with the serial number.

Co-Authored-By: Alin Balutoiu <abalutoiu@cloudbasesolutions.com>

Closes-Bug: #1526831
Depends-On: Ibe463ce9ffb9129cab4b99fe7967ccb5f2181958
Change-Id: Icf8697d917c814ec120fce9898661eb9a20ade48
This commit is contained in:
Lucian Petrut 2015-12-10 19:23:08 +02:00
parent 82bf282dd5
commit 9de0bec4f9
2 changed files with 159 additions and 83 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

152
nova/tests/unit/virt/hyperv/test_volumeops.py
View File

@ -55,6 +55,7 @@ class VolumeOpsTestCase(test_base.HyperVBaseTestCase):
super(VolumeOpsTestCase, self).setUp()
self._volumeops = volumeops.VolumeOps()
self._volumeops._volutils = mock.MagicMock()
self._volumeops._vmutils = mock.Mock()
def test_get_volume_driver(self):
fake_conn_info = {'driver_volume_type': mock.sentinel.fake_driver_type}
@ -83,30 +84,53 @@ class VolumeOpsTestCase(test_base.HyperVBaseTestCase):
block_device_info['block_device_mapping'][0]['connection_info'],
mock.sentinel.instance_name, True)
def test_fix_instance_volume_disk_paths(self):
def test_fix_instance_volume_disk_paths_empty_bdm(self):
self._volumeops.fix_instance_volume_disk_paths(
mock.sentinel.instance_name,
block_device_info={})
self.assertFalse(
self._volumeops._vmutils.get_vm_physical_disk_mapping.called)
@mock.patch.object(volumeops.VolumeOps, 'get_disk_path_mapping')
def test_fix_instance_volume_disk_paths(self, mock_get_disk_path_mapping):
block_device_info = get_fake_block_dev_info()
fake_vol_conn_info = (
block_device_info['block_device_mapping'][0]['connection_info'])
with test.nested(
mock.patch.object(self._volumeops,
'_get_volume_driver'),
mock.patch.object(self._volumeops,
'ebs_root_in_block_devices')
) as (mock_get_volume_driver,
mock_ebs_in_block_devices):
mock_disk1 = {
'mounted_disk_path': mock.sentinel.mounted_disk1_path,
'resource_path': mock.sentinel.resource1_path
}
mock_disk2 = {
'mounted_disk_path': mock.sentinel.mounted_disk2_path,
'resource_path': mock.sentinel.resource2_path
}
fake_vol_driver = mock_get_volume_driver.return_value
mock_ebs_in_block_devices.return_value = False
mock_vm_disk_mapping = {
mock.sentinel.disk1_serial: mock_disk1,
mock.sentinel.disk2_serial: mock_disk2
}
# In this case, only the first disk needs to be updated.
mock_phys_disk_path_mapping = {
mock.sentinel.disk1_serial: mock.sentinel.actual_disk1_path,
mock.sentinel.disk2_serial: mock.sentinel.mounted_disk2_path
}
self._volumeops.fix_instance_volume_disk_paths(
mock.sentinel.instance_name,
block_device_info)
vmutils = self._volumeops._vmutils
vmutils.get_vm_physical_disk_mapping.return_value = (
mock_vm_disk_mapping)
func = fake_vol_driver.fix_instance_volume_disk_path
func.assert_called_once_with(
mock.sentinel.instance_name,
fake_vol_conn_info, 0)
mock_get_disk_path_mapping.return_value = mock_phys_disk_path_mapping
self._volumeops.fix_instance_volume_disk_paths(
mock.sentinel.instance_name,
block_device_info)
vmutils.get_vm_physical_disk_mapping.assert_called_once_with(
mock.sentinel.instance_name)
mock_get_disk_path_mapping.assert_called_once_with(
block_device_info)
vmutils.set_disk_host_res.assert_called_once_with(
mock.sentinel.resource1_path,
mock.sentinel.actual_disk1_path)
@mock.patch.object(volumeops.VolumeOps, '_get_volume_driver')
def test_disconnect_volumes(self, mock_get_volume_driver):
@ -153,6 +177,26 @@ class VolumeOpsTestCase(test_base.HyperVBaseTestCase):
init_vol_conn.assert_called_once_with(
block_device_info['block_device_mapping'][0]['connection_info'])
@mock.patch.object(volumeops.VolumeOps,
'get_mounted_disk_path_from_volume')
def test_get_disk_path_mapping(self, mock_get_disk_path):
block_device_info = get_fake_block_dev_info()
block_device_mapping = block_device_info['block_device_mapping']
fake_conn_info = get_fake_connection_info()
block_device_mapping[0]['connection_info'] = fake_conn_info
mock_get_disk_path.return_value = mock.sentinel.disk_path
resulted_disk_path_mapping = self._volumeops.get_disk_path_mapping(
block_device_info)
mock_get_disk_path.assert_called_once_with(fake_conn_info)
expected_disk_path_mapping = {
mock.sentinel.serial: mock.sentinel.disk_path
}
self.assertEqual(expected_disk_path_mapping,
resulted_disk_path_mapping)
def test_group_block_devices_by_type(self):
block_device_map = get_fake_block_dev_info()['block_device_mapping']
block_device_map[0]['connection_info'] = {
@ -163,6 +207,21 @@ class VolumeOpsTestCase(test_base.HyperVBaseTestCase):
expected = {'iscsi': [block_device_map[0]]}
self.assertEqual(expected, result)
@mock.patch.object(volumeops.VolumeOps, '_get_volume_driver')
def test_get_mounted_disk_path_from_volume(self, mock_get_volume_driver):
fake_conn_info = get_fake_connection_info()
fake_volume_driver = mock_get_volume_driver.return_value
resulted_disk_path = self._volumeops.get_mounted_disk_path_from_volume(
fake_conn_info)
mock_get_volume_driver.assert_called_once_with(
connection_info=fake_conn_info)
get_mounted_disk = fake_volume_driver.get_mounted_disk_path_from_volume
get_mounted_disk.assert_called_once_with(fake_conn_info)
self.assertEqual(get_mounted_disk.return_value,
resulted_disk_path)
class ISCSIVolumeDriverTestCase(test_base.HyperVBaseTestCase):
"""Unit tests for Hyper-V ISCSIVolumeDriver class."""
@ -230,6 +289,22 @@ class ISCSIVolumeDriverTestCase(test_base.HyperVBaseTestCase):
def test_logout_storage_target(self):
self._check_logout_storage_target(disconnected_luns_count=1)
@mock.patch.object(volumeops.ISCSIVolumeDriver,
'_get_mounted_disk_from_lun')
def test_get_mounted_disk_path_from_volume(self,
mock_get_mounted_disk_from_lun):
connection_info = get_fake_connection_info()
resulted_disk_path = (
self._volume_driver.get_mounted_disk_path_from_volume(
connection_info))
mock_get_mounted_disk_from_lun.assert_called_once_with(
connection_info['data']['target_iqn'],
connection_info['data']['target_lun'],
wait_for_device=True)
self.assertEqual(mock_get_mounted_disk_from_lun.return_value,
resulted_disk_path)
@mock.patch.object(volumeops.ISCSIVolumeDriver,
'_get_mounted_disk_from_lun')
@mock.patch.object(volumeops.ISCSIVolumeDriver, 'logout_storage_target')
@ -269,7 +344,9 @@ class ISCSIVolumeDriverTestCase(test_base.HyperVBaseTestCase):
mock_login_storage_target.assert_called_once_with(connection_info)
mock_get_mounted_disk_from_lun.assert_called_once_with(
mock.sentinel.fake_iqn, mock.sentinel.fake_lun)
mock.sentinel.fake_iqn,
mock.sentinel.fake_lun,
wait_for_device=True)
if ebs_root:
get_ide_path.assert_called_once_with(
mock.sentinel.instance_name, 0)
@ -302,7 +379,9 @@ class ISCSIVolumeDriverTestCase(test_base.HyperVBaseTestCase):
mock.sentinel.instance_name)
mock_get_mounted_disk_from_lun.assert_called_once_with(
mock.sentinel.fake_iqn, mock.sentinel.fake_lun)
mock.sentinel.fake_iqn,
mock.sentinel.fake_lun,
wait_for_device=True)
self._volume_driver._vmutils.detach_vm_disk.assert_called_once_with(
mock.sentinel.instance_name,
mock_get_mounted_disk_from_lun.return_value)
@ -337,28 +416,6 @@ class ISCSIVolumeDriverTestCase(test_base.HyperVBaseTestCase):
mock.sentinel.physical_drive_path)
self.assertEqual(mock_get_target.return_value, result)
@mock.patch.object(volumeops.ISCSIVolumeDriver,
'_get_mounted_disk_from_lun')
def test_fix_instance_volume_disk_path(self, mock_get_disk_from_lun):
connection_info = get_fake_connection_info()
set_disk_host_res = self._volume_driver._vmutils.set_disk_host_resource
get_scsi_ctrl = self._volume_driver._vmutils.get_vm_scsi_controller
get_scsi_ctrl.return_value = mock.sentinel.controller_path
mock_get_disk_from_lun.return_value = mock.sentinel.mounted_path
self._volume_driver.fix_instance_volume_disk_path(
mock.sentinel.instance_name,
connection_info,
mock.sentinel.disk_address)
mock_get_disk_from_lun.assert_called_once_with(
mock.sentinel.fake_iqn, mock.sentinel.fake_lun, True)
get_scsi_ctrl.assert_called_once_with(mock.sentinel.instance_name)
set_disk_host_res.assert_called_once_with(
mock.sentinel.instance_name, mock.sentinel.controller_path,
mock.sentinel.disk_address, mock.sentinel.mounted_path)
@mock.patch('time.sleep')
def test_get_mounted_disk_from_lun_failure(self, fake_sleep):
self.flags(mounted_disk_query_retry_count=1, group='hyperv')
@ -422,6 +479,15 @@ class SMBFSVolumeDriverTestCase(test_base.HyperVBaseTestCase):
self._volume_driver._pathutils = mock.MagicMock()
self._volume_driver._volutils = mock.MagicMock()
@mock.patch.object(volumeops.SMBFSVolumeDriver,
'_get_disk_path')
def test_get_mounted_disk_path_from_volume(self, mock_get_disk_path):
disk_path = self._volume_driver.get_mounted_disk_path_from_volume(
mock.sentinel.conn_info)
self.assertEqual(mock_get_disk_path.return_value, disk_path)
mock_get_disk_path.assert_called_once_with(mock.sentinel.conn_info)
@mock.patch.object(volumeops.SMBFSVolumeDriver, 'ensure_share_mounted')
@mock.patch.object(volumeops.SMBFSVolumeDriver, '_get_disk_path')
def _check_attach_volume(self, mock_get_disk_path,

90
nova/virt/hyperv/volumeops.py
View File

@ -116,19 +116,23 @@ class VolumeOps(object):
block_device_info)
def fix_instance_volume_disk_paths(self, instance_name, block_device_info):
mapping = driver.block_device_info_get_mapping(block_device_info)
# Mapping containing the current disk paths for each volume.
actual_disk_mapping = self.get_disk_path_mapping(block_device_info)
if not actual_disk_mapping:
return
if self.ebs_root_in_block_devices(block_device_info):
mapping = mapping[1:]
# Mapping containing virtual disk resource path and the physical
# disk path for each volume serial number. The physical path
# associated with this resource may not be the right one,
# as physical disk paths can get swapped after host reboots.
vm_disk_mapping = self._vmutils.get_vm_physical_disk_mapping(
instance_name)
disk_address = 0
for vol in mapping:
connection_info = vol['connection_info']
volume_driver = self._get_volume_driver(
connection_info=connection_info)
volume_driver.fix_instance_volume_disk_path(
instance_name, connection_info, disk_address)
disk_address += 1
for serial, vm_disk in vm_disk_mapping.items():
actual_disk_path = actual_disk_mapping[serial]
if vm_disk['mounted_disk_path'] != actual_disk_path:
self._vmutils.set_disk_host_res(vm_disk['resource_path'],
actual_disk_path)
def get_volume_connector(self, instance):
if not self._initiator:
@ -150,6 +154,17 @@ class VolumeOps(object):
connection_info=connection_info)
volume_driver.initialize_volume_connection(connection_info)
def get_disk_path_mapping(self, block_device_info):
block_mapping = driver.block_device_info_get_mapping(block_device_info)
disk_path_mapping = {}
for vol in block_mapping:
connection_info = vol['connection_info']
disk_serial = connection_info['serial']
disk_path = self.get_mounted_disk_path_from_volume(connection_info)
disk_path_mapping[disk_serial] = disk_path
return disk_path_mapping
def _group_block_devices_by_type(self, block_device_mapping):
block_devices = collections.defaultdict(list)
for volume in block_device_mapping:
@ -158,6 +173,12 @@ class VolumeOps(object):
block_devices[volume_type].append(volume)
return block_devices
def get_mounted_disk_path_from_volume(self, connection_info):
volume_driver = self._get_volume_driver(
connection_info=connection_info)
return volume_driver.get_mounted_disk_path_from_volume(
connection_info)
class ISCSIVolumeDriver(object):
def __init__(self):
@ -220,6 +241,15 @@ class ISCSIVolumeDriver(object):
LOG.debug("Skipping disconnecting target %s as there "
"are LUNs still being used.", target_iqn)
def get_mounted_disk_path_from_volume(self, connection_info):
data = connection_info['data']
target_lun = data['target_lun']
target_iqn = data['target_iqn']
# Getting the mounted disk
return self._get_mounted_disk_from_lun(target_iqn, target_lun,
wait_for_device=True)
def attach_volume(self, connection_info, instance_name, ebs_root=False):
"""Attach a volume to the SCSI controller or to the IDE controller if
ebs_root is True
@ -231,14 +261,10 @@ class ISCSIVolumeDriver(object):
try:
self.login_storage_target(connection_info)
data = connection_info['data']
target_lun = data['target_lun']
target_iqn = data['target_iqn']
serial = connection_info['serial']
# Getting the mounted disk
mounted_disk_path = self._get_mounted_disk_from_lun(target_iqn,
target_lun)
mounted_disk_path = self.get_mounted_disk_path_from_volume(
connection_info)
if ebs_root:
# Find the IDE controller for the vm.
@ -261,6 +287,7 @@ class ISCSIVolumeDriver(object):
with excutils.save_and_reraise_exception():
LOG.error(_LE('Unable to attach volume to instance %s'),
instance_name)
target_iqn = connection_info['data']['target_iqn']
if target_iqn:
self.logout_storage_target(target_iqn)
@ -271,13 +298,9 @@ class ISCSIVolumeDriver(object):
{'connection_info': connection_info,
'instance_name': instance_name})
data = connection_info['data']
target_lun = data['target_lun']
target_iqn = data['target_iqn']
# Getting the mounted disk
mounted_disk_path = self._get_mounted_disk_from_lun(target_iqn,
target_lun)
target_iqn = connection_info['data']['target_iqn']
mounted_disk_path = self.get_mounted_disk_path_from_volume(
connection_info)
LOG.debug("Detaching physical disk from instance: %s",
mounted_disk_path)
@ -327,18 +350,6 @@ class ISCSIVolumeDriver(object):
def get_target_from_disk_path(self, physical_drive_path):
return self._volutils.get_target_from_disk_path(physical_drive_path)
def fix_instance_volume_disk_path(self, instance_name, connection_info,
disk_address):
data = connection_info['data']
target_lun = data['target_lun']
target_iqn = data['target_iqn']
mounted_disk_path = self._get_mounted_disk_from_lun(
target_iqn, target_lun, True)
ctrller_path = self._vmutils.get_vm_scsi_controller(instance_name)
self._vmutils.set_disk_host_resource(
instance_name, ctrller_path, disk_address, mounted_disk_path)
def get_target_lun_count(self, target_iqn):
return self._volutils.get_target_lun_count(target_iqn)
@ -364,6 +375,9 @@ class SMBFSVolumeDriver(object):
self._username_regex = re.compile(r'user(?:name)?=([^, ]+)')
self._password_regex = re.compile(r'pass(?:word)?=([^, ]+)')
def get_mounted_disk_path_from_volume(self, connection_info):
return self._get_disk_path(connection_info)
@export_path_synchronized
def attach_volume(self, connection_info, instance_name, ebs_root=False):
self.ensure_share_mounted(connection_info)
@ -443,10 +457,6 @@ class SMBFSVolumeDriver(object):
return username, password
def fix_instance_volume_disk_path(self, instance_name, connection_info,
disk_address):
self.ensure_share_mounted(connection_info)
def initialize_volume_connection(self, connection_info):
self.ensure_share_mounted(connection_info)