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 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 <interface> missing a <mac> 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

@@ -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 <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).
+    #
+    # 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 <uuid> 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)