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