Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Restart rhsm service after installation #3485

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

pkoprda
Copy link
Contributor

@pkoprda pkoprda commented Dec 18, 2024

  • Card ID: CCT-1027

Following the installation of the subscription-manager package, the dbus service was not loaded as expected. This update addresses the problem by ensuring that the dbus service is properly restarted after the installation or upgrade process.

@pkoprda
Copy link
Contributor Author

pkoprda commented Dec 18, 2024

I am not sure about the failing test 😿. However, here is a setup how you can test this change:

  1. Build RPM package from this commit (e.g. by using tito) - tito build --rpm --test - at least for me the RPM package was saved as /tmp/tito/subscription-manager-1.30.2-1.git.35.55c06d2.fc40.x86_64.rpm

  2. Upgrade subscription-manager package - dnf upgrade /tmp/tito/x86_64/subscription-manager-1.30.2-1.git.35.55c06d2.fc40.x86_64.rpm

  3. Then you can use the script get-environments.sh from jirihnidek/rhsm-dbus-examples repository (needs library.sh) to test this change:

#!/bin/bash

source ./library.sh

start_register_server
export username="admin"
export password="admin"
export org_id="content-sources-test"

echo "Trying to get list of organizations using dbus-send..."
dbus-send --address=${my_addr} --print-reply --dest='com.redhat.RHSM1.Register' \
       '/com/redhat/RHSM1/Register' \
       com.redhat.RHSM1.Register.GetEnvironments string:${username} \
       string:${password} \
       string:${org_id} \
       dict:string:string:"","" \
       string:""


stop_register_server

Comment on lines 684 to 685
%systemd_post rhsm.service
systemctl restart rhsm.service
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reference of the RPM macros: https://github.com/systemd/systemd/blob/46f881e32127cce03118dcd0471ded12d08d50b2/src/rpm/macros.systemd.in

%systemd_post in practice only installs the new service, and it does not do anything more than that

What seems to be needed here is %systemd_postun_with_restart, which seems to be marking the unit as "needs to be restarted", and then the rpm file triggers will make systemd restart the units marked as "no be restarted". Please note that the right place for that is in %postun, not %post.

* Card ID: CCT-1027

Following the installation of the subscription-manager package, the dbus
service was not loaded as expected. This update addresses the problem by
ensuring that the dbus service is properly restarted after the
installation or upgrade process.
Copy link
Contributor

@ptoscano ptoscano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the update!

The new macro does the trick, however only when the package with the scriptlet is removed, either because of uninstallation or upgrade. This means that when upgrading from a sub-man without this change to a sub-man with this change rhsm.service will not be restarted, since only the %preun of the already installed sub-man (i.e. that one without this change) is run.

As references to the RPM scriptlets, in particular the sections for syntax and ordering:

What I think we need here is to do the restart in %posttrans; according to the RPM macros for systemd (see the link I mentioned in my previous comment), %systemd_posttrans_with_restart should do it, and that needs to be placed in the %posttrans section.

@ptoscano
Copy link
Contributor

I am not sure about the failing test 😿.

The C10S job was fixed after #3495, so make sure to rebase.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants