-
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 9 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,65 @@ 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() | ||||||||||||||||
| 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
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.
Outdated
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.
"return None" at line 121 is sufficient and line 116 and 120 is redundant
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
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 an error type log
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
judyjoseph marked this conversation as resolved.
Show resolved
Hide resolved
Outdated
Copilot
AI
Oct 14, 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 fallback logic should handle potential exceptions from platform_psuutil.get_psu_presence(). Consider wrapping this call in a try-except block to prevent unhandled exceptions from propagating up.
| return platform_psuutil.get_psu_presence(psu_index) | |
| try: | |
| return platform_psuutil.get_psu_presence(psu_index) | |
| except Exception as e: | |
| if logger: | |
| logger.log_error("Exception in platform_psuutil.get_psu_presence({}): {}".format(psu_index, str(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.
Please wrap around this exception and log a warning message if it is not implemented.
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
Outdated
Copilot
AI
Oct 14, 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 fallback logic should handle potential exceptions from platform_psuutil.get_psu_status(). Consider wrapping this call in a try-except block to prevent unhandled exceptions from propagating up.
| return platform_psuutil.get_psu_status(psu_index) | |
| try: | |
| return platform_psuutil.get_psu_status(psu_index) | |
| except Exception as e: | |
| if logger: | |
| logger.log_error("Exception in platform_psuutil.get_psu_status({}): {}".format(psu_index, str(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.
Please add this exception
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, and at the suggested line 152, I changed log_error to log_warning.
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
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
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 log_warning method is being called with a second parameter True, but logger methods typically don't take a boolean parameter for console output. This could cause runtime errors if the logger doesn't support this parameter pattern.
| self.log_warning("Failed to load psuutil: %s" % (str(e)), True) | |
| self.log_warning("Failed to load psuutil: %s" % (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
Outdated
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 this behavior being changed? is there a benefit to continuing when psuutil isn't present?
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.
We don’t want psud to crash when psuutil cannot be loaded. Instead, we prefer to let platform_psuutil continue with default behavior. Also, since platform_psuutil is a global variable, I didn’t set it to None after the error to avoid overwriting any existing value.
@vvolam , please correct me if I’m wrong.
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, I too think we should stick to earlier behavior .. i.e if the platform psuutil not found, need to exit.
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.
Revert to the previous behavior
Outdated
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.
There's a grammatical error in the comment. It should be 'Don't expect PSUD to exit' instead of 'Don't expect the PSUD to exit'.
| # Don't expect the PSUD to exit just because psuutil is not available | |
| # Don't expect PSUD to exit just because psuutil is not 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.
This line has been removed.
Outdated
Copilot
AI
Sep 30, 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.
There's trailing whitespace on line 439. Consider removing it for cleaner code.
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.
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.