fix: Allow network to restart when wireless or team connection is specified #675

liangwen12year · 2024-02-18T03:11:15Z

Enhancement: Ask user's consent to restart NM due to wireless or team interfaces when the updates for network packages are available.

Reason: When wireless or team connections are specified and the updates for network packages are available, NetworkManager must be restarted, the role requires user's consent to restart NetworkManager. Otherwise, there
might be property conflicts between NetworkManager daemon and plugin, or
NetworkManager plugin is not taking effect.

Result: When wireless or team connections are specified and the updates for network packages are available, NetworkManager must be restarted, the role will ask user's explicit consent to restart NetworkManager.

Issue Tracker Tickets (Jira or BZ if any):

Resolves: https://issues.redhat.com/browse/NMT-1039

liangwen12year · 2024-02-18T03:11:46Z

[citest]

codecov · 2024-02-18T03:12:33Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 20.50%. Comparing base (b90e123) to head (0fca710).

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #675   +/-   ##
=======================================
  Coverage   20.50%   20.50%           
=======================================
  Files          10       10           
  Lines        1478     1478           
  Branches      433      433           
=======================================
  Hits          303      303           
  Misses       1174     1174           
  Partials        1        1

Flag	Coverage Δ
sanity	`20.50% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

liangwen12year · 2024-02-18T14:30:08Z

[citest]

liangwen12year · 2024-02-18T20:55:22Z

[citest]

tyll · 2024-02-19T14:32:26Z

This seems to be wrong. The role needs to handle this scenario properly (or the NM packages need to be changed).

tyll

The role should handle the scenario instead of doctoring the tests.

liangwen12year · 2024-02-19T21:35:09Z

[citest]

defaults/main.yml

liangwen12year · 2024-02-19T23:18:54Z

[citest]

tyll

This does not seem to be correct.

defaults/main.yml

liangwen12year · 2024-02-20T19:51:12Z

[citest]

tasks/main.yml

README.md

tasks/main.yml

liangwen12year · 2024-02-22T02:35:03Z

[citest]

liangwen12year · 2024-03-12T20:51:39Z

The PR description is missing the text for Enhancement/Reason/Result.

I added it.

liangwen12year · 2024-03-12T20:51:47Z

[citest]

tasks/main.yml

tests/ensure_provider_tests.py

tyll · 2024-03-13T06:04:42Z

tests/playbooks/tests_wireless_and_network_restart.yml

+    - name: Check if rpm ostree system - cannot test
+      meta: end_host
+      when: __network_is_ostree | d(false)


What's the reason this cannot be tested on an rpm ostree system?

https://github.com/linux-system-roles/network/blob/main/README-ostree.md#rpm-ostree, " the /usr filesystem is read-only, and the role cannot install packages".

The problem is that we don't have a good way currently to pass in the custom yum repos/copr repos that are used by these tests. If you could build an image with all of the repos and packages, then I think the tests should work.

main/README-ostree.md#rpm-ostree, " the /usr filesystem is read-only, and the role cannot install packages".

This is not a sufficient explanation. Because this still leaves the question what's the reason that this cannot be tested? The role needs to have a defined behavior when running against ostree systems.

The problem is that we don't have a good way currently to pass in the custom yum repos/copr repos that are used by these tests. If you could build an image with all of the repos and packages, then I think the tests should work.

Maybe I am missing something. If an ostree system is missing the wifi or team subpackage (or even some NM or Nmstate package), then the role needs to handle this with a proper error message (if we intend to support ostree targets). If the role handles this correctly.

The problem is that the test wants to remove the package NetworkManager-wifi then reinstall it later - packages cannot be removed from an ostree system - the only thing package management can do at runtime is to verify that the given package exists (or is absent).

If the test needs to verify the package NetworkManager-wifi is absent, then call the role to install it, then verify that the package NetworkManager-wifi is present, then at least this part of the test is not applicable for ostree systems. If we really want this test to be run on ostree systems, then we need to modify the test such that the testing of the package absence and presence is skipped on ostree systems. I don't know if that will substantially change what this test is doing.

I was thinking of changing name: Check if rpm ostree system - cannot test to something like name: Cannot test ostree systems because ...

ok. note that there are a few places in tests/ other than this that would require such a change. If @liangwen12year would like to take care of that in this PR, that's fine. But, perhaps since that change would be unrelated to this PR, I can create a separate PR after this one is merged to add such a descriptive name: to the other places in the code where test is skipped due to ostree.

If there are multiple places, maybe it is also worth to write this in one file and include it in all places. Doing this in a follow-up PR is fine.

tracked by #681

I meant doing this at other places and/or using a task file for all those places is something to do in a different PR. The task in this PR needs a good name, still.

I renamed the task into following.

- name: Cannot test ostree systems because the package can not be removed or installed - 'NetworkManager-wifi'

tests/playbooks/tests_wireless_and_network_restart.yml

tests/tasks/remove_and_assert_package_absent.yml

tests/ensure_provider_tests.py

liangwen12year · 2024-03-14T21:03:10Z

[citest]

Signed-off-by: Wen Liang <[email protected]>

liangwen12year · 2024-03-15T02:14:01Z

[citest]

If updates for network packages are available and wireless or team connections are specified, NetworkManager must be restarted, the role requires user's consent to restart NetworkManager. Otherwise, there might be property conflicts between NetworkManager daemon and plugin, or NetworkManager plugin is not taking effect. `update_cache` is enabled in the module tasks to check if updates for network packages are available due to wireless or team interfaces, in that case, NetworkManager needs user's explicit consent to be restarted after the network package updates. And using `state: latest` for checking the network package updates because we have to guarantee that NetworkManager and its plugin have the same and most recent version for configuring the network connections settings in the backend. It is worthwhile to mention that we have both tasks using dnf and yum module for checking available updates for network packages. Because checking package cache update is not supported in Ansible package module, Fedora and RHEL8+ use DNF package manager by default, RHEL7 uses yum package manager by default. This commit will address the situation that users forget to explicitly specify `network_allow_restart: true` when specifying wireless or team connections. Signed-off-by: Wen Liang <[email protected]>

…nnections Signed-off-by: Wen Liang <[email protected]>

liangwen12year · 2024-03-15T14:53:45Z

tests/playbooks/tests_wireless_and_network_restart.yml

+        - always
+
+    - name: Cannot test ostree systems because the package can not be removed
+        or installed - 'NetworkManager-wifi'


If we are using the jinja2 syntax for the '{{ package }}', the {{ package }}' should be appeared at the end of the task name.

FWIW, cannot is more common than can not.

Right, thanks for pointing it out, I will address it in another PR.

liangwen12year · 2024-03-15T14:57:39Z

@tyll , if you agree, then I will proceed to merge this PR.

liangwen12year · 2024-03-15T14:57:52Z

[citest]

tyll

Thank you, LGTM

liangwen12year force-pushed the nm_plugin_hotfix branch from 212f054 to 3578dad Compare February 18, 2024 14:24

liangwen12year force-pushed the nm_plugin_hotfix branch from 3578dad to 25f3183 Compare February 18, 2024 20:55

liangwen12year added the Wait_Review label Feb 19, 2024

liangwen12year requested review from richm and ffmancera February 19, 2024 13:40

richm previously approved these changes Feb 19, 2024

View reviewed changes

tyll requested changes Feb 19, 2024

View reviewed changes

liangwen12year dismissed richm’s stale review via 5adf18b February 19, 2024 21:34

liangwen12year force-pushed the nm_plugin_hotfix branch from 25f3183 to 5adf18b Compare February 19, 2024 21:34

liangwen12year changed the title ~~tests: Install newest NM to match NM plugin version~~ fix: Allow network to restart when wireless or team connection is specified Feb 19, 2024

richm reviewed Feb 19, 2024

View reviewed changes

defaults/main.yml Outdated Show resolved Hide resolved

liangwen12year force-pushed the nm_plugin_hotfix branch from 5adf18b to 876ed42 Compare February 19, 2024 22:59

tyll requested changes Feb 20, 2024

View reviewed changes

defaults/main.yml Outdated Show resolved Hide resolved

liangwen12year force-pushed the nm_plugin_hotfix branch from 876ed42 to f016efa Compare February 20, 2024 19:46

richm reviewed Feb 20, 2024

View reviewed changes

tasks/main.yml Outdated Show resolved Hide resolved

tyll reviewed Feb 21, 2024

View reviewed changes

README.md Outdated Show resolved Hide resolved

tyll reviewed Feb 21, 2024

View reviewed changes

README.md Outdated Show resolved Hide resolved

tyll reviewed Feb 21, 2024

View reviewed changes

tasks/main.yml Outdated Show resolved Hide resolved

ffmancera requested changes Feb 21, 2024

View reviewed changes

tasks/main.yml Outdated Show resolved Hide resolved

liangwen12year force-pushed the nm_plugin_hotfix branch 2 times, most recently from d8d2e33 to 2160db2 Compare February 22, 2024 02:30

richm reviewed Mar 12, 2024

View reviewed changes

tasks/main.yml Show resolved Hide resolved

richm reviewed Mar 12, 2024

View reviewed changes

tasks/main.yml Show resolved Hide resolved

liangwen12year force-pushed the nm_plugin_hotfix branch from f05c935 to c430ee3 Compare March 13, 2024 00:22

tyll reviewed Mar 13, 2024

View reviewed changes

tasks/main.yml Show resolved Hide resolved

tyll reviewed Mar 13, 2024

View reviewed changes

tests/ensure_provider_tests.py Outdated Show resolved Hide resolved

tyll reviewed Mar 13, 2024

View reviewed changes

tests/playbooks/tests_wireless_and_network_restart.yml Outdated Show resolved Hide resolved

tyll reviewed Mar 13, 2024

View reviewed changes

tests/tasks/remove_and_assert_package_absent.yml Outdated Show resolved Hide resolved

liangwen12year force-pushed the nm_plugin_hotfix branch 2 times, most recently from 256c06e to 2ba3dcc Compare March 13, 2024 15:14

tyll mentioned this pull request Mar 14, 2024

add better error message in case NetworkManager-wifi is not installed and network_allow_restart is not specified. #244

Closed

tyll reviewed Mar 14, 2024

View reviewed changes

tests/ensure_provider_tests.py Outdated Show resolved Hide resolved

liangwen12year force-pushed the nm_plugin_hotfix branch from 2ba3dcc to e8fe9f2 Compare March 14, 2024 18:21

docs: Update the description about the variable network_allow_restart

e7399a5

Signed-off-by: Wen Liang <[email protected]>

liangwen12year force-pushed the nm_plugin_hotfix branch from e8fe9f2 to d33a681 Compare March 15, 2024 02:13

liangwen12year requested review from tyll and richm March 15, 2024 02:18

liangwen12year added 2 commits March 15, 2024 10:51

tests: Consent to restart network when specifying wireless or team co…

0fca710

…nnections Signed-off-by: Wen Liang <[email protected]>

liangwen12year force-pushed the nm_plugin_hotfix branch from d33a681 to 0fca710 Compare March 15, 2024 14:51

liangwen12year commented Mar 15, 2024

View reviewed changes

tyll approved these changes Mar 15, 2024

View reviewed changes

richm approved these changes Mar 15, 2024

View reviewed changes

liangwen12year merged commit 44f937d into linux-system-roles:main Mar 15, 2024
33 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Allow network to restart when wireless or team connection is specified #675

fix: Allow network to restart when wireless or team connection is specified #675

liangwen12year commented Feb 18, 2024 •

edited

Loading

liangwen12year commented Feb 18, 2024

codecov bot commented Feb 18, 2024 •

edited

Loading

liangwen12year commented Feb 18, 2024

liangwen12year commented Feb 18, 2024

tyll commented Feb 19, 2024

tyll left a comment

liangwen12year commented Feb 19, 2024

liangwen12year commented Feb 19, 2024

tyll left a comment

liangwen12year commented Feb 20, 2024

liangwen12year commented Feb 22, 2024

liangwen12year commented Mar 12, 2024

liangwen12year commented Mar 12, 2024

tyll Mar 13, 2024

liangwen12year Mar 13, 2024

richm Mar 13, 2024

tyll Mar 13, 2024

richm Mar 13, 2024

richm Mar 14, 2024

tyll Mar 14, 2024

liangwen12year Mar 15, 2024

tyll Mar 15, 2024

liangwen12year Mar 15, 2024

liangwen12year commented Mar 14, 2024

liangwen12year commented Mar 15, 2024

liangwen12year Mar 15, 2024

tyll Mar 15, 2024

liangwen12year Mar 15, 2024

liangwen12year commented Mar 15, 2024

liangwen12year commented Mar 15, 2024

tyll left a comment

fix: Allow network to restart when wireless or team connection is specified #675

fix: Allow network to restart when wireless or team connection is specified #675

Conversation

liangwen12year commented Feb 18, 2024 • edited Loading

liangwen12year commented Feb 18, 2024

codecov bot commented Feb 18, 2024 • edited Loading

Codecov Report

liangwen12year commented Feb 18, 2024

liangwen12year commented Feb 18, 2024

tyll commented Feb 19, 2024

tyll left a comment

Choose a reason for hiding this comment

liangwen12year commented Feb 19, 2024

liangwen12year commented Feb 19, 2024

tyll left a comment

Choose a reason for hiding this comment

liangwen12year commented Feb 20, 2024

liangwen12year commented Feb 22, 2024

liangwen12year commented Mar 12, 2024

liangwen12year commented Mar 12, 2024

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

liangwen12year commented Mar 14, 2024

liangwen12year commented Mar 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liangwen12year commented Mar 15, 2024

liangwen12year commented Mar 15, 2024

tyll left a comment

Choose a reason for hiding this comment

liangwen12year commented Feb 18, 2024 •

edited

Loading

codecov bot commented Feb 18, 2024 •

edited

Loading