linux-system-roles / network

An ansible role to configure networking
https://linux-system-roles.github.io/network/
BSD 3-Clause "New" or "Revised" License
238 stars 106 forks source link

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

Closed liangwen12year closed 8 months ago

liangwen12year commented 9 months ago

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 commented 9 months ago

[citest]

codecov[bot] commented 9 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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

Additional details and impacted files ```diff @@ 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](https://app.codecov.io/gh/linux-system-roles/network/pull/675/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=linux-system-roles) | Coverage Δ | | |---|---|---| | [sanity](https://app.codecov.io/gh/linux-system-roles/network/pull/675/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=linux-system-roles) | `20.50% <ø> (ø)` | | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=linux-system-roles#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

liangwen12year commented 9 months ago

[citest]

liangwen12year commented 9 months ago

[citest]

tyll commented 9 months ago

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

liangwen12year commented 9 months ago

[citest]

liangwen12year commented 9 months ago

[citest]

liangwen12year commented 9 months ago

[citest]

liangwen12year commented 9 months ago

[citest]

liangwen12year commented 9 months ago

Since team is deprecated in RHEL-9, I think that it is not appropriate to test team connection in this PR, I will switch to wifi instead tomorrow.

liangwen12year commented 9 months ago

[citest]

liangwen12year commented 9 months ago

[citest]

liangwen12year commented 9 months ago

@richm , about the test failure in c9s container test, my original idea is to remove the conflicting NM package, and then check again if the updates for the network_package are available, then install the new NM package, but that would necessarily need to restart NM regardless of user's consent. It is bad because restarting NM can result in connectivity loss and therefore the role does not allow it without the consent. The problem here is that the local system has the NetworkManager package version NetworkManager-1:1.46.0-1.el9.x86_64 installed, but the baseos repo has the version NetworkManager-team-1:1.45.91-1.el9.x86_64 and NetworkManager-1:1.45.91-1.el9.x86_64. I honestly do not know why this situation would happen, I can imagine a way to do about it:

  1. Add a rescue task in main.yml explaining better the error message about the package conflicts
TASK [linux-system-roles.network : Check if updates for network packages are available through the DNF package manager due to wireless or team interfaces] ***
fatal: [localhost]: FAILED! => {"changed": false, "failures": [], "msg": "Depsolve Error occured: \n Problem: cannot install both NetworkManager-1:1.45.91-1.el9.x86_64 from baseos and NetworkManager-1:1.46.0-1.el9.x86_64 from @System\n  - package NetworkManager-team-1:1.45.91-1.el9.x86_64 from baseos requires NetworkManager(x86-64) = 1:1.45.91-1.el9, but none of the providers can be installed\n  - cannot install the best update candidate for package NetworkManager-1:1.46.0-1.el9.x86_64\n  - cannot install the best candidate for the job", "rc": 1, "results": []}
liangwen12year commented 9 months ago

@richm , about the test failure in c9s container test, my original idea is to remove the conflicting NM package, and then check again if the updates for the network_package are available, then install the new NM package, but that would necessarily need to restart NM regardless of user's consent. It is bad because restarting NM can result in connectivity loss and therefore the role does not allow it without the consent. The problem here is that the local system has the NetworkManager package version NetworkManager-1:1.46.0-1.el9.x86_64 installed, but the baseos repo has the version NetworkManager-team-1:1.45.91-1.el9.x86_64 and NetworkManager-1:1.45.91-1.el9.x86_64. I honestly do not know why this situation would happen, I can imagine a way to do about it:

  1. Add a rescue task in main.yml explaining better the error message about the package conflicts
TASK [linux-system-roles.network : Check if updates for network packages are available through the DNF package manager due to wireless or team interfaces] ***
fatal: [localhost]: FAILED! => {"changed": false, "failures": [], "msg": "Depsolve Error occured: \n Problem: cannot install both NetworkManager-1:1.45.91-1.el9.x86_64 from baseos and NetworkManager-1:1.46.0-1.el9.x86_64 from @System\n  - package NetworkManager-team-1:1.45.91-1.el9.x86_64 from baseos requires NetworkManager(x86-64) = 1:1.45.91-1.el9, but none of the providers can be installed\n  - cannot install the best update candidate for package NetworkManager-1:1.46.0-1.el9.x86_64\n  - cannot install the best candidate for the job", "rc": 1, "results": []}

Ok, after rebuilding the c9s container image in quay, the github c9s container integration test passed.

liangwen12year commented 9 months ago

[citest]

tyll commented 9 months ago

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

liangwen12year commented 8 months ago

[citest]

liangwen12year commented 8 months ago

[citest]

liangwen12year commented 8 months ago

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

I added it.

liangwen12year commented 8 months ago

[citest]

liangwen12year commented 8 months ago

[citest]

liangwen12year commented 8 months ago

[citest]

liangwen12year commented 8 months ago

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

liangwen12year commented 8 months ago

[citest]