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

Change PSU key to use get_name API instead of index #446

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

gregoryboudreau
Copy link
Contributor

@gregoryboudreau gregoryboudreau commented Mar 11, 2024

Change PSU key to use get_name API instead of index

Description

Changes PSU keys to be implemented via their get_name API like we do for fans instead of simple indexing. If the get_name API is not implemented by vendors then will fall back to the original indexing schema.

Motivation and Context

Makes PSU objects more clear in context by attaching their associated get_name to their PSU key similar to how it is done for fan objects.

How Has This Been Tested?

Along with other modifications in other repos, this is passing mgmt related test cases such as snmp tests and psu api tests. Additionally, can verify the output via psushow script.

Additional Information (Optional)

Related PRs:
sonic-net/sonic-mgmt#11944
sonic-net/sonic-utilities#3208
sonic-net/sonic-snmpagent#312

@gechiang gechiang requested review from prgeor and assrinivasan June 3, 2024 21:29
@gechiang
Copy link
Contributor

gechiang commented Jun 3, 2024

/azp run

Copy link

Commenter does not have sufficient privileges for PR 446 in repo sonic-net/sonic-platform-daemons

@assrinivasan
Copy link
Contributor

/azpw run

@assrinivasan
Copy link
Contributor

@gregoryboudreau useful addition. Couple questions:

If the get_name API is not implemented by vendors then will fall back to the original indexing schema.
Has this been tested?

Additionally, please fix unit test failures that appear to be indexing related. Thanks!

@prgeor
Copy link
Collaborator

prgeor commented Jun 4, 2024

@gregoryboudreau can you please run sonic-mgmt test cases for PSU with all the changes. Attach the result to this PR?

@gregoryboudreau
Copy link
Contributor Author

platform_tests/api/test_psu.py::TestPsuApi::test_get_name[mth64-m5-2] PASSED                                                                                                                                                                  [  7%]
platform_tests/api/test_psu.py::TestPsuApi::test_get_presence[mth64-m5-2] PASSED                                                                                                                                                              [ 14%]
platform_tests/api/test_psu.py::TestPsuApi::test_get_model[mth64-m5-2] PASSED                                                                                                                                                                 [ 21%]
platform_tests/api/test_psu.py::TestPsuApi::test_get_serial[mth64-m5-2] PASSED                                                                                                                                                                [ 28%]
platform_tests/api/test_psu.py::TestPsuApi::test_get_revision[mth64-m5-2] PASSED                                                                                                                                                              [ 35%]
platform_tests/api/test_psu.py::TestPsuApi::test_get_status[mth64-m5-2] PASSED                                                                                                                                                                [ 42%]
platform_tests/api/test_psu.py::TestPsuApi::test_get_position_in_parent[mth64-m5-2] PASSED                                                                                                                                                    [ 50%]
platform_tests/api/test_psu.py::TestPsuApi::test_is_replaceable[mth64-m5-2] PASSED                                                                                                                                                            [ 57%]
platform_tests/api/test_psu.py::TestPsuApi::test_fans[mth64-m5-2] PASSED                                                                                                                                                                      [ 64%]
platform_tests/api/test_psu.py::TestPsuApi::test_power[mth64-m5-2] SKIPPED (Unsupported platform API)                                                                                                                                         [ 71%]
platform_tests/api/test_psu.py::TestPsuApi::test_temperature[mth64-m5-2] PASSED                                                                                                                                                               [ 78%]
platform_tests/api/test_psu.py::TestPsuApi::test_led[mth64-m5-2] SKIPPED (On Cisco 8000, mellanox and Nokia 7215 platform, PSU led are unable to be controlled by software)                                                                   [ 85%]
platform_tests/api/test_psu.py::TestPsuApi::test_thermals[mth64-m5-2] PASSED                                                                                                                                                                  [ 92%]
platform_tests/api/test_psu.py::TestPsuApi::test_master_led[mth64-m5-2] PASSED                                                                                                                                                                [100%]
platform_tests/api/test_chassis.py::TestChassisApi::test_get_name[mth64-m5-2] PASSED                                                                                                                                                          [  4%]
platform_tests/api/test_chassis.py::TestChassisApi::test_get_presence[mth64-m5-2] PASSED                                                                                                                                                      [  8%]
platform_tests/api/test_chassis.py::TestChassisApi::test_get_model[mth64-m5-2] PASSED                                                                                                                                                         [ 12%]
platform_tests/api/test_chassis.py::TestChassisApi::test_get_serial[mth64-m5-2] PASSED                                                                                                                                                        [ 16%]
platform_tests/api/test_chassis.py::TestChassisApi::test_get_revision[mth64-m5-2] PASSED                                                                                                                                                      [ 20%]
platform_tests/api/test_chassis.py::TestChassisApi::test_get_status[mth64-m5-2] PASSED                                                                                                                                                        [ 25%]
platform_tests/api/test_chassis.py::TestChassisApi::test_get_position_in_parent[mth64-m5-2] PASSED                                                                                                                                            [ 29%]
platform_tests/api/test_chassis.py::TestChassisApi::test_is_replaceable[mth64-m5-2] PASSED                                                                                                                                                    [ 33%]
platform_tests/api/test_chassis.py::TestChassisApi::test_get_base_mac[mth64-m5-2] PASSED                                                                                                                                                      [ 37%]
platform_tests/api/test_chassis.py::TestChassisApi::test_get_system_eeprom_info[mth64-m5-2] PASSED                                                                                                                                            [ 41%]
platform_tests/api/test_chassis.py::TestChassisApi::test_get_reboot_cause[mth64-m5-2] PASSED                                                                                                                                                  [ 45%]
platform_tests/api/test_chassis.py::TestChassisApi::test_components[mth64-m5-2] PASSED                                                                                                                                                        [ 50%]
platform_tests/api/test_chassis.py::TestChassisApi::test_modules[mth64-m5-2] SKIPPED (No modules found on device)                                                                                                                             [ 54%]
platform_tests/api/test_chassis.py::TestChassisApi::test_fans[mth64-m5-2] PASSED                                                                                                                                                              [ 58%]
platform_tests/api/test_chassis.py::TestChassisApi::test_fan_drawers[mth64-m5-2] PASSED                                                                                                                                                       [ 62%]
platform_tests/api/test_chassis.py::TestChassisApi::test_psus[mth64-m5-2] PASSED                                                                                                                                                              [ 66%]
platform_tests/api/test_chassis.py::TestChassisApi::test_thermals[mth64-m5-2] PASSED                                                                                                                                                          [ 70%]
platform_tests/api/test_chassis.py::TestChassisApi::test_sfps[mth64-m5-2] PASSED                                                                                                                                                              [ 75%]
platform_tests/api/test_chassis.py::TestChassisApi::test_status_led[mth64-m5-2] PASSED                                                                                                                                                        [ 79%]
platform_tests/api/test_chassis.py::TestChassisApi::test_get_thermal_manager[mth64-m5-2] PASSED                                                                                                                                               [ 83%]
platform_tests/api/test_chassis.py::TestChassisApi::test_get_watchdog[mth64-m5-2] PASSED                                                                                                                                                      [ 87%]
platform_tests/api/test_chassis.py::TestChassisApi::test_get_eeprom[mth64-m5-2] PASSED                                                                                                                                                        [ 91%]
platform_tests/api/test_chassis.py::TestChassisApi::test_get_supervisor_slot[mth64-m5-2] SKIPPED (skipped as this test is applicable to modular chassis only)                                                                                 [ 95%]
platform_tests/api/test_chassis.py::TestChassisApi::test_get_my_slot[mth64-m5-2] SKIPPED (skipped as this test is applicable to modular chassis only)                                                                                         [100%]
platform_tests/daemon/test_psud.py::test_pmon_psud_running_status[mth64-m5-2] PASSED                                                                                                                                                          [ 25%]
platform_tests/daemon/test_psud.py::test_pmon_psud_stop_and_start_status[mth64-m5-2] PASSED                                                                                                                                                   [ 50%]
platform_tests/daemon/test_psud.py::test_pmon_psud_term_and_start_status[mth64-m5-2] PASSED                                                                                                                                                   [ 75%]
platform_tests/daemon/test_psud.py::test_pmon_psud_kill_and_start_status[mth64-m5-2] PASSED                                                                                                                                                   [100%]
snmp/test_snmp_psu.py::test_snmp_numpsu[mth64-m5-2] PASSED                                                                                                                                                                                    [ 50%]
snmp/test_snmp_psu.py::test_snmp_psu_status[mth64-m5-2] PASSED                                                                                                                                                                                [100%]

Let me know if there are other test cases that I missed in running, thanks!

@gregoryboudreau
Copy link
Contributor Author

@assrinivasan with the get_name API implemented:

root@mth64-m5-2:/home/cisco# show platform psustatus
PSU    Model         Serial         HW Rev  Voltage (V)    Current (A)    Power (W)    Status    LED
-----  ------------  -----------  --------  -------------  -------------  -----------  --------  -----
PSU0   PSU650W-ACPE  LIT241955UQ      0.00  11.996         24.0           288.0        OK        green
PSU1   PSU650W-ACPE  LIT241956QK      0.00  N/A            N/A            N/A          NOT OK    off

This is with the get_name API overwritten to raise NotImplemented for PSUs:

root@mth64-m5-2:/home/cisco# show plat psu
PSU    Model         Serial         HW Rev  Voltage (V)    Current (A)    Power (W)    Status    LED
-----  ------------  -----------  --------  -------------  -------------  -----------  --------  -----
PSU 1  PSU650W-ACPE  LIT241955UQ      0.00  11.98          24.0           288.0        OK        green
PSU 2  PSU650W-ACPE  LIT241956QK      0.00  N/A            N/A            N/A          NOT OK    off

The only functional difference between the two is the key/naming

@abdosi
Copy link
Contributor

abdosi commented Sep 13, 2024

@assrinivasan : can we please help with merge this.

Copy link
Contributor

@assrinivasan assrinivasan left a comment

Choose a reason for hiding this comment

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

LGTM

@assrinivasan
Copy link
Contributor

@prgeor please help review/merge as appropriate

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants