From 4ace790a3098d51f8219adc929da1909f1d6f90e Mon Sep 17 00:00:00 2001 From: Sylvain Bauza Date: Mon, 6 Oct 2025 14:36:53 +0200 Subject: [PATCH 1/2] Add a regression test for ImagePropsWeigher The weigher is unable to get the right image metadata for existing instances if they are not already on the HostState. Change-Id: I5bccf854662ecffe1d469bacc6e4afcb746d6b4d Signed-off-by: Sylvain Bauza (cherry picked from commit 04afc452b35ba09d10557662904a6b0a0914eaf5) (cherry picked from commit 180d43192ca1a26af43405b17df8b776dde79cfb) --- .../regressions/test_bug_2125935.py | 112 ++++++++++++++++++ 1 file changed, 112 insertions(+) create mode 100644 nova/tests/functional/regressions/test_bug_2125935.py diff --git a/nova/tests/functional/regressions/test_bug_2125935.py b/nova/tests/functional/regressions/test_bug_2125935.py new file mode 100644 index 00000000000..7308e75a368 --- /dev/null +++ b/nova/tests/functional/regressions/test_bug_2125935.py @@ -0,0 +1,112 @@ +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. + +from unittest import mock + +from nova.scheduler import weights +from nova.tests.functional import integrated_helpers + + +class HostNameWeigher(weights.BaseHostWeigher): + # We want to predictabilly have host1 first + _weights = {'host1': 1, 'host2': 0} + + def _weigh_object(self, host_state, weight_properties): + # Any undefined host gets no weight. + return self._weights.get(host_state.host, 0) + + +class TestImagePropsWeigher(integrated_helpers._IntegratedTestBase): + """Tests for image props weigher """ + + compute_driver = 'fake.MediumFakeDriver' + microversion = 'latest' + ADMIN_API = True + + def _setup_compute_service(self): + # Override to prevent the default 'compute' service from starting + pass + + def setUp(self): + weight_classes = [ + __name__ + '.HostNameWeigher', + 'nova.scheduler.weights.image_props.ImagePropertiesWeigher' + ] + self.flags(weight_classes=weight_classes, + group='filter_scheduler') + self.flags(image_props_weight_multiplier=2.0, group='filter_scheduler') + super(TestImagePropsWeigher, self).setUp() + + # Start only the compute services we want for testing + self.compute1 = self._start_compute('host1') + self.compute2 = self._start_compute('host2') + + @mock.patch('nova.weights.LOG.debug') + def test_boot(self, mock_debug): + server1 = self._create_server( + name='inst1', + networks='none', + ) + + # the weigher sees that there are no existing instances on both hosts + # with the same image props + mock_debug.assert_any_call( + "%s: raw weights %s", + "ImagePropertiesWeigher", + {('host2', 'host2'): 0.0, ('host1', 'host1'): 0.0}) + # because of the HostNameWeigher, we're sure that the instance lands + # on host1 + self.assertEqual('host1', server1['OS-EXT-SRV-ATTR:host']) + # let's make sure that we don't assert the calls from the previous + # schedules. + mock_debug.reset_mock() + + server2 = self._create_server( + name='inst2', + host='host2', + networks='none', + ) + # Since we force to a host, the weigher will not be called + self.assertEqual('host2', server2['OS-EXT-SRV-ATTR:host']) + + server3 = self._create_server( + name='inst3', + networks='none', + ) + + # The weigher is called but it says that there are no existing + # instances on both the hosts with the same image props, while we are + # sure that the values should be 1.0 for both hosts. + # FIXME(sbauza): That's due to bug/2125935 + mock_debug.assert_any_call( + "%s: raw weights %s", + "ImagePropertiesWeigher", + {('host2', 'host2'): 0.0, ('host1', 'host1'): 0.0}) + mock_debug.reset_mock() + # server3 is now on the same host than host1 as the weigh multiplier + # makes the scheduler to pack instances sharing the same image props. + self.assertEqual('host1', server3['OS-EXT-SRV-ATTR:host']) + + server4 = self._create_server( + name='inst4', + networks='none', + ) + # FIXME(sbauza): Same issue. The values should be 2.0 for host1 and 1.0 + # for host2. That said, due to the fact the HostState is refreshed + # already for the previous schedule, the system metadata is existing + # for this instance so that's why the weight is 1.0 for host1. + mock_debug.assert_any_call( + "%s: raw weights %s", + "ImagePropertiesWeigher", + {('host2', 'host2'): 0.0, ('host1', 'host1'): 1.0}) + # server4 is now packed with server1 and server3. + self.assertEqual('host1', server4['OS-EXT-SRV-ATTR:host']) From e59075ba2ab4d388c1d0c690eafde1e391c70d05 Mon Sep 17 00:00:00 2001 From: Nicolai Ruckel Date: Tue, 22 Jul 2025 09:49:55 +0200 Subject: [PATCH 2/2] Preserve UEFI NVRAM variable store MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Preserve NVRAM variable store during stop/start, hard reboot, live migration, and volume retype. This does not affect cold migration or shelve. For UEFI guests (hw_firmware_type=uefi), every time the instance is started, the UEFI variable storage for that instance (/var/lib/libvirt/qemu/nvram/instance-xxxxxxxx_VARS.fd) is deleted and reinitialized from the default template. The changes are based on this patch by Jonas Schäfer to preserve the vTPM state: https://review.opendev.org/c/openstack/nova/+/955657 Closes-Bug: #1633447 Closes-Bug: #2131730 Change-Id: I444a9285c07a04bf08a73772235f8dd73d75e513 Signed-off-by: Nicolai Ruckel (cherry picked from commit 35b1945522eea195e9795914739e3cfd6e14214b) (cherry picked from commit 40f52a9175d5568510909514ed60afc577712689) --- nova/tests/fixtures/libvirt.py | 1 + nova/tests/unit/virt/libvirt/test_config.py | 2 + nova/tests/unit/virt/libvirt/test_driver.py | 42 +++++++++++++++---- nova/tests/unit/virt/libvirt/test_guest.py | 6 +++ nova/virt/libvirt/driver.py | 25 ++++++----- nova/virt/libvirt/guest.py | 10 ++++- .../preserve-nvram-ab6d3d2fe923301f.yaml | 7 ++++ 7 files changed, 74 insertions(+), 19 deletions(-) create mode 100644 releasenotes/notes/preserve-nvram-ab6d3d2fe923301f.yaml diff --git a/nova/tests/fixtures/libvirt.py b/nova/tests/fixtures/libvirt.py index 423c1418933..c939e021c48 100644 --- a/nova/tests/fixtures/libvirt.py +++ b/nova/tests/fixtures/libvirt.py @@ -101,6 +101,7 @@ def _reset(): VIR_DOMAIN_UNDEFINE_MANAGED_SAVE = 1 VIR_DOMAIN_UNDEFINE_NVRAM = 4 VIR_DOMAIN_UNDEFINE_KEEP_TPM = 64 +VIR_DOMAIN_UNDEFINE_KEEP_NVRAM = 8 VIR_DOMAIN_AFFECT_CURRENT = 0 VIR_DOMAIN_AFFECT_LIVE = 1 diff --git a/nova/tests/unit/virt/libvirt/test_config.py b/nova/tests/unit/virt/libvirt/test_config.py index 8ae3f9c723d..4bf8ca17373 100644 --- a/nova/tests/unit/virt/libvirt/test_config.py +++ b/nova/tests/unit/virt/libvirt/test_config.py @@ -2826,6 +2826,7 @@ def _test_config_uefi(self): obj.os_mach_type = "pc-q35-5.1" obj.os_loader = '/tmp/OVMF_CODE.secboot.fd' obj.os_loader_type = 'pflash' + obj.os_nvram = '/foo/bar/instance-00000012_VARS.fd' obj.os_loader_secure = True obj.os_loader_stateless = True xml = obj.to_xml() @@ -2840,6 +2841,7 @@ def _test_config_uefi(self): hvm /tmp/OVMF_CODE.secboot.fd + /foo/bar/instance-00000012_VARS.fd """, # noqa: E501 xml, diff --git a/nova/tests/unit/virt/libvirt/test_driver.py b/nova/tests/unit/virt/libvirt/test_driver.py index 064c67f21c4..a74602984b7 100644 --- a/nova/tests/unit/virt/libvirt/test_driver.py +++ b/nova/tests/unit/virt/libvirt/test_driver.py @@ -18891,10 +18891,11 @@ def test_undefine_domain_disarms_keep_vtpm_if_not_supported( fake_guest = mock.Mock() mock_get.return_value = fake_guest - drvr._undefine_domain(instance, keep_vtpm=True) + drvr._undefine_domain(instance, keep_vtpm=True, keep_nvram=False) fake_guest.delete_configuration.assert_called_once_with( keep_vtpm=False, + keep_nvram=False, ) # Check that it truly forces it to False and doesn't do a `not` or @@ -18904,6 +18905,7 @@ def test_undefine_domain_disarms_keep_vtpm_if_not_supported( fake_guest.delete_configuration.assert_called_once_with( keep_vtpm=False, + keep_nvram=False, ) @mock.patch.object(host.Host, "get_guest") @@ -18914,9 +18916,11 @@ def test_undefine_domain_passes_keep_vtpm_if_supported(self, mock_get): fake_guest = mock.Mock() mock_get.return_value = fake_guest - drvr._undefine_domain(instance, keep_vtpm=True) + drvr._undefine_domain(instance, keep_vtpm=True, keep_nvram=False) - fake_guest.delete_configuration.assert_called_once_with(keep_vtpm=True) + fake_guest.delete_configuration.assert_called_once_with( + keep_vtpm=True, + keep_nvram=False) # Check that it does not force keep_vtpm to true, just because it is # supported. @@ -18925,8 +18929,27 @@ def test_undefine_domain_passes_keep_vtpm_if_supported(self, mock_get): fake_guest.delete_configuration.assert_called_once_with( keep_vtpm=False, + keep_nvram=False, ) + @mock.patch.object(host.Host, "get_guest") + def test_undefine_domain_passes_keep_nvram_if_supported(self, mock_get): + drvr = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), False) + instance = objects.Instance(**self.test_instance) + fake_guest = mock.Mock() + mock_get.return_value = fake_guest + drvr._undefine_domain(instance, keep_nvram=True) + fake_guest.delete_configuration.assert_called_once_with( + keep_vtpm=False, keep_nvram=True) + # Check that it does not force keep_nvram to true, just because it is + # supported. + fake_guest.reset_mock() + drvr._undefine_domain(instance, keep_nvram=False) + fake_guest.delete_configuration.assert_called_once_with( + keep_vtpm=False, + keep_nvram=False, + ) + @mock.patch.object(host.Host, "list_instance_domains") @mock.patch.object(objects.BlockDeviceMappingList, "bdms_by_instance_uuid") @mock.patch.object(objects.InstanceList, "get_by_filters") @@ -19226,7 +19249,7 @@ def test_get_instance_disk_info_from_config_raw_files(self, disk_actual_size = 3687091200 disk_actual_size_blocks = disk_actual_size / 512 expected_over_committed_disk_size = disk_virtual_size -\ - disk_actual_size + disk_actual_size mock_getsize.return_value = disk_virtual_size mock_stat.return_value = mock.Mock(st_blocks=disk_actual_size_blocks) @@ -21498,7 +21521,10 @@ def test_cleanup_pass( mock_delete_files.assert_called_once_with(fake_inst) # vTPM secret should not be deleted until instance is deleted. mock_delete_vtpm.assert_not_called() - mock_undefine.assert_called_once_with(fake_inst, keep_vtpm=False) + mock_undefine.assert_called_once_with( + fake_inst, + keep_vtpm=False, + keep_nvram=False) @mock.patch('nova.virt.libvirt.driver.LibvirtDriver._undefine_domain') @mock.patch('nova.crypto.delete_vtpm_secret') @@ -21524,7 +21550,8 @@ def test_cleanup_preserves_tpm_if_not_destroying_disks( mock_get_mapping.assert_called_once_with(None) mock_delete_files.assert_not_called() mock_delete_vtpm.assert_not_called() - mock_undefine.assert_called_once_with(fake_inst, keep_vtpm=True) + mock_undefine.assert_called_once_with(fake_inst, keep_vtpm=True, + keep_nvram=True) @mock.patch('nova.virt.libvirt.driver.LibvirtDriver._undefine_domain') @mock.patch('nova.crypto.delete_vtpm_secret') @@ -21549,7 +21576,8 @@ def test_cleanup_instance_marked_deleted( drvr.cleanup('ctxt', fake_inst, 'netinfo') # vTPM secret should not be deleted until instance is deleted. mock_delete_vtpm.assert_not_called() - mock_undefine.assert_called_once_with(fake_inst, keep_vtpm=False) + mock_undefine.assert_called_once_with(fake_inst, keep_vtpm=False, + keep_nvram=False) @mock.patch.object(libvirt_driver.LibvirtDriver, 'delete_instance_files', return_value=True) diff --git a/nova/tests/unit/virt/libvirt/test_guest.py b/nova/tests/unit/virt/libvirt/test_guest.py index 359013c54ea..3034ff0a9d2 100644 --- a/nova/tests/unit/virt/libvirt/test_guest.py +++ b/nova/tests/unit/virt/libvirt/test_guest.py @@ -145,6 +145,12 @@ def test_delete_configuration_with_keep_vtpm_true(self): fakelibvirt.VIR_DOMAIN_UNDEFINE_NVRAM | fakelibvirt.VIR_DOMAIN_UNDEFINE_KEEP_TPM) + def test_delete_configuration_keep_nvram(self): + self.guest.delete_configuration(keep_nvram=True) + self.domain.undefineFlags.assert_called_once_with( + fakelibvirt.VIR_DOMAIN_UNDEFINE_MANAGED_SAVE | + fakelibvirt.VIR_DOMAIN_UNDEFINE_KEEP_NVRAM) + def test_delete_configuration_exception(self): self.domain.undefineFlags.side_effect = fakelibvirt.libvirtError( 'oops') diff --git a/nova/virt/libvirt/driver.py b/nova/virt/libvirt/driver.py index 7054b0a1355..361522e8e30 100644 --- a/nova/virt/libvirt/driver.py +++ b/nova/virt/libvirt/driver.py @@ -1688,7 +1688,7 @@ def destroy(self, context, instance, network_info, block_device_info=None, self.cleanup(context, instance, network_info, block_device_info, destroy_disks, destroy_secrets=destroy_secrets) - def _delete_guest_configuration(self, guest, keep_vtpm): + def _delete_guest_configuration(self, guest, keep_vtpm, keep_nvram): """Wrapper around guest.delete_configuration which incorporates version checks for the additional arguments. @@ -1707,13 +1707,14 @@ def _delete_guest_configuration(self, guest, keep_vtpm): ) keep_vtpm = False - guest.delete_configuration(keep_vtpm=keep_vtpm) + guest.delete_configuration(keep_vtpm=keep_vtpm, keep_nvram=keep_nvram) - def _undefine_domain(self, instance, keep_vtpm=False): + def _undefine_domain(self, instance, keep_vtpm=False, keep_nvram=False): try: guest = self._host.get_guest(instance) try: - self._delete_guest_configuration(guest, keep_vtpm=keep_vtpm) + self._delete_guest_configuration(guest, keep_vtpm=keep_vtpm, + keep_nvram=keep_nvram) except libvirt.libvirtError as e: with excutils.save_and_reraise_exception() as ctxt: errcode = e.get_error_code() @@ -1800,9 +1801,10 @@ def _cleanup(self, context, instance, network_info, block_device_info=None, :param destroy_vifs: if plugged vifs should be unplugged :param cleanup_instance_dir: If the instance dir should be removed :param cleanup_instance_disks: If the instance disks should be removed. - Also removes ephemeral encryption secrets, if present. - :param destroy_secrets: If the cinder volume encryption libvirt secrets - should be deleted. + Also removes ephemeral encryption secrets, if present, as well as + vTPM and NVRAM data. + :param destroy_secrets: If the cinder volume encryption secrets should + be deleted. """ # zero the data on backend pmem device vpmems = self._get_vpmems(instance) @@ -1876,7 +1878,8 @@ def _cleanup(self, context, instance, network_info, block_device_info=None, self._cleanup_ephemeral_encryption_secrets( context, instance, block_device_info) - self._undefine_domain(instance, keep_vtpm=not cleanup_instance_disks) + self._undefine_domain(instance, keep_vtpm=not cleanup_instance_disks, + keep_nvram=not cleanup_instance_disks) def _cleanup_ephemeral_encryption_secrets( self, context, instance, block_device_info @@ -2450,7 +2453,8 @@ def _swap_volume(self, guest, disk_dev, conf, resize_to): # undefine it. If any part of this block fails, the domain is # re-defined regardless. if guest.has_persistent_configuration(): - self._delete_guest_configuration(guest, keep_vtpm=True) + self._delete_guest_configuration(guest, keep_vtpm=True, + keep_nvram=True) try: dev.copy(conf.to_xml(), reuse_ext=True) @@ -3579,7 +3583,8 @@ def _live_snapshot(self, context, instance, guest, disk_path, out_path, # If any part of this block fails, the domain is # re-defined regardless. if guest.has_persistent_configuration(): - self._delete_guest_configuration(guest, keep_vtpm=True) + self._delete_guest_configuration(guest, keep_vtpm=True, + keep_nvram=True) # NOTE (rmk): Establish a temporary mirror of our root disk and # issue an abort once we have a complete copy. diff --git a/nova/virt/libvirt/guest.py b/nova/virt/libvirt/guest.py index 6d7eb969df9..065181d89e6 100644 --- a/nova/virt/libvirt/guest.py +++ b/nova/virt/libvirt/guest.py @@ -295,12 +295,15 @@ def get_vcpus_info(self): yield VCPUInfo( id=vcpu[0], cpu=vcpu[3], state=vcpu[1], time=vcpu[2]) - def delete_configuration(self, keep_vtpm=False): + def delete_configuration(self, keep_vtpm=False, keep_nvram=False): """Undefines a domain from hypervisor. :param keep_vtpm: If true, the vTPM data will be preserved. Otherwise, it will be deleted. Defaults to false (that is, deleting the vTPM data). + :param keep_nvram: If true, the NVRAM data will be preserved. + Otherwise, it will be deleted. Defaults to false (that is, deleting + the NVRAM data). Calling this with `keep_vtpm` set to True should, eventually, be followed up with a call where it is set to False (after re-defining @@ -312,9 +315,12 @@ def delete_configuration(self, keep_vtpm=False): """ try: flags = libvirt.VIR_DOMAIN_UNDEFINE_MANAGED_SAVE - flags |= libvirt.VIR_DOMAIN_UNDEFINE_NVRAM if keep_vtpm: flags |= libvirt.VIR_DOMAIN_UNDEFINE_KEEP_TPM + if keep_nvram: + flags |= libvirt.VIR_DOMAIN_UNDEFINE_KEEP_NVRAM + else: + flags |= libvirt.VIR_DOMAIN_UNDEFINE_NVRAM self._domain.undefineFlags(flags) except libvirt.libvirtError: LOG.debug("Error from libvirt during undefineFlags for guest " diff --git a/releasenotes/notes/preserve-nvram-ab6d3d2fe923301f.yaml b/releasenotes/notes/preserve-nvram-ab6d3d2fe923301f.yaml new file mode 100644 index 00000000000..1dc0a594e9a --- /dev/null +++ b/releasenotes/notes/preserve-nvram-ab6d3d2fe923301f.yaml @@ -0,0 +1,7 @@ +--- +fixes: + - | + NVRAM variable store is preserved during stop/start, hard reboot, and live + migration by passing the corresponding flag to libvirt. + + See https://bugs.launchpad.net/nova/+bug/1633447 for more details.