Skip to content

[BMC] Align get_presence for the new infra#685

Open
benle7 wants to merge 1 commit into
sonic-net:masterfrom
benle7:host_bmc_communication
Open

[BMC] Align get_presence for the new infra#685
benle7 wants to merge 1 commit into
sonic-net:masterfrom
benle7:host_bmc_communication

Conversation

@benle7

@benle7 benle7 commented Jun 2, 2026

Copy link
Copy Markdown
Contributor

Description

  • Updated sonic_platform_base/bmc_base.py to check:
    • device_info.is_switch_host()
    • device_info.get_bmc_data()
  • Removed the older bmc_addr-only presence condition.
  • Updated tests/bmc_base_test.py to match the new behavior:
    • present on Switch-Host with bmc data
    • not present when bmc data is missing
    • not present on non-host even if bmc data exists

Motivation and Context

BMCBase.get_presence() was aligned with the new host/BMC split (sonic-net/sonic-buildimage#26544)
so BMC presence is reported only for Switch-Host platforms with runtime BMC data.
This avoids treating Switch-BMC side execution as BMC-present for host-only logic.

How Has This Been Tested?

  • Run unit tests:
    • pytest tests/bmc_base_test.py
  • Validate platform API behavior manually:
    • bmc.get_presence() returns True only on Switch-Host with valid /etc/sonic/bmc.json
    • returns False on Switch-BMC side or when BMC data is unavailable

Additional Information (Optional)

Signed-off-by: Ben Levi <belevi@nvidia.com>
@mssonicbld

Copy link
Copy Markdown
Collaborator

/azp run

@azure-pipelines

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

@benle7

benle7 commented Jun 15, 2026

Copy link
Copy Markdown
Contributor Author

@judyjoseph Can you please help to merge?
Thanks!

@oleksandrivantsiv oleksandrivantsiv requested a review from yxieca June 15, 2026 22:17

@yxieca yxieca left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Reviewed by AI agent on behalf of Ying.

Decision: Approve — clean, well-tested alignment change. CI green, zero regression on non-BMC platforms (old and new both return False there); the change is confined to Switch-Host/Switch-BMC platforms as intended. Two non-blocking suggestions:

  1. Docstring (please consider updating): get_presence() is an abstract-base method that platform implementations inherit, and its behavior changed, but the docstring still reads only "Check if the BMC device is present." Suggest documenting the new contract so it's the spec vendors rely on, e.g.:

    Returns True only on the Switch-Host side (is_switch_host()) when runtime BMC data (/etc/sonic/bmc.json) is available; returns False on the Switch-BMC side or when BMC data is absent.

  2. Presence condition semantics: the old check required the bmc_addr key specifically (bmc_data.get('bmc_addr')); the new get_bmc_data() is truthy for any non-empty bmc.json dict. So a bmc.json present but missing bmc_addr would now report present on a Switch-Host (previously False). In practice get_bmc_data() returns a fully-populated dict, so this is inert — but if the intended invariant is specifically "a BMC address is known," device_info.get_bmc_address() would preserve the stricter check. Otherwise, the docstring note in (1) documents "bmc.json present on host" as the intended signal. A test for the bmc_addr-less dict case would also lock in whichever semantic you choose.

Neither blocks merge.

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