Add X10QBi compatibility and a Platform abstraction for platform specific overrides

[samuel-emrys]

Overview

I've added compatibility for X10QBi motherboards, confirmed to work with the following configuration:

• Firmware 3.65.00
• BIOS Version: 3.1
• BMC: ASPEED2400
• Redfish Version: 1.0.1
• IPMI Revision: 2.0

Notably, this may not work on X10QBi boards with BIOS Version: 3.2, though I don't have the hardware necessary to confirm this.

This also introduces an abstraction that I've titled Platform that allows specific implementations of the ipmitool commands for boards that don't share compatibility with the existing ipmitool raw commands. I think most of the changes required are implemented, including unit testing and coverage. There are still a few things left to do before this is ready for merge, but I wanted to get your eyes on it sooner rather than later.

Still TODO:

• Update README to reflect X10QBi compatibility
• Update README to include X10QBi nuance commentary (like the fact that the valid fan controllers [zones] are referenced with addresses 0x10 (16), 0x11 (17), 0x12 (18) and 0x13 (19), rather than the default assumption at the moment of zone=0 or zone=1)
• Update README to include discussion on setting the new configuration parameter, [Ipmi.platform_name], and it's auto-discovery features if not set.
• Update config/smfc.conf to include [Ipmi.platform_name]
• Add log statements where necessary
• Potentially relocate test/test_02_ipmi.py::ipmi_exec_effect to somewhere more appropriate (test/test_02_data.py?) deferred pending feedback
• Fixup pylint omissions - docstrings etc
• Move set_fan_manual_mode() to be called internally from other set_* functions. My observation is that sometimes there are events that must reset these bits in the BMC. This means that smfc loses the ability to effect the changes required until the fan controllers are put back into manual mode. It would be more robust for smfc to ensure that the fan controllers are in manual mode for every attempt to modify fan pwm.

Any feedback you have is appreciated!

Summary of changes

• Add Platform abstract interface as the face of a strategy pattern to allow the underlying ipmitool raw executions to utilise platform-specific arguments where appropriate.
• Move existing functionality into a GenericPlatform subclass of the Platform interface, which will be the default behaviour.
• Add X10QBi subclass to the Platform interface to provide ipmitool raw commands specific to this platform.
• Add Ipmi.platform_name field to the configuration to allow users to select a platform specialisation. If not specified via the configuration file, smfc will now attempt to automatically discover the platform name and use the appropriate specialisation.
• Expand unit tests to encompass output verification of ipmitool sdr and ipmitool mc info to confirm that the output is parsed and ingested appropriately.
• Expand unit tests to include the platform_name as a parameter for most tests to ensure that both the Generic class and X10QBi class have coverage and meet expectations around behaviours.
• Refactor fan *_MODE variables into a FanMode IntEnum to avoid code duplication and silence pylint errors.
• Update README with guidance surrounding platform specific overrides to make it clear to the user how these overrides should be used.
• Update smfc.conf to include platform_name as an optional field to make it easy for users to identify that this is an option.
• Bump version to 4.3.0 to indicate new non-breaking feature.

Logs

Confirmation of platform type:

$ sudo ipmitool mc info
Device ID                 : 32
Device Revision           : 1
Firmware Revision         : 3.65
IPMI Version              : 2.0
Manufacturer ID           : 10876
Manufacturer Name         : Super Micro Computer Inc.
Product ID                : 1830 (0x0726)
Product Name              : X10QBi
Device Available          : yes
Provides Device SDRs      : no

Confirmation of working service:

$ sudo journalctl -u smfc.service -f
Nov 05 23:03:14 host smfc.service[10675]:    max_level = 100
Nov 05 23:03:14 host smfc.service[10675]:    hwmon_path = ['/sys/devices/platform/coretemp.0/hwmon/hwmon1/temp1_input', '/sys/devices/platform/coretemp.1/hwmon/hwmon2/temp1_input', '/sys/devices/platform/coretemp.2/hwmon/hwmon3/temp1_input', '/sys/devices/platform/coretemp.3/hwmon/hwmon4/temp1_input']
Nov 05 23:03:14 host smfc.service[10675]:    User-defined control function:
Nov 05 23:03:14 host smfc.service[10675]:    0. [T:30.0C - L:35%]
Nov 05 23:03:14 host smfc.service[10675]:    1. [T:36.7C - L:45%]
Nov 05 23:03:14 host smfc.service[10675]:    2. [T:43.3C - L:56%]
Nov 05 23:03:14 host smfc.service[10675]:    3. [T:50.0C - L:67%]
Nov 05 23:03:14 host smfc.service[10675]:    4. [T:56.7C - L:78%]
Nov 05 23:03:14 host smfc.service[10675]:    5. [T:63.3C - L:89%]
Nov 05 23:03:14 host smfc.service[10675]:    6. [T:70.0C - L:100%]
Nov 05 23:03:18 host smfc.service[10675]: CPU zone: new fan level > 46%/39.0C @ IPMI [16] zone(s).
Nov 05 23:11:19 host smfc.service[10675]: CPU zone: new fan level > 35%/33.0C @ IPMI [16] zone(s).
Nov 05 23:12:18 host smfc.service[10675]: CPU zone: new fan level > 46%/36.0C @ IPMI [16] zone(s).
Nov 05 23:14:23 host smfc.service[10675]: CPU zone: new fan level > 35%/33.0C @ IPMI [16] zone(s).
Nov 05 23:15:27 host smfc.service[10675]: CPU zone: new fan level > 46%/36.0C @ IPMI [16] zone(s).
Nov 05 23:18:18 host smfc.service[10675]: CPU zone: new fan level > 35%/33.0C @ IPMI [16] zone(s).
Nov 05 23:19:17 host smfc.service[10675]: CPU zone: new fan level > 46%/36.0C @ IPMI [16] zone(s).
Nov 05 23:22:24 host smfc.service[10675]: CPU zone: new fan level > 35%/33.0C @ IPMI [16] zone(s).
Nov 05 23:24:17 host smfc.service[10675]: CPU zone: new fan level > 46%/36.0C @ IPMI [16] zone(s).
Nov 05 23:28:28 host smfc.service[10675]: CPU zone: new fan level > 35%/33.0C @ IPMI [16] zone(s).
Nov 05 23:30:38 host smfc.service[10675]: CPU zone: new fan level > 46%/36.0C @ IPMI [16] zone(s).

I also confirmed visually that fan RPMs were changing appropriately using IPMIView.

Closes #96

[samuel-emrys]

What are your thoughts @petersulyok ?

[petersulyok]

Hi @samuel-emrys, I'm glad to see that you made it working for yourself.
Considering this PR I have the following things in my mind:

• this is an edge case with limited number of users
• I like your elegant Platform solution and still thinking on how it could be simpler/easier to test
• A question: can you clarify that the fans reach the 100% rotation speed if you configure 100 in the config? The reason why I'm asking is that for the X9 motherboards the level configuration was different.

[samuel-emrys]

Hi @petersulyok , relating to your question, I can confirm that setting the fan level to 100 in the config sets the fan to 100%. I kept the assumption that you had on the set_fan_level interface that the integers represented percentages. On the X10QBi, passing 100 (0x64) as a raw value sets PWM to ~39%. 100% PWM on the X10QBi is represented by 0xFF (255), so I normalise the input value here:

  def set_fan_level(
        self,
        ipmitool_fn: Callable[[List[str]], subprocess.CompletedProcess],
        zone: int,
        level: int,
    ) -> None:
        ... <snip>
        # Validate level parameter (must be in the interval [0..100%])
        validate_input_range(level, "level", 0, 100)

        # On the X10QBi, 100% is 0xFF (255), not 0x64 (100)
        normalised_level = level * 255 // 100

And you're right that it's an edge case with a limited number of users; my thought was that this approach could help you cater for a wider number of edge cases where there's platform specific logic for interacting with the BMC :)

I don't have any opinions on the unit testing; open to whatever improvements you think are appropriate.

[samuel-emrys]

Just an observation and some food for thought @petersulyok; part of the reason that testing needs to be verbose here is that the way the unit tests are currently structured is tied to the implementation rather than the interfaces. As the implementation changes, the unit tests need to be updated. One example of this is making an assertion for the number of times that ipmitool raw is dispatched.

I think this is a useful test during development but it makes a challenging test for regression - as the implementation changes, so does the assertion. I had to change this value a number of times as I was developing just to get things passing. This doesn't really provide a lot of value to confirm that I haven't broken things by modifying the way some functionality is implemented; an increase or decrease in the number of times ipmitool raw is called may be entirely sensible.

This is one example, but there are a few others as well. One way to simplify the unit test code would be to test only the behaviors that the interfaces demand, rather than the internals, which as mentioned, are tied to the implementation details. In other words, move to black box testing rather than white box testing.

I think there are practical limitations to doing this though; how do you black box test software designed to run on specific physical hardware? One possible answer is to mock the behavior of each bit of hardware but I'm not sure that would simplify things. I haven't thought about it too deeply so there's every chance there are meaningful interface behavior tests that can be implemented.

The other aspect is that moving to black box testing would be a pretty large refactor and I guess whether it's worth doing that depends on how much maintenance you foresee yourself doing in the future.

If you're not aware and want to read more about this philosophy of testing, it's called Behavior Driven Development.

[samuel-emrys]

Is this of interest @petersulyok? If not I'll close the PR and just maintain it on my fork.

[petersulyok]

Is this of interest @petersulyok? If not I'll close the PR and just maintain it on my fork.

Yes, I'm working on that. I played with different implementation strategies here. I try to merge this PR with slight refactoring to version 5.1.0. Thanks for your effort here.

My goal is to create a generic framework which could be extended later for more motherboards.

[samuel-emrys]

Thanks for sharing @petersulyok - if you would like me to make any changes just let me know :)

[petersulyok]

I merged this PR to the main branch preserving the abstract Platform design with some minor changes. The code is fully functional and tested, but some further design decisions will be needed on "platform_name=" parameter.
Please test it and let me know if it is working for you.

[samuel-emrys]

Thanks @petersulyok - I'll test when I next get an opportunity and let you know how it goes.

[petersulyok]

Thanks @petersulyok - I'll test when I next get an opportunity and let you know how it goes.

I saw here one thing I "fixed". The zone values are 0-3 (coming from configuration file) and mapped to 0x10-0x13 in the code.