Add controller, cache, and battery statuses to required sections #210
Conversation
There is a bug in the HPSSACLI parsing code that will not get the key-value pair for (a) properties that are at the end of a section or (b) properties that appear alone. In testing, controller status may appear alone and needs to be whitelisted by adding it to the required sections variable. In the event that cache status or battery status appear at the end of the section, I also added those to the whitelist. This fix is non-ideal, but it needs to appear in the next release, which is very soon. This issue may rear its head again later, so an overhaul of the HPSSACLI parsing routine may be appropriate. Signed-off-by: Blaine Gardner <[email protected]>
|
Some output showing the issue. ctrl all show status From HPSSACLI In hpsa.py hp_ctrl_status = {} # 'Controller Status' is absentctrl all show detail From HPSSACLI In hpsa.py ctrl_all_show = {'Smart HBA H244br in Slot 0 (Embedded) (RAID Mode)':
{'Slot': '0',
'Driver Name': 'hpsa',
'SATA NCQ Supported': 'True',
'Controller Memory Size': '256 MB',
'Encryption': 'Disabled',
'Expand Priority': 'Medium',
'Sanitize Erase Supported': 'True',
'Surface Scan Delay': '3 secs',
'Drive Write Cache': 'Disabled',
'Inconsistency Repair Policy': 'Disabled',
'Surface Scan Mode': 'Idle',
'Post Prompt Timeout': '15 secs',
'Controller Status': 'OK',
'Driver Supports HPE SSD Smart Path': 'True',
'Current Power Mode': 'MaxPerformance',
'Cache Board Present': 'False',
'Express Local Encryption': 'False',
'Queue Depth': 'Automatic',
'Hardware Revision': 'B',
'Serial Number': 'PDNMC%%LM8W07W',
'Negotiated PCIe Data Rate': 'PCIe 3.0 x4 (3940 MB/s)',
'Bus Interface': 'PCI',
'Cache Module Temperature (C)': '22',
'Wait for Cache Room': 'Disabled',
'Current Parallel Surface Scan Count': '1',
'Monitor and Performance Delay': '60 min',
'Latency Scheduler Setting': 'Disabled',
'Number of Ports': '1 Internal only',
'Cache Serial Number': 'PDNMC%%LM8W07W',
'Rebuild Priority': 'High',
'Controller Mode': 'RAID Mode',
'Firmware Version': '4.99-1',
'Spare Activation Mode': 'Activate on physical drive failure (default)',
'Elevator Sort': 'Enabled',
'Surface Analysis Inconsistency Notification': 'Disabled',
'Max Parallel Surface Scan Count': '16',
'Controller Temperature (C)': '43',
'Parallel Surface Scan Supported': 'Yes',
'Degraded Performance Optimization': 'Disabled',
'PCI Address (Domain:Bus:Device.Function)': '0000:04:00.0',
'Primary Boot Volume': 'None',
'Controller Mode Reboot': 'Not Required',
'Driver Version': '3.4.14'}
}
# 'Secondary Boot Volume' is absent |
|
@cathay4t We'll want you to take a look at this one and pull it in if you agree. Always helps to have a set of less biased eyes on this sort of change. :) |
|
@joehandzik Sure. I will take a look on this and perform some tests today. |
|
@BlaineEXE We might need to fix this again if any new entry was added to |
|
@cathay4t, this fix will still work if new entries are added to
For me it boils down to a few ideas. This fix is non-ideal, but it is functional for the time being. What really needs to happen is an entire overhaul of how HPSSACLI is parsed in order to collect lone properties. In the parsing code, it implicitly assumes that a line is a valid property if and only if it has another line below it with the same indentation level. It also hinges on the idea of I would propose that this fix be accepted now since it does fix the bug in 1.4.0. There is risk that future additions to libstoragemgmt may bring the root issues back to light, so work will begin after 1.4.0 release to overhaul the HPSSACLI parsing to improve robustness. |
|
I will try to fix the parser, it seems not that hard for me. If I failed to do so, I will take your PR. BTW, how hard is HPSSACLI to support output in JSON? MegaRAID already support JSON output for quit a while. |
|
+1 on adding json output |
|
@BlaineEXE I fixed the parser. Please test this patch instead cathay4t@a9a72eb |
|
@cathay4t, the patch you linked will fix only the case where a property is the very last item in HPSSACLI output (EOF). Although this is also an issue, it doesn't resolve the issue of properties being missed when they are alone or in at the end of a 'section' in the middle of HPSSACLI output. In my examples in the comment above, I simplified the output by copying only one of the sections. This is the full output: |
|
The HPSSACLI parsing code has technical debt. It needs to be overhauled. Coding the effort might take a few days, but to adequately test that the changes work on all configurations and don't introduce bugs is going to extend the effort to 2-3 weeks. Any patch we can generate now on the current code to fix issues (a) and (b) is going to be sufficiently complex that the fix will also add undue risk of introducing bugs, and it would be advisable to run the same 1-2 week test for the hotfix that would be done for the overhaul. The fix I've proposed is the least likely to introduce new bugs and fixes the only known occurrence of (b) while also fixing occurrences of (a) for controller battery status and cache status should they appear. (a) does not affect parsing of |
|
OK. Merged with issue #214 for future tracking. |
There is a bug in the HPSSACLI parsing code that will not get the
key-value pair for (a) properties that are at the end of a section or
(b) properties that appear alone.
In testing, controller status may appear alone and needs to be
whitelisted by adding it to the required sections variable. In the
event that cache status or battery status appear at the end of the
section, I also added those to the whitelist. This fix is non-ideal,
but it needs to appear in the next release, which is very soon.
This issue may rear its head again later, so an overhaul of the
HPSSACLI parsing routine may be appropriate.
Signed-off-by: Blaine Gardner [email protected]