Since virtproxyd was split into libvirt-daemon-proxy package it can
happen that, in case a distribution has such systemd preset, when
installing this package, already pre-enabled and configured units like
-tls.socket and -tcp.socket will get disabled.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2210058
Fixes: 5358618b1cd0afc126aed313249bf2134731665f
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
---
This is more like an RFC as I would really like to know what to really do in
this case.  What happens, basically, is that if you have libvirt-daemon-9.0.0
and set up virtproxyd-tls.socket for example and then upgrade to anything newer,
then the package libvirt-daemon-proxy will get installed.  The %post action
calls "%libvirt_daemon_systemd_post_inet virtproxyd" which calls "%systemd_post
with all virtproxyd units.  What %systemd_post is supposed to do is reset units
to a preset state in the case of package installation, but not during upgrade.
However the libvirt-daemon-proxy package did not exist on the system before, so
this action is not an update, but an installation.

If no preset is mentioned for a unit then `systemctl preset` does not change
anything.  However some distros might have a catch-all preset "disable *" for
some reason, I guess based on an example in the documentation, and there is no
way to override an already configured preset, you can only enable or disable a
unit in a preset.

That all means than it can happen that you enable virtproxyd-tcp.socket, for
example, then update your system and find that it is disabled.  There are
various ways to deal with this, but I don't see any one that would 100% satisfy
me with regards to all the issues and at the same time could be implemented
"soon enough" given libvirt already had three releases with the
libvirt-daemon-proxy split.

 libvirt.spec.in | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/libvirt.spec.in b/libvirt.spec.in
index 1f77cd90b772..50f521b7ce88 100644
--- a/libvirt.spec.in
+++ b/libvirt.spec.in
@@ -1592,7 +1592,13 @@ fi
 
 %post daemon-proxy
 %if %{with_modular_daemons}
-%libvirt_daemon_systemd_post_inet virtproxyd
+# Since this was split into a different package, a transparent update for the
+# virtproxyd units could actually disable an already configured ones
+# (e.g. virtproxyd-tls.socket) as %systemd_post runs `systemctl preset` if this
+# is an installation (and is skipped on update).  So skip this step for those
+# that need an extra setup to work since they will most likely not be preset to
+# enabled, but that is up to the point of the distribution.
+%libvirt_daemon_systemd_post virtproxyd
 %endif
 
 %preun daemon-proxy
-- 
2.41.0

On Wed, Jun 07, 2023 at 04:31:36PM +0200, Martin Kletzander wrote:
> Since virtproxyd was split into libvirt-daemon-proxy package it can
> happen that, in case a distribution has such systemd preset, when
> installing this package, already pre-enabled and configured units like
> -tls.socket and -tcp.socket will get disabled.
>
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2210058
> Fixes: 5358618b1cd0afc126aed313249bf2134731665f
> Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
> ---
> This is more like an RFC as I would really like to know what to really do in
> this case.  What happens, basically, is that if you have libvirt-daemon-9.0.0
> and set up virtproxyd-tls.socket for example and then upgrade to anything newer,
> then the package libvirt-daemon-proxy will get installed.  The %post action
> calls "%libvirt_daemon_systemd_post_inet virtproxyd" which calls "%systemd_post
> with all virtproxyd units.  What %systemd_post is supposed to do is reset units
> to a preset state in the case of package installation, but not during upgrade.
> However the libvirt-daemon-proxy package did not exist on the system before, so
> this action is not an update, but an installation.
>
> If no preset is mentioned for a unit then `systemctl preset` does not change
> anything.  However some distros might have a catch-all preset "disable *" for
> some reason, I guess based on an example in the documentation, and there is no
> way to override an already configured preset, you can only enable or disable a
> unit in a preset.
>
> That all means than it can happen that you enable virtproxyd-tcp.socket, for
> example, then update your system and find that it is disabled.  There are
> various ways to deal with this, but I don't see any one that would 100% satisfy
> me with regards to all the issues and at the same time could be implemented
> "soon enough" given libvirt already had three releases with the
> libvirt-daemon-proxy split.
>
>  libvirt.spec.in | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/libvirt.spec.in b/libvirt.spec.in
> index 1f77cd90b772..50f521b7ce88 100644
> --- a/libvirt.spec.in
> +++ b/libvirt.spec.in
> @@ -1592,7 +1592,13 @@ fi
>
>  %post daemon-proxy
>  %if %{with_modular_daemons}
> -%libvirt_daemon_systemd_post_inet virtproxyd
> +# Since this was split into a different package, a transparent update for the
> +# virtproxyd units could actually disable an already configured ones
> +# (e.g. virtproxyd-tls.socket) as %systemd_post runs `systemctl preset` if this
> +# is an installation (and is skipped on update).  So skip this step for those
> +# that need an extra setup to work since they will most likely not be preset to
> +# enabled, but that is up to the point of the distribution.
> +%libvirt_daemon_systemd_post virtproxyd

It's actually worse than that: if you are using the monolithic daemon
on a distro that uses split daemons by default (e.g. Fedora), then
upgrading will result in libvirt-daemon-proxy being installed and
virtproxyd being enabled due to the preset.

However, since libvirtd and virtproxyd use (by design) the same paths
for their sockets, this will result in socket activation being broken
and libvirtd effectively not working anymore once the first 120
seconds after boot have passed.

I have been meaning to look into this but other things got in the
way, so thank you for getting the ball rolling; unfortunately, I
don't think what you have come up with will help in the scenario I've
just described.

Basically we need to detect if we're installing the
libvirt-daemon-proxy package as part of an upgrade and *not touch
anything* if that's the case. I'm not sure how that can be achieved
in the context of RPM scriptlets though, or if it's even possible :(

CC'ing Laine, who reported this issue initially, and Dan, who worked
with us to track it down.

-- 
Andrea Bolognani / Red Hat / Virtualization

On 6/8/23 08:52, Andrea Bolognani wrote:
> On Wed, Jun 07, 2023 at 04:31:36PM +0200, Martin Kletzander wrote:
>> Since virtproxyd was split into libvirt-daemon-proxy package it can
>> happen that, in case a distribution has such systemd preset, when
>> installing this package, already pre-enabled and configured units like
>> -tls.socket and -tcp.socket will get disabled.
>>
>> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2210058
>> Fixes: 5358618b1cd0afc126aed313249bf2134731665f
>> Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
>> ---
>> This is more like an RFC as I would really like to know what to really do in
>> this case.  What happens, basically, is that if you have libvirt-daemon-9.0.0
>> and set up virtproxyd-tls.socket for example and then upgrade to anything newer,
>> then the package libvirt-daemon-proxy will get installed.  The %post action
>> calls "%libvirt_daemon_systemd_post_inet virtproxyd" which calls "%systemd_post
>> with all virtproxyd units.  What %systemd_post is supposed to do is reset units
>> to a preset state in the case of package installation, but not during upgrade.
>> However the libvirt-daemon-proxy package did not exist on the system before, so
>> this action is not an update, but an installation.
>>
>> If no preset is mentioned for a unit then `systemctl preset` does not change
>> anything.  However some distros might have a catch-all preset "disable *" for
>> some reason, I guess based on an example in the documentation, and there is no
>> way to override an already configured preset, you can only enable or disable a
>> unit in a preset.
>>
>> That all means than it can happen that you enable virtproxyd-tcp.socket, for
>> example, then update your system and find that it is disabled.  There are
>> various ways to deal with this, but I don't see any one that would 100% satisfy
>> me with regards to all the issues and at the same time could be implemented
>> "soon enough" given libvirt already had three releases with the
>> libvirt-daemon-proxy split.
>>
>>   libvirt.spec.in | 8 +++++++-
>>   1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/libvirt.spec.in b/libvirt.spec.in
>> index 1f77cd90b772..50f521b7ce88 100644
>> --- a/libvirt.spec.in
>> +++ b/libvirt.spec.in
>> @@ -1592,7 +1592,13 @@ fi
>>
>>   %post daemon-proxy
>>   %if %{with_modular_daemons}
>> -%libvirt_daemon_systemd_post_inet virtproxyd
>> +# Since this was split into a different package, a transparent update for the
>> +# virtproxyd units could actually disable an already configured ones
>> +# (e.g. virtproxyd-tls.socket) as %systemd_post runs `systemctl preset` if this
>> +# is an installation (and is skipped on update).  So skip this step for those
>> +# that need an extra setup to work since they will most likely not be preset to
>> +# enabled, but that is up to the point of the distribution.
>> +%libvirt_daemon_systemd_post virtproxyd
> 
> It's actually worse than that: if you are using the monolithic daemon
> on a distro that uses split daemons by default (e.g. Fedora),

Why use the monolithic and split daemons together? Shouldn't we discourage such 
configuration? :-)

> then
> upgrading will result in libvirt-daemon-proxy being installed and
> virtproxyd being enabled due to the preset.
> 
> However, since libvirtd and virtproxyd use (by design) the same paths
> for their sockets, this will result in socket activation being broken
> and libvirtd effectively not working anymore once the first 120
> seconds after boot have passed.
> 
> I have been meaning to look into this but other things got in the
> way, so thank you for getting the ball rolling; unfortunately, I
> don't think what you have come up with will help in the scenario I've
> just described.
> 
> Basically we need to detect if we're installing the
> libvirt-daemon-proxy package as part of an upgrade and *not touch
> anything* if that's the case. I'm not sure how that can be achieved
> in the context of RPM scriptlets though, or if it's even possible :(

It's possible to determine a package install vs upgrade, but IIUC the problem 
can occur when installing or upgrading libvirt-daemon-proxy. The usual 'if [ $1 
-ge 2 ]' for upgrade-only actions wont work in that case.

Can we skip the 'libvirt_daemon_systemd_post_inet' if virtproxyd is enabled? Or 
adjust the macro to not 'systemd_post' sockets that are already enabled?

Regards,
Jim

On Thu, Jun 08, 2023 at 12:35:45PM -0600, Jim Fehlig wrote:
> On 6/8/23 08:52, Andrea Bolognani wrote:
> > On Wed, Jun 07, 2023 at 04:31:36PM +0200, Martin Kletzander wrote:
> > > +# Since this was split into a different package, a transparent update for the
> > > +# virtproxyd units could actually disable an already configured ones
> > > +# (e.g. virtproxyd-tls.socket) as %systemd_post runs `systemctl preset` if this
> > > +# is an installation (and is skipped on update).  So skip this step for those
> > > +# that need an extra setup to work since they will most likely not be preset to
> > > +# enabled, but that is up to the point of the distribution.
> > > +%libvirt_daemon_systemd_post virtproxyd
> >
> > It's actually worse than that: if you are using the monolithic daemon
> > on a distro that uses split daemons by default (e.g. Fedora),
>
> Why use the monolithic and split daemons together? Shouldn't we discourage
> such configuration? :-)

Whether to use monlithic or split daemons is ultimately a choice of
the local admin. Fedora defaults to split daemons, but switching back
to a "classic" monolithic deployment is still a fully supported
scenario.

Additionally, the current default has been adopted relatively
recently, and we have made the explicit decision *not* to migrate
existing installations over. So if your OS was originally installed
before split daemons had become the default and you've been dutifully
upgrading to subsequent Fedora releases without ever reinstalling,
then you're also going to be using the monolithic daemon.

> > Basically we need to detect if we're installing the
> > libvirt-daemon-proxy package as part of an upgrade and *not touch
> > anything* if that's the case. I'm not sure how that can be achieved
> > in the context of RPM scriptlets though, or if it's even possible :(
>
> It's possible to determine a package install vs upgrade, but IIUC the
> problem can occur when installing or upgrading libvirt-daemon-proxy. The
> usual 'if [ $1 -ge 2 ]' for upgrade-only actions wont work in that case.

Yeah, exactly: it's easy to detect whether a single package is being
upgraded or installed from scratch, but in this case we would need to
know whether libvirt-daemon is being upgraded in the scriptlet for
libvirt-daemon-proxy, which I don't think is possible.

Can we somehow enforce that libvirt-daemon is fully configured before
libvirt-daemon-proxy? If so, we could create a witness file based on
the information we need in libvirt-daemon's %post, look at it in
libvirt-daemon-proxy's %post and base our decision on that.

> Can we skip the 'libvirt_daemon_systemd_post_inet' if virtproxyd is enabled?
> Or adjust the macro to not 'systemd_post' sockets that are already enabled?

Sockets *and* services. We don't want virtproxyd.service to be
started on boot if libvirtd.service will also be.

-- 
Andrea Bolognani / Red Hat / Virtualization

On 6/9/23 03:05, Andrea Bolognani wrote:
> On Thu, Jun 08, 2023 at 12:35:45PM -0600, Jim Fehlig wrote:
>> On 6/8/23 08:52, Andrea Bolognani wrote:
>>> On Wed, Jun 07, 2023 at 04:31:36PM +0200, Martin Kletzander wrote:
>>>> +# Since this was split into a different package, a transparent update for the
>>>> +# virtproxyd units could actually disable an already configured ones
>>>> +# (e.g. virtproxyd-tls.socket) as %systemd_post runs `systemctl preset` if this
>>>> +# is an installation (and is skipped on update).  So skip this step for those
>>>> +# that need an extra setup to work since they will most likely not be preset to
>>>> +# enabled, but that is up to the point of the distribution.
>>>> +%libvirt_daemon_systemd_post virtproxyd
>>>
>>> It's actually worse than that: if you are using the monolithic daemon
>>> on a distro that uses split daemons by default (e.g. Fedora),
>>
>> Why use the monolithic and split daemons together? Shouldn't we discourage
>> such configuration? :-)
> 
> Whether to use monlithic or split daemons is ultimately a choice of
> the local admin. Fedora defaults to split daemons, but switching back
> to a "classic" monolithic deployment is still a fully supported
> scenario.
> 
> Additionally, the current default has been adopted relatively
> recently, and we have made the explicit decision *not* to migrate
> existing installations over. So if your OS was originally installed
> before split daemons had become the default and you've been dutifully
> upgrading to subsequent Fedora releases without ever reinstalling,
> then you're also going to be using the monolithic daemon.
> 
>>> Basically we need to detect if we're installing the
>>> libvirt-daemon-proxy package as part of an upgrade and *not touch
>>> anything* if that's the case. I'm not sure how that can be achieved
>>> in the context of RPM scriptlets though, or if it's even possible :(
>>
>> It's possible to determine a package install vs upgrade, but IIUC the
>> problem can occur when installing or upgrading libvirt-daemon-proxy. The
>> usual 'if [ $1 -ge 2 ]' for upgrade-only actions wont work in that case.
> 
> Yeah, exactly: it's easy to detect whether a single package is being
> upgraded or installed from scratch, but in this case we would need to
> know whether libvirt-daemon is being upgraded in the scriptlet for
> libvirt-daemon-proxy, which I don't think is possible.

I think we need to know more than whether libvirt-daemon is being upgraded. We 
need to also know whether it's running, right? Even if libvirt-daemon is being 
upgraded, it's fine to call %systemd_post on virtproxyd sockets if the libvirtd 
ones are disabled, right? If this is the case, my idea to use trigger scriptlets 
doesn't help.

> Can we somehow enforce that libvirt-daemon is fully configured before
> libvirt-daemon-proxy? If so, we could create a witness file based on
> the information we need in libvirt-daemon's %post, look at it in
> libvirt-daemon-proxy's %post and base our decision on that.

What would the witness file indicate? As I understand, it would essentially have 
to indicate whether libvirtd sockets/service are enabled. If so, couldn't that 
be done directly in virtproxy post script? Something like the below hack?

Regards,
Jim

diff --git a/libvirt.spec.in b/libvirt.spec.in
index 1f77cd90b7..c848c70c0c 100644
--- a/libvirt.spec.in
+++ b/libvirt.spec.in
@@ -1592,7 +1592,19 @@ fi

  %post daemon-proxy
  %if %{with_modular_daemons}
-%libvirt_daemon_systemd_post_inet virtproxyd
+libvirtd_enabled=0
+for unit in -ro.socket .service .socket
+do
+    if systemctl is-enabled libvirtd$unit
+    then
+        libvirtd_enabled=1
+        break
+    fi
+done
+if [ $libvirtd_enabled -eq 0 ]
+then
+    %libvirt_daemon_systemd_post_inet virtproxyd
+fi
  %endif

  %preun daemon-proxy

On Wed, Jun 14, 2023 at 04:45:06PM -0600, Jim Fehlig wrote:
>On 6/9/23 03:05, Andrea Bolognani wrote:
>> On Thu, Jun 08, 2023 at 12:35:45PM -0600, Jim Fehlig wrote:
>>> On 6/8/23 08:52, Andrea Bolognani wrote:
>>>> On Wed, Jun 07, 2023 at 04:31:36PM +0200, Martin Kletzander wrote:
>>>>> +# Since this was split into a different package, a transparent update for the
>>>>> +# virtproxyd units could actually disable an already configured ones
>>>>> +# (e.g. virtproxyd-tls.socket) as %systemd_post runs `systemctl preset` if this
>>>>> +# is an installation (and is skipped on update).  So skip this step for those
>>>>> +# that need an extra setup to work since they will most likely not be preset to
>>>>> +# enabled, but that is up to the point of the distribution.
>>>>> +%libvirt_daemon_systemd_post virtproxyd
>>>>
>>>> It's actually worse than that: if you are using the monolithic daemon
>>>> on a distro that uses split daemons by default (e.g. Fedora),
>>>
>>> Why use the monolithic and split daemons together? Shouldn't we discourage
>>> such configuration? :-)
>>
>> Whether to use monlithic or split daemons is ultimately a choice of
>> the local admin. Fedora defaults to split daemons, but switching back
>> to a "classic" monolithic deployment is still a fully supported
>> scenario.
>>
>> Additionally, the current default has been adopted relatively
>> recently, and we have made the explicit decision *not* to migrate
>> existing installations over. So if your OS was originally installed
>> before split daemons had become the default and you've been dutifully
>> upgrading to subsequent Fedora releases without ever reinstalling,
>> then you're also going to be using the monolithic daemon.
>>
>>>> Basically we need to detect if we're installing the
>>>> libvirt-daemon-proxy package as part of an upgrade and *not touch
>>>> anything* if that's the case. I'm not sure how that can be achieved
>>>> in the context of RPM scriptlets though, or if it's even possible :(
>>>
>>> It's possible to determine a package install vs upgrade, but IIUC the
>>> problem can occur when installing or upgrading libvirt-daemon-proxy. The
>>> usual 'if [ $1 -ge 2 ]' for upgrade-only actions wont work in that case.
>>
>> Yeah, exactly: it's easy to detect whether a single package is being
>> upgraded or installed from scratch, but in this case we would need to
>> know whether libvirt-daemon is being upgraded in the scriptlet for
>> libvirt-daemon-proxy, which I don't think is possible.
>
>I think we need to know more than whether libvirt-daemon is being upgraded. We
>need to also know whether it's running, right? Even if libvirt-daemon is being
>upgraded, it's fine to call %systemd_post on virtproxyd sockets if the libvirtd
>ones are disabled, right? If this is the case, my idea to use trigger scriptlets
>doesn't help.
>
>> Can we somehow enforce that libvirt-daemon is fully configured before
>> libvirt-daemon-proxy? If so, we could create a witness file based on
>> the information we need in libvirt-daemon's %post, look at it in
>> libvirt-daemon-proxy's %post and base our decision on that.
>
>What would the witness file indicate? As I understand, it would essentially have
>to indicate whether libvirtd sockets/service are enabled. If so, couldn't that
>be done directly in virtproxy post script? Something like the below hack?
>

We already have some things for this.  There is
libvirt_daemon_schedule_restart, but that is done in the post phase.

What we could do is check the current state (whether libvirtd is running
and what is being installed) in %pretrans of the daemon and virtproxyd,
save it somewhere similar to libvirt_daemon_schedule_restart and then in
%posttrans or %post actually figure out what the new state should be.

Does that make sense?

>Regards,
>Jim
>
>diff --git a/libvirt.spec.in b/libvirt.spec.in
>index 1f77cd90b7..c848c70c0c 100644
>--- a/libvirt.spec.in
>+++ b/libvirt.spec.in
>@@ -1592,7 +1592,19 @@ fi
>
>  %post daemon-proxy
>  %if %{with_modular_daemons}
>-%libvirt_daemon_systemd_post_inet virtproxyd
>+libvirtd_enabled=0
>+for unit in -ro.socket .service .socket
>+do
>+    if systemctl is-enabled libvirtd$unit
>+    then
>+        libvirtd_enabled=1
>+        break
>+    fi
>+done
>+if [ $libvirtd_enabled -eq 0 ]
>+then
>+    %libvirt_daemon_systemd_post_inet virtproxyd
>+fi
>  %endif
>
>  %preun daemon-proxy
>

On 6/16/23 02:12, Martin Kletzander wrote:
> On Wed, Jun 14, 2023 at 04:45:06PM -0600, Jim Fehlig wrote:
>> On 6/9/23 03:05, Andrea Bolognani wrote:
>>> On Thu, Jun 08, 2023 at 12:35:45PM -0600, Jim Fehlig wrote:
>>>> On 6/8/23 08:52, Andrea Bolognani wrote:
>>>>> On Wed, Jun 07, 2023 at 04:31:36PM +0200, Martin Kletzander wrote:
>>>>>> +# Since this was split into a different package, a transparent update for 
>>>>>> the
>>>>>> +# virtproxyd units could actually disable an already configured ones
>>>>>> +# (e.g. virtproxyd-tls.socket) as %systemd_post runs `systemctl preset` 
>>>>>> if this
>>>>>> +# is an installation (and is skipped on update).  So skip this step for 
>>>>>> those
>>>>>> +# that need an extra setup to work since they will most likely not be 
>>>>>> preset to
>>>>>> +# enabled, but that is up to the point of the distribution.
>>>>>> +%libvirt_daemon_systemd_post virtproxyd
>>>>>
>>>>> It's actually worse than that: if you are using the monolithic daemon
>>>>> on a distro that uses split daemons by default (e.g. Fedora),
>>>>
>>>> Why use the monolithic and split daemons together? Shouldn't we discourage
>>>> such configuration? :-)
>>>
>>> Whether to use monlithic or split daemons is ultimately a choice of
>>> the local admin. Fedora defaults to split daemons, but switching back
>>> to a "classic" monolithic deployment is still a fully supported
>>> scenario.
>>>
>>> Additionally, the current default has been adopted relatively
>>> recently, and we have made the explicit decision *not* to migrate
>>> existing installations over. So if your OS was originally installed
>>> before split daemons had become the default and you've been dutifully
>>> upgrading to subsequent Fedora releases without ever reinstalling,
>>> then you're also going to be using the monolithic daemon.
>>>
>>>>> Basically we need to detect if we're installing the
>>>>> libvirt-daemon-proxy package as part of an upgrade and *not touch
>>>>> anything* if that's the case. I'm not sure how that can be achieved
>>>>> in the context of RPM scriptlets though, or if it's even possible :(
>>>>
>>>> It's possible to determine a package install vs upgrade, but IIUC the
>>>> problem can occur when installing or upgrading libvirt-daemon-proxy. The
>>>> usual 'if [ $1 -ge 2 ]' for upgrade-only actions wont work in that case.
>>>
>>> Yeah, exactly: it's easy to detect whether a single package is being
>>> upgraded or installed from scratch, but in this case we would need to
>>> know whether libvirt-daemon is being upgraded in the scriptlet for
>>> libvirt-daemon-proxy, which I don't think is possible.
>>
>> I think we need to know more than whether libvirt-daemon is being upgraded. We
>> need to also know whether it's running, right? Even if libvirt-daemon is being
>> upgraded, it's fine to call %systemd_post on virtproxyd sockets if the libvirtd
>> ones are disabled, right? If this is the case, my idea to use trigger scriptlets
>> doesn't help.
>>
>>> Can we somehow enforce that libvirt-daemon is fully configured before
>>> libvirt-daemon-proxy? If so, we could create a witness file based on
>>> the information we need in libvirt-daemon's %post, look at it in
>>> libvirt-daemon-proxy's %post and base our decision on that.
>>
>> What would the witness file indicate? As I understand, it would essentially have
>> to indicate whether libvirtd sockets/service are enabled. If so, couldn't that
>> be done directly in virtproxy post script? Something like the below hack?
>>
> 
> We already have some things for this.  There is
> libvirt_daemon_schedule_restart, but that is done in the post phase.
> 
> What we could do is check the current state (whether libvirtd is running
> and what is being installed) in %pretrans of the daemon and virtproxyd,
> save it somewhere similar to libvirt_daemon_schedule_restart and then in
> %posttrans or %post actually figure out what the new state should be.
> 
> Does that make sense?

Yes, and sorry for getting caught up in the followup issue Andrea raised. 
Although I'm having a hard time reproducing either issue, which makes it 
difficult to experiment and eventually test a fix.

My setup is a recent openSUSE Tumbleweed. For your issue I added the following 
presets to /usr/lib/systemd/system-preset/90-default-openSUSE.preset

enable virtqemud.socket
enable virtlogd.socket
enable virtnetworkd.socket
enable virtnodedevd.socket
enable virtstoraged.socket
enable virtsecretd.socket

Next I installed libvirt-daemon and libvirt-daemon-qemu without commit 
b1da03b5b3. As expected, all the above sockets where enabled, while 
virtproxyd.socket and libvirtd.socket were disabled. I then enabled 
virtproxyd.socket and virtproxyd-tcp.socket. Finally I updated the setup with 
packages containing b1da03b5b3, the new libvirt-daemon-proxy was installed, but 
in the end virtproxyd.socket and virtproxyd-tcp.socket were still enabled and 
the system seemed fine.

In an attempt to reproduce Andrea's report, I added the following presets to 
/usr/lib/systemd/system-preset/90-default-openSUSE.preset

enable virtqemud.socket
enable virtlogd.socket
enable virtnetworkd.socket
enable virtnodedevd.socket
enable virtstoraged.socket
enable virtsecretd.socket
enable virtproxyd.socket

As before, installed packages without commit b1da03b5b3. I then disabled all the 
virt* presets (with exception of virtlogd) and enabled libvirtd.socket. After 
updating to packages containing commit b1da03b5b3, virtproxyd.socket was still 
disabled and libvirt.socket was enabled. No problem connecting to libvirtd, even 
after the service timeout.

IIUC, the Tumbleweed default is to disable presets, similar to the Fedora 
default you mentioned earlier. The systemd-presets-common-SUSE package contains 
/usr/lib/systemd/system-preset/99-default-disable.preset, which has the single 
entry 'disable *'.

Any tips on reproducing the issue? I must be missing something. What are the 
libvirt-related presets in Fedora? I couldn't easily find them. FYI, the 
Tumbleweed ones are here

https://build.opensuse.org/package/show/Base:System/systemd-presets-common-SUSE
https://build.opensuse.org/package/show/Base:System/systemd-presets-branding-openSUSE

In the past I've never added libvirt services/sockets to the openSUSE presets 
since users could easily enable libvirtd and virtlogd and be done with it. (Note 
the SLES presets do enable libvirtd and virtlogd.) But it becomes more 
burdensome with modular daemons, so I plan to add the above presets (excluding 
virtproxyd) to the openSUSE defaults.

Regards,
Jim

On Fri, Jun 16, 2023 at 03:57:37PM -0600, Jim Fehlig wrote:
> On 6/16/23 02:12, Martin Kletzander wrote:
> > On Wed, Jun 14, 2023 at 04:45:06PM -0600, Jim Fehlig wrote:
> > > On 6/9/23 03:05, Andrea Bolognani wrote:
> > > > Can we somehow enforce that libvirt-daemon is fully configured before
> > > > libvirt-daemon-proxy? If so, we could create a witness file based on
> > > > the information we need in libvirt-daemon's %post, look at it in
> > > > libvirt-daemon-proxy's %post and base our decision on that.
> > >
> > > What would the witness file indicate? As I understand, it would essentially have
> > > to indicate whether libvirtd sockets/service are enabled. If so, couldn't that
> > > be done directly in virtproxy post script? Something like the below hack?

The current logic in all packages' %post scriptlet basically
translates to: if installing the package from scratch, apply the
systemd presets; if upgrading, leave things well alone.

What I would want the witness file to indicate would be:
libvirt-daemon is being upgraded, so when running the %post for
libvirt-daemon-proxy behave as in the upgrade scenario as opposed to
the install-from-scratch scenario.

Detecting whether the monolithic libvirtd and its sockets are enabled
could also work, but feels more fragile. Specifically, it would not
cover the scenario described by Martin, where the deployment is a
split daemon one but with some units that are disabled by default
based on presets have been manually enabled by the admin.

The problem is that I'm not sure we can create and process such a
witness file reliably. Figuring that out requires research and
real-life testing :)

> In an attempt to reproduce Andrea's report, I added the following presets to
> /usr/lib/systemd/system-preset/90-default-openSUSE.preset
>
> enable virtqemud.socket
> enable virtlogd.socket
> enable virtnetworkd.socket
> enable virtnodedevd.socket
> enable virtstoraged.socket
> enable virtsecretd.socket
> enable virtproxyd.socket
>
> As before, installed packages without commit b1da03b5b3. I then disabled all
> the virt* presets (with exception of virtlogd) and enabled libvirtd.socket.

I'm not sure this is entirely relevant, but just for completeness'
sake: in a monolithic daemon deployment, you want libvirtd.service in
addition to its sockets to be enabled. This is needed to make sure
domain autostart works as intended.

Basically, starting from a split daemon deployment, you want to
follow the steps outlined here[1] but in reverse.

> After updating to packages containing commit b1da03b5b3, virtproxyd.socket
> was still disabled and libvirt.socket was enabled. No problem connecting to
> libvirtd, even after the service timeout.

I'll try to reproduce this myself later this week, but what you're
describing doesn't match my expectations: virtproxyd.socket should
have been enabled during installation.

Are the packages you're using for testing a direct rebuild of the
spec file shipped upstream? Or have you integrated the changes into
the openSUSE spec file somehow?

Note that the scriptlets in the upstream spec file call out to some
standard macros, and it's also possible that the implementation of
said macros is not the same across Fedora and openSUSE.

> Any tips on reproducing the issue? I must be missing something. What are the
> libvirt-related presets in Fedora? I couldn't easily find them. FYI, the
> Tumbleweed ones are here
>
> https://build.opensuse.org/package/show/Base:System/systemd-presets-common-SUSE
> https://build.opensuse.org/package/show/Base:System/systemd-presets-branding-openSUSE

They're part of the fedora-release package[2].

Specifically, we have

  # Virtualization driver specific daemons. Start by default at boot for VM
  # autostart, but shutdown after 2 mins and socket activated thereafter
  enable virtqemud.service
  enable virtxend.service
  enable virtlxcd.service

  # Compatibility with libvirtd sockets for old clients and expose TCP sockets
  enable virtproxyd.socket

  # Secondary drivers providing supporting functionality to main virtualization
  # drivers, socket activated only when required
  enable virtinterfaced.socket
  enable virtnetworkd.socket
  enable virtnodedevd.socket
  enable virtnwfilterd.socket
  enable virtsecretd.socket
  enable virtstoraged.socket

> In the past I've never added libvirt services/sockets to the openSUSE
> presets since users could easily enable libvirtd and virtlogd and be done
> with it. (Note the SLES presets do enable libvirtd and virtlogd.) But it
> becomes more burdensome with modular daemons, so I plan to add the above
> presets (excluding virtproxyd) to the openSUSE defaults.

Are you okay with old(ish) clients not being able to connect to the
host? Because that's why the virtproxyd.socket is enabled by default
in Fedora: if the client predates the introduction of split daemons
it will try to connect to the old libvirtd socket, and in a split
daemons deployment they only way to make that work is by having
virtproxyd listening on it.


[1] https://libvirt.org/daemons.html#switching-to-modular-daemons
[2] https://src.fedoraproject.org/rpms/fedora-release/blob/rawhide/f/90-default.preset
-- 
Andrea Bolognani / Red Hat / Virtualization

On 6/26/23 10:06, Andrea Bolognani wrote:
> On Fri, Jun 16, 2023 at 03:57:37PM -0600, Jim Fehlig wrote:
>> On 6/16/23 02:12, Martin Kletzander wrote:
>>> On Wed, Jun 14, 2023 at 04:45:06PM -0600, Jim Fehlig wrote:
>>>> On 6/9/23 03:05, Andrea Bolognani wrote:
>>>>> Can we somehow enforce that libvirt-daemon is fully configured before
>>>>> libvirt-daemon-proxy? If so, we could create a witness file based on
>>>>> the information we need in libvirt-daemon's %post, look at it in
>>>>> libvirt-daemon-proxy's %post and base our decision on that.
>>>>
>>>> What would the witness file indicate? As I understand, it would essentially have
>>>> to indicate whether libvirtd sockets/service are enabled. If so, couldn't that
>>>> be done directly in virtproxy post script? Something like the below hack?
> 
> The current logic in all packages' %post scriptlet basically
> translates to: if installing the package from scratch, apply the
> systemd presets; if upgrading, leave things well alone.
> 
> What I would want the witness file to indicate would be:
> libvirt-daemon is being upgraded, so when running the %post for
> libvirt-daemon-proxy behave as in the upgrade scenario as opposed to
> the install-from-scratch scenario.

Is it enough to know that libvirt-daemon is being upgraded? Do we also need to 
know if libvirtd.service and any of the sockets are enabled? I think Martin 
already suggested a solution along these lines

https://listman.redhat.com/archives/libvir-list/2023-June/240312.html

> Detecting whether the monolithic libvirtd and its sockets are enabled
> could also work, but feels more fragile. Specifically, it would not
> cover the scenario described by Martin, where the deployment is a
> split daemon one but with some units that are disabled by default
> based on presets have been manually enabled by the admin.

Nod. I even apologized to Martin in another response for getting bogged down on 
your issue :-).

> The problem is that I'm not sure we can create and process such a
> witness file reliably. Figuring that out requires research and
> real-life testing :)
>
>> In an attempt to reproduce Andrea's report, I added the following presets to
>> /usr/lib/systemd/system-preset/90-default-openSUSE.preset
>>
>> enable virtqemud.socket
>> enable virtlogd.socket
>> enable virtnetworkd.socket
>> enable virtnodedevd.socket
>> enable virtstoraged.socket
>> enable virtsecretd.socket
>> enable virtproxyd.socket
>>
>> As before, installed packages without commit b1da03b5b3. I then disabled all
>> the virt* presets (with exception of virtlogd) and enabled libvirtd.socket.
> 
> I'm not sure this is entirely relevant, but just for completeness'
> sake: in a monolithic daemon deployment, you want libvirtd.service in
> addition to its sockets to be enabled. This is needed to make sure
> domain autostart works as intended.

Thanks for the reminder! BTW, does the same apply for virtnetworkd and 
virtstoraged, which also have resources that can be autostarted?

> Basically, starting from a split daemon deployment, you want to
> follow the steps outlined here[1] but in reverse.
> 
>> After updating to packages containing commit b1da03b5b3, virtproxyd.socket
>> was still disabled and libvirt.socket was enabled. No problem connecting to
>> libvirtd, even after the service timeout.
> 
> I'll try to reproduce this myself later this week, but what you're
> describing doesn't match my expectations: virtproxyd.socket should
> have been enabled during installation.

I would have thought so too and was surprised by the results. That test was done 
in a VM. I just did the same test on the host (freshly updated Tumbleweed) with 
the same results.

> Are the packages you're using for testing a direct rebuild of the
> spec file shipped upstream? Or have you integrated the changes into
> the openSUSE spec file somehow?

I've made the changes to the openSUSE Factory spec file, which closely tracks 
the upstream one.

> Note that the scriptlets in the upstream spec file call out to some
> standard macros, and it's also possible that the implementation of
> said macros is not the same across Fedora and openSUSE.

The openSUSE scriptlets use the 'service_add_pre' macro from the 
systemd-rpm-macros package

https://build.opensuse.org/package/view_file/Base:System/systemd-rpm-macros/macros.systemd?expand=1

In the end, something like 'systemctl --no-reload preset $unit' is called.

>> Any tips on reproducing the issue? I must be missing something. What are the
>> libvirt-related presets in Fedora? I couldn't easily find them. FYI, the
>> Tumbleweed ones are here
>>
>> https://build.opensuse.org/package/show/Base:System/systemd-presets-common-SUSE
>> https://build.opensuse.org/package/show/Base:System/systemd-presets-branding-openSUSE
> 
> They're part of the fedora-release package[2].

Thanks for the pointer and info!

> Specifically, we have
> 
>    # Virtualization driver specific daemons. Start by default at boot for VM
>    # autostart, but shutdown after 2 mins and socket activated thereafter
>    enable virtqemud.service
>    enable virtxend.service
>    enable virtlxcd.service
> 
>    # Compatibility with libvirtd sockets for old clients and expose TCP sockets
>    enable virtproxyd.socket
> 
>    # Secondary drivers providing supporting functionality to main virtualization
>    # drivers, socket activated only when required
>    enable virtinterfaced.socket
>    enable virtnetworkd.socket
>    enable virtnodedevd.socket
>    enable virtnwfilterd.socket
>    enable virtsecretd.socket
>    enable virtstoraged.socket
> 
>> In the past I've never added libvirt services/sockets to the openSUSE
>> presets since users could easily enable libvirtd and virtlogd and be done
>> with it. (Note the SLES presets do enable libvirtd and virtlogd.) But it
>> becomes more burdensome with modular daemons, so I plan to add the above
>> presets (excluding virtproxyd) to the openSUSE defaults.
> 
> Are you okay with old(ish) clients not being able to connect to the
> host? Because that's why the virtproxyd.socket is enabled by default
> in Fedora: if the client predates the introduction of split daemons
> it will try to connect to the old libvirtd socket, and in a split
> daemons deployment they only way to make that work is by having
> virtproxyd listening on it.

Understood, and I'm fine with that on Tumbleweed for the time being. It would be 
interesting to hear complaints so I know there's demand for the use-case.

Regards,
Jim

On Mon, Jun 26, 2023 at 04:16:33PM -0600, Jim Fehlig wrote:
> On 6/26/23 10:06, Andrea Bolognani wrote:
> > The current logic in all packages' %post scriptlet basically
> > translates to: if installing the package from scratch, apply the
> > systemd presets; if upgrading, leave things well alone.
> >
> > What I would want the witness file to indicate would be:
> > libvirt-daemon is being upgraded, so when running the %post for
> > libvirt-daemon-proxy behave as in the upgrade scenario as opposed to
> > the install-from-scratch scenario.
>
> Is it enough to know that libvirt-daemon is being upgraded? Do we also need
> to know if libvirtd.service and any of the sockets are enabled? I think
> Martin already suggested a solution along these lines
>
> https://listman.redhat.com/archives/libvir-list/2023-June/240312.html

If we can detect this upgrade scenario reliably and behave
accordingly, I don't think looking at whether any specific service is
enabled or running would be necessary and, as I mentioned below, I
think this would make for an overall more solid approach. I'm just
still unclear on whether that's actually feasible :)

> > Detecting whether the monolithic libvirtd and its sockets are enabled
> > could also work, but feels more fragile. Specifically, it would not
> > cover the scenario described by Martin, where the deployment is a
> > split daemon one but with some units that are disabled by default
> > based on presets have been manually enabled by the admin.
>
> Nod. I even apologized to Martin in another response for getting bogged down
> on your issue :-).

I believe the two are actually one and the same, since they're both a
consequence of the package manager being convinced that virtproxyd
needs to be handled as a newly installed service when it's in fact
simply being upgraded.

> > I'm not sure this is entirely relevant, but just for completeness'
> > sake: in a monolithic daemon deployment, you want libvirtd.service in
> > addition to its sockets to be enabled. This is needed to make sure
> > domain autostart works as intended.
>
> Thanks for the reminder! BTW, does the same apply for virtnetworkd and
> virtstoraged, which also have resources that can be autostarted?

I'm actually not sure. The Fedora presets seem to indicate it does
not, but I can't really rule out that simply being a bug :)

> > > After updating to packages containing commit b1da03b5b3, virtproxyd.socket
> > > was still disabled and libvirt.socket was enabled. No problem connecting to
> > > libvirtd, even after the service timeout.
> >
> > I'll try to reproduce this myself later this week, but what you're
> > describing doesn't match my expectations: virtproxyd.socket should
> > have been enabled during installation.
>
> I would have thought so too and was surprised by the results. That test was
> done in a VM. I just did the same test on the host (freshly updated
> Tumbleweed) with the same results.

I have managed to reproduce the issue on Fedora 38, with the
following steps:

  * build the libvirt 9.0.0 RPMs from upstream sources and use them
    to create a very minimal QEMU installation consisting of just

      libvirt-client
      libvirt-daemon
      libvirt-daemon-driver-qemu
      libvirt-libs

  * reboot the machine, verify that a basic VM can be started;

  * turn the deployment into a monolithic one by running

      # systemctl disable --now virtqemud virtqemud{,-ro,-admin}.socket
      # systemctl mask virtqemud virtqemud{,-ro,-admin}.socket
      # systemctl disable --now virtproxyd virtproxyd{,-ro,-admin}.socket
      # systemctl mask virtproxyd virtproxyd{,-ro,-admin}.socket
      # systemctl unmask libvirtd libvirtd{,-ro,-admin}.socket
      # systemctl enable libvirtd libvirtd{,-ro,-admin}.socket

  * reboot the machine again and repeat the smoke test;

  * build the libvirt 9.1.0 RPMs from upstream sources and upgrade.
    As expected, this will result in a few new packages being added
    to the system:

      libvirt-client
      libvirt-daemon
      libvirt-daemon-common
      libvirt-daemon-driver-qemu
      libvirt-daemon-lock
      libvirt-daemon-log
      libvirt-daemon-plugin-lockd
      libvirt-daemon-proxy
      libvirt-libs

  * reboot the machine and try again.

Things will actually still work! But there's a somewhat subtle reason
for that ;)

During the upgrade, the following was printed:

    Running scriptlet: libvirt-daemon-proxy-9.1.0-1.fc38.x86_64
    Installing       : libvirt-daemon-proxy-9.1.0-1.fc38.x86_64
    Running scriptlet: libvirt-daemon-proxy-9.1.0-1.fc38.x86_64
  Failed to preset unit: Unit file
/etc/systemd/system/virtproxyd.socket is masked.

That's because I had *masked* all the split daemons in addition to
disabling them, as suggested on [1]. If I take a more naive approach
and simply disable the units, then I get

    Running scriptlet: libvirt-daemon-proxy-9.1.0-1.fc38.x86_64
    Installing       : libvirt-daemon-proxy-9.1.0-1.fc38.x86_64
    Running scriptlet: libvirt-daemon-proxy-9.1.0-1.fc38.x86_64
  Created symlink
/etc/systemd/system/sockets.target.wants/virtproxyd.socket →
/usr/lib/systemd/system/virtproxyd.socket.

during upgrade and after a reboot libvirtd is unusable, manifesting
the issue I was talking about.

Now, we could simply tell users that they need to disable things
properly O:-) but that's clearly not very helpful. It also wouldn't
do anything to solve Martin's issue.

Interestingly, I also get

    Running scriptlet: libvirt-daemon-log-9.1.0-1.fc38.x86_64
    Installing       : libvirt-daemon-log-9.1.0-1.fc38.x86_64
    Running scriptlet: libvirt-daemon-log-9.1.0-1.fc38.x86_64
  Removed "/etc/systemd/system/sockets.target.wants/virtlogd.socket".

    Running scriptlet: libvirt-daemon-lock-9.1.0-1.fc38.x86_64
    Installing       : libvirt-daemon-lock-9.1.0-1.fc38.x86_64
    Running scriptlet: libvirt-daemon-lock-9.1.0-1.fc38.x86_64
  Removed "/etc/systemd/system/sockets.target.wants/virtlockd.socket".

because, just as is the case for virtproxyd, the package manager is
convinced that these are completely new services and so it applies
the presets for them. However, libvirtd.service and virtqemud.service
both contain

  [Unit]
  Requires=virtlogd.socket
  Wants=virtlockd.socket

  [Install]
  Also=virtlogd.socket
  Also=virtlockd.socket

which I guess ultimately takes precedence over the preset, and the
sockets end up remaining enabled after all. Regardless, it would be
probably a good idea to apply whatever approach we end up taking for
virtproxyd to these two services too.

> > Are the packages you're using for testing a direct rebuild of the
> > spec file shipped upstream? Or have you integrated the changes into
> > the openSUSE spec file somehow?
>
> I've made the changes to the openSUSE Factory spec file, which closely
> tracks the upstream one.
>
> > Note that the scriptlets in the upstream spec file call out to some
> > standard macros, and it's also possible that the implementation of
> > said macros is not the same across Fedora and openSUSE.
>
> The openSUSE scriptlets use the 'service_add_pre' macro from the
> systemd-rpm-macros package
>
> https://build.opensuse.org/package/view_file/Base:System/systemd-rpm-macros/macros.systemd?expand=1
>
> In the end, something like 'systemctl --no-reload preset $unit' is called.

There might be some additional nuance that we're missing. I will try
to play around with openSUSE and understand the situation better
tomorrow.


[1] https://libvirt.org/daemons.html#switching-to-modular-daemons
-- 
Andrea Bolognani / Red Hat / Virtualization

On Tue, Jun 27, 2023 at 09:21:55AM -0700, Andrea Bolognani wrote:
> On Mon, Jun 26, 2023 at 04:16:33PM -0600, Jim Fehlig wrote:
> > On 6/26/23 10:06, Andrea Bolognani wrote:
> > > Note that the scriptlets in the upstream spec file call out to some
> > > standard macros, and it's also possible that the implementation of
> > > said macros is not the same across Fedora and openSUSE.
> >
> > The openSUSE scriptlets use the 'service_add_pre' macro from the
> > systemd-rpm-macros package
> >
> > https://build.opensuse.org/package/view_file/Base:System/systemd-rpm-macros/macros.systemd?expand=1
> >
> > In the end, something like 'systemctl --no-reload preset $unit' is called.
>
> There might be some additional nuance that we're missing. I will try
> to play around with openSUSE and understand the situation better
> tomorrow.

Yup, that was exactly it!

I'll omit a bunch of details for readability and because I have to
get off the computer shortly :) but you should still get the gist.


In Fedora (and I guess upstream systemd) the %systemd_post RPM macro
is defined as

  if [ $1 -eq 1 ]; then \
    systemd-update-helper install-system-units %{?*}; \
  fi

where the install-systemd-units command is implemented as

  systemctl --no-reload preset "$@"

So whenever a new package is installed, the preset is going to be
applied to all the units passed to the macro; subsequent runs of the
scriptlet, during upgrade, will leave things alone.


In openSUSE (I'm talking about Tumbleweed specifically, but Leap
implements the same idea, just in a slightly different way) things
work differently: the %service_add_pre and %service_add_post macros
call

  systemd-update-helper mark-install-system-units %{?*}

and

  systemd-update-helper install-system-units %{?*}

respectively, and those two helper commands are implemented as

  for unit in "$@" ; do
    if [ ! -e /usr/lib/systemd/system/"$unit" ]; then
      touch /run/systemd/rpm/needs-preset/"$unit"
    fi
  done

and

  for unit in "$@" ; do
    if [ -e /run/systemd/rpm/needs-preset/"$unit" ]; then
      rm /run/systemd/rpm/needs-preset/"$unit"
      systemctl --no-reload preset "$unit"
    fi
  done


So at this point it should be obvious why you were unable to
reproduce the issue on openSUSE: while in Fedora the decision about
whether to apply the presets to a unit is based entirely upon whether
the package that ships it is being installed from scratch or is an
update, openSUSE looks at whether the unit existed on the machine
before the package was installed and only applies presets for units
that are newly introduced.

Well, that, and having all units disabled by default in the presets,
of course :)


It seems to me that the openSUSE approach is far superior to the
Fedora / upstream one, because as we've seen it deals gracefully with
units being moved between packages. And that's not the only scenario:
if we were to introduce a new unit to any existing package, for
example, Fedora would not enable it by default regardless of presets.

In the immediate future, I think we should look into implementing a
similar logic to that of openSUSE in the libvirt spec. This would
take care of solving both of the issue that I have reported and the
one that Martin has.

In the longer run, I think it would make a lot of sense to advocate
for this approach to be adopted in Fedora and systemd upstream, which
would both reduce our maintenance burden and ensure as many other
projects as possible can benefit from it.

-- 
Andrea Bolognani / Red Hat / Virtualization

On 6/30/23 10:58, Andrea Bolognani wrote:
> On Tue, Jun 27, 2023 at 09:21:55AM -0700, Andrea Bolognani wrote:
>> On Mon, Jun 26, 2023 at 04:16:33PM -0600, Jim Fehlig wrote:
>>> On 6/26/23 10:06, Andrea Bolognani wrote:
>>>> Note that the scriptlets in the upstream spec file call out to some
>>>> standard macros, and it's also possible that the implementation of
>>>> said macros is not the same across Fedora and openSUSE.
>>>
>>> The openSUSE scriptlets use the 'service_add_pre' macro from the
>>> systemd-rpm-macros package
>>>
>>> https://build.opensuse.org/package/view_file/Base:System/systemd-rpm-macros/macros.systemd?expand=1
>>>
>>> In the end, something like 'systemctl --no-reload preset $unit' is called.
>>
>> There might be some additional nuance that we're missing. I will try
>> to play around with openSUSE and understand the situation better
>> tomorrow.
> 
> Yup, that was exactly it!
> 
> I'll omit a bunch of details for readability and because I have to
> get off the computer shortly :) but you should still get the gist.
> 
> 
> In Fedora (and I guess upstream systemd) the %systemd_post RPM macro
> is defined as
> 
>    if [ $1 -eq 1 ]; then \
>      systemd-update-helper install-system-units %{?*}; \
>    fi
> 
> where the install-systemd-units command is implemented as
> 
>    systemctl --no-reload preset "$@"
> 
> So whenever a new package is installed, the preset is going to be
> applied to all the units passed to the macro; subsequent runs of the
> scriptlet, during upgrade, will leave things alone.
> 
> 
> In openSUSE (I'm talking about Tumbleweed specifically, but Leap
> implements the same idea, just in a slightly different way) things
> work differently: the %service_add_pre and %service_add_post macros
> call
> 
>    systemd-update-helper mark-install-system-units %{?*}
> 
> and
> 
>    systemd-update-helper install-system-units %{?*}
> 
> respectively, and those two helper commands are implemented as
> 
>    for unit in "$@" ; do
>      if [ ! -e /usr/lib/systemd/system/"$unit" ]; then
>        touch /run/systemd/rpm/needs-preset/"$unit"
>      fi
>    done
> 
> and
> 
>    for unit in "$@" ; do
>      if [ -e /run/systemd/rpm/needs-preset/"$unit" ]; then
>        rm /run/systemd/rpm/needs-preset/"$unit"
>        systemctl --no-reload preset "$unit"
>      fi
>    done

I followed the paper trail here and copied that 'systemctl' command in a 
previous response, but shame on my laziness and not looking at the pre macro to 
see the bigger picture. Once again, thanks for the snooping.

> So at this point it should be obvious why you were unable to
> reproduce the issue on openSUSE: while in Fedora the decision about
> whether to apply the presets to a unit is based entirely upon whether
> the package that ships it is being installed from scratch or is an
> update, openSUSE looks at whether the unit existed on the machine
> before the package was installed and only applies presets for units
> that are newly introduced.
> 
> Well, that, and having all units disabled by default in the presets,
> of course :)
> 
> 
> It seems to me that the openSUSE approach is far superior to the
> Fedora / upstream one, because as we've seen it deals gracefully with
> units being moved between packages. And that's not the only scenario:
> if we were to introduce a new unit to any existing package, for
> example, Fedora would not enable it by default regardless of presets.

Yep, understood. The openSUSE approach is more flexible and avoids per-package 
hacks to handle such scenarios.

> In the immediate future, I think we should look into implementing a
> similar logic to that of openSUSE in the libvirt spec. This would
> take care of solving both of the issue that I have reported and the
> one that Martin has.

Agreed. Were you planning to work on that? If you don't have time in the near 
future, I should have some cycles next week after the US holiday.

> In the longer run, I think it would make a lot of sense to advocate
> for this approach to be adopted in Fedora and systemd upstream, which
> would both reduce our maintenance burden and ensure as many other
> projects as possible can benefit from it.

Agree with this too, but not volunteering to work on it :-).

Regards,
Jim

On Fri, Jun 30, 2023 at 01:46:05PM -0600, Jim Fehlig wrote:
> On 6/30/23 10:58, Andrea Bolognani wrote:
> > In the immediate future, I think we should look into implementing a
> > similar logic to that of openSUSE in the libvirt spec. This would
> > take care of solving both of the issue that I have reported and the
> > one that Martin has.
>
> Agreed. Were you planning to work on that? If you don't have time in the
> near future, I should have some cycles next week after the US holiday.

Yeah, I'll take care of it. It only makes sense, considering that
Fedora/RHEL are affected by the issue and openSUSE/SLES aren't.

> > In the longer run, I think it would make a lot of sense to advocate
> > for this approach to be adopted in Fedora and systemd upstream, which
> > would both reduce our maintenance burden and ensure as many other
> > projects as possible can benefit from it.
>
> Agree with this too, but not volunteering to work on it :-).

Fair enough :)

-- 
Andrea Bolognani / Red Hat / Virtualization

On Mon, Jul 03, 2023 at 08:28:16AM +0000, Andrea Bolognani wrote:
> On Fri, Jun 30, 2023 at 01:46:05PM -0600, Jim Fehlig wrote:
> > On 6/30/23 10:58, Andrea Bolognani wrote:
> > > In the immediate future, I think we should look into implementing a
> > > similar logic to that of openSUSE in the libvirt spec. This would
> > > take care of solving both of the issue that I have reported and the
> > > one that Martin has.
> >
> > Agreed. Were you planning to work on that? If you don't have time in the
> > near future, I should have some cycles next week after the US holiday.
>
> Yeah, I'll take care of it. It only makes sense, considering that
> Fedora/RHEL are affected by the issue and openSUSE/SLES aren't.

Done.

https://listman.redhat.com/archives/libvir-list/2023-July/240723.html

-- 
Andrea Bolognani / Red Hat / Virtualization

On 6/9/23 03:05, Andrea Bolognani wrote:
> On Thu, Jun 08, 2023 at 12:35:45PM -0600, Jim Fehlig wrote:
>> On 6/8/23 08:52, Andrea Bolognani wrote:
>>> On Wed, Jun 07, 2023 at 04:31:36PM +0200, Martin Kletzander wrote:
>>>> +# Since this was split into a different package, a transparent update for the
>>>> +# virtproxyd units could actually disable an already configured ones
>>>> +# (e.g. virtproxyd-tls.socket) as %systemd_post runs `systemctl preset` if this
>>>> +# is an installation (and is skipped on update).  So skip this step for those
>>>> +# that need an extra setup to work since they will most likely not be preset to
>>>> +# enabled, but that is up to the point of the distribution.
>>>> +%libvirt_daemon_systemd_post virtproxyd
>>>
>>> It's actually worse than that: if you are using the monolithic daemon
>>> on a distro that uses split daemons by default (e.g. Fedora),
>>
>> Why use the monolithic and split daemons together? Shouldn't we discourage
>> such configuration? :-)
> 
> Whether to use monlithic or split daemons is ultimately a choice of
> the local admin. Fedora defaults to split daemons, but switching back
> to a "classic" monolithic deployment is still a fully supported
> scenario.

Nod. Having both installed and using one or the other is fine. Not sure why I 
was thinking both were being used at the same time, but I think we can agree 
that is unwise :-).

> Additionally, the current default has been adopted relatively
> recently, and we have made the explicit decision *not* to migrate
> existing installations over. So if your OS was originally installed
> before split daemons had become the default and you've been dutifully
> upgrading to subsequent Fedora releases without ever reinstalling,
> then you're also going to be using the monolithic daemon.

Yes, it's the same for Leap or Tumbleweed users.

> 
>>> Basically we need to detect if we're installing the
>>> libvirt-daemon-proxy package as part of an upgrade and *not touch
>>> anything* if that's the case. I'm not sure how that can be achieved
>>> in the context of RPM scriptlets though, or if it's even possible :(
>>
>> It's possible to determine a package install vs upgrade, but IIUC the
>> problem can occur when installing or upgrading libvirt-daemon-proxy. The
>> usual 'if [ $1 -ge 2 ]' for upgrade-only actions wont work in that case.
> 
> Yeah, exactly: it's easy to detect whether a single package is being
> upgraded or installed from scratch, but in this case we would need to
> know whether libvirt-daemon is being upgraded in the scriptlet for
> libvirt-daemon-proxy, which I don't think is possible.

It may be possible using trigger scriptlets

http://ftp.rpm.org/api/4.4.2.2/triggers.html

I need to finish another task before looking in more detail. If anyone picks 
this up in the meantime please let me know.

> Can we somehow enforce that libvirt-daemon is fully configured before
> libvirt-daemon-proxy? If so, we could create a witness file based on
> the information we need in libvirt-daemon's %post, look at it in
> libvirt-daemon-proxy's %post and base our decision on that.

We may be able to accomplish this with trigger scriptlets and state files

https://docs.fedoraproject.org/en-US/packaging-guidelines/Scriptlets/#_saving_state_between_scriptlets

>> Can we skip the 'libvirt_daemon_systemd_post_inet' if virtproxyd is enabled?
>> Or adjust the macro to not 'systemd_post' sockets that are already enabled?
> 
> Sockets *and* services. We don't want virtproxyd.service to be
> started on boot if libvirtd.service will also be.

As a side note, I see virtproxyd service and sockets 'Conflicts' with libvirtd 
counterparts.

Regards,
Jim

On 6/7/23 16:31, Martin Kletzander wrote:
> Since virtproxyd was split into libvirt-daemon-proxy package it can
> happen that, in case a distribution has such systemd preset, when
> installing this package, already pre-enabled and configured units like
> -tls.socket and -tcp.socket will get disabled.
> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2210058
> Fixes: 5358618b1cd0afc126aed313249bf2134731665f
> Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
> ---
> This is more like an RFC as I would really like to know what to really do in
> this case.  What happens, basically, is that if you have libvirt-daemon-9.0.0
> and set up virtproxyd-tls.socket for example and then upgrade to anything newer,
> then the package libvirt-daemon-proxy will get installed.  The %post action
> calls "%libvirt_daemon_systemd_post_inet virtproxyd" which calls "%systemd_post
> with all virtproxyd units.  What %systemd_post is supposed to do is reset units
> to a preset state in the case of package installation, but not during upgrade.
> However the libvirt-daemon-proxy package did not exist on the system before, so
> this action is not an update, but an installation.
> 
> If no preset is mentioned for a unit then `systemctl preset` does not change
> anything.  However some distros might have a catch-all preset "disable *" for
> some reason, I guess based on an example in the documentation, and there is no
> way to override an already configured preset, you can only enable or disable a
> unit in a preset.
> 
> That all means than it can happen that you enable virtproxyd-tcp.socket, for
> example, then update your system and find that it is disabled.  There are
> various ways to deal with this, but I don't see any one that would 100% satisfy
> me with regards to all the issues and at the same time could be implemented
> "soon enough" given libvirt already had three releases with the
> libvirt-daemon-proxy split.
> 
>  libvirt.spec.in | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/libvirt.spec.in b/libvirt.spec.in
> index 1f77cd90b772..50f521b7ce88 100644
> --- a/libvirt.spec.in
> +++ b/libvirt.spec.in
> @@ -1592,7 +1592,13 @@ fi
>  
>  %post daemon-proxy
>  %if %{with_modular_daemons}
> -%libvirt_daemon_systemd_post_inet virtproxyd
> +# Since this was split into a different package, a transparent update for the
> +# virtproxyd units could actually disable an already configured ones
> +# (e.g. virtproxyd-tls.socket) as %systemd_post runs `systemctl preset` if this
> +# is an installation (and is skipped on update).  So skip this step for those
> +# that need an extra setup to work since they will most likely not be preset to
> +# enabled, but that is up to the point of the distribution.
> +%libvirt_daemon_systemd_post virtproxyd
>  %endif
>  
>  %preun daemon-proxy

I think this is sensible approach. Although at this point it may be a
bit late (at least for bleeding edge distros). But hey, slower distros
may still benefit from this change. You can count on my:

Reviewed-by: Michal Privoznik <mprivozn@redhat.com>

Michal

On Thu, Jun 08, 2023 at 09:55:28AM +0200, Michal Prívozník wrote:
>On 6/7/23 16:31, Martin Kletzander wrote:
>> Since virtproxyd was split into libvirt-daemon-proxy package it can
>> happen that, in case a distribution has such systemd preset, when
>> installing this package, already pre-enabled and configured units like
>> -tls.socket and -tcp.socket will get disabled.
>>
>> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2210058
>> Fixes: 5358618b1cd0afc126aed313249bf2134731665f
>> Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
>> ---
>> This is more like an RFC as I would really like to know what to really do in
>> this case.  What happens, basically, is that if you have libvirt-daemon-9.0.0
>> and set up virtproxyd-tls.socket for example and then upgrade to anything newer,
>> then the package libvirt-daemon-proxy will get installed.  The %post action
>> calls "%libvirt_daemon_systemd_post_inet virtproxyd" which calls "%systemd_post
>> with all virtproxyd units.  What %systemd_post is supposed to do is reset units
>> to a preset state in the case of package installation, but not during upgrade.
>> However the libvirt-daemon-proxy package did not exist on the system before, so
>> this action is not an update, but an installation.
>>
>> If no preset is mentioned for a unit then `systemctl preset` does not change
>> anything.  However some distros might have a catch-all preset "disable *" for
>> some reason, I guess based on an example in the documentation, and there is no
>> way to override an already configured preset, you can only enable or disable a
>> unit in a preset.
>>
>> That all means than it can happen that you enable virtproxyd-tcp.socket, for
>> example, then update your system and find that it is disabled.  There are
>> various ways to deal with this, but I don't see any one that would 100% satisfy
>> me with regards to all the issues and at the same time could be implemented
>> "soon enough" given libvirt already had three releases with the
>> libvirt-daemon-proxy split.
>>
>>  libvirt.spec.in | 8 +++++++-
>>  1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/libvirt.spec.in b/libvirt.spec.in
>> index 1f77cd90b772..50f521b7ce88 100644
>> --- a/libvirt.spec.in
>> +++ b/libvirt.spec.in
>> @@ -1592,7 +1592,13 @@ fi
>>
>>  %post daemon-proxy
>>  %if %{with_modular_daemons}
>> -%libvirt_daemon_systemd_post_inet virtproxyd
>> +# Since this was split into a different package, a transparent update for the
>> +# virtproxyd units could actually disable an already configured ones
>> +# (e.g. virtproxyd-tls.socket) as %systemd_post runs `systemctl preset` if this
>> +# is an installation (and is skipped on update).  So skip this step for those
>> +# that need an extra setup to work since they will most likely not be preset to
>> +# enabled, but that is up to the point of the distribution.
>> +%libvirt_daemon_systemd_post virtproxyd
>>  %endif
>>
>>  %preun daemon-proxy
>
>I think this is sensible approach. Although at this point it may be a
>bit late (at least for bleeding edge distros). But hey, slower distros
>may still benefit from this change. You can count on my:
>
>Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
>

Thank you, I'll give Jim (original author) and others some time to chime
in too.

>Michal
>