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.
[...]
> After some time we can deprecate use of libvirtd and after some more
> time delete it entirely, leaving us in a pretty world filled with
> prancing unicorns.
Awww!
> The main downside with introducing a new daemon, and with the
> per-driver daemons in general, is figuring out the correct upgrade
> path.
>
> The conservative option is to leave libvirtd running if it was
> an existing installation. Only use the new daemons & virtproxyd
> on completely new installs.
>
> The aggressive option is to disable libvirtd if already running
> and activate all the new daemons.
I vote for the conservative option :)
As an aside, the above basically a master class in how to write a
good commit message. Well done!
[...]
> +++ b/src/remote/Makefile.inc.am
[...]
> +VIRTD_UNIT_VARS = \
> + $(COMMON_UNIT_VARS) \
> + -e 's|[@]deps[@]|Conflicts=$(LIBVIRTD_SOCKET_UNIT_FILES)|g' \
> + $(NULL)
Considering that we only use LIBVIRTD_SOCKET_UNIT_FILES here, I'd
move its definition to this general area.
[...]
> +++ b/src/remote/remote_daemon.c
> @@ -303,11 +303,19 @@ static int daemonErrorLogFilter(virErrorPtr err, int priority)
>
> static int daemonInitialize(void)
> {
> -#ifdef MODULE_NAME
> +#ifndef LIBVIRTD
> +# ifdef MODULE_NAME
> + /* This a dedicated per-driver daemon build */
> if (virDriverLoadModule(MODULE_NAME, MODULE_NAME "Register", true) < 0)
> return -1;
> +# else
> + /* This is virtproxyd which merely proxies to the per-driver
> + * daemons for back compat, and also allows IP connectivity.
> + */
> +# endif
> #else
> - /*
> + /* This is the legacy monolithic libvirtd built with all drivers
> + *
This is exactly the kind of comment I suggested you add in patch 9,
so I guess just move the first and third one to that patch.
[...]
> @@ -893,9 +901,9 @@ daemonUsage(const char *argv0, bool privileged)
> { "-h | --help", N_("Display program help") },
> { "-v | --verbose", N_("Verbose messages") },
> { "-d | --daemon", N_("Run as a daemon & write PID file") },
> -#ifdef ENABLE_IP
> +#if defined(ENABLE_IP) && defined (LIBVIRTD)
Extra whitespace in "defined (LIBVIRTD)".
[...]
> +++ 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?
> +[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.
> +[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?
Overall, the changes look good.
--
Andrea Bolognani / Red Hat / Virtualization
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
