openstack
/
nova
Code Issues Proposed changes

libvirt: Flatten 'get_domain' function 

Recent changes to the code mean the function 'get_domain' is the sole
caller of '_get_domain_by_name'. This structure is unnecessarily bloated
and prevents us from raising valid InstanceNotFound exceptions
containing Instance.uuid values. Fold the latter functions into the
former, shedding this excess weight and partially resolving the bug.

Change-Id: I43d608a629b3dba204264f1db97fb35b205aee85
Closes-Bug: #1522454
This commit is contained in:
Stephen Finucane 2016-12-13 13:47:38 +00:00
parent 3167f74960
commit 0f26569649
2 changed files with 42 additions and 57 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

42
nova/tests/unit/virt/libvirt/test_host.py
View File

@ -19,7 +19,9 @@ from eventlet import greenthread
import mock
from oslo_utils import uuidutils
import six
import testtools
from nova.compute import vm_states
from nova import exception
from nova import objects
from nova.objects import fields as obj_fields
@ -414,55 +416,43 @@ class HostTestCase(test.NoDBTestCase):
self.assertTrue(self.host.has_version(None, hv_ver, hv_type))
@mock.patch.object(fakelibvirt.virConnect, "lookupByName")
def test_get_domain_by_name(self, fake_lookup):
def test_get_domain(self, fake_lookup):
dom = fakelibvirt.virDomain(self.host.get_connection(),
"<domain id='7'/>")
instance = objects.Instance(id="124")
fake_lookup.return_value = dom
self.assertEqual(dom, self.host._get_domain_by_name("wibble"))
fake_lookup.assert_called_once_with("wibble")
self.assertEqual(dom, self.host.get_domain(instance))
fake_lookup.assert_called_once_with("instance-0000007c")
@mock.patch.object(fakelibvirt.virConnect, "lookupByName")
def test_get_domain_by_name_raises(self, fake_lookup):
def test_get_domain_raises(self, fake_lookup):
instance = objects.Instance(uuid=uuids.instance,
vm_state=vm_states.ACTIVE)
fake_lookup.side_effect = fakelibvirt.make_libvirtError(
fakelibvirt.libvirtError,
'Domain not found: no domain with matching name',
error_code=fakelibvirt.VIR_ERR_NO_DOMAIN,
error_domain=fakelibvirt.VIR_FROM_QEMU)
self.assertRaises(exception.InstanceNotFound,
self.host._get_domain_by_name,
"wibble")
with testtools.ExpectedException(exception.InstanceNotFound):
self.host.get_domain(instance)
fake_lookup.assert_called_once_with("wibble")
fake_lookup.assert_called_once_with(uuids.instance)
@mock.patch.object(host.Host, "_get_domain_by_name")
def test_get_domain(self, fake_get_domain):
@mock.patch.object(fakelibvirt.virConnect, "lookupByName")
def test_get_guest(self, fake_lookup):
dom = fakelibvirt.virDomain(self.host.get_connection(),
"<domain id='7'/>")
fake_get_domain.return_value = dom
instance = objects.Instance(id="124")
self.assertEqual(dom, self.host.get_domain(instance))
fake_get_domain.assert_called_once_with("instance-0000007c")
@mock.patch.object(host.Host, "_get_domain_by_name")
def test_get_guest(self, fake_get_domain):
dom = fakelibvirt.virDomain(self.host.get_connection(),
"<domain id='7'/>")
fake_get_domain.return_value = dom
fake_lookup.return_value = dom
instance = objects.Instance(id="124")
guest = self.host.get_guest(instance)
self.assertEqual(dom, guest._domain)
self.assertIsInstance(guest, libvirt_guest.Guest)
fake_get_domain.assert_called_once_with("instance-0000007c")
fake_lookup.assert_called_once_with("instance-0000007c")
@mock.patch.object(fakelibvirt.Connection, "listAllDomains")
def test_list_instance_domains(self, mock_list_all):

57
nova/virt/libvirt/host.py
View File

@ -517,51 +517,46 @@ class Host(object):
return self._version_check(
lv_ver=lv_ver, hv_ver=hv_ver, hv_type=hv_type, op=operator.ne)
# TODO(sahid): needs to be private
def get_domain(self, instance):
"""Retrieve libvirt domain object for an instance.
:param instance: an nova.objects.Instance object
Attempt to lookup the libvirt domain objects
corresponding to the Nova instance, based on
its name. If not found it will raise an
exception.InstanceNotFound exception. On other
errors, it will raise an exception.NovaException
exception.
:returns: a libvirt.Domain object
"""
return self._get_domain_by_name(instance.name)
def get_guest(self, instance):
"""Retrieve libvirt domain object for an instance.
:param instance: an nova.objects.Instance object
:returns: a nova.virt.libvirt.Guest object
"""
return libvirt_guest.Guest(
self.get_domain(instance))
def _get_domain_by_name(self, instance_name):
"""Retrieve libvirt domain object given an instance name.
"""Retrieve libvirt guest object for an instance.
All libvirt error handling should be handled in this method and
relevant nova exceptions should be raised in response.
:param instance: a nova.objects.Instance object
:returns: a nova.virt.libvirt.Guest object
:raises exception.InstanceNotFound: The domain was not found
:raises exception.NovaException: A libvirt error occured
"""
return libvirt_guest.Guest(self.get_domain(instance))
# TODO(sahid): needs to be private
def get_domain(self, instance):
"""Retrieve libvirt domain object for an instance.
All libvirt error handling should be handled in this method and
relevant nova exceptions should be raised in response.
:param instance: a nova.objects.Instance object
:returns: a libvirt.Domain object
:raises exception.InstanceNotFound: The domain was not found
:raises exception.NovaException: A libvirt error occured
"""
try:
conn = self.get_connection()
return conn.lookupByName(instance_name)
return conn.lookupByName(instance.name)
except libvirt.libvirtError as ex:
error_code = ex.get_error_code()
if error_code == libvirt.VIR_ERR_NO_DOMAIN:
raise exception.InstanceNotFound(instance_id=instance_name)
raise exception.InstanceNotFound(instance_id=instance.uuid)
# TODO(stephenfin): Stop using NovaException here - it's too
# generic. InternalError would be a better fit.
msg = (_('Error from libvirt while looking up %(instance_name)s: '
'[Error Code %(error_code)s] %(ex)s') %
{'instance_name': instance_name,
{'instance_name': instance.name,
'error_code': error_code,
'ex': ex})
raise exception.NovaException(msg)