Skip to content
Open
Show file tree
Hide file tree
Changes from 22 commits
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
110 changes: 83 additions & 27 deletions sonic-psud/scripts/psud
Original file line number Diff line number Diff line change
Expand Up @@ -90,31 +90,87 @@ exit_code = 0
# temporary wrappers that are compliable with both new platform api and old-style plugin mode


def _wrapper_get_num_psus():
def _wrapper_get_num_psus(logger):
Copy link
Contributor

Choose a reason for hiding this comment

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

why is logger being passed in here? its not used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for pointing it out. I’ve removed it.

if platform_chassis is not None:
try:
return platform_chassis.get_num_psus()
except NotImplementedError:
pass
return platform_psuutil.get_num_psus()
if platform_psuutil is not None:
return platform_psuutil.get_num_psus()
if logger:
logger.log_warning("PSU provider unavailable; defaulting to 0 PSUs")
return 0
Copy link

Copilot AI Oct 14, 2025

Choose a reason for hiding this comment

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

The function should maintain consistent fallback behavior. When platform_psuutil is None, returning 0 may not accurately represent the actual number of PSUs on the system. Consider returning a default value that makes sense for the platform or raising an appropriate exception.

Suggested change
return 0
raise RuntimeError("Unable to determine number of PSUs: neither platform_chassis nor platform_psuutil is available")

Copilot uses AI. Check for mistakes.
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a warning log something like, if we have to return 0

if logger:
    logger.log_warning("No PSU provider available; assuming 0 PSUs")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done



def _wrapper_get_psu_presence(psu_index):
def _wrapper_get_psu(logger, 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

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

Choose a reason for hiding this comment

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

done

:param logger: Logger instance for error/warning messages
:param psu_index: PSU index (1-based)
:return: PSU object if available, None otherwise
Comment on lines +108 to +111
Copy link

Copilot AI Oct 24, 2025

Choose a reason for hiding this comment

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

The docstring should clarify that logger can be None and that when None is passed, errors/warnings won't be logged. This is an important behavior detail for callers of this function.

Suggested change
Get PSU object from platform chassis
:param logger: Logger instance for error/warning messages
:param psu_index: PSU index (1-based)
:return: PSU object if available, None otherwise
Get PSU object from platform chassis.
:param logger: Logger instance for error/warning messages. Can be None; if None is passed, errors and warnings will not be logged.
:param psu_index: PSU index (1-based)
:return: PSU object if available, None otherwise
Note:
If logger is None, errors and warnings encountered during execution will not be logged.

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

Choose a reason for hiding this comment

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

done


Comment on lines +108 to +112
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 docstring should explicitly mention that this function only attempts to retrieve PSU from platform_chassis and does not fall back to platform_psuutil, as this is a key behavioral difference from other wrapper functions like _wrapper_get_psu_presence and _wrapper_get_psu_status.

Suggested change
Get PSU object from platform chassis
:param logger: Logger instance for error/warning messages
:param psu_index: PSU index (1-based)
:return: PSU object if available, None otherwise
Get PSU object from platform chassis.
This function only attempts to retrieve the PSU from `platform_chassis` and does not fall back to
`platform_psuutil`, unlike other wrapper functions such as `_wrapper_get_psu_presence` and
`_wrapper_get_psu_status`.
:param logger: Logger instance for error/warning messages
:param psu_index: PSU index (1-based)
:return: PSU object if available, None otherwise

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

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.

Note:
If logger is None, errors and warnings encountered during execution will not be logged.
Comment on lines +113 to +114
Copy link

Copilot AI Nov 10, 2025

Choose a reason for hiding this comment

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

[nitpick] The docstring mentions that 'logger is None' will suppress logging, but this behavior should be more prominently documented in the parameter description itself, not just in a Note section. Consider updating line 109 to: :param logger: Logger instance for error/warning messages (can be None to suppress logging)

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

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.

"""
if platform_chassis is not None:
try:
return platform_chassis.get_psu(psu_index - 1).get_presence()
except NotImplementedError:
pass
return platform_psuutil.get_psu_presence(psu_index)
return platform_chassis.get_psu(psu_index - 1)
except NotImplementedError as e:
if logger:
logger.log_warning("get_psu() not implemented by platform chassis: {}".format(str(e)))
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be less than a warning imo, maybe a notice

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @vvolam, you mentioned in an earlier comment that this should use a warning level, so I’m a bit unsure which one to use here. Could you please help confirm which level is appropriate?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes NotImplemented exception could be notice as we are logging warning for failed case.

Copy link
Contributor

Choose a reason for hiding this comment

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

notice is not available, so you can leave this as warning for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, got it, I’ll keep it as warning for now.

except Exception as e:
if logger:
logger.log_error("Failed to get PSU {} from platform chassis: {}".format(psu_index, str(e)))
return None


def _wrapper_get_psu_status(psu_index):
def _wrapper_get_psu_presence(logger, psu_index):
if platform_chassis is not None:
psu = _wrapper_get_psu(logger, psu_index)
if psu:
try:
return psu.get_presence()
except NotImplementedError:
pass
except Exception as e:
if logger:
logger.log_warning("Exception in psu.get_presence() for PSU {}: {}".format(psu_index, str(e)))
return False
if platform_psuutil is not None:
try:
return platform_chassis.get_psu(psu_index - 1).get_powergood_status()
except NotImplementedError:
pass
return platform_psuutil.get_psu_status(psu_index)
return platform_psuutil.get_psu_presence(psu_index)
except Exception as e:
if logger:
logger.log_warning("Exception in platform_psuutil.get_psu_presence({}): {}".format(psu_index, str(e)))
return False
if logger:
logger.log_error("Failed to get PSU {} presence".format(psu_index))
return False


def _wrapper_get_psu_status(logger, psu_index):
if platform_chassis is not None:
psu = _wrapper_get_psu(logger, psu_index)
if psu:
try:
return psu.get_powergood_status()
except NotImplementedError:
pass
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment as above, add another condition as
"except Exception as e:
return False
"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

except Exception as e:
if logger:
logger.log_warning("Exception in psu.get_powergood_status() for PSU {}: {}".format(psu_index, str(e)))
return False
if platform_psuutil is not None:
try:
return platform_psuutil.get_psu_status(psu_index)
except Exception as e:
if logger:
logger.log_warning("Exception in platform_psuutil.get_psu_status({}): {}".format(psu_index, str(e)))
return False
if logger:
logger.log_error("Failed to get PSU {} status".format(psu_index))
return False


#
Expand All @@ -123,13 +179,12 @@ 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

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.

try:
return platform_chassis.get_psu(psu_index - 1).get_name()
except NotImplementedError:
pass
except IndexError:
#some functionality is expectent on returning an expected key even if the psu object itself does not exist
pass
psu = _wrapper_get_psu(None, psu_index)
if psu:
try:
return psu.get_name()
except Exception:
pass
Copy link
Contributor

Choose a reason for hiding this comment

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

same comment as above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Comment on lines +186 to +187
Copy link

Copilot AI Nov 10, 2025

Choose a reason for hiding this comment

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

Catching a bare Exception without logging or handling specific exception types makes debugging harder. The old code caught NotImplementedError and IndexError separately. Consider at minimum adding a comment explaining why all exceptions are silently ignored here, or restore the specific exception handling for clarity.

Suggested change
except Exception:
pass
except NotImplementedError:
pass
except IndexError:
pass
except Exception as e:
# Unexpected exception; log if logger is available, else silently ignore
# (logger is None here, so we cannot log)
pass

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

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.

return PSU_INFO_KEY_TEMPLATE.format(psu_index)


Expand Down Expand Up @@ -409,7 +464,7 @@ class DaemonPsud(daemon_base.DaemonBase):
self.phy_entity_tbl = swsscommon.Table(state_db, PHYSICAL_ENTITY_INFO_TABLE)

# Post psu number info to STATE_DB
self.num_psus = _wrapper_get_num_psus()
self.num_psus = _wrapper_get_num_psus(self)
fvs = swsscommon.FieldValuePairs([(CHASSIS_INFO_PSU_NUM_FIELD, str(self.num_psus))])
self.chassis_tbl.set(CHASSIS_INFO_KEY, fvs)

Expand All @@ -418,8 +473,9 @@ class DaemonPsud(daemon_base.DaemonBase):
for psu_index in range(1, self.num_psus + 1):
self.psu_tbl._del(get_psu_key(psu_index))

self.chassis_tbl._del(CHASSIS_INFO_KEY)
self.chassis_tbl._del(CHASSIS_INFO_POWER_KEY_TEMPLATE.format(1))
if self.chassis_tbl is not None:
self.chassis_tbl._del(CHASSIS_INFO_KEY)
self.chassis_tbl._del(CHASSIS_INFO_POWER_KEY_TEMPLATE.format(1))

# Override signal handler from DaemonBase
def signal_handler(self, sig, frame):
Expand Down Expand Up @@ -474,7 +530,7 @@ class DaemonPsud(daemon_base.DaemonBase):

def _update_single_psu_data(self, index, psu):
name = get_psu_key(index)
presence = _wrapper_get_psu_presence(index)
presence = _wrapper_get_psu_presence(self, index)
power_good = False
voltage = NOT_AVAILABLE
voltage_high_threshold = NOT_AVAILABLE
Expand All @@ -488,7 +544,7 @@ class DaemonPsud(daemon_base.DaemonBase):
in_current = NOT_AVAILABLE
max_power = NOT_AVAILABLE
if presence:
power_good = _wrapper_get_psu_status(index)
power_good = _wrapper_get_psu_status(self, index)
voltage = try_get(psu.get_voltage, NOT_AVAILABLE)
voltage_high_threshold = try_get(psu.get_voltage_high_threshold, NOT_AVAILABLE)
voltage_low_threshold = try_get(psu.get_voltage_low_threshold, NOT_AVAILABLE)
Expand Down Expand Up @@ -612,8 +668,8 @@ class DaemonPsud(daemon_base.DaemonBase):
(PSU_INFO_IN_CURRENT_FIELD, str(in_current)),
(PSU_INFO_IN_VOLTAGE_FIELD, str(in_voltage)),
(PSU_INFO_POWER_MAX_FIELD, str(max_power)),
(PSU_INFO_PRESENCE_FIELD, 'true' if _wrapper_get_psu_presence(index) else 'false'),
(PSU_INFO_STATUS_FIELD, 'true' if _wrapper_get_psu_status(index) else 'false'),
(PSU_INFO_PRESENCE_FIELD, 'true' if _wrapper_get_psu_presence(self, index) else 'false'),
(PSU_INFO_STATUS_FIELD, 'true' if _wrapper_get_psu_status(self, index) else 'false'),
])
self.psu_tbl.set(name, fvs)

Expand Down Expand Up @@ -642,7 +698,7 @@ class DaemonPsud(daemon_base.DaemonBase):
:return:
"""
psu_name = get_psu_key(psu_index)
presence = _wrapper_get_psu_presence(psu_index)
presence = _wrapper_get_psu_presence(self, psu_index)
fan_list = psu.get_all_fans()
for index, fan in enumerate(fan_list):
fan_name = try_get(fan.get_name, '{} FAN {}'.format(psu_name, index + 1))
Expand Down
Loading
Loading