Re: [libvirt] [PATCH 19/41] remote: introduce virtproxyd daemon to handle IP connectivity
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=-@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 :|