From a7d8302a5e19653fe17c7cf74c635e6f47d0a601 Mon Sep 17 00:00:00 2001 From: Ilya Etingof Date: Tue, 30 Oct 2018 18:16:56 +0100 Subject: [PATCH] Fix libvirt driver to handle domains by UUID The libvirt driver does not support domain identifiers in form of UUID despite the abstract API requirement. This change fixes that and effectively unifies libvirt driver API with the nova one. Story: 2004306 Task: 27868 Change-Id: I3098146e8f7d79234870b68f090678857c666475 --- sushy_tools/emulator/drivers/libvirtdriver.py | 317 +++++++++--------- .../tests/unit/emulator/test_libvirt.py | 40 ++- 2 files changed, 206 insertions(+), 151 deletions(-) diff --git a/sushy_tools/emulator/drivers/libvirtdriver.py b/sushy_tools/emulator/drivers/libvirtdriver.py index f0b84bd7..880fbf2c 100644 --- a/sushy_tools/emulator/drivers/libvirtdriver.py +++ b/sushy_tools/emulator/drivers/libvirtdriver.py @@ -13,6 +13,8 @@ # License for the specific language governing permissions and limitations # under the License. +import logging +import uuid import xml.etree.ElementTree as ET from collections import namedtuple @@ -28,6 +30,10 @@ except ImportError: is_loaded = bool(libvirt) + +logger = logging.getLogger(__name__) + + BiosProcessResult = namedtuple('BiosProcessResult', ['tree', 'attributes_written', @@ -99,6 +105,27 @@ class LibvirtDriver(AbstractDriver): def __init__(self, uri=None): self._uri = uri or self.LIBVIRT_URI + def _get_domain(self, identity, readonly=False): + with libvirt_open(self._uri, readonly=readonly) as conn: + try: + return conn.lookupByName(identity) + + except libvirt.libvirtError as ex: + try: + uu_identity = uuid.UUID(identity) + + return conn.lookupByUUID(uu_identity.bytes) + + except (ValueError, libvirt.libvirtError): + msg = ('Error finding domain by name/UUID "%(identity)s" ' + 'at libvirt URI %(uri)s": %(err)s' % + {'identity': identity, + 'uri': self._uri, 'err': ex}) + + logger.debug(msg) + + raise FishyError(msg) + @property def driver(self): """Return human-friendly driver information @@ -122,13 +149,12 @@ class LibvirtDriver(AbstractDriver): The universal unique identifier (UUID) for this system. Can be used in place of system name if there are duplicates. - :param identity: libvirt domain name or ID + :param identity: libvirt domain name or UUID :returns: computer system UUID """ - with libvirt_open(self._uri, readonly=True) as conn: - domain = conn.lookupByName(identity) - return domain.UUIDString() + domain = self._get_domain(identity, readonly=True) + return domain.UUIDString() def get_power_state(self, identity): """Get computer system power state @@ -138,10 +164,8 @@ class LibvirtDriver(AbstractDriver): :returns: current power state as *On* or *Off* `str` or `None` if power state can't be determined """ - with libvirt_open(self._uri, readonly=True) as conn: - - domain = conn.lookupByName(identity) - return 'On' if domain.isActive() else 'Off' + domain = self._get_domain(identity, readonly=True) + return 'On' if domain.isActive() else 'Off' def set_power_state(self, identity, state): """Set computer system power state @@ -154,35 +178,33 @@ class LibvirtDriver(AbstractDriver): :raises: `FishyError` if power state can't be set """ - with libvirt_open(self._uri) as conn: + domain = self._get_domain(identity) - domain = conn.lookupByName(identity) + try: + if state in ('On', 'ForceOn'): + if not domain.isActive(): + domain.create() + elif state == 'ForceOff': + if domain.isActive(): + domain.destroy() + elif state == 'GracefulShutdown': + if domain.isActive(): + domain.shutdown() + elif state == 'GracefulRestart': + if domain.isActive(): + domain.reboot() + elif state == 'ForceRestart': + if domain.isActive(): + domain.reset() + elif state == 'Nmi': + if domain.isActive(): + domain.injectNMI() - try: - if state in ('On', 'ForceOn'): - if not domain.isActive(): - domain.create() - elif state == 'ForceOff': - if domain.isActive(): - domain.destroy() - elif state == 'GracefulShutdown': - if domain.isActive(): - domain.shutdown() - elif state == 'GracefulRestart': - if domain.isActive(): - domain.reboot() - elif state == 'ForceRestart': - if domain.isActive(): - domain.reset() - elif state == 'Nmi': - if domain.isActive(): - domain.injectNMI() + except libvirt.libvirtError as e: + msg = ('Error changing power state at libvirt URI "%(uri)s": ' + '%(error)s' % {'uri': self._uri, 'error': e}) - except libvirt.libvirtError as e: - msg = ('Error changing power state at libvirt URI "%(uri)s": ' - '%(error)s' % {'uri': self._uri, 'error': e}) - - raise FishyError(msg) + raise FishyError(msg) def get_boot_device(self, identity): """Get computer system boot device name @@ -192,19 +214,17 @@ class LibvirtDriver(AbstractDriver): :returns: boot device name as `str` or `None` if device name can't be determined """ - with libvirt_open(self._uri, readonly=True) as conn: + domain = self._get_domain(identity, readonly=True) - domain = conn.lookupByName(identity) + tree = ET.fromstring(domain.XMLDesc()) - tree = ET.fromstring(domain.XMLDesc()) + boot_element = tree.find('.//boot') + if boot_element is not None: + boot_source_target = ( + self.BOOT_DEVICE_MAP_REV.get(boot_element.get('dev')) + ) - boot_element = tree.find('.//boot') - if boot_element is not None: - boot_source_target = ( - self.BOOT_DEVICE_MAP_REV.get(boot_element.get('dev')) - ) - - return boot_source_target + return boot_source_target def set_boot_device(self, identity, boot_source): """Get/Set computer system boot device name @@ -216,39 +236,38 @@ class LibvirtDriver(AbstractDriver): :raises: `FishyError` if boot device can't be set """ - with libvirt_open(self._uri) as conn: + domain = self._get_domain(identity) - domain = conn.lookupByName(identity) + # XML schema: https://libvirt.org/formatdomain.html#elementsOSBIOS + tree = ET.fromstring(domain.XMLDesc()) - # XML schema: https://libvirt.org/formatdomain.html#elementsOSBIOS - tree = ET.fromstring(domain.XMLDesc()) + try: + target = self.BOOT_DEVICE_MAP[boot_source] - try: - target = self.BOOT_DEVICE_MAP[boot_source] + except KeyError: + msg = ('Unknown power state requested: ' + '%(boot_source)s' % {'boot_source': boot_source}) - except KeyError: - msg = ('Unknown power state requested: ' - '%(boot_source)s' % {'boot_source': boot_source}) + raise FishyError(msg) - raise FishyError(msg) + for os_element in tree.findall('os'): + # Remove all "boot" elements + for boot_element in os_element.findall('boot'): + os_element.remove(boot_element) - for os_element in tree.findall('os'): - # Remove all "boot" elements - for boot_element in os_element.findall('boot'): - os_element.remove(boot_element) + # Add a new boot element with the request boot device + boot_element = ET.SubElement(os_element, 'boot') + boot_element.set('dev', target) - # Add a new boot element with the request boot device - boot_element = ET.SubElement(os_element, 'boot') - boot_element.set('dev', target) - - try: + try: + with libvirt_open(self._uri) as conn: conn.defineXML(ET.tostring(tree).decode('utf-8')) - except libvirt.libvirtError as e: - msg = ('Error changing boot device at libvirt URI "%(uri)s": ' - '%(error)s' % {'uri': self._uri, 'error': e}) + except libvirt.libvirtError as e: + msg = ('Error changing boot device at libvirt URI "%(uri)s": ' + '%(error)s' % {'uri': self._uri, 'error': e}) - raise FishyError(msg) + raise FishyError(msg) def get_boot_mode(self, identity): """Get computer system boot mode. @@ -256,21 +275,19 @@ class LibvirtDriver(AbstractDriver): :returns: either *Uefi* or *Legacy* as `str` or `None` if current boot mode can't be determined """ - with libvirt_open(self._uri, readonly=True) as conn: + domain = self._get_domain(identity, readonly=True) - domain = conn.lookupByName(identity) + # XML schema: https://libvirt.org/formatdomain.html#elementsOSBIOS + tree = ET.fromstring(domain.XMLDesc()) - # XML schema: https://libvirt.org/formatdomain.html#elementsOSBIOS - tree = ET.fromstring(domain.XMLDesc()) + loader_element = tree.find('.//loader') - loader_element = tree.find('.//loader') + if loader_element is not None: + boot_mode = ( + self.BOOT_MODE_MAP_REV.get(loader_element.get('type')) + ) - if loader_element is not None: - boot_mode = ( - self.BOOT_MODE_MAP_REV.get(loader_element.get('type')) - ) - - return boot_mode + return boot_mode def set_boot_mode(self, identity, boot_mode): """Set computer system boot mode. @@ -281,54 +298,55 @@ class LibvirtDriver(AbstractDriver): :raises: `FishyError` if boot mode can't be set """ - with libvirt_open(self._uri) as conn: + domain = self._get_domain(identity, readonly=True) - domain = conn.lookupByName(identity) + # XML schema: https://libvirt.org/formatdomain.html#elementsOSBIOS + tree = ET.fromstring(domain.XMLDesc()) - # XML schema: https://libvirt.org/formatdomain.html#elementsOSBIOS - tree = ET.fromstring(domain.XMLDesc()) + try: + loader_type = self.BOOT_MODE_MAP[boot_mode] + + except KeyError: + msg = ('Unknown boot mode requested: ' + '%(boot_mode)s' % {'boot_mode': boot_mode}) + + raise FishyError(msg) + + for os_element in tree.findall('os'): + type_element = os_element.find('type') + if type_element is None: + os_arch = None + else: + os_arch = type_element.get('arch') try: - loader_type = self.BOOT_MODE_MAP[boot_mode] + loader_path = self.BOOT_LOADER_MAP[boot_mode][os_arch] except KeyError: - msg = ('Unknown boot mode requested: ' - '%(boot_mode)s' % {'boot_mode': boot_mode}) + # NOTE(etingof): assume no specific boot loader + loader_path = '' - raise FishyError(msg) + # Update all "loader" elements + for loader_element in os_element.findall('loader'): + loader_element.set('type', loader_type) + # NOTE(etingof): here we override previous boot loader for + # for the domain. If it's different than what we have + # hardcoded in the BOOT_LOADER_MAP, we won't be able to + # revert back to the original boor loader should we change + # domain boot mode. + loader_element.text = loader_path - for os_element in tree.findall('os'): - type_element = os_element.find('type') - if type_element is None: - os_arch = None - else: - os_arch = type_element.get('arch') + with libvirt_open(self._uri) as conn: try: - loader_path = self.BOOT_LOADER_MAP[boot_mode][os_arch] + conn.defineXML(ET.tostring(tree).decode('utf-8')) - except KeyError: - # NOTE(etingof): assume no specific boot loader - loader_path = '' + except libvirt.libvirtError as e: + msg = ('Error changing boot mode at libvirt URI ' + '"%(uri)s": %(error)s' % {'uri': self._uri, + 'error': e}) - # Update all "loader" elements - for loader_element in os_element.findall('loader'): - loader_element.set('type', loader_type) - # NOTE(etingof): here we override previous boot loader for - # for the domain. If it's different than what we have - # hardcoded in the BOOT_LOADER_MAP, we won't be able to - # revert back to the original boor loader should we change - # domain boot mode. - loader_element.text = loader_path - - try: - conn.defineXML(ET.tostring(tree).decode('utf-8')) - - except libvirt.libvirtError as e: - msg = ('Error changing boot mode at libvirt URI "%(uri)s": ' - '%(error)s' % {'uri': self._uri, 'error': e}) - - raise FishyError(msg) + raise FishyError(msg) def get_total_memory(self, identity): """Get computer system total memory @@ -338,9 +356,8 @@ class LibvirtDriver(AbstractDriver): :returns: available RAM in GiB as `int` or `None` if total memory count can't be determined """ - with libvirt_open(self._uri, readonly=True) as conn: - domain = conn.lookupByName(identity) - return int(domain.maxMemory() / 1024 / 1024) + domain = self._get_domain(identity, readonly=True) + return int(domain.maxMemory() / 1024 / 1024) def get_total_cpus(self, identity): """Get computer system total count of available CPUs @@ -350,25 +367,23 @@ class LibvirtDriver(AbstractDriver): :returns: available CPU count as `int` or `None` if CPU count can't be determined """ - with libvirt_open(self._uri, readonly=True) as conn: + domain = self._get_domain(identity, readonly=True) - domain = conn.lookupByName(identity) + tree = ET.fromstring(domain.XMLDesc()) - tree = ET.fromstring(domain.XMLDesc()) + total_cpus = 0 - total_cpus = 0 + if domain.isActive(): + total_cpus = domain.maxVcpus() - if domain.isActive(): - total_cpus = domain.maxVcpus() + # If we can't get it from maxVcpus() try to find it by + # inspecting the domain XML + if total_cpus <= 0: + vcpu_element = tree.find('.//vcpu') + if vcpu_element is not None: + total_cpus = int(vcpu_element.text) - # If we can't get it from maxVcpus() try to find it by - # inspecting the domain XML - if total_cpus <= 0: - vcpu_element = tree.find('.//vcpu') - if vcpu_element is not None: - total_cpus = int(vcpu_element.text) - - return total_cpus or None + return total_cpus or None def _process_bios_attributes(self, domain_xml, @@ -451,21 +466,24 @@ class LibvirtDriver(AbstractDriver): :raises: `FishyError` if BIOS attributes cannot be saved """ - with libvirt_open(self._uri) as conn: - libvirt_domain = conn.lookupByName(identity) - result = self._process_bios_attributes(libvirt_domain.XMLDesc(), - bios_attributes, - update_existing_attributes) + domain = self._get_domain(identity) - if result.attributes_written: - try: + result = self._process_bios_attributes(domain.XMLDesc(), + bios_attributes, + update_existing_attributes) + + if result.attributes_written: + try: + with libvirt_open(self._uri) as conn: conn.defineXML(ET.tostring(result.tree).decode('utf-8')) - except libvirt.libvirtError as e: - msg = ('Error updating BIOS attributes' - ' at libvirt URI "%(uri)s": ' - '%(error)s' % {'uri': self._uri, 'error': e}) - raise FishyError(msg) - return result.bios_attributes + + except libvirt.libvirtError as e: + msg = ('Error updating BIOS attributes' + ' at libvirt URI "%(uri)s": ' + '%(error)s' % {'uri': self._uri, 'error': e}) + raise FishyError(msg) + + return result.bios_attributes def get_bios(self, identity): """Get BIOS section @@ -516,9 +534,8 @@ class LibvirtDriver(AbstractDriver): :returns: list of network interfaces dict with their attributes """ - with libvirt_open(self._uri, readonly=True) as conn: - domain = conn.lookupByName(identity) - tree = ET.fromstring(domain.XMLDesc()) - return [{'id': iface.get('address'), 'mac': iface.get('address')} - for iface in tree.findall( - ".//devices/interface[@type='network']/mac")] + domain = self._get_domain(identity, readonly=True) + tree = ET.fromstring(domain.XMLDesc()) + return [{'id': iface.get('address'), 'mac': iface.get('address')} + for iface in tree.findall( + ".//devices/interface[@type='network']/mac")] diff --git a/sushy_tools/tests/unit/emulator/test_libvirt.py b/sushy_tools/tests/unit/emulator/test_libvirt.py index 43ec7f5d..8c3f4578 100644 --- a/sushy_tools/tests/unit/emulator/test_libvirt.py +++ b/sushy_tools/tests/unit/emulator/test_libvirt.py @@ -11,6 +11,8 @@ # under the License. import libvirt +import uuid + from oslotest import base from six.moves import mock @@ -24,6 +26,40 @@ class LibvirtDriverTestCase(base.BaseTestCase): self.test_driver = LibvirtDriver() super(LibvirtDriverTestCase, self).setUp() + @mock.patch('libvirt.open', autospec=True) + def test__get_domain_by_name(self, libvirt_mock): + domain_id = 'name' + domain_info = 'domain' + + conn_mock = libvirt_mock.return_value + lookupByName_mock = conn_mock.lookupByName + lookupByName_mock.return_value = domain_info + lookupByUUID_mock = conn_mock.lookupByUUID + + domain = self.test_driver._get_domain(domain_id) + + self.assertEqual(domain_info, domain) + lookupByName_mock.assert_called_once_with(domain_id) + self.assertFalse(lookupByUUID_mock.called) + + @mock.patch('libvirt.open', autospec=True) + def test__get_domain_by_uuid(self, libvirt_mock): + domain_id = uuid.UUID('b9fbc4f5-2c81-4c80-97ea-272621fb7360') + domain_info = 'domain' + + conn_mock = libvirt_mock.return_value + lookupByName_mock = conn_mock.lookupByName + lookupByName_mock.side_effect = libvirt.libvirtError( + 'domain not found') + lookupByUUID_mock = conn_mock.lookupByUUID + lookupByUUID_mock.return_value = domain_info + + domain = self.test_driver._get_domain(str(domain_id)) + + self.assertEqual(domain_info, domain) + lookupByName_mock.assert_called_once_with(str(domain_id)) + lookupByUUID_mock.assert_called_once_with(domain_id.bytes) + @mock.patch('libvirt.openReadOnly', autospec=True) def test_uuid(self, libvirt_mock): conn_mock = libvirt_mock.return_value @@ -169,7 +205,8 @@ class LibvirtDriverTestCase(base.BaseTestCase): self.assertEqual('Legacy', boot_mode) @mock.patch('libvirt.open', autospec=True) - def test_set_boot_mode(self, libvirt_mock): + @mock.patch('libvirt.openReadOnly', autospec=True) + def test_set_boot_mode(self, libvirt_mock, libvirt_rw_mock): with open('sushy_tools/tests/unit/emulator/domain.xml', 'r') as f: data = f.read() @@ -179,6 +216,7 @@ class LibvirtDriverTestCase(base.BaseTestCase): self.test_driver.set_boot_mode('zzzz-yyyy-xxxx', 'Uefi') + conn_mock = libvirt_rw_mock.return_value conn_mock.defineXML.assert_called_once_with(mock.ANY) @mock.patch('libvirt.openReadOnly', autospec=True)