Package: sysstat
Version: 12.7.5-2
Tags: patch
User: [email protected]
Usertags: origin-ubuntu oracular ubuntu-patch

When systemd presets are applied, the various systemd services shipped
by sysstat become enabled, even if the debconf sysstat/enable setting
and /etc/default/sysstat asks for them to be disabled.

This happens, for example, if the user prepares a "golden image" and
then boots it, resulting in /etc/machine-id being recreated and systemd
running the equivalent of `systemctl preset-all`. Then the sysstat
services will be enabled even though /etc/default/sysstat says
ENABLED="no".

Further, the manage_systemd_services() implementation in
sysstat.postinst calls systemctl directly instead of using
deb-systemd-invoke, thus bypassing user choices via policy-rc.d.

To fix both of these issues at once, I suggest that you instead use
ExecCondition in the systemd services to implement the behaviour
specified through the debconf sysstat/enable setting and the ENABLED
variable in /etc/default/sysstat.

This way, dh_installsystemd is used as normal, so the various complex
interactions between maintainer scripts and user choices, with respect
to enable, disable, presets, policy-rc.d, etc will all work correctly as
expected.

Please see the attached patch, which is my current work in progress.
I've been careful to use precisely the same logic as used in the
existing maintainer scripts to interpret /etc/default/sysstat, to ensure
that edge-case behaviour does not change.

In Ubuntu, we started to install sysstat by default in 24.04, which is
why this issue came up. On every fresh installation of Ubuntu 24.04,
/etc/default/sysstat is mismatching the real state of the services. I
intend to fix this issue in Ubuntu shortly using something like the
attached patch, together with upgrade path handling. In order to avoid
unnecessary divergence, I'd appreciate discussion if you'd like to
approach the fix for this bug in Debian differently.

Thanks,

Robie

diff --git a/debian/patches/series b/debian/patches/series
index afceded..507edfe 100644
--- a/debian/patches/series
+++ b/debian/patches/series
@@ -11,3 +11,4 @@
 14-simtest-run-all.patch
 15-sa2-bash.patch
 16-empty-dirs.patch
+service-conditional-on-setting
diff --git a/debian/patches/service-conditional-on-setting 
b/debian/patches/service-conditional-on-setting
new file mode 100644
index 0000000..a2bc5a0
--- /dev/null
+++ b/debian/patches/service-conditional-on-setting
@@ -0,0 +1,27 @@
+--- a/cron/sysstat-collect.service.in
++++ b/cron/sysstat-collect.service.in
+@@ -13,5 +13,6 @@
+ [Service]
+ Type=oneshot
+ User=@CRON_OWNER@
++ExecCondition=sh -c 'ENABLED=false; [ -r /etc/default/sysstat ] && . 
/etc/default/sysstat; test "$${ENABLED}" = "true"'
+ ExecStart=@SA_LIB_DIR@/sa1 1 1
+ 
+--- a/cron/sysstat-summary.service.in
++++ b/cron/sysstat-summary.service.in
+@@ -12,4 +12,5 @@
+ [Service]
+ Type=oneshot
+ User=@CRON_OWNER@
++ExecCondition=sh -c 'ENABLED=false; [ -r /etc/default/sysstat ] && . 
/etc/default/sysstat; test "$${ENABLED}" = "true"'
+ ExecStart=@SA_LIB_DIR@/sa2 -A
+--- a/sysstat.service.in
++++ b/sysstat.service.in
+@@ -13,6 +13,7 @@
+ Type=oneshot
+ RemainAfterExit=yes
+ User=@CRON_OWNER@
++ExecCondition=sh -c 'ENABLED=false; [ -r /etc/default/sysstat ] && . 
/etc/default/sysstat; test "$${ENABLED}" = "true"'
+ ExecStart=@SA_LIB_DIR@/sa1 --boot
+ 
+ [Install]
diff --git a/debian/sysstat.postinst b/debian/sysstat.postinst
index 870d678..6ae366e 100644
--- a/debian/sysstat.postinst
+++ b/debian/sysstat.postinst
@@ -44,42 +44,6 @@ EOF
     rm -f "$def_file"
 }
 
-manage_systemd_services()
-{
-    ENABLED="$1"
-    all_services='sysstat-collect.timer sysstat-summary.timer sysstat.service'
-    num_all_services=3
-
-    # deb-systemd-helper does not have the --now option
-    sysctl='/bin/systemctl'
-    num_disabled=0
-
-    [ -x "$sysctl" ] && [ -d /run/systemd/system ] || return 0
-
-    for service in $all_services; do
-        "$sysctl" --quiet is-enabled "$service" || num_disabled=$(expr 
$num_disabled + 1)
-    done
-
-    if [ $num_disabled != 0 ] && [ $num_disabled !=  $num_all_services ]; then
-        # Do not override local administator choice to enable only some of 
systemd files
-        return 0
-    fi
-
-    [ $num_disabled = 0 ] && is_enabled="true" || is_enabled="false"
-    [ "$ENABLED" != "$is_enabled" ] || return 0
-
-    [ "$ENABLED" = "true" ] && enable_arg="enable" || enable_arg="disable"
-
-    for service in $all_services; do
-        case $service in
-            *.timer) options="--now" ;;
-            *)       options=""      ;;
-        esac
-
-        "$sysctl" "$enable_arg" $options "$service" >/dev/null 2>&1 || true
-    done
-}
-
 ### Main ###
 
 . /usr/share/debconf/confmodule
@@ -119,8 +83,6 @@ fi
 #DEBHELPER#
 
 if [ "$1" = "configure" ] && [ -n "$ENABLED" ]; then
-    manage_systemd_services "$ENABLED"
-
     # execute sa1 in a subshell not to clobber the postinst script with 
potentially
     # unsafe values from "$DEFAULT"
     if [ "$ENABLED" = "true" ] && [ -x /usr/libexec/sysstat/sa1 ] ; then

Reply via email to