From 541336a688478c8a09d28e73ccf7c418c1f308ec Mon Sep 17 00:00:00 2001 From: Lee Yarwood Date: Fri, 17 Jul 2020 00:45:10 +0100 Subject: [PATCH] libvirt: Handle VIR_ERR_DEVICE_MISSING when detaching devices Introduced in libvirt v4.1.0 [1] this error code replaces the previously raised VIR_ERR_INVALID_ARG, VIR_ERR_OPERATION_FAILED and VIR_ERR_INVALID_ARG codes [2][3]. VIR_ERR_OPERATION_FAILED was introduced and tested as an active/live/hot unplug config device detach error code in I131aaf28d2f5d5d964d4045e3d7d62207079cfb0. VIR_ERR_INTERNAL_ERROR was introduced and tested as an active/live/hot unplug config device detach error code in I3055cd7641de92ab188de73733ca9288a9ca730a. VIR_ERR_INVALID_ARG was introduced and tested as an inactive/persistent/cold unplug config device detach error code in I09230fc47b0950aa5a3db839a070613c9c817576. This change introduces support for the new VIR_ERR_DEVICE_MISSING error code while also retaining coverage for these codes until MIN_LIBVIRT_VERSION is bumped past v4.1.0. The majority of this change is test code motion with the existing tests being modified to run against either the active or inactive versions of the above error codes for the time being. test_detach_device_with_retry_operation_internal and test_detach_device_with_retry_invalid_argument_no_live have been removed as they duplicate the logic within the now refactored _test_detach_device_with_retry_second_detach_failure. [1] https://libvirt.org/git/?p=libvirt.git;a=commit;h=bb189c8e8c93f115c13fa3bfffdf64498f3f0ce1 [2] https://libvirt.org/git/?p=libvirt.git;a=commit;h=126db34a81bc9f9f9710408f88cceaa1e34bbbd7 [3] https://libvirt.org/git/?p=libvirt.git;a=commit;h=2f54eab7c7c618811de23c60a51e910274cf30de Closes-Bug: #1887946 Change-Id: I7eb86edc130d186a66c04b229d46347ec5c0b625 (cherry picked from commit 902f09af251d2b2e56fb2f2900a3510baf38a508) (cherry picked from commit 93058ae1b8bc1b1728f08b9e606b68318751fc3b) (cherry picked from commit 863d6ef7601302901fa3368ea8457b3564eeb501) (cherry picked from commit 76428c1a6a7796391957a3e83207f85cfe924505) (cherry picked from commit 74b053f47a659a0250d051020d6c8b4e3c256e7d) --- nova/tests/unit/virt/libvirt/fakelibvirt.py | 1 + nova/tests/unit/virt/libvirt/test_guest.py | 122 ++++++++++---------- nova/virt/libvirt/guest.py | 10 +- 3 files changed, 73 insertions(+), 60 deletions(-) diff --git a/nova/tests/unit/virt/libvirt/fakelibvirt.py b/nova/tests/unit/virt/libvirt/fakelibvirt.py index 69b1f748854f..ecb764081064 100644 --- a/nova/tests/unit/virt/libvirt/fakelibvirt.py +++ b/nova/tests/unit/virt/libvirt/fakelibvirt.py @@ -129,6 +129,7 @@ VIR_ERR_NO_SECRET = 66 VIR_ERR_AGENT_UNRESPONSIVE = 86 VIR_ERR_ARGUMENT_UNSUPPORTED = 74 VIR_ERR_OPERATION_UNSUPPORTED = 84 +VIR_ERR_DEVICE_MISSING = 99 # Readonly VIR_CONNECT_RO = 1 diff --git a/nova/tests/unit/virt/libvirt/test_guest.py b/nova/tests/unit/virt/libvirt/test_guest.py index 70299dfe1ee3..f8475518bc78 100644 --- a/nova/tests/unit/virt/libvirt/test_guest.py +++ b/nova/tests/unit/virt/libvirt/test_guest.py @@ -303,7 +303,9 @@ class GuestTestCase(test.NoDBTestCase): self.assertIn('foo', six.text_type(ex)) @mock.patch.object(libvirt_guest.Guest, "detach_device") - def test_detach_device_with_retry_operation_failed(self, mock_detach): + def _test_detach_device_with_retry_second_detach_failure( + self, mock_detach, error_code=fakelibvirt.VIR_ERR_DEVICE_MISSING, + error_message="device not found: disk vdb not found"): # This simulates a retry of the transient/live domain detach # failing because the device is not found conf = mock.Mock(spec=vconfig.LibvirtConfigGuestDevice) @@ -314,56 +316,62 @@ class GuestTestCase(test.NoDBTestCase): fake_device = "vdb" fake_exc = fakelibvirt.make_libvirtError( fakelibvirt.libvirtError, "", - error_message="operation failed: disk vdb not found", + 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) + + # TODO(lyarwood): Remove this test once MIN_LIBVIRT_VERSION is >= 4.1.0 + def test_detach_device_with_retry_second_detach_operation_failed(self): + self._test_detach_device_with_retry_second_detach_failure( error_code=fakelibvirt.VIR_ERR_OPERATION_FAILED, - 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) + error_message="operation failed: disk vdb not found") - @mock.patch.object(libvirt_guest.Guest, "detach_device") - def test_detach_device_with_retry_operation_internal(self, mock_detach): - # 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 = "" - 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="operation failed: disk vdb not found", + # TODO(lyarwood): Remove this test once MIN_LIBVIRT_VERSION is >= 4.1.0 + def test_detach_device_with_retry_second_detach_internal_error(self): + self._test_detach_device_with_retry_second_detach_failure( error_code=fakelibvirt.VIR_ERR_INTERNAL_ERROR, - 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) + error_message="operation failed: disk vdb not found") - def test_detach_device_with_retry_invalid_argument(self): - # This simulates a persistent domain detach failing because - # the device is not found + def test_detach_device_with_retry_second_detach_device_missing(self): + self._test_detach_device_with_retry_second_detach_failure() + + def _test_detach_device_with_retry_first_detach_failure( + self, error_code=fakelibvirt.VIR_ERR_DEVICE_MISSING, + error_message="device not found: disk vdb not found"): + # 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 = "" self.domain.isPersistent.return_value = True get_config = mock.Mock() - # Simulate the persistent domain attach attempt followed by the live - # domain attach attempt and success + # 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="invalid argument: no target device vdb", - error_code=fakelibvirt.VIR_ERR_INVALID_ARG, + error_message=error_message, + error_code=error_code, error_domain=fakelibvirt.VIR_FROM_DOMAIN) - # Detach from persistent raises not found, detach from live succeeds + # 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) @@ -378,30 +386,26 @@ class GuestTestCase(test.NoDBTestCase): self.domain.detachDeviceFlags.assert_called_once_with( "", flags=fakelibvirt.VIR_DOMAIN_AFFECT_LIVE) - def test_detach_device_with_retry_invalid_argument_no_live(self): - # This simulates a persistent domain detach failing because - # the device is not found - conf = mock.Mock(spec=vconfig.LibvirtConfigGuestDevice) - conf.to_xml.return_value = "" - self.domain.isPersistent.return_value = True + # TODO(lyarwood): Remove this test once MIN_LIBVIRT_VERSION is >= 4.1.0 + def test_detach_device_with_retry_first_detach_operation_failed(self): + self._test_detach_device_with_retry_first_detach_failure( + error_code=fakelibvirt.VIR_ERR_OPERATION_FAILED, + error_message="operation failed: disk vdb not found") - get_config = mock.Mock() - # Simulate the persistent domain attach attempt - get_config.return_value = conf - fake_device = "vdb" - fake_exc = fakelibvirt.make_libvirtError( - fakelibvirt.libvirtError, "", - error_message="invalid argument: no target device vdb", + # TODO(lyarwood): Remove this test once MIN_LIBVIRT_VERSION is >= 4.1.0 + def test_detach_device_with_retry_first_detach_internal_error(self): + self._test_detach_device_with_retry_first_detach_failure( + error_code=fakelibvirt.VIR_ERR_INTERNAL_ERROR, + error_message="operation failed: disk vdb not found") + + # TODO(lyarwood): Remove this test once MIN_LIBVIRT_VERSION is >= 4.1.0 + def test_detach_device_with_retry_first_detach_invalid_arg(self): + self._test_detach_device_with_retry_first_detach_failure( error_code=fakelibvirt.VIR_ERR_INVALID_ARG, - error_domain=fakelibvirt.VIR_FROM_DOMAIN) - # Detach from persistent raises not found - self.domain.detachDeviceFlags.side_effect = fake_exc - self.assertRaises(exception.DeviceNotFound, - self.guest.detach_device_with_retry, get_config, - fake_device, live=False, 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( - "", flags=fakelibvirt.VIR_DOMAIN_AFFECT_CONFIG) + error_message="invalid argument: no target device vdb") + + def test_detach_device_with_retry_first_detach_device_missing(self): + self._test_detach_device_with_retry_first_detach_failure() def test_get_xml_desc(self): self.guest.get_xml_desc() diff --git a/nova/virt/libvirt/guest.py b/nova/virt/libvirt/guest.py index 573d800bf714..33713b8e9e2c 100644 --- a/nova/virt/libvirt/guest.py +++ b/nova/virt/libvirt/guest.py @@ -406,14 +406,22 @@ class Guest(object): except libvirt.libvirtError as ex: with excutils.save_and_reraise_exception(): errcode = ex.get_error_code() + # TODO(lyarwood): Remove libvirt.VIR_ERR_OPERATION_FAILED + # and libvirt.VIR_ERR_INTERNAL_ERROR once + # MIN_LIBVIRT_VERSION is >= 4.1.0 if errcode in (libvirt.VIR_ERR_OPERATION_FAILED, - libvirt.VIR_ERR_INTERNAL_ERROR): + libvirt.VIR_ERR_INTERNAL_ERROR, + libvirt.VIR_ERR_DEVICE_MISSING): + # TODO(lyarwood): Remove the following error message + # check once we only care about VIR_ERR_DEVICE_MISSING errmsg = ex.get_error_message() if 'not found' in errmsg: # This will be raised if the live domain # detach fails because the device is not found raise exception.DeviceNotFound( device=alternative_device_name) + # TODO(lyarwood): Remove libvirt.VIR_ERR_INVALID_ARG once + # MIN_LIBVIRT_VERSION is >= 4.1.0 elif errcode == libvirt.VIR_ERR_INVALID_ARG: errmsg = ex.get_error_message() if 'no target device' in errmsg: