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

LinJin23 · 2025-08-11T06:27:14Z

Description

This PR refactors the PSU object retrieval logic in psud by extracting the platform_chassis.get_psu() call into a dedicated wrapper function _wrapper_get_psu(). In addition, the process no longer exits when platform_psuutil is unavailable. The change improves exception handling consistency and code maintainability by centralizing PSU object retrieval logic.

Motivation and Context

The original code had duplicate platform_chassis.get_psu(psu_index - 1) calls scattered across multiple wrapper functions, making it difficult to maintain consistent exception handling and increasing code duplication.

How Has This Been Tested?

Unit tests were extended to test the PSU exception handling features and tested in physical platform.

Additional Information (Optional)

mssonicbld · 2025-08-11T06:27:22Z

/azp run

azure-pipelines · 2025-08-11T06:27:31Z

Azure Pipelines successfully started running 1 pipeline(s).

vvolam · 2025-08-22T18:46:33Z

sonic-psud/scripts/psud

+    if platform_chassis is not None:
+        try:
+            return platform_chassis.get_psu(psu_index - 1)
+        except NotImplementedError: # TODO: Is NotImplementedError sufficient? 


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

vvolam · 2025-08-22T18:47:32Z

sonic-psud/scripts/psud

 def _wrapper_get_psu_presence(psu_index):
    if platform_chassis is not None:
+        psu = _wrapper_get_psu(psu_index)
+        if not psu:


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

mssonicbld · 2025-08-26T14:41:11Z

/azp run

azure-pipelines · 2025-08-26T14:41:21Z

Azure Pipelines successfully started running 1 pipeline(s).

gregoryboudreau · 2025-09-10T20:25:19Z

sonic-psud/scripts/psud

+        try:
+            return platform_chassis.get_psu(psu_index - 1)
+        except NotImplementedError as e:
+            logger.log_warning("get_psu() not implemented by platform chassis: {}".format(str(e)))


is this something that needs to be logged? Also in its current form, you'll get two conflicting logs. Say you run the wrapper to get presence, this get_psu wrapper will log "get_psu() not implemented by platform chassis: {}" and return none, but now that its none the get_presence will print "Failed to get PSU {} object from platform chassis".

This also seems to deviate from the prior behavior where if the platform_chassis doesn't have the get_psu, it will fallback to the platform_psuutil case. Is it possible that some vendors have not implemented this get_psu on the platform_chassis and still rely on this fallback?

Thanks for your pointing out.

gregoryboudreau · 2025-09-10T20:29:45Z

sonic-psud/scripts/psud

+        psu = _wrapper_get_psu(psu_index)
+        if not psu:
+            logger.log_error("Failed to get PSU {} object from platform chassis".format(psu_index))
+            return ''


imo this should be a case where it returns the PSU_INFO_KEY_TEMPLATE, not an empty string.

LinJin23 · 2025-09-23T14:15:49Z

/azpw run

mssonicbld · 2025-09-23T14:15:52Z

/AzurePipelines run

azure-pipelines · 2025-09-23T14:16:01Z

Azure Pipelines successfully started running 1 pipeline(s).

Signed-off-by: Lin Jin <[email protected]>

mssonicbld · 2025-09-23T14:52:29Z

/azp run

azure-pipelines · 2025-09-23T14:52:39Z

Azure Pipelines successfully started running 1 pipeline(s).

Signed-off-by: Lin Jin <[email protected]>

mssonicbld · 2025-09-23T15:16:53Z

/azp run

azure-pipelines · 2025-09-23T15:17:03Z

Azure Pipelines successfully started running 1 pipeline(s).

Signed-off-by: Lin Jin <[email protected]>

mssonicbld · 2025-09-23T15:32:06Z

/azp run

azure-pipelines · 2025-09-23T15:32:28Z

Azure Pipelines successfully started running 1 pipeline(s).

Signed-off-by: Lin Jin <[email protected]>

mssonicbld · 2025-09-23T15:42:59Z

/azp run

azure-pipelines · 2025-09-23T15:43:08Z

Azure Pipelines successfully started running 1 pipeline(s).

Signed-off-by: Lin Jin <[email protected]>

mssonicbld · 2025-09-23T16:02:30Z

/azp run

azure-pipelines · 2025-09-23T16:02:42Z

Azure Pipelines successfully started running 1 pipeline(s).

azure-pipelines · 2025-11-04T22:38:15Z

Azure Pipelines successfully started running 1 pipeline(s).

Copilot

Pull Request Overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 11 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-11-04T22:53:43Z

sonic-psud/scripts/psud

+            except NotImplementedError:
+                pass
+            except Exception as e:
+                logger.log_warning("Exception in psu.get_presence() for PSU {}: {}".format(psu_index, str(e)))


Missing null check for logger before calling log_warning. This will cause an AttributeError if logger is None. The same pattern is correctly handled on line 143-144 with an if logger: check.

Suggested change

logger.log_warning("Exception in psu.get_presence() for PSU {}: {}".format(psu_index, str(e)))

if logger:

logger.log_warning("Exception in psu.get_presence() for PSU {}: {}".format(psu_index, str(e)))

sonic-psud/scripts/psud

Copilot · 2025-11-04T22:53:44Z

sonic-psud/scripts/psud

+                # some functionality is expectent on returning an expected key even if the psu object itself does not exist
+                pass
+            except Exception as e:
+                return False


Returning False in get_psu_key() is inconsistent with the function's purpose. The function should return a string key identifying the PSU. In all other exception paths (NotImplementedError, IndexError), it falls through to return PSU_INFO_KEY_TEMPLATE.format(psu_index) on line 191. This general Exception handler should either return the same default template or be removed to allow the fall-through behavior.

Suggested change

return False

pass

sonic-psud/tests/test_psud.py

Signed-off-by: Lin Jin <[email protected]>

judyjoseph

LGTM

judyjoseph · 2025-11-05T21:22:35Z

/azp run

azure-pipelines · 2025-11-05T21:22:44Z

Azure Pipelines successfully started running 1 pipeline(s).

Signed-off-by: Lin Jin <[email protected]>

mssonicbld · 2025-11-06T07:24:31Z

/azp run

azure-pipelines · 2025-11-06T07:24:41Z

Azure Pipelines successfully started running 1 pipeline(s).

Copilot

Pull Request Overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

sonic-psud/scripts/psud

Signed-off-by: Lin Jin <[email protected]>

mssonicbld · 2025-11-06T08:53:29Z

/azp run

azure-pipelines · 2025-11-06T08:53:38Z

Azure Pipelines successfully started running 1 pipeline(s).

Copilot

Pull Request Overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-11-06T08:55:51Z

sonic-psud/scripts/psud

 #

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


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.

I don’t think it’s necessary.

Copilot · 2025-11-06T08:55:51Z

sonic-psud/scripts/psud

+    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
+    


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

I don’t think it’s necessary.

Copilot

Pull Request Overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-11-10T15:52:44Z

sonic-psud/tests/test_psud.py

+    psud.platform_psuutil = mock.MagicMock()
+    psud._wrapper_get_num_psus(None)
+    assert psud.platform_chassis.get_num_psus.call_count >= 1
+


Trailing whitespace should be removed for code cleanliness.

Suggested change

Copilot · 2025-11-10T15:52:44Z

sonic-psud/tests/test_psud.py

+    mock_logger = mock.MagicMock()
+    mock_psu = mock.MagicMock()
+    mock_psu.get_presence.return_value = True
+


Trailing whitespace should be removed for code cleanliness.

Suggested change

Copilot · 2025-11-10T15:52:45Z

sonic-psud/tests/test_psud.py

+def test_wrapper_get_psu():
+    mock_logger = mock.MagicMock()
+    mock_psu = mock.MagicMock()
+


Trailing whitespace should be removed for code cleanliness.

Suggested change

Copilot · 2025-11-10T15:52:45Z

sonic-psud/tests/test_psud.py

+    mock_logger = mock.MagicMock()
+    mock_psu = mock.MagicMock()
+    mock_psu.get_powergood_status.return_value = True
+


Trailing whitespace should be removed for code cleanliness.

Suggested change

Copilot · 2025-11-10T15:52:46Z

sonic-psud/tests/test_psud.py

+def test_get_psu_key():
+    mock_psu = mock.MagicMock()
+    mock_psu.get_name.return_value = "PSU-1"
+


Trailing whitespace should be removed for code cleanliness.

Suggested change

Copilot · 2025-11-10T15:52:46Z

sonic-psud/scripts/psud

+    Note:
+        If logger is None, errors and warnings encountered during execution will not be logged.


[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)

I don’t think it’s necessary.

Copilot · 2025-11-10T15:52:46Z

sonic-psud/scripts/psud

+            except Exception:
+                pass


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

I don’t think it’s necessary.

judyjoseph

LGTM, please take care of general comments above

Signed-off-by: Lin Jin <[email protected]>

mssonicbld · 2025-11-13T07:18:25Z

/azp run

azure-pipelines · 2025-11-13T07:18:33Z

Azure Pipelines successfully started running 1 pipeline(s).

LinJin23 · 2025-11-13T23:59:56Z

Hi @liamkearney-msft, could you please help review?

add exception handling in add_exception_psu

cf713d9

LinJin23 changed the title ~~add exception handling in add_exception_psu~~ [psud] Refactor PSU object retrieval to improve exception handling Aug 11, 2025

LinJin23 requested a review from vvolam August 13, 2025 01:08

vvolam reviewed Aug 22, 2025

View reviewed changes

address review comments

6063a6e

gregoryboudreau reviewed Sep 10, 2025

View reviewed changes

Fix tests and add handler to platform_psuutil

188652c

Signed-off-by: Lin Jin <[email protected]>

Fix comment

455572d

Signed-off-by: Lin Jin <[email protected]>

Fix test

048fc91

Signed-off-by: Lin Jin <[email protected]>

Fix test

bf4dede

Signed-off-by: Lin Jin <[email protected]>

Add tests

cf7161e

Signed-off-by: Lin Jin <[email protected]>

LinJin23 requested review from gregoryboudreau and vvolam September 23, 2025 16:11

LinJin23 requested a review from Copilot November 4, 2025 22:51

Copilot AI reviewed Nov 4, 2025

View reviewed changes

enhance error logging in PSU presence and status retrieval functions

101d8fe

Signed-off-by: Lin Jin <[email protected]>

judyjoseph previously approved these changes Nov 5, 2025

View reviewed changes

judyjoseph requested a review from vvolam November 5, 2025 21:22

fix unit test

d30d02a

Signed-off-by: Lin Jin <[email protected]>

LinJin23 dismissed judyjoseph’s stale review via d30d02a November 6, 2025 07:24

LinJin23 requested a review from Copilot November 6, 2025 07:27

Copilot AI reviewed Nov 6, 2025

View reviewed changes

sonic-psud/scripts/psud Outdated Show resolved Hide resolved

remove redundant exception

1dea21c

Signed-off-by: Lin Jin <[email protected]>

LinJin23 requested a review from Copilot November 6, 2025 08:53

Copilot AI reviewed Nov 6, 2025

View reviewed changes

LinJin23 requested a review from judyjoseph November 10, 2025 00:13

judyjoseph requested a review from Copilot November 10, 2025 15:50

Copilot AI reviewed Nov 10, 2025

View reviewed changes

judyjoseph previously approved these changes Nov 11, 2025

View reviewed changes

format whitespace

9c89a65

Signed-off-by: Lin Jin <[email protected]>

LinJin23 dismissed judyjoseph’s stale review via 9c89a65 November 13, 2025 07:18

LinJin23 requested review from liamkearney-msft and removed request for gregoryboudreau November 13, 2025 23:59

	logger.log_warning("Exception in psu.get_presence() for PSU {}: {}".format(psu_index, str(e)))
	if logger:
	logger.log_warning("Exception in psu.get_presence() for PSU {}: {}".format(psu_index, str(e)))

-    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.

		Note:
		If logger is None, errors and warnings encountered during execution will not be logged.

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

Are you sure you want to change the base?

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

Conversation

LinJin23 commented Aug 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation and Context

How Has This Been Tested?

Additional Information (Optional)

Uh oh!

mssonicbld commented Aug 11, 2025

Uh oh!

azure-pipelines bot commented Aug 11, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mssonicbld commented Aug 26, 2025

Uh oh!

azure-pipelines bot commented Aug 26, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LinJin23 commented Sep 23, 2025

Uh oh!

mssonicbld commented Sep 23, 2025

Uh oh!

azure-pipelines bot commented Sep 23, 2025

Uh oh!

mssonicbld commented Sep 23, 2025

Uh oh!

azure-pipelines bot commented Sep 23, 2025

Uh oh!

mssonicbld commented Sep 23, 2025

Uh oh!

azure-pipelines bot commented Sep 23, 2025

Uh oh!

mssonicbld commented Sep 23, 2025

Uh oh!

azure-pipelines bot commented Sep 23, 2025

Uh oh!

mssonicbld commented Sep 23, 2025

Uh oh!

azure-pipelines bot commented Sep 23, 2025

Uh oh!

mssonicbld commented Sep 23, 2025

Uh oh!

azure-pipelines bot commented Sep 23, 2025

Uh oh!

azure-pipelines bot commented Nov 4, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Nov 4, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI Nov 4, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

LinJin23 commented Aug 11, 2025 •

edited

Loading