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
|
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]
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]
9ef9f23 to
7077649
Compare
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
Hi @sonic-net/sonic-platform-common-maintainer, this approved PR has workflow run(s) waiting for approval. Please help review. Thanks! ---Powered by SONiC BuildBot
|
rebuild-source: sonic-net#735 @ nexthop-ai/sonic-platform-common 7077649 [case: upstream:open]
rebuild-source: sonic-net/pull/735 @ nexthop-ai/sonic-platform-common 7077649 [case: upstream:open]
Thank you @judyjoseph , Removed the redundant test_leak class attribute from LeakageSensorBase |
7077649 to
05f8e3a
Compare
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
nikamirrr
left a comment
There was a problem hiding this comment.
Thanks for the rework — the is_leak() / get_leak_severity() / is_test_leak() contract landed as we discussed on the Option 1 thread, and the TYPE_CHECKING guard for the LeakageSensorTestBase forward reference is in. CI is green here.
One code item left, plus a process one:
is_test_leak()is no longer safe on subclasses that skipsuper().__init__(). The reply on the line-34 thread said the class-leveltest_leak = Falsewas kept as a read fallback; it is not on this head, so a leaking out-of-tree sensor now raisesAttributeError. One-line fix, commented inline.- The Option 1 follow-ups. The two
sonic-buildimagechanges this depends on do not seem to be raised yet, and the caveat that was going into the PR description is not there. Until the ast2700 change lands,is_test_leak()is hardwired toFalseon that platform — worth stating in the description so it is discoverable.
The third comment is a suggestion on the reference clear_test_leaks(), which vendors will copy.
Happy to re-review as soon as the first one is addressed.
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>
05f8e3a to
7faa761
Compare
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
Cherry-pick PR to msft-202608: Azure/sonic-platform-common.msft#137 |
|
Thank you for looking into this, @judyjoseph @nikamirr @parvathi-nexthop . I really appreciate your valuable comments and feedback. |
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)