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

[Andrea Bolognani]

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=- 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.

> +[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