diff --git a/compute_hyperv/nova/migrationops.py b/compute_hyperv/nova/migrationops.py index 3e22d61e..700deb2c 100644 --- a/compute_hyperv/nova/migrationops.py +++ b/compute_hyperv/nova/migrationops.py @@ -19,9 +19,11 @@ Management class for migration / resize operations. import os import re +from nova import block_device import nova.conf from nova import exception from nova.virt import configdrive +from nova.virt import driver from os_win import utilsfactory from oslo_config import cfg from oslo_log import log as logging @@ -77,11 +79,21 @@ class MigrationOps(object): return revert_path - def _check_target_flavor(self, instance, flavor): + def _check_target_flavor(self, instance, flavor, block_device_info): + ephemerals = driver.block_device_info_get_ephemerals(block_device_info) + eph_size = (block_device.get_bdm_ephemeral_disk_size(ephemerals) or + instance.flavor.ephemeral_gb) + new_root_gb = flavor.root_gb curr_root_gb = instance.flavor.root_gb + new_eph_size = flavor.ephemeral_gb - if new_root_gb < curr_root_gb: + root_down = new_root_gb < curr_root_gb + ephemeral_down = new_eph_size < eph_size + booted_from_volume = self._block_dev_man.is_boot_from_volume( + block_device_info) + + if root_down and not booted_from_volume: raise exception.InstanceFaultRollback( exception.CannotResizeDisk( reason=_("Cannot resize the root disk to a smaller size. " @@ -89,6 +101,16 @@ class MigrationOps(object): "size: %(new_root_gb)s GB.") % { 'curr_root_gb': curr_root_gb, 'new_root_gb': new_root_gb})) + # We allow having a new flavor with no ephemeral storage, in which + # case we'll just remove all the ephemeral disks. + elif ephemeral_down and new_eph_size: + reason = (_("The new flavor ephemeral size (%(flavor_eph)s) is " + "smaller than the current total ephemeral disk size: " + "%(current_eph)s.") % + dict(flavor_eph=flavor.ephemeral_gb, + current_eph=eph_size)) + raise exception.InstanceFaultRollback( + exception.CannotResizeDisk(reason=reason)) def migrate_disk_and_power_off(self, context, instance, dest, flavor, network_info, @@ -96,7 +118,7 @@ class MigrationOps(object): retry_interval=0): LOG.debug("migrate_disk_and_power_off called", instance=instance) - self._check_target_flavor(instance, flavor) + self._check_target_flavor(instance, flavor, block_device_info) self._vmops.power_off(instance, timeout, retry_interval) instance_path = self._move_vm_files(instance) @@ -294,6 +316,13 @@ class MigrationOps(object): self._migrationutils.realize_vm(instance.name) + # During a resize, ephemeral disks may be removed. We cannot remove + # disks from a planned vm, for which reason we have to do this after + # *realizing* it. At the same time, we cannot realize a VM before + # updating disks to use the destination paths. + ephemerals = block_device_info['ephemerals'] + self._check_ephemeral_disks(instance, ephemerals, resize_instance) + self._vmops.configure_remotefx(instance, vm_gen, resize_instance) if CONF.hyperv.enable_instance_metrics_collection: self._metricsutils.enable_vm_metrics_collection(instance.name) @@ -366,13 +395,20 @@ class MigrationOps(object): new_size = instance.flavor.root_gb * units.Gi self._check_resize_vhd(root_vhd_path, root_vhd_info, new_size) - ephemerals = block_device_info['ephemerals'] - self._check_ephemeral_disks(instance, ephemerals, resize_instance) - def _check_ephemeral_disks(self, instance, ephemerals, resize_instance=False): instance_name = instance.name new_eph_gb = instance.get('ephemeral_gb', 0) + ephemerals_to_remove = set() + + if not ephemerals and new_eph_gb: + # No explicit ephemeral disk bdm was retrieved, yet the flavor + # provides ephemeral storage, for which reason we're adding a + # default ephemeral disk. + eph = dict(device_type='disk', + drive_addr=0, + size=new_eph_gb) + ephemerals.append(eph) if len(ephemerals) == 1: # NOTE(claudiub): Resize only if there is one ephemeral. If there @@ -380,7 +416,8 @@ class MigrationOps(object): # also exists in the libvirt driver and it has to be addressed in # the future. ephemerals[0]['size'] = new_eph_gb - elif sum(eph['size'] for eph in ephemerals) != new_eph_gb: + elif new_eph_gb and sum( + eph['size'] for eph in ephemerals) != new_eph_gb: # New ephemeral size is different from the original ephemeral size # and there are multiple ephemerals. LOG.warning("Cannot resize multiple ephemeral disks for instance.", @@ -391,7 +428,7 @@ class MigrationOps(object): existing_eph_path = self._pathutils.lookup_ephemeral_vhd_path( instance_name, eph_name) - if not existing_eph_path: + if not existing_eph_path and eph['size']: eph['format'] = self._vhdutils.get_best_supported_vhd_format() eph['path'] = self._pathutils.get_ephemeral_vhd_path( instance_name, eph['format'], eph_name) @@ -399,10 +436,24 @@ class MigrationOps(object): # ephemerals should have existed. raise exception.DiskNotFound(location=eph['path']) - if eph['size']: - # create ephemerals - self._vmops.create_ephemeral_disk(instance.name, eph) - self._vmops.attach_ephemerals(instance_name, [eph]) + # We cannot rely on the BlockDeviceInfoManager class to + # provide us a disk slot as it's only usable when creating + # new instances (it's not aware of the current disk address + # layout). + # There's no way in which IDE may be requested for new + # ephemeral disks (after a resize), so we'll just enforce + # SCSI for now. os-win does not currently allow retrieving + # free IDE slots. + ctrller_path = self._vmutils.get_vm_scsi_controller( + instance.name) + ctrl_addr = self._vmutils.get_free_controller_slot( + ctrller_path) + eph['disk_bus'] = constants.CTRL_TYPE_SCSI + eph['ctrl_disk_addr'] = ctrl_addr + + # create ephemerals + self._vmops.create_ephemeral_disk(instance.name, eph) + self._vmops.attach_ephemerals(instance_name, [eph]) elif eph['size'] > 0: # ephemerals exist. resize them. eph['path'] = existing_eph_path @@ -410,6 +461,18 @@ class MigrationOps(object): self._check_resize_vhd( eph['path'], eph_vhd_info, eph['size'] * units.Gi) else: - # ephemeral new size is 0, remove it. - self._pathutils.remove(existing_eph_path) eph['path'] = None + # ephemeral new size is 0, remove it. + ephemerals_to_remove.add(existing_eph_path) + + if not new_eph_gb: + # The new flavor does not provide any ephemeral storage. We'll + # remove any existing ephemeral disk (default ones included). + attached_ephemerals = self._vmops.get_attached_ephemeral_disks( + instance.name) + ephemerals_to_remove |= set(attached_ephemerals) + + for eph_path in ephemerals_to_remove: + self._vmutils.detach_vm_disk(instance_name, eph_path, + is_physical=False) + self._pathutils.remove(eph_path) diff --git a/compute_hyperv/nova/vmops.py b/compute_hyperv/nova/vmops.py index 0990a7b4..f1caa848 100644 --- a/compute_hyperv/nova/vmops.py +++ b/compute_hyperv/nova/vmops.py @@ -237,6 +237,12 @@ class VMOps(object): self._vhdutils.create_dynamic_vhd(eph_info['path'], eph_info['size'] * units.Gi) + def get_attached_ephemeral_disks(self, instance_name): + vm_image_disks = self._vmutils.get_vm_storage_paths( + instance_name)[0] + return [image_path for image_path in vm_image_disks + if os.path.basename(image_path).lower().startswith('eph')] + @staticmethod def _get_vif_metadata(context, instance_id): vifs = objects.VirtualInterfaceList.get_by_instance_uuid(context, @@ -611,9 +617,13 @@ class VMOps(object): constants.BDI_DEVICE_TYPE_TO_DRIVE_TYPE[ eph['device_type']]) - filename = os.path.basename(eph['path']) - self._block_dev_man.update_bdm_connection_info( - eph._bdm_obj, eph_filename=filename) + # This may be an ephemeral added by default by us, in which + # case there won't be a bdm object. + bdm_obj = getattr(eph, '_bdm_obj', None) + if bdm_obj: + filename = os.path.basename(eph['path']) + self._block_dev_man.update_bdm_connection_info( + eph._bdm_obj, eph_filename=filename) def _attach_drive(self, instance_name, path, drive_addr, ctrl_disk_addr, controller_type, drive_type=constants.DISK): diff --git a/compute_hyperv/tests/unit/test_migrationops.py b/compute_hyperv/tests/unit/test_migrationops.py index 2301d6a8..44171e63 100644 --- a/compute_hyperv/tests/unit/test_migrationops.py +++ b/compute_hyperv/tests/unit/test_migrationops.py @@ -16,7 +16,9 @@ import os import ddt import mock +from nova import block_device from nova import exception +from nova.virt import driver from os_win import exceptions as os_win_exc from oslo_utils import units @@ -82,13 +84,43 @@ class MigrationOpsTestCase(test_base.HyperVBaseTestCase): mock_instance.name, mock_get_export_dir.return_value) self.assertEqual(mock_get_revert_dir.return_value, vm_files_path) - def test_check_target_flavor(self): + @ddt.data({}, + {'ephemerals_size': 2}, + {'ephemerals_size': 3, 'flavor_eph_size': 0}, + {'ephemerals_size': 3, 'expect_invalid_flavor': True}, + {'current_root_gb': 3, 'expect_invalid_flavor': True}, + {'current_root_gb': 3, 'boot_from_vol': True}) + @ddt.unpack + @mock.patch.object(driver, 'block_device_info_get_ephemerals') + @mock.patch.object(block_device, 'get_bdm_ephemeral_disk_size') + def test_check_target_flavor(self, mock_get_eph_size, mock_get_eph, + ephemerals_size=0, + flavor_eph_size=2, + flavor_root_gb=2, + current_root_gb=1, + boot_from_vol=False, + expect_invalid_flavor=False): mock_instance = fake_instance.fake_instance_obj(self.context) - mock_instance.flavor.root_gb = 1 - mock_flavor = mock.MagicMock(root_gb=0) - self.assertRaises(exception.InstanceFaultRollback, - self._migrationops._check_target_flavor, - mock_instance, mock_flavor) + mock_instance.flavor.root_gb = current_root_gb + mock_flavor = mock.MagicMock(root_gb=flavor_root_gb, + ephemeral_gb=flavor_eph_size) + + mock_get_eph_size.return_value = ephemerals_size + self._block_dev_man.is_boot_from_volume.return_value = boot_from_vol + + if expect_invalid_flavor: + self.assertRaises(exception.InstanceFaultRollback, + self._migrationops._check_target_flavor, + mock_instance, mock_flavor, + mock.sentinel.block_device_info) + else: + self._migrationops._check_target_flavor( + mock_instance, mock_flavor, mock.sentinel.block_device_info) + + mock_get_eph.assert_called_once_with(mock.sentinel.block_device_info) + mock_get_eph_size.assert_called_once_with(mock_get_eph.return_value) + self._block_dev_man.is_boot_from_volume.assert_called_once_with( + mock.sentinel.block_device_info) def test_check_and_attach_config_drive(self): mock_instance = fake_instance.fake_instance_obj( @@ -129,7 +161,8 @@ class MigrationOpsTestCase(test_base.HyperVBaseTestCase): self._FAKE_TIMEOUT, self._FAKE_RETRY_INTERVAL) self.assertEqual(mock_move_vm_files.return_value, disk_info) - mock_check_flavor.assert_called_once_with(instance, flavor) + mock_check_flavor.assert_called_once_with( + instance, flavor, mock.sentinel.bdi) self._migrationops._vmops.power_off.assert_called_once_with( instance, self._FAKE_TIMEOUT, self._FAKE_RETRY_INTERVAL) mock_move_vm_files.assert_called_once_with(instance) @@ -343,19 +376,22 @@ class MigrationOpsTestCase(test_base.HyperVBaseTestCase): self._vmops.power_on.assert_called_once_with( mock_instance, network_info=mock.sentinel.network_info) + @mock.patch.object(migrationops.MigrationOps, '_check_ephemeral_disks') @mock.patch.object(migrationops.MigrationOps, '_check_and_update_disks') @mock.patch.object(migrationops.MigrationOps, '_update_disk_image_paths') @mock.patch.object(migrationops.MigrationOps, '_import_vm') def test_import_and_setup_vm(self, mock_import_vm, mock_update_disk_image_paths, - mock_check_and_update_disks): + mock_check_and_update_disks, + mock_check_eph_disks): self.flags(enable_instance_metrics_collection=True, group='hyperv') + block_device_info = {'ephemerals': mock.sentinel.ephemerals} mock_instance = fake_instance.fake_instance_obj(self.context) self._migrationops._import_and_setup_vm( self.context, mock_instance, mock.sentinel.instance_dir, - mock.sentinel.image_meta, mock.sentinel.block_device_info, + mock.sentinel.image_meta, block_device_info, resize_instance=mock.sentinel.resize_instance) get_image_vm_gen = self._vmops.get_image_vm_generation @@ -367,17 +403,20 @@ class MigrationOpsTestCase(test_base.HyperVBaseTestCase): mock.sentinel.image_meta, mock.sentinel.instance_dir, mock.sentinel.resize_instance) self._migrationops._volumeops.connect_volumes.assert_called_once_with( - mock.sentinel.block_device_info) + block_device_info) mock_update_disk_image_paths.assert_called_once_with( mock_instance, mock.sentinel.instance_dir) mock_check_and_update_disks.assert_called_once_with( self.context, mock_instance, get_image_vm_gen.return_value, - mock.sentinel.image_meta, mock.sentinel.block_device_info, + mock.sentinel.image_meta, block_device_info, resize_instance=mock.sentinel.resize_instance) self._volumeops.fix_instance_volume_disk_paths.assert_called_once_with( - mock_instance.name, mock.sentinel.block_device_info) + mock_instance.name, block_device_info) self._migrationops._migrationutils.realize_vm.assert_called_once_with( mock_instance.name) + mock_check_eph_disks.assert_called_once_with( + mock_instance, mock.sentinel.ephemerals, + mock.sentinel.resize_instance) self._migrationops._vmops.configure_remotefx.assert_called_once_with( mock_instance, get_image_vm_gen.return_value, mock.sentinel.resize_instance) @@ -453,9 +492,7 @@ class MigrationOpsTestCase(test_base.HyperVBaseTestCase): @ddt.data(constants.DISK, mock.sentinel.root_type) @mock.patch.object(migrationops.MigrationOps, '_check_base_disk') @mock.patch.object(migrationops.MigrationOps, '_check_resize_vhd') - @mock.patch.object(migrationops.MigrationOps, '_check_ephemeral_disks') def test_check_and_update_disks(self, root_type, - mock_check_eph_disks, mock_check_resize_vhd, mock_check_base_disk): mock_instance = fake_instance.fake_instance_obj(self.context) @@ -492,8 +529,6 @@ class MigrationOpsTestCase(test_base.HyperVBaseTestCase): mock_instance.flavor.root_gb * units.Gi)) else: self.assertFalse(self._pathutils.lookup_root_vhd.called) - mock_check_eph_disks.assert_called_once_with( - mock_instance, mock.sentinel.ephemerals, True) mock_check_resize_vhd.assert_has_calls(expected_check_resize) self._vhdutils.get_vhd_info.assert_has_calls( @@ -532,8 +567,9 @@ class MigrationOpsTestCase(test_base.HyperVBaseTestCase): instance=mock_instance) def test_check_ephemeral_disks_exception(self): - mock_instance = fake_instance.fake_instance_obj(self.context) - mock_ephemerals = [dict()] + mock_instance = fake_instance.fake_instance_obj(self.context, + ephemeral_gb=1) + mock_ephemerals = [dict(size=1)] lookup_eph_path = ( self._migrationops._pathutils.lookup_ephemeral_vhd_path) @@ -543,13 +579,22 @@ class MigrationOpsTestCase(test_base.HyperVBaseTestCase): self._migrationops._check_ephemeral_disks, mock_instance, mock_ephemerals) + @ddt.data({}, + {'existing_eph_path': mock.sentinel.eph_path}, + {'existing_eph_path': mock.sentinel.eph_path, + 'new_eph_size': 0}, + {'use_default_eph': True}) + @ddt.unpack @mock.patch.object(migrationops.MigrationOps, '_check_resize_vhd') - def _test_check_ephemeral_disks(self, mock_check_resize_vhd, - existing_eph_path=None, new_eph_size=42): + def test_check_ephemeral_disks(self, mock_check_resize_vhd, + existing_eph_path=None, new_eph_size=42, + use_default_eph=False): + mock_vmops = self._migrationops._vmops + mock_instance = fake_instance.fake_instance_obj(self.context) mock_instance.ephemeral_gb = new_eph_size eph = {} - mock_ephemerals = [eph] + mock_ephemerals = [eph] if not use_default_eph else [] mock_pathutils = self._migrationops._pathutils lookup_eph_path = mock_pathutils.lookup_ephemeral_vhd_path @@ -561,17 +606,36 @@ class MigrationOpsTestCase(test_base.HyperVBaseTestCase): mock_get_vhd_format = mock_vhdutils.get_best_supported_vhd_format mock_get_vhd_format.return_value = mock.sentinel.vhd_format + self._vmutils.get_free_controller_slot.return_value = ( + mock.sentinel.ctrl_slot) + + attached_eph_paths = [mock.sentinel.eph_path, + mock.sentinel.default_eph_path] + mock_vmops.get_attached_ephemeral_disks.return_value = ( + attached_eph_paths) + self._migrationops._check_ephemeral_disks(mock_instance, mock_ephemerals, True) - self.assertEqual(mock_instance.ephemeral_gb, eph['size']) + if not use_default_eph: + self.assertEqual(mock_instance.ephemeral_gb, eph['size']) if not existing_eph_path: - mock_vmops = self._migrationops._vmops mock_vmops.create_ephemeral_disk.assert_called_once_with( - mock_instance.name, eph) - self.assertEqual(mock.sentinel.vhd_format, eph['format']) - self.assertEqual(mock.sentinel.get_path, eph['path']) + mock_instance.name, mock.ANY) + self._vmutils.get_vm_scsi_controller.assert_called_once_with( + mock_instance.name) + self._vmutils.get_free_controller_slot.assert_called_once_with( + self._vmutils.get_vm_scsi_controller.return_value) + + create_eph_args = mock_vmops.create_ephemeral_disk.call_args_list + created_eph = create_eph_args[0][0][1] + self.assertEqual(mock.sentinel.vhd_format, created_eph['format']) + self.assertEqual(mock.sentinel.get_path, created_eph['path']) + self.assertEqual(constants.CTRL_TYPE_SCSI, + created_eph['disk_bus']) + self.assertEqual(mock.sentinel.ctrl_slot, + created_eph['ctrl_disk_addr']) elif new_eph_size: mock_check_resize_vhd.assert_called_once_with( existing_eph_path, @@ -579,15 +643,11 @@ class MigrationOpsTestCase(test_base.HyperVBaseTestCase): mock_instance.ephemeral_gb * units.Gi) self.assertEqual(existing_eph_path, eph['path']) else: - self._migrationops._pathutils.remove.assert_called_once_with( - existing_eph_path) - - def test_check_ephemeral_disks_create(self): - self._test_check_ephemeral_disks() - - def test_check_ephemeral_disks_resize(self): - self._test_check_ephemeral_disks(existing_eph_path=mock.sentinel.path) - - def test_check_ephemeral_disks_remove(self): - self._test_check_ephemeral_disks(existing_eph_path=mock.sentinel.path, - new_eph_size=0) + self._vmutils.detach_vm_disk.assert_has_calls( + [mock.call(mock_instance.name, eph_path, + is_physical=False) + for eph_path in attached_eph_paths], + any_order=True) + self._migrationops._pathutils.remove.assert_has_calls( + [mock.call(eph_path) for eph_path in attached_eph_paths], + any_order=True) diff --git a/compute_hyperv/tests/unit/test_vmops.py b/compute_hyperv/tests/unit/test_vmops.py index 686972b0..ba989c8a 100644 --- a/compute_hyperv/tests/unit/test_vmops.py +++ b/compute_hyperv/tests/unit/test_vmops.py @@ -356,6 +356,22 @@ class VMOpsTestCase(test_base.HyperVBaseTestCase): mock_create_dynamic_vhd.assert_called_once_with('fake_eph_path', 10 * units.Gi) + def test_get_attached_ephemeral_disks(self): + ephemeral_disks = [os.path.join('image_dir', img_name) + for img_name in ['eph0.vhdx', 'eph1.vhdx']] + image_disks = ephemeral_disks + [ + os.path.join('image_dir', 'root.vhdx')] + + self._vmutils.get_vm_storage_paths.return_value = ( + image_disks, mock.sentinel.passthrough_disks) + + ret_val = self._vmops.get_attached_ephemeral_disks( + mock.sentinel.instance_name) + + self.assertEqual(ephemeral_disks, ret_val) + self._vmutils.get_vm_storage_paths.assert_called_once_with( + mock.sentinel.instance_name) + @mock.patch.object(vmops.objects, 'PCIDeviceBus') @mock.patch.object(vmops.objects, 'NetworkInterfaceMetadata') @mock.patch.object(vmops.objects.VirtualInterfaceList, @@ -868,7 +884,9 @@ class VMOpsTestCase(test_base.HyperVBaseTestCase): 'drive_addr': 0, 'ctrl_disk_addr': 0}, {'path': None}] - ephemerals = [FakeBDM(eph) for eph in ephemerals] + ephemerals = [FakeBDM(ephemerals[0]), + ephemerals[1], + FakeBDM(ephemerals[2])] self._vmops.attach_ephemerals(mock_instance.name, ephemerals) @@ -880,10 +898,9 @@ class VMOpsTestCase(test_base.HyperVBaseTestCase): ]) mock_update_conn = ( self._vmops._block_dev_man.update_bdm_connection_info) - mock_update_conn.assert_has_calls( - [mock.call(mock.sentinel.bdm_obj, - eph_filename=os.path.basename(eph['path'])) - for eph in ephemerals if eph.get('path')]) + mock_update_conn.assert_called_once_with( + mock.sentinel.bdm_obj, + eph_filename=os.path.basename(ephemerals[0]['path'])) def test_attach_drive_vm_to_scsi(self): self._vmops._attach_drive(