(In reply to Alena from comment #0)
>    lm_sensor makes no sense in a VM, so running it provides pointless
> resource consumption.

It's not as simple as that. lm_sensors can still be useful inside a virtual machine, namely if you setup passthrough of some of the host devices.

For instance I can imagine a scenario where a user has set up a virtual machine with GPU passthrough and they monitor the GPU temperature from within the VM.

The lm_sensors service is a oneshot service (i.e., it's not a daemon) and it runs just a couple of quick commands. Also, the service can always be disabled using systemctl, or lm_sensors can be uninstalled. Why does the customer have lm_sensors installed in the first place?

I'm inclined to close this WONTFIX. It seems like a potentially breaking change with little benefit.

Closing per comment#1.

Can we please revisit this?

Coming here from bz1921633, where:
- we have pcp-pmda-lmsensors, which is recording sensor/fan readings into
  PCP archive files
- it relies on lmsensors, so we introduced a dependency on lmsensors
- which brought lm_sensors.service also to many virtual guests, where the
  unit is run on startup and producing errors.

microcode_ctl.service is a bit similar: it can not update microcode from
virtual guests.  ConditionVirtualization=false solves the issue there
nicely.

So the issue we want to solve in requesting ConditionVirtualization=false :
---
[root@rhel8u3a ~]# systemctl status lm_sensors
● lm_sensors.service - Hardware Monitoring Sensors
   Loaded: loaded (/usr/lib/systemd/system/lm_sensors.service; enabled; vendor preset: enabled)
   Active: failed (Result: exit-code) since Thu 2021-01-28 23:53:00 JST; 9h ago
  Process: 716 ExecStart=/usr/bin/sensors -s (code=exited, status=1/FAILURE)
  Process: 703 ExecStart=/usr/libexec/lm_sensors/lm_sensors-modprobe-wrapper $BUS_MODULES $HWMON_MODULES (code=exited, status=1/FAILURE)
 Main PID: 716 (code=exited, status=1/FAILURE)

Jan 28 23:52:59 rhel8u3a.local systemd[1]: Starting Hardware Monitoring Sensors...
Jan 28 23:52:59 rhel8u3a.local lm_sensors-modprobe-wrapper[703]: No sensors with loadable kernel modules configured.
Jan 28 23:52:59 rhel8u3a.local lm_sensors-modprobe-wrapper[703]: Please, run 'sensors-detect' as root in order to search for available sensors.
Jan 28 23:52:59 rhel8u3a.local sensors[716]: No sensors found!
Jan 28 23:52:59 rhel8u3a.local sensors[716]: Make sure you loaded all the kernel drivers you need.
Jan 28 23:52:59 rhel8u3a.local sensors[716]: Try sensors-detect to find out which these are.
Jan 28 23:53:00 rhel8u3a.local systemd[1]: lm_sensors.service: Main process exited, code=exited, status=1/FAILURE
Jan 28 23:53:00 rhel8u3a.local systemd[1]: lm_sensors.service: Failed with result 'exit-code'.
Jan 28 23:53:00 rhel8u3a.local systemd[1]: Failed to start Hardware Monitoring Sensors.
---

We also have many customers who have a single kickstart file/package
selection, which they deploy on both physical and virtual guests.
For them, lm_sensors.service dealing better with virtual guests would
also be good.

I agree with Ondrej that in some cases lmsensors.service might be
desired in virtual guests.  Could we then deploy for the majority
of deployments (with ConditionVirtualization=false), and have the
admin customize the service for that use case case?
Customization of systemd service units is well proven and documented.

*** Bug 1921633 has been marked as a duplicate of this bug. ***

Hi Christian, the solution I would like to propose is to introduce a flag, which will allow sensors to return 0 on no sensors detected.
However I would still print message in stderr that says:

No sensors found!
Make sure you loaded all the kernel drivers you need.
Try sensors-detect to find out which these are.

Will this solution work for customers?

I would like to leave this message, because it'll be a good debug point for people who actually want to run lm_sensors on VMs, but since it'll never fail it'll be hard for them to notice the problem without this message in log.

Hoi Artem,

>  I would like to propose is to introduce a flag, which will allow sensors to return 0 on no sensors detected.

With that, the service would lm_sensors.service not fail on guests, 
that can potentially solve the bz1921633 situation.
Where would you want to set the flag, what would be the default setting?
I would hope for that flag to be set by default.  In other words, an
admin of RHEL in virtual guests should not have to set this themself.

There might be situations when the customers would want to
see lm_sensors.service failing.

> However I would still print message in stderr that says:
>
> No sensors found!
> Make sure you loaded all the kernel drivers you need.
> Try sensors-detect to find out which these are.
>
> Will this solution work for customers?

Would modifying that need upstream work?
Are you open to tune that, so admins of virtual guests understand
that they might see the desired outcome?

> I would like to leave this message, because it'll be a good debug point
> for people who actually want to run lm_sensors on VMs, but since
> it'll never fail it'll be hard for them to notice the problem without
> this message in log.

That makes sense.
But then, we start relying on an error message being read by someone.
The error code is normally the more direct way to tell if something fails,
and triggering an admin to search for details.
Let me understand your flag idea first better.

> Let me understand your flag idea first better.

Idea is that when no sensors found the message will be printed in the error log (same as now) in any case. In case if the flag set the exit status will be 0 (1 in case flag is not set).
After introduction of that in code I'll modify the way systemd starts the lm_sensors service.

The idea is to create an init service for lm_sensors which will have ConditionVirtualization=vm (this init service will actually start only on virtual machines)
The init service will add the flag in file (?), which will be used by a regular lm_sensors service to get the startup flags.

In a VM environment the init service will be started and flag will be set. In a regular environment the service will be not started and no flag will be used.

I'm not sure what is the best way to implement the systemd layer of the solution, I'll do a research how it might be done and if this is done in some other package.

Does that make sense?

Summing up my understanding of the practical outcome if implemented,
on a RHEL deployment in the guest with lmsensors installed without 
manually touching files to set flags:

- on physical:
  - if sensors found, return 0
  - if sensors not found, return 1 + text
- on virtual:
  - if sensors found, return 0
  - if sensors not fount, return 0 + text

That should technically solve the current biggest issue,
the failing systemd unit.  
Following remarks:

- That solution frees users who to use lmsensors in the guest from
  having to create a modified systemd unit.
  I still think wonder if that additional complexity is wort it,
  as a small minority wants lmsensors in guests.  The complexity
  comes with danger of regressions.
- With the planned solution, virt-guest users do no longer see
  a failing systemd-service, that is good.

  The message in the logs still suggests the outcome might be bad:
  sensors[716]: No sensors found!
  sensors[716]: Make sure you loaded all the kernel drivers you need.
  sensors[716]: Try sensors-detect to find out which these are.

  Is that from us or from upstream?
  Ideally, we could modify it so it informs that the outcome
  of not finding sensors might be the expected outcome, which it
  is for 99% of virtual guests.

Maybe the use case of the solution you have in mind might also
deserve a new systemd unit option, like "MakeServiceNeverFail"
or such.
One implementation might me to have lmsensors.service run a wrapper,
which internally checks if it runs on virtual, and then handles
the return codes appropriately.  But having things run directly
run by systemd might be cleanest.

Actually, the whole topic might also be relevant for upstream,
not sure how active upstream is.

Upstream patch:
https://github.com/lm-sensors/lm-sensors/commit/a0ef84f6583dbf427ff5a3534528e1e72bd00137

Fedora patch:
https://src.fedoraproject.org/rpms/lm_sensors/c/a2bee3abb72af537635fcb94b51f652ca7684f5c?branch=rawhide

Christian, your understanding for the behavior is correct.

The solution is to start service using wrapper, which will determine startup flags using the /usr/bin/systemd-detect-virt

I'll create the same patch for RHEL.

Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory (lm_sensors bug fix and enhancement update), and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

https://access.redhat.com/errata/RHBA-2021:4353