From 3402732f04298d526d25a78190f711e1c4151d9a Mon Sep 17 00:00:00 2001 From: Matt Low Date: Thu, 15 Sep 2022 14:25:58 -0600 Subject: [PATCH] Improved domain definition handling From libvirt docs (https://libvirt.org/html/libvirt-libvirt-domain.html#virDomainDefineXML): -- A previous definition for this domain with the same UUID and name would be overridden if it already exists. Before this commit, if a definition was given without a UUID, the appropriate domain would be created with a libvirt-generated UUID. If the definition was modified (with UUID still left out), the module would quietly exit with `changes: False` and make no changes (masking a libvirt error that a domain with the same name already existed). If a UUID was given, the module would work (mostly) as expected and update the domain's definition. However, if the did not match the of an existing domain with the same name, the module would quietly exit without making changes again. I say (mostly) as expected, because it was still possible to create non-idempotent XML definitions. For example, if network interface MAC addresses were left out of the XML definition, libvirt would generate entirely new ones; probably not what most users would expect. To summarize the changes of this commit: - Incoming XML is checked for a UUID - A domain with the same name checked for - If a domain with the same name already exists and the incoming UUID is missing, apply the existing UUID to the incoming XML. - If a UUID is defined in the incoming XML and it doesn't match that of an existing domain with the same name, exit with an error - For any incoming XML missing a element, we will attempt to fetch MAC of the matching interface from the existing domain's XML - Diff info is supplied, showing the changes between previously and newly defined domain XML. Note: it is still possible to apply non-idempotent libvirt definitions with this module. However, the support for diffing makes it easier to find those and fix the problems your definitions (or request support in this module to handle those cases where feasible). --- plugins/modules/virt.py | 144 ++++++++++++++++++++++++++++++++++------ 1 file changed, 125 insertions(+), 19 deletions(-) diff --git a/plugins/modules/virt.py b/plugins/modules/virt.py index d1b3596..e2dd35d 100644 --- a/plugins/modules/virt.py +++ b/plugins/modules/virt.py @@ -475,7 +475,43 @@ def define(self, xml): self.__get_conn() return self.conn.define_from_xml(xml) -def handle_define(module: AnsibleModule, v: Virt): + +def _lookup_interface_mac(match_interface, interfaces): + def _source_network(interface): + ''' return the libvirt network name for libvirt interfaces ''' + return interface.find('source').get('network') + + def _source_bridge(interface): + ''' return the bridge device for bridged interfaces ''' + return interface.find('source').get('bridge') + + def _source_dev(interface): + ''' return the source device for macvtap interfaces ''' + return interface.find('source').get('dev') + + def _get_mac(interface): + return interface.find('mac').get('address') + + matched = None + + _type = match_interface.get('type') + for interface in interfaces: + if _type != interface.get('type') or interface.find("mac") is None: + continue + + if _type == 'network' and _source_network(interface) == _source_network(match_interface): + return _get_mac(interface) + if _type == 'bridge' and _source_bridge(interface) == _source_bridge(match_interface): + return _get_mac(interface) + if _type == 'direct' and _source_dev(interface) == _source_dev(match_interface): + return _get_mac(interface) + # TODO: support additional types + + return None + + +def handle_define(module, v): + ''' handle `command: define` ''' xml = module.params.get('xml', None) guest = module.params.get('name', None) autostart = module.params.get('autostart', None) @@ -508,36 +544,106 @@ def handle_define(module: AnsibleModule, v: Virt): res = dict() # From libvirt docs (https://libvirt.org/html/libvirt-libvirt-domain.html#virDomainDefineXML): - # -- A previous definition for this domain would be overridden if it already exists. + # -- A previous definition for this domain with the same UUID and name would + # be overridden if it already exists. + # + # If a domain is defined without a , libvirt will generate one for it. + # If an attempt is made to re-define the same xml (with the same and + # no ), libvirt will complain with the following error: # - # In real world testing with libvirt versions 1.2.17-13, 2.0.0-10 and 3.9.0-14 - # on qemu and lxc domains results in: # operation failed: domain '' already exists with # - # In case a domain would be indeed overwritten, we should protect idempotency: + # If a domain with a similiar but different is defined, + # libvirt complains with the same error. However, if a domain is defined + # with the same and as an existing domain, then libvirt will + # update the domain with the new definition (automatically handling + # addition/removal of devices. some changes may require a boot). + # + # Users will often want to define their domains without an explicit + # UUID, instead giving them a unique name. + # + # In similar fashion, libvirt will automatically generate MAC addresses for + # interfaces if they are defined without one. If a domain is re-defined with + # XML that is missing MAC addresses, libvirt will generate entirely new MAC + # addresses, which is probably not what most users want. + # + # Therefore, if and/or interface MAC addresess are missing from + # incoming xml, we will attempt to bring them over from the existing domain. + existing_xml = None + existing_xml_raw = None try: - existing_domain_xml = v.get_vm(domain_name).XMLDesc( - libvirt.VIR_DOMAIN_XML_INACTIVE - ) + existing_domain = v.get_vm(domain_name) except VMNotFound: - existing_domain_xml = None + existing_domain = None + + if existing_domain is not None: + # we are updating a domain's definition + incoming_uuid = incoming_xml.findtext('./uuid') + existing_uuid = existing_domain.UUIDString() + + if incoming_uuid is not None: + if incoming_uuid != existing_uuid: + # A user should not try defining a domain with the same name but + # different UUID + module.fail_json(msg="attempting to re-define domain %s/%s with a different UUID: %s" % ( + domain_name, existing_uuid, incoming_uuid + )) + else: + # UUID was missing in incoming xml, so add it + etree.SubElement(incoming_xml, 'uuid').text = existing_uuid + + existing_xml_raw = existing_domain.XMLDesc(libvirt.VIR_DOMAIN_XML_INACTIVE) + existing_xml = etree.fromstring(existing_xml_raw) + + # handle copying over MAC addresses from existing definition + existing_interfaces = existing_xml.findall('./devices/interface') + for interface in incoming_xml.findall('./devices/interface'): + mac = interface.find('mac') + if mac is None: + # incoming interface is missing MAC address, so we will add it + # from the existing domain if it has a matching interface + existing_mac = _lookup_interface_mac(interface, existing_interfaces) + if existing_mac: + etree.SubElement(interface, 'mac', { + 'address': existing_mac, + }) + try: - domain = v.define(xml) - if existing_domain_xml: - # if we are here, then libvirt redefined existing domain as the doc promised - new_domain_xml = domain.XMLDesc(libvirt.VIR_DOMAIN_XML_INACTIVE) - if existing_domain_xml != new_domain_xml: - res = {'changed': True, 'change_reason': 'config changed'} + domain_xml = etree.tostring(incoming_xml).decode() + + # TODO: support check mode + domain = v.define(domain_xml) + + if existing_xml is not None: + # In this case, we may have updated the definition or it might be the same. + # We compare the domain's previous xml with its new state and diff + # the changes. This allows users to fix their xml if it results in + # non-idempotent behaviour (e.g. libvirt mutates it each time) + new_xml = domain.XMLDesc(libvirt.VIR_DOMAIN_XML_INACTIVE) + if existing_xml_raw != new_xml: + res.update({ + 'changed': True, + 'change_reason': 'domain definition changed', + 'diff': { + 'before': existing_xml_raw, + 'after': new_xml + } + }) else: - res = {'changed': True, 'created': domain.name()} + # there was no existing XML, so this is a newly created domain + res.update({'changed': True, 'created': domain.name()}) + except libvirtError as e: - if e.get_error_code() != 9: # 9 means 'domain already exists' error - module.fail_json(msg='libvirtError: %s' % e.get_error_message()) + module.fail_json(msg='libvirtError: %s' % e.get_error_message()) + except Exception as e: + module.fail_json(msg='an unknown error occured: %s' % e) + if autostart is not None and v.autostart(domain_name, autostart): - res = {'changed': True, 'change_reason': 'autostart'} + res.update({'changed': True, 'change_reason': 'autostart'}) return res + def core(module): state = module.params.get('state', None)