Am Wed, 21 Jul 2021 03:16:39 -0700
schrieb Andrea Bolognani <abologna@xxxxxxxxxx>:
> 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
Attachment:
pgpJR281Qq5VD.pgp
Description: Digitale Signatur von OpenPGP
