[libvirt] [PATCH 19/41] remote: introduce virtproxyd daemon to handle IP connectivity

[Daniel P. Berrangé]

On Sun, Jul 28, 2019 at 04:42:52PM +0200, Andrea Bolognani wrote:
> On Tue, 2019-07-23 at 17:02 +0100, Daniel P. Berrangé wrote:
> [...]
> >  - We can make virtproxyd and the virtXXXd per-driver daemons all
> >    have "Conflicts: libvirtd.service" in their systemd unit files.
> >    This will guarantee that libvirtd is never started at the same
> >    time, as this would result in two daemons running the same driver.
> >    Fortunately drivers use locking to protect themselves, but it is
> >    better to avoid starting a daemon we know will conflict.
> 
> I feel like this will need to be tested extensively to make sure
> we're always doing the right thing, including on non-systemd hosts.

Testing is quite easy - just try to start the two units and make
sure only one ends up running.  Similarly for non-systemd hosts,
start both daemons & see that only one succeeds - the others
fail with lock conflict.


> > +++ b/src/remote/virtproxyd.service.in
> > @@ -0,0 +1,24 @@
> > +[Unit]
> > +Description=Virtualization daemon
> > +Conflicts=libvirtd.service
> > +Requires=virtproxyd.socket
> > +Requires=virtproxyd-ro.socket
> > +Requires=virtproxyd-admin.socket
> > +After=network.target
> > +After=dbus.service
> > +After=apparmor.service
> > +After=local-fs.target
> > +After=remote-fs.target
> > +Documentation=man:libvirtd(8)
> > +Documentation=https://libvirt.org
> 
> There are a few non-obvious changes between libvirtd.service.in and
> this file:
> 
>   -Requires=virtlogd.socket
>   -Requires=virtlockd.socket
>   -Wants=systemd-machined.service
>   -Before=libvirt-guests.service
>   -After=iscsid.service
>   -After=systemd-logind.service
>   -After=systemd-machined.service
> 
> I can see why we'd move the relationships with iscsid and virtlockd
> to virtstoraged, except looking ahead to patch 23 I see you haven't
> actually done that; either way, I'm not so convinced about the
> remaining changes. Care to explain the rationale behind them?

virtproxdy contains no drivers, so it doesn't need to depend
on any of these services.

virtdstoraged/qemud/lxcd  should have gained some of these though.

> 
> > +[Service]
> > +Type=notify
> > +ExecStart=@sbindir@/virtproxyd --timeout 120
> > +ExecReload=/bin/kill -HUP $MAINPID
> > +Restart=on-failure
> 
> More changes in this section:
> 
>   -EnvironmentFile=- at sysconfdir@/sysconfig/libvirtd
>   -KillMode=process
>   -LimitNOFILE=8192
>   -TasksMax=32768
> 
> EnvironmentFile is clearly no longer needed, while both LimitNOFILE
> and TasksMax probably belong to the hypervisor-specific daemons, but
> I'm unclear on why KillMode was changed.

The systemd default is fine as we don't need any other processes to
survive shutdown.

> > +[Install]
> > +WantedBy=multi-user.target
> > +Also=virtproxyd.socket
> > +Also=virtproxyd-ro.socket
> 
> Kind of a side note since it's pre-existing, but don't we want to
> list virtproxyd-admin.socket here too?

It is redundant - the deps force virtproxyd-admin.socket to become
enabled regardless.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|