libvirt-guests configurability regression

[Laszlo Ersek]

On 11/10/22 11:32, Andrea Bolognani wrote:
> On Thu, Nov 10, 2022 at 08:57:25AM +0000, Daniel P. Berrangé wrote:
>> On Wed, Nov 09, 2022 at 09:17:08PM +0100, Olaf Hering wrote:
>>> Wed, 9 Nov 2022 09:04:12 -0800 Andrea Bolognani <abologna at redhat.com>:
>>>> Olaf, can you please remind me why the files we dropped were
>>>> problematic but these ones apparently aren't?
>>>
>>> These are equally problematic because they are owned by the admin as well.
>>>
>>> They are essentially empty, their content is a duplication from doc/ (I hope).
>>> They should not be part of libvirt.git.
>>
>> We are not going to remove these files from git or from the RPM, it would
>> be incredibly user hostile. I didn't mind removing the sysconfig files
>> because they were legacy configs from initscripts era which are obsolete
>> with systemd and so should not really be used except for existing
>> deployments.

Hmm. That's a good distinction actually. So I guess my original
complaint should be rephrased as one against systemd style service
configuration -- then again, that ship has sailed ;)

Thanks for the enlightenment!

> Should we reconsider the fate of libvirt-guests's sysconfig file
> then?
> 
> The other sysconfig files basically only influenced the way each
> service is spawned (VIRT*_ARGS, mostly used for --timeout) but in the
> case of libvirt-guests the sysconfig file is used to configure the
> behavior in ways that don't really fall under the purview of systemd.

I'm not sure if I know enough to claim this myself -- the libvirt-guests
service's configuration may actually fall in systemd territory. I just
find the override files

  /etc/systemd/system/NAME.service.d/override.conf

from "systemctl edit" much less intuitive than the previous way.

BTW, now that I've retried "systemctl edit", it seems that the override
file is "primed" from the central service file
(/usr/lib/systemd/system/NAME.service) -- the latter is included,
commented out, when $EDITOR starts up. So if all the documentation and
the default knob assignments were included in
"/usr/lib/systemd/system/libvirt-guests.service", then "systemctl edit"
would indeed be a mostly complete replacement for the sysconfig file.

In fact, commit 8eb4461645c5 does indicate *just this* method -- but as
I pointed out up-thread, the central "libvirt-guests.service" file does
not include the actual knobs and their docs; those are (at best) in
"tools/libvirt-guests.sh.in".

Instead, the central service file currently contains (excerpt):

> [Service]
> EnvironmentFile=-/etc/sysconfig/libvirt-guests
> # Hack just call traditional service until we factor
> # out the code
> ExecStart=/usr/libexec/libvirt-guests.sh start
> ExecStop=/usr/libexec/libvirt-guests.sh stop

I guess *that* is the actual problem -- it's technical debt. This
"factoring out" (= the migration of the knobs and the docs to the
service file) should have *preceded* commit 8eb4461645c5. Commit
8eb4461645c5 alluded to the proper (modern) way for configuring
services, and with that argument removed the sysconfig file for
"libvirt-guests" -- but the libvirt-guests service had not converted
sufficiently for that.

Therefore I do claim that commit 8eb4461645c5 is buggy -- it was too early.

Olaf said up-thread, "the script needs to be adjusted to recognize
existing environment variables", and technically, that may indeed be the
solution; I'm just saying it was actualy a *pre-requisite* (missed or
ignored) for commit 8eb4461645c5.

> Maybe a proper /etc/libvirt/libvirt-guests.conf would make sense, for
> consistency with other services? But then, you don't want to be
> parsing libvirt's config format from a shell script. Could we rewrite
> libvirt-guests in C, possibly improving on the hairier parts of it in
> the process? Perhaps even take the opportunity to reconsider the way
> some of its settings interact with each other.
> 
> I might be getting a bit ahead of myself here :)
> 

I think the logic being implemented as a shell script is fine, but its
configuration (env vars), and the related documentation, needs to be
moved / added to the central service file, so that "systemctl edit" can
present them as a crutch to the user, formatted in comments.

The manual page is *not* a substitute for this.

Laszlo