Skip to content

Add vendor-independent leak test API - #735

Open
chinmoy-nexthop wants to merge 1 commit into
sonic-net:masterfrom
nexthop-ai:chinmoy.leak-test.api.base
Open

Add vendor-independent leak test API#735
chinmoy-nexthop wants to merge 1 commit into
sonic-net:masterfrom
nexthop-ai:chinmoy.leak-test.api.base

Conversation

@chinmoy-nexthop

Copy link
Copy Markdown

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

Description

Motivation and Context

How Has This Been Tested?

Additional Information (Optional)

@mssonicbld

Copy link
Copy Markdown
Collaborator

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
There may be pipelines that require an authorized user to comment /azp run to run.

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).

@judyjoseph

Copy link
Copy Markdown
Contributor

@fraserg-arista to review as well, this could be first step on sonic-net/SONiC#2441

@nikamirrr nikamirrr left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Duplicate submission from a retried API call — superseded by the review below, which carries all 15 inline comments. Please disregard this entry.)

@nikamirrr nikamirrr left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. is_test_leak() is decoupled from is_leak(), so a stale flag exempts a real leak from
    mitigation.
  2. The reference clear_test_leaks() erases real leaks on sensors that were never injected.
  3. The flag reaches no consumer and no STATE_DB field, while set_test_leak defaults to CRITICAL
    and system_critical_leak_action defaults to power_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.

Comment thread sonic_platform_base/liquid_cooling_base.py Outdated
Comment thread tests/leakage_sensor_test_base_test.py Outdated
Comment thread sonic_platform_base/liquid_cooling_base.py
Comment thread tests/leak_test_api_base.py Outdated
Comment thread sonic_platform_base/leakage_sensor_test_base.py
Comment thread sonic_platform_base/liquid_cooling_base.py
Comment thread sonic_platform_base/leakage_sensor_test_base.py
Comment thread tests/leak_test_api_base.py Outdated
Comment thread tests/leak_test_api_base.py Outdated
Comment thread sonic_platform_base/liquid_cooling_base.py
@mssonicbld

Copy link
Copy Markdown
Collaborator

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>
@chinmoy-nexthop
chinmoy-nexthop force-pushed the chinmoy.leak-test.api.base branch from 508549d to 9ef9f23 Compare August 14, 2026 09:20
@mssonicbld

Copy link
Copy Markdown
Collaborator

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).

@chinmoy-nexthop

Copy link
Copy Markdown
Author

(Duplicate submission from a retried API call — superseded by the review below, which carries all 15 inline comments. Please disregard this entry.)

@nikamirrr Thank you.
It is absolutely fine. Maybe your agent was a little too verbose. I have looked into your feedback and taken care of a few things that I felt were appropriate.

Addressed the following:-

  • test_leak is now a class attribute, so is_test_leak() is safe on subclasses that don't chain super().init()
  • is_test_leak() only reports True while the sensor is actually leaking, so a stale flag can't mask a real leak
  • get_leak_severity() returns None when there's no leak, as documented
  • Fixed get_profile() — it was using getattr() on a dict and could never find a profile
  • Added a TYPE_CHECKING import so the LeakageSensorTestBase annotation resolves
  • Moved the conformance suite into sonic_platform_base/ so it ships in the wheel, and added an in-repo subclass so it runs in CI
  • Suite now clears injected leaks on teardown even when an assertion fails, fails loudly if SENSOR_NAMES isn't set, and verifies is_test_leak() is False after withdrawal
  • Reference clear_test_leaks() only touches injected sensors, restores prior sensor state, and returns the aggregated result

@mssonicbld

Copy link
Copy Markdown
Collaborator

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

nh-grecs Bot pushed a commit to nexthop-ai/sonic-platform-common that referenced this pull request Aug 23, 2026
rebuild-source: sonic-net#735 @ nexthop-ai/sonic-platform-common 9ef9f23 [case: upstream:open]
nh-grecs Bot pushed a commit to nexthop-ai/sonic-platform-common that referenced this pull request Aug 24, 2026
rebuild-source: sonic-net#735 @ nexthop-ai/sonic-platform-common 9ef9f23 [case: upstream:open]
nh-grecs Bot pushed a commit to nexthop-ai/sonic-platform-common that referenced this pull request Aug 25, 2026
rebuild-source: sonic-net#735 @ nexthop-ai/sonic-platform-common 9ef9f23 [case: upstream:open]
nh-grecs Bot pushed a commit to nexthop-ai/sonic-platform-common that referenced this pull request Aug 26, 2026
rebuild-source: sonic-net#735 @ nexthop-ai/sonic-platform-common 9ef9f23 [case: upstream:open]
nh-grecs Bot pushed a commit to nexthop-ai/sonic-platform-common that referenced this pull request Aug 27, 2026
rebuild-source: sonic-net#735 @ nexthop-ai/sonic-platform-common 9ef9f23 [case: upstream:open]
nh-grecs Bot pushed a commit to nexthop-ai/sonic-platform-common that referenced this pull request Aug 27, 2026
rebuild-source: sonic-net#735 @ nexthop-ai/sonic-platform-common 9ef9f23 [case: upstream:open]
nh-grecs Bot pushed a commit to nexthop-ai/sonic-platform-common that referenced this pull request Aug 28, 2026
rebuild-source: sonic-net#735 @ nexthop-ai/sonic-platform-common 9ef9f23 [case: upstream:open]
nh-grecs Bot pushed a commit to nexthop-ai/sonic-platform-common that referenced this pull request Aug 29, 2026
rebuild-source: sonic-net#735 @ nexthop-ai/sonic-platform-common 9ef9f23 [case: upstream:open]
nh-grecs Bot pushed a commit to nexthop-ai/sonic-platform-common that referenced this pull request Aug 29, 2026
rebuild-source: sonic-net#735 @ nexthop-ai/sonic-platform-common 9ef9f23 [case: upstream:open]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants