Cast temp workaround for N/A counter values on Broadcom platforms from 202412#1955
Cast temp workaround for N/A counter values on Broadcom platforms from 202412#1955justin-wong-ce wants to merge 1 commit into
Conversation
|
/azp run |
|
Azure Pipelines will not run the associated pipelines, because the pull request was updated after the run command was issued. Review the pull request again and issue a new run command. |
|
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
|
Casting temp-workaround for N/A counters on management ports on 202605. This impacts all hwsku-topo combos that uses management ports (including TH5). Casting from: Azure/sonic-sairedis.msft#69 Proper fix under review at: master: sonic-net#1774 202511: sonic-net#1862 202605: Opened soon Signed-off-by: Justin Wong <jvwong@arista.com>
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@justin-wong-ce - Thanks for carrying this temporary 202605 workaround. I have one concern before this merges: the PR description scopes the fix to Broadcom management-port Could you either scope the zero-fill to the known Broadcom management-port unsupported-counter case, or confirm why it is safe for all single-counter One minor thing as well: the new |
lolyu
left a comment
lolyu
left a comment
There was a problem hiding this comment.
Review: #1955
Reviewed the full FlexCounter.cpp and the inner collectData overload to verify the semantics, not just the diff. As a temporary workaround (superseded by #1774/#1862/#1956) the approach is acceptable and correctly scoped — single file, no ABI/header/test churn.
Verified the true→false flip on collectData is intentional and correct: the 4th arg is log_err, so the flip only downgrades the per-poll failure log from ERROR to INFO (it doesn't change control flow or counters). That's the right call here, since failed reads are now the expected handled case and an ERROR every poll interval would spam syslog. And stats(statIds.size(), 0) is load-bearing — without the zero-init a failed read would write uninitialized stack values to Redis.
Two things worth addressing before merge (inline): #1 is a concrete bug — the two new log lines use %x for sai_object_id_t (uint64), which is UB and will misprint the OID. #2 is a behavior flag — "transient failure now publishes 0" affects all ports, not just management ports, and deserves an explicit comment so it's a documented tradeoff. Neither is hard to fix; #1 is a two-character change with the correct pattern already in the same function.
| SWSS_LOG_WARN("%s RID %s can't provide the statistic", m_name.c_str(), sai_serialize_object_id(rid).c_str()); | ||
| if (!collectData(rid, counter_ids, effective_stats_mode, false, stats)) | ||
| { | ||
| SWSS_LOG_INFO("counter read failed on RID 0x%x on intf 0x%x, adding to objectIdsMap regardless", rid, vid); |
There was a problem hiding this comment.
🟠 Format-specifier bug (UB). rid and vid are sai_object_id_t = uint64_t, but 0x%x is a 32-bit unsigned int specifier. Passing a uint64_t to %x is undefined behavior and will misprint/truncate the OID (you'll get the wrong half of the 64-bit value, and on some ABIs the second arg shifts) — which defeats the log's purpose when someone is debugging which port failed. The file's own convention is right here in the same function: use 0x%\" PRIx64 (cf. the Fallback to single call for object 0x%\" PRIx64 lines just above) or sai_serialize_object_id(rid).c_str(). Suggest:
SWSS_LOG_INFO("counter read failed on RID 0x%\" PRIx64 \" on intf 0x%\" PRIx64 \", adding to objectIdsMap regardless", rid, vid);(Same fix needed on the sibling line in the main collectData loop ~1162.)
| if (!collectData(rid, statIds, effective_stats_mode, false, stats)) | ||
| { | ||
| continue; | ||
| SWSS_LOG_INFO("counter read failed on RID 0x%x on intf 0x%x, filling with '0' value", rid, vid); |
There was a problem hiding this comment.
🟠 Same %x-for-uint64_t UB as line 995 — fix this one too:
SWSS_LOG_INFO("counter read failed on RID 0x%\" PRIx64 \" on intf 0x%\" PRIx64 \", filling with '0' value", rid, vid);| std::vector<uint64_t> stats(statIds.size()); | ||
| if (!collectData(rid, statIds, effective_stats_mode, true, stats)) | ||
| std::vector<uint64_t> stats(statIds.size(), 0); | ||
| if (!collectData(rid, statIds, effective_stats_mode, false, stats)) |
There was a problem hiding this comment.
🟡 Semantic behavior change worth an explicit comment. The old code did continue on a failed read — leaving the previous Redis value in place. This now writes 0 for every counter on failure. For management ports that never support the counter, zeros-forever is exactly the intended fix. But for a normal port with a transient SAI hiccup, this publishes a spurious 0, which downstream rate math (show interface counters, SNMP, gNMI) reads as a counter reset to zero — a false "counters cleared"/traffic-stopped blip, plus a rate spike on the next good poll as the value jumps back. Since this is an acknowledged temp workaround that may be an accepted tradeoff, but it affects all ports (not just management), please add a code comment noting the intentional "publish 0 on read failure" behavior so it's documented rather than a field surprise. The proper fix (#1774) avoids this by only polling per-port-supported counters.
| if (it_vid != m_objectIdsMap.end()) | ||
| { | ||
| // Remove and re-add if vid already exists | ||
| m_objectIdsMap.erase(it_vid); |
There was a problem hiding this comment.
🟡 This now inserts into m_objectIdsMap unconditionally (previously gated behind a successful collectData). Consistent with the workaround's intent (poll regardless), but it means an object whose counters genuinely never succeed stays in the map and is polled+fails every interval (now at INFO). Bounded and intended — worth a one-line comment that the unconditional insert is deliberate, so a future reader doesn't mistake it for the success-gating that was here before.
|
@StormLiangMS , regarding your concerns:
I agree that there is a risk where failures are masked away - this is precisely why this was a temporary workaround limited to specific branches. This is a compromise that has been acknowledged when this temporary workaround is done for the 202412 and 202505 branch. Counter values also cannot go negative as unsigned ints are used for its storage, and anything not a number will cause issues in testing (i.e.
The existing FlexCounter implementation is unable to distinguish between ports - there is no notion of multiple ports where all ports are treated as the same. This is precisely why the full fix is needed fast. Any sort of check on interfaces will involve so many parts that it will either be extremely inperformant or become the full fix itself.
Will do. |
|
Using #1962 as the merging PR |
4 participants
Description of PR
Summary:
Casting temp-workaround for N/A counters on management ports on 202605. This impacts all hwsku-topo combos that uses management ports (including TH5).
Casting from:
Azure/sonic-sairedis.msft#69
Proper fix under review at:
master: #1774
202511: #1862
202605: #1956
Fixes #1753 (temporary fix)
Type of change
Approach
What is the motivation for this PR?
To fix
N/Acounters on management ports on Broadcom platforms.Work item tracking
How did you do it?
Temporarily fill in
0values for counters that are supported in the usual interfaces but not supported in the management ports.How did you verify/test it?
show interface countersno longer showN/Acounters.Any platform specific information?
Broadcom only.
Documentation