From cc8bd076e343c00f04554798db2e074b4a99ce1b Mon Sep 17 00:00:00 2001 From: Chris Smart Date: Thu, 4 Aug 2022 21:39:02 +1000 Subject: [PATCH 01/13] Release 1.2.0 commit (#132) --- CHANGELOG.rst | 22 ++++++++++++++++++ changelogs/changelog.yaml | 23 +++++++++++++++++++ .../108_make_virt_net_modify_idempotent.yml | 2 -- .../fragments/113_extra_inventory_info.yml | 3 --- changelogs/fragments/virt_pool_no_path.yml | 2 -- galaxy.yml | 2 +- 6 files changed, 46 insertions(+), 8 deletions(-) delete mode 100644 changelogs/fragments/108_make_virt_net_modify_idempotent.yml delete mode 100644 changelogs/fragments/113_extra_inventory_info.yml delete mode 100644 changelogs/fragments/virt_pool_no_path.yml diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 668b708..0452a46 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -5,6 +5,28 @@ Community.Libvirt Release Notes .. contents:: Topics +v1.2.0 +====== + +Release Summary +--------------- + +This is the minor release of the ``community.libvirt`` collection. +This changelog contains all changes to the modules and plugins in this collection +that have been made after the previous release. + +Minor Changes +------------- + +- libvirt - add extra guest information to inventory (https://github.com/ansible-collections/community.libvirt/pull/113). +- libvirt - replace the calls to listDomainsID() and listDefinedDomains() with listAllDomains() in find_vm() (https://github.com/ansible-collections/community.libvirt/pull/117) + +Bugfixes +-------- + +- virt_net - fix modify function which was not idempotent, depending on whether the network was active. See https://github.com/ansible-collections/community.libvirt/issues/107. +- virt_pool - crashed out if pool didn't contain a target path. Fix allows this not to be set. (https://github.com/ansible-collections/community.libvirt/issues/129). + v1.1.0 ====== diff --git a/changelogs/changelog.yaml b/changelogs/changelog.yaml index 4dad01b..62a1b1a 100644 --- a/changelogs/changelog.yaml +++ b/changelogs/changelog.yaml @@ -60,3 +60,26 @@ releases: - 114_replace_distutils_spawn.yml - 90_add_integration_test_for_virt_pool.yml release_date: '2022-05-12' + 1.2.0: + changes: + bugfixes: + - virt_net - fix modify function which was not idempotent, depending on whether + the network was active. See https://github.com/ansible-collections/community.libvirt/issues/107. + - virt_pool - crashed out if pool didn't contain a target path. Fix allows this + not to be set. (https://github.com/ansible-collections/community.libvirt/issues/129). + minor_changes: + - libvirt - add extra guest information to inventory (https://github.com/ansible-collections/community.libvirt/pull/113). + - libvirt - replace the calls to listDomainsID() and listDefinedDomains() with + listAllDomains() in find_vm() (https://github.com/ansible-collections/community.libvirt/pull/117) + release_summary: 'This is the minor release of the ``community.libvirt`` collection. + + This changelog contains all changes to the modules and plugins in this collection + + that have been made after the previous release.' + fragments: + - 1.2.0.yml + - 108_make_virt_net_modify_idempotent.yml + - 113_extra_inventory_info.yml + - 117_find_vms_update_calls.yml + - virt_pool_no_path.yml + release_date: '2022-08-04' diff --git a/changelogs/fragments/108_make_virt_net_modify_idempotent.yml b/changelogs/fragments/108_make_virt_net_modify_idempotent.yml deleted file mode 100644 index 7dc284c..0000000 --- a/changelogs/fragments/108_make_virt_net_modify_idempotent.yml +++ /dev/null @@ -1,2 +0,0 @@ -bugfixes: - - virt_net - fix modify function which was not idempotent, depending on whether the network was active. See https://github.com/ansible-collections/community.libvirt/issues/107. diff --git a/changelogs/fragments/113_extra_inventory_info.yml b/changelogs/fragments/113_extra_inventory_info.yml deleted file mode 100644 index 5d044c9..0000000 --- a/changelogs/fragments/113_extra_inventory_info.yml +++ /dev/null @@ -1,3 +0,0 @@ ---- -minor_changes: - - libvirt - add extra guest information to inventory (https://github.com/ansible-collections/community.libvirt/pull/113). \ No newline at end of file diff --git a/changelogs/fragments/virt_pool_no_path.yml b/changelogs/fragments/virt_pool_no_path.yml deleted file mode 100644 index 382f769..0000000 --- a/changelogs/fragments/virt_pool_no_path.yml +++ /dev/null @@ -1,2 +0,0 @@ -bugfixes: - - virt_pool - crashed out if pool didn't contain a target path. Fix allows this not to be set. (https://github.com/ansible-collections/community.libvirt/issues/129). diff --git a/galaxy.yml b/galaxy.yml index 24e1d6d..79e5feb 100644 --- a/galaxy.yml +++ b/galaxy.yml @@ -1,6 +1,6 @@ namespace: community name: libvirt -version: 1.1.1 +version: 1.2.0 readme: README.md authors: - Ansible (https://github.com/ansible) From fc5406bb27e8ae1a98658dc657aadc5278b98bb5 Mon Sep 17 00:00:00 2001 From: Chris Smart Date: Thu, 4 Aug 2022 21:42:48 +1000 Subject: [PATCH 02/13] Update galaxy to next expected release (#133) --- galaxy.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/galaxy.yml b/galaxy.yml index 79e5feb..598db27 100644 --- a/galaxy.yml +++ b/galaxy.yml @@ -1,6 +1,6 @@ namespace: community name: libvirt -version: 1.2.0 +version: 1.2.1 readme: README.md authors: - Ansible (https://github.com/ansible) From d91eca9a9bed728293e613fd94dd41deee21c1c4 Mon Sep 17 00:00:00 2001 From: Bernhard Turmann <44261994+bturmann@users.noreply.github.com> Date: Thu, 11 Aug 2022 10:06:55 +0200 Subject: [PATCH 03/13] Replace functions listStoragePools and listDefinedStoragePools (#134) According to the libvirt storage API reference, the functions - listStoragePools (virConnectListStoragePools) - listDefinedStoragePools (virConnectListDefinedStoragePools) should not be used anymore: "The use of this function is discouraged. Instead, use virConnectListAllStoragePools()" ... "This API solves the race inherent between virConnectListStoragePools and virConnectListDefinedStoragePools." Source: https://libvirt.org/html/libvirt-libvirt-storage.html#virConnectListAllStoragePools Compatibility / Risk: The function "virConnectListAllStoragePools()" appeared in version 0.10.2 which was released in 2012. It seems rather unlikely that somebody is still using an older unsupported libvirt version, so the risk should be rather low. Source: https://libvirt.org/hvsupport.html#virStorageDriver The function can be used with or without a flag: - get ALL pools with listAllStoragePools() - get active pools with listAllStoragePools(2) - get inactive pools with listAllStoragePools(1) Before, loops were required to build a list of active and inactive pools. Now, the function returns a list of all pools in just one call. Co-authored-by: Andrew Klychkov Co-authored-by: Andrew Klychkov --- ...virt_pool_replace_functions_listStoragePools.yml | 2 ++ plugins/modules/virt_pool.py | 13 ++----------- 2 files changed, 4 insertions(+), 11 deletions(-) create mode 100644 changelogs/fragments/134_virt_pool_replace_functions_listStoragePools.yml diff --git a/changelogs/fragments/134_virt_pool_replace_functions_listStoragePools.yml b/changelogs/fragments/134_virt_pool_replace_functions_listStoragePools.yml new file mode 100644 index 0000000..272032e --- /dev/null +++ b/changelogs/fragments/134_virt_pool_replace_functions_listStoragePools.yml @@ -0,0 +1,2 @@ +bugfixes: + - virt_pool - replace discouraged functions ``listStoragePools`` and ``listDefinedStoragePools`` with ``listAllStoragePools`` to fix potential race conditions (https://github.com/ansible-collections/community.libvirt/pull/134). diff --git a/plugins/modules/virt_pool.py b/plugins/modules/virt_pool.py index 70145c6..aee89a3 100644 --- a/plugins/modules/virt_pool.py +++ b/plugins/modules/virt_pool.py @@ -222,17 +222,8 @@ def __init__(self, uri, module): def find_entry(self, entryid): # entryid = -1 returns a list of everything - results = [] - - # Get active entries - for name in self.conn.listStoragePools(): - entry = self.conn.storagePoolLookupByName(name) - results.append(entry) - - # Get inactive entries - for name in self.conn.listDefinedStoragePools(): - entry = self.conn.storagePoolLookupByName(name) - results.append(entry) + # Get all entries + results = self.conn.listAllStoragePools() if entryid == -1: return results From 787b4f278fc210a87b935604c79a71649915b193 Mon Sep 17 00:00:00 2001 From: Bernhard Turmann <44261994+bturmann@users.noreply.github.com> Date: Sat, 13 Aug 2022 11:51:36 +0200 Subject: [PATCH 04/13] Replace discouraged function listVolumes (#135) According to the libvirt storage API reference, the function - listVolumes (virStoragePoolListVolumes) should not be used anymore: "The use of this function is discouraged. Instead, use virStoragePoolListAllVolumes()." Source: https://libvirt.org/html/libvirt-libvirt-storage.html#virStoragePoolListVolumes Compatibility / Risk: The function "virStoragePoolListAllVolumes()" appeared in version 0.10.2 which was released in 2012. It seems rather unlikely that somebody is still using an older unsupported libvirt version, so the risk should be rather low. Source: https://libvirt.org/hvsupport.html#virStorageDriver --- .../fragments/135_virt_pool_replace_function_listVolumes.yml | 2 ++ plugins/modules/virt_pool.py | 4 ++-- 2 files changed, 4 insertions(+), 2 deletions(-) create mode 100644 changelogs/fragments/135_virt_pool_replace_function_listVolumes.yml diff --git a/changelogs/fragments/135_virt_pool_replace_function_listVolumes.yml b/changelogs/fragments/135_virt_pool_replace_function_listVolumes.yml new file mode 100644 index 0000000..f79e66e --- /dev/null +++ b/changelogs/fragments/135_virt_pool_replace_function_listVolumes.yml @@ -0,0 +1,2 @@ +bugfixes: + - virt_pool - replace discouraged function ``listAllVolumes`` with ``listAllVolumes`` to fix potential race conditions (https://github.com/ansible-collections/community.libvirt/pull/135). diff --git a/plugins/modules/virt_pool.py b/plugins/modules/virt_pool.py index aee89a3..98c73cf 100644 --- a/plugins/modules/virt_pool.py +++ b/plugins/modules/virt_pool.py @@ -287,7 +287,7 @@ def get_volume_count(self, entryid): return self.find_entry(entryid).numOfVolumes() def get_volume_names(self, entryid): - return self.find_entry(entryid).listVolumes() + return self.find_entry(entryid).listAllVolumes() def get_devices(self, entryid): xml = etree.fromstring(self.find_entry(entryid).XMLDesc(0)) @@ -497,7 +497,7 @@ def facts(self, facts_mode='facts'): results[entry]["volume_count"] = self.conn.get_volume_count(entry) results[entry]["volumes"] = list() for volume in self.conn.get_volume_names(entry): - results[entry]["volumes"].append(volume) + results[entry]["volumes"].append(volume.name()) else: results[entry]["volume_count"] = -1 From 54313e5582927fd9db8a0656446bce9d4d2642c8 Mon Sep 17 00:00:00 2001 From: Bernhard Turmann Date: Sat, 27 Aug 2022 07:46:36 +0200 Subject: [PATCH 05/13] Fix virt module to undefine a domain with nvram or other metadata (#136) Libvirt function undefine() is not able to delete nvram or other metadata. Therefore it is replaced with undefineFlags() which is able to handle it by using flags. All possible flags are listed in 'ENTRY_UNDEFINE_FLAGS_MAP'. Integer 23 makes undefine successful in all cases (1 + 2 + 4 + 16). Source: https://libvirt.org/html/libvirt-libvirt-domain.html#virDomainUndefineFlags The flags nvram(4) and keep_nvram(8) are mutually exclusive. To control the metadata handling during undefine, two new options 'force' and 'flags' are introduced including documentation and examples. Compatibility / Risk: The function undefineFlags() appeared in libvirt version 0.9.4 which was release in 2011. It seems rather unlikely that somebody is still using an older unsupported libvirt version. Additionally, if none of the new module options are provided, then it behaves as before and maintains backward compatibility, means the overall risk of this commit should be rather low. Source: https://libvirt.org/hvsupport.html#virHypervisorDriver --- .../fragments/136_fix_undefine_nvram.yml | 2 + plugins/modules/virt.py | 94 ++++++++++++++++++- 2 files changed, 92 insertions(+), 4 deletions(-) create mode 100644 changelogs/fragments/136_fix_undefine_nvram.yml diff --git a/changelogs/fragments/136_fix_undefine_nvram.yml b/changelogs/fragments/136_fix_undefine_nvram.yml new file mode 100644 index 0000000..8af24df --- /dev/null +++ b/changelogs/fragments/136_fix_undefine_nvram.yml @@ -0,0 +1,2 @@ +bugfixes: + - virt - fix virt module to undefine a domain with nvram, managed_save, snapshot_metadata or checkpoints_metadata (https://github.com/ansible-collections/community.libvirt/issues/40). diff --git a/plugins/modules/virt.py b/plugins/modules/virt.py index bbc2add..ced8705 100644 --- a/plugins/modules/virt.py +++ b/plugins/modules/virt.py @@ -16,6 +16,29 @@ short_description: Manages virtual machines supported by libvirt description: - Manages virtual machines supported by I(libvirt). +options: + flags: + choices: [ 'managed_save', 'snapshots_metadata', 'nvram', 'keep_nvram', 'checkpoints_metadata'] + description: + - Pass additional parameters. + - Currently only implemented with command C(undefine). + Specify which metadata should be removed with C(undefine). + Useful option to be able to C(undefine) guests with UEFI nvram. + C(nvram) and C(keep_nvram) are conflicting and mutually exclusive. + Consider option C(force) if all related metadata should be removed. + type: list + elements: str + force: + description: + - Enforce an action. + - Currently only implemented with command C(undefine). + This option can be used instead of providing all C(flags). + If C(yes), C(undefine) removes also any related nvram or other metadata, if existing. + If C(no) or not set, C(undefine) executes only if there is no nvram or other metadata existing. + Otherwise the task fails and the guest is kept defined without change. + C(yes) and option C(flags) should not be provided together. In this case + C(undefine) ignores C(yes), considers only C(flags) and issues a warning. + type: bool extends_documentation_fragment: - community.libvirt.virt.options_uri - community.libvirt.virt.options_xml @@ -67,6 +90,30 @@ xml: "{{ lookup('template', 'vm_template.xml.j2') }}" autostart: yes +# Undefine VM only, if it has no existing nvram or other metadata +- name: Undefine qemu VM + community.libvirt.virt: + name: foo + +# Undefine VM and force remove all of its related metadata (nvram, snapshots, etc.) +- name: "Undefine qemu VM with force" + community.libvirt.virt: + name: foo + force: yes + +# Undefine VM and remove all of its specified metadata specified +# Result would the same as with force=true +- name: Undefine qemu VM with list of flags + community.libvirt.virt: + name: foo + flags: managed_save, snapshots_metadata, nvram, checkpoints_metadata + +# Undefine VM, but keep its nvram +- name: Undefine qemu VM and keep its nvram + community.libvirt.virt: + name: foo + flags: keep_nvram + # Listing VMs - name: List all VMs community.libvirt.virt: @@ -134,6 +181,17 @@ 6: 'crashed', } +ENTRY_UNDEFINE_FLAGS_MAP = { + 'managed_save': 1, + 'snapshots_metadata': 2, + 'nvram': 4, + 'keep_nvram': 8, + 'checkpoints_metadata': 16, +} + +ALL_FLAGS = [] +ALL_FLAGS.extend(ENTRY_UNDEFINE_FLAGS_MAP.keys()) + class VMNotFound(Exception): pass @@ -198,8 +256,8 @@ def create(self, vmid): def destroy(self, vmid): return self.find_vm(vmid).destroy() - def undefine(self, vmid): - return self.find_vm(vmid).undefine() + def undefine(self, vmid, flag): + return self.find_vm(vmid).undefineFlags(flag) def get_status2(self, vm): state = vm.info()[0] @@ -367,11 +425,11 @@ def destroy(self, vmid): self.__get_conn() return self.conn.destroy(vmid) - def undefine(self, vmid): + def undefine(self, vmid, flag): """ Stop a domain, and then wipe it from the face of the earth. (delete disk/config file) """ self.__get_conn() - return self.conn.undefine(vmid) + return self.conn.undefine(vmid, flag) def status(self, vmid): """ @@ -419,6 +477,8 @@ def core(module): autostart = module.params.get('autostart', None) guest = module.params.get('name', None) command = module.params.get('command', None) + force = module.params.get('force', None) + flags = module.params.get('flags', None) uri = module.params.get('uri', None) xml = module.params.get('xml', None) @@ -513,6 +573,30 @@ def core(module): elif not guest: module.fail_json(msg="%s requires 1 argument: guest" % command) + + elif command == 'undefine': + # Use the undefine function with flag to also handle various metadata. + # This is especially important for UEFI enabled guests with nvram. + # Provide flag as an integer of all desired bits, see 'ENTRY_UNDEFINE_FLAGS_MAP'. + # Integer 23 takes care of all cases (23 = 1 + 2 + 4 + 16). + flag = 0 + if flags is not None: + if force is True: + module.warn("Ignoring 'force', because 'flags' are provided.") + nv = ['nvram', 'keep_nvram'] + # Check mutually exclusive flags + if set(nv) <= set(flags): + raise ValueError("Flags '%s' are mutually exclusive" % "' and '".join(nv)) + for item in flags: + # Get and add flag integer from mapping, otherwise 0. + flag += ENTRY_UNDEFINE_FLAGS_MAP.get(item, 0) + elif force is True: + flag = 23 + # Finally, execute with flag + res = getattr(v, command)(guest, flag) + if not isinstance(res, dict): + res = {command: res} + else: res = getattr(v, command)(guest) if not isinstance(res, dict): @@ -539,6 +623,8 @@ def main(): state=dict(type='str', choices=['destroyed', 'paused', 'running', 'shutdown']), autostart=dict(type='bool'), command=dict(type='str', choices=ALL_COMMANDS), + flags=dict(type='list', elements='str', choices=ALL_FLAGS), + force=dict(type='bool'), uri=dict(type='str', default='qemu:///system'), xml=dict(type='str'), ), From 92a69b890b26c71a8f2a4b4d103618072b6b259a Mon Sep 17 00:00:00 2001 From: Chris Smart Date: Sat, 27 Aug 2022 16:27:34 +1000 Subject: [PATCH 06/13] fix up failing pylint test, use before assignment The latest pylint test is failing with error that variable might be used before it is assigned in a couple of try, except, finally blocks. This patch defaults the variables to an empty string, as the variable would be set to a string in either the try or the except. --- plugins/inventory/libvirt.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/plugins/inventory/libvirt.py b/plugins/inventory/libvirt.py index 903870c..d0cd62f 100644 --- a/plugins/inventory/libvirt.py +++ b/plugins/inventory/libvirt.py @@ -160,6 +160,7 @@ def parse(self, inventory, loader, path, cache=True): ) # This needs the guest powered on, 'qemu-guest-agent' installed and the org.qemu.guest_agent.0 channel configured. + domain_guestInfo = '' try: # type==0 returns all types (users, os, timezone, hostname, filesystem, disks, interfaces) domain_guestInfo = domain.guestInfo(types=0) @@ -173,6 +174,7 @@ def parse(self, inventory, loader, path, cache=True): ) # This needs the guest powered on, 'qemu-guest-agent' installed and the org.qemu.guest_agent.0 channel configured. + domain_interfaceAddresses = '' try: domain_interfaceAddresses = domain.interfaceAddresses(source=libvirt.VIR_DOMAIN_INTERFACE_ADDRESSES_SRC_AGENT) except libvirt.libvirtError as e: From 8406595d1e5969c95d13d8c1f9099c624e3ac82c Mon Sep 17 00:00:00 2001 From: Chris Smart Date: Mon, 22 Aug 2022 09:06:24 +1000 Subject: [PATCH 07/13] CI: remove f35 from devel Fedora 35 is deprecated from devel, this commit removes it from devel and adds it to Docker 2_13 so that it continues to be tested. --- .azure-pipelines/azure-pipelines.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.azure-pipelines/azure-pipelines.yml b/.azure-pipelines/azure-pipelines.yml index f691242..6d6a4bc 100644 --- a/.azure-pipelines/azure-pipelines.yml +++ b/.azure-pipelines/azure-pipelines.yml @@ -126,8 +126,6 @@ stages: targets: - name: CentOS 7 test: centos7 - - name: Fedora 35 - test: fedora35 - name: Fedora 36 test: fedora36 - name: openSUSE 15 py3 @@ -149,6 +147,8 @@ stages: test: centos7 - name: Fedora 34 test: fedora34 + - name: Fedora 35 + test: fedora35 - name: openSUSE 15 py2 test: opensuse15py2 - name: openSUSE 15 py3 From 494339a89858a0071a64708e12be4371e8529d6b Mon Sep 17 00:00:00 2001 From: Maxwell G Date: Tue, 6 Sep 2022 21:01:31 -0500 Subject: [PATCH 08/13] Combine REVIEW_CHECKLIST.md and CONTRIBUTING.md and fix links --- CONTRIBUTING.md | 4 +++- REVIEW_CHECKLIST.md | 3 --- 2 files changed, 3 insertions(+), 4 deletions(-) delete mode 100644 REVIEW_CHECKLIST.md diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index edcfe55..70cd555 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -1,3 +1,5 @@ # Contributing -Refer to the [Ansible Contributing guidelines](https://github.com/ansible/community-docs/blob/main/contributing.rst) to learn how to contribute to this collection. +Refer to the [Ansible Contributing guidelines](https://docs.ansible.com/ansible/devel/community/index.html) to learn how to contribute to this collection. + +Refer to the [review checklist](https://docs.ansible.com/ansible/devel/community/collection_contributors/collection_reviewing.html) when triaging issues or reviewing PRs. diff --git a/REVIEW_CHECKLIST.md b/REVIEW_CHECKLIST.md deleted file mode 100644 index 9dccf7e..0000000 --- a/REVIEW_CHECKLIST.md +++ /dev/null @@ -1,3 +0,0 @@ -# Review Checklist - -Refer to the [Collection review checklist](https://github.com/ansible/community-docs/blob/main/review_checklist.rst). From 925f239113c695270aff72876142c0f9a7d06f44 Mon Sep 17 00:00:00 2001 From: Chris Smart Date: Wed, 7 Sep 2022 07:22:37 +1000 Subject: [PATCH 09/13] doc: set booleans to true/false for consistency The Ansible Steering Committee voted to make all docs refer to booleans as true/false, as there is currently a mix of yes/no/True/False/true/false. See https://github.com/ansible-community/community-topics/issues/116 Fixes #139 --- plugins/modules/virt.py | 14 +++++++------- plugins/modules/virt_net.py | 4 ++-- plugins/modules/virt_pool.py | 4 ++-- 3 files changed, 11 insertions(+), 11 deletions(-) diff --git a/plugins/modules/virt.py b/plugins/modules/virt.py index ced8705..0bcc48e 100644 --- a/plugins/modules/virt.py +++ b/plugins/modules/virt.py @@ -33,11 +33,11 @@ - Enforce an action. - Currently only implemented with command C(undefine). This option can be used instead of providing all C(flags). - If C(yes), C(undefine) removes also any related nvram or other metadata, if existing. - If C(no) or not set, C(undefine) executes only if there is no nvram or other metadata existing. + If C(true), C(undefine) removes also any related nvram or other metadata, if existing. + If C(false) or not set, C(undefine) executes only if there is no nvram or other metadata existing. Otherwise the task fails and the guest is kept defined without change. - C(yes) and option C(flags) should not be provided together. In this case - C(undefine) ignores C(yes), considers only C(flags) and issues a warning. + C(true) and option C(flags) should not be provided together. In this case + C(undefine) ignores C(true), considers only C(flags) and issues a warning. type: bool extends_documentation_fragment: - community.libvirt.virt.options_uri @@ -81,14 +81,14 @@ - name: Set autostart for a VM community.libvirt.virt: name: foo - autostart: yes + autostart: true # Defining a VM and making is autostart with host. VM will be off after this task - name: Define vm from xml and set autostart community.libvirt.virt: command: define xml: "{{ lookup('template', 'vm_template.xml.j2') }}" - autostart: yes + autostart: true # Undefine VM only, if it has no existing nvram or other metadata - name: Undefine qemu VM @@ -99,7 +99,7 @@ - name: "Undefine qemu VM with force" community.libvirt.virt: name: foo - force: yes + force: true # Undefine VM and remove all of its specified metadata specified # Result would the same as with force=true diff --git a/plugins/modules/virt_net.py b/plugins/modules/virt_net.py index 7492cac..de8418a 100644 --- a/plugins/modules/virt_net.py +++ b/plugins/modules/virt_net.py @@ -109,12 +109,12 @@ - name: Ensure that a given network will be started at boot community.libvirt.virt_net: - autostart: yes + autostart: true name: br_nat - name: Disable autostart for a given network community.libvirt.virt_net: - autostart: no + autostart: false name: br_nat - name: Add a new host in the dhcp pool diff --git a/plugins/modules/virt_pool.py b/plugins/modules/virt_pool.py index 98c73cf..3b01173 100644 --- a/plugins/modules/virt_pool.py +++ b/plugins/modules/virt_pool.py @@ -124,12 +124,12 @@ - name: Ensure that a given pool will be started at boot community.libvirt.virt_pool: - autostart: yes + autostart: true name: vms - name: Disable autostart for a given pool community.libvirt.virt_pool: - autostart: no + autostart: false name: vms ''' From 95a544c6eb4d7c497eec597d362aaa74cbae7d65 Mon Sep 17 00:00:00 2001 From: Chris Smart Date: Tue, 27 Sep 2022 18:30:31 +1000 Subject: [PATCH 10/13] CI: add 2.14 tests, ignore-2.15 to avoid CI failing Fixes #143 --- .azure-pipelines/azure-pipelines.yml | 47 ++++++++++++++++++++++++++++ tests/sanity/ignore-2.15.txt | 2 ++ 2 files changed, 49 insertions(+) create mode 100644 tests/sanity/ignore-2.15.txt diff --git a/.azure-pipelines/azure-pipelines.yml b/.azure-pipelines/azure-pipelines.yml index 6d6a4bc..e0b10f3 100644 --- a/.azure-pipelines/azure-pipelines.yml +++ b/.azure-pipelines/azure-pipelines.yml @@ -56,6 +56,18 @@ stages: - name: Units test: 'devel/units/1' + - stage: Ansible_2_14 + displayName: Sanity & Units 2.14 + dependsOn: [] + jobs: + - template: templates/matrix.yml + parameters: + targets: + - name: Sanity + test: '2.14/sanity/1' + - name: Units + test: '2.14/units/1' + - stage: Ansible_2_13 displayName: Sanity & Units 2.13 dependsOn: [] @@ -135,6 +147,23 @@ stages: - name: Ubuntu 22.04 test: ubuntu2204 + - stage: Docker_2_14 + displayName: Docker 2.14 + dependsOn: [] + jobs: + - template: templates/matrix.yml + parameters: + testFormat: 2.14/linux/{0}/1 + targets: + - name: CentOS 7 + test: centos7 + - name: Fedora 36 + test: fedora36 + - name: openSUSE 15 py3 + test: opensuse15 + - name: Ubuntu 20.04 + test: ubuntu2004 + - stage: Docker_2_13 displayName: Docker 2.13 dependsOn: [] @@ -276,6 +305,21 @@ stages: - name: FreeBSD 13.1 test: freebsd/13.1 + - stage: Remote_2_14 + displayName: Remote 2.14 + dependsOn: [] + jobs: + - template: templates/matrix.yml + parameters: + testFormat: '2.14/{0}/1' + targets: + - name: RHEL 9.0 + test: rhel/9.0 + - name: RHEL 8.6 + test: rhel/8.6 + - name: FreeBSD 13.1 + test: freebsd/13.1 + - stage: Remote_2_13 displayName: Remote 2.13 dependsOn: [] @@ -361,18 +405,21 @@ stages: condition: succeededOrFailed() dependsOn: - Ansible_devel + - Ansible_2_14 - Ansible_2_13 - Ansible_2_12 - Ansible_2_11 - Ansible_2_10 - Ansible_2_9 - Docker_devel + - Docker_2_14 - Docker_2_13 - Docker_2_12 - Docker_2_11 - Docker_2_10 - Docker_2_9 - Remote_devel + - Remote_2_14 - Remote_2_13 - Remote_2_12 - Remote_2_11 diff --git a/tests/sanity/ignore-2.15.txt b/tests/sanity/ignore-2.15.txt new file mode 100644 index 0000000..50392e8 --- /dev/null +++ b/tests/sanity/ignore-2.15.txt @@ -0,0 +1,2 @@ +tests/utils/shippable/timing.py shebang +tests/utils/shippable/check_matrix.py replace-urlopen From 8e5f445f12d3001f49a60ed2e7fbd46d3f0d7ed5 Mon Sep 17 00:00:00 2001 From: Matt Low Date: Thu, 15 Sep 2022 11:31:41 -0600 Subject: [PATCH 11/13] Refactor define command handling into separate function --- plugins/modules/virt.py | 86 +++++++++++++++++++++++------------------ 1 file changed, 48 insertions(+), 38 deletions(-) diff --git a/plugins/modules/virt.py b/plugins/modules/virt.py index 0bcc48e..a0af7ca 100644 --- a/plugins/modules/virt.py +++ b/plugins/modules/virt.py @@ -470,6 +470,53 @@ def define(self, xml): self.__get_conn() return self.conn.define_from_xml(xml) +def handle_define(module: AnsibleModule, v: Virt): + xml = module.params.get('xml', None) + guest = module.params.get('name', None) + autostart = module.params.get('autostart', None) + + if not xml: + module.fail_json(msg="define requires xml argument") + if guest: + # there might be a mismatch between guest 'name' in the module and in the xml + module.warn("'xml' is given - ignoring 'name'") + try: + domain_name = re.search('(.*)', xml).groups()[0] + except AttributeError: + module.fail_json(msg="Could not find domain 'name' in xml") + + 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. + # + # 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: + try: + existing_domain_xml = v.get_vm(domain_name).XMLDesc( + libvirt.VIR_DOMAIN_XML_INACTIVE + ) + except VMNotFound: + existing_domain_xml = None + 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'} + else: + res = {'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()) + if autostart is not None and v.autostart(domain_name, autostart): + res = {'changed': True, 'change_reason': 'autostart'} + + return res def core(module): @@ -480,7 +527,6 @@ def core(module): force = module.params.get('force', None) flags = module.params.get('flags', None) uri = module.params.get('uri', None) - xml = module.params.get('xml', None) v = Virt(uri, module) res = dict() @@ -533,43 +579,7 @@ def core(module): if command: if command in VM_COMMANDS: if command == 'define': - if not xml: - module.fail_json(msg="define requires xml argument") - if guest: - # there might be a mismatch between quest 'name' in the module and in the xml - module.warn("'xml' is given - ignoring 'name'") - try: - domain_name = re.search('(.*)', xml).groups()[0] - except AttributeError: - module.fail_json(msg="Could not find domain 'name' in xml") - - # 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. - # - # 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: - try: - existing_domain_xml = v.get_vm(domain_name).XMLDesc( - libvirt.VIR_DOMAIN_XML_INACTIVE - ) - except VMNotFound: - existing_domain_xml = None - try: - domain = v.define(xml) - if existing_domain_xml: - # if we are here, then libvirt redefined existing domain as the doc promised - if existing_domain_xml != domain.XMLDesc(libvirt.VIR_DOMAIN_XML_INACTIVE): - res = {'changed': True, 'change_reason': 'config changed'} - else: - res = {'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()) - if autostart is not None and v.autostart(domain_name, autostart): - res = {'changed': True, 'change_reason': 'autostart'} + res.update(handle_define(module, v)) elif not guest: module.fail_json(msg="%s requires 1 argument: guest" % command) From f44e80558e16eee2ef0cf803908067eb7db0a57d Mon Sep 17 00:00:00 2001 From: Matt Low Date: Thu, 15 Sep 2022 12:23:16 -0600 Subject: [PATCH 12/13] Parse incoming XML instead of using regex to get name The parsed XML will be useful for future features. Also change domain name handling. Allow specifying from either xml or the `name` parameter, but raise an error if they're both present and not equal. --- plugins/modules/virt.py | 45 ++++++++++++++++++++++++++++++++--------- 1 file changed, 36 insertions(+), 9 deletions(-) diff --git a/plugins/modules/virt.py b/plugins/modules/virt.py index a0af7ca..d1b3596 100644 --- a/plugins/modules/virt.py +++ b/plugins/modules/virt.py @@ -155,7 +155,12 @@ else: HAS_VIRT = True -import re +try: + from lxml import etree +except ImportError: + HAS_XML = False +else: + HAS_XML = True from ansible.module_utils.basic import AnsibleModule from ansible.module_utils._text import to_native @@ -476,14 +481,29 @@ def handle_define(module: AnsibleModule, v: Virt): autostart = module.params.get('autostart', None) if not xml: - module.fail_json(msg="define requires xml argument") - if guest: - # there might be a mismatch between guest 'name' in the module and in the xml - module.warn("'xml' is given - ignoring 'name'") + module.fail_json(msg="define requires 'xml' argument") try: - domain_name = re.search('(.*)', xml).groups()[0] - except AttributeError: - module.fail_json(msg="Could not find domain 'name' in xml") + incoming_xml = etree.fromstring(xml) + except etree.XMLSyntaxError: + # TODO: provide info from parser + module.fail_json(msg="given XML is invalid") + + # We'll support supplying the domain's name either from 'name' parameter or xml + # + # But we will fail if both are defined and not equal. + domain_name = incoming_xml.findtext("./name") + if domain_name is not None: + if guest is not None and domain_name != guest: + module.fail_json("given 'name' parameter does not match name in XML") + else: + if guest is None: + module.fail_json("missing 'name' parameter and no name provided in XML") + domain_name = guest + # since there's no in the xml, we'll add it + etree.SubElement(incoming_xml, 'name').text = domain_name + + if domain_name == '': + module.fail_json(msg="domain name cannot be an empty string") res = dict() @@ -641,7 +661,14 @@ def main(): ) if not HAS_VIRT: - module.fail_json(msg='The `libvirt` module is not importable. Check the requirements.') + module.fail_json( + msg='The `libvirt` module is not importable. Check the requirements.' + ) + + if not HAS_XML: + module.fail_json( + msg='The `lxml` module is not importable. Check the requirements.' + ) rc = VIRT_SUCCESS try: From c4fe15801de0a1b61456acc8d2a5a704b0fe6ea5 Mon Sep 17 00:00:00 2001 From: Matt Low Date: Thu, 15 Sep 2022 14:25:58 -0600 Subject: [PATCH 13/13] 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']), ), )