From 55e2c851f15d68e7891f867cc5f32eece9a95034 Mon Sep 17 00:00:00 2001 From: Lee Yarwood Date: Tue, 6 Jun 2017 14:08:37 -0400 Subject: [PATCH] libvirt: Check if domain is persistent before detaching devices Previously the libvirt driver would always assume that it was only detaching devices (volumes or virtual interfaces) from a persistent domain however that is not always the case. For example when rolling back from a live migration an attempt is made to detach volumes from the transient destination domain that is being cleaned up. This attempt would fail with the previous assumption of the domain being persistent in place. This change introduces a simple call to has_persistent_configuration within detach_device_with_retry to confirm the state of the domain before attempting to detach. Closes-Bug: #1669857 Closes-Bug: #1696125 Change-Id: I95948721a0119f5f54dbe50d4455fd47d422164b (cherry picked from commit 563c0927d14d052e3f1fad80df95fe4a7c48d38b) --- nova/tests/unit/virt/libvirt/test_driver.py | 3 ++ nova/tests/unit/virt/libvirt/test_guest.py | 40 ++++++++++++++++----- nova/tests/unit/virt/test_virt_drivers.py | 6 +--- nova/virt/libvirt/driver.py | 3 +- nova/virt/libvirt/guest.py | 9 +++-- 5 files changed, 40 insertions(+), 21 deletions(-) diff --git a/nova/tests/unit/virt/libvirt/test_driver.py b/nova/tests/unit/virt/libvirt/test_driver.py index 93672d7265d1..034328229b31 100644 --- a/nova/tests/unit/virt/libvirt/test_driver.py +++ b/nova/tests/unit/virt/libvirt/test_driver.py @@ -461,6 +461,9 @@ class FakeVirtDomain(object): def isActive(self): return True + def isPersistent(self): + return True + class CacheConcurrencyTestCase(test.NoDBTestCase): def setUp(self): diff --git a/nova/tests/unit/virt/libvirt/test_guest.py b/nova/tests/unit/virt/libvirt/test_guest.py index 6d8a65dacca7..d32486a610c5 100644 --- a/nova/tests/unit/virt/libvirt/test_guest.py +++ b/nova/tests/unit/virt/libvirt/test_guest.py @@ -227,6 +227,21 @@ class GuestTestCase(test.NoDBTestCase): "", 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 = "" + get_config = mock.Mock() + get_config.side_effect = [conf, conf, 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( + "", 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 = "" @@ -234,10 +249,10 @@ class GuestTestCase(test.NoDBTestCase): # Force multiple retries of detach get_config.side_effect = [conf, conf, conf, None] dev_path = "/dev/vdb" + self.domain.isPersistent.return_value = True retry_detach = self.guest.detach_device_with_retry( - get_config, dev_path, persistent=True, live=True, - inc_sleep_time=.01) + 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( "", flags=(fakelibvirt.VIR_DOMAIN_AFFECT_CONFIG | @@ -256,10 +271,11 @@ class GuestTestCase(test.NoDBTestCase): conf.to_xml.return_value = "" # 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", persistent=True, live=True, - inc_sleep_time=.01, max_retry_count=3) + 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( "", flags=(fakelibvirt.VIR_DOMAIN_AFFECT_CONFIG | @@ -273,17 +289,19 @@ class GuestTestCase(test.NoDBTestCase): 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", persistent=True, live=True) + get_config, "/dev/vdb", live=True) self.assertIn("/dev/vdb", six.text_type(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, persistent=True, live=True, + get_config, mock.sentinel.device, live=True, alternative_device_name='foo') self.assertIn('foo', six.text_type(ex)) @@ -293,6 +311,8 @@ class GuestTestCase(test.NoDBTestCase): # 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( @@ -302,7 +322,7 @@ class GuestTestCase(test.NoDBTestCase): 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, persistent=True, live=True, + 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) @@ -313,6 +333,8 @@ class GuestTestCase(test.NoDBTestCase): # 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( @@ -323,8 +345,8 @@ class GuestTestCase(test.NoDBTestCase): mock_detach.side_effect = fake_exc self.assertRaises(exception.DeviceNotFound, self.guest.detach_device_with_retry, - get_config, fake_device, persistent=True, live=True, - inc_sleep_time=.01, max_retry_count=3) + get_config, fake_device, live=True, inc_sleep_time=.01, + max_retry_count=3) def test_get_xml_desc(self): self.guest.get_xml_desc() diff --git a/nova/tests/unit/virt/test_virt_drivers.py b/nova/tests/unit/virt/test_virt_drivers.py index b03fd7c0e341..bc23e19b1a4e 100644 --- a/nova/tests/unit/virt/test_virt_drivers.py +++ b/nova/tests/unit/virt/test_virt_drivers.py @@ -136,15 +136,11 @@ class _FakeDriverBackendTestCase(object): pass def fake_detach_device_with_retry(_self, get_device_conf_func, device, - persistent, live, - max_retry_count=7, - inc_sleep_time=2, - max_sleep_time=30): + live, *args, **kwargs): # Still calling detach, but instead of returning function # that actually checks if device is gone from XML, just continue # because XML never gets updated in these tests _self.detach_device(get_device_conf_func(device), - persistent=persistent, live=live) return fake_wait diff --git a/nova/virt/libvirt/driver.py b/nova/virt/libvirt/driver.py index 49fdc9d8e965..066ce317805f 100644 --- a/nova/virt/libvirt/driver.py +++ b/nova/virt/libvirt/driver.py @@ -1359,7 +1359,6 @@ class LibvirtDriver(driver.ComputeDriver): # volume is still in use. wait_for_detach = guest.detach_device_with_retry(guest.get_disk, disk_dev, - persistent=True, live=live) wait_for_detach() @@ -1437,7 +1436,7 @@ class LibvirtDriver(driver.ComputeDriver): # Now we are going to loop until the interface is detached or we # timeout. wait_for_detach = guest.detach_device_with_retry( - guest.get_interface_by_cfg, cfg, persistent=True, live=live, + guest.get_interface_by_cfg, cfg, live=live, alternative_device_name=vif.get('address')) wait_for_detach() except exception.DeviceNotFound: diff --git a/nova/virt/libvirt/guest.py b/nova/virt/libvirt/guest.py index 7a69cfd019de..95a3955b37fa 100644 --- a/nova/virt/libvirt/guest.py +++ b/nova/virt/libvirt/guest.py @@ -361,9 +361,8 @@ class Guest(object): devs.append(dev) return devs - def detach_device_with_retry(self, get_device_conf_func, device, - persistent, live, max_retry_count=7, - inc_sleep_time=2, + def detach_device_with_retry(self, get_device_conf_func, device, live, + max_retry_count=7, inc_sleep_time=2, max_sleep_time=30, alternative_device_name=None): """Detaches a device from the guest. After the initial detach request, @@ -374,8 +373,6 @@ class Guest(object): :param get_device_conf_func: function which takes device as a parameter and returns the configuration for device :param device: device to detach - :param persistent: bool to indicate whether the change is - persistent or not :param live: bool to indicate whether it affects the guest in running state :param max_retry_count: number of times the returned function will @@ -420,6 +417,8 @@ class Guest(object): if conf is None: raise exception.DeviceNotFound(device=alternative_device_name) + persistent = self.has_persistent_configuration() + LOG.debug('Attempting initial detach for device %s', device) _try_detach_device(conf, persistent, live) LOG.debug('Start retrying detach until device %s is gone.', device)