handle different storage controller responses #58
Conversation
|
Can you provide the iLO version and the vendor (Inspur)'s official documentation for Redfish ? |
ILO version is 2.99. It returns storage controller information with Inspur servers have a different problem. When sending request to @rgildein I have made changes based on your reviews. I will push them when I complete unit tests. |
| """ | ||
| if "StorageControllers" in storage_controllers: | ||
| storage_controllers_list: List[Dict] = storage_controllers.get("StorageControllers") | ||
| elif "Controllers" in storage_controllers: |
There was a problem hiding this comment.
docs: Could we leave a comment here mentioning about the ILO version/ vendor that requires the code to handle the "Controllers" key?
| return cls.systems_root_uri + f"{system_id}/{storage_name}/" | ||
| return cls.systems_root_uri + f"{system_id}/{storage_name}" | ||
|
|
||
| def _find_storage_controller_data(self, storage_controllers: Dict) -> List[Dict]: |
There was a problem hiding this comment.
Naming the parameter here storage_controllers is a bit misleading. When called like
self._find_storage_controller_data(
self.redfish_obj.get(curr_storage_uri).dict
)
it would contain all the data from that particular storage id since curr_storage_uri would be /redfish/v1/Systems/s1/Storage/Stor1 for example. Maybe something along the lines of storage_data could sound more appropriate?
The if condition would also make more sense:
if "StorageControllers" in storage_data:
storage_controllers_list: List[Dict] = storage_data.get("StorageControllers")
| storage_controllers_list: List[Dict] = [] | ||
| controller_list = storage_controllers.get("Controllers") | ||
| for controller in controller_list.values(): | ||
| all_controller_data = self.redfish_obj.get(controller).dict |
There was a problem hiding this comment.
docs: It would also be nice to have a comment here to understand what we're expecting the controller_list to look like, to better understand why we need another nested for loop querying for the Members key.
Eg: Maybe we could mention that Controllers only returns a list of links to the storage controllers when fetched from Members. And to get individual storage controller data, we need to query via separate GET requests to the URI pointed by @odata.id?
| storage_controller_data_in_curr_system.append( | ||
| { | ||
| "storage_id": storage_id, | ||
| "controller_id": data["MemberId"], | ||
| "controller_id": data["MemberId"] if data.get("MemberId", None) else data.get("Id"), |
There was a problem hiding this comment.
docs: A comment here as well mentioning that Id is what we expect if the key being used is Controllers in the storage data would be helpful
| @@ -485,7 +485,7 @@ def test__storage_root_uri(self): | |||
| @patch( | |||
| "prometheus_hardware_exporter.collectors.redfish.redfish_utilities.systems.get_system_ids" | |||
| ) | |||
| def test_get_storage_controller_data_success( | |||
| def test_get_storage_controller_data_success_with_storagecontrollers( | |||
There was a problem hiding this comment.
nit: Maybe test_get_storage_controller_data_success and test_get_storage_controller_data_success_with_non_standard_api could be more descriptive test names?
|
Thanks @sdemircan for the work, this has been superseded by #71. Feel free to report further issues if you find any ! |
While checking storage controller data in get_storage_controller_data function API response varies between different ILO versions and different vendors(ie. Inspur).