Fan Health Incorrectly Set to "OK" When Unable to Read Property

FarahRasheed1 commented 1 month ago

Is this the right place to submit this?

[X] This is not a security vulnerability or a crashing bug
[X] This is not a question about how to use OpenBMC
[X] This is not a bug in an OpenBMC fork or a bug in code still under code review.
[X] This is not a request for a new feature.

Bug Description

When the system is unable to read the property for fan health, the health status is incorrectly set to "OK" (default set here https://github.com/openbmc/bmcweb/blob/master/redfish-core/lib/fan.hpp#L228). This can lead to misleading information being displayed to the user, as the actual health status of the fan is unknown and should be set to "Critical" in case of an error.

Steps to Reproduce:

Use a system that has a sensor, dimm, or fan that is "absent."
Navigate to Hardware status -> Inventory and LEDs screen in the webui-vue GUI to check the health and state.
Notice that even for "absent" dimm, the health is set to OK which is misleading.

Reason for the error: The health is defaulted to OK on https://github.com/openbmc/bmcweb/blob/master/redfish-core/lib/fan.hpp#L228, but never updated to Critical if unable to retrieve the fan health later in getFanHealth (https://github.com/openbmc/bmcweb/blob/master/redfish-core/lib/fan.hpp#L238-L247) if the property can not be retrieved via a DBUS call.

Version

67c9d4e715c705cd05fd04f7c8cd4fad300a4666

Additional Information

No response

edtanous commented 1 month ago

What platform and openbmc commit SHA1 did you test this on? There are several fan backends, so it's very likely whichever one you're using is the cause of the bug you're seeing.

Latest bcmweb

Your commit points to a non permalink, so "master" doesn't answer the question. Please provide the openbmc sha1 that you tested on.

Use a system that has a sensor, dimm, or fan that is "absent."

How does one accomplish this?

Notice that even for "absent" dimm, the health is set to OK which is misleading.

This on its own doesn't denote a bug. A fan might be absent and that is expected. That decision is up to the fan backend to make.

FarahRasheed1 commented 1 month ago

There are two different fields here in Status: the Health field and the State field. The State is being reported as Absent (which is expected). The health, though, is defaulted to OK initially, and not being updated if the call to dbus sends an error message back (which would imply the health is not ok). If there is a dbus error, what should the field be set to? Is health of OK fine in this case since that is what bmcweb reports back when it doesn't get a response from the backend?

I'll update the bug report with the sha1.

edtanous commented 1 month ago

What platform and openbmc commit SHA1 did you test this on?

Asking this again, because you haven't answered the platform question either, which is arguably more important because we're most likely looking at a fan backend problem here.

edtanous commented 1 month ago

This is not a bug in an OpenBMC fork or a bug in code still under code review.

Despite this box being checked, discussions with @FarahRasheed1 on gerrit showed that this was in fact a bug in a fork. If we can reproduce this on an OpenBMC build SHA, I'm happy to reopen, but complex system/interface issues like this are highly platform and config dependent, so we would need the full set of code available somewhere for anyone to hope to start looking into this.

openbmc / bmcweb