libvirt-guests configurability regression
Hi, I'm really unhappy about commit 8eb4461645c5 ("remove sysconfig files", 2022-01-17), first included in release v8.1.0. The (a) well-documented and (b) easily editable config file "/etc/sysconfig/libvirt-guests" is now gone. So if I want to do now on Fedora 36 the same thing that I used to do on up to and including Fedora 35, I now need to consult a new manual page (from grandparent commit 161727417a91, "docs: Add man page for libvirt-guests", 2022-01-17), and collect a bunch of options manually. The message on commit 8eb4461645c5 says, Remove the sysconfig file and place the current desired default into the service file. which I briefly considered a consolation, figuring I'd just copy the collected bunch of options (and hopefully their comments!) to the same place as before, from the "service file" -- "libvirt-guests.service". However, the actual commit does not live up to its promise; for example, the important ON_SHUTDOWN knob is only *removed* from the codebase by the commit; it is not reintroduced anywhere (certainly not in "libvirt-guests.service"). Well, the manual page, two commits up the branch, documents it, but that's totally no viable replacement. As of f8ebb5816350:
$ git grep -w ON_SHUTDOWN
docs/manpages/libvirt-guests.rst:- ON_SHUTDOWN=suspend docs/manpages/libvirt-guests.rst: time to shutdown. When setting ON_SHUTDOWN=shutdown, you must also set docs/manpages/libvirt-guests.rst: "ON_SHUTDOWN" is set to "shutdown". If Set to 0, guests will be shutdown one tools/libvirt-guests.sh.in:ON_SHUTDOWN="suspend" tools/libvirt-guests.sh.in: if [ "x$ON_SHUTDOWN" = xshutdown ]; then tools/libvirt-guests.sh.in: ON_SHUTDOWN="shutdown"
... It seems that "tools/libvirt-guests.sh.in" does have some built-in defaults (going back as far as to 66823690e469, "Init script for handling guests on shutdown/boot", 2010-05-21), which I could copy and modify presumably; however, those defaults still lack the previously directly adjacent documentation. Please consider remedying this. Readily editable config files with documentation and defaults included are very powerful. They are not suitable for all config formats of course (especially hierarchical ones: consider the domain XML for example), but for flat or otherwise simply structured config files, offering that productivity boost to end-users is a no-brainer, IMO. Please restore it if you can. Thanks Laszlo
Mon, 7 Nov 2022 14:17:00 +0100 Laszlo Ersek <lersek@redhat.com>:
The (a) well-documented and (b) easily editable config file "/etc/sysconfig/libvirt-guests" is now gone.
Right, these admin-owned files are not installed anymore. The runtime code still uses the files, in case they are created by the admin.
The message on commit 8eb4461645c5 says, Remove the sysconfig file and place the current desired default into the service file.
Yes, and right in the next paragraph the commit messages states the files are (still) recognized. As of f8b6c7e5, libvirt-guests.sh initializes some internal defaults, then it loads the admin-owned sysconfig file, in case it was created. The documentation about this specific tool (libvirt-guests(1)) states what values from /etc/sysconfig/libvirt-guests will be recognized. While browsing various user mailing lists in the past years I often saw messages like "system was upgraded to new version, and as a result an expected file somewhere in /etc does not exist anymore. What now?". Apparently some folks expect files to be present before they can be edited. In my experience it is always possible to create the required files and fill them with the desired content, based on available documentation. Olaf
On 11/07/22 18:01, Olaf Hering wrote:
Mon, 7 Nov 2022 14:17:00 +0100 Laszlo Ersek <lersek@redhat.com>:
The (a) well-documented and (b) easily editable config file "/etc/sysconfig/libvirt-guests" is now gone.
Right, these admin-owned files are not installed anymore. The runtime code still uses the files, in case they are created by the admin.
The message on commit 8eb4461645c5 says, Remove the sysconfig file and place the current desired default into the service file.
Yes, and right in the next paragraph the commit messages states the files are (still) recognized.
As of f8b6c7e5, libvirt-guests.sh initializes some internal defaults, then it loads the admin-owned sysconfig file, in case it was created.
The documentation about this specific tool (libvirt-guests(1)) states what values from /etc/sysconfig/libvirt-guests will be recognized.
While browsing various user mailing lists in the past years I often saw messages like "system was upgraded to new version, and as a result an expected file somewhere in /etc does not exist anymore. What now?". Apparently some folks expect files to be present before they can be edited. In my experience it is always possible to create the required files and fill them with the desired content, based on available documentation.
It's obviously possible; the question is how comfortable it is, how much time the user now needs to spend on something that used to be much easier / faster before. Can you at least include the previously shipped *documented* config file as a template in a contrib or docs directory or something? Thanks Laszlo
Tue, 8 Nov 2022 08:46:34 +0100 Laszlo Ersek <lersek@redhat.com>:
Can you at least include the previously shipped *documented* config file as a template in a contrib or docs directory or something?
This is up to the maintainers of libvirt.git to decide. If I were in charge, nothing would be installed into /etc. What needs to go there, in case it is required, is already reasonably well documented. Olaf
On Tue, Nov 08, 2022 at 04:58:37PM +0100, Olaf Hering wrote:
Tue, 8 Nov 2022 08:46:34 +0100 Laszlo Ersek <lersek@redhat.com>:
Can you at least include the previously shipped *documented* config file as a template in a contrib or docs directory or something?
This is up to the maintainers of libvirt.git to decide.
If I were in charge, nothing would be installed into /etc. What needs to go there, in case it is required, is already reasonably well documented.
Laszlo, I feel your frustration. I don't think a revert to the previous situation is going to happen, because there are some real advantages resulting from not shipping admin-owned files in our packages. That said, the current situation is clearly not ideal either, so let's try to find a way to make things at least a bit better :) I think a reasonable compromise would be to add Environment=URIS="default" Environment=ON_BOOT="start" ... to libvirt-guests.service. Maybe instead of having the extended documentation that was originally in the defaults file we could have a shorter, one-line version? Maybe a pointer to the manual page? Whatever comments we put there will show up when running 'systemctl edit libvirt-guests', which makes them fairly discoverable IMO. That's really the key point, because even today you can change the behavior both with a defaults file and a systemd unit override. What do you think? Would that work for you? -- Andrea Bolognani / Red Hat / Virtualization
Tue, 8 Nov 2022 10:07:21 -0800 Andrea Bolognani <abologna@redhat.com>:
I think a reasonable compromise would be to add Environment=URIS="default" Environment=ON_BOOT="start" to libvirt-guests.service.
The script needs to be adjusted to recognize existing environment variables. Olaf
On 11/08/22 19:07, Andrea Bolognani wrote:
On Tue, Nov 08, 2022 at 04:58:37PM +0100, Olaf Hering wrote:
Tue, 8 Nov 2022 08:46:34 +0100 Laszlo Ersek <lersek@redhat.com>:
Can you at least include the previously shipped *documented* config file as a template in a contrib or docs directory or something?
This is up to the maintainers of libvirt.git to decide.
If I were in charge, nothing would be installed into /etc. What needs to go there, in case it is required, is already reasonably well documented.
Laszlo,
I feel your frustration.
I don't think a revert to the previous situation is going to happen, because there are some real advantages resulting from not shipping admin-owned files in our packages.
That said, the current situation is clearly not ideal either, so let's try to find a way to make things at least a bit better :)
I think a reasonable compromise would be to add
Environment=URIS="default" Environment=ON_BOOT="start" ...
to libvirt-guests.service. Maybe instead of having the extended documentation that was originally in the defaults file we could have a shorter, one-line version? Maybe a pointer to the manual page?
Whatever comments we put there will show up when running 'systemctl edit libvirt-guests', which makes them fairly discoverable IMO. That's really the key point, because even today you can change the behavior both with a defaults file and a systemd unit override.
What do you think? Would that work for you?
I'd prefer an (otherwise initially inactive) file somewhere in the distribution that followed the style of (say) "/etc/libvirt/qemu.conf". I could copy that in-place, and then edit it. If this still counts as duplication, then let's stick with the status quo. Thanks, Laszlo
On Tue, Nov 08, 2022 at 08:46:34AM +0100, Laszlo Ersek wrote:
On 11/07/22 18:01, Olaf Hering wrote:
Mon, 7 Nov 2022 14:17:00 +0100 Laszlo Ersek <lersek@redhat.com>:
The (a) well-documented and (b) easily editable config file "/etc/sysconfig/libvirt-guests" is now gone.
Right, these admin-owned files are not installed anymore. The runtime code still uses the files, in case they are created by the admin.
The message on commit 8eb4461645c5 says, Remove the sysconfig file and place the current desired default into the service file.
Yes, and right in the next paragraph the commit messages states the files are (still) recognized.
As of f8b6c7e5, libvirt-guests.sh initializes some internal defaults, then it loads the admin-owned sysconfig file, in case it was created.
The documentation about this specific tool (libvirt-guests(1)) states what values from /etc/sysconfig/libvirt-guests will be recognized.
While browsing various user mailing lists in the past years I often saw messages like "system was upgraded to new version, and as a result an expected file somewhere in /etc does not exist anymore. What now?". Apparently some folks expect files to be present before they can be edited. In my experience it is always possible to create the required files and fill them with the desired content, based on available documentation.
It's obviously possible; the question is how comfortable it is, how much time the user now needs to spend on something that used to be much easier / faster before.
Can you at least include the previously shipped *documented* config file as a template in a contrib or docs directory or something?
That would just be duplicating information already provideed in the manpage, where there is much more detailed description, so I don't think that would be useful. With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 11/08/22 18:36, Daniel P. Berrangé wrote:
On Tue, Nov 08, 2022 at 08:46:34AM +0100, Laszlo Ersek wrote:
On 11/07/22 18:01, Olaf Hering wrote:
Mon, 7 Nov 2022 14:17:00 +0100 Laszlo Ersek <lersek@redhat.com>:
The (a) well-documented and (b) easily editable config file "/etc/sysconfig/libvirt-guests" is now gone.
Right, these admin-owned files are not installed anymore. The runtime code still uses the files, in case they are created by the admin.
The message on commit 8eb4461645c5 says, Remove the sysconfig file and place the current desired default into the service file.
Yes, and right in the next paragraph the commit messages states the files are (still) recognized.
As of f8b6c7e5, libvirt-guests.sh initializes some internal defaults, then it loads the admin-owned sysconfig file, in case it was created.
The documentation about this specific tool (libvirt-guests(1)) states what values from /etc/sysconfig/libvirt-guests will be recognized.
While browsing various user mailing lists in the past years I often saw messages like "system was upgraded to new version, and as a result an expected file somewhere in /etc does not exist anymore. What now?". Apparently some folks expect files to be present before they can be edited. In my experience it is always possible to create the required files and fill them with the desired content, based on available documentation.
It's obviously possible; the question is how comfortable it is, how much time the user now needs to spend on something that used to be much easier / faster before.
Can you at least include the previously shipped *documented* config file as a template in a contrib or docs directory or something?
That would just be duplicating information already provideed in the manpage, where there is much more detailed description, so I don't think that would be useful.
The following files: /etc/libvirt/libvirt-admin.conf /etc/libvirt/libvirt.conf /etc/libvirt/libvirtd.conf /etc/libvirt/libxl.conf /etc/libvirt/libxl-lockd.conf /etc/libvirt/lxc.conf /etc/libvirt/qemu.conf /etc/libvirt/qemu-lockd.conf /etc/libvirt/virtinterfaced.conf /etc/libvirt/virtlockd.conf /etc/libvirt/virtlogd.conf /etc/libvirt/virtlxcd.conf /etc/libvirt/virtnetworkd.conf /etc/libvirt/virtnodedevd.conf /etc/libvirt/virtnwfilterd.conf /etc/libvirt/virtproxyd.conf /etc/libvirt/virtqemud.conf /etc/libvirt/virtsecretd.conf /etc/libvirt/virtstoraged.conf /etc/libvirt/virtvboxd.conf /etc/libvirt/virtxend.conf all exist out of the box too, and follow the style that I'm requesting (= they are heavily documented in-line). Laszlo
On Wed, Nov 09, 2022 at 07:24:08AM +0100, Laszlo Ersek wrote:
On 11/08/22 18:36, Daniel P. Berrangé wrote:
On Tue, Nov 08, 2022 at 08:46:34AM +0100, Laszlo Ersek wrote:
Can you at least include the previously shipped *documented* config file as a template in a contrib or docs directory or something?
That would just be duplicating information already provideed in the manpage, where there is much more detailed description, so I don't think that would be useful.
The following files:
/etc/libvirt/libvirt-admin.conf /etc/libvirt/libvirt.conf [...] /etc/libvirt/virtvboxd.conf /etc/libvirt/virtxend.conf
all exist out of the box too, and follow the style that I'm requesting (= they are heavily documented in-line).
Uh. Good point. Olaf, can you please remind me why the files we dropped were problematic but these ones apparently aren't? -- Andrea Bolognani / Red Hat / Virtualization
Wed, 9 Nov 2022 09:04:12 -0800 Andrea Bolognani <abologna@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. The upgrade needs to be handled like it is done with libvirt_sysconfig_pre/libvirt_sysconfig_posttrans in libvirt.spec. Olaf
On Wed, Nov 09, 2022 at 09:17:08PM +0100, Olaf Hering wrote:
Wed, 9 Nov 2022 09:04:12 -0800 Andrea Bolognani <abologna@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. With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
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@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.
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. 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 :) -- Andrea Bolognani / Red Hat / Virtualization
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@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
participants (4)
-
Andrea Bolognani -
Daniel P. Berrangé -
Laszlo Ersek -
Olaf Hering