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
This commit is contained in:
Matt Riedemann 2017-06-06 14:08:37 -04:00
parent 0fc64f0ab6
commit 563c0927d1
5 changed files with 40 additions and 21 deletions

View File

@ -524,6 +524,9 @@ class FakeVirtDomain(object):
def isActive(self):
return True
def isPersistent(self):
return True
class CacheConcurrencyTestCase(test.NoDBTestCase):
def setUp(self):

View File

@ -227,6 +227,21 @@ class GuestTestCase(test.NoDBTestCase):
"</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, 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>"
@ -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(
"</xml>", flags=(fakelibvirt.VIR_DOMAIN_AFFECT_CONFIG |
@ -256,10 +271,11 @@ class GuestTestCase(test.NoDBTestCase):
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", 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(
"</xml>", 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 = "</xml>"
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 = "</xml>"
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()

View File

@ -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

View File

@ -1365,7 +1365,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()
@ -1467,7 +1466,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:

View File

@ -371,9 +371,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,
@ -384,8 +383,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
@ -430,6 +427,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)