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.

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

fix(redfish): Make redfish storage name in URI dynamic. #49

Merged
merged 10 commits into from
Nov 23, 2023

Conversation

dashmage
Copy link
Contributor

@dashmage dashmage commented Nov 17, 2023

The redfish storage name in the URI was initially hardcoded to "Storage". On some servers which did not conform to the schema specification, this name was provided differently, eg: Storages.

This change aims to find the storage uri name dynamically while fetching the storage controller and storage drive data.

Fixes canonical/hardware-observer-operator#91, canonical/hardware-observer-operator#108

The redfish storage name in the URI was initially hardcoded to "Storage".
On some servers which did not conform to the schema specification, this
name was provided differently, eg: Storages.

This change aims to find the storage uri name dynamically while fetching
the storage controller and storage drive data.

Fixes #91, #108
@dashmage
Copy link
Contributor Author

dashmage commented Nov 17, 2023

There's duplicate code between the get_storage_drive_data and the get_storage_controller_data methods while navigating the URIs and fetching the storage related data. Currently trying to refactor the common code into a separate method.

Update: This refactoring work seems to be much more complicated than on first glance and there needs to be some thought behind the design. One server may have many different system ids each with multiple storage ids and each of those could have many storage controllers/ storage drives. Will tackle this in another PR if necessary in order to avoid increasing this PRs scope too much.

New issue raised here: #51

tests/unit/test_redfish.py Outdated Show resolved Hide resolved
prometheus_hardware_exporter/collectors/redfish.py Outdated Show resolved Hide resolved
prometheus_hardware_exporter/collectors/redfish.py Outdated Show resolved Hide resolved
@dashmage dashmage marked this pull request as ready for review November 17, 2023 10:58
@dashmage dashmage requested review from a team, Pjack, aieri, agileshaw, jneo8 and rgildein November 17, 2023 10:58
Also add unit test for new method.
tests/unit/test_redfish.py Outdated Show resolved Hide resolved
tests/unit/test_redfish.py Outdated Show resolved Hide resolved
tests/unit/test_redfish.py Outdated Show resolved Hide resolved
tests/unit/test_redfish.py Show resolved Hide resolved
tests/unit/test_redfish.py Outdated Show resolved Hide resolved
tests/unit/test_redfish.py Outdated Show resolved Hide resolved
tests/unit/test_redfish.py Outdated Show resolved Hide resolved
tests/unit/test_redfish.py Outdated Show resolved Hide resolved
- Remove API call assertions.
- Remove systems id related assertions
Copy link

@Pjack Pjack left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@jneo8 jneo8 left a comment

Choose a reason for hiding this comment

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

Just some nitpick of naming and comment.

prometheus_hardware_exporter/collectors/redfish.py Outdated Show resolved Hide resolved
prometheus_hardware_exporter/collectors/redfish.py Outdated Show resolved Hide resolved
prometheus_hardware_exporter/collectors/redfish.py Outdated Show resolved Hide resolved
prometheus_hardware_exporter/collectors/redfish.py Outdated Show resolved Hide resolved
- Update system_dict name.
- make `systems_root_uri` a class var.
- make `_storage_root_uri` a classmethod.
Copy link
Contributor

@sudeephb sudeephb left a comment

Choose a reason for hiding this comment

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

LGTM

@dashmage dashmage merged commit 03a17a7 into canonical:main Nov 23, 2023
3 checks passed
@dashmage dashmage deleted the dynamic_storage_name branch November 23, 2023 09:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants