From b117c19c493d04ce4e4be4f6f63639baa886a95b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Denis=20Krienb=C3=BChl?= Date: Wed, 5 Mar 2025 10:09:28 +0100 Subject: [PATCH 1/4] Always reuse the driver element for live-migration Changes to the config, or to Nova behavior, may cause the driver elemnt in the libvirt xml config to change. VMs created before the config or behavior change, can then no longer be live-migrated. By always copying the driver element, compatibility between source and target XML is increased and live-migration issues are avoided. (cherry picked from commit 029f6dd91db8548988ff4c4111a4c12d55391061) (cherry picked from commit 6ad83365d50fb7f3e6e4843fefedeb8b8ec6f628) --- nova/virt/libvirt/migration.py | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/nova/virt/libvirt/migration.py b/nova/virt/libvirt/migration.py index 25d271cac76..8e3cb22ac10 100644 --- a/nova/virt/libvirt/migration.py +++ b/nova/virt/libvirt/migration.py @@ -416,6 +416,10 @@ def _update_vif_xml(xml_doc, migrate_data, get_vif_config): # can't change during live migration. address = interface_dev.find('address') mtu = interface_dev.find('mtu') + # We always use the driver already found on the VM, as any changes to + # it have a high chance of causing libvirt to fail, due to mismatching + # virtio options. + driver = interface_dev.find('driver') # Now clear the interface's current elements and insert everything # from the destination vif config xml. interface_dev.clear() @@ -428,7 +432,14 @@ def _update_vif_xml(xml_doc, migrate_data, get_vif_config): # the live migration will crash. if dest_interface_subelem.tag == 'mtu' and mtu is None: continue + # Never use the driver from the newly created XML + if dest_interface_subelem.tag == 'driver': + continue interface_dev.insert(index, dest_interface_subelem) + # Re-insert the driver. + if driver is not None: + interface_dev.insert(index + 1, driver) + index += 1 # And finally re-insert the hw address. interface_dev.insert(index + 1, address) From 87f2c6009d600497c20210eaefb7d7e68ae39920 Mon Sep 17 00:00:00 2001 From: Gaudenz Steinlin Date: Mon, 3 Mar 2025 16:56:04 +0100 Subject: [PATCH 2/4] Make SSH compression for remote copy configurable Add a configuration option to enable SSH compression for copying ephemeral disks to remote hosts. This replaces the static list NO_COMPRESSION_TYPES. On fast local networks compression actually reduces the copy bandwith because of the CPU overhead. Implements: blueprint configurable-no-compression-image-types Change-Id: Ia935b45c56f81d5922608e37d5f263e5dd2e8a6f (cherry picked from commit 2a49c17dd526505f1b145d875542910778c88e02) (cherry picked from commit 9ddd567e7e9698e798e9a690d2255c9d0f96ee71) (cherry picked from commit 8b00e7d0235061aded8353e0f51a0f4caf91ef0d) --- nova/conf/libvirt.py | 7 +++++++ nova/tests/unit/virt/libvirt/test_driver.py | 20 +++++++++++++++++-- nova/tests/unit/virt/libvirt/test_utils.py | 4 ++-- nova/virt/libvirt/driver.py | 5 +---- nova/virt/libvirt/utils.py | 2 +- nova/virt/libvirt/volume/remotefs.py | 2 +- ...h-compression-config-4633a047e410983e.yaml | 12 +++++++++++ 7 files changed, 42 insertions(+), 10 deletions(-) create mode 100644 releasenotes/notes/ssh-compression-config-4633a047e410983e.yaml diff --git a/nova/conf/libvirt.py b/nova/conf/libvirt.py index abcefee777b..e3cd8ec31c4 100644 --- a/nova/conf/libvirt.py +++ b/nova/conf/libvirt.py @@ -959,6 +959,13 @@ * images_volume_group * [workarounds]/ensure_libvirt_rbd_instance_dir_cleanup * compute.force_raw_images +"""), + cfg.BoolOpt('remote_copy_compression', + default=False, + help=""" +Use SSH compression to copy ephemeral disks to a remote host. On fast +local networks compression often reduces the copy bandwidth because the CPU +cannot compress fast enough. """), cfg.StrOpt('images_volume_group', help=""" diff --git a/nova/tests/unit/virt/libvirt/test_driver.py b/nova/tests/unit/virt/libvirt/test_driver.py index cf824df3e98..56e26b39529 100644 --- a/nova/tests/unit/virt/libvirt/test_driver.py +++ b/nova/tests/unit/virt/libvirt/test_driver.py @@ -23098,13 +23098,14 @@ def _test_migrate_disk_and_power_off( self, ctxt, flavor_obj, mock_execute, mock_exists, mock_rename, mock_is_shared, mock_get_host_ip, mock_destroy, mock_get_disk_info, mock_vtpm, mock_unplug_vifs, - mock_cleanup, block_device_info=None, params_for_instance=None): + mock_cleanup, block_device_info=None, params_for_instance=None, + images_type='qcow2', assert_compression=False): """Test for nova.virt.libvirt.driver.LivirtConnection .migrate_disk_and_power_off. """ instance = self._create_instance(params=params_for_instance) - disk_info = list(fake_disk_info_byname(instance).values()) + disk_info = list(fake_disk_info_byname(instance, images_type).values()) disk_info_text = jsonutils.dumps(disk_info) mock_get_disk_info.return_value = disk_info @@ -23131,11 +23132,26 @@ def _test_migrate_disk_and_power_off( instance.uuid, mock.ANY, mock.ANY, '10.0.0.1', mock.ANY, mock.ANY) mock_unplug_vifs.assert_called_once() + assert_func = ( + self.assertIn if assert_compression + else self.assertNotIn + ) + for i, info in enumerate(disk_info): + self.assertIn('scp', mock_execute.call_args_list[i + 1].args) + assert_func('-C', mock_execute.call_args_list[i + 1].args) + def test_migrate_disk_and_power_off(self): flavor = {'root_gb': 10, 'ephemeral_gb': 20} flavor_obj = objects.Flavor(**flavor) self._test_migrate_disk_and_power_off(self.context, flavor_obj) + def test_migrate_disk_and_power_off_compressed(self): + CONF.set_override('remote_copy_compression', True, group='libvirt') + flavor = {'root_gb': 10, 'ephemeral_gb': 20} + flavor_obj = objects.Flavor(**flavor) + self._test_migrate_disk_and_power_off(self.context, flavor_obj, + assert_compression=True) + @mock.patch('nova.virt.libvirt.driver.LibvirtDriver._disconnect_volume') def test_migrate_disk_and_power_off_boot_from_volume(self, disconnect_volume): diff --git a/nova/tests/unit/virt/libvirt/test_utils.py b/nova/tests/unit/virt/libvirt/test_utils.py index c47b2e947cb..7cf4e5b50ba 100644 --- a/nova/tests/unit/virt/libvirt/test_utils.py +++ b/nova/tests/unit/virt/libvirt/test_utils.py @@ -57,14 +57,14 @@ def test_copy_image_remote_ssh(self, mock_rem_fs_remove): self.flags(remote_filesystem_transport='ssh', group='libvirt') libvirt_utils.copy_image('src', 'dest', host='host') mock_rem_fs_remove.assert_called_once_with('src', 'host:dest', - on_completion=None, on_execute=None, compression=True) + on_completion=None, on_execute=None, compression=False) @mock.patch('nova.virt.libvirt.volume.remotefs.RsyncDriver.copy_file') def test_copy_image_remote_rsync(self, mock_rem_fs_remove): self.flags(remote_filesystem_transport='rsync', group='libvirt') libvirt_utils.copy_image('src', 'dest', host='host') mock_rem_fs_remove.assert_called_once_with('src', 'host:dest', - on_completion=None, on_execute=None, compression=True) + on_completion=None, on_execute=None, compression=False) @mock.patch('os.path.exists', return_value=True) def test_disk_type_from_path(self, mock_exists): diff --git a/nova/virt/libvirt/driver.py b/nova/virt/libvirt/driver.py index 64ebb9e8149..d895759f468 100644 --- a/nova/virt/libvirt/driver.py +++ b/nova/virt/libvirt/driver.py @@ -229,9 +229,6 @@ def repr_method(self): # Virtuozzo driver support MIN_VIRTUOZZO_VERSION = (7, 0, 0) -# Names of the types that do not get compressed during migration -NO_COMPRESSION_TYPES = ('qcow2',) - # number of serial console limit QEMU_MAX_SERIAL_PORTS = 4 # Qemu supports 4 serial consoles, we remove 1 because of the PTY one defined @@ -12053,7 +12050,7 @@ def migrate_disk_and_power_off(self, context, instance, dest, if fname == 'disk.swap': continue - compression = info['type'] not in NO_COMPRESSION_TYPES + compression = CONF.libvirt.remote_copy_compression libvirt_utils.copy_image(from_path, img_path, host=dest, on_execute=on_execute, on_completion=on_completion, diff --git a/nova/virt/libvirt/utils.py b/nova/virt/libvirt/utils.py index 902852a7332..dce5ca1849c 100644 --- a/nova/virt/libvirt/utils.py +++ b/nova/virt/libvirt/utils.py @@ -310,7 +310,7 @@ def copy_image( receive: bool = False, on_execute: ty.Optional[ty.Callable] = None, on_completion: ty.Optional[ty.Callable] = None, - compression: bool = True, + compression: bool = False, ) -> None: """Copy a disk image to an existing directory diff --git a/nova/virt/libvirt/volume/remotefs.py b/nova/virt/libvirt/volume/remotefs.py index 8ddcaa1fccc..9d9085c0992 100644 --- a/nova/virt/libvirt/volume/remotefs.py +++ b/nova/virt/libvirt/volume/remotefs.py @@ -99,7 +99,7 @@ def remove_dir(self, host, dst_path, on_execute=None, on_completion=on_completion) def copy_file(self, src, dst, on_execute=None, - on_completion=None, compression=True): + on_completion=None, compression=False): LOG.debug("Copying file %s to %s", src, dst) self.driver.copy_file(src, dst, on_execute=on_execute, on_completion=on_completion, diff --git a/releasenotes/notes/ssh-compression-config-4633a047e410983e.yaml b/releasenotes/notes/ssh-compression-config-4633a047e410983e.yaml new file mode 100644 index 00000000000..1c9659722da --- /dev/null +++ b/releasenotes/notes/ssh-compression-config-4633a047e410983e.yaml @@ -0,0 +1,12 @@ +--- +features: + - | + A new configuration option [libvirt]/remote_copy_compression has been + added. This option replaces the static list of disk formats which use + SSH compression for remote copy operations. + + On fast local networks compression often reduces the copy bandwidth because + the CPU cannot compress fast enough. + + By default compression is disabled. In previous releases compression was + for the raw, flat, lvm and ploop disks types. From 49d4e1558bc1a387813eb9bb0ce1fc8bb66ce14d Mon Sep 17 00:00:00 2001 From: Gaudenz Steinlin Date: Wed, 26 Mar 2025 09:23:08 +0100 Subject: [PATCH 3/4] Resize ephemeral disk on instance resize Block device mappings for ephemeral and swap disks are not resized during instance resize operations. This causes virt drivers to not resize them. This patch resizes bdms for ephemeral and swap disks during finish_resize if: - All space allowed by the old flavor is used. - There is a single ephemeral disk or no ephemeral disk. - Swap is reduced in all cases if the new flavor allows less swap than currently allocated. For all other cases, the disk configuration is not touched as it's unclear how to resize. Note: Resizing to a flavor with less ephemeral disk space is not allowed and already prohibited in the API layer. TODO from old change: Virt driver related changes: - libvirt: a new imagebackend method _get_original_template_name is added, which aim is to get template name of an existing disk, which differs from specified one from driver's _create_image method in the case of resizing. Really, if we resize an ephemeral disk from 10 GB to 20 GB, the bdm object is already has 20 GB size, and _create_image specifies template name for 20 GB disk, but existing disk is created from 10 GB template and must continue to work with it. There is no reason to charge driver with such details, so that the new virtual method of imagebackend is the best choice. Resize ephemerals down details by drivers: - hyperv prevents resizing down in finish_migration method (except resizing to zero, in which case it removes ephemeral disk). This method raises an exception, and the instance is goes to error state. Unfortunately the same occures with revert of resize up operation when ephemeral bdm size is decreasing. I.e. it looks like this patch brings a problem there; - vmwareapi does not honor ephemeral bdms at all, but works with flavor ephemeral size directly. Moreover it recreates ephemeral disks on any resize operation. So that it probably does need to do something additionally with this patch; - ironic does not seem to has resizing supported at all. Conclusions: if vmwareapi behavior (recreating of ephemerals) is legal, fix hyperv only; if not - an early performed common check preventing resizing down whould be suitable. TODOes: - fix handling of resized bdms in hyperv; - check against lvm and ploop image backends; - fix a bug with disk.info inconsistency for flat backend after resizing (disks becomes qcow2 while disk.info still has 'raw' for them); Change-Id: Ie42a0b2f0019202c6cb1a1533a838a8549c000c8 Related-Bug: 1558880 Related-Bug: 1605720 (cherry picked from commit dcabe3588afeade262a4ec14d1cc7067d1f0ef0e) (cherry picked from commit 86662d540ba01369e48ad6023faa81ad37c458db) --- api-ref/source/servers-actions.inc | 3 +- nova/compute/manager.py | 183 ++++++ nova/tests/unit/compute/test_compute.py | 24 +- nova/tests/unit/compute/test_compute_mgr.py | 574 ++++++++++++++++++ .../unit/virt/libvirt/test_imagebackend.py | 27 +- nova/virt/block_device.py | 5 + nova/virt/libvirt/imagebackend.py | 5 +- 7 files changed, 800 insertions(+), 21 deletions(-) diff --git a/api-ref/source/servers-actions.inc b/api-ref/source/servers-actions.inc index 7a33990dbe2..29b3d24793d 100644 --- a/api-ref/source/servers-actions.inc +++ b/api-ref/source/servers-actions.inc @@ -841,7 +841,8 @@ automatically confirms the resize operation after the configured interval. .. _resize_confirm_window: https://docs.openstack.org/nova/latest/configuration/config.html#DEFAULT.resize_confirm_window .. note:: There is a `known limitation `__ - that ephemeral disks are not resized. + that ephemeral disks are not resized if they are smaller than the + flavors ephemeral size or if there are multiple ephemeral disks. Normal response codes: 202 diff --git a/nova/compute/manager.py b/nova/compute/manager.py index fbe45df38fe..dffc8d1853b 100644 --- a/nova/compute/manager.py +++ b/nova/compute/manager.py @@ -4942,6 +4942,7 @@ def _delete_stashed_flavor_info(self, instance): instance.old_flavor = None instance.new_flavor = None instance.system_metadata.pop('old_vm_state', None) + instance.system_metadata.pop('old_disk_sizes', None) instance.save() @wrap_exception() @@ -5486,6 +5487,13 @@ def _finish_revert_resize( # compatibility if old_vm_state is not set old_vm_state = instance.system_metadata.get( 'old_vm_state', vm_states.ACTIVE) + old_sizes = instance.system_metadata.get('old_disk_sizes') + if old_sizes: + self._revert_resize_ephemeral_bdms(context, instance, + jsonutils.loads(old_sizes)) + # Refresh bdms + bdms = objects.BlockDeviceMappingList.get_by_instance_uuid( + context, instance.uuid) # Revert the flavor and host/node fields to their previous values self._revert_instance_flavor_host_node(instance, migration) @@ -6271,6 +6279,176 @@ def _complete_volume_attachments(self, context, bdms): self.volume_api.attachment_complete( context, bdm.attachment_id) + def _update_ephemeral_bdms(self, context, instance, block_device_info, + ephemeral_size, swap_size): + """Update ephemeral bdm objects in DB (for instance resizing). + + Updates volume size of block device mapping object according to + specified new size. Works only for a single ephemeral and swap disks. + Stores the result into the DB. + + Creates a new bdm if necessary. + + :param context: Request context object. + :param instance: An instance which bdms are to be updated. + :param block_device_info: Instance block device mapping in driver + format. + :param ephemeral_size: New size in GB of the single ephemeral disk. + Creates a new bdm if no bdm exists. + If None - does nothing with ephemerals. + If 0 - removes the ephemeral disk + :param swap_size: A size in MB of swap disk. Creates a new bdm if no + swap bdm exists and non zero size is specified. + Destroys swap bdm if zero size is specified. + If None - does nothing with swap. + """ + ephemerals = driver.block_device_info_get_ephemerals(block_device_info) + swap = driver.block_device_info_get_swap(block_device_info) + reassing_device_names = False + + # Resize ephemeral disk + if ephemeral_size is not None: + if not ephemerals and ephemeral_size > 0: + LOG.debug( + f'Creating ephemeral bdm: size={ephemeral_size}.', + instance=instance + ) + bdm = objects.BlockDeviceMapping( + context=context, instance_uuid=instance.uuid, + source_type='blank', destination_type='local', + device_type='disk', delete_on_termination=True, + boot_index=-1, volume_size=ephemeral_size) + bdm.create() + reassing_device_names = True + elif len(ephemerals) == 1 and ephemeral_size == 0: + LOG.debug('Deleting ephemeral bdm.', instance=instance) + ephemerals[0].destroy() + reassing_device_names = True + elif len(ephemerals) == 1: + LOG.debug( + f'Resizing ephemeral bdm: new_size={ephemeral_size}, ' + f'old_size={ephemerals[0].volume_size}.', + instance=instance, + ) + ephemerals[0].volume_size = ephemeral_size + ephemerals[0].save() + + # Resize swap + if swap_size is not None: + if swap['swap_size'] == 0 and swap_size > 0: + LOG.debug( + f'Creating swap bdm: size={swap_size}.', + instance=instance, + ) + bdm = objects.BlockDeviceMapping( + context=context, instance_uuid=instance.uuid, + source_type='blank', destination_type='local', + device_type='disk', guest_format='swap', + boot_index=-1, volume_size=swap_size) + bdm.create() + reassing_device_names = True + elif swap['swap_size'] > 0 and swap_size == 0: + LOG.debug('Deleting swap bdm.', instance=instance) + swap.destroy() + reassing_device_names = True + elif swap['swap_size'] != swap_size: + LOG.debug( + f'Resizing swap bdm: old_size={swap.volume_size} ' + f'new_size={swap_size}', + instance=instance, + ) + swap.volume_size = swap_size + swap.save() + + if reassing_device_names: + bdms = objects.BlockDeviceMappingList.get_by_instance_uuid( + context, instance.uuid) + self._default_block_device_names(instance, None, bdms) + + def _resize_ephemeral_bdms(self, context, instance, bdms): + """Resize ephemeral bdm objects in DB (for instance resizing). + + For an instance in resizing state (having actual old and new flavor + fields) this method resizes the instance's ephemeral and swap bdms + objects, persists them into DB, preparing them to be used by virt + driver to create/resize disks. + + Supported features: + - add/remove/resize swap disk; + - add/increase single ephemeral disk; + - keep custom user specified ephemeral/swap size if it differs from + flavor specified size. + + :param context: Request context object. + :param instance: An instance which bdms are to be resized. + :returns: A serializable object contains original bdm sizes. + """ + + block_device_info = self._get_instance_block_device_info( + context, instance, bdms=bdms) + + # Handle ephemeral disk resizing + old_ephemeral_flavor = instance.old_flavor.ephemeral_gb + new_ephemeral_flavor = instance.new_flavor.ephemeral_gb + ephemerals = driver.block_device_info_get_ephemerals(block_device_info) + old_ephemeral_size = new_ephemeral_size = None + + changed = old_ephemeral_flavor != new_ephemeral_flavor + single_ephemeral = len(ephemerals) == 1 + + # Current config uses the maximum ephemeral disk size of the old flavor + max_size = ( + (len(ephemerals) == 0 and old_ephemeral_flavor == 0) or + (single_ephemeral and + old_ephemeral_flavor == ephemerals[0]['size']) + ) + + if changed and max_size: + old_ephemeral_size = (ephemerals[0]['size'] if len(ephemerals) > 0 + else 0) + new_ephemeral_size = new_ephemeral_flavor + + # Handle Swap resizing + swap = driver.block_device_info_get_swap(block_device_info) + old_swap_size = new_swap_size = None + + if (instance.new_flavor.swap != instance.old_flavor.swap and + (swap['swap_size'] == instance.old_flavor.swap or + swap['swap_size'] > instance.new_flavor.swap)): + old_swap_size = swap['swap_size'] + new_swap_size = instance.new_flavor.swap + + if new_ephemeral_size is not None or new_swap_size is not None: + self._update_ephemeral_bdms(context, instance, block_device_info, + new_ephemeral_size, new_swap_size) + + return old_ephemeral_size, old_swap_size + + def _revert_resize_ephemeral_bdms(self, context, instance, old_disk_sizes): + """Revert resized ephemeral bdm objects in DB (for instance resizing). + + Restore ephemeral and swap bdms previously resized with + _resize_ephemeral_bdms. + + :param context: Request context object. + :param instance: An instance which bdms are to be reverted. + :param old_disk_sizes: An object created with _resize_ephemeral_bdms. + """ + old_ephemeral_size, old_swap_size = old_disk_sizes + if old_ephemeral_size is None and old_swap_size is None: + return + + LOG.debug( + f'Reverting ephemeral bdms: ephemeral_size={old_ephemeral_size}, ' + f'swap_size={old_swap_size}.', + instance=instance, + ) + bdms = objects.BlockDeviceMappingList.get_by_instance_uuid( + context, instance.uuid) + block_device_info = driver.get_block_device_info(instance, bdms) + self._update_ephemeral_bdms(context, instance, block_device_info, + old_ephemeral_size, old_swap_size) + def _finish_resize(self, context, instance, migration, disk_info, image_meta, bdms, request_spec): resize_instance = False # indicates disks have been resized @@ -6297,6 +6475,11 @@ def _finish_resize(self, context, instance, migration, disk_info, if old_flavor[key] != new_flavor[key]: resize_instance = True break + if resize_instance: + old_sizes = self._resize_ephemeral_bdms( + context, instance, bdms) + instance.system_metadata['old_disk_sizes'] = jsonutils.dumps( + old_sizes) instance.apply_migration_context() # NOTE(tr3buchet): setup networks on destination host diff --git a/nova/tests/unit/compute/test_compute.py b/nova/tests/unit/compute/test_compute.py index 6cddd2ca858..3e4c841622e 100644 --- a/nova/tests/unit/compute/test_compute.py +++ b/nova/tests/unit/compute/test_compute.py @@ -4835,10 +4835,16 @@ def _test_finish_resize(self, power_on, resize_instance=True): fake_bdms = objects.BlockDeviceMappingList(objects=[ objects.BlockDeviceMapping(destination_type='volume', attachment_id=uuids.attachment_id, - device_name='/dev/vdb'), + device_name='/dev/vdb', + no_device=False, + source_type=None), objects.BlockDeviceMapping(destination_type='volume', - attachment_id=None), - objects.BlockDeviceMapping(destination_type='local') + attachment_id=None, + no_device=False, + source_type=None), + objects.BlockDeviceMapping(destination_type='local', + no_device=False, + source_type=None) ]) with test.nested( @@ -4857,11 +4863,13 @@ def _test_finish_resize(self, power_on, resize_instance=True): mock.patch.object(instance, 'save'), mock.patch.object(self.compute.driver, 'get_volume_connector'), mock.patch.object(self.compute.volume_api, 'attachment_update'), - mock.patch.object(self.compute.volume_api, 'attachment_complete') + mock.patch.object(self.compute.volume_api, 'attachment_complete'), + mock.patch('nova.virt.driver.block_device_info_get_ephemerals'), + mock.patch('nova.virt.driver.block_device_info_get_swap') ) as (mock_get_bdm, mock_setup, mock_net_mig, mock_get_nw, mock_notify, mock_notify_action, mock_virt_mig, mock_get_blk, mock_mig_save, mock_inst_save, mock_get_vol_connector, mock_attachment_update, - mock_attachment_complete): + mock_attachment_complete, mock_get_ephemerals, mock_get_swap): def _mig_save(): self.assertEqual(migration.status, 'finished') self.assertEqual(vm_state, instance.vm_state) @@ -4933,9 +4941,9 @@ def _instance_save1(expected_task_state=None): instance, disk_info, 'fake-nwinfo1', test.MatchType(objects.ImageMeta), resize_instance, mock.ANY, 'fake-bdminfo', power_on) - mock_get_blk.assert_called_once_with(self.context, instance, - refresh_conn_info=True, - bdms=fake_bdms) + mock_get_blk.assert_called_with(self.context, instance, + refresh_conn_info=True, + bdms=fake_bdms) mock_inst_save.assert_has_calls(inst_call_list) mock_mig_save.assert_called_once_with() diff --git a/nova/tests/unit/compute/test_compute_mgr.py b/nova/tests/unit/compute/test_compute_mgr.py index d26844c180d..c44fa49a709 100644 --- a/nova/tests/unit/compute/test_compute_mgr.py +++ b/nova/tests/unit/compute/test_compute_mgr.py @@ -13485,3 +13485,577 @@ def test_delete_swap_bdm(self, mock_bdm_destroy, mock_create_bdm): mock_bdm_destroy.assert_called_once() self.instance.get_bdms.assert_called_once() + + +@mock.patch('nova.objects.BlockDeviceMappingList.get_by_instance_uuid') +@mock.patch.object(objects.BlockDeviceMapping, 'create', autospec=True) +@mock.patch.object(objects.BlockDeviceMapping, 'destroy', autospec=True) +@mock.patch.object(objects.BlockDeviceMapping, 'save', autospec=True) +class ComputeManagerBDMResizeTestCase(test.NoDBTestCase): + + def setUp(self): + super().setUp() + self.flags(compute_driver='fake.SameHostColdMigrateDriver') + self.compute = manager.ComputeManager() + self.context = context.RequestContext( + fakes.FAKE_USER_ID, + fakes.FAKE_PROJECT_ID, + ) + + def _create_migration_instance( + self, + old_flavor_ephemeral=0, + old_flavor_swap=0, + new_flavor_ephemeral=0, + new_flavor_swap=0, + instance_ephemerals=None, + instance_swap=None, + ): + old_flavor = fake_flavor.fake_flavor_obj( + self.context, + name='old-flavor', + id=1, + flavorid='1', + ephemeral_gb=old_flavor_ephemeral, + swap=old_flavor_swap, + ) + + new_flavor = fake_flavor.fake_flavor_obj( + self.context, + name='new-flavor', + id=2, + flavorid='2', + ephemeral_gb=new_flavor_ephemeral, + swap=new_flavor_swap, + ) + + instance = fake_instance.fake_instance_obj( + self.context, + flavor=old_flavor, + vm_state=vm_states.ACTIVE, + expected_attrs=['metadata', 'system_metadata', 'info_cache'], + ) + + # This would be set in _finish_resize + instance.old_flavor = instance.flavor + # This would be set in _prep_resize + instance.new_flavor = new_flavor + + # Root disk + bdm_objects = [ + fake_block_device.fake_bdm_object( + self.context, + dict(source_type='image', + destination_type='local', + boot_index=0, + image_id=uuids.image_id, + device_name='/dev/vda', + volume_size=1)) + ] + + # Ephemeral disks + if instance_ephemerals: + for i, size in enumerate(instance_ephemerals): + bdm_objects.append( + fake_block_device.fake_bdm_object( + self.context, + dict(source_type='blank', + destination_type='local', + device_type='disk', + boot_index=-1, + # Ephemeral disks start at /dev/vdb + device_name=f'/dev/vd{chr(ord("b") + i)}', + volume_size=size) + ) + ) + + # Swap disks + if instance_swap: + bdm_objects.append( + fake_block_device.fake_bdm_object( + self.context, + dict(source_type='blank', + destination_type='local', + device_type='disk', + guest_format='swap', + boot_index=-1, + volume_size=instance_swap), + ) + ) + + bdm_list = objects.BlockDeviceMappingList(objects=bdm_objects) + + return instance, bdm_list + + def _setup_mocks(self, mock_bdm_get, mock_create, mock_destroy, bdm_list): + + def create(_self): + + # Set UUID and ID + _self.uuid = uuids.bdm + _self.id = 1 + + # Fill in any unset fields with default values + for field in _self.fields: + if field not in _self._changed_fields: + if _self.fields[field].nullable: + value = None + else: + value = _self.fields[field].default + + setattr(_self, field, value) + + bdm_list.objects.append(_self) + + def destroy(_self): + bdm_list.objects.remove(_self) + + mock_create.side_effect = create + mock_destroy.side_effect = destroy + mock_bdm_get.return_value = bdm_list + + def test_resize_ephemeral_bdms_single_full( + self, + mock_save, + mock_destroy, + mock_create, + mock_bdm_get + ): + """Test resizing a single ephemeral disk using the full allocation of + the flavor. The disk should be resized to the ephemeral size of the + new flavor. + """ + + instance, bdms = self._create_migration_instance( + old_flavor_ephemeral=1, + new_flavor_ephemeral=2, + instance_ephemerals=[1], + ) + + self._setup_mocks(mock_bdm_get, mock_create, mock_destroy, bdms) + + old_sizes = self.compute._resize_ephemeral_bdms( + self.context, + instance, + bdms, + ) + + self.assertEqual(bdms[1].volume_size, 2) + self.assertEqual(old_sizes, (1, None)) + mock_save.assert_called_once() + + # Test reverting + mock_save.reset_mock() + mock_create.reset_mock() + mock_destroy.reset_mock() + + self.compute._revert_resize_ephemeral_bdms( + self.context, + instance, + old_sizes, + ) + + mock_save.assert_called_once() + mock_create.assert_not_called() + mock_destroy.assert_not_called() + self.assertEqual(bdms[1].volume_size, 1) + + def test_resize_ephemeral_bdms_single_ephemeral_smaller( + self, + mock_save, + mock_destroy, + mock_create, + mock_bdm_get + ): + """Test resizing a single ephemeral disk not using the full allocation + of the flavor. The disk should not be resized. + """ + instance, bdms = self._create_migration_instance( + old_flavor_ephemeral=2, + new_flavor_ephemeral=3, + instance_ephemerals=[1], + ) + + self._setup_mocks(mock_bdm_get, mock_create, mock_destroy, bdms) + + old_sizes = self.compute._resize_ephemeral_bdms( + self.context, + instance, + bdms, + ) + + self.assertEqual(bdms[1].volume_size, 1) + self.assertEqual(old_sizes, (None, None)) + mock_save.assert_not_called() + + # Test reverting + mock_save.reset_mock() + mock_create.reset_mock() + mock_destroy.reset_mock() + + self.compute._revert_resize_ephemeral_bdms( + self.context, + instance, + old_sizes, + ) + + mock_save.assert_not_called() + mock_create.assert_not_called() + mock_destroy.assert_not_called() + self.assertEqual(bdms[1].volume_size, 1) + + def test_resize_ephemeral_bdms_multiple_ephemerals( + self, + mock_save, + mock_destroy, + mock_create, + mock_bdm_get + ): + """Test resizing with multiple ephemeral disks. This should not change + the ephemeral disk sizes. + """ + instance, bdms = self._create_migration_instance( + old_flavor_ephemeral=3, + new_flavor_ephemeral=4, + instance_ephemerals=[2, 1], + ) + + self._setup_mocks(mock_bdm_get, mock_create, mock_destroy, bdms) + + old_sizes = self.compute._resize_ephemeral_bdms( + self.context, + instance, + bdms, + ) + + self.assertEqual(bdms[1].volume_size, 2) + self.assertEqual(bdms[2].volume_size, 1) + self.assertEqual(old_sizes, (None, None)) + mock_save.assert_not_called() + + # Test reverting + mock_save.reset_mock() + mock_create.reset_mock() + mock_destroy.reset_mock() + + self.compute._revert_resize_ephemeral_bdms( + self.context, + instance, + old_sizes, + ) + + mock_save.assert_not_called() + mock_create.assert_not_called() + mock_destroy.assert_not_called() + self.assertEqual(bdms[1].volume_size, 2) + self.assertEqual(bdms[2].volume_size, 1) + + def test_resize_ephemeral_bdms_to_single_ephemeral( + self, + mock_save, + mock_destroy, + mock_create, + mock_bdm_get + ): + """Test resizing an instance without an ephemeral disk to a flavor + with ephemeral storage. This should add an ephemeral disk. + """ + instance, bdms = self._create_migration_instance( + old_flavor_ephemeral=0, + new_flavor_ephemeral=1, + instance_ephemerals=[], + ) + + self._setup_mocks(mock_bdm_get, mock_create, mock_destroy, bdms) + + old_sizes = self.compute._resize_ephemeral_bdms( + self.context, + instance, + bdms, + ) + + mock_create.assert_called_once() + mock_save.assert_called_once() # after assigning device name + mock_destroy.assert_not_called() + self.assertEqual(old_sizes, (0, None)) + self.assertEqual(len(bdms), 2) + self.assertEqual(bdms[1].volume_size, 1) + + # Test reverting + mock_save.reset_mock() + mock_create.reset_mock() + mock_destroy.reset_mock() + + self.compute._revert_resize_ephemeral_bdms( + self.context, + instance, + old_sizes, + ) + + mock_save.assert_not_called() + mock_create.assert_not_called() + mock_destroy.assert_called_once() + self.assertEqual(len(bdms), 1) + + def test_resize_ephemeral_bdms_swap_full_larger( + self, + mock_save, + mock_destroy, + mock_create, + mock_bdm_get + ): + """Test resizing to a flavor with more swap space if all swap space + allowed by the current flavor is used. Swap will be resized. + """ + instance, bdms = self._create_migration_instance( + old_flavor_swap=1, + new_flavor_swap=2, + instance_swap=1, + ) + + self._setup_mocks(mock_bdm_get, mock_create, mock_destroy, bdms) + + old_sizes = self.compute._resize_ephemeral_bdms( + self.context, + instance, + bdms, + ) + + self.assertEqual(bdms[1].volume_size, 2) + self.assertEqual(old_sizes, (None, 1)) + mock_save.assert_called_once() + + # Test reverting + mock_save.reset_mock() + mock_create.reset_mock() + mock_destroy.reset_mock() + + self.compute._revert_resize_ephemeral_bdms( + self.context, + instance, + old_sizes, + ) + + mock_save.assert_called_once() + mock_create.assert_not_called() + mock_destroy.assert_not_called() + self.assertEqual(bdms[1].volume_size, 1) + + def test_resize_ephemeral_bdms_swap_full_smaller( + self, + mock_save, + mock_destroy, + mock_create, + mock_bdm_get + ): + """Test resizing to a flavor with less swap space if all swap space + allowed by the current flavor is used. Swap will always be resized. + """ + instance, bdms = self._create_migration_instance( + old_flavor_swap=2, + new_flavor_swap=1, + instance_swap=2, + ) + + self._setup_mocks(mock_bdm_get, mock_create, mock_destroy, bdms) + + old_sizes = self.compute._resize_ephemeral_bdms( + self.context, + instance, + bdms, + ) + + self.assertEqual(bdms[1].volume_size, 1) + self.assertEqual(old_sizes, (None, 2)) + mock_save.assert_called_once() + + # Test reverting + mock_save.reset_mock() + mock_create.reset_mock() + mock_destroy.reset_mock() + + self.compute._revert_resize_ephemeral_bdms( + self.context, + instance, + old_sizes, + ) + + mock_save.assert_called_once() + mock_create.assert_not_called() + mock_destroy.assert_not_called() + self.assertEqual(bdms[1].volume_size, 2) + + def test_resize_ephemeral_bdms_swap_unused( + self, + mock_save, + mock_destroy, + mock_create, + mock_bdm_get + ): + """Test resizing to a flavor with more swap space if not all swap + space allowed by the old flavor is used. Swap will not be resized. + """ + instance, bdms = self._create_migration_instance( + old_flavor_swap=2, + new_flavor_swap=3, + instance_swap=1, + ) + + self._setup_mocks(mock_bdm_get, mock_create, mock_destroy, bdms) + + old_sizes = self.compute._resize_ephemeral_bdms( + self.context, + instance, + bdms, + ) + + self.assertEqual(bdms[1].volume_size, 1) + self.assertEqual(old_sizes, (None, None)) + mock_save.assert_not_called() + + # Test reverting + mock_save.reset_mock() + mock_create.reset_mock() + mock_destroy.reset_mock() + + self.compute._revert_resize_ephemeral_bdms( + self.context, + instance, + old_sizes, + ) + + mock_save.assert_not_called() + mock_create.assert_not_called() + mock_destroy.assert_not_called() + self.assertEqual(bdms[1].volume_size, 1) + + def test_resize_ephemeral_bdms_swap_too_large( + self, + mock_save, + mock_destroy, + mock_create, + mock_bdm_get + ): + """Test resizing to a flavor with less swap space than allowed by the + new flavor for an instance that does not use all swap space allowed + by the old flavor. Swap will be sized down to the new flavor. + """ + instance, bdms = self._create_migration_instance( + old_flavor_swap=3, + new_flavor_swap=1, + instance_swap=2, + ) + + self._setup_mocks(mock_bdm_get, mock_create, mock_destroy, bdms) + + old_sizes = self.compute._resize_ephemeral_bdms( + self.context, + instance, + bdms, + ) + + self.assertEqual(bdms[1].volume_size, 1) + self.assertEqual(old_sizes, (None, 2)) + mock_save.assert_called_once() + + # Test reverting + mock_save.reset_mock() + mock_create.reset_mock() + mock_destroy.reset_mock() + + self.compute._revert_resize_ephemeral_bdms( + self.context, + instance, + old_sizes, + ) + + mock_save.assert_called_once() + mock_create.assert_not_called() + mock_destroy.assert_not_called() + self.assertEqual(bdms[1].volume_size, 2) + + def test_resize_ephemeral_bdms_to_noswap( + self, + mock_save, + mock_destroy, + mock_create, + mock_bdm_get + ): + """Test resizing from a flavor with swap space to one without swap.""" + instance, bdms = self._create_migration_instance( + old_flavor_swap=1, + new_flavor_swap=0, + instance_swap=1, + ) + + self._setup_mocks(mock_bdm_get, mock_create, mock_destroy, bdms) + + old_sizes = self.compute._resize_ephemeral_bdms( + self.context, + instance, + bdms, + ) + + mock_destroy.assert_called_once() + mock_save.assert_not_called() + self.assertEqual(old_sizes, (None, 1)) + self.assertEqual(len(bdms), 1) + + # Test reverting + mock_save.reset_mock() + mock_create.reset_mock() + mock_destroy.reset_mock() + + self.compute._revert_resize_ephemeral_bdms( + self.context, + instance, + old_sizes, + ) + + mock_save.assert_called_once() # after assigning device name + mock_create.assert_called_once() + mock_destroy.assert_not_called() + self.assertEqual(bdms[1].volume_size, 1) + + def test_resize_ephemeral_bdms_to_swap( + self, + mock_save, + mock_destroy, + mock_create, + mock_bdm_get + ): + """Test resizing from a flavor without swap space to one with swap.""" + instance, bdms = self._create_migration_instance( + old_flavor_swap=0, + new_flavor_swap=1 + ) + + self._setup_mocks(mock_bdm_get, mock_create, mock_destroy, bdms) + + old_sizes = self.compute._resize_ephemeral_bdms( + self.context, + instance, + bdms, + ) + + mock_create.assert_called_once() + mock_save.assert_called_once() # after assigning device name + self.assertEqual(old_sizes, (None, 0)) + self.assertEqual(len(bdms), 2) + self.assertEqual(bdms[1].volume_size, 1) + + # Test reverting + mock_save.reset_mock() + mock_create.reset_mock() + mock_destroy.reset_mock() + + self.compute._revert_resize_ephemeral_bdms( + self.context, + instance, + old_sizes, + ) + + mock_save.assert_not_called() + mock_create.assert_not_called() + mock_destroy.assert_called_once() + self.assertEqual(len(bdms), 1) diff --git a/nova/tests/unit/virt/libvirt/test_imagebackend.py b/nova/tests/unit/virt/libvirt/test_imagebackend.py index 29e5d55055c..b569b944862 100644 --- a/nova/tests/unit/virt/libvirt/test_imagebackend.py +++ b/nova/tests/unit/virt/libvirt/test_imagebackend.py @@ -197,13 +197,12 @@ def test_libvirt_info(self): self.assertEqual(30 * units.Mi, disk.disk_total_bytes_sec) self.assertEqual(3 * units.Ki, disk.disk_total_iops_sec) - @mock.patch('nova.virt.disk.api.get_disk_size') - def test_get_disk_size(self, get_disk_size): + def _test_get_disk_size(self, get_disk_size, size_call_attr='path'): get_disk_size.return_value = 2361393152 image = self.image_class(self.INSTANCE, self.NAME) self.assertEqual(2361393152, image.get_disk_size(image.path)) - get_disk_size.assert_called_once_with(image.path) + get_disk_size.assert_called_once_with(getattr(image, size_call_attr)) def _test_libvirt_info_scsi_with_unit(self, disk_unit): disk_info = { @@ -883,9 +882,11 @@ def fake_exists(path): mock_exists.side_effect = fake_exists - # Fake create_volume causes exists to return true for the volume + # Fake create_volume causes exists to return true and set the size for + # the volume def fake_create_volume(vg, lv, size, sparse=False): exists.add(os.path.join('/dev', vg, lv)) + mock_lvm.get_volume_size.return_value = size mock_lvm.create_volume.side_effect = fake_create_volume @@ -960,6 +961,10 @@ def test_create_image_generated_negative(self, mock_remove, mock_create): fn.assert_called_once_with(target=self.PATH, ephemeral_size=None) mock_remove.assert_called_once_with([self.PATH]) + @mock.patch('nova.virt.libvirt.storage.lvm.get_volume_size') + def test_get_disk_size(self, get_disk_size): + self._test_get_disk_size(get_disk_size) + def test_prealloc_image(self): CONF.set_override('preallocate_images', 'space') @@ -1308,6 +1313,10 @@ def test_create_image_generated_encrypt_negative(self): self.PATH.rpartition('/')[2]) self.lvm.remove_volumes.assert_called_with([self.LV_PATH]) + @mock.patch('nova.virt.libvirt.storage.lvm.get_volume_size') + def test_get_disk_size(self, get_disk_size): + self._test_get_disk_size(get_disk_size) + def test_prealloc_image(self): self.flags(preallocate_images='space') fake_processutils.fake_execute_clear_log() @@ -1569,13 +1578,9 @@ def test_image_path(self): self.assertEqual(image.path, rbd_path) - def test_get_disk_size(self): - image = self.image_class(self.INSTANCE, self.NAME) - with mock.patch.object(image.driver, 'size') as size_mock: - size_mock.return_value = 2361393152 - - self.assertEqual(2361393152, image.get_disk_size(image.path)) - size_mock.assert_called_once_with(image.rbd_name) + @mock.patch('nova.storage.rbd_utils.RBDDriver.size') + def test_get_disk_size(self, get_disk_size): + self._test_get_disk_size(get_disk_size, 'rbd_name') @mock.patch.object(images, 'qemu_img_info', return_value=imageutils.QemuImgInfo()) diff --git a/nova/virt/block_device.py b/nova/virt/block_device.py index f7265117eb2..fc83a4ebdf8 100644 --- a/nova/virt/block_device.py +++ b/nova/virt/block_device.py @@ -216,6 +216,9 @@ def save(self): setattr(self._bdm_obj, attr_name, self[lookup_name]) self._bdm_obj.save() + def destroy(self): + self._bdm_obj.destroy() + class DriverSwapBlockDevice(DriverBlockDevice): _fields = set([ @@ -227,6 +230,7 @@ class DriverSwapBlockDevice(DriverBlockDevice): 'encryption_format', 'encryption_options', ]) + _proxy_as_attr_inherited = set(['volume_size']) _readonly_fields = set(['encrypted']) _update_on_save = { 'disk_bus': None, @@ -325,6 +329,7 @@ class DriverEphemeralBlockDevice(DriverBlockDevice): 'encryption_format': None, 'encryption_options': None, } + _proxy_as_attr_inherited = set(['volume_size']) def _transform(self): if not block_device.new_format_is_ephemeral(self._bdm_obj): diff --git a/nova/virt/libvirt/imagebackend.py b/nova/virt/libvirt/imagebackend.py index b1cfabf59b9..f058ffe73d3 100644 --- a/nova/virt/libvirt/imagebackend.py +++ b/nova/virt/libvirt/imagebackend.py @@ -304,7 +304,7 @@ def fetch_func_sync(target, *args, **kwargs): if size: # create_image() only creates the base image if needed, so # we cannot rely on it to exist here - if os.path.exists(base) and size > self.get_disk_size(base): + if os.path.exists(base) and size > self.get_disk_size(self.path): self.resize_image(size) if (self.preallocate and self._can_fallocate() and @@ -866,6 +866,9 @@ def create_lvm_image(base, size): def resize_image(self, size): pass + def get_disk_size(self, path): + return lvm.get_volume_size(path) + @contextlib.contextmanager def remove_volume_on_error(self, path): try: From 586c57dce09be9ec7da888fe4aceda19611ba8f4 Mon Sep 17 00:00:00 2001 From: Gaudenz Steinlin Date: Tue, 27 May 2025 18:09:00 +0200 Subject: [PATCH 4/4] Don't patch mock.patch with oslotest autospec The custom autospec implementation in oslotest breaks mocking of bound instance methods. All tests pass without this custom implementation and no other OpenStack project uses this code. Change-Id: I0e9cf3bb6b0d622f29550aef29433cfbf76c259c (cherry picked from commit 5667e0583187f92ed232ddc80b1a49f6c0597074) --- nova/test.py | 7 ------- 1 file changed, 7 deletions(-) diff --git a/nova/test.py b/nova/test.py index ee848cdab1a..894707dbd4b 100644 --- a/nova/test.py +++ b/nova/test.py @@ -89,13 +89,6 @@ class TestingException(Exception): pass -# NOTE(claudiub): this needs to be called before any mock.patch calls are -# being done, and especially before any other test classes load. This fixes -# the mock.patch autospec issue: -# https://github.com/testing-cabal/mock/issues/396 -mock_fixture.patch_mock_module() - - def _poison_unfair_compute_resource_semaphore_locking(): """Ensure that every locking on COMPUTE_RESOURCE_SEMAPHORE is called with fair=True.