Extra inventory info - Githubissues

dseeley commented 2 years ago

SUMMARY

Add more guest information to the community.libvirt.libvirt inventory plugin, in keeping with similar such cloud modules. Include the return from the info(), XMLDesc(), guestInfo() and interfaceAddresses() libvirt functions.

ISSUE TYPE

Feature Pull Request

COMPONENT NAME

libvirt

ADDITIONAL INFORMATION

Many cloud inventory modules include substantial information about the estate being queried; this change goes some way to adding extra such information. As well as the actual XML that defines each guest, this adds the disk topology and the guest IP addresses (the latter two require that guest is powered on, qemu-guest-agent is installed and the _org.qemu.guestagent.0 channel is configured (failure is graceful in the even that these are not true).

Andersson007 commented 2 years ago

There are integration tests for this plugin in tests/integration/targets/inventory_libvirt/tasks. Is it possible to cover the changes there?

csmart commented 2 years ago

@dseeley hi, thanks for the PR! I'm not a libvirt user / maintainer, so one minor general thing from me

Gah, sorry @dseeley, I think I somehow added that suggestion from @Andersson007 into your branch... I should stick to the command line! I'll squash it down and out once it merges, but you might need to pull from your own repo before you make any more changes...

dseeley commented 2 years ago

@dseeley hi, thanks for the PR! I'm not a libvirt user / maintainer, so one minor general thing from me

Gah, sorry @dseeley, I think I somehow added that suggestion from @Andersson007 into your branch... I should stick to the command line! I'll squash it down and out once it merges, but you might need to pull from your own repo before you make any more changes...

No worries, not a problem! (I had assumed that the PR reference was pulled from the prefix in the name of the fragment?)

dseeley commented 2 years ago

There are integration tests for this plugin in tests/integration/targets/inventory_libvirt/tasks. Is it possible to cover the changes there?

I'm not familiar with the testing strategy, but I can have a look. From what I can gather, it appears that the existing inventory test simply tests that the plugin doesn't fail.

Andersson007 commented 2 years ago

There are integration tests for this plugin in tests/integration/targets/inventory_libvirt/tasks. Is it possible to cover the changes there?

I'm not familiar with the testing strategy, but I can have a look. From what I can gather, it appears that the existing inventory test simply tests that the plugin doesn't fail.

Ah, got it, thanks! @dseeley

Is it relevant for all supported libvirt versions? (maybe my questions are dumb, I'm not a maintainer of this repo)
I'm trying to figure out if after we release the collection current users' playbooks will work after they update their Ansible, i.e. the PR doesn't introduce breaking changes
If breaking change are possible, can we avoid them, say, buy using try: except: or by libvirt version checking?
Would be nice if someone checks the PR manually

csmart commented 2 years ago

@Andersson007 @dseeley I will definitely test this myself before we merge it, just running short on time at the moment. At a glance, there is some filter work that I think would benefit from this.

Andersson007 commented 2 years ago

@Andersson007 @dseeley I will definitely test this myself before we merge it, just running short on time at the moment. At a glance, there is some filter work that I think would benefit from this.

@csmart great, thanks! No rush. Trust your judgment:) If no breaking changes, i'm personally OK with any improvements:)

csmart commented 2 years ago

@dseeley thanks, this works and looks pretty handy. I feel like we do need to probably set up some test cases for this, to make sure each distro will support the elements you're trying to extract. Did you have any luck with constructing a test?

Andersson007 commented 2 years ago

BTW we have the Integration test quick start guide. May be helpful for understanding.

dseeley commented 2 years ago

dseeley thanks, this works and looks pretty handy. I feel like we do need to probably set up some test cases for this, to make sure each distro will support the elements you're trying to extract. Did you have any luck with constructing a test?

It seems the current CI integration test for inventory_libvirt is unsupported, and even if run --allow-unsupported, it relies on a user-installed libvirt_qemu/lxc? Is my understanding correct (not looked at this before now)?

Is the expectation to port the existing 'runme.sh' test to standard CI integration tests?

csmart commented 2 years ago

@dseeley it looks like the current test just does a simple inventory list at tests/integration/targets/inventory_libvirt/playbooks/test-inventory.yml and prints the hostvars. I expect that if the integration test is passing on multiple hosts, then we should be good to go. Thanks for your work here!

csmart commented 2 years ago

@dseeley any chance you could do a rebase and force push on this for me? Then I think we can merge it in. Cheers!

dseeley commented 2 years ago

@dseeley any chance you could do a rebase and force push on this for me? Then I think we can merge it in. Cheers!

@csmart - this is up to date now, thanks.

csmart commented 2 years ago

Thanks!

ansible-collections / community.libvirt

Extra inventory info #113

SUMMARY

ISSUE TYPE

COMPONENT NAME

ADDITIONAL INFORMATION