nm: Fix the incorrect change indication when apply the same config twice #396

cathay4t · 2021-06-24T14:34:01Z

Patch 1

When applying the same network connections twice, the second apply still
shows changed: true.

The root cause:

When user never asked about ethtool configuration, network-role
will generate an all-default NM.SettingEthtool and pass it to
NetworkManager daemon. But NetworkManager discard it when saving to
ifcfg plugin as ifcfg plugin will not keep empty ethtool option.
During second apply, the NM.SimpleConnection.compare will return
False indicating configuration changed because of on-disk connection
has no ethtool setting while pending connection does.

To fix it, we just remove the all-default NM.SettingEthtool.

Patch 2

Fix the incorrect change indication for dns option

When applying with dhcp4: "no" or auto6: "no", we get incorrect
change indication even when network connection was not changed.

The root cause is the NM.SettingIPConfig.clear_dns_options(True) will
create an empty list which will be discard by ifcfg plugin.
The follow up NM.Connection.compare() will show configuration changed
as dns option entry missing.

Fixed by remove dns option completely before appending.

coveralls · 2021-06-24T14:37:46Z

Pull Request Test Coverage Report for Build 1000358175

0 of 0 changed or added relevant lines in 0 files are covered.
409 unchanged lines in 1 file lost coverage.
Overall coverage decreased (-0.03%) to 40.178%

Files with Coverage Reduction	New Missed Lines	%
/home/runner/work/network/network/library/network_connections.py	409	21.29%

Totals
Change from base Build 991459915:	-0.03%
Covered Lines:	1039
Relevant Lines:	2586

💛 - Coveralls

thom311 · 2021-06-24T15:58:43Z

fix in NM here: https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/905

(the workaround here is of course still necessary)

liangwen12year

LGTM, nice patch.

tyll · 2021-06-25T06:43:00Z

It would be great to have a test for this that applies a certain profile twice that triggered this error and asserts that the module does not report "changed" for the second time the profile is applied. What do you think?

tyll · 2021-06-29T07:18:13Z

tests/tests_change_indication_on_repeat_run.yml

+            network_connections:
+              - name: "{{ interface }}"
+                state: up
+                type: ethernet
+                ip:
+                  dhcp4: "no"
+                  auto6: "no"


This variable can be set in line 5 for the playbook then it does not need to be repeated and it would ensure that the profiles are always the same.

I had trouble on this. Could you provide an example?

This variable

Which variable?

I had trouble on this.

What did you try, and what was the result?

I tried to place the network_connections at the host level variable, and then remove that from import_role task. Not work.
Even with this in import_role task will not work:

- block: - import_role: name: linux-system-roles.network vars: - network_connections: " {{ network_connections }} " register: __network_connections_result

not sure what the problem is - here is an example playbook calling a role with a variable defined at the host level - https://gist.github.com/richm/e9d338d8b39254b84b90fd2bf0b0b0a1

Thanks for the suggestion. The code is working now.

new patch added.

cathay4t · 2021-06-29T15:20:07Z

@liangwen12year @thom311 I have included another patch fixing the DNS option change indication problem. Please review it again.

tests/tests_change_indication_on_repeat_run.yml

cathay4t · 2021-07-01T08:34:54Z

@tyll @richm Patch updated to use vars to mitigate the duplication in test code.

tests/tests_change_indication_on_repeat_run.yml

library/network_connections.py

tyll

👍

tyll

👍

tyll · 2021-07-01T16:19:54Z

[citest bad]

tyll · 2021-07-01T20:41:14Z

[citest bad]

When applying the same network connections twice, the second apply still shows `changed: true`. The root cause: * When user never asked about ethtool configuration, network-role will generate an all-default `NM.SettingEthtool` and pass it to NetworkManager daemon. But NetworkManager discard it when saving to ifcfg plugin as ifcfg plugin will not keep empty ethtool option. * During second apply, the `NM.SimpleConnection.compare` will return False indicating configuration changed because of on-disk connection has no ethtool setting while pending connection does. To fix it, we just remove the all-default `NM.SettingEthtool`. Signed-off-by: Gris Ge <fge@redhat.com>

When applying with `dhcp4: "no"` or `auto6: "no"`, we get incorrect change indication even when network connection was not changed. The root cause is the `NM.SettingIPConfig.clear_dns_options(True)` will create an empty list which will be discard by ifcfg plugin. The follow up `NM.Connection.compare()` will show configuration changed as dns option entry missing. Fixed by remove dns option completely before appending. Signed-off-by: Gris Ge <fge@redhat.com>

cathay4t · 2021-07-05T09:40:30Z

[citest bad]

cathay4t · 2021-07-05T12:15:49Z

The CI failure is unrelated. Force merging.

cathay4t added the Wait_Review label Jun 24, 2021

thom311 previously approved these changes Jun 24, 2021

View reviewed changes

cathay4t force-pushed the fix_changed branch from 72b0337 to 8460d9f Compare June 24, 2021 14:52

liangwen12year self-requested a review June 24, 2021 16:59

liangwen12year previously approved these changes Jun 24, 2021

View reviewed changes

liangwen12year enabled auto-merge (rebase) June 24, 2021 20:14

liangwen12year disabled auto-merge June 24, 2021 20:14

cathay4t added Work_Ongoing and removed Wait_Review labels Jun 29, 2021

cathay4t force-pushed the fix_changed branch 2 times, most recently from 4feeac3 to adf68bd Compare June 29, 2021 06:52

tyll reviewed Jun 29, 2021

View reviewed changes

cathay4t force-pushed the fix_changed branch 3 times, most recently from 4a8a093 to b2e1a8d Compare June 29, 2021 15:16

cathay4t added Wait_Review and removed Work_Ongoing labels Jun 29, 2021

cathay4t force-pushed the fix_changed branch from b2e1a8d to 7b921c1 Compare June 29, 2021 15:25

richm reviewed Jun 29, 2021

View reviewed changes

tests/tests_change_indication_on_repeat_run.yml Outdated Show resolved Hide resolved

richm reviewed Jun 29, 2021

View reviewed changes

tests/tests_change_indication_on_repeat_run.yml Outdated Show resolved Hide resolved

tyll reviewed Jun 30, 2021

View reviewed changes

tests/tests_change_indication_on_repeat_run.yml Show resolved Hide resolved

cathay4t force-pushed the fix_changed branch 2 times, most recently from 5ab784d to 1cb331e Compare July 1, 2021 08:33

tyll reviewed Jul 1, 2021

View reviewed changes

tests/tests_change_indication_on_repeat_run.yml Outdated Show resolved Hide resolved

cathay4t force-pushed the fix_changed branch 3 times, most recently from d041e89 to 4e65cc4 Compare July 1, 2021 12:04

tyll reviewed Jul 1, 2021

View reviewed changes

library/network_connections.py Outdated Show resolved Hide resolved

tyll reviewed Jul 1, 2021

View reviewed changes

library/network_connections.py Show resolved Hide resolved

tyll approved these changes Jul 1, 2021

View reviewed changes

tyll enabled auto-merge (rebase) July 1, 2021 16:19

cathay4t force-pushed the fix_changed branch from 4e65cc4 to 7add5f4 Compare July 5, 2021 08:00

cathay4t added 2 commits July 5, 2021 16:01

cathay4t force-pushed the fix_changed branch from 7add5f4 to dda9cd1 Compare July 5, 2021 08:01

cathay4t disabled auto-merge July 5, 2021 12:15

cathay4t merged commit 245ff58 into linux-system-roles:main Jul 5, 2021

cathay4t deleted the fix_changed branch July 5, 2021 12:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

nm: Fix the incorrect change indication when apply the same config twice #396

nm: Fix the incorrect change indication when apply the same config twice #396

cathay4t commented Jun 24, 2021 •

edited

coveralls commented Jun 24, 2021 •

edited

thom311 commented Jun 24, 2021 •

edited

liangwen12year left a comment

tyll commented Jun 25, 2021 •

edited

tyll Jun 29, 2021

cathay4t Jun 29, 2021

richm Jun 29, 2021

cathay4t Jun 30, 2021

richm Jun 30, 2021

cathay4t Jul 5, 2021

cathay4t commented Jun 29, 2021

cathay4t commented Jul 1, 2021

tyll left a comment

tyll left a comment

tyll commented Jul 1, 2021

tyll commented Jul 1, 2021

cathay4t commented Jul 5, 2021

cathay4t commented Jul 5, 2021

nm: Fix the incorrect change indication when apply the same config twice #396

nm: Fix the incorrect change indication when apply the same config twice #396

Conversation

cathay4t commented Jun 24, 2021 • edited

Patch 1

Patch 2

coveralls commented Jun 24, 2021 • edited

Pull Request Test Coverage Report for Build 1000358175

💛 - Coveralls

thom311 commented Jun 24, 2021 • edited

liangwen12year left a comment

Choose a reason for hiding this comment

tyll commented Jun 25, 2021 • edited

tyll Jun 29, 2021

Choose a reason for hiding this comment

cathay4t Jun 29, 2021

Choose a reason for hiding this comment

richm Jun 29, 2021

Choose a reason for hiding this comment

cathay4t Jun 30, 2021

Choose a reason for hiding this comment

richm Jun 30, 2021

Choose a reason for hiding this comment

cathay4t Jul 5, 2021

Choose a reason for hiding this comment

cathay4t commented Jun 29, 2021

cathay4t commented Jul 1, 2021

tyll left a comment

Choose a reason for hiding this comment

tyll left a comment

Choose a reason for hiding this comment

tyll commented Jul 1, 2021

tyll commented Jul 1, 2021

cathay4t commented Jul 5, 2021

cathay4t commented Jul 5, 2021

cathay4t commented Jun 24, 2021 •

edited

coveralls commented Jun 24, 2021 •

edited

thom311 commented Jun 24, 2021 •

edited

tyll commented Jun 25, 2021 •

edited