anaconda Use separate main context for nmclient in threads

The PR is a followup of https://github.com/rhinstaller/anaconda/pull/4212 by @t-feng. It incorporates the review comments from the original PR and suggestions by @thom311 (also using very helpful example https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/blob/b3181cfbee0ba3f0cd757b9d9b851e61da3cd000/examples/python/gi/gmaincontext.py)

It should fix https://bugzilla.redhat.com/show_bug.cgi?id=1931389

TODO:

[x] Add tests for sync_call_nm_client
[x] Ask @thom311 for review
[x] Squash

Sep 08 '22 09:09 rvykydal

Hello @rvykydal! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

In the file pyanaconda/modules/network/nm_client.py:

Line 24:1: E402 module level import not at top of file Line 28:1: E402 module level import not at top of file Line 38:1: E402 module level import not at top of file

Comment last updated at 2022-09-13 12:31:36 UTC

Sep 08 '22 09:09 pep8speaks

/kickstart-test --testtype smoke

Sep 08 '22 10:09 rvykydal

/kickstart-test --testtype network

Sep 08 '22 12:09 rvykydal

/kickstart-test --testtype network

Sep 09 '22 10:09 rvykydal

/kickstart-test --testtype smoke

Sep 13 '22 14:09 rvykydal

I've added tests, some minor fixes, and a commit https://github.com/rhinstaller/anaconda/pull/4322/commits/1edacdc6b01a9dbb12d99e72f3fc7608eb9119bc that moves the function to core/glib.py as it is not limited to NM but usable for glib async methods in in general.

Sep 20 '22 12:09 rvykydal

/kickstart-tests --testtype smoke

Sep 20 '22 13:09 rvykydal

/kickstart-tests --testtype network

Sep 20 '22 17:09 rvykydal

@thom311 - Hello Thomas, this is a follow-up of https://github.com/rhinstaller/anaconda/pull/4212. Could you please review it if you find some time ?

Sep 21 '22 06:09 rvykydal

/kickstart-test --testtype smoke

Sep 29 '22 11:09 rvykydal

Overall looks good and an improvement.

There might be one problem still...

I would address those problems later in a separate commit.

Sep 30 '22 09:09 thom311

First thank you for the review @thom311.

Overall looks good and an improvement. There might be one problem still...

I would address those problems later in a separate commit.

Yes, I think we will address them if/when they can become a problem, we went simple for the start to focus on fixing our main issue first and I kind of neglected correct releasing of the resources (already described in the https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/blob/main/examples/python/gi/gmaincontext.py example nicely) in this PR because we run the tasks creating new NM client only during the installation in the (throwaway) installer environment, and only during network module initialization - 2 task / nm client instances are created - before user interaction starts. We don't expect it to be run on regular system repeatedly.

But I think we should document this limitation for future reference.

Oct 05 '22 08:10 rvykydal

There might be one problem still...

I would address those problems later in a separate commit.

https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/1414 will add a helper function to libnm to make it easier to cleanup the NMClient and the GMainContext.

Oct 05 '22 20:10 thom311

/kickstart-test --testtype smoke

Oct 07 '22 10:10 rvykydal

There might be one problem still...

I would address those problems later in a separate commit.

https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/1414 will add a helper function to libnm to make it easier to cleanup the NMClient and the GMainContext.

Thank you, I'll use it.

Oct 11 '22 07:10 rvykydal

Overall looks good and an improvement. There might be one problem still...

I would address those problems later in a separate commit.

https://github.com/rhinstaller/anaconda/pull/4388

Oct 21 '22 12:10 rvykydal