From c4fe15801de0a1b61456acc8d2a5a704b0fe6ea5 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 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 - Add a `mutate_flags` parameter to `virt`: - Copy the of an existing domain when the `ADD_UUID` mutate flag is supplied and the incoming XML is missing a . - Attempt to reuse the MAC address of an existing interface having the same , when the `ADD_MAC_ADDRESSES` mutate flag is supplied. - Attempt to reuse the MAC address of an existing interface of similar type and source when the `ADD_MAC_ADDRESSES_FUZZY` mutate flag is supplied. - 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). --- .../142_virt_define_improvements.yml | 3 + plugins/doc_fragments/virt.py | 21 +++ plugins/modules/virt.py | 175 ++++++++++++++++-- 3 files changed, 180 insertions(+), 19 deletions(-) create mode 100644 changelogs/fragments/142_virt_define_improvements.yml diff --git a/changelogs/fragments/142_virt_define_improvements.yml b/changelogs/fragments/142_virt_define_improvements.yml new file mode 100644 index 0000000..d2269ff --- /dev/null +++ b/changelogs/fragments/142_virt_define_improvements.yml @@ -0,0 +1,3 @@ +minor_changes: + - virt - support ``--diff`` for ``define`` command (https://github.com/ansible-collections/community.libvirt/pull/142/). + - virt - add `mutate_flags` parameter to enable XML mutation (add UUID, MAC addresses from existing domain) (https://github.com/ansible-collections/community.libvirt/pull/142/). diff --git a/plugins/doc_fragments/virt.py b/plugins/doc_fragments/virt.py index a607299..6e4ec35 100644 --- a/plugins/doc_fragments/virt.py +++ b/plugins/doc_fragments/virt.py @@ -65,3 +65,24 @@ class ModuleDocFragment(object): - Must be raw XML content using C(lookup). XML cannot be reference to a file. type: str """ + + OPTIONS_MUTATE_FLAGS = r""" +options: + mutate_flags: + description: + - For each mutate_flag, we will modify the given XML in some way + - ADD_UUID will add an existing domain's UUID to the xml if it's missing + - ADD_MAC_ADDRESSES will look up interfaces in the existing domain with a + matching alias and copy the MAC address over. The matching interface + doesn't need to be of the same type or source network. + - ADD_MAC_ADDRESSES_FUZZY will try to match incoming interfaces with + interfaces of the existing domain sharing the same type and source + network/device. It may not always result in the expected outcome, + particularly if a domain has multiple interface attached to the same + host device and only some of those devices have s. + Use caution, do some testing for your particular use-case! + choices: [ ADD_UUID, ADD_MAC_ADDRESSES, ADD_MAC_ADDRESSES_FUZZY ] + type: list + elements: str + default: ['ADD_UUID'] + """ diff --git a/plugins/modules/virt.py b/plugins/modules/virt.py index d1b3596..529e6cd 100644 --- a/plugins/modules/virt.py +++ b/plugins/modules/virt.py @@ -46,6 +46,7 @@ - community.libvirt.virt.options_autostart - community.libvirt.virt.options_state - community.libvirt.virt.options_command + - community.libvirt.virt.options_mutate_flags - community.libvirt.requirements author: - Ansible Core Team @@ -194,6 +195,8 @@ 'checkpoints_metadata': 16, } +MUTATE_FLAGS = ['ADD_UUID', 'ADD_MAC_ADDRESSES', 'ADD_MAC_ADDRESSES_FUZZY'] + ALL_FLAGS = [] ALL_FLAGS.extend(ENTRY_UNDEFINE_FLAGS_MAP.keys()) @@ -475,10 +478,27 @@ def define(self, xml): self.__get_conn() return self.conn.define_from_xml(xml) -def handle_define(module: AnsibleModule, v: Virt): + +# A dict of interface types (found in their `type` attribute) to the +# corresponding "source" attribute name of their elements +# user networks don't have a element +# +# We do not support fuzzy matching against any interface types +# not defined here +INTERFACE_SOURCE_ATTRS = { + 'network': 'network', + 'bridge': 'bridge', + 'direct': 'dev', + 'user': 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) + mutate_flags = module.params.get('mutate_flags', []) if not xml: module.fail_json(msg="define requires 'xml' argument") @@ -508,36 +528,152 @@ 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). try: - existing_domain_xml = v.get_vm(domain_name).XMLDesc( - libvirt.VIR_DOMAIN_XML_INACTIVE - ) + existing_domain = v.get_vm(domain_name) + existing_xml_raw = existing_domain.XMLDesc(libvirt.VIR_DOMAIN_XML_INACTIVE) + existing_xml = etree.fromstring(existing_xml_raw) except VMNotFound: - existing_domain_xml = None + existing_domain = None + existing_xml_raw = None + existing_xml = 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 and 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: + if 'ADD_UUID' in mutate_flags and incoming_uuid is None: + # Users will often want to define their domains without an explicit + # UUID, instead giving them a unique name - so we support bringing + # over the UUID from the existing domain + etree.SubElement(incoming_xml, 'uuid').text = existing_uuid + + existing_devices = existing_xml.find('./devices') + + if 'ADD_MAC_ADDRESSES' in mutate_flags: + for interface in incoming_xml.xpath('./devices/interface[not(mac) and alias]'): + search_alias = interface.find('alias').get('name') + xpath = "./interface[alias[@name='%s']]" % search_alias + try: + matched_interface = existing_devices.xpath(xpath)[0] + existing_devices.remove(matched_interface) + etree.SubElement(interface, 'mac', { + 'address': matched_interface.find('mac').get('address') + }) + except IndexError: + module.warn("Could not match interface %i of incoming XML by alias %s." % ( + interface.getparent().index(interface) + 1, search_alias + )) + + if 'ADD_MAC_ADDRESSES_FUZZY' in mutate_flags: + # the counts of interfaces of a similar type/source + # key'd with tuple of (type, source) + similar_interface_counts = {} + + def get_interface_count(_type, source=None): + key = (_type, source if _type != "user" else None) + if key not in similar_interface_counts: + similar_interface_counts[key] = 1 + else: + similar_interface_counts[key] += 1 + return similar_interface_counts[key] + + # iterate user-defined interfaces + for interface in incoming_xml.xpath('./devices/interface'): + _type = interface.get('type') + + if interface.find('mac') is not None and interface.find('alias') is not None: + continue + + if _type not in INTERFACE_SOURCE_ATTRS: + module.warn("Skipping fuzzy MAC matching for interface %i of incoming XML: unsupported interface type '%s'." % ( + interface.getparent().index(interface) + 1, _type + )) + continue + + source_attr = INTERFACE_SOURCE_ATTRS[_type] + source = interface.find('source').get(source_attr) if source_attr else None + similar_count = get_interface_count(_type, source) + + if interface.find('mac') is not None: + # we want to count these, but not try to change their MAC address + continue + + if source: + xpath = "./interface[@type='%s' and source[@%s='%s']]" % ( + _type, source_attr, source) + else: + xpath = "./interface[@type = '%s']" % source_attr + + matching_interfaces = existing_devices.xpath(xpath) + try: + matched_interface = matching_interfaces[similar_count - 1] + etree.SubElement(interface, 'mac', { + 'address': matched_interface.find('./mac').get('address'), + }) + except IndexError: + module.warn("Could not fuzzy match interface %i of incoming XML." % ( + interface.getparent().index(interface) + 1 + )) + 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_domain 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) @@ -657,6 +793,7 @@ def main(): force=dict(type='bool'), uri=dict(type='str', default='qemu:///system'), xml=dict(type='str'), + mutate_flags=dict(type='list', elements='str', choices=MUTATE_FLAGS, default=['ADD_UUID']), ), )