community.libvirt
community.libvirt copied to clipboard
ansible-collections
Implement ansible module for management of libvirt volumes
SUMMARY
This PR adds a module to create, delete and modify libvirt volumes.
This PR is based on the fork which was subject of https://github.com/ansible-collections/community.libvirt/pull/45
ISSUE TYPE
- New Module Pull Request
COMPONENT NAME
virt_volume
ADDITIONAL INFORMATION
Some of the code of the new ansible module as well as parts of the code of virt_pool, have been refactored to module_utils to 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 https://github.com/ansible-collections/community.libvirt/pull/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.
@NK308 thanks for the PR I would separate the code:
1. You refactor the code in one PR 2. Then you add the module in another PR or wise versaIt's easy to make a mistake and imo not a good idea in general to mix refactoring and new features in a single PR. Hence, i'm suggesting splitting the PR.
cc @csmart
Hi I the reason, why the new module and the refactoring are mixed up here is, that I based my branch on a pre-existing branch, which came with this intermixing of new features and refactoring, and I tried to keep changes to a minimum while resolving the conflicts between that branch and main. I probably would have done some things different, if I wrote the module all by myself.
About the refactoring topic, if I'm gonna split the refactoring off this PR anyway... This collection seems to contain a lot of redundant code since each module is basically a wrapper around a class from the libvirt python bindings. Maybe all of the modules should be refactored together, introducing some inheritance?
The coding style seems to be a bit weird in general. The find_entry() function for example is two completely different functions in one:
@overload def find_entry(self, entryid: str) -> virSomething def find_entry(self, entryid: int = -1) -> list[virSomething]So there is actually no reason, to not split that into two different functions.
Also the naming is a bit irritating since there in most modules, there are at least two classes available with a function named find_entry, which are not related but basically do the same, with different return types. Having the same name would make sense if the classes inherited the method from the same super class, but since they don't, names like find_volume, find_pool, find_domain, etc. would be a lot more self-explanatory.
Also it seems very arbitrary, which methods take the python wrapper of some libvirt object, or it's name (and call find_entry by itself to get the wrapper). I think, it would be better coding style, to have a wrapper class for all the classes from the libvirt python-bindings, so that each of the libvirt objects gets wrapped into its own object instead of writing classes, consisting only of methods, which could as well be standalone functions.
I'm not sure, how much of that might be prevented by the requirement to keep compatibility down to python2. Things like generic classes surely won't work.
@NK308 hello, i wonder if we can remote those remote targets
I'm not sure about that. In some of the previous commits, there was an actual error message, which showed an actual problem with an integration test, which dissappeared after I fixed it. And this error only showed on the Remote RHEL targets for some reason, I don't understand.
"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 @csmart @drybjed @odyssey4me any thoughts?
@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. I was also assuming, a PR has to pass those targets to be accepted to a collection in the community namespace.
@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. I was also assuming, a PR has to pass those targets to be accepted to a collection in the community namespace.
@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. If it is, it makes sense to test against several relevant distros but I guess it doesn't make sense to test on not supported ones.
I now have removed the OS specific variables from the integration tests, if that's what you meant. The remaining operating systems represented in tests/integration/targets/virt_volume/vars are actually not yet end of life, despite being old, or at least still receive some kind of extended support.
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. That's why I don't consider it a good idea to remove something related to those system.
Hi @NK308 thanks very much for reviving this, I'll take a look and put in any comments in line.
The original branch from @flagbot on which my branch is based, had shared code between the virt_storage and virt_pool modules, but @Andersson007 recommended me to put anything, that involves the total refactoring of an already existing module, into a separate PR. That's why I basically reversed the refactoring that exists in https://github.com/flagbot/community.libvirt/tree/feature/virt_volume.
The original branch from @flagbot on which my branch is based, had shared code between the
virt_storageandvirt_poolmodules, but @Andersson007 recommended me to put anything, that involves the total refactoring of an already existing module, into a separate PR. That's why I basically reversed the refactoring that exists in https://github.com/flagbot/community.libvirt/tree/feature/virt_volume.
@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).
The original branch from @flagbot on which my branch is based, had shared code between the
virt_storageandvirt_poolmodules, but @Andersson007 recommended me to put anything, that involves the total refactoring of an already existing module, into a separate PR. That's why I basically reversed the refactoring that exists in https://github.com/flagbot/community.libvirt/tree/feature/virt_volume.@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 https://github.com/ansible-collections/community.libvirt/pull/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. At least to me, the point, where we start to refactor common code into module_utils, seems to be the right point in time for this discussion.
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
The original branch from @flagbot on which my branch is based, had shared code between the
virt_storageandvirt_poolmodules, but @Andersson007 recommended me to put anything, that involves the total refactoring of an already existing module, into a separate PR. That's why I basically reversed the refactoring that exists in https://github.com/flagbot/community.libvirt/tree/feature/virt_volume.
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 check is something that probably should be fixed here to help create a complete test. But if @Andersson007 prefers to merge it as it is, then we can work on that and try to clean it up later.
Just wanted to follow this up to see if we were thinking we should merge this as is, and then follow up with a separate MR? Thanks!
Just wanted to follow this up to see if we were thinking we should merge this as is, and then follow up with a separate MR? Thanks!
I'm OK with either option UPDATE: I can't fix stuff myself later:) So maybe it's worth to finish everything right here
@stratus-ss are you interested in having a look at this MR too? I'm interested in what you think and also that it complements #191
@stratus-ss are you interested in having a look at this MR too? I'm interested in what you think and also that it complements #191
I'll take a deeper look shortly. I read this over a few times and nothing specific jumped out at me. I'd have to think through how this might be achieved in order to reflect on what the user has done here.
Having said all that, I doubt I will come up with anything near what @NK308 has done here
Just following up whether we think we should merge this as is, as per above comments.. thanks!
Let's merge this as it is and hope we can fix any shortcomings in the future, given it's been dragging on a while. Thanks @NK308