Add API design for leak sensor test bits - #2441
Conversation
|
/azp run |
|
No pipelines are associated with this pull request. |
|
Note: this is written on top of #2413 The intent here is that the API is simple enough that it can be implemented in software or hardware, as appropriate for each implementer's leak detection design, so let me know if this is a practical API for you to implement. |
|
@fraserg-arista would be great if we have this as a separate PR and get it merged first -- This is useful for community, Let me know your thoughts |
|
@chinmoy-nexthop f.y.i |
Thank you Judy. will take a look. |
|
|
||
| The get_leak_severity() API call has to determine whether any condition indicates a severe leak detected from a leak sensor.This decision could be based on: | ||
|
|
||
| When the test bit is enabled, the effective leak state for that sensor is forced to `True` even if the hardware leak input is clear. This affects only leak presence. The physical health returned by `is_leak_sensor_ok()` is unchanged, and `get_leak_severity()` continues to return the platform-defined severity for a leak on that sensor. The test bit is intended for validation and is cleared by default at boot. |
There was a problem hiding this comment.
a privileged caller that enables a test leak may be able to trigger the same destructive path as a real leak. yes, we talked about distinction between test leaks from real leaks, but I do not see that reflect in the design.
There was a problem hiding this comment.
Yeah, I think you're right here: the fact of this being a test is passed through to thermalctld but it only calculates the overall state of the system for bmcctld to act on, but bmcctld doesn't know that this is a test so it will apply the system policy.
|
|
||
| The get_leak_severity() API call has to determine whether any condition indicates a severe leak detected from a leak sensor.This decision could be based on: | ||
|
|
||
| When the test bit is enabled, the effective leak state for that sensor is forced to `True` even if the hardware leak input is clear. This affects only leak presence. The physical health returned by `is_leak_sensor_ok()` is unchanged, and `get_leak_severity()` continues to return the platform-defined severity for a leak on that sensor. The test bit is intended for validation and is cleared by default at boot. |
There was a problem hiding this comment.
i mean , the documented defaults are system_leak_policy=enabled and system_critical_leak_action=power_off here i see.
| 3. Aggregate per-sensor results into a system leak severity. | ||
| 4. Expose per-sensor and system leak status through Redis DB for BMC policy enforcement and CLI. | ||
| 5. Avoid additional PMON overhead on platforms that are not liquid cooled. | ||
| 6. Support a per-sensor test API that can force a leak indication without requiring physical liquid in the chassis. |
There was a problem hiding this comment.
I think the test bit intent is good, but we need to add the the design of “test vs real leak” with explicit flow diagram and boundaries . can you please add.
| | `set_test_leak(enable)` | Enables or disables a per-sensor test bit that forces the sensor to report a leak | | ||
| | `is_test_leak_enabled()` | Returns whether the per-sensor forced-leak test bit is enabled | |
There was a problem hiding this comment.
is there a way to disable these two APIs in production? Since they appear to be intended only for testing, we may not need them in production. Leaving them enabled could introduce a potential misuse risk.
There was a problem hiding this comment.
An interesting question! Do you know if we have any models in Sonic for this kind of behaviour? (e.g. something that needs a configuration setting to enable a test/debug mode)
|
|
||
| - `syslog_only` | ||
| - `graceful_shutdown` | ||
| - `power_off` |
There was a problem hiding this comment.
Would it make sense to limit the graceful_shutdown/power-off action to cases where the leak source is a test injection? What do you think?
| | get_leak_severity() | Get the per-sensor leak severity as `LeakSeverity.CRITICAL`, `LeakSeverity.MINOR`, or `None` | | ||
| | get_leak_profile() | Return the `LeakSensorProfileBase` associated with this leak sensor type | | ||
| | set_test_leak(enable) | Enable or disable a per-sensor test bit that forces the sensor to report a leak | | ||
| | is_test_leak_enabled() | Return whether the per-sensor forced-leak test bit is enabled | |
There was a problem hiding this comment.
@fraserg-arista Can we have a separate LeakTest class eg: LeakageSensorTestBase where we can define a set of APIs which every platform owner can implement in their platform modules ? This way we can simulate any of the above platofrm API's in the sonic-mgmt test infra
| - power_off : Immediately power off Switch-Host via platform API module->set_admin_state(DOWN). | ||
| ``` | ||
|
|
||
| * **config liquid-cool leak-test sensor** |
There was a problem hiding this comment.
@fraserg-arista let me know your thought .. I was thinking we really don't need a config command .. but instead using the platform_modules which each platform owner can implement and use it in the sonic_mgmt leak tests to simulate leaks.
We can discuss ideas in the coming workgroup meeting
The intention of this API is to allow platforms to expose leak test functionality in their liquid cooling implementation. The state is written to the state DB so that other platform components can distinguish between real and test leaks (e.g. by logging that they would trigger a power cycle, without actually carrying out the action). Signed-off-by: Fraser Gordon <fraserg@arista.com>
77ffed4 to
99e5cda
Compare
|
/azp run |
|
No pipelines are associated with this pull request. |
4 participants
The intention of this API is to allow platforms to expose leak test functionality in their liquid cooling implementation. The state is written to the state DB so that other platform components can distinguish between real and test leaks (e.g. by logging that they would trigger a power cycle, without actually carrying out the action).