Merge "libvirt: Do not destroy volume secrets during _hard_reboot"

Author: Zuul

nova/tests/unit/virt/libvirt/test_driver.py

@@ -9049,6 +9049,19 @@ def test_disconnect_volume_luks(self, mock_get_volume_encryptor):
                                                          uuids.volume_id)
         mock_encryptor.detach_volume.assert_not_called()
 
+        # assert that no attempt to remove the secret is made when
+        # destroy_secrets=False
+        drvr._host.find_secret.reset_mock()
+        drvr._host.delete_secret.reset_mock()
+        drvr._disconnect_volume(
+            self.context,
+            connection_info,
+            instance,
+            encryption=encryption,
+            destroy_secrets=False
+        )
+        drvr._host.delete_secret.assert_not_called()
+
         # assert that the encryptor is used if no secret is found
         drvr._host.find_secret.reset_mock()
         drvr._host.delete_secret.reset_mock()
@@ -10112,6 +10125,36 @@ def test_detach_encryptor_native_luks_device_path_secret_missing(
         mock_find_secret.assert_called_once_with('volume', uuids.volume_id)
         mock_get_encryptor.assert_not_called()
 
+    @mock.patch('nova.virt.libvirt.host.Host.delete_secret')
+    @mock.patch('nova.virt.libvirt.host.Host.find_secret', new=mock.Mock())
+    def test_detach_encryptor_skip_secret_removal(self, mock_delete_secret):
+        drvr = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), False)
+        drvr._detach_encryptor(
+            self.context,
+            {
+                'data': {
+                    'volume_id': uuids.volume_id
+                }
+            },
+            None,
+            destroy_secrets=False
+        )
+        # Assert that no attempt is made to delete the volume secert
+        mock_delete_secret.assert_not_called()
+
+        drvr._detach_encryptor(
+            self.context,
+            {
+                'data': {
+                    'volume_id': uuids.volume_id
+                }
+            },
+            None,
+            destroy_secrets=True
+        )
+        # Assert that volume secert is deleted
+        mock_delete_secret.assert_called_once_with('volume', uuids.volume_id)
+
     def test_allow_native_luksv1(self):
         drvr = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), False)
         self.assertFalse(drvr._allow_native_luksv1({}))
@@ -15752,7 +15795,8 @@ def destroy_side_effect(*args, **kwargs):
             mock_domain_destroy.assert_called_once_with()
             mock_teardown_container.assert_called_once_with(instance)
             mock_cleanup.assert_called_once_with(self.context, instance,
-                                                 network_info, None, False)
+                                                 network_info, None, False,
+                                                 destroy_secrets=True)
 
     @mock.patch.object(libvirt_driver.LibvirtDriver, 'cleanup')
     @mock.patch.object(libvirt_driver.LibvirtDriver, '_teardown_container')
@@ -15772,7 +15816,8 @@ def test_destroy_lxc_calls_teardown_container_when_no_domain(self,
                                           mock.call(instance)])
         mock_teardown_container.assert_called_once_with(instance)
         mock_cleanup.assert_called_once_with(self.context, instance,
-                                             network_info, None, False)
+                                             network_info, None, False,
+                                             destroy_secrets=True)
 
     @mock.patch.object(host.Host, 'get_guest')
     def test_reboot_different_ids(self, mock_get):
@@ -15993,7 +16038,8 @@ def test_hard_reboot(self, mock_get_mdev, mock_destroy, mock_get_disk_info,
         mock_get_mdev.assert_called_once_with(instance)
         mock_destroy.assert_called_once_with(self.context, instance,
                 network_info, destroy_disks=False,
-                block_device_info=block_device_info)
+                block_device_info=block_device_info,
+                destroy_secrets=False)
 
         mock_get_guest_xml.assert_called_once_with(self.context, instance,
             network_info, mock.ANY, mock.ANY,
@@ -19278,6 +19324,59 @@ def test_cleanup_instance_dir_with_rbd_workaround(self,
         self.assertTrue(instance.cleaned)
         save.assert_called_once_with()
 
+    @mock.patch('nova.virt.libvirt.driver.LibvirtDriver._disconnect_volume')
+    @mock.patch('nova.virt.libvirt.driver.LibvirtDriver._undefine_domain',
+                new=mock.Mock())
+    @mock.patch('nova.virt.libvirt.driver.LibvirtDriver._get_vpmems',
+                new=mock.Mock(return_value=None))
+    def test_cleanup_destroy_secrets(self, mock_disconnect_volume):
+        block_device_info = {
+            'block_device_mapping': [
+                {
+                    'connection_info': mock.sentinel.connection_info
+                }
+            ]
+        }
+        instance = objects.Instance(self.context, **self.test_instance)
+        drvr = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI())
+
+        # Pass destroy_vifs=False and destroy_disks=False as we only care about
+        # asserting the behaviour of destroy_secrets in this test.
+        drvr.cleanup(
+            self.context,
+            instance,
+            network_info={},
+            block_device_info=block_device_info,
+            destroy_vifs=False,
+            destroy_disks=False,
+            destroy_secrets=False
+        )
+        drvr.cleanup(
+            self.context,
+            instance,
+            network_info={},
+            block_device_info=block_device_info,
+            destroy_vifs=False,
+            destroy_disks=False,
+        )
+
+        # Assert that disconnect_volume is called with destroy_secrets=False
+        # and destroy_secrets=True by default
+        mock_disconnect_volume.assert_has_calls([
+            mock.call(
+                self.context,
+                mock.sentinel.connection_info,
+                instance,
+                destroy_secrets=False
+            ),
+            mock.call(
+                self.context,
+                mock.sentinel.connection_info,
+                instance,
+                destroy_secrets=True
+            )
+        ])
+
     @mock.patch.object(libvirt_driver.LibvirtDriver, '_get_volume_encryption')
     @mock.patch.object(libvirt_driver.LibvirtDriver, '_allow_native_luksv1')
     def test_swap_volume_native_luks_blocked(self, mock_allow_native_luksv1,

nova/virt/libvirt/driver.py

@@ -1411,7 +1411,7 @@ def destroy(self, context, instance, network_info, block_device_info=None,
         # unblock the waiting threads and clean up.
         self._device_event_handler.cleanup_waiters(instance.uuid)
         self.cleanup(context, instance, network_info, block_device_info,
-                     destroy_disks)
+                     destroy_disks, destroy_secrets=destroy_secrets)
 
     def _undefine_domain(self, instance):
         try:
@@ -1485,11 +1485,12 @@ def cleanup(self, context, instance, network_info, block_device_info=None,
                 block_device_info=block_device_info,
                 destroy_vifs=destroy_vifs,
                 cleanup_instance_dir=cleanup_instance_dir,
-                cleanup_instance_disks=cleanup_instance_disks)
+                cleanup_instance_disks=cleanup_instance_disks,
+                destroy_secrets=destroy_secrets)
 
     def _cleanup(self, context, instance, network_info, block_device_info=None,
                  destroy_vifs=True, cleanup_instance_dir=False,
-                 cleanup_instance_disks=False):
+                 cleanup_instance_disks=False, destroy_secrets=True):
         """Cleanup the domain and any attached resources from the host.
 
         This method cleans up any pmem devices, unplugs VIFs, disconnects
@@ -1530,7 +1531,9 @@ def _cleanup(self, context, instance, network_info, block_device_info=None,
                 continue
 
             try:
-                self._disconnect_volume(context, connection_info, instance)
+                self._disconnect_volume(
+                    context, connection_info, instance,
+                    destroy_secrets=destroy_secrets)
             except Exception as exc:
                 with excutils.save_and_reraise_exception() as ctxt:
                     if cleanup_instance_disks:
@@ -1847,8 +1850,13 @@ def _should_disconnect_target(self, context, instance, multiattach,
         return (False if connection_count > 1 else True)
 
     def _disconnect_volume(self, context, connection_info, instance,
-                           encryption=None):
-        self._detach_encryptor(context, connection_info, encryption=encryption)
+                           encryption=None, destroy_secrets=True):
+        self._detach_encryptor(
+            context,
+            connection_info,
+            encryption=encryption,
+            destroy_secrets=destroy_secrets
+        )
         vol_driver = self._get_volume_driver(connection_info)
         volume_id = driver_block_device.get_volume_id(connection_info)
         multiattach = connection_info.get('multiattach', False)
@@ -1961,7 +1969,8 @@ def _attach_encryptor(self, context, connection_info, encryption):
                                                    encryption)
             encryptor.attach_volume(context, **encryption)
 
-    def _detach_encryptor(self, context, connection_info, encryption):
+    def _detach_encryptor(self, context, connection_info, encryption,
+                          destroy_secrets=True):
         """Detach the frontend encryptor if one is required by the volume.
 
         The request context is only used when an encryption metadata dict is
@@ -1973,7 +1982,11 @@ def _detach_encryptor(self, context, connection_info, encryption):
         """
         volume_id = driver_block_device.get_volume_id(connection_info)
         if volume_id and self._host.find_secret('volume', volume_id):
+            if not destroy_secrets:
+                LOG.debug("Skipping volume secret destruction")
+                return
             return self._host.delete_secret('volume', volume_id)
+
         if encryption is None:
             encryption = self._get_volume_encryption(context, connection_info)
 
@@ -3729,7 +3742,8 @@ def _hard_reboot(self, context, instance, network_info,
         # we can here without losing data. This allows us to re-initialise from
         # scratch, and hopefully fix, most aspects of a non-functioning guest.
         self.destroy(context, instance, network_info, destroy_disks=False,
-                     block_device_info=block_device_info)
+                     block_device_info=block_device_info,
+                     destroy_secrets=False)
 
         # Convert the system metadata to image metadata
         # NOTE(mdbooth): This is a workaround for stateless Nova compute