Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

handle different storage controller responses #58

Closed
wants to merge 2 commits into from
+131 −9
Closed

handle different storage controller responses #58

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3bd80a3
handle different storage controller responses #57
sdemircan Dec 5, 2023
57a1b84
add unit tests for both type of storage controller responses
sdemircan Dec 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 30 additions & 7 deletions prometheus_hardware_exporter/collectors/redfish.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -221,7 +221,29 @@ def _storage_root_uri(cls, system_id: str, storage_name: str) -> str:
system_id: "S1"
storage_name" "Storage"
"""
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]:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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")

"""Return storage controller list.

Returns:
storage_controller_list: list of storage controllers for
requested storage id.

"""
if "StorageControllers" in storage_controllers:
storage_controllers_list: List[Dict] = storage_controllers.get("StorageControllers")
elif "Controllers" in storage_controllers:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

docs: Could we leave a comment here mentioning about the ILO version/ vendor that requires the code to handle the "Controllers" key?

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

for controller in all_controller_data["Members"]:
storage_controllers_list.append(self.redfish_obj.get(controller["@odata.id"]).dict)
else:
raise KeyError("Couldn't fetch storage controller data")

return storage_controllers_list

def get_storage_controller_data(self) -> Tuple[Dict[str, int], Dict[str, List]]:
"""Return storage controller data and count.
Expand Down Expand Up @@ -287,21 +309,22 @@ def get_storage_controller_data(self) -> Tuple[Dict[str, int], Dict[str, List]]:
for storage_id in storage_ids:
# eg: /redfish/v1/Systems/1/Storage/XYZ123
curr_storage_uri = (
RedfishHelper._storage_root_uri(system_id, storage_name) + storage_id
RedfishHelper._storage_root_uri(system_id, storage_name) + "/" + storage_id
)

# list of storage controllers for that storage id
storage_controllers_list: List[Dict] = self.redfish_obj.get(curr_storage_uri).dict[
"StorageControllers"
]
storage_controllers_list: List[Dict] = self._find_storage_controller_data(
self.redfish_obj.get(curr_storage_uri).dict
)
storage_controller_count[system_id] += len(storage_controllers_list)

# picking out the required data from each storage controller in the list
for data in storage_controllers_list:
print(data)
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"),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

"state": data["Status"]["State"],
"health": data["Status"]["Health"] or "NA",
}
Expand Down Expand Up @@ -455,7 +478,7 @@ def get_storage_drive_data(self) -> Tuple[Dict[str, int], Dict[str, List]]:
for storage_id in storage_ids:
# eg: /redfish/v1/Systems/1/Storage/XYZ123/
curr_storage_uri = (
RedfishHelper._storage_root_uri(system_id, storage_name) + storage_id
RedfishHelper._storage_root_uri(system_id, storage_name) + "/" + storage_id
)

# list of storage drives for that storage id
Expand Down
103 changes: 101 additions & 2 deletions tests/unit/test_redfish.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -474,7 +474,7 @@ def mock_get_response(uri):
def test__storage_root_uri(self):
"""Test RedfishHelper._storage_root_uri method."""
for storage_name in ["Storage", "Storages", "TestStorage"]:
expected_uri = f"/redfish/v1/Systems/S1/{storage_name}/"
expected_uri = f"/redfish/v1/Systems/S1/{storage_name}"
uri = RedfishHelper._storage_root_uri("S1", storage_name)
assert uri == expected_uri

Expand All @@ -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(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

self, mock_get_system_ids, mock_get_collection_ids, mock_redfish_client
):
mock_redfish_obj = Mock()
Expand Down Expand Up @@ -550,6 +550,105 @@ def mock_get_response(uri):
},
)

@patch("prometheus_hardware_exporter.collectors.redfish.redfish_client")
@patch(
"prometheus_hardware_exporter.collectors.redfish.redfish_utilities.collections.get_collection_ids" # noqa: E501
)
@patch(
"prometheus_hardware_exporter.collectors.redfish.redfish_utilities.systems.get_system_ids"
)

def test_get_storage_controller_data_success_with_controllers(
self, mock_get_system_ids, mock_get_collection_ids, mock_redfish_client
):
mock_redfish_obj = Mock()
mock_system_ids = ["s1"]
mock_storage_ids = ["STOR1", "STOR2"]

mock_get_system_ids.return_value = mock_system_ids
mock_redfish_client.return_value = mock_redfish_obj
mock_get_collection_ids.return_value = mock_storage_ids

def mock_get_response(uri):
response = Mock()
if uri.endswith("Systems/s1/Storage/STOR1"):
response.dict = {
"Controllers": {
"@odata.id": "/redfish/v1/Systems/s1/Storage/STOR1/Controllers"
}
}
elif uri.endswith("Systems/s1/Storage/STOR2"):
response.dict = {
"Controllers": {
"@odata.id": "/redfish/v1/Systems/s1/Storage/STOR2/Controllers"
}
}
elif uri.endswith("Systems/s1/Storage/STOR1/Controllers"):
response.dict = {
"Members": [
{
"@odata.id": "/redfish/v1/Systems/s1/Storage/STOR1/Controllers/sc0"
}
]
}
elif uri.endswith("Systems/s1/Storage/STOR2/Controllers"):
response.dict = {
"Members": [
{
"@odata.id": "/redfish/v1/Systems/s1/Storage/STOR2/Controllers/sc1"
}
]
}
elif uri.endswith("Systems/s1/Storage/STOR1/Controllers/sc0"):
response.dict = {
"Status": {
"State": "Enabled",
"Health": "OK",
},
"Id": "sc0"
}
elif uri.endswith("Systems/s1/Storage/STOR2/Controllers/sc1"):
response.dict = {
"Status": {
"State": "Enabled",
"Health": "OK",
},
"Id": "sc1"
}
# response for GET request to /redfish/v1/Systems/<sys_id>/
elif "Systems" in uri:
response.dict = {"Storage": {"@odata.id": "/redfish/v1/Systems/sX/Storage"}}
return response

mock_redfish_obj.get.side_effect = mock_get_response

with RedfishHelper(Mock()) as helper:
(
storage_controller_count,
storage_controller_data,
) = helper.get_storage_controller_data()

self.assertEqual(storage_controller_count, {"s1": 2})
self.assertEqual(
storage_controller_data,
{
"s1": [
{
"storage_id": "STOR1",
"controller_id": "sc0",
"health": "OK",
"state": "Enabled",
},
{
"storage_id": "STOR2",
"controller_id": "sc1",
"health": "OK",
"state": "Enabled",
},
]
}
)

@patch("prometheus_hardware_exporter.collectors.redfish.redfish_client")
@patch(
"prometheus_hardware_exporter.collectors.redfish.redfish_utilities.collections.get_collection_ids" # noqa: E501
Expand Down