[PATCH v1] remove sysconfig files

Wed Jul 21 10:16:39 UTC 2021

On Tue, Jul 20, 2021 at 07:00:20PM +0200, Olaf Hering wrote:
> sysconfig files are owned by the admin of the host. He has the liberty
> to put anything he wants into these files. This makes it difficult to
> provide different built-in defaults.

s/He has/They have/
s/he wants/they want/

> +++ b/NEWS.rst
> @@ -15,6 +15,16 @@ v7.6.0 (unreleased)
>
>  * **Improvements**
>
> +  * packaging: sysconfig files no longer installed
> +
> +    libvirt used to provide defaults in various /etc/sysconfig/ files, such
> +    as /etc/sysconfig/libvirt. Since these files are owned by the admin, this
> +    made it difficult to change built-in defaults in case such file was
> +    modified by the admin. The built-in defaults are now part of the provided
> +    systemd unit files, such as libvirtd.service. These unit files continue
> +    to parse sysconfig files, in case they are created by the admin and filled
> +    with the desired key=value pairs.

The releae notes should be updated in a separate commit, to make it
possible to backport the functional changes only without running into
unnecessary conflicts.

> +++ b/docs/remote.html.in
> @@ -139,7 +139,7 @@ Blank lines and comments beginning with <code>#</code> are ignored.
>          <td>
>    Listen for secure TLS connections on the public TCP/IP port.
>    Note: it is also necessary to start the server in listening mode by
> -  running it with --listen or editing /etc/sysconfig/libvirtd by uncommenting the LIBVIRTD_ARGS="--listen" line
> +  running it with --listen or editing /etc/sysconfig/libvirtd by adding a LIBVIRTD_ARGS="--listen" line

This should mention the alternative method of configuring the
service, which is adding a systemd unit override for the
Environment=LIBVIRTD_ARGS=... variable.

I wonder if users will get this right? The interactions between the
various ways of configuring the arguments for each daemon have just
gotten more complex, and I can definitely foresee subtle
configuration mistakes happening because of it.

> +++ b/libvirt.spec.in
> @@ -197,6 +197,18 @@
> +%define libvirt_sc_pre() \
> +	for sc in %{?*} ; do \
> +	test -f "%{_sysconfdir}/sysconfig/${sc}.rpmsave" || continue ;\
> +	mv -v "%{_sysconfdir}/sysconfig/${sc}.rpmsave" "%{_sysconfdir}/sysconfig/${sc}.rpmsave.old" ;\
> +	done \
> +	%{nil}
> +%define libvirt_sc_posttrans() \
> +	for sc in %{?*} ; do \
> +	test -f "%{_sysconfdir}/sysconfig/${sc}.rpmsave" || continue ;\
> +	mv -v "%{_sysconfdir}/sysconfig/${sc}.rpmsave" "%{_sysconfdir}/sysconfig/${sc}" ;\
> +	done \
> +	%{nil}

Please confirm whether I understand these correctly.

The idea is that we want existing sysconfig files to be preserved
when the package is updated, but rpm by default will rename them to
.rpmsave once it realizes the corresponding files are gone from the
package.

So in %pre you make sure existing .rpmsave files are moved out of the
way, and then in %posttrans you move the current sysconfig files,
which had been renamed by rpm, them back to their original location.

This will all turn into a no-op when you're upgrading from a package
where the sysconf files have already been gone to a newer version,
right? Any scenario in which the rpm run is interrupted, for example
between %pre and %posttrans, and the state becomes inconsistent?

> -%post daemon
> -%global post_units \\\
> -        virtlockd.socket virtlockd-admin.socket \\\
> -        virtlogd.socket virtlogd-admin.socket \\\
> -        libvirtd.socket libvirtd-ro.socket libvirtd-admin.socket \\\
> -        libvirtd-tcp.socket libvirtd-tls.socket \\\
> -        libvirtd.service \\\
> -        libvirt-guests.service
> -
> -%systemd_post %post_units
> -
> -# request daemon restart in posttrans
> -mkdir -p %{_localstatedir}/lib/rpm-state/libvirt || :
> -touch %{_localstatedir}/lib/rpm-state/libvirt/restart || :
> -
>
>  %posttrans daemon
> +%libvirt_sc_posttrans libvirtd virtproxyd virtlogd virtlockd libvirt-guests
> +%global post_units \\\
> +        virtlockd.socket virtlockd-admin.socket \\\
> +        virtlogd.socket virtlogd-admin.socket \\\
> +        libvirtd.socket libvirtd-ro.socket libvirtd-admin.socket \\\
> +        libvirtd-tcp.socket libvirtd-tls.socket \\\
> +        libvirtd.service \\\
> +        libvirt-guests.service
> +
> +%systemd_post %post_units
> +
> +# request daemon restart in posttrans
> +mkdir -p %{_localstatedir}/lib/rpm-state/libvirt || :
> +touch %{_localstatedir}/lib/rpm-state/libvirt/restart || :

Moving this stuff around changes its semantics. I'm not familiar
enough with rpm packaging to understand exactly the consequences, but
I think you should be extremely careful with this kind of change and
definitely not perform it at the same time as you're adding new
functionality.

>  if [ -f %{_localstatedir}/lib/rpm-state/libvirt/restart ]; then
>      # See if user has previously modified their install to
>      # tell libvirtd to use --listen
> -    grep -E '^LIBVIRTD_ARGS=.*--listen' /etc/sysconfig/libvirtd 1>/dev/null 2>&1
> +    if test -f /etc/sysconfig/libvirtd
> +    then
> +        grep -E '^LIBVIRTD_ARGS=.*--listen' /etc/sysconfig/libvirtd 1>/dev/null 2>&1
> +    fi

I don't think you need to make this conditional: if the file doesn't
exist, grep will exit with a non-zero code, same as if the file
existed but no match was found in it.

Pre-existing: am I missing something, or is the daemon actually *not*
being restarted when --listen is found? We mask a bunch of units and
that's pretty much it.

Also pre-existing: do we even care about handling upgrades from
versions of the daemon that didn't have support for systemd socket
passing at this point? The .spec file explicitly limits support to
RHEL 8 and Fedora 33, which should be plenty recent enough to make
the entire dance unnecessary.

> +%pre daemon-driver-interface
> +%libvirt_sc_posttrans virtinterfaced

Wrong function called in %pre (there are a few more instances of this
mistake in the patch).

>  %if %{with_qemu}
> +%pre daemon-driver-qemu
> +%libvirt_sc_pre virtqemud
> +# We want soft static allocation of well-known ids, as disk images
> +# are commonly shared across NFS mounts by id rather than name; see
> +# https://fedoraproject.org/wiki/Packaging:UsersAndGroups
> +getent group kvm >/dev/null || groupadd -f -g 36 -r kvm
> +getent group qemu >/dev/null || groupadd -f -g 107 -r qemu
> +if ! getent passwd qemu >/dev/null; then
> +  if ! getent passwd 107 >/dev/null; then
> +    useradd -r -u 107 -g qemu -G kvm -d / -s /sbin/nologin -c "qemu user" qemu
> +  else
> +    useradd -r -g qemu -G kvm -d / -s /sbin/nologin -c "qemu user" qemu
> +  fi
> +fi
> +exit 0

Moving this section is probably a good idea, but it shouldn't happen
at the same time as you're changing things. Make all pure code
movements a separate preparatory commit, please.

> --- a/src/locking/virtlockd.sysconf
> +++ /dev/null
> @@ -1,3 +0,0 @@
> -# Customizations for the virtlockd.service systemd unit
> -
> -VIRTLOCKD_ARGS=""

You're dropping the sysconfig file without adding the corresponding
Environment= directive in the .service file. Even though the default
is to pass no arguments to this daemon (and virtlogd), we should
still include that line for discoverability and consistency purposes.

> +++ b/src/remote/libvirtd.service.in
> @@ -28,6 +28,13 @@ Documentation=https://libvirt.org
>
>  [Service]
>  Type=notify
> +# Override the QEMU/SDL default audio driver probing when
> +# starting virtual machines using SDL graphics
> +# NB these have no effect for VMs using VNC, unless vnc_allow_host_audio
> +# is enabled in /etc/libvirt/qemu.conf
> +#Environment=QEMU_AUDIO_DRV=sdl
> +#Environment=SDL_AUDIODRIVER=pulse
> +Environment=LIBVIRTD_ARGS="--timeout 120"

You're losing the documentation for the --timeout option, as well as
both the documentation and the example usage for the --listen option.

> +++ b/tools/libvirt-guests.sh.in
> @@ -30,13 +30,53 @@ test ! -r "$sysconfdir"/rc.d/init.d/functions ||
> +# URIs to check for running guests
> +# example: URIS='default xen:///system vbox+tcp://host/system lxc:///system'
>  URIS="default"
[...]
> +# If non-zero, try to sync guest time on domain resume. Be aware, that
> +# this requires guest agent with support for time synchronization
> +# running in the guest. By default, this functionality is turned off.
>  SYNC_TIME=0

Why did you move these here instead of adding them to the .service
file? We certainly don't want users to edit the script directly in
order to configure its behavior, or having to look at the source code
to understand what the various settings do.

>  test -f "$sysconfdir"/sysconfig/libvirt-guests &&

Pre-existing: we source the sysconfig file in the .service file
through the EnvironmentFile= directive, then set a bunch of defaults
at the top of the script, then source the sysconfig file again. That
seems sketchy, but honestly pretty much all of libvirt-guests feels
fragile and poorly thought out :(

All the comments I've made up until here are about the purely
technical side of the changes. Overall, I'm still not entirely sold
on the idea of this actually being an improvement over the status
quo.

In particular, I worry about changes in defaults being more difficult
for users to detect: in Debian at least, changes to the default
sysconfig files result in the admin being given the possibility to
review and tweak their local customizations at package upgrade time,
and by moving the defaults off to the .service files we're losing
that convenience. I understand other distros don't have the same
tooling around configuration files, but still it feels like a step
backwards in this regard.

-- 
Andrea Bolognani / Red Hat / Virtualization