lxd_container: support lxd instance types #3661

rchicoli · 2021-11-03T22:14:53Z

SUMMARY

This MR adds support to lxd_container module to deploy either containers or virtual-machines. For that it was necessary to create two new parameters api_endpoint and type:

api_endpoint: is required in order to be backwards compatible with old LXD servers, which does not support the new API endpoint /1.0/virtual-machines or /1.0/instances (implemented in lxd 3.19)
type: there is two different types of instances, “container” or “virtual-machine”

It is related to:

LXD inventory: Support virtual machines #3519

It is a different approach of an open MR:

lxd_virtual_machine - new module #3480

ISSUE TYPE

Feature Pull Request

COMPONENT NAME

lxd_container

ADDITIONAL INFORMATION

The old functionality has not changed. Additionally I've tested the changes only with the latest LXD version 4.19.

it is possible to create virtual-machine or container
if an instance already exists and someone tries to update its "type" from container to virtual-machine or vice-versa, it fails with a warning, otherwise I was only getting changed as status, which is not correct.

conloos · 2021-11-03T23:11:11Z

Hi @rchicoli,

there are several people who are currently working on providing the functionality of the 4.x lxd-api version.
See: lxd_virtual_machine - new module #3480.
@felix I prefer to have only one plugin. My suggestion is to rename lxd_container to lxd, as far as possible and e.g. add a symbolic link for backward compatibility.

It is to late so i didn't look deep on the changes.

Thanks Frank

rchicoli · 2021-11-04T08:24:50Z

Hi @conloos

I've seen the open MR 3480, but like I mentioned I've took a different approach. If the machine "type" is given, then it uses /containers or /virtual-machines.
It is also possible to change the default API endpoint to /1.0/instances, at the end it will work in the same way, but I guess it would be easier to understand the API calls, if someone is following the LXD documentation

FYI: this new API exists since LXD v3.19.

I wouldn't rename or symlink to lxd, because is too generic. For example, if we would create a different module for managing LXC Volumes, then I would expect another module named lxc_volume, just like lxd_profile. For that reason my suggestion are:

to keep the module name as lxd_container, as the LXC CLI stands for Linux Containers
to rename to lxd_instance (similar to the new API endpoint), symlink to lxd_container for backward compatibility and add the deprecation warning

What do you think?

Have a good day

rchicoli · 2021-11-04T11:08:19Z

plugins/modules/cloud/lxd/lxd_container.py

+            It can be set to "/1.0/instances" for creating instances of type container or virtual-machine.
+          - If the LXD server version is older than 3.19, then this should be set to "/1.0/containers".
+        type: str
+        required: false
    architecture:
        description:
          - 'The architecture for the container (for example C(x86_64) or C(i686)).


I've seen in the documentation that there are multiple lines that reference to container and not an instance. I can update this tonight, if this MR is welcome

Update the lxd_container module to enable the new LXD API endpoint, which supports different types of instances, such as containers and virtual machines. The type attributes can be set explicitly to create containers or virtual machines.

conloos · 2021-11-06T10:59:35Z

Hi @rchicoli,

I looked at your changes and was wondering if "api_endpoint" is needed at all.
By checking whether the branch "/1.0/instances" exists, it can, from my point of view, be configured automatically so that the user does not have to do a configuration by hand and we minimize the configuration options.

I agree that all documentation have to be updated, like it is done in #3519.

Greetings Frank

felixfontein · 2021-11-06T20:32:28Z

I think something like api_endpoint should not really be exposed to users, it should be selected somehow by the module (based on auto-detection and/or user input).

rchicoli · 2021-11-06T23:06:47Z

Ok. the changes should always be backwards compatible, imho to give the user the opportunity to switch to a new endpoint would have been the better choice. Let me try to explain.

I see only one problem. If the user deploys a container and for whatever reason, he changes its "type" to virtual-machine and set the "state" to absent, then the container won't be deleted and Ansible will report that there is nothing to be changed.
The reason is, if you query /1.0/instances, LXD responses everything. Now if the type has changed, then the internal API endpoints will be switched to /1.0/virtual-machines, LXD says, nothing found. Ansible is happy, because he thinks the instance does not exist.
I guess you get my point. If the API would be /instances LXD is clever enough to remove the running instance, even though the type wouldn't match

Anyways, I will revert that..

remove the /1.0/instances api
and reuse only /1.0/containers or /1.0/virtual-machines based on the type provided

plugins/modules/cloud/lxd/lxd_container.py

changelogs/fragments/3661-lxd_container-add-vm-support.yml

plugins/modules/cloud/lxd/lxd_container.py

rchicoli · 2021-11-10T22:24:40Z

hey @felixfontein thanks for the review.
The changes are already done. Please let me know if you find something else, otherwise I would rebase and force push everything.

plugins/modules/cloud/lxd/lxd_container.py

felixfontein

Looks good to me. Just some nit comments. @conloos what do you think?

plugins/modules/cloud/lxd/lxd_container.py

russoz · 2021-11-13T01:52:52Z

Hi, sorry for not participating in the discussion earlier but, isn't a bit misleading to have a module named lxd_container that can manage something that is not a container? I see all the replace("container", "instance") throughout the file, but the module name remains the same. Wouldn't it be more consistent to copy this PR's code to a new lxd_instance and deprecate lxd_container? Just my $0.02.

Otherwise, disregarding that terminology conundrum, it LGTM.

felixfontein · 2021-11-13T12:46:05Z

there are several people who are currently working on providing the functionality of the 4.x lxd-api version. See: lxd_virtual_machine - new module #3480. @felix I prefer to have only one plugin. My suggestion is to rename lxd_container to lxd, as far as possible and e.g. add a symbolic link for backward compatibility.

It is to late so i didn't look deep on the changes.

@russoz at the beginning of this PR @conloos wrote the above. I think renaming it is a good idea (I don't care whether lxd or lxd_instance), instead of having suddenly two mostly identical modules to maintain.

conloos · 2021-11-13T14:16:31Z

Hello @ALL,

from my point of view everything looks fine and the changes don't break anything.

In the next step, the module should be completely reviewed/rewritten.
I checked the swagger documentation and the endpoints "containers" and "virtual-machines" are not listed. So I think there are only for backward compatibility. So in the next release, the module should be check the exported endpoints and use "containers" for compatibility and "instances" if there is support for.

I can make that, but if @rchicoli is interested I'm fine with that.

The second thing is the name of the module. I vote for "renaming to: lxd_instance and set a deprecate message by using lxd_container" as @rchicoli it has already suggested. Maybe a symlink and check which name was used to call up the module is possible?

Thanks Frank

felixfontein · 2021-11-13T14:44:30Z

from my point of view everything looks fine and the changes don't break anything.

In that case I will merge once the final nits are fixed.

The second thing is the name of the module. I vote for "renaming to: lxd_instance and set a deprecate message by using lxd_container" as @rchicoli it has already suggested. Maybe a symlink and check which name was used to call up the module is possible?

The symlink will mainly be needed for Ansible 2.9, for ansible-base 2.10 and later an entry in meta/runtime.yml will be needed. I can help with the rename; if someone starts a PR for the rename which renames the module (and does all the documentation updates etc. that are necessary) and keeps the checkbox for maintainer pushes enabled, I can push the remaining technical parts to it.

rchicoli · 2021-11-13T18:11:31Z

In the next step, the module should be completely reviewed/rewritten.

I agree.

I checked the swagger documentation and the endpoints "containers" and "virtual-machines" are not listed. So I think there are only for backward compatibility. So in the next release, the module should be check the exported endpoints and use "containers" for compatibility and "instances" if there is support for.

Exactly. I have the feeling they didn't want to duplicate the swagger api to add the "containers" and "virtual-machines" endpoints. So you are wondering here is the API implementation:
https://github.com/lxc/lxd/blob/e6523c33b1c6987f794623a5abbc857b8a32a729/lxd/instances.go#L41-L45

I can make that, but if @rchicoli is interested I'm fine with that.

you can take over this time. At the moment I am kinda busy with other projects.

I want to be a bit precautious before squashing all commits, since it is my first contribution.
So would it be fine to rebase everything now?

If I understood it right, further changes will be done as a nextt step in a separated MR.

I am glad to contribute to the code. Thanks everyone for the support.

felixfontein · 2021-11-20T07:20:07Z

Merging since there's an implicit 'shipit' by @conloos in #3661 (comment) (sorry, missed that).

patchback · 2021-11-20T07:20:27Z

Backport to stable-4: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-4/58eb94fff38b7eba8d5116bd4b3c35c9ced08bbe/pr-3661

Backported as #3761

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

* lxd_container: support lxd instance types Update the lxd_container module to enable the new LXD API endpoint, which supports different types of instances, such as containers and virtual machines. The type attributes can be set explicitly to create containers or virtual machines. * lxd_container: rename references from containers to instances * lxd_container: add an example of creating vms * lxd_container: update doc * lxd_container: fix pylint * resolve converstation * remove type from config * remove outdated validation related to the instance api * correct diff * changing last bits * add missing dot (cherry picked from commit 58eb94f)

felixfontein · 2021-11-20T07:20:47Z

@rchicoli thanks for your contribution!
@conloos @russoz thanks for reviewing!

* lxd_container: support lxd instance types Update the lxd_container module to enable the new LXD API endpoint, which supports different types of instances, such as containers and virtual machines. The type attributes can be set explicitly to create containers or virtual machines. * lxd_container: rename references from containers to instances * lxd_container: add an example of creating vms * lxd_container: update doc * lxd_container: fix pylint * resolve converstation * remove type from config * remove outdated validation related to the instance api * correct diff * changing last bits * add missing dot (cherry picked from commit 58eb94f) Co-authored-by: rchicoli <rchicoli@users.noreply.github.com>

This comment has been minimized.

Sign in to view

rchicoli force-pushed the add-vm-support branch from 2edd7f8 to af218bd Compare November 3, 2021 22:33

ansibullbot removed ci_verified Push fixes to PR branch to re-run CI needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Nov 3, 2021

rchicoli commented Nov 4, 2021

View reviewed changes

rchicoli force-pushed the add-vm-support branch from af218bd to ca40c41 Compare November 4, 2021 14:36

lxd_container: support lxd instance types

fc27c49

Update the lxd_container module to enable the new LXD API endpoint, which supports different types of instances, such as containers and virtual machines. The type attributes can be set explicitly to create containers or virtual machines.

rchicoli force-pushed the add-vm-support branch from ca40c41 to fc27c49 Compare November 4, 2021 14:39

felixfontein added the backport-4 label Nov 6, 2021

rchicoli added 3 commits November 7, 2021 00:19

lxd_container: rename references from containers to instances

dc1009b

lxd_container: add an example of creating vms

f91dc80

lxd_container: update doc

3738e47

ansibullbot added 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 6, 2021

lxd_container: fix pylint

7c39f54

ansibullbot removed ci_verified Push fixes to PR branch to re-run CI needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Nov 7, 2021

felixfontein reviewed Nov 10, 2021

View reviewed changes

rchicoli added 2 commits November 10, 2021 23:02

resolve converstation

9035b11

remove type from config

239cf8d

rchicoli commented Nov 10, 2021

View reviewed changes

plugins/modules/cloud/lxd/lxd_container.py Outdated Show resolved Hide resolved

rchicoli added 2 commits November 10, 2021 23:41

remove outdated validation related to the instance api

9feac9f

correct diff

5293c9c

felixfontein added the check-before-release PR will be looked at again shortly before release and merged if possible. label Nov 12, 2021

felixfontein reviewed Nov 12, 2021

View reviewed changes

plugins/modules/cloud/lxd/lxd_container.py Outdated Show resolved Hide resolved

plugins/modules/cloud/lxd/lxd_container.py Outdated Show resolved Hide resolved

plugins/modules/cloud/lxd/lxd_container.py Outdated Show resolved Hide resolved

rchicoli added 2 commits November 13, 2021 18:34

changing last bits

255b5ca

add missing dot

7232df9

felixfontein approved these changes Nov 20, 2021

View reviewed changes

felixfontein removed the check-before-release PR will be looked at again shortly before release and merged if possible. label Nov 20, 2021

felixfontein merged commit 58eb94f into ansible-collections:main Nov 20, 2021

patchback bot mentioned this pull request Nov 20, 2021

[PR #3661/58eb94ff backport][stable-4] lxd_container: support lxd instance types #3761

Merged

adamcstephens mentioned this pull request Dec 13, 2021

lxd_virtual_machine - new module #3480

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lxd_container: support lxd instance types #3661

lxd_container: support lxd instance types #3661

rchicoli commented Nov 3, 2021

This comment has been minimized.

conloos commented Nov 3, 2021

rchicoli commented Nov 4, 2021 •

edited

Loading

rchicoli Nov 4, 2021

conloos commented Nov 6, 2021

felixfontein commented Nov 6, 2021

rchicoli commented Nov 6, 2021

rchicoli commented Nov 10, 2021

felixfontein left a comment

russoz commented Nov 13, 2021

felixfontein commented Nov 13, 2021

conloos commented Nov 13, 2021

felixfontein commented Nov 13, 2021

rchicoli commented Nov 13, 2021

felixfontein commented Nov 20, 2021

patchback bot commented Nov 20, 2021 •

edited

Loading

felixfontein commented Nov 20, 2021

lxd_container: support lxd instance types #3661

lxd_container: support lxd instance types #3661

Conversation

rchicoli commented Nov 3, 2021

SUMMARY

ISSUE TYPE

COMPONENT NAME

ADDITIONAL INFORMATION

This comment has been minimized.

conloos commented Nov 3, 2021

rchicoli commented Nov 4, 2021 • edited Loading

rchicoli Nov 4, 2021

Choose a reason for hiding this comment

conloos commented Nov 6, 2021

felixfontein commented Nov 6, 2021

rchicoli commented Nov 6, 2021

rchicoli commented Nov 10, 2021

felixfontein left a comment

Choose a reason for hiding this comment

russoz commented Nov 13, 2021

felixfontein commented Nov 13, 2021

conloos commented Nov 13, 2021

felixfontein commented Nov 13, 2021

rchicoli commented Nov 13, 2021

felixfontein commented Nov 20, 2021

patchback bot commented Nov 20, 2021 • edited Loading

Backport to stable-4: 💚 backport PR created

felixfontein commented Nov 20, 2021

rchicoli commented Nov 4, 2021 •

edited

Loading

patchback bot commented Nov 20, 2021 •

edited

Loading