-
Notifications
You must be signed in to change notification settings - Fork 195
[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.
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
base: master
Are you sure you want to change the base?
Changes from all commits
cf713d9
6063a6e
188652c
455572d
048fc91
bf4dede
cf7161e
5bdf678
2fcc059
9a11b05
3995d5f
98bee9b
a7e21b5
8a52b30
508d208
0a37102
23efbcf
1ae9698
5058880
101d8fe
d30d02a
1dea21c
9c89a65
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -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): | ||||||||||||||||||||||||||||||
| 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 | ||||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add a warning log something like, if we have to return 0
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
| Get PSU object from platform chassis | |
| Get PSU object from platform chassis | |
| :param logger: Logger instance for error/warning messages |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@LinJin23 Pls fix this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Copilot
AI
Oct 24, 2025
There was a problem hiding this comment.
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.
| 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Copilot
AI
Nov 6, 2025
There was a problem hiding this comment.
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.
| 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 |
There was a problem hiding this comment.
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.
Copilot
AI
Nov 10, 2025
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
judyjoseph marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment.
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
"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Copilot
AI
Nov 6, 2025
There was a problem hiding this comment.
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.
| 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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same comment as above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Copilot
AI
Nov 10, 2025
There was a problem hiding this comment.
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.
| 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.