Auto-use enablePerPortCounterDiscovery syncd arg on Broadcom ASICs

[justin-wong-ce]

Description of PR

Summary:
Auto-use the enablePerPortCounterDiscovery syncd arg for Broadcom ASICs in an easily expandable way such that other ASIC types can be easily added to use this behavior.

This is part 2 of 2 fix in addressing sonic-net/sonic-buildimage#28460. There are two fix methods under consideration, this PR outlines the method of deciding code path via the ASIC type.

This change requires the changes in #2000 to work.

Fixes -
Partially Fixes: sonic-net/sonic-buildimage#28460

Type of change

• Bug fix
• New feature
• Refactor / cleanup
• Documentation update
• Test improvement

Approach

What is the motivation for this PR?

In #1774 , a new counter support discovery method is introduced for being able to read counter support for platforms where different ports can have different capabilities.
The method worked well for some platforms, not so much for some others.

This change auto-uses the new syncd arg enablePerPortCounterDiscovery (introduced in #2000) when the broadcom ASIC type is detected in syncd_init_common.sh.

Work item tracking

• Microsoft ADO (number only):

How did you do it?

Add logic in syncd_init_common.sh to auto apply the syncd arg when broadcom ASIC type is detected.

How did you verify/test it?

syncd unit tests pass on master and on 202605.
On physical hardware, an image is built with the changes cherry-picked to 202605 and tests are ran in 2 scenarios:

1. With the change as is
2. syncd_init_common.sh hotpatched to filter on another asic type.

In both scenarios, syncd enters the expected code path.
Additionally, a subset of counters-focused sonic-mgmt tests are ran. There is no test result difference compared to tests done without this change.
Tests were ran on Arista-7260CX3-D108C8 and Arista-7060X6-64PE-B-C512S2.

The list of sonic-mgmt tests ran:

test_pretest.py
dhcp_relay/test_dhcp_counter_stress.py
drop_packets/test_drop_counters.py
drop_packets/test_configurable_drop_counters.py
gnmi/test_gnmi_countersdb.py
snmp/test_snmp_queue_counters.py
test_posttest.py

To ensure no warm and fast reboot issues from any potential time increase from syncd, some reboot-related tests are also ran. The following sonic-mgmt tests pass with this change casted, tested on Arista-7260CX3-D108C8 and Arista-7060CX-32S-C32:

platform_tests/test_advanced_reboot.py::test_warm_reboot_sad*
platform_tests/test_advanced_reboot.py::test_warm_reboot_mac_jump
platform_tests/test_advanced_reboot.py::test_warm_reboot
platform_tests/test_advanced_reboot.py::test_fast_reboot

Any platform specific information?

Broadcom

Documentation

[mssonicbld]

/azp run

[azure-pipelines]

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

[azure-pipelines]

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

[azure-pipelines]

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

[lolyu]

Overall the approach is sound, but a few points worth addressing:

1. Placement — -G is added outside config_syncd(), unlike every other ASIC arg.
All ASIC-specific arg logic lives in config_syncd_*() functions dispatched from config_syncd(), and there's already a config_syncd_bcm() doing SONIC_ASIC_SUBTYPE-gated handling. Adding this new case at top level (right after the bulk-counter block) splits Broadcom handling across two places, which a future reader won't expect. Moving -G into config_syncd_bcm() keeps all broadcom logic together and consistent with the rest of the file.

If the intent is genuinely a multi-ASIC-expandable table via one case, that's a fair design goal — but then it shouldn't be broadcom-only sitting outside the dispatch.

2. No explanatory comment. Every other non-obvious arg here has at least a hint. A bare -G tells the reader nothing. Please add a comment referencing the motivation (per-port counter discovery, #1774 / sonic-buildimage#28460).

Suggested:

# Enable per-port counter capability discovery (see sonic-buildimage#28460, #1774).
# Broadcom platforms can have per-port counter support differences.
CMD_ARGS+=" -G"

3. Boot-type ordering. This runs before config_syncd() / warm-fast-reboot arg assembly, so -G is unconditionally present across all boot types including warm/fast/express reboot. Presumably intended, but worth confirming per-port counter discovery is harmless (and desired) during warm/fast reboot.

4. Hard dependency on #2000. -G is unknown to syncd until #2000 merges; if merge order is reversed, syncd fails to start on Broadcom. Please ensure #2000 lands first (worth calling out explicitly / gating).

[justin-wong-ce]

If the intent is genuinely a multi-ASIC-expandable table via one case, that's a fair design goal — but then it shouldn't be broadcom-only sitting outside the dispatch.

This is the intention, however it is currently unknown which platform the issue is affecting, therefore I do not know what other additional platforms to add here.

2. No explanatory comment. Every other non-obvious arg here has at least a hint. A bare -G tells the reader nothing. Please add a comment referencing the motivation (per-port counter discovery, #1774 / sonic-buildimage#28460).

Ack, will add.

3. Boot-type ordering. This runs before config_syncd() / warm-fast-reboot arg assembly, so -G is unconditionally present across all boot types including warm/fast/express reboot. Presumably intended, but worth confirming per-port counter discovery is harmless (and desired) during warm/fast reboot.

This is intentional. Warm and fast reboot will need to run this in order for counters to be setup properly in those scenarios as well.
The following sonic-mgmt tests pass with this change casted, tested on Arista-7260CX3-D108C8 and Arista-7060CX-32S-C32:

platform_tests/test_advanced_reboot.py::test_warm_reboot_sad*
platform_tests/test_advanced_reboot.py::test_warm_reboot_mac_jump
platform_tests/test_advanced_reboot.py::test_warm_reboot
platform_tests/test_advanced_reboot.py::test_fast_reboot

Will add this testing info to the PR description.

4. Hard dependency on #2000. -G is unknown to syncd until #2000 merges; if merge order is reversed, syncd fails to start on Broadcom. Please ensure #2000 lands first (worth calling out explicitly / gating).

Yes.

[mssonicbld]

/azp run

[azure-pipelines]

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

[lolyu]

/azp run

[azure-pipelines]

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

[mssonicbld]

/azp run

[azure-pipelines]

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

[justin-wong-ce]

Reminder this needs backport to 202605.

[lolyu]

/azp run

[azure-pipelines]

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

[lolyu]

LGTM

[mssonicbld]

This PR has backport request label(s) for branch(es): 202605, 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., 202605: <test result>) in the Test result section as well in your PR description.

_{---Powered by SONiC BuildBot}

[rookie-who]

Hi — this PR is flagged for cherry-pick to the 202605 branch (Request for 202605 Branch). To move forward with the cherry-pick, could you please:

1. Test this change on the 202605 branch and add the test results to the PR description
2. Add the label Tested for 202605 branch once testing is complete

This helps us track readiness and ensures we don't cherry-pick untested changes. Thanks!

Comment by @vaibhavhd via automated tooling