From 4ace790a3098d51f8219adc929da1909f1d6f90e Mon Sep 17 00:00:00 2001 From: Sylvain Bauza Date: Mon, 6 Oct 2025 14:36:53 +0200 Subject: [PATCH 1/4] 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 f4c66e732d9260bb9fa5448ade5dbcdbd00dea33 Mon Sep 17 00:00:00 2001 From: Sylvain Bauza Date: Tue, 30 Sep 2025 16:08:57 +0200 Subject: [PATCH 2/4] Fix fill_metadata usage for the ImagePropertiesWeigher When using the weigher, we need to target the right cell context for the existing instances in the host. fill_metadata was also having an issue as we need to pass the dict value from the updated dict by keying the instance uuid, not the whole dict of updated instances. Change-Id: I18260095ed263da4204f21de27f866568843804e Closes-Bug: #2125935 Signed-off-by: Sylvain Bauza (cherry picked from commit 98885344bd93531d569ded3f4492f473b09d0614) (cherry picked from commit 4b54a14ad64bbbf0e3b2fc8beb3698fc8f874c0d) --- doc/source/admin/scheduling.rst | 8 ++ nova/objects/instance.py | 3 +- nova/scheduler/weights/image_props.py | 14 +++- .../regressions/test_bug_2125935.py | 15 ++-- nova/tests/unit/objects/test_instance.py | 13 +++- .../weights/test_weights_image_props.py | 75 ++++++++++++++++++- .../notes/bug-2125935-de97147d5ff960b5.yaml | 8 ++ 7 files changed, 118 insertions(+), 18 deletions(-) create mode 100644 releasenotes/notes/bug-2125935-de97147d5ff960b5.yaml diff --git a/doc/source/admin/scheduling.rst b/doc/source/admin/scheduling.rst index ea68cc1b72e..7b79c503065 100644 --- a/doc/source/admin/scheduling.rst +++ b/doc/source/admin/scheduling.rst @@ -1104,6 +1104,14 @@ provided in the option value. The resulted host weight would then be multiplied by the value of :oslo.config:option:`filter_scheduler.image_props_weight_multiplier`. + +.. note:: + + The weigher compares the values of the image properties as strings. If some + image properties are lists (eg. hw_numa_nodes), then if the values are + ordered differently, then the weigher will consider them as different + values. + Utilization-aware scheduling ---------------------------- diff --git a/nova/objects/instance.py b/nova/objects/instance.py index cf72d76d68e..181686e9aa5 100644 --- a/nova/objects/instance.py +++ b/nova/objects/instance.py @@ -1606,7 +1606,8 @@ def fill_metadata(self): for inst in [i for i in self if i.uuid in updated]: # Patch up our instances with system_metadata from the fill # operation - inst.system_metadata = utils.instance_sys_meta(updated) + inst.system_metadata = utils.instance_sys_meta( + updated[inst.uuid]) @base.remotable_classmethod def get_uuids_by_host(cls, context, host): diff --git a/nova/scheduler/weights/image_props.py b/nova/scheduler/weights/image_props.py index c28e908576c..12284080b0a 100644 --- a/nova/scheduler/weights/image_props.py +++ b/nova/scheduler/weights/image_props.py @@ -74,10 +74,18 @@ def _weigh_object(self, host_state, request_spec): # context so we can access all of them, not only the ones from the # request. ctxt = nova_context.get_admin_context() - insts = objects.InstanceList(ctxt, - objects=host_state.instances.values()) # system_metadata isn't loaded yet, let's do this. - insts.fill_metadata() + # Given all instances are in the same host, we can target the same + # cell. + try: + cell_mapping = objects.CellMapping.get_by_uuid( + ctxt, host_state.cell_uuid) + except exception.CellMappingNotFound: + return weight + with nova_context.target_cell(ctxt, cell_mapping) as cell_ctxt: + insts = objects.InstanceList(cell_ctxt, + objects=host_state.instances.values()) + insts.fill_metadata() for inst in insts: try: diff --git a/nova/tests/functional/regressions/test_bug_2125935.py b/nova/tests/functional/regressions/test_bug_2125935.py index 7308e75a368..45810c0e23e 100644 --- a/nova/tests/functional/regressions/test_bug_2125935.py +++ b/nova/tests/functional/regressions/test_bug_2125935.py @@ -83,14 +83,12 @@ def test_boot(self, mock_debug): 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 + # now the weigher sees that host1 has an instance with the same image + # props mock_debug.assert_any_call( "%s: raw weights %s", "ImagePropertiesWeigher", - {('host2', 'host2'): 0.0, ('host1', 'host1'): 0.0}) + {('host2', 'host2'): 1.0, ('host1', 'host1'): 1.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. @@ -100,13 +98,10 @@ def test_boot(self, mock_debug): 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. + # eventually, the weigher sees the two existing instances on host1 mock_debug.assert_any_call( "%s: raw weights %s", "ImagePropertiesWeigher", - {('host2', 'host2'): 0.0, ('host1', 'host1'): 1.0}) + {('host2', 'host2'): 1.0, ('host1', 'host1'): 2.0}) # server4 is now packed with server1 and server3. self.assertEqual('host1', server4['OS-EXT-SRV-ATTR:host']) diff --git a/nova/tests/unit/objects/test_instance.py b/nova/tests/unit/objects/test_instance.py index 0e041ecd377..b97964c222d 100644 --- a/nova/tests/unit/objects/test_instance.py +++ b/nova/tests/unit/objects/test_instance.py @@ -2066,6 +2066,8 @@ def test_fill_metadata(self): 'host': 'foo'} for i in range(5): + # persist some metadata directly in the DB + values['system_metadata'] = {'BTTF2': '2015'} db.instance_create(self.context, dict(values)) @@ -2089,12 +2091,17 @@ def test_fill_metadata(self): self.assertEqual(0, len([i for i in insts if 'system_metadata' not in i])) + #  Inst 1 is now updated with the new metadata + self.assertEqual({'BTTF1': '1955'}, insts[1].system_metadata) + # Inst 2 should have not had its in-memory copy clobbered self.assertEqual({'bttf3': '1885'}, insts[2].system_metadata) - # Inst 1 should have system_metadata loaded, but empty - self.assertIn('system_metadata', insts[0]) - self.assertEqual({}, insts[0].system_metadata) + # Other instances should have system_metadata loaded directly from the + # DB + for i in [0, 3, 4]: + self.assertIn('system_metadata', insts[i]) + self.assertEqual({'BTTF2': '2015'}, insts[i].system_metadata) def test_fill_metadata_nop(self): insts = objects.InstanceList([objects.Instance(uuid=uuids.inst, diff --git a/nova/tests/unit/scheduler/weights/test_weights_image_props.py b/nova/tests/unit/scheduler/weights/test_weights_image_props.py index c32ef96dafb..2610a6569f7 100644 --- a/nova/tests/unit/scheduler/weights/test_weights_image_props.py +++ b/nova/tests/unit/scheduler/weights/test_weights_image_props.py @@ -16,6 +16,8 @@ from oslo_utils.fixture import uuidsentinel as uuids +from nova import context as nova_context +from nova import exception from nova import objects from nova.scheduler import weights from nova.scheduler.weights import image_props @@ -36,7 +38,7 @@ properties=objects.ImageMetaProps(os_distro='linux', hw_machine_type='pc')) -class ImagePropertiesWeigherTestCase(test.NoDBTestCase): +class _ImagePropertiesWeigherBase(test.NoDBTestCase): def setUp(self): super().setUp() @@ -90,6 +92,9 @@ def _get_all_hosts(self): return [fakes.FakeHostState(host, node, values) for host, node, values in host_values] + +class ImagePropertiesWeigherTestCase(_ImagePropertiesWeigherBase): + @mock.patch('nova.objects.InstanceList.fill_metadata') def test_multiplier_default(self, mock_fm): hostinfo_list = self._get_all_hosts() @@ -194,3 +199,71 @@ def test_multiplier_per_property(self, mock_fm): self.assertEqual('host3', weights[0].obj.host) mock_fm.assert_has_calls([mock.call(), mock.call(), mock.call(), mock.call()]) + + +class TestTargetCellCalled(_ImagePropertiesWeigherBase): + # Using real cell infrastructure instead of SingleCellSimple fixture + # as we need to verify set_target_cell calls are made + USES_DB_SELF = True + + @mock.patch('nova.objects.InstanceList.fill_metadata') + @mock.patch.object(nova_context, 'set_target_cell') + @mock.patch('nova.objects.CellMapping.get_by_uuid') + @mock.patch.object(nova_context.RequestContext, 'from_dict') + @mock.patch.object(nova_context, 'get_admin_context') + def test_target_cell_called(self, mock_get_admin_context, mock_from_dict, + mock_get_by_uuid, mock_target_cell, mock_fm): + fake_context = nova_context.RequestContext(is_admin=True) + fake_cell_ctx = nova_context.RequestContext(is_admin=True) + # let's simulate that we set a cell context as target_cell calls + # from_dict internally + mock_from_dict.return_value = fake_cell_ctx + mock_get_admin_context.return_value = fake_context + self.flags(image_props_weight_multiplier=1.0, group='filter_scheduler') + + fake_cell_mapping = objects.CellMapping(uuid=uuids.cell1) + mock_get_by_uuid.side_effect = [fake_cell_mapping, fake_cell_mapping, + # host3 won't have a cell mapping + exception.CellMappingNotFound( + uuid=uuids.cell2)] + + # Just create three hosts with only one instance, we just want to test + # the calls to target_cell and fill_metadata. + hostinfo_list = [ + fakes.FakeHostState('host1', 'node1', + {'instances': {uuids.inst1: objects.Instance( + uuid=uuids.inst1)}, + 'cell_uuid': uuids.cell1}), + fakes.FakeHostState('host2', 'node2', + {'instances': {}, 'cell_uuid': uuids.cell1}), + # host3 is in a different cell + fakes.FakeHostState('host3', 'node3', + {'instances': {}, 'cell_uuid': uuids.cell2}), + ] + request_spec = objects.RequestSpec(image=PROP_WIN_PC) + + weights = self.weight_handler.get_weighed_objects( + self.weighers, hostinfo_list, weighing_properties=request_spec) + + mock_get_by_uuid.assert_has_calls([ + mock.call(fake_context, uuids.cell1), + mock.call(fake_context, uuids.cell1), + mock.call(fake_context, uuids.cell2)]) + + # Now we see that set_target_cell is called with the cell context only + # twice given host3 does not have a cell mapping + mock_target_cell.assert_has_calls( + [mock.call(fake_cell_ctx, fake_cell_mapping), + mock.call(fake_cell_ctx, fake_cell_mapping)]) + + # Ensure that fill_metadata was called with the correct context + def _fake_fill_metadata(_self): + self.assertEqual(fake_cell_ctx, _self._context) + mock_fm.side_effect = _fake_fill_metadata + + # Double check that we called it only twice + self.assertEqual(2, mock_fm.call_count) + + # host1 wins but we return all three hosts. + self.assertEqual(3, len(weights)) + self.assertEqual('host1', weights[0].obj.host) diff --git a/releasenotes/notes/bug-2125935-de97147d5ff960b5.yaml b/releasenotes/notes/bug-2125935-de97147d5ff960b5.yaml new file mode 100644 index 00000000000..e07482f5313 --- /dev/null +++ b/releasenotes/notes/bug-2125935-de97147d5ff960b5.yaml @@ -0,0 +1,8 @@ +--- +fixes: + - | + Previously the ImagePropertiesWeigher was not running correctly as it wasn't + targeting the correct cell context for getting the system metadata of the + instances. This was fixed in bug `#2125935`_. + + .. _`#2125935`: https://bugs.launchpad.net/nova/+bug/2125935 From 80551125dd7d6c2e81539e03e600de0b68d9b641 Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Tue, 27 Aug 2024 17:16:54 +0100 Subject: [PATCH 3/4] api: Handle empty imageRef alongside null for local BDM check We don't want users creating BDMs with source_type=image, destination_type=local and boot_index=0, since that redefines our root disk definition which should be controlled by the flavor's 'disk' property. We did have a check for this, but it explicitly checked for the 'imageRef' being set to null - which isn't permitted by the schemas -instead of the empty string that is expected when creating a BFV instance. Correct this check. Change-Id: Id3442c3d315e6bb63e6c3675f4ab104885b3884f Signed-off-by: Stephen Finucane Closes-Bug: #2077980 (cherry picked from commit 53eb2cdeab4d25c8842d1f76e074ed3c24323d0d) (cherry picked from commit 96b0126895a0892c55a5a35cb26f71d627738cbb) --- nova/api/openstack/compute/servers.py | 5 +- .../regressions/test_bug_2077980.py | 49 +++++++++++++++++++ 2 files changed, 51 insertions(+), 3 deletions(-) create mode 100644 nova/tests/functional/regressions/test_bug_2077980.py diff --git a/nova/api/openstack/compute/servers.py b/nova/api/openstack/compute/servers.py index 5c1617e7fb8..027b2a7764b 100644 --- a/nova/api/openstack/compute/servers.py +++ b/nova/api/openstack/compute/servers.py @@ -512,12 +512,11 @@ def _process_bdms_for_create( create_kwargs['legacy_bdm'] = True elif block_device_mapping_v2: # Have to check whether --image is given, see bug 1433609 - image_href = server_dict.get('imageRef') - image_uuid_specified = image_href is not None + image_ref = server_dict.get('imageRef') try: block_device_mapping = [ block_device.BlockDeviceDict.from_api(bdm_dict, - image_uuid_specified) + image_uuid_specified=image_ref not in {None, ''}) for bdm_dict in block_device_mapping_v2] except exception.InvalidBDMFormat as e: raise exc.HTTPBadRequest(explanation=e.format_message()) diff --git a/nova/tests/functional/regressions/test_bug_2077980.py b/nova/tests/functional/regressions/test_bug_2077980.py new file mode 100644 index 00000000000..05b27ede7f7 --- /dev/null +++ b/nova/tests/functional/regressions/test_bug_2077980.py @@ -0,0 +1,49 @@ +# 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 oslo_utils.fixture import uuidsentinel as uuids + +from nova.tests.functional.api import client +from nova.tests.functional import integrated_helpers + + +class TestImageToLocalBDM(integrated_helpers._IntegratedTestBase): + """Regression test for bug 2077980 + + This regression test asserts the behaviour of server creation requests when + using a BDM with source_type=image, destination_type=local, and + boot_index=0, which should result in a failure. + """ + microversion = 'latest' + + # TODO(stephenfin): We should eventually fail regardless of the value of + # imageRef, but that requires an API microversion + def test_image_to_local_bdm__empty_image(self): + """Assert behaviour when booting with imageRef set to empty string""" + server = { + 'name': 'test_image_to_local_bdm__null_image', + 'imageRef': '', + 'flavorRef': '1', # m1.tiny from DefaultFlavorsFixture, + 'networks': 'none', + 'block_device_mapping_v2': [{ + 'boot_index': 0, + 'uuid': uuids.image, + 'source_type': 'image', + 'destination_type': 'local'}], + } + # NOTE(stephenfin): This should always fail as we don't allow users to + # set this as it would conflict with the flavor definition. + ex = self.assertRaises( + client.OpenStackApiException, self.api.post_server, + {'server': server}) + self.assertEqual(400, ex.response.status_code) + self.assertIn("Mapping image to local is not supported.", str(ex)) From e59075ba2ab4d388c1d0c690eafde1e391c70d05 Mon Sep 17 00:00:00 2001 From: Nicolai Ruckel Date: Tue, 22 Jul 2025 09:49:55 +0200 Subject: [PATCH 4/4] 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.