Bug#806949: ifupdown: some tweaks to networking.service

[Guus Sliepen]

On Thu, Dec 03, 2015 at 01:06:01PM +0100, Martin Pitt wrote:

> many thanks for adding a native networking.service! Michael and I
> found some corrections and tweaks to be made,

Thanks for having a look at it. I'm still quite new to SystemD, so any
help there is welcome :)

> and we would also like
> to remove the /lib/systemd/system/networking.service.d/systemd.conf
> drop-in.

Great.

>  - 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.

Ah, that clarifies things for me.

>  - 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.

ifupdown itself only writes to /run/network, but indeed any plugins
might require a writable root.

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

No problem. Are there other such services that might require loading
before ifupdown? SELinux?

>  - 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'

Ok, in that case it might be nice to add --read-environment to ifquery
as well, so EXCLUDE_INTERFACES will be read. Something like this?

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

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

Yes. The only problem though is that it requires the admin to correctly
specify which interfaces are "auto" and which are "hotplug", because
ifupdown cannot really figure that out itself. And there might be some
cases where a virtual interface depends on one or more hotpluggable
ones.

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

Yes. I'll start preparing 0.8.2 :)

-- 
Met vriendelijke groet / with kind regards,
      Guus Sliepen <guus at debian.org>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://alioth-lists.debian.net/pipermail/pkg-systemd-maintainers/attachments/20151203/00dbec9a/attachment-0002.sig>