Bug 2210058 - libvirt rpm scripts reset systemd units [NEEDINFO]
Summary: libvirt rpm scripts reset systemd units
Keywords:
Status: ASSIGNED
Alias: None
Product: Red Hat Enterprise Linux 9
Classification: Red Hat
Component: libvirt
Version: CentOS Stream
Hardware: x86_64
OS: Linux
unspecified
high
Target Milestone: rc
: ---
Assignee: Andrea Bolognani
QA Contact: Lili Zhu
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2023-05-25 15:22 UTC by lejeczek
Modified: 2023-08-09 12:13 UTC (History)
9 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2023-06-02 08:54:19 UTC
Type: Bug
Target Upstream Version: 9.6.0
Embargoed:
abologna: needinfo? (jdenemar)


Attachments (Terms of Use)


Links
System ID Private Priority Status Summary Last Updated
Red Hat Issue Tracker RHELPLAN-158263 0 None None None 2023-05-26 07:21:11 UTC

Description lejeczek 2023-05-25 15:22:07 UTC
Description of problem:

Hi.
I don't think it's a good idea to have package reset(or disable enabled units) upon/during rpm updates.
thanks, L.

Version-Release number of selected component (if applicable):

libvirt-daemon-driver-qemu-9.3.0-2.el9.x86_64

How reproducible:


Steps to Reproduce:
1.
2.
3.

Actual results:


Expected results:


Additional info:

Comment 1 Peter Krempa 2023-05-26 07:19:23 UTC
Could you please elaborate?

Which unit got disabled? What was the version you upgraded from? I presume 'libvirt-daemon-driver-qemu-9.3.0-2.el9.x86_64' is the one you upgraded to. Did you have any specific manual setup?

Comment 2 lejeczek 2023-05-26 18:12:15 UTC
virtproxyd-tls.socket I found disabled after the upgrade.

Comment 3 Martin Kletzander 2023-06-02 08:54:19 UTC
I can not reproduce this bug due to multiple reasons.  There's still too much info missing even after it was requested.  I still tried reproducing it the best I could, upgrading from 9.2.0-1.el9.x86_64.rpm:

[root@localhost ~]# systemctl enable --now virtproxyd-tls.socket
Created symlink /etc/systemd/system/sockets.target.wants/virtproxyd-tls.socket -> /usr/lib/systemd/system/virtproxyd-tls.socket.
[root@localhost ~]# systemctl status virtproxyd-tls.socket
* virtproxyd-tls.socket - Libvirt proxy TLS IP socket
     Loaded: loaded (/usr/lib/systemd/system/virtproxyd-tls.socket; enabled; preset: disabled)
     Active: active (listening) since Thu 2023-06-01 09:50:11 EDT; 1min 43s ago
      Until: Thu 2023-06-01 09:50:11 EDT; 1min 43s ago
   Triggers: * virtproxyd.service
     Listen: [::]:16514 (Stream)
      Tasks: 0 (limit: 48952)
     Memory: 8.0K
        CPU: 1ms
     CGroup: /system.slice/virtproxyd-tls.socket

Jun 01 09:50:11 localhost.localdomain systemd[1]: Listening on Libvirt proxy TLS IP socket.
[root@localhost ~]# ps -ef | grep dnf
root        4949    4663 20 09:51 pts/0    00:00:42 /usr/bin/python3 /usr/bin/dnf --enablerepo=* update -y --allowerasing
root       35474    4960  0 09:54 pts/1    00:00:00 grep --color=auto dnf
[root@localhost ~]# while killall -0 dnf; do sleep 1; done; systemctl status virtproxyd-tls.socket
dnf: no process found
* virtproxyd-tls.socket - Libvirt proxy TLS IP socket
     Loaded: loaded (/usr/lib/systemd/system/virtproxyd-tls.socket; enabled; preset: disabled)
     Active: active (listening) since Thu 2023-06-01 09:50:11 EDT; 6min ago
      Until: Thu 2023-06-01 09:50:11 EDT; 6min ago
   Triggers: * virtproxyd.service
     Listen: [::]:16514 (Stream)
      Tasks: 0 (limit: 48952)
     Memory: 8.0K
        CPU: 1ms
     CGroup: /system.slice/virtproxyd-tls.socket

Jun 01 09:50:11 localhost.localdomain systemd[1]: Listening on Libvirt proxy TLS IP socket.
[root@localhost ~]# rpm -q libvirt
libvirt-9.3.0-2.el9.x86_64

And it did not get disabled.

Comment 4 lejeczek 2023-06-02 09:24:48 UTC
Not in units, nothing custom. Version(s) I did mentioned.
Please ask specifically what you want me to provide - I'll try my best to help.

Comment 5 lejeczek 2023-06-02 09:28:29 UTC
Take a look at this, here is a snippet from '--scripts':
specifically for: 
...
postinstall scriptlet (using /bin/sh):
 
if [ $1 -eq 1 ] && [ -x "/usr/lib/systemd/systemd-update-helper" ]; then 
    # Initial installation 
    /usr/lib/systemd/systemd-update-helper install-system-units virtproxyd.socket virtproxyd-ro.socket virtproxyd-admin.socket virtproxyd-tls.socket virtproxyd-tcp.socket virtproxyd.service || : 
fi
...

Now, trying to reproduce that, manually:

-> $ /usr/lib/systemd/systemd-update-helper remove-system-units virtproxyd.service virtproxyd-ro.socket virtproxyd-admin.socket virtproxyd-tls.socket virtproxyd-tcp.socket virtproxyd.socket
Removed "/etc/systemd/system/sockets.target.wants/virtproxyd.socket".
Removed "/etc/systemd/system/sockets.target.wants/virtproxyd-tls.socket".

-> $ systemctl status -l virtproxyd-tls.socket 
○ virtproxyd-tls.socket - Libvirt proxy TLS IP socket
     Loaded: loaded (/usr/lib/systemd/system/virtproxyd-tls.socket; enabled; preset: disabled)

now:
-> $ systemctl daemon-reload  # which happens when os reboots, which was how I found it all.

-> $ systemctl status -l virtproxyd-tls.socket 
○ virtproxyd-tls.socket - Libvirt proxy TLS IP socket
     Loaded: loaded (/usr/lib/systemd/system/virtproxyd-tls.socket; disabled; preset: disabled)

Comment 6 Martin Kletzander 2023-06-02 12:45:40 UTC
(In reply to Peter Krempa from comment #1)
> Could you please elaborate?
> 
> Which unit got disabled?

this we know, that's virtproxyd-tls.socket

> What was the version you upgraded from?  I presume 'libvirt-daemon-driver-qemu-9.3.0-2.el9.x86_64' is the one you upgraded to.

Still missing the version you updated from.

> Did you have any specific manual setup?

Even properly filling in the bug template would answer this.

Comment 7 Martin Kletzander 2023-06-02 12:50:19 UTC
(In reply to lejeczek from comment #5)
> Take a look at this, here is a snippet from '--scripts':
> specifically for: 
> ...
> postinstall scriptlet (using /bin/sh):
>  
> if [ $1 -eq 1 ] && [ -x "/usr/lib/systemd/systemd-update-helper" ]; then 
>     # Initial installation 

This is only when installing previously not installed package.

>     /usr/lib/systemd/systemd-update-helper install-system-units
> virtproxyd.socket virtproxyd-ro.socket virtproxyd-admin.socket
> virtproxyd-tls.socket virtproxyd-tcp.socket virtproxyd.service || : 
> fi
> ...
> 
> Now, trying to reproduce that, manually:
> 
> -> $ /usr/lib/systemd/systemd-update-helper remove-system-units
> virtproxyd.service virtproxyd-ro.socket virtproxyd-admin.socket
> virtproxyd-tls.socket virtproxyd-tcp.socket virtproxyd.socket
> Removed "/etc/systemd/system/sockets.target.wants/virtproxyd.socket".
> Removed "/etc/systemd/system/sockets.target.wants/virtproxyd-tls.socket".
> 

You remove the units, effectively disabling them,

> -> $ systemctl status -l virtproxyd-tls.socket 
> ○ virtproxyd-tls.socket - Libvirt proxy TLS IP socket
>      Loaded: loaded (/usr/lib/systemd/system/virtproxyd-tls.socket; enabled;
> preset: disabled)
> 
> now:
> -> $ systemctl daemon-reload  # which happens when os reboots, which was how
> I found it all.
> 

then you reload the daemon,

> -> $ systemctl status -l virtproxyd-tls.socket 
> ○ virtproxyd-tls.socket - Libvirt proxy TLS IP socket
>      Loaded: loaded (/usr/lib/systemd/system/virtproxyd-tls.socket;
> disabled; preset: disabled)

and then the unit is disabled.  I don't see what you want to show here, sorry.

Comment 8 lejeczek 2023-06-02 14:07:46 UTC
What I showed were obvious snippets & I was saying that that what upgrade did/does.
Part(s) scriptlets probably happened, took place - I most likely was using penultimate release to this latest one - as I think I see 'libvirt-daemon-proxy' being a "brand-new" introduction package in the 9.3.0-2.el9

It reproduces in my lab - when I downgrade, repos supply me with 9.0.0-7.el9 version.
I have:
libvirt-libs-9.0.0-7.el9.x86_64
libvirt-client-9.0.0-7.el9.x86_64
libvirt-daemon-9.0.0-7.el9.x86_64
libvirt-daemon-driver-storage-core-9.0.0-7.el9.x86_64
libvirt-daemon-driver-network-9.0.0-7.el9.x86_64
libvirt-daemon-driver-nwfilter-9.0.0-7.el9.x86_64
libvirt-daemon-config-nwfilter-9.0.0-7.el9.x86_64
libvirt-daemon-config-network-9.0.0-7.el9.x86_64
libvirt-daemon-driver-storage-disk-9.0.0-7.el9.x86_64
libvirt-daemon-driver-storage-iscsi-9.0.0-7.el9.x86_64
libvirt-daemon-driver-storage-logical-9.0.0-7.el9.x86_64
libvirt-daemon-driver-storage-mpath-9.0.0-7.el9.x86_64
libvirt-daemon-driver-storage-rbd-9.0.0-7.el9.x86_64
libvirt-daemon-driver-storage-scsi-9.0.0-7.el9.x86_64
libvirt-daemon-driver-storage-9.0.0-7.el9.x86_64
libvirt-daemon-driver-interface-9.0.0-7.el9.x86_64
libvirt-daemon-driver-nodedev-9.0.0-7.el9.x86_64
libvirt-daemon-driver-qemu-9.0.0-7.el9.x86_64
libvirt-daemon-driver-secret-9.0.0-7.el9.x86_64
libvirt-9.0.0-7.el9.x86_64

I upgrade, having that "older" version up & running, both 'virtproxyd.socket' & 'virtproxyd-tls.socket' enabled & up and immediately - after successful update - I find: virtproxyd-tls.socket - DISABLED.

Comment 9 Martin Kletzander 2023-06-07 12:18:25 UTC
OK, now we're getting somewhere, thank you.

So virtproxyd was split from libvirt-daemon into libvirt-daemon-proxy since v9.1.0.  When updating libvirt from older version the new package libvirt-daemon-proxy gets installed.  And since from that package's point of view (or more precisely from the scriptlet) it is not visible that this is an update rather than an install.  So the scriptlet runs systemd_post which bubbles down to:

  systemd preset --no-reload preset virtproxyd.socket virtproxyd-ro.socket virtproxyd-admin.socet virtproxyd-tls.socket virtproxyd-tcp.socket virtproxyd.service

which would not happen on further updates.

However the system preset is to disable everything not explicitly listed as enabled (see /usr/lib/systemd/system-preset/99-default-disable.preset) and hence the enabled service gets disabled.

Since this is deciphered we can talk about the possibilities.

One thing would be to not use %systemd_post for this unit and maybe all other -tls.socket units.  That would violate the untold premise of using presets for systemd units.
Another could be to not include the "disable *" in the distro package.  I'm not sure what the reason is behind that and I'm not the right one to say.
The last one would be to "just let it go", but since RHEL 9.2 does not have the new package yet and we have some time until RHEL 9.3 is released, so I think one of the first two is the right way to go.

I'll try to propose the change upstream and will see where does the discussion take us.

Comment 11 Andrea Bolognani 2023-07-14 14:41:06 UTC
Patches posted upstream.

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

Comment 12 Andrea Bolognani 2023-07-27 16:20:49 UTC
Patches pushed upstream.

  commit a7bc8d16062c786eae298b3db3ee2f0d06b12151
  Author: Andrea Bolognani <abologna>
  Date:   Wed Jul 5 18:48:32 2023 +0200

    rpm: Switch to new macros for handling of systemd units
    
    In most cases the replacement is straightforward, with the
    biggest difference being that we now schedule restarts during
    %pre instead of %post. This also means that we can get rid of
    %post for most packages, reducing the number of scriptlets that
    need to run during install/upgrade.
    
    Notable exceptions are libvirt-guests.service, where we stop
    using the standard systemd macros to adopt our custom ones, as
    well as the virtlogd and virtlockd services, where the reload
    operation is moved from %postun to %posttrans.
    
    https://bugzilla.redhat.com/show_bug.cgi?id=2210058
    
    Signed-off-by: Andrea Bolognani <abologna>
    Reviewed-by: Martin Kletzander <mkletzan>

  commit 3bfc76a953c24793c2193768cca68d428490c01a
  Author: Andrea Bolognani <abologna>
  Date:   Wed Jul 5 18:07:34 2023 +0200

    rpm: Introduce new macros for handling of systemd units
    
    systemd provides a number of standard RPM macros but they don't
    quite satisfy our requirements, as evidenced by the fact that we
    have already built some custom tooling around them.
    
    Scenarios that the standard macros don't cover and that we're
    already addressing with our custom ones:
    
      * for some services (libvirtd, virtnetworkd, virtnwfilterd)
        there are multiple conditions that might lead to a restart,
        and we want to make sure that they're not needlessly
        restarted several times per transaction;
    
      * some services (virtlogd, virtlockd) must not be restarted
        during upgrade, so we have to reload them instead.
    
    Issues that neither the standard macros nor our custom ones
    address:
    
      * presets for units should be applied when the unit is first
        installed, not when the package that contains it is.
    
    The package split that happened in 9.1.0 highlighted why this
    last point is so important: when virtproxyd and its sockets
    were moved from libvirt-daemon to the new libvirt-daemon-proxy
    package, upgrades from 9.0.0 caused presets for them to be
    applied.
    
    On a platform such as Fedora, where modular daemons are the
    default, this has resulted in breaking existing deployments in
    at least two scenarios.
    
    The first one is machines that were configured to use the
    monolithic daemon, either because the local admin had manually
    changed the configuration or because the installation dated
    back to before modular daemons had become the default. In this
    case, virtproxyd.socket being enabled resulted in a silent
    conflict with libvirtd.socket, which by design shares the same
    path, and thus a completely broken setup.
    
    The second one is machines where virtproxy-tls.socket, which is
    disabled by default, had manually been enabled: in this case,
    applying the presets resulted in it being disabled and thus a
    loss of remote availability.
    
    Note that these are just two concrete scenarios, but the problem
    is more generic. For example, if we were to add more units to an
    existing package, per the current approach they wouldn't have
    their presets applied.
    
    The new macros are designed to avoid all of the pitfalls
    mentioned above. As a bonus, they're also simpler to use: where
    the current approach requires restarts and other operations to
    be handled separately, the new one integrates the two so that,
    for each scriptlet, a single macro call is needed.
    
    https://bugzilla.redhat.com/show_bug.cgi?id=2210058
    
    Signed-off-by: Andrea Bolognani <abologna>
    Reviewed-by: Martin Kletzander <mkletzan>

  v9.6.0-rc1-8-ga7bc8d1606


Note You need to log in before you can comment on or make changes to this bug.