nmcli: module refactor #1113

jbronn · 2020-10-17T02:19:17Z

Refactor nmcli module for consistency and to remove duplicate and/or unused code:

Connection creation/modification methods:
- connection_update: replaces all {create,modify}_connection_* methods
- connection_options: returns consistent nmcli options for all connection types and when detecting changes.
Use consistent settings across connection all types:
- IP settings: e.g., ipv4.addresses instead of ip4 or ipv4.address.
- Connection settings: e.g., connection.autoconnect, connection.interface-nameinstead ofautoconnect, ifname`.
- Bridges: e.g., bridge-port.hairpin-mode instead of bridge-port.hairpin.
- Tunnels: e.g., ip-tunnel.parent instead of dev.
Use all settings when creating a connection so it doesn't need to be immediately modified (less nmcli commands issued).

Bugfixes:

Fixes nmcli: Errors on Ubuntu 16.04 due to invalid 'autoconnect' parameter #459: invalid autoconnect parameter.
Fixes nmcli: cannot modify connection's interface name after creation #1089: cannot modify ifname after connection creation.

New features:

Fixes nmcli: remove Python DBus and GTK Object library dependencies #1112: Remove dependency on DBus Python bindings.

ADDITIONAL NOTES

This PR wouldn't be possible without the work of @thinkdoggie and contributors in nmcli: add idempotent support for any kinds of connections #562 to fix idempotency and add diffs.
The following are easier to address with these changes:
- Multiple IP addresses: nmcli: allow multiple IP addresses #1088
- Missing VLAN argument implementations: nmcli: cannot modify connection's interface name after creation #1089.
- Additional DNS settings: nmcli: new arguments to ignore automatic dns servers and gateways #1087, Add ipv4.dns-options support to nmcli ansible/ansible#66498.
- MAC address and MTU customization (tickets forthcoming).
- WiFi connections and 802.1x auth.

…modifying connections and detecting changes. * Keep DNS list arguments as lists internally. * Remove duplicated code where practical.

ansibullbot · 2020-10-17T02:20:45Z

cc @alcamie101 @nerzhul
click here for bot help

felixfontein

I'm not familiar with the module's code nor did I look through all the code-changes. I'm mainly interested in:

Will this change make something stop working in certain situations that worked before? (If something specific is not installed?)
Is there any (intended) behavior change which breaks existing use-cases?

changelogs/fragments/nmcli-refactor.yml

thinkdoggie · 2020-10-17T18:40:37Z

plugins/modules/net_tools/nmcli.py

-            if self.conn_name == con_item:
-                return True
+    def connection_exists(self):
+        cmd = [self.nmcli_bin, 'con', 'show', self.conn_name]


It is recommend to use command nmcli con show instead of nmcli con show <conn_name> for detecting if connection exists. Here're some reasons:

The performance of nmcli con show <conn_name> may slower than the former one, because all details will be dumped to the standard output.

Using nmcli con show could avoid most potential exceptions in parsing and displaying active connection data.

Not only connection name, but also any other aliases (uuid, path, path), can be used to denote a connection in nmcli con show <conn_id>. But it's better only to match the connection name for determining the existence.

The result of connection list fetched from nmcli con show can also be cached in Nmcli object to avoid duplicated queries. If so, the cached data should be cleaned up in method remove_connection() and create_connection() as well.

Exactly the type of feedback I was looking for! I'll look into fixing the connection_exists implementation tonight.

Fixed in ffeae0b, but no caching.

thinkdoggie · 2020-10-17T18:51:31Z

plugins/modules/net_tools/nmcli.py

+                         'ipv6.dns',
+                         'ipv6.dns-search'):
+            return list
+        return None


Suppose default type is str, isn't it?

Fixed in 618307e.

thinkdoggie · 2020-10-17T18:56:43Z

plugins/modules/net_tools/nmcli.py

+        for setting, value in options.items():
+            setting_type = self.settings_type(setting)
+            type_cast = None
+            if setting_type == bool:


The operator is is preferred for checking type objects compared to the operator ==.

For example, if setting_type is bool:.

While it was an improvement over issubclass(setting_type, bool), I agree the identity operator is what should be used instead here; fixed in 766433e.

thinkdoggie · 2020-10-17T18:58:05Z

plugins/modules/net_tools/nmcli.py

-                elif key in ['ipv4.dns', 'ipv4.dns-search', 'ipv6.dns', 'ipv6.dns-search']:
-                    values = raw_value.split(',')
-                    conn_info[key] = values
+                elif key_type == list:


The operator is is preferred for checking type objects compared to the operator ==.

For example, elif key_type is list:

Fixed in 766433e.

thinkdoggie · 2020-10-17T19:13:40Z

@jbronn thanks for your good ideas and elaborate work.

I add several comments about the implementation details for your references. I didn't test the code yet, but it looks good to work fine. :-)

Co-authored-by: Felix Fontein <felix@fontein.de>

jbronn · 2020-10-19T16:09:41Z

@felixfontein

Will this change make something stop working in certain situations that worked before? (If something specific is not installed?)

I don't think so. What I've done, requirements-wise with fixing #1112, is to eliminate the dependency on the Python bindings and not NetworkManager itself. The updated description in the DOCUMENTATION string should reflect the package requirements for the platforms now.

Is there any (intended) behavior change which breaks existing use-cases?

As far as I know, all existing use-cases should be preserved. Other than the new "feature" of #1112, I tried to ensure I didn't introduce any other new features to muddle review on an otherwise large diff -- for example, I could've easily extended mac support beyond bridges, but didn't.

Here are some differences from the previous implementation:

Now that consistent options are used when creating connections, there's no need to issue a secondary nmcli con mod command to fix settings not done at creation.
I still bring connections up, even though I'm unsure of its necessity as nmcli defaults autoconnect=True and NetworkManager brings them up automatically after creation. The previous implementation's logic is now in the create_connection_up property.
There was a lot of logic (sometimes conflicting) around coming up with a default connection or interface name. These code paths are never reached because conn_name is a required parameter and won't ever be None; connection_update now handles this logic.
The test changes are small considering:
- Reduced duplication in the pytest fixtures (e.g., override execute_command in mocker_set rather than individually in each test fixture).
- A VXLAN test case was found to be using the invalid interface name as a side-effect of fixing ~~#459~~ #1089 .
- MTU settings had to be added to team-slave and ethernet connection outputs as detecting changes for it is now supported.

Please advise as to what test cases I should add if the current ones are insufficient.

felixfontein · 2020-10-19T18:45:58Z

@jbronn thanks for your reply! Sounds good to me.

About the tests: I wish there would be integration tests, but that's probably a lot harder in CI than unit tests (if possible at all). I can't say much about the unit tests, since I neither know nmcli, the module nor the unit tests enough :)

* Improve variable naming, use `convert_func` instead of `type_cast`.

… con show`.

thinkdoggie

+1 that looks good to me. :)

felixfontein · 2020-10-20T05:01:13Z

Changelog fragment looks good, too :)

@alcamie101 @nerzhul any comments?

If nobody complains, I'll merge in a couple of days (assuming @jbronn you don't want to change anything anymore).

jbronn · 2020-10-20T13:23:30Z

I don't plan on adding anything more, thanks for the review y'all! In the meantime, I'll look into implementing integration tests for the other tickets I've opened.

* * Refactor `nmcli` module to use consistent parameters when creating/modifying connections and detecting changes. * Keep DNS list arguments as lists internally. * Remove duplicated code where practical. * DBus and GObject dependencies are not necessary. * Update changelog fragment. Co-authored-by: Felix Fontein <felix@fontein.de> * Update changelog fragment. Co-authored-by: Felix Fontein <felix@fontein.de> * Use identity operator instead of equality for type comparison. * Don't start changelog notes with a capital letter. * * Have `settings_type` return `str` by default instead of `None`. * Improve variable naming, use `convert_func` instead of `type_cast`. * Revert new feature of allowing ethernet types as slaves. * Bring back `list_connection_info` to list all connections with `nmcli con show`. Co-authored-by: Felix Fontein <felix@fontein.de> (cherry picked from commit 7722800)

felixfontein · 2020-10-23T05:14:18Z

@jbronn thanks a lot for doing this!
@thinkdoggie thanks a lot for reviewing!

* * Refactor `nmcli` module to use consistent parameters when creating/modifying connections and detecting changes. * Keep DNS list arguments as lists internally. * Remove duplicated code where practical. * DBus and GObject dependencies are not necessary. * Update changelog fragment. Co-authored-by: Felix Fontein <felix@fontein.de> * Update changelog fragment. Co-authored-by: Felix Fontein <felix@fontein.de> * Use identity operator instead of equality for type comparison. * Don't start changelog notes with a capital letter. * * Have `settings_type` return `str` by default instead of `None`. * Improve variable naming, use `convert_func` instead of `type_cast`. * Revert new feature of allowing ethernet types as slaves. * Bring back `list_connection_info` to list all connections with `nmcli con show`. Co-authored-by: Felix Fontein <felix@fontein.de> (cherry picked from commit 7722800) Co-authored-by: Justin Bronn <jbronn@gmail.com>

mator · 2020-10-23T16:10:31Z

Can we look for other nmcli issues, via searching nmcli in issues? seems like there's quite a few of them, and maybe others are already fixed with this merged PR. Thanks.

jbronn added 2 commits October 16, 2020 20:01

* Refactor nmcli module to use consistent parameters when creating/…

e6b51f1

…modifying connections and detecting changes. * Keep DNS list arguments as lists internally. * Remove duplicated code where practical.

DBus and GObject dependencies are not necessary.

63b58f1

ansibullbot added affects_2.10 community_review module module needs_triage net_tools plugins plugin (any type) tests tests labels Oct 17, 2020

felixfontein reviewed Oct 17, 2020

View reviewed changes

changelogs/fragments/nmcli-refactor.yml Outdated Show resolved Hide resolved

changelogs/fragments/nmcli-refactor.yml Outdated Show resolved Hide resolved

thinkdoggie reviewed Oct 17, 2020

View reviewed changes

felixfontein mentioned this pull request Oct 18, 2020

nmcli - add requirements for arch linux #1116

Closed

jbronn and others added 3 commits October 19, 2020 09:16

Update changelog fragment.

a125f57

Co-authored-by: Felix Fontein <felix@fontein.de>

Update changelog fragment.

202e47b

Co-authored-by: Felix Fontein <felix@fontein.de>

Use identity operator instead of equality for type comparison.

766433e

ansibullbot added unit tests/unit and removed needs_triage labels Oct 19, 2020

jbronn added 4 commits October 19, 2020 20:26

Don't start changelog notes with a capital letter.

9bda3b3

* Have settings_type return str by default instead of None.

618307e

* Improve variable naming, use `convert_func` instead of `type_cast`.

Revert new feature of allowing ethernet types as slaves.

fa3a6a3

Bring back list_connection_info to list all connections with `nmcli…

ffeae0b

… con show`.

thinkdoggie approved these changes Oct 20, 2020

View reviewed changes

felixfontein added the backport-1 label Oct 20, 2020

felixfontein merged commit 7722800 into ansible-collections:main Oct 23, 2020

patchback bot mentioned this pull request Oct 23, 2020

[PR #1113/77228005 backport][stable-1] nmcli: module refactor #1159

Merged

felixfontein mentioned this pull request May 19, 2021

nmcli: new arguments to ignore automatic dns servers and gateways #1087

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

nmcli: module refactor #1113

nmcli: module refactor #1113

jbronn commented Oct 17, 2020 •

edited

ansibullbot commented Oct 17, 2020

felixfontein left a comment

thinkdoggie Oct 17, 2020

jbronn Oct 19, 2020

jbronn Oct 20, 2020

thinkdoggie Oct 17, 2020

jbronn Oct 20, 2020

thinkdoggie Oct 17, 2020

jbronn Oct 19, 2020

thinkdoggie Oct 17, 2020

jbronn Oct 19, 2020

thinkdoggie commented Oct 17, 2020 •

edited

jbronn commented Oct 19, 2020 •

edited

felixfontein commented Oct 19, 2020

thinkdoggie left a comment

felixfontein commented Oct 20, 2020

jbronn commented Oct 20, 2020

felixfontein commented Oct 23, 2020

mator commented Oct 23, 2020

nmcli: module refactor #1113

nmcli: module refactor #1113

Conversation

jbronn commented Oct 17, 2020 • edited

ADDITIONAL NOTES

ansibullbot commented Oct 17, 2020

felixfontein left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thinkdoggie commented Oct 17, 2020 • edited

jbronn commented Oct 19, 2020 • edited

felixfontein commented Oct 19, 2020

thinkdoggie left a comment

Choose a reason for hiding this comment

felixfontein commented Oct 20, 2020

jbronn commented Oct 20, 2020

felixfontein commented Oct 23, 2020

mator commented Oct 23, 2020

jbronn commented Oct 17, 2020 •

edited

thinkdoggie commented Oct 17, 2020 •

edited

jbronn commented Oct 19, 2020 •

edited