[DTrace-devel] [PATCH] dtprobed: make sure the daemon is restarted

[Kris Van Hees]

On Fri, Feb 23, 2024 at 08:57:50PM +0000, Nick Alcock wrote:
> Unfortunately relying on the presets and a %systemd_postun_with_restart to
> enable and restart dtprobed is not enough: older installations used
> %systemd_postun, so when we upgrade from one of those, the daemon is not
> restarted.  The older installations themselves enabled and restarted it by
> hand.  Presets take the burden of enabling from us, but not the burden of
> restarting: we still have to do that, and doing it in %postun is only good
> enough when this is the package being upgraded *from*.
> 
> So forcibly restart it in %post (after %systemd_post installs its service
> files, etc).  This does mean that reinstallations may restart the daemon
> twice, but this is harmless, and in any case only applies to OL8 and below:
> OL9 amortizes all restarts and does them in %posttrans.
> 
> Doing this is tangled and ridiculous: the actual underlying method used to
> restart differs on OL8-and-before and on OL9+, but the
> %systemd_postun_with_restart macro does the right thing on both distros.
> Unfortunately because it's meant to run from %postun it checks for the wrong
> value of $1 (>= 1, meaning everything but install, while we want >= 2 which
> means the same thing in %post).  So wrap the whole thing in a conditional to
> prevent it double-restarting on new installations.
> 
> On OL8 and below we have another problem: the unit file has changed, but the
> systemd macros on OL8 and below don't reload it except on initial
> installation: and the change is essential because without it dtprobed
> doesn't have permission to write anything to /run/dtrace.  So on OL7 and
> OL8, do a daemon-reload before restarting.
> 
> The result appears to survive initial installation, upgrade from .12 and
> .13.1, and rpm --reinstall on both OL8 and OL9.
> 
> While we're changing this, take out the mention of dtrace-usdt.target in
> %systemd_preun: as a .target, it's meaningless to restart it or do anything
> else %preun does to it, and naming it there causes horrible warning messages
> on uninstallation.
> 
> Signed-off-by: Nick Alcock <nick.alcock at oracle.com>
> ---
>  dtrace.spec | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/dtrace.spec b/dtrace.spec
> index 278bdf53b7e1e..f583d59132e9e 100644
> --- a/dtrace.spec
> +++ b/dtrace.spec
> @@ -207,6 +207,20 @@ rm -rf $RPM_BUILD_DIR/%{name}-%{version}
>  /sbin/ldconfig
>  %udev_rules_update
>  %systemd_post dtprobed.service
> +# Force a daemon restart on upgrade even if the previous package did
> +# not use presets and did not request a restart on uninstallation.
> +# (In OL10, when upgrades from 1.13.1 and below are no longer possible,
> +# we can remove this.)

I would drop the "In OL10".  All that matters is the DTrace version.

> +# systemd_postun_with_restart does the right thing here, though we need
> +# to wrap it in an extra conditional to make it not run on install.
> +if [ $1 -gt 1 ] ; then

This is confusing because your commit msg mentions >= 2 (which is the same,
but appears confusing that you wouldn't use the same expression in both
places).

> +    # OL8 and below don't reload the unit file properly.
> +    %if "%{?dist}" == ".el7" || "%{?dist}" == ".el8"
> +    systemctl daemon-reload || :
> +    %endif
> +
> +    %systemd_postun_with_restart dtprobed.service
> +fi
>  
>  # if sdt-systemtap.h doesn't exist then we can move the existing dtrace sdt.h
>  SYSINCDIR=/usr/include/sys
> @@ -218,7 +232,7 @@ elif [ ! -e $SYSINCDIR/sdt.h ]; then
>  fi
>  
>  %preun
> -%systemd_preun dtprobed.service dtrace-usdt.target
> +%systemd_preun dtprobed.service
>  
>  %postun
>  /sbin/ldconfig
> -- 
> 2.43.0.272.gce700b77fd