[lm-sensors] [PATCH] systemd units rework

[lm-sensors] [PATCH] systemd units rework

[Jaromir Capik]

[-- Attachment #1: Type: text/plain, Size: 1378 bytes --]

Hello guys.

The attached patch fixes several things our users complained about.

The patch introduces 3 wrappers for the service files and example
/etc/sysconfig/* configuration files

The lm_sensors-modprobe*wrapper scriptlets change the way how
the modprobe failures look like as the users had no idea that
the failures are caused by missing configuration and that
led to frequent bug reports. The wrapper checks whether the
modprobe is called with any arguments and leaves a hint
message when no modules are passed to the modprobe.

Te sensord-service-wrapper enhances the sensord service so that
it supports all relevant switches needed by the users which
were previously omitted and thus unsupported.

The service files now contain a placeholder for the target
wrapper directory and distribution build scripts can do
the substitution easily with two sed calls ...

Example for RPM based distributions:
sed -i "s|\@WRAPPER_DIR\@|%{_libexecdir}/%{name}|" sensord.service
sed -i "s|\@WRAPPER_DIR\@|%{_libexecdir}/%{name}|" lm_sensors.service

The patch is applicable on the latest trunk.
Please, check it and apply if you find it worthy.

Thank you.

Regards,
Jaromir.

--
Jaromir Capik
Red Hat Czech, s.r.o.
Software Engineer / Secondary Arch

Email: jcapik@redhat.com
Web: www.cz.redhat.com
Red Hat Czech s.r.o., Purkynova 99/71, 612 45, Brno, Czech Republic
IC: 27690016 

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: systemd-units-rework.patch --]
[-- Type: text/x-patch; name=systemd-units-rework.patch, Size: 4122 bytes --]

Index: prog/init/lm_sensors-modprobe-r-wrapper
===================================================================
--- prog/init/lm_sensors-modprobe-r-wrapper	(revision 0)
+++ prog/init/lm_sensors-modprobe-r-wrapper	(working copy)
@@ -0,0 +1,8 @@
+#!/bin/sh
+if [ $# -ne 0 ]; then
+  /usr/sbin/modprobe -abr $@
+else
+  echo "No sensors with loadable kernel modules configured."
+  echo "Please, run 'sensors-detect' as root in order to search for available sensors."
+  exit 1
+fi
Index: prog/init/lm_sensors-modprobe-wrapper
===================================================================
--- prog/init/lm_sensors-modprobe-wrapper	(revision 0)
+++ prog/init/lm_sensors-modprobe-wrapper	(working copy)
@@ -0,0 +1,8 @@
+#!/bin/sh
+if [ $# -ne 0 ]; then
+  /usr/sbin/modprobe -ab $@
+else
+  echo "No sensors with loadable kernel modules configured."
+  echo "Please, run 'sensors-detect' as root in order to search for available sensors."
+  exit 1
+fi
Index: prog/init/lm_sensors.service
===================================================================
--- prog/init/lm_sensors.service	(revision 6214)
+++ prog/init/lm_sensors.service	(working copy)
@@ -1,13 +1,13 @@
 [Unit]
-Description=Initialize hardware monitoring sensors
+Description=Hardware Monitoring Sensors
 
 [Service]
 EnvironmentFile=/etc/sysconfig/lm_sensors
 Type=oneshot
 RemainAfterExit=yes
-ExecStart=-/sbin/modprobe -qab $BUS_MODULES $HWMON_MODULES
+ExecStart=-@WRAPPER_DIR@/lm_sensors-modprobe-wrapper $BUS_MODULES $HWMON_MODULES
 ExecStart=/usr/bin/sensors -s
-ExecStop=-/sbin/modprobe -qabr $BUS_MODULES $HWMON_MODULES
+ExecStop=-@WRAPPER_DIR@/lm_sensors-modprobe-r-wrapper $BUS_MODULES $HWMON_MODULES
 
 [Install]
 WantedBy=multi-user.target
Index: prog/init/lm_sensors.sysconfig
===================================================================
--- prog/init/lm_sensors.sysconfig	(revision 0)
+++ prog/init/lm_sensors.sysconfig	(working copy)
@@ -0,0 +1,2 @@
+# /etc/sysconfig/lm_sensors - Defines modules loaded by the lm_sensors service
+# Run 'sensors-detect' to generate this config file
Index: prog/init/sensord-service-wrapper
===================================================================
--- prog/init/sensord-service-wrapper	(revision 0)
+++ prog/init/sensord-service-wrapper	(working copy)
@@ -0,0 +1,13 @@
+#!/bin/sh
+
+. /etc/sysconfig/sensord
+
+ARGS=""
+[ "$INTERVAL" = "" ] || ARGS=`echo "$ARGS -i $INTERVAL"`
+[ "$LOG_INTERVAL" = "" ] || ARGS=`echo "$ARGS -l $LOG_INTERVAL"`
+[ "$RRD_INTERVAL" = "" ] || ARGS=`echo "$ARGS -t $RRD_INTERVAL"`
+[ "$RRD_LOGFILE" = "" ] || ARGS=`echo "$ARGS -r $RRD_LOGFILE"`
+[ "$RRD_NO_AVG" = "1" ] && ARGS=`echo "$ARGS -T"`
+[ "$LOAD_AVG" = "1" ] && ARGS=`echo "$ARGS -a"`
+
+/usr/sbin/sensord -f daemon $ARGS
Index: prog/init/sensord.service
===================================================================
--- prog/init/sensord.service	(revision 6214)
+++ prog/init/sensord.service	(working copy)
@@ -1,12 +1,12 @@
 [Unit]
-Description=Log hardware monitoring data
+Description=Hardware Monitoring Data Logger
 After=lm_sensors.service
 
 [Service]
-EnvironmentFile=/etc/sysconfig/sensord
+EnvironmentFile=-/etc/sysconfig/sensord
 Type=forking
 PIDFile=/var/run/sensord.pid
-ExecStart=/usr/sbin/sensord -i $INTERVAL -l $LOG_INTERVAL -f daemon
+ExecStart=@WRAPPER_DIR@/sensord-service-wrapper
 
 [Install]
 WantedBy=multi-user.target
Index: prog/init/sensord.sysconfig
===================================================================
--- prog/init/sensord.sysconfig	(revision 0)
+++ prog/init/sensord.sysconfig	(working copy)
@@ -0,0 +1,21 @@
+# configuration for harware sensors monitoring daemon
+# use suffix "m" for minutes, "s" for seconds, "h" for hours
+# 0 turns the facility off
+
+# interval between scanning alarms
+INTERVAL=1m
+
+# interval between logging
+LOG_INTERVAL=20m
+
+# interval between RRD logging
+# RRD_INTERVAL=1m
+
+# RRD db location
+# RRD_LOGFILE=/var/log/sensors.rrd
+
+# Switch RRD in non-average mode ... 1 - enabled, 0 - disabled (default)
+# RRD_NO_AVG=1
+
+# Include load average in RRD ... 1 - enabled, 0 - disabled (default)
+# LOAD_AVG=1

[-- Attachment #3: Type: text/plain, Size: 153 bytes --]

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] systemd units rework

[Jean Delvare]

Hi Jaromir,

Sorry for the late reply. I seem to recall we discussed this briefly
over IRC, but let me write down my updated views here so that the can be
discussed.

On Fri, 24 Jan 2014 08:56:16 -0500 (EST), Jaromir Capik wrote:
> The attached patch fixes several things our users complained about.

I get complaints on my side as well, that's even why I remembered this
old thread:
https://bugzilla.novell.com/show_bug.cgi?idˆ2719
https://bugzilla.novell.com/show_bug.cgi?idˆ2720

> The patch introduces 3 wrappers for the service files and example
> /etc/sysconfig/* configuration files
> 
> The lm_sensors-modprobe*wrapper scriptlets change the way how
> the modprobe failures look like as the users had no idea that
> the failures are caused by missing configuration and that
> led to frequent bug reports. The wrapper checks whether the
> modprobe is called with any arguments and leaves a hint
> message when no modules are passed to the modprobe.

I see which problem you are trying to solve with that. However I do not
like wrappers much. One of the key design goals of systemd was to avoid
running scripts every now and then, because scripts are slow. I am not
necessarily a big fan of systemd, but if you switch to it, you have to
do it right, otherwise you get the pain without the benefits.

Also, some hwmon drivers are loaded automatically. At this time these
are mostly CPU and GPU sensors, but I hope that in the future some
mainboard sensors drivers can be loaded automatically too. This means
that running sensors-detect will become optional (it already is on some
machines, laptops in particular.) The lack of configuration file should
thus not be a problem and should not prevent the service from starting.
It should also not spam the logs forever, which your wrappers scripts
are doing.

My plan to solve this specific issue is to have sensors-detect write a
file to /etc/modules-load.d instead of /etc/sysconfig. Systemd has a
service to load modules, I want to use that instead of doing it on my
own. Then the lm_sensors service itself would simply depend on
systemd-modules-load.service, and its only task would be to call
"sensors -s". I think this is elegant and it requires no wrapper script.

The only issue which my approach does not solve is that the user gets
no clue that running sensors-detect may get him/her additional sensor
values. I'm not sure that writing that message to the system log is the
best way anyway, both because of a lack of visibility and because it
keeps filling the system log. Mentioning it in sensors(1), sensord(8)
and as a comment in lm_sensors.Service itself might be more
appropriate. It can also be explained better on the lm-sensors wiki and
in distribution documentation pages / wiki.

> Te sensord-service-wrapper enhances the sensord service so that
> it supports all relevant switches needed by the users which
> were previously omitted and thus unsupported.

We (openSUSE) don't really have a need for that at the moment because we
only support two options: $INTERVAL (-i) and $LOG_INTERVAL (-l). Same
as the upstream service file supports, actually. These options can be
passed to sensord unconditionally, so all I had to do it put default
values in a template sysconfig file (which I agree is missing upstream,
I'll add it.)

If we ever decide to support extra options in openSUSE, then I think I
would rather add a generic variable EXTRA_OPTIONS
in /etc/sysconfig/sensord and let the user pass everything he/she needs
there. This way, no wrapper script is needed, and you don't have to
change anything if sensord gets new options added. In fact I would
question why things where not implemented that way from the beginning.

Anyway, before this happens, I think we want to clean the rrd-related
code in sensord a lot. It's way too fragile the way it is right now, any
change in the sensors setup will break the rrd logging. On my own
systems I am running separate instances of sensord for each sensor chip
to workaround this weakness, but I do not expect all users to do that,
and that can't be easily integrated in a generic way anyway. There must
be a better way to make sensord rrd databases handle device additions
or removal. I have been willing to tackle that issue for a long time
but I can never find the time to actually look into it :(

> The service files now contain a placeholder for the target
> wrapper directory and distribution build scripts can do
> the substitution easily with two sed calls ...
> 
> Example for RPM based distributions:
> sed -i "s|\@WRAPPER_DIR\@|%{_libexecdir}/%{name}|" sensord.service
> sed -i "s|\@WRAPPER_DIR\@|%{_libexecdir}/%{name}|" lm_sensors.service
> 
> The patch is applicable on the latest trunk.
> Please, check it and apply if you find it worthy.

I don't really want to apply it, sorry. This is one way of handling the
existing issues, but there are others, and I don't think that your
approach is the best one. I'm not saying mine is perfect either, they
are just different.

-- 
Jean Delvare
SUSE L3 Support

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors