On Wed, Jul 21, 2021 at 01:23:56PM +0200, Olaf Hering wrote: > 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. In this case, "they" is intended as a gender-neutral pronoun rather than an indication that there is more than one person acting as admin for the machine :) > > > 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. Both options are equally valid for configuring the service, and so both should be documented. > > 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. I'm not entirely sure whether there are more precise guidelines on what to do in these scenarios in the context of Fedora or some other distribution, so I'll just say that this sounds reasonable enough. > > 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. Note that the daemon restart always happens *after* the transaction, in the %posttrans 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. Yeah, I think that could go too. We have very broad backward compatibility guarantees when it comes to libvirt, but in the context of a spec file that explicitly refuses to work on anything but fairly recent platforms I think it's absolutely fair to tone down these guarantees accordingly. > > > -# 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. Spelling it out explicitly would help making things more discoverable, the same way it currently does for the sysconfig file, so I'd much rather we kept it even if in a different form. > > > +# 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. Both options are indeed documented in libvirtd(8). --listen is a particularly tricky one, but with virtproxyd taking over its role in the modular daemon scenario and the latter slated to become the default configuration soon, perhaps that's good enough. > > > +# 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. It looks like the only documentation for the various options that can be used to tweak the service's behavior was the one found in the sysconfig file. Perhaps a man page should be introduced? > > 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. But then libvirt-guests would end up being the only service that cannot be configured via a systemd override... I think we should strive for consistency here. > 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. For most software, the default configuration files consist of mostly comments and act more like documentation for what the local configuration might look like. Claiming that the defaults are actually defined in the source code is correct, but also not very useful if it means that the local admin needs to go poking at said sources to figure out how to configure their machine... -- Andrea Bolognani / Red Hat / Virtualization
