From 406d590a364d2c3ebc91e5f28f94011b158459d2 Mon Sep 17 00:00:00 2001 From: Simon Hensel Date: Tue, 23 Jan 2024 16:16:17 +0100 Subject: [PATCH] Always delete NVRAM files when deleting instances When deleting an instance, always send VIR_DOMAIN_UNDEFINE_NVRAM to delete the NVRAM file, regardless of whether the image is of type UEFI. This prevents a bug when rebuilding an instance from an UEFI image to a non-UEFI image. Closes-Bug: #1997352 Change-Id: I24648f5b7895bf5d093f222b6c6e364becbb531f Signed-off-by: Simon Hensel --- nova/tests/unit/virt/libvirt/test_driver.py | 33 +++++++-------------- nova/tests/unit/virt/libvirt/test_guest.py | 5 ---- nova/virt/libvirt/driver.py | 28 ++++------------- nova/virt/libvirt/guest.py | 5 ++-- 4 files changed, 17 insertions(+), 54 deletions(-) diff --git a/nova/tests/unit/virt/libvirt/test_driver.py b/nova/tests/unit/virt/libvirt/test_driver.py index 868e024370b5..b4e0d795de67 100644 --- a/nova/tests/unit/virt/libvirt/test_driver.py +++ b/nova/tests/unit/virt/libvirt/test_driver.py @@ -5406,13 +5406,6 @@ class LibvirtConnTestCase(test.NoDBTestCase, '/usr/share/OVMF/OVMF_VARS.fd', cfg.os_nvram_template) self.assertFalse(cfg.os_loader_secure) - def test_check_uefi_support_aarch64(self): - self.mock_uname.return_value = fakelibvirt.os_uname( - 'Linux', '', '5.4.0-0-generic', '', fields.Architecture.AARCH64) - drvr = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), True) - image_meta = objects.ImageMeta.from_dict(self.test_image_meta) - self.assertTrue(drvr._check_uefi_support(image_meta)) - def test_get_guest_config_with_block_device(self): drvr = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), True) @@ -17645,7 +17638,7 @@ class LibvirtConnTestCase(test.NoDBTestCase, self.assertEqual(2, mock_domain.ID.call_count) mock_domain.destroy.assert_called_once_with() - mock_domain.undefineFlags.assert_called_once_with(1) + mock_domain.undefineFlags.assert_called_once_with(5) mock_domain.undefine.assert_called_once_with() mock_save.assert_called_once_with() @@ -17667,7 +17660,7 @@ class LibvirtConnTestCase(test.NoDBTestCase, self.assertEqual(1, mock_domain.ID.call_count) mock_domain.destroy.assert_called_once_with() - mock_domain.undefineFlags.assert_called_once_with(1) + mock_domain.undefineFlags.assert_called_once_with(5) mock_domain.hasManagedSaveImage.assert_has_calls([mock.call(0)]) mock_domain.managedSaveRemove.assert_called_once_with(0) mock_domain.undefine.assert_called_once_with() @@ -17692,7 +17685,7 @@ class LibvirtConnTestCase(test.NoDBTestCase, self.assertEqual(1, mock_domain.ID.call_count) mock_domain.destroy.assert_called_once_with() - mock_domain.undefineFlags.assert_called_once_with(1) + mock_domain.undefineFlags.assert_called_once_with(5) mock_domain.hasManagedSaveImage.assert_has_calls([mock.call(0)]) mock_domain.undefine.assert_called_once_with() mock_save.assert_called_once_with() @@ -17718,10 +17711,10 @@ class LibvirtConnTestCase(test.NoDBTestCase, self.assertEqual(1, mock_domain.ID.call_count) mock_domain.destroy.assert_called_once_with() - # NVRAM flag should not called only host support uefi - mock_domain.undefineFlags.assert_called_once_with( - fakelibvirt.VIR_DOMAIN_UNDEFINE_MANAGED_SAVE - ) + mock_domain.undefineFlags.assert_has_calls([mock.call( + fakelibvirt.VIR_DOMAIN_UNDEFINE_MANAGED_SAVE | + fakelibvirt.VIR_DOMAIN_UNDEFINE_NVRAM + )]) mock_domain.undefine.assert_not_called() mock_save.assert_called_once_with() @@ -17746,8 +17739,6 @@ class LibvirtConnTestCase(test.NoDBTestCase, self.assertEqual(1, mock_domain.ID.call_count) mock_domain.destroy.assert_called_once_with() - # undefineFlags should now be called with 5 as uefi supported - # by both host and guest mock_domain.undefineFlags.assert_has_calls([mock.call( fakelibvirt.VIR_DOMAIN_UNDEFINE_MANAGED_SAVE | fakelibvirt.VIR_DOMAIN_UNDEFINE_NVRAM @@ -20676,7 +20667,7 @@ class LibvirtConnTestCase(test.NoDBTestCase, resize_to = 1 expected_resize_to = resize_to * units.Gi - drvr._swap_volume(mock_guest, 'vdb', mock_conf, resize_to, None) + drvr._swap_volume(mock_guest, 'vdb', mock_conf, resize_to) # Assert that virDomainBlockCopy is called mock_dev.copy.assert_called_once_with( @@ -20716,7 +20707,7 @@ class LibvirtConnTestCase(test.NoDBTestCase, # Assert that exception.VolumeRebaseFailed is raised self.assertRaises(exception.VolumeRebaseFailed, drvr._swap_volume, - mock_guest, 'vdb', mock_conf, 0, None) + mock_guest, 'vdb', mock_conf, 0) # Assert that virDomainBlockCopy is called mock_dev.copy.assert_called_once_with( @@ -20735,9 +20726,6 @@ class LibvirtConnTestCase(test.NoDBTestCase, swap_volume, disconnect_volume): conn = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI()) instance = objects.Instance(**self.test_instance) - image_meta = objects.ImageMeta.from_dict(self.test_image_meta) - hw_firmware_type = image_meta.properties.get( - 'hw_firmware_type') old_connection_info = {'driver_volume_type': 'fake', 'serial': 'old-volume-id', 'data': {'device_path': '/fake-old-volume', @@ -20770,8 +20758,7 @@ class LibvirtConnTestCase(test.NoDBTestCase, connect_volume.assert_called_once_with(self.context, new_connection_info, instance) - swap_volume.assert_called_once_with(guest, 'vdb', - conf, 1, hw_firmware_type) + swap_volume.assert_called_once_with(guest, 'vdb', conf, 1) disconnect_volume.assert_called_once_with(self.context, old_connection_info, instance) diff --git a/nova/tests/unit/virt/libvirt/test_guest.py b/nova/tests/unit/virt/libvirt/test_guest.py index cdcbc00c7f98..49ba7c87aeb7 100644 --- a/nova/tests/unit/virt/libvirt/test_guest.py +++ b/nova/tests/unit/virt/libvirt/test_guest.py @@ -134,11 +134,6 @@ class GuestTestCase(test.NoDBTestCase): def test_delete_configuration(self): self.guest.delete_configuration() - self.domain.undefineFlags.assert_called_once_with( - fakelibvirt.VIR_DOMAIN_UNDEFINE_MANAGED_SAVE) - - def test_delete_configuration_with_nvram(self): - self.guest.delete_configuration(support_uefi=True) self.domain.undefineFlags.assert_called_once_with( fakelibvirt.VIR_DOMAIN_UNDEFINE_MANAGED_SAVE | fakelibvirt.VIR_DOMAIN_UNDEFINE_NVRAM) diff --git a/nova/virt/libvirt/driver.py b/nova/virt/libvirt/driver.py index 7f5f48c04731..87309c817db0 100644 --- a/nova/virt/libvirt/driver.py +++ b/nova/virt/libvirt/driver.py @@ -1565,10 +1565,7 @@ class LibvirtDriver(driver.ComputeDriver): try: guest = self._host.get_guest(instance) try: - hw_firmware_type = instance.image_meta.properties.get( - 'hw_firmware_type') - support_uefi = self._check_uefi_support(hw_firmware_type) - guest.delete_configuration(support_uefi) + guest.delete_configuration() except libvirt.libvirtError as e: with excutils.save_and_reraise_exception() as ctxt: errcode = e.get_error_code() @@ -2227,7 +2224,7 @@ class LibvirtDriver(driver.ComputeDriver): self._disconnect_volume(context, connection_info, instance, encryption=encryption) - def _swap_volume(self, guest, disk_dev, conf, resize_to, hw_firmware_type): + def _swap_volume(self, guest, disk_dev, conf, resize_to): """Swap existing disk with a new block device. Call virDomainBlockRebase or virDomainBlockCopy with Libvirt >= 6.0.0 @@ -2237,7 +2234,6 @@ class LibvirtDriver(driver.ComputeDriver): :param: disk_dev: Device within the domain that is being swapped :param: conf: LibvirtConfigGuestDisk object representing the new volume :param: resize_to: Size of the dst volume, 0 if the same as the src - :param: hw_firmware_type: fields.FirmwareType if set in the imagemeta """ dev = guest.get_block_device(disk_dev) @@ -2258,8 +2254,7 @@ class LibvirtDriver(driver.ComputeDriver): # undefine it. If any part of this block fails, the domain is # re-defined regardless. if guest.has_persistent_configuration(): - support_uefi = self._check_uefi_support(hw_firmware_type) - guest.delete_configuration(support_uefi) + guest.delete_configuration() try: dev.copy(conf.to_xml(), reuse_ext=True) @@ -2324,12 +2319,9 @@ class LibvirtDriver(driver.ComputeDriver): self._connect_volume(context, new_connection_info, instance) conf = self._get_volume_config( instance, new_connection_info, disk_info) - hw_firmware_type = instance.image_meta.properties.get( - 'hw_firmware_type') try: - self._swap_volume(guest, disk_dev, conf, - resize_to, hw_firmware_type) + self._swap_volume(guest, disk_dev, conf, resize_to) except exception.VolumeRebaseFailed: with excutils.save_and_reraise_exception(): self._disconnect_volume(context, new_connection_info, instance) @@ -3384,10 +3376,7 @@ class LibvirtDriver(driver.ComputeDriver): # If any part of this block fails, the domain is # re-defined regardless. if guest.has_persistent_configuration(): - hw_firmware_type = image_meta.properties.get( - 'hw_firmware_type') - support_uefi = self._check_uefi_support(hw_firmware_type) - guest.delete_configuration(support_uefi) + guest.delete_configuration() # NOTE (rmk): Establish a temporary mirror of our root disk and # issue an abort once we have a complete copy. @@ -6562,13 +6551,6 @@ class LibvirtDriver(driver.ComputeDriver): return flavor return instance.flavor - def _check_uefi_support(self, hw_firmware_type): - caps = self._host.get_capabilities() - return self._host.supports_uefi and ( - hw_firmware_type == fields.FirmwareType.UEFI or - caps.host.cpu.arch == fields.Architecture.AARCH64 - ) - def _check_secure_boot_support( self, arch: str, diff --git a/nova/virt/libvirt/guest.py b/nova/virt/libvirt/guest.py index aeafdcc539be..c8d9611587a7 100644 --- a/nova/virt/libvirt/guest.py +++ b/nova/virt/libvirt/guest.py @@ -295,12 +295,11 @@ class Guest(object): yield VCPUInfo( id=vcpu[0], cpu=vcpu[3], state=vcpu[1], time=vcpu[2]) - def delete_configuration(self, support_uefi=False): + def delete_configuration(self): """Undefines a domain from hypervisor.""" try: flags = libvirt.VIR_DOMAIN_UNDEFINE_MANAGED_SAVE - if support_uefi: - flags |= libvirt.VIR_DOMAIN_UNDEFINE_NVRAM + flags |= libvirt.VIR_DOMAIN_UNDEFINE_NVRAM self._domain.undefineFlags(flags) except libvirt.libvirtError: LOG.debug("Error from libvirt during undefineFlags for guest "