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