6:23 a.m.
Am Wed, 21 Jul 2021 03:16:39 -0700
schrieb Andrea Bolognani <abologna(a)redhat.com>:
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/
I assumed a single host admin.
> +++ 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.
Why is that systemd part needed?
The sysconfig files are recognized, they just have to be created.
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.
Yes, I have seen supposedly educated people struggling with the fact that a file does not
exist on-disk.
> +++ 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.
Only files marked as %config and which have been modified will be preserved as .rpmsave.
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.
Stale .rpmsave files will be preserved, just in case they contain any valuable data.
During upgrade rpm may create a .rpmsave file, which is then renamed back.
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?
If a transaction is interrupted for whatever reason the system is in an inconstant state.
No automation can get it out of such state.
> -%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.
There is no point in restarting libvirtd in the middle of the transaction.
The spec file gives no ordering hints to rpm.
But yeah, I need to double check this part.
Perhaps it needs to be in a separate patch.
> 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.
Yeah, I considered this. Lets use the ENOENT as condition.
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.
I will double check this part.
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.
There is also a check for version 1.3 from 2015.
I think in practice it is very unlikely that one can upgrade from a pre-1.3 version to
libvirt.git#master on a live system.
> +%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).
Thanks for spotting this.
> %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.
I can do such movements in a separate patch, good idea.
> --- 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.
I think this is not required. The command line is obvious, what variable is expected, and
what sysconfig would be parsed to obtain it.
> +++ 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.
I think documentation should go into documentation files, instead of code or configuration
files.
In case the commits which added --timeout and/or --listen failed to document it properly,
that mistake has to be fixed in a separate patch.
> +++ 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.
I just moved documentation into the code. Perhaps libvirt-guests is already documented
elsewhere, and such documentation should be extended.
There old sysconfig file did not provide any defaults, as a result the service file does
not need to provide defaults either. The defaults remain in the code.
> 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 :(
I think the sourcing can be removed from the service file.
All other usage of EnvironmentFile= is just to obtain potential command line knobs.
This can be done in a separate patch.
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.
Yeah. On the SUSE side the sysconfig files are not owned by a package.
We could just wipe the templates.
But we would have to maintain a patch which puts the desired built-in defaults into the
individual service files.
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.
It is up to the Debian package maintainer to sort this out in his environment.
I think it is and was wrong to put anything into /etc and claim these knobs are the
default behavior.
Every default has to go into the code, so that it can be changed over time, if required.
I admit, a few packages with complex configuration have to be handled differently.
Olaf