Implement ansible module for management of libvirt volumes #180
Conversation
First conflict free merge.
Trying to define a volume, that already exists caused the integration test to fail with a traceback that implies, a code block has been executed, which should have been skipped in the case of the target volume already existing. The previous code based on raising and catching an exception in case the volume does not already exist, but resulting in the exception bein risen anyway during the test, which lead to the failure. This commit replaces the exception based check in hope to fix this bug.
…nged Trying to define a volume, that already exists caused the integration test to fail with a traceback that implies, a code block has been executed, which should have been skipped in the case of the target volume already existing. The previous code based on raising and catching an exception in case the volume does not already exist, but resulting in the exception bein risen anyway during the test, which lead to the failure. This commit replaces the exception based check in hope to fix this bug.
|
"no connection driver available for qemu:///system" is a new error, and I'm not able to tell, why it starts showing up exactly now, and not earlier. |
|
I'm unfortunately completely unfamiliar with the technology and have no idea about why the tests fail |
|
@NK308 i took a quick look, i see you test on very old distros, how about just testing only on ubuntu 24.04 if it makes sense? |
|
@Andersson007 The issue seems to be very specific to the RHEL remote targets, even the current version, while older versions of other operating systems don't seem to cause any problems. |
@NK308 my comment is not related to the failures, i just think it's an extra thing and there's no such a requirement to test against them. I'm not familiar with the technology though and I don't know if it's distro dependent or not. |
|
I now have removed the OS specific variables from the integration tests, if that's what you meant. The remaining operating systems represented in |
|
@NK308 how about removing RH 7 and updating Ubuntu 16 to 24.04 ? |
|
Those versions are not end of life yet, and servers can still receive extended support, as stated here https://en.wikipedia.org/wiki/Ubuntu_version_history and here https://en.wikipedia.org/wiki/Red_Hat_Enterprise_Linux. |
|
Hi @NK308 thanks very much for reviving this, I'll take a look and put in any comments in line. |
There was a problem hiding this comment.
@NK308 thanks again for this, I've done an initial review with comments in line, if you have time to take a look that would be great. I think one of the things we should consider is perhaps combining the volume and pool tests as volume is dependent on a pool, or reworking them to have some common parts to call. Also, I think it'd be great if the format of the volume tests were similar in style to the pool tests, e.g. with "check" mode, etc. I did test check mode but it didn't seem to behave as expected, so that might need some attention (but that's an inline comment). Feel free to let me know how you go and if you need a hand. Thanks!
| rc = VIRT_SUCCESS | ||
| try: | ||
| rc, result = core(module) | ||
| except Exception as e: |
There was a problem hiding this comment.
Maybe we can add a custom exception for this, like with EntryNotFound(Exception)?
There was a problem hiding this comment.
The module es already using EntryNotFoundand at the end of the main method any exception should be caught. That's why Exception is caught there.
| v = VirtVolume(uri, module, pool) | ||
| res = {} | ||
|
|
||
| if state and command == 'list_volumes': |
There was a problem hiding this comment.
is state actually used here? state is being passed to list_volumes but AFAICT it's not used by find_entry or listAllStoragePools()...
| aliases: [ "volume" ] | ||
| description: | ||
| - Name of the volume being managed. Note that the volume must be previously | ||
| defined with xml. |
There was a problem hiding this comment.
I think maybe we should add a note here that name is required if state is defined?
| description: | ||
| - Specify which state you want a volume to be in. | ||
| If 'present', ensure that the volume is present but do not change its | ||
| state; if it's missing, you need to specify xml argument. |
There was a problem hiding this comment.
suggest, if it's missing, you need to define it by specifying the xml argument.
| @@ -0,0 +1,85 @@ | |||
| --- | |||
There was a problem hiding this comment.
I wonder whether it might be better to include virt_volume tests as a part of the virt_pool test, as the pools are a dependency and it would save time and duplication?...
| xml=dict(), | ||
| mode=dict(choices=ALL_MODES), | ||
| ), | ||
| supports_check_mode=True |
There was a problem hiding this comment.
in my testing, it doesn't seem like check mode is working as expected, defining a volume in a pool with check_mode: true still creates the volume
volume doesn't exist:
$ sudo ls -l /var/lib/libvirt/images/testing-volume
ls: cannot access '/var/lib/libvirt/images/testing-volume': No such file or directory
$ sudo virsh vol-info testing-volume default
error: failed to get vol 'testing-volume'
error: Storage volume not found: no storage vol with matching path 'testing-volume'
test with ansible playbook volume_test.yml:
---
- hosts: localhost
gather_facts: false
become: true
tasks:
- name: "Define volume on check_mode(pre)"
community.libvirt.virt_volume:
state: present
name: testing-volume
pool: default
xml: |
<volume>
<name>testing-volume</name>
<allocation>0</allocation>
<capacity unit="M">10</capacity>
<target>
<permissions>
<mode>0644</mode>
<label>virt_image_t</label>
</permissions>
</target>
</volume>
check_mode: true
run ansible, changed as expected:
$ ansible-playbook -i localhost, -K volume_test.yml
BECOME password:
PLAY [localhost] *****************************************************************************************************
TASK [Define volume on check_mode(pre)] ******************************************************************************
changed: [localhost]
PLAY RECAP ***********************************************************************************************************
localhost : ok=1 changed=1 unreachable=0 failed=0 skipped=0 rescued=0 ignored=0
check volume, was incorrectly created:
$ sudo ls -l /var/lib/libvirt/images/testing-volume
-rw-r--r--. 1 root root 10485760 Nov 24 15:56 /var/lib/libvirt/images/testing-volume
$ sudo virsh vol-info testing-volume default
Name: testing-volume
Type: file
Capacity: 10.00 MiB
Allocation: 0.00 B
|
The original branch from @flagbot on which my branch is based, had shared code between the |
@NK308 can we incorporate any of the suggestions by @csmart or you think it's sufficient for merge as is? I can't provide any feedback on the refactoring but it'm for keeping unrelated things separate (i.e. in separate PRs). |
I think, the best thing would be, to incorporate some of the smaller changes, merge this PR, then do some refactoring in a second PR and then handling the stuff about the diffs and checks in a third PR. But thinking about the diffs... I can think about some features, that would have some overlapp with that topic, like a command which would be to able to edit/update things like domains or networks. I mean ... both features would require code to do a more in-depth analysis of the xml definitions, that I can find here right now. This might hold for the freatures of #183 (I didn't really take a look into it jet). So before we do the refactoring, we might want to have a more in-depth discussion about how we want the outline of the code in module_utils to look like, and how it should look like, to provide a good foundation for new features in the future. |
| - Manage I(libvirt) volumes inside a storage pool. | ||
| options: | ||
| name: | ||
| aliases: [ "volume" ] |
There was a problem hiding this comment.
| aliases: [ "volume" ] |
Only name is ok here, there's a recommendation in the docs to avoid aliases whenever possible
There was a problem hiding this comment.
I think, the aliases are a good idea here, because in playbooks with lots of tasks from community.libvirt it improves the readability, to not have every task contain a name which references to a totally different kind of structure.
|
|
||
| module = AnsibleModule( | ||
| argument_spec=dict( | ||
| name=dict(aliases=['volume']), |
There was a problem hiding this comment.
| name=dict(aliases=['volume']), | |
| name=dict(), |
| - Name of the volume being managed. Note that the volume must be previously | ||
| defined with xml. |
There was a problem hiding this comment.
| - Name of the volume being managed. Note that the volume must be previously | |
| defined with xml. | |
| - Name of the volume being managed. Note that the volume must be previously | |
| defined with xml. |
Is it not required? Just to be sure
There was a problem hiding this comment.
It is required, with the exception of the list_volumes, info and facts command.
| - Leonardo Galli (@galli-leo) | ||
| short_description: Manage libvirt volumes inside a storage pool | ||
| description: | ||
| - Manage I(libvirt) volumes inside a storage pool. |
There was a problem hiding this comment.
| - Manage I(libvirt) volumes inside a storage pool. | |
| - Manage I(libvirt) volumes inside a storage pool. | |
| attributes: | |
| check_mode: | |
| description: Supports check_mode. | |
| support: full |
|
I would highly suggest updating ubuntu 16 to at least 22 or better 24 and RedHat to the latest. Those old ones are really old and receive only security updates |
Co-authored-by: Andrew Klychkov <[email protected]>
Co-authored-by: Andrew Klychkov <[email protected]>
Different MR for unrelated features is good, but I'm also wary of merging something for which the coverage of the tests isn't quite complete and then it never gets fixed up later. Duplication is fine, but I think the |
SUMMARY
This PR adds a module to create, delete and modify libvirt volumes.
This PR is based on the fork which was subject of #45
ISSUE TYPE
COMPONENT NAME
virt_volumeADDITIONAL INFORMATION
Some of the code of the new ansible module as well as parts of the code of
virt_pool, have been refactored tomodule_utilsto reduce redundancy.For now I have only tested the simple creation und deletion of storage volumes, so there might some more testing be needed.
For now I didn't do much more, compared to #45, than bringing the branch up to date with the main branch, including the adoption of changes in the code and doc style from the other modules.