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

[Simon Fels]

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.

[Martin Pitt (pitti)]

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)]

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)]

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)]

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)]

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

[Martin Pitt (pitti)]

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)]

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

[Simon Fels (morphis)]

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)]

Fixed review comments.

[Martin Pitt (pitti)]

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

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.

diff --git a/src/netplan b/src/netplan
index bdef802..40663c8 100755
--- a/src/netplan
+++ b/src/netplan
@@ -29,6 +29,9 @@ import yaml
 
 path_generate = os.environ.get('NETPLAN_GENERATE_PATH', '/lib/netplan/generate')
 
+NM_SERVICE_NAME = 'NetworkManager.service'
+NM_SNAP_SERVICE_NAME = 'snap.network-manager.networkmanager.service'
+
 
 #
 # helper functions
@@ -67,11 +70,24 @@ def parse_args():
     return args
 
 
+def is_nm_snap_enabled():
+    return subprocess.call(['systemctl', '--quiet', 'is-enabled', NM_SNAP_SERVICE_NAME], stderr=subprocess.DEVNULL) == 0
+
+
+def nmcli(args):
+    binary_name = 'nmcli'
+
+    if is_nm_snap_enabled():
+        binary_name = 'network-manager.nmcli'
+
+    subprocess.check_call([binary_name] + args, stderr=subprocess.DEVNULL)
+
+
 def nm_running():  # pragma: nocover (covered in autopkgtest)
     '''Check if NetworkManager is running'''
 
     try:
-        subprocess.check_call(['nmcli', 'general'], stdout=subprocess.DEVNULL, stderr=subprocess.STDOUT)
+        nmcli(['general'])
         return True
     except (OSError, subprocess.SubprocessError):
         return False
@@ -236,6 +252,17 @@ def command_generate():
     os.execv(argv[0], argv)
 
 
+def systemctl_network_manager(action):
+    service_name = NM_SERVICE_NAME
+
+    # If the network-manager snap is installed use its service
+    # name rather than the one of the deb packaged NetworkManager
+    if is_nm_snap_enabled():
+        service_name = NM_SNAP_SERVICE_NAME
+
+    subprocess.check_call(['systemctl', action, '--no-block', service_name])
+
+
 def command_apply():  # pragma: nocover (covered in autopkgtest)
     if subprocess.call([path_generate]) != 0:
         sys.exit(1)
@@ -258,8 +285,12 @@ def command_apply():  # pragma: nocover (covered in autopkgtest)
             # restarting NM does not cause new config to be applied, need to shut down devices first
             for device in devices:
                 # ignore failures here -- some/many devices might not be managed by NM
-                subprocess.call(['nmcli', 'device', 'disconnect', device], stderr=subprocess.DEVNULL)
-            subprocess.check_call(['systemctl', 'stop', '--no-block', 'NetworkManager.service'])
+                try:
+                    nmcli(['device', 'disconnect', device])
+                except CalledProcessError:
+                    pass
+
+            systemctl_network_manager('stop')
     else:
         logging.debug('no netplan generated NM configuration exists')
 
@@ -278,7 +309,7 @@ def command_apply():  # pragma: nocover (covered in autopkgtest)
         subprocess.check_call(['systemctl', 'start', '--no-block', 'systemd-networkd.service'] +
                               [os.path.basename(f) for f in glob('/run/systemd/system/*.wants/netplan-wpa@*.service')])
     if restart_nm:
-        subprocess.call(['systemctl', 'start', '--no-block', 'NetworkManager.service'])
+        systemctl_network_manager('start')
 
 
 def command_ifupdown_migrate():