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

Add new module lxd_network #31428

Open
wants to merge 2 commits into
base: devel
from

Conversation

Projects
None yet
5 participants
@Nani-o
Copy link
Contributor

Nani-o commented Oct 6, 2017

SUMMARY

This PR add a new module to manage lxd networks, a recent feature.
It is heavily based on the already existing lxd_profile module since the lxd API for networks and profile share a similar structure.

The APIs are described at the following urls :

ISSUE TYPE
  • New Module Pull Request
COMPONENT NAME

lxd_network

ANSIBLE VERSION
ansible 2.4.0.0
  config file = /etc/ansible/ansible.cfg
  configured module search path = [u'/etc/ansible/modules']
  ansible python module location = /usr/local/lib/python2.7/dist-packages/ansible
  executable location = /usr/local/bin/ansible
  python version = 2.7.12 (default, Nov 19 2016, 06:48:10) [GCC 5.4.0 20160609]
ADDITIONAL INFORMATION

The whole purpose of this module is to allow the management of networks configuration inside recent lxd versions with ansible.

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Oct 6, 2017

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Oct 6, 2017

The test ansible-test sanity --test pep8 [?] failed with the following errors:

lib/ansible/modules/cloud/lxd/lxd_network.py:142:7: E225 missing whitespace around operator
lib/ansible/modules/cloud/lxd/lxd_network.py:348:33: E201 whitespace after '('

The test ansible-test sanity --test validate-modules [?] failed with the following errors:

lib/ansible/modules/cloud/lxd/lxd_network.py:0:0: E306 version_added is not a valid version number: '2.X'
lib/ansible/modules/cloud/lxd/lxd_network.py:0:0: E316 ANSIBLE_METADATA.metadata_version: not a valid value for dictionary value @ data['metadata_version']. Got '1.0'

click here for bot help

@Nani-o Nani-o force-pushed the Nani-o:lxd_network branch Oct 6, 2017

@ansibot ansibot removed the ci_verified label Oct 6, 2017

@Nani-o Nani-o force-pushed the Nani-o:lxd_network branch to 8c9ca0f Oct 6, 2017

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Oct 7, 2017

@hnakamur

As a maintainer of a module in the same namespace this new module has been submitted to, your vote counts for shipits. Please review this module and add shipit if you would like to see it merged.

click here for bot help

@hnakamur

This comment has been minimized.

Copy link
Contributor

hnakamur commented Oct 7, 2017

First of all, thank you very much for your pull request!

I tried the lxd_network module on my Ubuntu 16.04 and LXD 2.18.
I confirmed it works as expected on most parts.

However there are some points to be improved.

(1) When I ran the playbook below to create network and ran it again, I expect the task state to be ok but the actual state was changed

---
- hosts: localhost
  connection: local
  tasks:
    - name: Create a network
      lxd_network:
        name: lxdbr1
        state: present
        config:
          ipv4.address: 10.155.92.2/24
        description: My network

(2) When I ran the playbook below to rename the network created above, the network of the old name still existed with the managed value NO.

---
- hosts: localhost
  connection: local
  tasks:
    - name: Rename a network
      lxd_network:
        name: lxdbr1
        new_name: lxdbr1b
        state: present
(venv) hnakamur@express:~/ansible-lxd_network-work$ lxc network list
+---------+----------+---------+-------------+---------+
|  NAME   |   TYPE   | MANAGED | DESCRIPTION | USED BY |
+---------+----------+---------+-------------+---------+

...(snip)...

+---------+----------+---------+-------------+---------+
| lxdbr1  | bridge   | NO      |             | 0       |
+---------+----------+---------+-------------+---------+
| lxdbr1b | bridge   | YES     | My network  | 0       |
+---------+----------+---------+-------------+---------+

...(snip)...

My ansible environment:

(venv) hnakamur@express:~/ansible-lxd_network-work$ ansible --version
ansible 2.5.0
  config file = /home/hnakamur/ansible-lxd_network-work/ansible.cfg
  configured module search path = ['/home/hnakamur/.ansible/plugins/modules', '/usr/share/ansib
le/plugins/modules']
  ansible python module location = /home/hnakamur/ansible-lxd_network-work/venv/lib/python3.5/s
ite-packages/ansible
  executable location = /home/hnakamur/ansible-lxd_network-work/venv/bin/ansible
  python version = 3.5.2 (default, Sep 14 2017, 22:51:06) [GCC 5.4.0 20160609]

I used pip install git+https://github.com/Nani-o/ansible@lxd_network to install ansible.

In the pull request Add lxd_container module by hnakamur · Pull Request #2208 · ansible/ansible-modules-extras, I decided to use the LXD API instead of the lxc command.

However after that, I noticed the feature to manage remotes is missing in the LXD API.
I must admit my decision was wrong and I think now we should switch lxd_container and lxd_profile to use lxc command instead of LXD API.

Maybe it is better for lxd_network to use lxc command too.
Or use LXD API in the first version of lxd_network and then switch to use lxc command later.

Whichever, we'd better to select features of lxc_network to be supported in both of
lxc command and LXD API.

By looking lxc network help, I don't think renaming network is supported in lxc command.
So I think we'd better not include the renaming feature to lxd_network module.

@Nani-o What do you think?

@Nani-o

This comment has been minimized.

Copy link
Contributor Author

Nani-o commented Oct 7, 2017

Thanks @hnakamur taking the time to review this.

(1) I will look into implementing a check to maintain idempotency

(2) That's a bug with lxd that I have already reported and had been fixed see

I'm not against the idea of using lxd cli client, because as we already talked about it allows for patterns like :

lxd-client $ lxc network create [<remote>:]<network> [key=value...]

Where remote is a remote host configured in the lxd client :

lxd-client $ lxc remote add lxd-server1 1.2.3.4
Certificate fingerprint: fdb06d909b77a5311d7437cabb6c203374462b907f3923cefc91dd5fce8d7b60
ok (y/n)? y
Admin password for lxd-server1: 
Client certificate stored at server: lxd-server1

For this to work, the configuration has to be changed on the lxd-server1 host :

lxd-server1 $ lxc config set core.https_address 1.2.3.4:8443
lxd-server1 $ lxc config set core.trust_password something-secure

So we could imagine a remote argument that would allow this :

---
- hosts: localhost
  connection: local
  tasks:
    - name: Create a network
      lxd_network:
        name: lxdbr1
        remote: lxd-server1
        state: present
        config:
          ipv4.address: 10.155.92.2/24
        description: My network

In this example it means that localhost has an lxd-client configured with the correct remote, and we should then be able to create networks/containers/configs/... from that host.

The only two concerns I have are :

  • This means we need an lxd_remote (and probably an lxd_config ?) module. And I fear the interactive prompt.
  • I don't if it's really pertinent to think about this, but how the lxd connection plugin would allow this ? Unless doing something like this ? :
container1 ansible_lxd_client=lxd-client ansible_lxd_remote=lxd-server1

If it meets the standard of ansible, I would be happy to apply thoses changes to my pull request.

@Nani-o

This comment has been minimized.

Copy link
Contributor Author

Nani-o commented Oct 8, 2017

I have written a first version of the module using lxc command, and I feel that it's gonna be complicated.

The first problem I encountered is to modify the configuration of the network.

When you create a network, you can specify inline values :

$ lxc network create lxdbr0 key=value key2=value2 

Thoses values are specific to the "config" part, you can't specify the description this way for example. So you need a second command to edit the description.

When you then want to edit theses values, for example when the network is already present, but the config is different. There is 2 solutions :

  • echoing a yaml using stdin
$ echo "config:\n  ipv4.address: 192.168.1.1/24\n" | lxc network edit lxdbr0
  • using the lxc network get and set method (only for config part, not description)
$ lxc network set lxdbr0 set ipv4.address: 192.168.1.1/24

The second solution would mean executing multiple set and unset command in order to remove/replace the old_keys by the new_keys.

While the first solution could potentially work, it is difficult to maintain idempotency, for example if you only specify an ipv4.address, the "lxc network create" command still add some defaults values :

$ lxc network create test ipv4.address=192.168.1.2/24
Network test created
$ lxc network show test
config:
  ipv4.address: 192.168.1.2/24
  ipv6.address: fd42:34bc:3d30:7265::1/64
  ipv6.nat: "true"
description: ""
name: test3
type: bridge
used_by: []
managed: true

In this case a second run based on the config key would result in a difference, meaning two runs that changed things.

The only acceptable solution I could imagine, would have been to create the networks with no configs and then pushing it using stdin and "lxc network edit" command. But you can't create an "empty" network unlike profile :

$ lxc profile create test
Profile test created
$ lxc profile show test
config: {}
description: ""
devices: {}
name: test
used_by: []

I'm not sure anymore that using the cli is a good idea, even if the API does'nt have all the feature, it is a reliable way to communicate with LXD.

@Nani-o

This comment has been minimized.

Copy link
Contributor Author

Nani-o commented Oct 9, 2017

And I just tested against your point (1) :

$ ansible-playbook test.yml -v
Using /etc/ansible/ansible.cfg as config file

PLAY [localhost] *****************************************************************************************************

TASK [Create a network] **********************************************************************************************
changed: [localhost] => {"actions": ["create"], "changed": true, "failed": false, "old_state": "absent"}

PLAY RECAP ***********************************************************************************************************
localhost                  : ok=1    changed=1    unreachable=0    failed=0

$ lxc network show lxdbr1
config:
  ipv4.address: 10.155.92.2/24
  ipv6.address: fd42:1cb9:1eca:2a35::1/64
  ipv6.nat: "true"
description: My network
name: lxdbr1
type: bridge
used_by: []
managed: true

$ ansible-playbook test.yml -v
Using /etc/ansible/ansible.cfg as config file

PLAY [localhost] *****************************************************************************************************

TASK [Create a network] **********************************************************************************************
changed: [localhost] => {"actions": ["apply_network_configs"], "changed": true, "failed": false, "old_state": "present"}

PLAY RECAP ***********************************************************************************************************
localhost                  : ok=1    changed=1    unreachable=0    failed=0

$ lxc network show lxdbr1
config:
  ipv4.address: 10.155.92.2/24
description: My network
name: lxdbr1
type: bridge
used_by: []
managed: true

It's actually not a behavior of the lxd cli client, but the API add ipv4 and ipv6 defaults when you don't specify them. I gonna do some tests and think about a good implementation.

@bcoca bcoca removed the needs_triage label Oct 9, 2017

@hnakamur

This comment has been minimized.

Copy link
Contributor

hnakamur commented Oct 10, 2017

For point (2), thanks for your investigating time to implement the client using the lxc command.

I'm not sure anymore that using the cli is a good idea, even if the API does'nt have all the feature, it is a reliable way to communicate with LXD.

I agree with you. We'd better prefer using the LXD API here.

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Oct 10, 2017

The test ansible-test sanity --test pep8 [?] failed with the following errors:

lib/ansible/modules/cloud/lxd/lxd_network.py:185:1: E302 expected 2 blank lines, found 1
lib/ansible/modules/cloud/lxd/lxd_network.py:214:35: W291 trailing whitespace

The test ansible-test sanity --test pylint [?] failed with the following error:

lib/ansible/modules/cloud/lxd/lxd_network.py:214:0: trailing-whitespace Trailing whitespace

click here for bot help

@Nani-o Nani-o force-pushed the Nani-o:lxd_network branch to 58cfdbb Oct 10, 2017

@Nani-o

This comment has been minimized.

Copy link
Contributor Author

Nani-o commented Oct 10, 2017

I added the modifications to maintain idempotency. In order to do that, if either one of the subkeys ipv4.address or ipv6.address in the config argument aren't specified, we assign a value of none.

Since they are subkeys of the config argument, I couldn't declare the default value of the argument the "ansible way" :

            config=dict(
                type='dict',
                default={'ipv4.address': none, 'ipv6.address': none}
            ),

The default value will only be used when config isn't supplied, but we need to enforce subkeys of the config argument, not the config argument itself. So I instead merge a dictionnary that contain the default value with the config supplied by the user.

There is certainly a better way to do that, but i'm not seeing how right now, and I don't know if it's a deal breaker for this module. I've added some doc to explain the behavior.

EDIT : Perhaps this behavior is correct but can be completed with some sort of switch, if toggled, the module will keep existing ipv4 and ipv6 when the config doesn't contains one. I will try an implementation to make it clear.

@ansibot ansibot added the stale_ci label Oct 19, 2017

@turlando

This comment has been minimized.

Copy link

turlando commented Jul 19, 2018

Is there any update?

@Nani-o

This comment has been minimized.

Copy link
Contributor Author

Nani-o commented Jul 19, 2018

Hi,

Due to my work (and my limited python skills :D), I do not reallly have the time for pushing this further.

I see that I let a message saying I will make an implementation with some sort of toggle variable, and I can’t really remember why I had this idea. It’s a pretty poor design.

I’m using this module since I « wrote » it in one of my role and had no problem with it.

If someone can help me with this, I would voluntarily do the same for an « lxd_storage » module which will be in the same vein (a port of the already existing lxd_profile module).

EDIT: I just tried to play around the module to see if things was missing. I noticed edge case where the idempotency was not respected, that leads me to find problems with the LXD endpoint (see).

So the LXD API would be fixed in version 3.3 and the idempotency should works as expected.
By now I thinks what can be done to improve this module :

  • Add an update_only toggle that would use the PATCH method to update the config instead of replacing it. In this mode we will only check for the subkeys inputted and not the whole config dict.
  • Force the update_only mode when LXD version is before 3.3 since replacing is not possible (furthermore updating needs the PUT method instead of the PATCH in thoses versions)
  • Add a message to the documentation to explain this weird behavior

Another idea would be to force none value on subkeys presents in the running config and absent from the input config. Thus maintaining a pseudo « replace » mode for version before 3.3. Something that bogs me, is that you will always need to check the LXD version and that will add a call to the API.

I will happily do theses changes. But I would like to debate over what should be expected before implementing it if possible.

Any Ideas ?

@turlando

This comment has been minimized.

Copy link

turlando commented Jul 24, 2018

Hey there,

I've been looking at your code and I have refactored it a bit. I also hardened the process that verifies whether an update is required and I don't think there is the need to use the PATCH method at all.

Here is a working commit and here is my branch. Feel free to rebase on it if you wish. I also rebased on the devel branch since yours was not updated.

I tried to:

  • create a new network
  • delete a network
  • rename a network
  • change the configuration as follows
    • ipv6.address: noneipv6.address: auto (update required)
    • ipv6.address: autoipv6.address: none (update required)
    • ipv6.address: none(nothing) (update not required)
    • (nothing) → ipv6.address: none (update not required)

Let me know if there is any corner case that won't work for you. Keep in mind that this is my first time ever working with Ansible internals so maybe there could be some error.

Note: My environment is LXD 3.2 installed through Snappy on Debian Stretch amd64.

@Nani-o

This comment has been minimized.

Copy link
Contributor Author

Nani-o commented Jul 24, 2018

Hi,

First thanks you for taking the time to implement this.

After testing your modifications everything seems fine to me, and I like how you handle the idempotency. However there is now one point of logic that I would warn about.

Let me try to get some context before since it is even not clear as I would want in my own head haha.

For the LXD API, theses two configs and any combination of theses keys are the same :

config: {}
config:
  ipv4.address: auto
  ipv4.nat: true
  ipv6.address: auto
  ipv6.nat: true

Today this isn’t clear since the PUT endpoint is working as if it was the PATCH one. So today something like :

---
- hosts: localhost
  vars:
    payloads:
      - state: absent
      - config:
          ipv4.address: none
          ipv6.address: none
      - config:
          ipv4.address: auto
  connection: local
  tasks:
    - name: Create a network
      lxd_network:
        name: lxdbr1
        state: "{{ item.state | default('present') }}"
        config: "{{ item.config | default({'ipv4.address': '192.168.1.2/24'}) }}"
        description: My network
        url: unix:/var/snap/lxd/common/lxd/unix.socket
      with_items: "{{ payloads }}"

Will ends up with this config as a result :

config:
  ipv4.address: 10.239.73.1/24
  ipv6.address: none
description: My network
name: lxdbr1
type: bridge
used_by: []
managed: true
status: Created
locations:
- none

Where it should be :

config:
  ipv4.address: 10.40.238.1/24
  ipv4.nat: "true"
  ipv6.address: fd42:21f3:1478:320a::1/64
  ipv6.nat: "true"
description: My network
name: lxdbr1
type: bridge
used_by: []
managed: true
status: Created
locations:
- none

We can not do anything about this, it is how LXD is working right now. But in a near future with the working endpoint, I think that ‘none’ value should not be treated the same as nothing.

And should be ipv6.address: none -> (nothing) (update required).

And here is the trick, if you do this in the actuals LXD versions this would break the idempotency since the endpoint is not working properly.

Also when using other config keys, for example imagine that I use this config :

config:
  bridge.driver: openvswitch
  ipv4.address: auto
  ipv6.address: auto

If I decide to remove the bridge.driver value, this wil not trigger a change in the module where it should have. I agree that with the current endpoint, even if you PUT this config to the endpoint, nothing will change, but it shoulds.

How do you think thoses problematics should be handled ?

P.S : Also what do you think about adding {}as a default value for config ?

@turlando

This comment has been minimized.

Copy link

turlando commented Jul 24, 2018

Hi,

I don't think I fully understood your first example (I'm actually quite a noob in Ansible). I have the feeling that the issue is the LXD API not being consistent between read and write operations.

Regaring to the second example one way of solving it is either hardcoding the default configuration and checking the remote values against it (until the default values change and everything break) or it them from the server (if possible): if the remote's bridge.driver is openvswitch, and it is missing from the module configuration and the default is native then an update is required. If you run the play again it will also not trigger an update.

@Nani-o

This comment has been minimized.

Copy link
Contributor Author

Nani-o commented Jul 24, 2018

Hi,

Sorry for that, I just copy pasted my test playbook without further explanation.

It justs :

  • delete lxdbr1 (to make sure it was not already created, since in our case POST and PUT behave differently when they should get us consistent changes)
  • create lxdbr1 ipv4 and ipv6 addresses set to none
  • modify lxdbr1 setting ipv4 to auto and not specifying ipv6

What I wanted to show there it's the fact that the correct way for LXD to handle this is to wipe off entirely the configuration then put ours in place.

Since those two configs :

config:
  ipv4.address: auto
  ipv6.address: auto
config:
  ipv4.address: auto

are actually the same config, they should produce this exact same config :

config:
  ipv4.address: 10.40.238.1/24
  ipv4.nat: "true"
  ipv6.address: fd42:21f3:1478:320a::1/64
  ipv6.nat: "true"
description: My network
name: lxdbr1
type: bridge
used_by: []
managed: true
status: Created
locations:
- none

And today this is only true regarding creation of a profile not when trying to replace it since it will only take the keys you supplied and set them up letting the present key you have not specified right there.

Concerning hardcoding things in the module, I thinks (as you thinks yourself I suppose) that certainly could lead to more pain than usefulness. I do not say that is not the solution, it will certainly keep away the problems we are facing now, but could introduce new as LXD evolves.

The more and more I'm thinking to this the more I think to take a different approach. We should take as a statement that the endpoint is not working great and could behave differently depending the version it uses.

My proposition

We just take the two configs (running via GET, user supplied in module) and checks that they are exactly the same :

  • YES : We mark this as ok, we do nothing.
  • NO : Then we try to modify the configuration.

Then we just check again the current configuration and compare with with the initial one and if they are exactly the same :

  • YES : We mark this as ok, this was actually an other way to express the same thing or the endpoint doesn't behave quite correctly.
  • NO : We mark this as changed, there was an update needed in order to change the desired state.

We should stop trying to predict how the LXD API will behave on what we gonna give it to her, since it's unreliable and based on a set of rule that can change at any moment the LXD project decide it.

If I tell this is by placing me as an LXD user, not an ansible user. I want this module to act the same as if I would have used the API or the command line myself.

By all means, if I get more mess than I go towards a good solution, please don't hesitate to tell me. I'm new to all of this and don't even know if my thinkings are good or if I'm just wasting people's time.

@turlando

This comment has been minimized.

Copy link

turlando commented Jul 26, 2018

Hi again, I've been thinking a while about your proposal and I have various concerns:

We just take the two configs [...] and check that they are exactly the same

This will hardly happen, since even in most basic case, where a configuration is not provided, LXD will implicitly set ipv4.nat to true among other values. This is the very reason I already provide a default configuration (DEFAULT_LXD_CONFIG) which is the configuration returned by LXD for a network created without any "inner" configuration being specified.

You already pointed out that a non existing parameter is different from a parameter having none as value. I totally agree with this.

A missing configuration parameter as far as I understand represents the default value provided by LXD.

After all, when you provide a configuration value in your Ansible play you are overriding either an existing value or a default value, aren't you?

Given all this and given the current state of the LXD API I would say that providing the module with a default configuration (hardcoding?) would solve most of the issue we're discussing.

I'd be way happier if there was a way to gather the default values from the LXD API itself, but apparently there's not.


Then we try to modify the configuration.

It is not clear to me what kind of changes would you apply.


Regarding your concerns about the module being messy, I think that as far as the code is clear enough about what it is doing, and when not possible it is then carefully commented, it will be easier to track down potential issues (as you can see, I removed all the not necessary abstractions, such as the huge class that used to be). I'd love to see unit tests implemented too, I'm trying to figure out how can it be done.


Let me know if you think I missed the point or any of the things I wrote don't make sense.

@Nani-o

This comment has been minimized.

Copy link
Contributor Author

Nani-o commented Jul 27, 2018

This will hardly happen, since even in most basic case, where a configuration is not provided, LXD will implicitly set ipv4.nat to true among other values. This is the very reason I already provide a default configuration (DEFAULT_LXD_CONFIG) which is the configuration returned by LXD for a network created without any "inner" configuration being specified.

You are right, that’s why I wanted to check between theGET output before and after we made our PUT request.

Given all this and given the current state of the LXD API I would say that providing the module with a default configuration (hardcoding?) would solve most of the issue we're discussing.

I agree, chances are this should not change frequently or at all.

Regarding your concerns about the module being messy, I think that as far as the code is clear enough about what it is doing, and when not possible it is then carefully commented, it will be easier to track down potential issues (as you can see, I removed all the not necessary abstractions, such as the huge class that used to be). I'd love to see unit tests implemented too, I'm trying to figure out how can it be done.

You certainly misread, I was talking about the problematics we were talking about, not your implementation.

@turlando

This comment has been minimized.

Copy link

turlando commented Jul 30, 2018

You are right, that’s why I wanted to check between the GET output before and after we made our PUT request.

Now I understand more clearly your proposal. I actually like this idea because because of its simple design, but this also means that this module will never get a working --check mode, because we're determining whether an update happened, and not if the update has to take place.

You certainly misread, I was talking about the problematics we were talking about, not your implementation.

Sorry, I didn't mean to be rude :) I was actually talking about the design: even if the LXD API require us to write nasty workaround it's possible to keep the code clear and clean :)

So, what should we do?

@Nani-o

This comment has been minimized.

Copy link
Contributor Author

Nani-o commented Jul 31, 2018

Now I understand more clearly your proposal. I actually like this idea because because of its simple design, but this also means that this module will never get a working --check mode, because we're determining whether an update happened, and not if the update has to take place.

This is a thing, I was not even thinking about the check mode.

So, what should we do?

I'm afraid I'm in the same position as you are. I don't know what is the better implementation, but to keep things ansible-esque, your proposition of hardcoding the values seems more preferable now. I would love some external opinions on this.

@turlando

This comment has been minimized.

Copy link

turlando commented Aug 1, 2018

So... paging @hnakamur :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.