Skip to content

[psud] Refactor PSU object retrieval to improve exception handling #659

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

Open
wants to merge 23 commits into
base: master
Choose a base branch
from
+418 −48
Open

[psud] Refactor PSU object retrieval to improve exception handling #659

Changes from 1 commit
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
cf713d9
add exception handling in add_exception_psu
LinJin23 Aug 11, 2025
6063a6e
address review comments
LinJin23 Aug 26, 2025
188652c
Fix tests and add handler to platform_psuutil
LinJin23 Sep 23, 2025
455572d
Fix comment
LinJin23 Sep 23, 2025
048fc91
Fix test
LinJin23 Sep 23, 2025
bf4dede
Fix test
LinJin23 Sep 23, 2025
cf7161e
Add tests
LinJin23 Sep 23, 2025
5bdf678
add init value when hit exception
LinJin23 Sep 24, 2025
2fcc059
Revert "add init value when hit exception"
LinJin23 Sep 25, 2025
9a11b05
fix format
LinJin23 Oct 7, 2025
3995d5f
remove unused logger
LinJin23 Oct 8, 2025
98bee9b
refactor: rename _wrapper_get_psu_status to _wrapper_get_psu_presence…
LinJin23 Oct 10, 2025
a7e21b5
address copilot comments
LinJin23 Oct 10, 2025
8a52b30
Revert to the previous behavior to keep exit when failed in get psuutil
LinJin23 Oct 23, 2025
508d208
enhance PSU wrapper functions to accept logger for improved error han…
LinJin23 Oct 23, 2025
0a37102
Revert to the original psuutil exception handling logic
LinJin23 Oct 24, 2025
23efbcf
add docstring
LinJin23 Oct 24, 2025
1ae9698
add error handling and logging for PSU presence and status retrieval
LinJin23 Nov 4, 2025
5058880
add null check for chassis_tbl before deleting keys
LinJin23 Nov 4, 2025
101d8fe
enhance error logging in PSU presence and status retrieval functions
LinJin23 Nov 5, 2025
d30d02a
fix unit test
LinJin23 Nov 6, 2025
1dea21c
remove redundant exception
LinJin23 Nov 6, 2025
9c89a65
format whitespace
LinJin23 Nov 13, 2025
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
30 changes: 27 additions & 3 deletions sonic-psud/scripts/psud
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,19 +99,40 @@ def _wrapper_get_num_psus():
return platform_psuutil.get_num_psus()


def _wrapper_get_psu(psu_index):
"""
Get PSU object from platform chassis
Copy link

Copilot AI Oct 10, 2025

Choose a reason for hiding this comment

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

The docstring is missing the logger parameter description. Add ':param logger: Logger instance for error/warning messages' to document the logger parameter.

Suggested change
Get PSU object from platform chassis
Get PSU object from platform chassis
:param logger: Logger instance for error/warning messages

Copilot uses AI. Check for mistakes.
Copy link
Contributor

@vvolam vvolam Oct 16, 2025

Choose a reason for hiding this comment

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

@LinJin23 Pls fix this.

Copy link
Contributor Author

@LinJin23 LinJin23 Oct 23, 2025

Choose a reason for hiding this comment

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

done

:param psu_index: PSU index (1-based)
:return: PSU object if available, None otherwise
"""
if platform_chassis is not None:
try:
return platform_chassis.get_psu(psu_index - 1)
except NotImplementedError: # TODO: Is NotImplementedError sufficient?
Copy link
Contributor

@vvolam vvolam Aug 22, 2025

Choose a reason for hiding this comment

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

Log a warning for this exception and return None, Also add a general exception to make the code robust.

Copy link
Contributor Author

@LinJin23 LinJin23 Sep 23, 2025

Choose a reason for hiding this comment

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

Done

pass # TODO: When caught, what should we do here?
# TODO: If platform_chassis is None, should we return None or add logging and return None?
return None


def _wrapper_get_psu_presence(psu_index):
if platform_chassis is not None:
psu = _wrapper_get_psu(psu_index)
if not psu:
Copy link
Contributor

@vvolam vvolam Aug 22, 2025

Choose a reason for hiding this comment

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

if psu returned is None here, add exception here to log error and returning false.

Copy link
Contributor Author

@LinJin23 LinJin23 Sep 23, 2025

Choose a reason for hiding this comment

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

Done

return False # TODO: What should we do when PSU object is not available?
try:
return platform_chassis.get_psu(psu_index - 1).get_presence()
return psu.get_presence()
except NotImplementedError:
pass
return platform_psuutil.get_psu_presence(psu_index)


def _wrapper_get_psu_status(psu_index):
if platform_chassis is not None:
psu = _wrapper_get_psu(psu_index)
if not psu:
return False # TODO: What should we do when PSU object is not available?
try:
return platform_chassis.get_psu(psu_index - 1).get_powergood_status()
return psu.get_powergood_status()
except NotImplementedError:
pass
return platform_psuutil.get_psu_status(psu_index)
Expand All @@ -123,8 +144,11 @@ def _wrapper_get_psu_status(psu_index):

def get_psu_key(psu_index):
if platform_chassis is not None:
Copy link

Copilot AI Nov 6, 2025

Choose a reason for hiding this comment

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

The get_psu_key function passes None as the logger parameter to _wrapper_get_psu. While this is intentional to avoid logging in this context, the function should include a comment explaining why logging is suppressed here, as this is a deviation from other usages where self (the logger) is passed.

Suggested change
if platform_chassis is not None:
if platform_chassis is not None:
# Intentionally pass None as the logger to suppress logging in this context,
# as get_psu_key is only used for key generation and not for error reporting.
# This deviates from other usages where a logger is passed.

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

@LinJin23 LinJin23 Nov 13, 2025

Choose a reason for hiding this comment

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

I don’t think it’s necessary.

psu = _wrapper_get_psu(psu_index)
if not psu:
return "" # TODO: What should we do when PSU object is not available?
try:
return platform_chassis.get_psu(psu_index - 1).get_name()
return psu.get_name()
except NotImplementedError:
pass
except IndexError:
Expand Down
Loading