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 <uuid> did not match the
<uuid> 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 <uuid> of an existing domain when the `ADD_UUID` mutate
    flag is supplied and the incoming XML is missing a <uuid>.
  - Attempt to reuse the MAC address of an existing interface having the
    same <alias>, 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).

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/).

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 <mac>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']
+    """

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  <source> elements
+# user networks don't have a <source> 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 <uuid>, libvirt will generate one for it.
+    # If an attempt is made to re-define the same xml (with the same <name> and
+    # no <uuid>), 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 '<name>' already exists with <uuid>
     #
-    # In case a domain would be indeed overwritten, we should protect idempotency:
+    # If a domain with a similiar <name> but different <uuid> is defined,
+    # libvirt complains with the same error. However, if a domain is defined
+    # with the same <name> and <uuid> 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']),
         ),
     )