Replace blind retry with libvirt event waiting in detach

Nova so far applied a retry loop that tried to periodically detach the
device from libvirt while the device was visible in the domain xml. This
could lead to an issue where an already progressing detach on the
libvirt side is interrupted by nova re-sending the detach request for
the same device. See bug #1882521 for more information.

Also if there was both a persistent and a live domain the nova tried the
detach from both at the same call. This lead to confusion about the
result when such call failed. Was the detach failed partially?

We can do better, at least for the live detach case. Based on the
libvirt developers detaching from the persistent domain always
succeeds and it is a synchronous process. Detaching from the live
domain can be both synchronous or asynchronous depending on the guest
OS and the load on the hypervisor. But for live detach libvirt always
sends an event [1] nova can wait for.

So this patch does two things.

1) Separates the detach from the persistent domain from the detach from
   the live domain to make the error cases clearer.

2) Changes the retry mechanism.

   Detaching from the persistent domain is not retried. If libvirt
   reports device not found, while both persistent and live detach
   is needed, the error is ignored, and the process continues with
   the live detach. In any other case the error considered as fatal.

   Detaching from the live domain is changed to always wait for the
   libvirt event. In case of timeout, the live detach is retried.
   But a failure event from libvirt considered fatal, based on the
   information from the libvirt developers, so in this case the
   detach is not retried.

Related-Bug: #1882521

[1]https://libvirt.org/html/libvirt-libvirt-domain.html#virConnectDomainEventDeviceRemovedCallback

Change-Id: I7f2b6330decb92e2838aa7cee47fb228f00f47da
(cherry picked from commit e56cc4f)

Author: Balazs Gibizer

nova/conf/libvirt.py

@@ -875,6 +875,30 @@
 * It's recommended to consider including ``x86_64=q35`` in
   :oslo.config:option:`libvirt.hw_machine_type`; see
   :ref:`deploying-sev-capable-infrastructure` for more on this.
+"""),
+    cfg.IntOpt('device_detach_attempts',
+               default=8,
+               min=1,
+               help="""
+Maximum number of attempts the driver tries to detach a device in libvirt.
+
+Related options:
+
+* :oslo.config:option:`libvirt.device_detach_timeout`
+
+"""),
+    cfg.IntOpt('device_detach_timeout',
+               default=20,
+               min=1,
+               help="""
+Maximum number of seconds the driver waits for the success or the failure
+event from libvirt for a given device detach attempt before it re-trigger the
+detach.
+
+Related options:
+
+* :oslo.config:option:`libvirt.device_detach_attempts`
+
 """),
 ]
 

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

@@ -1278,9 +1278,17 @@ def attachDeviceFlags(self, xml, flags):
         self.attachDevice(xml)
 
     def detachDevice(self, xml):
+        # TODO(gibi): this should handle nics similarly to attachDevice()
         disk_info = _parse_disk_info(etree.fromstring(xml))
-        disk_info['_attached'] = True
-        return disk_info in self._def['devices']['disks']
+        attached_disk_info = None
+        for attached_disk in self._def['devices']['disks']:
+            if attached_disk['target_dev'] == disk_info.get('target_dev'):
+                attached_disk_info = attached_disk
+                break
+
+        if attached_disk_info:
+            self._def['devices']['disks'].remove(attached_disk_info)
+        return attached_disk_info is not None
 
     def detachDeviceFlags(self, xml, flags):
         self.detachDevice(xml)

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

@@ -21,7 +21,6 @@
 from oslo_utils import encodeutils
 
 from nova import context
-from nova import exception
 from nova import test
 from nova.tests.unit.virt.libvirt import fakelibvirt
 from nova.virt.libvirt import config as vconfig
@@ -213,212 +212,6 @@ def test_detach_device_persistent_live(self):
             "</xml>", flags=(fakelibvirt.VIR_DOMAIN_AFFECT_CONFIG |
                              fakelibvirt.VIR_DOMAIN_AFFECT_LIVE))
 
-    def test_detach_device_with_retry_from_transient_domain(self):
-        conf = mock.Mock(spec=vconfig.LibvirtConfigGuestDevice)
-        conf.to_xml.return_value = "</xml>"
-        get_config = mock.Mock()
-        get_config.side_effect = [conf, conf, conf, None, None]
-        dev_path = "/dev/vdb"
-        self.domain.isPersistent.return_value = False
-        retry_detach = self.guest.detach_device_with_retry(
-            get_config, dev_path, live=True, inc_sleep_time=.01)
-        self.domain.detachDeviceFlags.assert_called_once_with(
-            "</xml>", flags=fakelibvirt.VIR_DOMAIN_AFFECT_LIVE)
-        self.domain.detachDeviceFlags.reset_mock()
-        retry_detach()
-        self.assertEqual(1, self.domain.detachDeviceFlags.call_count)
-
-    def test_detach_device_with_retry_detach_success(self):
-        conf = mock.Mock(spec=vconfig.LibvirtConfigGuestDevice)
-        conf.to_xml.return_value = "</xml>"
-        get_config = mock.Mock()
-        # Force multiple retries of detach
-        get_config.side_effect = [conf, conf, conf, conf, conf, None, None]
-        dev_path = "/dev/vdb"
-        self.domain.isPersistent.return_value = True
-
-        retry_detach = self.guest.detach_device_with_retry(
-            get_config, dev_path, live=True, inc_sleep_time=.01)
-        # Ensure we've only done the initial detach call
-        self.domain.detachDeviceFlags.assert_called_once_with(
-            "</xml>", flags=(fakelibvirt.VIR_DOMAIN_AFFECT_CONFIG |
-                             fakelibvirt.VIR_DOMAIN_AFFECT_LIVE))
-
-        get_config.assert_called_with(dev_path)
-
-        # Some time later, we can do the wait/retry to ensure detach succeeds
-        self.domain.detachDeviceFlags.reset_mock()
-        retry_detach()
-        # Should have two retries before we pretend device is detached
-        self.assertEqual(2, self.domain.detachDeviceFlags.call_count)
-
-    def test_detach_device_with_retry_detach_failure(self):
-        conf = mock.Mock(spec=vconfig.LibvirtConfigGuestDevice)
-        conf.to_xml.return_value = "</xml>"
-        # Continue to return some value for the disk config
-        get_config = mock.Mock(return_value=conf)
-        self.domain.isPersistent.return_value = True
-
-        retry_detach = self.guest.detach_device_with_retry(
-            get_config, "/dev/vdb", live=True, inc_sleep_time=.01,
-            max_retry_count=3)
-        # Ensure we've only done the initial detach call
-        self.domain.detachDeviceFlags.assert_called_once_with(
-            "</xml>", flags=(fakelibvirt.VIR_DOMAIN_AFFECT_CONFIG |
-                             fakelibvirt.VIR_DOMAIN_AFFECT_LIVE))
-
-        # Some time later, we can do the wait/retry to ensure detach
-        self.domain.detachDeviceFlags.reset_mock()
-        # Should hit max # of retries
-        self.assertRaises(exception.DeviceDetachFailed, retry_detach)
-        self.assertEqual(4, self.domain.detachDeviceFlags.call_count)
-
-    def test_detach_device_with_retry_device_not_found(self):
-        get_config = mock.Mock(return_value=None)
-        self.domain.isPersistent.return_value = True
-        ex = self.assertRaises(
-            exception.DeviceNotFound, self.guest.detach_device_with_retry,
-            get_config, "/dev/vdb", live=True)
-        self.assertIn("/dev/vdb", str(ex))
-
-    def test_detach_device_with_retry_device_not_found_alt_name(self):
-        """Tests to make sure we use the alternative name in errors."""
-        get_config = mock.Mock(return_value=None)
-        self.domain.isPersistent.return_value = True
-        ex = self.assertRaises(
-            exception.DeviceNotFound, self.guest.detach_device_with_retry,
-            get_config, mock.sentinel.device, live=True,
-            alternative_device_name='foo')
-        self.assertIn('foo', str(ex))
-
-    @mock.patch.object(libvirt_guest.Guest, "detach_device")
-    def _test_detach_device_with_retry_second_detach_failure(
-            self, mock_detach, error_code=None, error_message=None,
-            supports_device_missing=False):
-        # This simulates a retry of the transient/live domain detach
-        # failing because the device is not found
-        conf = mock.Mock(spec=vconfig.LibvirtConfigGuestDevice)
-        conf.to_xml.return_value = "</xml>"
-        self.domain.isPersistent.return_value = True
-
-        get_config = mock.Mock(return_value=conf)
-        fake_device = "vdb"
-        fake_exc = fakelibvirt.make_libvirtError(
-            fakelibvirt.libvirtError, "",
-            error_message=error_message,
-            error_code=error_code,
-            error_domain=fakelibvirt.VIR_FROM_DOMAIN)
-        mock_detach.side_effect = [None, fake_exc]
-        retry_detach = self.guest.detach_device_with_retry(
-            get_config, fake_device, live=True,
-            inc_sleep_time=.01, max_retry_count=3)
-        # Some time later, we can do the wait/retry to ensure detach
-        self.assertRaises(exception.DeviceNotFound, retry_detach)
-        # Check that the save_and_reraise_exception context manager didn't log
-        # a traceback when the libvirtError was caught and DeviceNotFound was
-        # raised.
-        self.assertNotIn('Original exception being dropped',
-                         self.stdlog.logger.output)
-
-    def test_detach_device_with_retry_second_detach_device_missing(self):
-        self._test_detach_device_with_retry_second_detach_failure(
-            error_code=fakelibvirt.VIR_ERR_DEVICE_MISSING,
-            error_message="device not found: disk vdb not found",
-            supports_device_missing=True)
-
-    def _test_detach_device_with_retry_first_detach_failure(
-            self, error_code=None, error_message=None,
-            supports_device_missing=False):
-        # This simulates a persistent or live domain detach failing because the
-        # device is not found during the first attempt to detach the device.
-        # We should still attempt to detach the device from the live config if
-        # the detach from persistent failed OR we should retry the detach from
-        # the live config if the first detach from live config failed.
-        # Note that the side effects in this test case [fake_exc, None] could
-        # not happen in real life if the first detach failed because the detach
-        # from live raised not found. In real life, the second attempt to
-        # detach from live would raise not found again because the device is
-        # not present. The purpose of this test is to verify that we try to
-        # detach a second time if the first detach fails, so we are OK with the
-        # unrealistic side effects for detach from live failing the first time.
-        conf = mock.Mock(spec=vconfig.LibvirtConfigGuestDevice)
-        conf.to_xml.return_value = "</xml>"
-        self.domain.isPersistent.return_value = True
-
-        get_config = mock.Mock()
-        # Simulate an inactive or live detach attempt which fails (not found)
-        # followed by a live config detach attempt that is successful
-        get_config.side_effect = [conf, conf, conf, None, None]
-        fake_device = "vdb"
-        fake_exc = fakelibvirt.make_libvirtError(
-            fakelibvirt.libvirtError, "",
-            error_message=error_message,
-            error_code=error_code,
-            error_domain=fakelibvirt.VIR_FROM_DOMAIN)
-        # Detach from persistent or live raises not found, detach from live
-        # succeeds afterward
-        self.domain.detachDeviceFlags.side_effect = [fake_exc, None]
-        retry_detach = self.guest.detach_device_with_retry(get_config,
-            fake_device, live=True, inc_sleep_time=.01, max_retry_count=3)
-        # We should have tried to detach from the persistent domain
-        self.domain.detachDeviceFlags.assert_called_once_with(
-            "</xml>", flags=(fakelibvirt.VIR_DOMAIN_AFFECT_CONFIG |
-                             fakelibvirt.VIR_DOMAIN_AFFECT_LIVE))
-        # During the retry detach, should detach from the live domain
-        self.domain.detachDeviceFlags.reset_mock()
-        retry_detach()
-        # We should have tried to detach from the live domain
-        self.domain.detachDeviceFlags.assert_called_once_with(
-            "</xml>", flags=fakelibvirt.VIR_DOMAIN_AFFECT_LIVE)
-
-    def test_detach_device_with_retry_first_detach_device_missing(self):
-        self._test_detach_device_with_retry_first_detach_failure(
-            error_code=fakelibvirt.VIR_ERR_DEVICE_MISSING,
-            error_message="device not found: disk vdb not found",
-            supports_device_missing=True)
-
-    def test_detach_device_with_already_in_process_of_unplug_error(self):
-        # Assert that DeviceNotFound is raised when encountering
-        # https://bugzilla.redhat.com/show_bug.cgi?id=1878659
-        # This is raised as QEMU returns a VIR_ERR_INTERNAL_ERROR when
-        # a request to device_del is made while another is about to complete.
-
-        self.domain.isPersistent.return_value = True
-        conf = mock.Mock(spec=vconfig.LibvirtConfigGuestDevice)
-        conf.to_xml.return_value = "</xml>"
-
-        existing_unplug_exc = fakelibvirt.make_libvirtError(
-            fakelibvirt.libvirtError, "",
-            error_message='device vdb is already in the process of unplug',
-            error_code=fakelibvirt.VIR_ERR_INTERNAL_ERROR,
-            error_domain=fakelibvirt.VIR_FROM_DOMAIN
-        )
-        device_missing_exc = fakelibvirt.make_libvirtError(
-            fakelibvirt.libvirtError, "",
-            error_message='device not found: disk vdb not found',
-            error_code=fakelibvirt.VIR_ERR_DEVICE_MISSING,
-            error_domain=fakelibvirt.VIR_FROM_DOMAIN
-        )
-
-        # Raise VIR_ERR_INTERNAL_ERROR on the second call before raising
-        # VIR_ERR_DEVICE_MISSING to mock the first call successfully detaching
-        # the device asynchronously.
-        self.domain.detachDeviceFlags.side_effect = [
-            None,
-            existing_unplug_exc,
-            device_missing_exc
-        ]
-
-        retry_detach = self.guest.detach_device_with_retry(
-            mock.Mock(return_value=conf),
-            'vdb',
-            live=True,
-            inc_sleep_time=.01
-        )
-
-        # Assert that we raise exception.DeviceNotFound
-        self.assertRaises(exception.DeviceNotFound, retry_detach)
-
     def test_get_xml_desc(self):
         self.guest.get_xml_desc()
         self.domain.XMLDesc.assert_called_once_with(flags=0)

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

@@ -416,6 +416,7 @@ def test_get_volume_connector_storage_ip(self):
         self.assertEqual(storage_ip, result['ip'])
 
     @catch_notimplementederror
+    @mock.patch('threading.Event.wait', new=mock.Mock())
     @mock.patch.object(libvirt.driver.LibvirtDriver, '_build_device_metadata',
                        return_value=objects.InstanceDeviceMetadata())
     def test_attach_detach_volume(self, _):
@@ -452,6 +453,7 @@ def test_swap_volume(self, _):
                                         '/dev/sda', 2))
 
     @catch_notimplementederror
+    @mock.patch('threading.Event.wait', new=mock.Mock())
     @mock.patch.object(libvirt.driver.LibvirtDriver, '_build_device_metadata',
                        return_value=objects.InstanceDeviceMetadata())
     def test_attach_detach_different_power_states(self, _):