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 <melwittt@gmail.com>

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

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)

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):

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',

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