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

Ledctl: add command to retrieve default controller #248

Merged
merged 9 commits into from
Oct 16, 2024

Conversation

ktanska
Copy link
Contributor

@ktanska ktanska commented Sep 2, 2024

Add new command to ledctl, which enables to print the default,
preferred available controller for given device. Output from this
can be helpful when user must specify controller in other
ledctl commands.
Add parameter tests for new command: --default-controller.
Use new method to choose default controller for device. It is especially
needed when VMD and NPEM are available on one platform. Skip IBPI test
when drive is connected to different default controller, than used to test.
Test multipath drives only with default controller connected.

@ktanska ktanska force-pushed the ktanska/issue189 branch 2 times, most recently from 7361edb to 95ea477 Compare September 2, 2024 14:24
@ktanska ktanska changed the title Ktanska/issue189 Ledctl: add command to retrieve best controller Sep 2, 2024
Copy link
Contributor

@mtkaczyk mtkaczyk left a comment

Choose a reason for hiding this comment

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

Few small comments, LGTM

src/ledctl/ledctl.c Outdated Show resolved Hide resolved
tests/ledctl/ledctl_cmd.py Outdated Show resolved Hide resolved
tests/ledctl/slot_test.py Outdated Show resolved Hide resolved
@ktanska ktanska force-pushed the ktanska/issue189 branch 4 times, most recently from 99cad22 to 08be01b Compare September 4, 2024 12:10
mtkaczyk
mtkaczyk previously approved these changes Sep 10, 2024
Copy link
Contributor

@bkucman bkucman left a comment

Choose a reason for hiding this comment

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

Hi,
please add description of new option to ledctl manual and new entry to ledctl help.

src/ledctl/ledctl.c Outdated Show resolved Hide resolved
src/ledctl/help.c Outdated Show resolved Hide resolved
src/ledctl/ledctl.c Outdated Show resolved Hide resolved
bkucman
bkucman previously approved these changes Sep 18, 2024
Copy link
Contributor

@mtkaczyk mtkaczyk left a comment

Choose a reason for hiding this comment

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

Sorry, I just realized that we have to cover this in unit library tests:
https://github.com/intel/ledmon/blob/main/tests/lib_unit_test.c

doc/ledctl.pod Outdated Show resolved Hide resolved
@ktanska
Copy link
Contributor Author

ktanska commented Sep 25, 2024

Sorry, I just realized that we have to cover this in unit library tests: https://github.com/intel/ledmon/blob/main/tests/lib_unit_test.c

I see that lib_unit_test script uses only slot feature. I've retested it and problem did not appear.

mtkaczyk
mtkaczyk previously approved these changes Sep 27, 2024
@mtkaczyk
Copy link
Contributor

mtkaczyk commented Oct 2, 2024

@peluse could you please look? Especially for wording, I'm not sure if "best" is the appropriate.
maybe "default" would be better?

The feature is addressing is a scenario where multiple controllers are detected for one disk (NPEM, VMD).
The routine proposed here allows user to query ledctl to return used controller by default (aka. best) , you don't have to determine it manually.

Add new command to ledctl, which enables to print the best
available controller for given device. Output from this
can be helpful when user must specify controller in other
ledctl commands.
Fixes intel#189

Signed-off-by: Kinga Stefaniuk <kinga.stefaniuk@intel.com>
Add parameter tests for new command: --best-controller.
Use new method to choose best controller for device. It is especially
needed when VMD and NPEM are available on one platform. Skip IBPI test
when drives are connected to better controller, than used to test.
Test multipath drives only with best controller connected.

Signed-off-by: Kinga Stefaniuk <kinga.stefaniuk@intel.com>
Add checking if controller used to find slot is not filtered out. It is
needed to be checked for tests which are using drives instead of slots.

Signed-off-by: Kinga Stefaniuk <kinga.stefaniuk@intel.com>
- Print "unsupported controller type" instead of "?" when ledctl is not
  able to find supported controller for device
- Add help option for "--best-controller" command
- Align tests to new help mode

Signed-off-by: Kinga Stefaniuk <kinga.stefaniuk@intel.com>
Describe "--best-controller" option in ledctl manual.

Signed-off-by: Kinga Stefaniuk <kinga.stefaniuk@intel.com>
Fix formatting error reported by YAPF.

Signed-off-by: Kinga Stefaniuk <kinga.stefaniuk@intel.com>
- change message for device without best controller

Signed-off-by: Kinga Stefaniuk <kinga.stefaniuk@intel.com>
Update ledctl.pod.

Signed-off-by: Kinga Stefaniuk <kinga.stefaniuk@intel.com>
@ktanska ktanska requested a review from mtkaczyk October 8, 2024 13:25
@ktanska ktanska changed the title Ledctl: add command to retrieve best controller Ledctl: add command to retrieve default controller Oct 8, 2024
mku514k
mku514k previously approved these changes Oct 8, 2024
Copy link
Contributor

@mku514k mku514k left a comment

Choose a reason for hiding this comment

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

Small nitpicks from me

doc/ledctl.pod Outdated Show resolved Hide resolved
src/ledctl/ledctl.c Outdated Show resolved Hide resolved
mku514k
mku514k previously approved these changes Oct 8, 2024
Copy link
Contributor

@mku514k mku514k left a comment

Choose a reason for hiding this comment

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

LGTM

tests/ledctl/slot_test.py Outdated Show resolved Hide resolved
tests/ledctl/slot_test.py Outdated Show resolved Hide resolved
src/ledctl/ledctl.c Outdated Show resolved Hide resolved
mku514k
mku514k previously approved these changes Oct 11, 2024
doc/ledctl.pod Outdated Show resolved Hide resolved
Rename --best-controller to --default-controller.

Signed-off-by: Kinga Stefaniuk <kinga.stefaniuk@intel.com>
@mtkaczyk mtkaczyk requested a review from mku514k October 16, 2024 10:12
@ktanska ktanska merged commit a8b320d into intel:main Oct 16, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants