-
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 19 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,85 @@ 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 | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
| return 0 | |
| raise RuntimeError("Unable to determine number of PSUs: neither platform_chassis nor platform_psuutil is available") |
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.
Add a warning log something like, if we have to return 0
if logger:
logger.log_warning("No PSU provider available; assuming 0 PSUs")
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 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.
The docstring is missing the logger parameter description. Add ':param logger: Logger instance for error/warning messages' to document the logger parameter.
| 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
Outdated
Copilot
AI
Nov 4, 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.
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.
| 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))) |
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
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
LinJin23 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
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.
Outdated
Copilot
AI
Oct 23, 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 comment explaining the IndexError exception handling was removed. This comment provided valuable context about why the exception is caught and should be preserved: 'some functionality is expectent on returning an expected key even if the psu object itself does not exist'.
| pass | |
| pass | |
| # some functionality is expectent on returning an expected key even if the psu object itself does not exist |
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
LinJin23 marked this conversation as resolved.
Outdated
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
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
Outdated
Copilot
AI
Nov 4, 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.
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.
| return False | |
| 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.
why is logger being passed in here? its not used.
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 for pointing it out. I’ve removed it.