-
Notifications
You must be signed in to change notification settings - Fork 195
Add suport for liquid cooling leakage detection #690
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?
Conversation
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@judyjoseph would you please help review this PR? |
|
/azp run |
|
Commenter does not have sufficient privileges for PR 690 in repo sonic-net/sonic-platform-daemons |
|
/azpw run |
|
/AzurePipelines run |
|
Azure Pipelines will not run the associated pipelines, because the pull request was updated after the run command was issued. Review the pull request again and issue a new run command. |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@judyjoseph please review comment feedback. |
|
|
||
| self.log_debug("End liquid cooling updating") | ||
|
|
||
| if self.task_stopping_event.is_set(): |
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.
can we check if there is task_stopping_event.is_set() before calling update ?
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 understand, I check this flag in line 497, only it is not set, will the update be executed.
| def _refresh_leak_status(self): | ||
| for index, sensor in enumerate(self.liquid_cooling.leakage_sensors, start = 1): | ||
| sensor_name = try_get(sensor.get_name, 'leakage{}'.format(index)) | ||
| sensor_leak_status = sensor.is_leak() |
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.
How is the failure condition handled ?
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 is_leak will simply return the sensor object status, which is a string as an attribute of the sensor object, this operation can not failed, no exception can happen
|
|
||
| assert liquid_cooling_updater._refresh_leak_status.call_count == 1 | ||
|
|
||
|
|
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.
Can we add a few more tests to cover the failure and exception cases
Signed-off-by: Yuanzhe, Liu <[email protected]>
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azpw run |
|
/AzurePipelines run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Description
Motivation and Context
Following this HLD for liquid cooling sonic-net/SONiC#2032, I created this pr to check liquid cooling device status periodically
This pr is part of the HLD and it has dependency on sonic-net/sonic-platform-common#603
How Has This Been Tested?
Additional Information (Optional)