From 787d2a130053bd369d35480d6534f01b07c2310d Mon Sep 17 00:00:00 2001 From: melanie witt Date: Mon, 22 Sep 2025 08:34:47 -0700 Subject: [PATCH] Move cleanup of vTPM secret from driver to compute Currently, vTPM secrets are deleted from Barbican any time instance disks are deleted when driver.destroy() is called. This is fine if the instance is also being deleted but if it's not, such as during a resize revert, it will fail with the following error: nova.exception.Invalid: Refusing to create an emulated TPM with no secret! which will bubble up to the API as a HTTP 500. This moves deletion of the vTPM secret from Barbican from the libvirt driver destroy() path to the compute manager _delete_instance() path so that the vTPM secret is deleted only if the instance is being deleted. Closes-Bug: #2125030 Change-Id: I1a43dc0502e1e65b4ef0348610f5eddb43dbff02 Signed-off-by: melanie witt --- nova/compute/manager.py | 4 ++ nova/tests/functional/libvirt/test_vtpm.py | 18 -------- nova/tests/unit/compute/test_compute_mgr.py | 46 +++++++++++++++++++-- nova/tests/unit/virt/libvirt/test_driver.py | 6 ++- nova/virt/libvirt/driver.py | 8 ++-- 5 files changed, 54 insertions(+), 28 deletions(-) diff --git a/nova/compute/manager.py b/nova/compute/manager.py index 022128612e..ff4ada222f 100644 --- a/nova/compute/manager.py +++ b/nova/compute/manager.py @@ -69,6 +69,7 @@ from nova.compute import vm_states from nova import conductor import nova.conf import nova.context +from nova import crypto from nova import exception from nova import exception_wrapper from nova.i18n import _ @@ -963,6 +964,9 @@ class ComputeManager(manager.Manager): self._clean_instance_console_tokens(context, instance) self._delete_scheduler_instance_info(context, instance.uuid) + # Delete the vTPM secret in the key manager service if needed. + crypto.delete_vtpm_secret(context, instance) + def _validate_pinning_configuration(self, instances): if not self.driver.capabilities.get('supports_pcpus', False): return diff --git a/nova/tests/functional/libvirt/test_vtpm.py b/nova/tests/functional/libvirt/test_vtpm.py index e39c87288a..6fb36dc404 100644 --- a/nova/tests/functional/libvirt/test_vtpm.py +++ b/nova/tests/functional/libvirt/test_vtpm.py @@ -434,21 +434,3 @@ class VTPMServersTestNonShared(VTPMServersTest): self.useFixture(fixtures.MockPatch( 'nova.compute.manager.ComputeManager._is_instance_storage_shared', return_value=False)) - - # FIXME: Remove this entire method when - # https://bugs.launchpad.net/nova/+bug/2125030 is fixed. - def test_resize_revert_server__vtpm_to_vtpm_same_config(self): - ex = self.assertRaises( - client.OpenStackApiException, - self._test_resize_revert_server__vtpm_to_vtpm) - self.assertEqual(500, ex.response.status_code) - - # FIXME: Remove this entire method when - # https://bugs.launchpad.net/nova/+bug/2125030 is fixed. - def test_resize_revert_server__vtpm_to_vtpm_different_config(self): - extra_specs = {'hw:tpm_model': 'tpm-tis', 'hw:tpm_version': '2.0'} - ex = self.assertRaises( - client.OpenStackApiException, - self._test_resize_revert_server__vtpm_to_vtpm, - extra_specs=extra_specs) - self.assertEqual(500, ex.response.status_code) diff --git a/nova/tests/unit/compute/test_compute_mgr.py b/nova/tests/unit/compute/test_compute_mgr.py index fe7d236d5e..8f5d014912 100644 --- a/nova/tests/unit/compute/test_compute_mgr.py +++ b/nova/tests/unit/compute/test_compute_mgr.py @@ -1752,6 +1752,40 @@ class ComputeManagerUnitTestCase(test.NoDBTestCase, self.compute._get_power_state, instance) + @mock.patch('nova.scheduler.client.report.SchedulerReportClient.' + 'delete_allocation_for_instance') + @mock.patch('nova.crypto.delete_vtpm_secret') + @ddt.data(0, 3600) + def test__complete_deletion( + self, reclaim_instance_interval, mock_delete_vtpm, + mock_delete_alloc): + self.flags(reclaim_instance_interval=reclaim_instance_interval) + instance = objects.Instance(uuid=uuids.instance) + + with mock.patch.multiple( + self.compute, + _update_resource_tracker=mock.DEFAULT, + _clean_instance_console_tokens=mock.DEFAULT, + _delete_scheduler_instance_info=mock.DEFAULT) as mocks: + self.compute._complete_deletion(self.context, instance) + + mocks['_update_resource_tracker'].assert_called_once_with( + self.context, instance) + mocks['_clean_instance_console_tokens'].assert_called_once_with( + self.context, instance) + mocks['_delete_scheduler_instance_info'].assert_called_once_with( + self.context, instance.uuid) + mock_delete_vtpm.assert_called_once_with(self.context, instance) + # _complete_deletion() is only called at actual delete time (either + # regular delete or when reaping after soft delete). The force argument + # differs based on actual or reap delete for other reasons. + if reclaim_instance_interval > 0: + mock_delete_alloc.assert_called_once_with( + self.context, instance.uuid, force=False) + else: + mock_delete_alloc.assert_called_once_with( + self.context, instance.uuid, force=True) + @mock.patch.object(manager.ComputeManager, '_mount_all_shares') @mock.patch.object(manager.ComputeManager, '_get_share_info') @mock.patch.object(manager.ComputeManager, '_get_power_state') @@ -1792,6 +1826,7 @@ class ComputeManagerUnitTestCase(test.NoDBTestCase, mock_get_share_info.assert_called_once_with(mock.ANY, instance) mock_mount.assert_called_once_with(mock.ANY, instance, share_info) + @mock.patch('nova.crypto.delete_vtpm_secret') @mock.patch.object(objects.BlockDeviceMapping, 'destroy') @mock.patch.object(objects.BlockDeviceMappingList, 'get_by_instance_uuid') @mock.patch.object(objects.Instance, 'destroy') @@ -1800,7 +1835,7 @@ class ComputeManagerUnitTestCase(test.NoDBTestCase, def test_init_instance_complete_partial_deletion( self, mock_ids_from_instance, mock_inst_destroy, mock_obj_load_attr, mock_get_by_instance_uuid, - mock_bdm_destroy): + mock_bdm_destroy, mock_delete_vtpm): """Test to complete deletion for instances in DELETED status but not marked as deleted in the DB """ @@ -1833,9 +1868,10 @@ class ComputeManagerUnitTestCase(test.NoDBTestCase, instance.user_id) mock_inst_destroy.side_effect = fake_inst_destroy() - with mock.patch( - "nova.compute.manager.ComputeManager._get_share_info", - return_value=objects.ShareMappingList(), + with mock.patch.multiple( + self.compute, + _get_share_info=mock.Mock(return_value=objects.ShareMappingList()), + _clean_instance_console_tokens=mock.DEFAULT, ): self.compute._init_instance(self.context, instance) @@ -1843,6 +1879,8 @@ class ComputeManagerUnitTestCase(test.NoDBTestCase, # instance was deleted from db. self.assertNotEqual(0, instance.deleted) + mock_delete_vtpm.assert_called_once_with(self.context, instance) + @mock.patch('nova.compute.manager.LOG') def test_init_instance_complete_partial_deletion_raises_exception( self, mock_log): diff --git a/nova/tests/unit/virt/libvirt/test_driver.py b/nova/tests/unit/virt/libvirt/test_driver.py index c25ea49e14..ccdf992153 100644 --- a/nova/tests/unit/virt/libvirt/test_driver.py +++ b/nova/tests/unit/virt/libvirt/test_driver.py @@ -21756,7 +21756,8 @@ class LibvirtConnTestCase(test.NoDBTestCase, mock_unplug.assert_called_once_with(fake_inst, 'netinfo', True) mock_get_mapping.assert_called_once_with(None) mock_delete_files.assert_called_once_with(fake_inst) - mock_delete_vtpm.assert_called_once_with('ctxt', 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) @mock.patch('nova.virt.libvirt.driver.LibvirtDriver._undefine_domain') @@ -21780,7 +21781,8 @@ class LibvirtConnTestCase(test.NoDBTestCase, instance_save.side_effect = exception.InstanceNotFound( instance_id=uuids.instance) drvr.cleanup('ctxt', fake_inst, 'netinfo') - mock_delete_vtpm.assert_called_once_with('ctxt', 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) @mock.patch.object(libvirt_driver.LibvirtDriver, 'delete_instance_files', diff --git a/nova/virt/libvirt/driver.py b/nova/virt/libvirt/driver.py index 4375dc754b..4a8d7718bf 100644 --- a/nova/virt/libvirt/driver.py +++ b/nova/virt/libvirt/driver.py @@ -1649,7 +1649,8 @@ class LibvirtDriver(driver.ComputeDriver): :param destroy_disks: if local ephemeral disks should be destroyed :param migrate_data: optional migrate_data object :param destroy_vifs: if plugged vifs should be unplugged - :param destroy_secrets: Indicates if secrets should be destroyed + :param destroy_secrets: Indicates if libvirt secrets for Cinder volume + encryption should be destroyed """ cleanup_instance_dir = False cleanup_instance_disks = False @@ -1703,8 +1704,8 @@ class LibvirtDriver(driver.ComputeDriver): :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 secrets should - be deleted. + :param destroy_secrets: If the cinder volume encryption libvirt secrets + should be deleted. """ # zero the data on backend pmem device vpmems = self._get_vpmems(instance) @@ -1769,7 +1770,6 @@ class LibvirtDriver(driver.ComputeDriver): pass if cleanup_instance_disks: - crypto.delete_vtpm_secret(context, instance) # Make sure that the instance directory files were successfully # deleted before destroying the encryption secrets in the case of # image backends that are not 'lvm' or 'rbd'. We don't want to