Fix virt module to undefine a domain with nvram or other metadata #136

bturmann · 2022-08-12T16:26:40Z

SUMMARY

Fixes #40

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

ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME

virt

ADDITIONAL INFORMATION

My tests confirm that the bugfix works as expected, like undefine a qemu guest with UEFI nvram or a qemu guest with BIOS. Also other commands are still working, like start, shutdown or destroy.

ansible adhoc command:

$ ansible -b -m community.libvirt.virt -a "name=guest99 command=undefine force=true" host99

csmart · 2022-08-13T09:48:48Z

This looks good, thanks @bturmann, but I need to go over it a bit carefully as I think this will change the behaviour quite a bit that people may have been relying on - for example, users may rely on the fact that nvram isn't cleaned up, or that VMs with snapshots aren't removed.

I think it would be good to call this out in the changelog, not just as a bugfix, but potential behaviour change?

Andersson007 · 2022-08-15T08:25:22Z

if it's a behavioral change, it must be announced now (with major_changes:) but released in the next major release (the PR must be merged right before it happens).

bturmann · 2022-08-15T08:45:53Z

Agreed. What do you think about following?

introduce a new parameter "force", similar to what other modules do
only If force would be set to "True" a guest would be undefined even though having nvram or other metadata
if force is not defined (should be default) or set to false, the behaviour is as of today being fully backward compatible
explain parameter force in module doc

Andersson007 · 2022-08-15T08:51:05Z

Agreed. What do you think about following?

introduce a new parameter "force", similar to what other modules do

only If force would be set to "True" a guest would be undefined even though having nvram or other metadata

if force is not defined (should be default) or set to false, the behaviour is as of today being fully backward compatible

explain parameter force in module doc

Sounds like a solution to me, @csmart what do you think?

csmart · 2022-08-15T09:08:50Z

That sounds excellent! Thanks.

plugins/modules/virt.py

bturmann · 2022-08-16T12:45:41Z

I just updated the first comment above to reflect the latest overall status.

The PR got a bit larger, but I think for good reasons. With the two new options, it is now possible to undefine successfully a guest including nvram or other metadata. Users have a choice whether to simply use force=true or provide the desired flags individually.

Special cases, like using mutually exclusive flags or if force and flags are provided together are handled as well with either an exception or warning.

For existing use cases, where none of the new options are provided, then the behaviour is exactly the same as before.

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

csmart · 2022-08-21T23:14:09Z

@bturmann this looks really great, thanks a lot for the work here! I will do some testing myself and if you're happy, then I'll merge it in.

csmart · 2022-08-27T05:46:31Z

Tested well and looks great to me. Thanks @bturmann!

Andersson007 · 2022-08-29T07:11:34Z

@bturmann @csmart thanks folks for your great work!

* main: Improved domain definition handling Parse incoming XML instead of using regex to get name Refactor define command handling into separate function CI: add 2.14 tests, ignore-2.15 to avoid CI failing doc: set booleans to true/false for consistency Combine REVIEW_CHECKLIST.md and CONTRIBUTING.md and fix links CI: remove f35 from devel fix up failing pylint test, use before assignment Fix virt module to undefine a domain with nvram or other metadata (ansible-collections#136) Replace discouraged function listVolumes (ansible-collections#135) Replace functions listStoragePools and listDefinedStoragePools (ansible-collections#134) Update galaxy to next expected release (ansible-collections#133) Release 1.2.0 commit (ansible-collections#132)

# By Chris Smart (6) and others # Via Chris Smart * main: Improved domain definition handling Parse incoming XML instead of using regex to get name Refactor define command handling into separate function CI: add 2.14 tests, ignore-2.15 to avoid CI failing doc: set booleans to true/false for consistency Combine REVIEW_CHECKLIST.md and CONTRIBUTING.md and fix links CI: remove f35 from devel fix up failing pylint test, use before assignment Fix virt module to undefine a domain with nvram or other metadata (ansible-collections#136) Replace discouraged function listVolumes (ansible-collections#135) Replace functions listStoragePools and listDefinedStoragePools (ansible-collections#134) Update galaxy to next expected release (ansible-collections#133) Release 1.2.0 commit (ansible-collections#132)

* modules_virt_additions: Improved domain definition handling Parse incoming XML instead of using regex to get name Refactor define command handling into separate function CI: add 2.14 tests, ignore-2.15 to avoid CI failing doc: set booleans to true/false for consistency Combine REVIEW_CHECKLIST.md and CONTRIBUTING.md and fix links CI: remove f35 from devel fix up failing pylint test, use before assignment Fix virt module to undefine a domain with nvram or other metadata (ansible-collections#136) Replace discouraged function listVolumes (ansible-collections#135) Replace functions listStoragePools and listDefinedStoragePools (ansible-collections#134) Update galaxy to next expected release (ansible-collections#133) Release 1.2.0 commit (ansible-collections#132)

tomkivlin · 2023-09-05T08:56:06Z

Was this fix released?

I can't see the updated docs on https://docs.ansible.com/ansible/latest/collections/community/libvirt/virt_module.html and when I try with flags: nvram I get an error of

Unsupported parameters for (community.libvirt.virt) module: flags. Supported parameters include: autostart, command, name, state, uri, xml (guest).

I'm using:

community.libvirt             1.2.0

Happy to create an issue if needed but thought I'd check here first.

bturmann · 2023-09-05T09:57:52Z

Was this fix released?

Indeed, the latest release tag seems to be 1.2.0. I left a comment in the Release Plan issue.

csmart · 2023-09-05T11:16:36Z

Thanks for the bump, I will arrange a new release soon.

tomkivlin · 2023-09-05T11:19:29Z

@csmart thanks! Happy to help (if help is needed).

bturmann · 2023-09-26T07:40:54Z

@tomkivlin
This change is now included in the new release 1.3.0 available on Galaxy. The docs are still showing 1.2.0 and I am not sure when they get updated.

tomkivlin · 2023-09-26T09:18:15Z

Many thanks 🙏

bturmann marked this pull request as draft August 12, 2022 16:43

bturmann force-pushed the fix_virt_undefine_nvram branch from 59c7f07 to f93851c Compare August 12, 2022 17:07

bturmann marked this pull request as ready for review August 12, 2022 17:13

bturmann force-pushed the fix_virt_undefine_nvram branch from f93851c to b10380c Compare August 15, 2022 06:53

bturmann marked this pull request as draft August 15, 2022 08:30

csmart reviewed Aug 16, 2022

View reviewed changes

plugins/modules/virt.py Outdated Show resolved Hide resolved

bturmann force-pushed the fix_virt_undefine_nvram branch from 5495963 to d08b2fb Compare August 16, 2022 12:18

bturmann marked this pull request as ready for review August 16, 2022 12:46

bturmann force-pushed the fix_virt_undefine_nvram branch from d08b2fb to 9c04800 Compare August 16, 2022 13:06

bturmann force-pushed the fix_virt_undefine_nvram branch from 9c04800 to 6abcda1 Compare August 16, 2022 13:09

csmart merged commit 54313e5 into ansible-collections:main Aug 27, 2022

csmart mentioned this pull request Aug 27, 2022

CI: remove f35 from devel #137

Merged

bturmann deleted the fix_virt_undefine_nvram branch August 29, 2022 09:11

bturmann mentioned this pull request Sep 26, 2023

I can’t do “undefine” for a virtual machine with uefi nvram #159

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix virt module to undefine a domain with nvram or other metadata #136

Fix virt module to undefine a domain with nvram or other metadata #136

bturmann commented Aug 12, 2022 •

edited

Loading

csmart commented Aug 13, 2022 •

edited

Loading

Andersson007 commented Aug 15, 2022

bturmann commented Aug 15, 2022

Andersson007 commented Aug 15, 2022

csmart commented Aug 15, 2022

bturmann commented Aug 16, 2022

csmart commented Aug 21, 2022

csmart commented Aug 27, 2022

Andersson007 commented Aug 29, 2022

tomkivlin commented Sep 5, 2023

bturmann commented Sep 5, 2023

csmart commented Sep 5, 2023

tomkivlin commented Sep 5, 2023

bturmann commented Sep 26, 2023

tomkivlin commented Sep 26, 2023

Fix virt module to undefine a domain with nvram or other metadata #136

Fix virt module to undefine a domain with nvram or other metadata #136

Conversation

bturmann commented Aug 12, 2022 • edited Loading

SUMMARY

Compatibility / Risk:

ISSUE TYPE

COMPONENT NAME

ADDITIONAL INFORMATION

csmart commented Aug 13, 2022 • edited Loading

Andersson007 commented Aug 15, 2022

bturmann commented Aug 15, 2022

Andersson007 commented Aug 15, 2022

csmart commented Aug 15, 2022

bturmann commented Aug 16, 2022

csmart commented Aug 21, 2022

csmart commented Aug 27, 2022

Andersson007 commented Aug 29, 2022

tomkivlin commented Sep 5, 2023

bturmann commented Sep 5, 2023

csmart commented Sep 5, 2023

tomkivlin commented Sep 5, 2023

bturmann commented Sep 26, 2023

tomkivlin commented Sep 26, 2023

bturmann commented Aug 12, 2022 •

edited

Loading

csmart commented Aug 13, 2022 •

edited

Loading