Merge "HyperV: Fix vm disk path issue"

2 files changed, 163 insertions, 87 deletions

diff --git a/nova/tests/unit/virt/hyperv/test_volumeops.py b/nova/tests/unit/virt/hyperv/test_volumeops.py
index 186094bfbc..f4a8401f1c 100644
--- a/nova/tests/unit/virt/hyperv/test_volumeops.py
+++ b/nova/tests/unit/virt/hyperv/test_volumeops.py
@@ -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):
-        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):
+    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)
 
-            fake_vol_driver = mock_get_volume_driver.return_value
-            mock_ebs_in_block_devices.return_value = False
+    @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()
 
-            self._volumeops.fix_instance_volume_disk_paths(
-                mock.sentinel.instance_name,
-                block_device_info)
+        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
+        }
+
+        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
+        }
+
+        vmutils = self._volumeops._vmutils
+        vmutils.get_vm_physical_disk_mapping.return_value = (
+            mock_vm_disk_mapping)
+
+        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)
 
-            func = fake_vol_driver.fix_instance_volume_disk_path
-            func.assert_called_once_with(
-                mock.sentinel.instance_name,
-                fake_vol_conn_info, 0)
+        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."""
@@ -232,6 +291,22 @@ class ISCSIVolumeDriverTestCase(test_base.HyperVBaseTestCase):
 
     @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')
     @mock.patch.object(volumeops.ISCSIVolumeDriver, 'login_storage_target')
     def test_attach_volume_exception(self, mock_login_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._smbutils = 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,
diff --git a/nova/virt/hyperv/volumeops.py b/nova/virt/hyperv/volumeops.py
index 3dbbd57906..f3ad30eda4 100644
--- a/nova/virt/hyperv/volumeops.py
+++ b/nova/virt/hyperv/volumeops.py
@@ -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)
-
-        if self.ebs_root_in_block_devices(block_device_info):
-            mapping = mapping[1:]
-
-        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
+        # 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
+
+        # 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)
+
+        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)