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

Fix wifi nmcli backup/restore /delete on 24.04 Desktop (BugFix) #1324

Merged
merged 15 commits into from
Sep 13, 2024

Conversation

seankingyang
Copy link
Contributor

@seankingyang seankingyang commented Jul 8, 2024

Description

NetworkManager behavior in 24.04 is a little different with former.

It will using the netplan to dispatch the networking backend daemon, and the NetworkManager config file (*.nmconnection) is not a permanent file (will not under the /etc/NetworkManager/system-connections), but the temporary file (under /run/Networkmanger/system-connections).

The permanent file will only exist in netplan (/etc/netplan).

Based on the these reason, there are some part need to modify:

  1. Backup and restore to the original path, not always copy to the fixed path.
  2. Not use the nmcli delete (will call the dbus api), it will also delete the netplan config file, and it will make the networking can't recover anymore
  3. Using the nmcli np/down to control which SSID we need to connect and disconnect
  4. Checking the connecting SSID is our test ap when doing the ping test.

Modify part

wifi_nmcli_backup.py

  • func: save_connections
    • Retrieve the complete file paths of all *.nmconnection files. Then, establish a respective full path directory within $PLAINBOX_SESSION_SHARE/stored-system-connections/ and transfer the files into it.
  • func: restore_connections
    • Relocate the *.nmconnection files located in $PLAINBOX_SESSION_SHARE/stored-system-connections/*/**/ back to their initial paths.

wifi_nmcli_test.py

  • func: _get_nm_wireless_connections
    • Isolated from the original function to enable its use by other functions.
  • func: get_nm_activate_connection
    • This function serves to identify activated SSIDs.
    • It also verifies the connection status of the test AP.
  • func: turn_up_connection
    • Utilizes nmcli up command to select the desired SSID for connection establishment.
    • The command executes only when the target SSID is inactive, ensuring error-free activation even if it's already activated.
  • func: turn_down_nm_connections
    • Replaces original func:cleanup_nm_connections with nmcli delete to clear all connections. This method, while not foolproof, doesn't allow recovery of original connections after testing (even with wifi_nmcli_backup.py restore).
    • Employs nmcli down to deactivate all connections. NetworkManager can function without any active SSID, allowing access even when those APs are available.
  • func: delete_test_ap_ssid_connection
    • Selectively removes only the test AP's ssid post-testing. Executing this at test initiation ensures TEST_CONN isn't within NetworkManager configuration.
  • func: list_aps
    • Introduced essid parameter for filtering expected connection APs during open/secured connectivity testing.
    • Enhances clarity in output presentation during open/secured connectivity assessments.
  • func: show_aps
    • Separate functionality from func:list_aps; specifically prints aps_dict.
  • func: wait_for_connected
    • Added essid parameter for validation of connecting ap as our anticipated testing ap.
  • func: open_connection and func: secured_connection
    • Minor adjustments made.
  • func: parser_args
    • Isolated from original main function setup.
  • func: main
    • Independently structured from initial main functionality
    • Several minor modifications applied
    • Ensures deletion of TEST_CONN before subsequent tests and upon completion
    • Activates or restores the original connection's SSID/AP post-testing

Resolved issues

OEMQA-4582 NetworkManager test case impact in 24.04 Desktop

Documentation

Tests

Copy link

codecov bot commented Jul 8, 2024

Codecov Report

Attention: Patch coverage is 92.42424% with 10 lines in your changes missing coverage. Please review.

Project coverage is 46.77%. Comparing base (b92edc5) to head (2753641).
Report is 141 commits behind head on main.

Files with missing lines Patch % Lines
providers/base/bin/wifi_nmcli_test.py 91.52% 10 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1324      +/-   ##
==========================================
+ Coverage   45.72%   46.77%   +1.04%     
==========================================
  Files         367      367              
  Lines       39134    39317     +183     
  Branches     6618     6648      +30     
==========================================
+ Hits        17894    18390     +496     
+ Misses      20565    20236     -329     
- Partials      675      691      +16     
Flag Coverage Δ
provider-base 22.84% <92.42%> (+2.92%) ⬆️

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.

@seankingyang seankingyang force-pushed the Fix_wifi_nmcli branch 7 times, most recently from 1f7061a to 718d9a7 Compare July 9, 2024 03:28
@seankingyang seankingyang marked this pull request as ready for review July 9, 2024 03:30
@fernando79513 fernando79513 self-assigned this Jul 16, 2024
@seankingyang
Copy link
Contributor Author

Rebase

Copy link
Collaborator

@fernando79513 fernando79513 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GJ! The functionality seems fine to me.
I added some comments to make the code more simple and clear and pointed some issues with the tests.

Also, as in the other PR, it would be great if you could add some of the information that's on the PR description (very nice, BTW) as comments on the code.

providers/base/bin/wifi_nmcli_backup.py Outdated Show resolved Hide resolved
providers/base/bin/wifi_nmcli_backup.py Outdated Show resolved Hide resolved
providers/base/tests/test_wifi_nmcli_backup.py Outdated Show resolved Hide resolved
providers/base/bin/wifi_nmcli_test.py Show resolved Hide resolved
providers/base/bin/wifi_nmcli_test.py Outdated Show resolved Hide resolved
providers/base/tests/test_wifi_nmcli_test.py Show resolved Hide resolved
providers/base/tests/test_wifi_nmcli_test.py Show resolved Hide resolved
providers/base/tests/test_wifi_nmcli_test.py Outdated Show resolved Hide resolved
providers/base/tests/test_wifi_nmcli_test.py Outdated Show resolved Hide resolved
providers/base/tests/test_wifi_nmcli_test.py Outdated Show resolved Hide resolved
@fernando79513 fernando79513 added the waiting-for-changes The review has been completed but the PR is waiting for changes from the author label Aug 30, 2024
seankingyang and others added 8 commits September 6, 2024 10:58
Simplify the unit test

Co-authored-by: Fernando Bravo <39527354+fernando79513@users.noreply.github.com>
Fix typo

Co-authored-by: Fernando Bravo <39527354+fernando79513@users.noreply.github.com>
Simplify the unit test

Co-authored-by: Fernando Bravo <39527354+fernando79513@users.noreply.github.com>
Simplify the unit test

Co-authored-by: Fernando Bravo <39527354+fernando79513@users.noreply.github.com>
Only catch the sp.CalledProcessError

Co-authored-by: Fernando Bravo <39527354+fernando79513@users.noreply.github.com>
@seankingyang seankingyang removed the waiting-for-changes The review has been completed but the PR is waiting for changes from the author label Sep 6, 2024
@seankingyang
Copy link
Contributor Author

@fernando79513 please review these changes~~

Copy link
Collaborator

@fernando79513 fernando79513 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the changes. I just added some small issues and a couple of comments.

providers/base/tests/test_wifi_nmcli_test.py Outdated Show resolved Hide resolved
providers/base/tests/test_wifi_nmcli_test.py Outdated Show resolved Hide resolved
providers/base/tests/test_wifi_nmcli_test.py Outdated Show resolved Hide resolved
providers/base/bin/wifi_nmcli_test.py Show resolved Hide resolved
providers/base/bin/wifi_nmcli_backup.py Outdated Show resolved Hide resolved
@seankingyang seankingyang force-pushed the Fix_wifi_nmcli branch 2 times, most recently from dfdeccf to ccb417e Compare September 13, 2024 02:19
@seankingyang
Copy link
Contributor Author

@fernando79513, please review the changes ~~

@fernando79513 fernando79513 merged commit bf03f1a into main Sep 13, 2024
41 checks passed
@fernando79513 fernando79513 deleted the Fix_wifi_nmcli branch September 13, 2024 11:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants