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

Updating SNMP implementation to handle change of PSU Keys #312

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

Conversation

gregoryboudreau
Copy link

@gregoryboudreau gregoryboudreau commented Mar 11, 2024

- What I did
Modified snmp implementation of Cisco MIB to use updated format for psu keys instead of index of PSU
- How I did it
Used a consistently sorted list of keys to allow for the modified statedb key naming for psus
- How to verify it
Ran the associated sonic-mgmt tests for PSUs (phy_entity and snmp_psu)
- Description for the changelog

Modifies SNMP implementation of Cisco PSUs to use key names instead of simple indexing

Related PRs:
sonic-net/sonic-mgmt#11944
sonic-net/sonic-utilities#3208
sonic-net/sonic-platform-daemons#446

gechiang
gechiang previously approved these changes Jun 3, 2024
@gechiang gechiang requested a review from SuvarnaMeenakshi June 3, 2024 21:30
@abdosi
Copy link
Contributor

abdosi commented Sep 13, 2024

let's fix this and rerun,

@mssonicbld
Copy link
Collaborator

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@@ -70,7 +71,8 @@ def _get_psu_presence(self, psu_index):
Get PSU presence
:return: the presence of particular PSU
"""
psu_name = PSU_INFO_KEY_TEMPLATE.format(psu_index)
psu_keys = natsorted(self.statedb.keys(self.statedb.STATE_DB, "PSU_INFO|*"))
psu_name = psu_keys[psu_index-1].removeprefix("PSU_INFO|")
Copy link
Contributor

Choose a reason for hiding this comment

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

minor comment, in line, 75 psu_name is actually not "psu_name", but it is the numerical value from psu name and is then used to get the psu key mibs.psu_info_table(psu_name).
Can we rename "psu_name" in line 75 or directly use "psu_keys[psu_index-1]" in line 75 instead of mibs.psu_info_table(psu_name)

Copy link
Author

Choose a reason for hiding this comment

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

you are correct in that the mibs.psu_info_table is redundant as we already have the full key for redis, I will update this accordingly.

@mssonicbld
Copy link
Collaborator

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@SuvarnaMeenakshi
Copy link
Contributor

SuvarnaMeenakshi commented Mar 12, 2025

@gregoryboudreau , will there by any impact in rfc2737 or 3433 implementation which uses PSU_INFO?

KEY_PATTERN = mibs.psu_info_table("*")

@SuvarnaMeenakshi
Copy link
Contributor

@gregoryboudreau can you merge with latest master

@mssonicbld
Copy link
Collaborator

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@gregoryboudreau
Copy link
Author

@gregoryboudreau , will there by any impact in rfc2737 or 3433 implementation which uses PSU_INFO?

KEY_PATTERN = mibs.psu_info_table("*")

definitely possible, didn't see that originally. Will take a look.

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

Successfully merging this pull request may close these issues.

6 participants