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

LXD inventory: Support virtual machines #3519

Merged
merged 17 commits into from
Dec 13, 2021
Merged

LXD inventory: Support virtual machines #3519

merged 17 commits into from
Dec 13, 2021

Conversation

ElieDeloumeau
Copy link
Contributor

@ElieDeloumeau ElieDeloumeau commented Oct 6, 2021

SUMMARY

Adds full compatibility for LXD 4.x. It allows to list containers and VMs

The plugin is already only compatible with LXD 4 (due to a call to /1.0/networks/{interface}/state), this PR adds requirement and VM support

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

lxd inventory plugin

ADDITIONAL INFORMATION

I renamed "container" to "instance" (sed) and fixed some specific needs: L611, L632, L636, L649

My LXD instances:

$ lxc list -c nt volatile.last_state.power="RUNNING"
+----------------+-----------------+
|      NAME      |      TYPE       |
+----------------+-----------------+
| gitlab-runner  | VIRTUAL-MACHINE |
+----------------+-----------------+
| grafana        | CONTAINER       |
+----------------+-----------------+
| home-assistant | VIRTUAL-MACHINE |
+----------------+-----------------+
| influxdb       | CONTAINER       |
+----------------+-----------------+
| mqtt           | CONTAINER       |
+----------------+-----------------+
| mysql          | CONTAINER       |
+----------------+-----------------+
| pi-hole        | CONTAINER       |
+----------------+-----------------+
| sozu           | CONTAINER       |
+----------------+-----------------+
| test           | CONTAINER       |
+----------------+-----------------+
| test-centos8   | CONTAINER       |
+----------------+-----------------+
| test-debian11  | CONTAINER       |
+----------------+-----------------+

Before (containers only):

$ ansible-inventory -i inventory/lxd.yml --playbook-dir ./ --graph
@all:
  |--@ungrouped:
  |  |--grafana
  |  |--influxdb
  |  |--mqtt
  |  |--mysql
  |  |--pi-hole
  |  |--sozu
  |  |--test
  |  |--test-centos8
  |  |--test-debian11

After (containers and VM):

$ ansible-inventory -i inventory/lxd.yml --playbook-dir ./ --graph
@all:
  |--@ungrouped:
  |  |--gitlab-runner
  |  |--grafana
  |  |--home-assistant
  |  |--influxdb
  |  |--mqtt
  |  |--mysql
  |  |--pi-hole
  |  |--sozu
  |  |--test
  |  |--test-centos8
  |  |--test-debian11

@ansibullbot
Copy link
Collaborator

@ansibullbot ansibullbot added feature This issue/PR relates to a feature request inventory inventory plugin new_contributor Help guide this first time contributor plugins plugin (any type) needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Oct 6, 2021
@ElieDeloumeau ElieDeloumeau changed the title LXD 4.x compatibility (Containers and VMs) Draft: LXD 4.x compatibility (Containers and VMs) Oct 6, 2021
@ElieDeloumeau
Copy link
Contributor Author

I will fix the tests

@conloos
Copy link
Contributor

conloos commented Oct 6, 2021

Hi @ElieDeloumeau,

this looks very good. I am currently working on caching and will put it on hold until it has been taken over.

Thanks Frank

Copy link
Collaborator

@felixfontein felixfontein left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution! Please add a changelog fragment.

Please note that all your changes must be backwards compatible, i.e. they must not break existing setups.

plugins/inventory/lxd.py Show resolved Hide resolved
plugins/inventory/lxd.py Show resolved Hide resolved
@felixfontein felixfontein added backport-3 check-before-release PR will be looked at again shortly before release and merged if possible. labels Oct 6, 2021
@ansibullbot

This comment has been minimized.

@ansibullbot ansibullbot added merge_commit This PR contains at least one merge commit. Please resolve! needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html labels Oct 7, 2021
Co-authored-by: Felix Fontein <[email protected]>
@ansibullbot ansibullbot added tests tests unit tests/unit and removed merge_commit This PR contains at least one merge commit. Please resolve! needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Oct 7, 2021
@ansibullbot ansibullbot added needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI and removed merge_commit This PR contains at least one merge commit. Please resolve! needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Nov 22, 2021
@felixfontein
Copy link
Collaborator

@conloos how do you want to proceed? Is this PR ready for merging from your point of view? If you give your OK before tomorrow very early morning, I can merge it. I'll work on the 4.1.0 release very early tomorrow morning, if the PR doesn't make it in until then it has to wait for 4.2.0 in ~four weeks (of course it can get merged earlier, it just won't show up in a release before then).

@conloos
Copy link
Contributor

conloos commented Nov 23, 2021

Hi Felix,

I can't check everything today morning, including my tests.
I'll do the review by the weekend and suggest that I work on the caching then.
So we can ship both features in 4 weeks.

Greetings Frank

@ansibullbot ansibullbot added the stale_ci CI is older than 7 days, rerun before merging label Dec 1, 2021
@felixfontein
Copy link
Collaborator

@conloos it has been a couple of weeks, any progress so far? :)

@conloos
Copy link
Contributor

conloos commented Dec 9, 2021

Hi @felixfontein,

in the last weeks I develop the next Inventory AixBOMS.
I hope to create the pull request in the next week.

At the weekend i check the changes at the lxd plungin to be prepared for the next release.

Thanks Frank

@conloos
Copy link
Contributor

conloos commented Dec 13, 2021

Hi @felixfontein,

everything is ok.
I checked the plugin on two servers.

But at the moment I am not able to get a working Unittest.
Its not a problem by the inventory its at ansible_collections.community.internal_test_tools.

But here all checks passed.

So please ship it.

Thanks Frank

@felixfontein
Copy link
Collaborator

@conloos the problems you are experiencing with the unit tests are probably because you do not have https://galaxy.ansible.com/community/internal_test_tools installed where ansible-test will find it (I think it only looks for other collections in the same directory structure where the current collection is in).

@ansibullbot ansibullbot removed the stale_ci CI is older than 7 days, rerun before merging label Dec 13, 2021
@conloos
Copy link
Contributor

conloos commented Dec 13, 2021

@conloos the problems you are experiencing with the unit tests are probably because you do not have https://galaxy.ansible.com/community/internal_test_tools installed where ansible-test will find it (I think it only looks for other collections in the same directory structure where the current collection is in).

I use the Docker version: ansible-test units --docker -v

@felixfontein
Copy link
Collaborator

If you run git clone https://github.com/ansible-collections/community.internal_test_tools.git ../internal_test_tools before running that, the collection should be available.

@felixfontein felixfontein merged commit 8825ef4 into ansible-collections:main Dec 13, 2021
@patchback
Copy link

patchback bot commented Dec 13, 2021

Backport to stable-4: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-4/8825ef4711ab44598f9e480b3d5ea75c608701c8/pr-3519

Backported as #3900

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

@felixfontein felixfontein removed the check-before-release PR will be looked at again shortly before release and merged if possible. label Dec 13, 2021
patchback bot pushed a commit that referenced this pull request Dec 13, 2021
* LXD 4.x compatibility (Containers and VMs)

* add changelog fragment

* update fixture

* update plugin options

* backwards compatible alias

Co-authored-by: Felix Fontein <[email protected]>

* Update changelogs/fragments/3519-inventory-support-lxd-4.yml

Co-authored-by: Felix Fontein <[email protected]>

* add lxd 4.0 requirement

* filter for type of virtualization added. due to duplication in the namespace, "type" is not used as the keyword but "nature".

* add type filter

Since the first version of this inventory plugin only supports containers,
a filter function was added to filter between containers and
virtual machines or both.
By default only containers are displayed, as in the first version of the plugin.
This behavior will change in the future.

* rename C(nature) to C(type)

The term "nature" does not fit into the lxd namespace.
Therefore i renamed nature to type.

* update changelog fragment

* Update plugins/inventory/lxd.py

Co-authored-by: Felix Fontein <[email protected]>

* Apply suggestions from code review

Co-authored-by: Felix Fontein <[email protected]>

* rename typefilter to type_filter

* fix tests with type_filter

* Update plugins/inventory/lxd.py

* Update plugins/inventory/lxd.py

Co-authored-by: Felix Fontein <[email protected]>
Co-authored-by: Frank Dornheim <“[email protected]@users.noreply.github.com”>
(cherry picked from commit 8825ef4)
@felixfontein
Copy link
Collaborator

@ElieDeloumeau thanks for your contribution!
@conloos thanks for reviewing!

@ElieDeloumeau
Copy link
Contributor Author

@felixfontein @conloos Thanks for helping me 😃

@ElieDeloumeau ElieDeloumeau deleted the lxd_4 branch December 14, 2021 00:41
felixfontein pushed a commit that referenced this pull request Dec 14, 2021
* LXD 4.x compatibility (Containers and VMs)

* add changelog fragment

* update fixture

* update plugin options

* backwards compatible alias

Co-authored-by: Felix Fontein <[email protected]>

* Update changelogs/fragments/3519-inventory-support-lxd-4.yml

Co-authored-by: Felix Fontein <[email protected]>

* add lxd 4.0 requirement

* filter for type of virtualization added. due to duplication in the namespace, "type" is not used as the keyword but "nature".

* add type filter

Since the first version of this inventory plugin only supports containers,
a filter function was added to filter between containers and
virtual machines or both.
By default only containers are displayed, as in the first version of the plugin.
This behavior will change in the future.

* rename C(nature) to C(type)

The term "nature" does not fit into the lxd namespace.
Therefore i renamed nature to type.

* update changelog fragment

* Update plugins/inventory/lxd.py

Co-authored-by: Felix Fontein <[email protected]>

* Apply suggestions from code review

Co-authored-by: Felix Fontein <[email protected]>

* rename typefilter to type_filter

* fix tests with type_filter

* Update plugins/inventory/lxd.py

* Update plugins/inventory/lxd.py

Co-authored-by: Felix Fontein <[email protected]>
Co-authored-by: Frank Dornheim <“[email protected]@users.noreply.github.com”>
(cherry picked from commit 8825ef4)

Co-authored-by: Élie <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature This issue/PR relates to a feature request inventory inventory plugin new_contributor Help guide this first time contributor plugins plugin (any type) tests tests unit tests/unit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants