Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extra inventory info #113

Merged
merged 10 commits into from Jul 31, 2022
Merged

Conversation

dseeley
Copy link
Contributor

@dseeley dseeley commented Apr 15, 2022

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.guest_agent.0 channel is configured (failure is graceful in the even that these are not true).

Copy link
Contributor

@Andersson007 Andersson007 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

changelogs/fragments/113_extra_inventory_info.yml Outdated Show resolved Hide resolved
@Andersson007
Copy link
Contributor

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

Co-authored-by: Andrew Klychkov <aaklychkov@mail.ru>
@csmart
Copy link
Collaborator

csmart commented Apr 21, 2022

@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
Copy link
Contributor Author

dseeley commented Apr 21, 2022

@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
Copy link
Contributor Author

dseeley commented Apr 21, 2022

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
Copy link
Contributor

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
Copy link
Collaborator

csmart commented Apr 26, 2022

@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
Copy link
Contributor

@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
Copy link
Collaborator

csmart commented Jun 5, 2022

@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
Copy link
Contributor

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

@dseeley
Copy link
Contributor Author

dseeley commented Jun 12, 2022

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
Copy link
Collaborator

csmart commented Jul 30, 2022

@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
Copy link
Collaborator

csmart commented Jul 31, 2022

@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
Copy link
Contributor Author

dseeley commented Jul 31, 2022

@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 csmart merged commit d298ee1 into ansible-collections:main Jul 31, 2022
@csmart
Copy link
Collaborator

csmart commented Jul 31, 2022

Thanks!

dseeley added a commit to dseeley/community.libvirt that referenced this pull request Jul 31, 2022
dseeley added a commit to dseeley/community.libvirt that referenced this pull request Jul 31, 2022
dseeley added a commit to dseeley/community.libvirt that referenced this pull request Jul 31, 2022
* modules_virt_additions:
  Extra inventory info (ansible-collections#113)
@dseeley dseeley deleted the extra_inventory_info branch July 31, 2022 07:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants