ifupdown: some tweaks to networking.service

[Martin Pitt]

Package: ifupdown
Version: 0.8.1
Tags: patch

Hello Guus,

many thanks for adding a native networking.service! Michael and I
found some corrections and tweaks to be made, and we would also like
to remove the /lib/systemd/system/networking.service.d/systemd.conf
drop-in.

The attached patch to networking.service changes the following:

 - Drop Wants=network-pre.target. This is a passive target which
   should be pulled in by e. g. firewalls, not network management
   service.  See man systemd.special(7).

 - Add Wants=network.target instead. It's passive target too, and as
   networking.service is a provider it needs to pull it in.

 - Add Before=network.target. This is important during shutdown to
   avoid stopping networking.service before any consumer of it (e. g.
   NFS, apache, etc.) -- otherwise shutting down these might hang due
   to cutting the network too early.

The above are similar to systemd-networkd.service or
NetworkManager.service.

 - Add After=local-fs.target. Depending on a read-only root partition
   might not be enough, as there might be actions in /e/n/i which
   require a writable root, or other mounted partitions. The current
   sysvinit file has "Required-Start: $local_fs" and we should
   probably keep that until we are really sure that we can drop it.

 - Add After=apparmor.service. Apparmor needs to run very early as it
   commonly installs profiles for network-facing software like
   dhcp-client. Note that this is a no-op when apparmor isn't being
   used, as there is no Wants= (i. e. just ordering, no dependency).

 - Drop systemd-udev-settle.service. Please let's not do this
   unconditionally.  udev-settle is a really bad workaround for software which does not
   know about hotplugging, it's racy, might take a long time
   unnecessarily, and isn't required at all when one does not actually
   use ifupdown.

   The init.d script does this conditionally at least:

	if [ -n "$(ifquery --list --exclude=lo)" ] || [ -n "$(ifquery --list --allow=hotplug)" ]; then
		udevadm settle || true

   Although this looks overzealous to me -- I thought the intention
   was to make sure that all "static" interfaces are present before
   you start ifup'ing them, i. e. for the "auto" class. We
   specifically want "allow-hotplug" devices to *not* block the boot,
   only for the "auto" ones, no?

   So if you want to keep the current approach, I suggest adding this
   instead:

   ExecStartPre=-/bin/sh -c '[ -n "$(ifquery --list --exclude=lo)" ] && udevadm settle'

   (which is what the patch does ATM).

   In the medium term I'd be interested in cleaning this up, though.
   IMHO there needs to be a proper ifupdown-wait-online.service which
   just starts the virtual interfaces, let all the real ones be
   handleld by udev hotplug rules, and just waits until all "auto"
   services are up. This avoids the race condition and unnecessary
   blocking on udev settle. But this should be discussed in a separate
   bug, and I'll also send one for moving the "allow-hotplug" handling
   bits into ifupdown (where they fit much better than in the udev
   package).

   But the above just mimics what the sysvinit script does, so it's a
   good iteration. :-)

Thanks for considering,

Martin

-- 
Martin Pitt                        | http://www.piware.de
Ubuntu Developer (www.ubuntu.com)  | Debian Developer  (www.debian.org)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: networking.service.diff
Type: text/x-diff
Size: 878 bytes
Desc: not available
URL: <http://lists.alioth.debian.org/pipermail/pkg-systemd-maintainers/attachments/20151203/3c3b56fa/attachment.diff>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.alioth.debian.org/pipermail/pkg-systemd-maintainers/attachments/20151203/3c3b56fa/attachment.sig>