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)