Add vendor-independent leak test API - #735
Conversation
|
/azp run |
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
@fraserg-arista to review as well, this could be first step on sonic-net/SONiC#2441 |
nikamirrr
left a comment
There was a problem hiding this comment.
Automated review pass focused on correctness. Fifteen inline comments below.
Findings 1, 2, 6, 7, 8, 9, 10, 11, 14 and 15 were confirmed by executing the code rather than
by reading it — including running the full LeakTestApiBase suite against both a correct and a
deliberately-sloppy implementation.
The through-line is the PR's central safety promise: an injected leak is published like a real one
but must never trigger a mitigation action. Three independent gaps each break it on their own:
is_test_leak()is decoupled fromis_leak(), so a stale flag exempts a real leak from
mitigation.- The reference
clear_test_leaks()erases real leaks on sensors that were never injected. - The flag reaches no consumer and no STATE_DB field, while
set_test_leakdefaults toCRITICAL
andsystem_critical_leak_actiondefaults topower_off— so a "non-destructive" test can power
the switch off.
The conformance suite cannot catch any of the three: I built an implementation that is wrong in
exactly the dangerous way and it passes 8/8.
Also worth a look, below the cut: a shared mutable default leakage_sensors_list=[] in
LiquidCoolingBase.__init__; test_injection_is_non_destructive being a tautology that passes when
injection is a no-op; and @abstractmethod ... pass against this repo's own written rule in
.github/copilot-instructions.md ("Abstract methods: Raise NotImplementedError in base class"),
which also makes super().set_test_leak(...) silently return a falsy None.
|
This PR has backport request label(s) for branch(es): msft-202608, but is missing required test information. Please make sure you tick the tested branch(es) in the Tested branch section and provide test evidence (e.g., 202608: <test result>) in the Test result section as well in your PR description. ---Powered by SONiC BuildBot
|
Add a common API for injecting a simulated leak into the leak detection path, so the reporting chain can be validated without wetting hardware. Injection is non-destructive: an injected leak is published like any other leak, and is additionally flagged through LeakageSensorBase is_test_leak() so consumers must not take a mitigation action on it. - leakage_sensor_test_base.py: new LeakageSensorTestBase defining is_leak_test_supported(), set_test_leak(), is_test_leak_enabled() and clear_test_leaks() - liquid_cooling_base.py: add is_test_leak() on LeakageSensorBase and get_leak_sensor_test() on LiquidCoolingBase, defaulting to False and None so platforms without injection support are unaffected - tests/leak_test_api_base.py: LeakTestApiBase, a reusable conformance suite a platform subclasses to validate its implementation against the common contract Signed-off-by: Chinmoy Dey <chinmoy@nexthop.ai>
508549d to
9ef9f23
Compare
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
@nikamirrr Thank you. Addressed the following:-
|
|
Hi, there are workflow run(s) waiting for approval, you may be first-time contributor. I will notify maintainers to help approve once PR is approved. Thanks! ---Powered by SONiC BuildBot
|
rebuild-source: sonic-net#735 @ nexthop-ai/sonic-platform-common 9ef9f23 [case: upstream:open]
rebuild-source: sonic-net#735 @ nexthop-ai/sonic-platform-common 9ef9f23 [case: upstream:open]
rebuild-source: sonic-net#735 @ nexthop-ai/sonic-platform-common 9ef9f23 [case: upstream:open]
rebuild-source: sonic-net#735 @ nexthop-ai/sonic-platform-common 9ef9f23 [case: upstream:open]
rebuild-source: sonic-net#735 @ nexthop-ai/sonic-platform-common 9ef9f23 [case: upstream:open]
rebuild-source: sonic-net#735 @ nexthop-ai/sonic-platform-common 9ef9f23 [case: upstream:open]
rebuild-source: sonic-net#735 @ nexthop-ai/sonic-platform-common 9ef9f23 [case: upstream:open]
rebuild-source: sonic-net#735 @ nexthop-ai/sonic-platform-common 9ef9f23 [case: upstream:open]
rebuild-source: sonic-net#735 @ nexthop-ai/sonic-platform-common 9ef9f23 [case: upstream:open]
Add a common API for injecting a simulated leak into the leak detection path, so the reporting chain can be validated without wetting hardware.
Injection is non-destructive: an injected leak is published like any other leak, and is additionally flagged through LeakageSensorBase is_test_leak() so consumers must not take a mitigation action on it.
Description
Motivation and Context
How Has This Been Tested?
Additional Information (Optional)