Merge ~slyon/network-manager:netplan-integration into network-manager:ubuntu/master

Proposed by Lukas Märdian
Status: Merged
Merged at revision: bcd43413ecea3493bfbcf091b95508a4120ba7d3
Proposed branch: ~slyon/network-manager:netplan-integration
Merge into: network-manager:ubuntu/master
Diff against target: 1558 lines (+1454/-1)
11 files modified
debian/changelog (+14/-0)
debian/control (+4/-0)
debian/network-manager.postinst (+33/-0)
debian/network-manager.preinst (+18/-0)
debian/patches/netplan/0001-netplan-Adopt-buildsystems-for-Netplan-integration.patch (+134/-0)
debian/patches/netplan/0002-netplan-make-use-of-libnetplan-for-YAML-backend.patch (+525/-0)
debian/patches/series (+2/-0)
debian/rules (+2/-1)
debian/tests/control (+4/-0)
debian/tests/nm.py (+1/-0)
debian/tests/nm_netplan.py (+717/-0)
Reviewer Review Type Date Requested Status
Sebastien Bacher Approve
Danilo Egea Gondolfo (community) Approve
Lukas Märdian Abstain
Review via email: mp+423735@code.staging.launchpad.net

Description of the change

Deploying the NetworkManager-Netplan integration (a.k.a "Netplan everywhere") on Ubuntu Desktop by default.

This depends on libnetplan >= v0.106 and we cannot depend on libnetplan on i386, so we're dropping the patch to the NetworkManager daemon binary on i386, using quilt (that binary isn't built on i386 anyway).

On installation of the package any existing configuration from /etc/NetworkManager/system-connections will be backed up in /root/NetworkManaker.bak

On updade of the package, any existing configuration from /etc/NetworkManager/system-connections will be migrated to /etc/netplan and translated into corresponding YAML files. Missing settings will be handled usinng the "passthrough" and "nm-devices" fallback mechanism.

Netplan patches are managed via git-buildpackage patch-queue (Using "Gbp-Pq: Topic netplan")

To post a comment you must log in.
Revision history for this message
Olivier Gayot (ogayot) wrote :

Thanks! A few comments inline.

Revision history for this message
Danilo Egea Gondolfo (danilogondolfo) wrote :

Left one comment inline about the migration script.

review: Needs Fixing
Revision history for this message
Sebastien Bacher (seb128) wrote :

Confirming the issue raised by Danilo, the script failed to migrate several connections using a ca-cert and pointing to a filename stored in directory with a 'é' in its name

it also displayed some warnings (translated from french so the wording might not be the correct english one)

> Warning: there is another connection with the name '...'. Refer to the connection using its uuid '...'

review: Needs Fixing
Revision history for this message
Sebastien Bacher (seb128) wrote :

Unsure if that's specific to the change there or a new mantic issue but the autopkgtest against a ppa testbuild failed

https://autopkgtest.ubuntu.com/results/autopkgtest-mantic-ubuntu-desktop-transitions/mantic/amd64/n/network-manager/20230508_144934_e64d7@/log.gz

'Error: failed to reload connections: GDBus.Error:org.freedesktop.DBus.Error.UnknownMethod: Object does not exist at path “/org/freedesktop/NetworkManager/Settings”.
dpkg: error processing package network-manager (--configure):
 installed network-manager package post-installation script subprocess returned error exit status 1'

Revision history for this message
Lukas Märdian (slyon) wrote :

Thanks!

I've fixed the utf-8 (non-ASCII keyfiles) issue reported by @seb128 and @danilogondolfo as suggested by Danilo (i.e. checking for the "text/plain" mime type instead).

Thanks Sebastien for the "Error: failed to reload connections: GDBus.Error:org.freedesktop.DBus.Error.UnknownMethod" error, that's a good catch! It didn't happen on my Lunar autopkgtests, but I could reproduce it on Mantic, too. Seems to be some kind of race condition when debhelper tries to re-start NetworkManager.service after upgrade and then the postinst script is calling into "nmcli", while NM is not yet fully up. I've added a small "nm-online -s" line in there to make it wait for NM, which now passes the autopkgtests on Mantic again:
```
autopkgtest [15:07:36]: @@@@@@@@@@@@@@@@@@@@ summary
wpa-dhclient PASS
nm.py PASS
killswitches-no-urfkill PASS
urfkill-integration PASS
nm_netplan.py PASS
qemu-system-x86_64: terminating on signal 15 from pid 1944756 (/usr/bin/python3)
```

For the "Warning: there is another connection with the name '...'. Refer to the connection using its uuid '...'" message, I don't really know where it comes from, as we're only modifying connections through nmcli, using their UUID identifiers... Could you try to provide a reproducer for that?

PTAL.

Revision history for this message
Sebastien Bacher (seb128) wrote :

Thanks for the tweaks, I restored the connections from the backup dir, tried to the new revision and now hit

+ echo Migrating Ziggo (03c8f2a7-268d-4765-b626-efcc02dd686c) to /etc/netplan
Migrating Ziggo (03c8f2a7-268d-4765-b626-efcc02dd686c) to /etc/netplan
+ nmcli con mod 03c8f2a7-268d-4765-b626-efcc02dd686c con-name Ziggo.NETPLAN_MIGRATE
Erreur : la modification de la connexion « Ziggo.NETPLAN_MIGRATE » a échoué : Remote peer disconnected
dpkg: erreur de traitement du paquet network-manager (--install) :
 le sous-processus paquet network-manager script post-installation installé a renvoyé un état de sortie d'erreur 1

there is a NetworkManager crash report from that time in /var/crash, it hit the
nms-keyfile-writer.c assert on l538 which is part of this changeset, the journal includes

> <error> [1683703791.7570] BUG: the profile cannot be stored in keyfile format without becoming unusable: invalid connection: 802-1x.identity: la propriété est manquante
and
> <warn> [1683703795.8560] keyfile: load: "/run/NetworkManager/system-connections/netplan-NM-03c8f2a7-268d-4765-b626-efcc02dd686c-Ziggo.nmconnection": failed to load connection: invalid connection: 802-1x.identity: la propriété est manquante

I'm sending the corresponding file via email

So it seems we have a bug there that n-m is hitting an assertion, also it feels wrong than failing to migrate one connection is failing the package installation and letting the packaging system in a state where most users will not be able to recover from easily

review: Needs Fixing
Revision history for this message
Lukas Märdian (slyon) wrote :

Thank you very much for the valuable feedback! I've rebased the branch to properly include the current fixes and moved forward to a v4 Netplan-integration patch, which makes use of "#if WITH_NETPLAN" inside the NM codebase, to allow "--enable-netplan"/"--disable-netplan" buildflags (enabled by default), so we can easily disable it on i386 and don't need that quilt hack anymore.

Also, thanks for providing the test files by email. I agree we shouldn't fail the installation of the package when the migration of a single connection profile fails, it should just skip that connection and keep it as a keyfile. I'll be working on that!

I'll also investigate that crash, but I think we should not block on that one (once we are able to skip non-migrated connections). I'll try to collect all the information and create a corresponding bug report.

Revision history for this message
Lukas Märdian (slyon) wrote :

So Danilo found the root cause of the "802-1x.identity" issue (crash), which is a bug in libnetplan (Netplan's keyfile parser). This will be handled via in bug #2016625 (details to follow there)

Revision history for this message
Lukas Märdian (slyon) wrote :

I've just pushed another update to the branch, which handles migration failures gracefully:
Before migration it will create a temporary backup of the keyfile in and restore that keyfile backup, should the migration fail.

We're working on the crash (libnetplan bug) separately, as mentioned above. I've pushed the current state of this branch into the "Netplan Everywhere" PPA via https://code.launchpad.net/~canonical-foundations/+recipe/network-manager-netplan-lunar-gu

Let me know what you think!

Revision history for this message
Sebastien Bacher (seb128) wrote :

Thanks, I can confirm that the new changes make the package install without error and that it rollbacks the problematic connection. The autopkgtests issue is also resolve

The warning I mentioned earlier about the exiting name/refer by uid is printing by

> nmcli con mod daffb72f-c980-4ede-95cf-31fb7d24484d con-name Hotspot

There is another connection with the same name. The command is correctly issued with the uid so I think the warning is safe to ignore

I will upload the changes to mantic today unless you would prefer to see the netplan fix landing first?

review: Approve
Revision history for this message
Danilo Egea Gondolfo (danilogondolfo) :
Revision history for this message
Danilo Egea Gondolfo (danilogondolfo) wrote :

I left an inline comment about some code we probably want to remove from the #if #endif block.

Revision history for this message
Lukas Märdian (slyon) wrote :

Thank you for the additional reviews!

@seb128, please go ahead and upload the changes into Mantic, we'll be fixing (lib-)netplan in parallel.

@danilogondolfo, you're right it is NetworkManager code. But in fact we're not patching it out, but rather creating a copy of that block for the Netplan codepath. So we should be safe ignoring that whole block inside #if WITH_NETPLAN ... #endif if the Netplan buildflag is disabled. It is visible in the GitHub diff, see: https://github.com/slyon/NetworkManager/commit/aaa791cb61b24a9416db18f2290f711bc9a8f26e#diff-e46473e1ac3ce2315efbd5fbad59951998e274ca21244f6b9e807f20e67db58aL344

review: Abstain
Revision history for this message
Danilo Egea Gondolfo (danilogondolfo) :
review: Approve
Revision history for this message
Sebastien Bacher (seb128) wrote :

Thanks, merged and uploaded to Ubuntu now!

review: Approve

There was an error fetching revisions from git servers. Please try again in a few minutes. If the problem persists, contact Launchpad support.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches

to all changes: