Add a plugin to get information from ipmitool

[gthvn1]

Commands are:

• get_all_sensors: run ipmitool sdr list - returns a JSON string with all sensors
• get_sensor: run ipmitool sdr get <sensor> - returns details about sensors passed as parameter
• get_ipmi_lan: returns network info of the IPMI server as a JSON

[gthvn1]

@AtaxyaNetwork , do you think that it will be possible to test this plugin on a 2crsi server in order to check that output are correct? Thanks a lot for your great help.

[gthvn1]

For info, @AtaxyaNetwork tested it several weeks ago and it looks ok. Don't know if it needs to be checked with XO team but for me it looks ok to merge it. @stormi I added you as reviewer but feel free to add someone else :)

[stormi]

• Is ipmitool guaranteed to report information structured as expected by the code on every XCP-ng host? I see no error handling outside the automatic one that will report the exception raised by python. I just tried on a nested XCP-ng VM and got [15:51 xcpng-ci-82-b1 ~]# ipmitool sdr list Could not open device at /dev/ipmi0 or /dev/ipmi/0 or /dev/ipmidev/0: No such file or directory.
• Code changes to this repository must come with unit tests in the same repository. Ideally also covering error handling.

[gthvn1]

This PR supersedes #47

[gthvn1]

@stormi in case of error it raises a raise_plugin_error raised from decorator error_wrapped. Isn't what is expected? I thought it was the purpose of the decorator.

[gthvn1]

In the Readme I see:

Error: raise a XenAPIPlugin.Failure describing the error, to do that: either raise the XenAPIPlugin.Failure exception directly or use error_wrapped from xcpngutils"

[gthvn1]

• And about tests I will add them

[stormi]

In the Readme I see:

Error: raise a XenAPIPlugin.Failure describing the error, to do that: either raise the XenAPIPlugin.Failure exception directly or use error_wrapped from xcpngutils"

Yes, but then errors that mean nothing to users will be displayed to users in XO. We'll likely need something better than Could not open device at /dev/ipmi0 or /dev/ipmi/0 or /dev/ipmidev/0: No such file or directory. hidden in some XO logs when the feature doesn't work as expected.

[gthvn1]

In the Readme I see:

Error: raise a XenAPIPlugin.Failure describing the error, to do that: either raise the XenAPIPlugin.Failure exception directly or use error_wrapped from xcpngutils"

Yes, but then errors that mean nothing to users will be displayed to users in XO. We'll likely need something better than Could not open device at /dev/ipmi0 or /dev/ipmi/0 or /dev/ipmidev/0: No such file or directory. hidden in some XO logs when the feature doesn't work as expected.

You will have "Error parameters: Command '['ipmitool', 'sdr', 'list']' failed with code: 1, stdout: '', stderr: 'Could not open device at /dev/ipmi0 or /dev/ipmi/0 or /dev/ipmidev/0: No such file or directory". It is the error parameters returned from the wrapper. What should I put instead? I mean it is the error :)

[gthvn1]

Isn't XO that should handle this better? Not sure to see what you want instead of the "real error"?

[stormi]

Depends what the API user (XO) would like. I think they would appreciate it if we provided a way to discriminate between "not supported on this hardware" and unexpected errors on supported hardware, for example.

[gthvn1]

So we should add some XO people to review right?

[gthvn1]

and I'm not sure to know how we can discriminate between "not supported on this hardware" and unexpected errors on supported hardware.

[benjamreis]

The PR LGTM - I'd like some error cases tested as @stormi said.

Regarding the error report - IDK how this will be exposed in XO (I think we need more context) so hard to tell - if the error is directly presented to the user as is then a better error message would be preferrable but if its an internal thin in XO that will be procesed before giving a result then the translation can be done their as well.
It's not common for a XAPI plugin to do it usually.

[stormi]

Another argument for better feature support detection: I think "normal" errors will pollute host logs, so it would seem better to me if XO would first ask "do you have an IPMI device?" and then only if true asks for the data.

[gthvn1]

The PR LGTM - I'd like some error cases tested as @stormi said.

Yes it is planed. I'm waiting comment from XO guys (that I already contacted and I should have feedback this week) before implementing error case because I don't know yet what they are expected...

[gthvn1]

Another argument for better feature support detection: I think "normal" errors will pollute host logs, so it would seem better to me if XO would first ask "do you have an IPMI device?" and then only if true asks for the data.

Or as suggested before instead of a new function I can return an explicit string if I have an error "Could not open device" and only raise an exception for other errors. It won't cover all unexpected errors (maybe it will if the only "normal" error is the missing device) but it is an improvement.

[gthvn1]

After discussing with XO team (Mathieu) I will add a new method to ensure that hardware supports IPMI. This method is_ipmi_module_available will return a boolean (or a Json with true or false if we cannot return a boolean).
Now calling get all sensors, get sensor and Co will always raise a XenAPIPlugin Error in case of failure. It is up to you to check if IPMI is supported on your hardware by calling the new method.

[benjamreis]

What could be even better is still checking in every method that IPMI is available and if not raise a XenAPI.Failure explaining it's not.

[gthvn1]

Currently if IPMI is not available when running a command you will have this XenAPIplugin error:

Error code: 1
Error parameters: Command '['ipmitool', 'sdr', 'list']' failed with code: 1, stdout: '', stderr: 'Could not open device at /dev/ipmi0 or /dev/ipmi/0 or /dev/ipmidev/0: No such file or directory
', Traceback (most recent call last):
  File "/etc/xapi.d/plugins/xcpngutils/__init__.py", line 127, in wrapper
    return func(*args, **kwds)
  File "/etc/xapi.d/plugins/ipmitool.py", line 33, in sensor_data
    output = run_command(["ipmitool", "sdr", "list"])
  File "/etc/xapi.d/plugins/xcpngutils/__init__.py", line 79, in run_command
    raise ProcessException(code, command, stdout, stderr)
ProcessException: Command '['ipmitool', 'sdr', 'list']' failed with code: 1

So you suggest to replace the stderr of this error for something more clear? Because in this particular case the is_ipmi_device_available will return false so you are able to understand that the raise is related to your system that doesn't support IPMI. I think that if we raise an error we shouldn't modify its stderr and keep exactly what is returned by the command. I think that's why we added a new method to detect "normal" error.

[gthvn1]

• I still need to modify test to add is_ipmi_device_available.
• I updated the code with comment from @benjamreis about returning a better error message for known error.
• Add tests failure for all commands (passed, normal failed and abnormal failed)
• Add a test when the sensor name passed as parameter for sensor_info is wrong

[gthvn1]

All tests added. The last modification is when an error occured when trying to get information about a sensor that doesn't exist.

[gthvn1]

For information the last version of the plugin has been tested successfully by @AtaxyaNetwork 👍 on her machine.