Merge ~morphis/netplan/+git/netplan:nm-snap-support into netplan:master

Proposed by Simon Fels on 2016-09-23
Status: Rejected
Rejected by: Martin Pitt on 2016-09-27
Proposed branch: ~morphis/netplan/+git/netplan:nm-snap-support
Merge into: netplan:master
Diff against target: 82 lines (+35/-4)
1 file modified
src/netplan (+35/-4)
Reviewer Review Type Date Requested Status
Martin Pitt 2016-09-23 Disapprove on 2016-09-26
Review via email: mp+306607@code.launchpad.net

Description of the Change

Add support for the network-manager snap

If the network-manager snap is installed we should restart its service unit rather than the one of the debian package to get it to read its configuration files again.

To post a comment you must log in.
Martin Pitt (pitti) wrote :

Having a different name for the well-known NetworkManager.service is ugly IMHO -- you are going to need to sprinkle this alternative name into every piece of software that tries to talk to it. And you can't possibly co-install it with the .deb as they would clash on D-Bus names.

So I'd really prefer to avoid patches like this, as they are super-ugly. I acknowledge that snapd presumably wants a canonical name like "snap.something", but the unit could just have "Alias=NetworkManager.service" to make it compatible with the deb.

I also have some inline comments, but they are hopefully moot with the Alias.

review: Needs Fixing
Simon Fels (morphis) wrote :

The naming of the service file unit or the Alias is nothing we can control from the snap itself as the service unit is created from snapd which creates a unique name for it to avoid clashes between multiple snaps. Created https://bugs.launchpad.net/snappy/+bug/1626986 for this.

For the short term we need to go with such a patch as I can't say if this will be accepted as a feature for snapd or not.

Simon Fels (morphis) wrote :

Reworked things a bit as we need to use the nmcli binary from the snap too. With that it makes things even harder to just abstract the snap use through an Alias= line in the systemd service unit as we also need to use the right nmcli binary which is "network-manager.nmcli" when it comes from the snap and just "nmcli" when its on a Ubuntu classic system.

Martin Pitt (pitti) :
review: Disapprove
Martin Pitt (pitti) wrote :

I'm sorry, but this is just broken. That binary name is being used way more widely than the .service name. If you want to credibly provide an OS component as a snap, then this also must provide the expected API, CLI, etc. to the user. Note that it already is not possible to install this on touch, or a "personal" snappy device etc. which has network-manager.deb installed. I. e. it's going to clash anyway, so you can as well provide the proper executable names.

review: Disapprove
Simon Fels (morphis) wrote :

@Martin: Sure, and we would do if snapd would allow this :-)

Martin Pitt (pitti) wrote :

Right, but then we need to teach snapd about it -- we have the source :-)

Anyway, as discussed on IRC I will add it to the xenial backport to unblock this -- but this again now puts *me* in the position of having to get rid of it instead of the actually responsible people.

Martin Pitt (pitti) wrote :

There's a bug and a style issue, comments inline.

Simon Fels (morphis) wrote :

Sure, we have the source but given all the existing work for GA and the commercial projects we have ongoing and need to finish in time we need to do this somewhere rather than not doing it which would put things at risk.

I agree this needs to be fixed in a better way. There will be AFAIK a session on the UES sprint to discuss providing commands like nmcli to the system from snaps. That would be one thing we need to solve for this.

Simon Fels (morphis) wrote :

Fixed review comments.

Martin Pitt (pitti) wrote :

Two more issues (one even leads to FTBFS), I'll fix them when applying this to the xenial backport. See bug 1627641.

Simon Fels (morphis) :

Unmerged commits

9878fa0... by Simon Fels on 2016-09-23

Add support for network-manager coming from a snap

On real Ubuntu Core systems NetworkManager will always come from a
snap and we need to support this as a valid option here too.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/src/netplan b/src/netplan
2index bdef802..40663c8 100755
3--- a/src/netplan
4+++ b/src/netplan
5@@ -29,6 +29,9 @@ import yaml
6
7 path_generate = os.environ.get('NETPLAN_GENERATE_PATH', '/lib/netplan/generate')
8
9+NM_SERVICE_NAME = 'NetworkManager.service'
10+NM_SNAP_SERVICE_NAME = 'snap.network-manager.networkmanager.service'
11+
12
13 #
14 # helper functions
15@@ -67,11 +70,24 @@ def parse_args():
16 return args
17
18
19+def is_nm_snap_enabled():
20+ return subprocess.call(['systemctl', '--quiet', 'is-enabled', NM_SNAP_SERVICE_NAME], stderr=subprocess.DEVNULL) == 0
21+
22+
23+def nmcli(args):
24+ binary_name = 'nmcli'
25+
26+ if is_nm_snap_enabled():
27+ binary_name = 'network-manager.nmcli'
28+
29+ subprocess.check_call([binary_name] + args, stderr=subprocess.DEVNULL)
30+
31+
32 def nm_running(): # pragma: nocover (covered in autopkgtest)
33 '''Check if NetworkManager is running'''
34
35 try:
36- subprocess.check_call(['nmcli', 'general'], stdout=subprocess.DEVNULL, stderr=subprocess.STDOUT)
37+ nmcli(['general'])
38 return True
39 except (OSError, subprocess.SubprocessError):
40 return False
41@@ -236,6 +252,17 @@ def command_generate():
42 os.execv(argv[0], argv)
43
44
45+def systemctl_network_manager(action):
46+ service_name = NM_SERVICE_NAME
47+
48+ # If the network-manager snap is installed use its service
49+ # name rather than the one of the deb packaged NetworkManager
50+ if is_nm_snap_enabled():
51+ service_name = NM_SNAP_SERVICE_NAME
52+
53+ subprocess.check_call(['systemctl', action, '--no-block', service_name])
54+
55+
56 def command_apply(): # pragma: nocover (covered in autopkgtest)
57 if subprocess.call([path_generate]) != 0:
58 sys.exit(1)
59@@ -258,8 +285,12 @@ def command_apply(): # pragma: nocover (covered in autopkgtest)
60 # restarting NM does not cause new config to be applied, need to shut down devices first
61 for device in devices:
62 # ignore failures here -- some/many devices might not be managed by NM
63- subprocess.call(['nmcli', 'device', 'disconnect', device], stderr=subprocess.DEVNULL)
64- subprocess.check_call(['systemctl', 'stop', '--no-block', 'NetworkManager.service'])
65+ try:
66+ nmcli(['device', 'disconnect', device])
67+ except CalledProcessError:
68+ pass
69+
70+ systemctl_network_manager('stop')
71 else:
72 logging.debug('no netplan generated NM configuration exists')
73
74@@ -278,7 +309,7 @@ def command_apply(): # pragma: nocover (covered in autopkgtest)
75 subprocess.check_call(['systemctl', 'start', '--no-block', 'systemd-networkd.service'] +
76 [os.path.basename(f) for f in glob('/run/systemd/system/*.wants/netplan-wpa@*.service')])
77 if restart_nm:
78- subprocess.call(['systemctl', 'start', '--no-block', 'NetworkManager.service'])
79+ systemctl_network_manager('start')
80
81
82 def command_ifupdown_migrate():

Subscribers

People subscribed via source and target branches