Add isolate reason, isolated link count, and -nz filtering for fabric cli commands - #4641
Conversation
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@vmittal-msft @ysmanman @kenneth-arista , Please review. |
There was a problem hiding this comment.
Pull request overview
This PR updates SONiC’s fabric CLI surface area (both the show fabric ... Click commands and the underlying fabricstat script) to expose additional fabric isolation/capacity telemetry and to add optional “non-zero only” filtering to reduce noise in command outputs.
Changes:
- Add
ISOLATE_REASONdisplay to fabric isolation output and introduce-nz/--nonzerofiltering for isolation, port, queue, and rate views. - Extend fabric capacity output to include
isolated_linksalongside operating/total links. - Update unit tests and mock STATE_DB/COUNTERS_DB fixtures to cover the new columns and nonzero filtering behavior (primarily for
fabricstat).
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/mock_tables/state_db.json | Adds isolated_links to fabric capacity fixture data. |
| tests/mock_tables/counters_db.json | Adjusts a counter fixture value to align with new nonzero filtering expectations. |
| tests/mock_tables/asic1/state_db.json | Adds isolation flags/reason fields and isolated_links for multi-ASIC fixture coverage. |
| tests/mock_tables/asic1/counters_db.json | Adjusts FEC uncorrectable counters to support nonzero-delta filtering tests. |
| tests/mock_tables/asic0/state_db.json | Adds isolation flags/reason fields and isolated_links for asic0 fixture coverage. |
| tests/mock_tables/asic0/counters_db.json | Adjusts a counter fixture value to align with nonzero filtering expectations. |
| tests/fabricstat_test.py | Expands expected outputs and adds new test cases for fabricstat -nz modes. |
| show/fabric.py | Adds -nz/--nonzero options to show fabric commands and forwards to fabricstat. |
| scripts/fabricstat | Implements isolate reason display, isolated link count in capacity output, and nonzero filtering logic. |
There was a problem hiding this comment.
Thanks for the change — I went through the full diff. Overall it looks good and the logic is consistent. A few notes below: one bug-fix call-out, agreement with the two automated comments, and a few items those didn't cover.
Nice catch / bug fix
Moving the rxRate/txRate/rxData/txData/time/local_time initialization inside the per-port loop in FabricRate.rate_print fixes a real state-leak — ports missing rate fields previously inherited the prior port's values. Confirmed against the mock data (ports 1/3/5 have no OLD_*_RATE_AVG, and the old expected output was asserting those leaked values). Good fix.
On the two automated review comments — both valid
- Missing click-layer tests for
-nz.fabricstat_test.pyexercises the script directly via subprocess, so theshow fabric ...click forwarding (if nonzero: cmd += ['-nz']) for port/queue/rate/isolation is untested.tests/show_test.py::TestShowFabricalready coversport/queue— worth adding-nzvariants there. N/Atreated as non-zero._fabric_port_row_has_nonzero_counterreturnsTrueforNone(N/A), so N/A rows are always shown while the help text says "non-zero". Reasonable behavior, but the help could call it out.
A few additional items
- Line length:
scripts/fabricstat~line 394 (the wrappedtable_cnt.append(...)) is 123 chars, over the repo's--max-line-length=120. It was added in the first commit, so the diff-based pre-commit hook won't flag it, but a full flake8 run will. - Inconsistent empty-table behavior under
-nz:rate_printreturns and prints nothing when the filtered body is empty, while isolation/port/queue still print a header with no rows. Minor, but worth making uniform. -nzsemantics differ by view: for the fullport/queueviews the delta is computed vs. saved/cleared stats, so without a prior-Cthe delta equals the absolute counter and nearly every active link shows — it's most meaningful after a clear there (isolation/rate reflect current state regardless). Might be worth a doc note.-nzis silently ignored with-c(capacity) and reachability — fine, just undocumented.
I also confirmed -nz does not collide with -n/namespace in either argparse or click (exact registered option strings win), so no problem there.
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azpw run |
|
Retrying failed(or canceled) jobs... |
|
Retrying failed(or canceled) stages in build 1152028: ✅Stage Build:
|
Addressed these review comments |
|
/azpw run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
… cli commands Signed-off-by: saksarav <sakthivadivu.saravanaraj@nokia.com>
Signed-off-by: saksarav <sakthivadivu.saravanaraj@nokia.com>
Signed-off-by: saksarav <sakthivadivu.saravanaraj@nokia.com>
3ea8230 to
959a3a9
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@kenneth-arista and @vmittal-msft to sign-off |
vmittal-msft
left a comment
There was a problem hiding this comment.
Thanks for addressing the earlier round — the click-layer -nz tests (port/queue/rate/isolation), the empty-table early-returns, the semicolon (E703) cleanup, and the N/A note in help text all look good.
A few small nits remain. None fail CI (the pre-commit hook only lints the last commit's HEAD^..HEAD diff), but a full flake8 --max-line-length=120 scripts/fabricstat flags two of them:
-
scripts/fabricstat:625— line too long (128 > 120, E501). The newcapacity_header = [...]list is 128 chars. This is the same >120 line-length class as the earliertable_cnt.appendnote, just reintroduced on the capacity header. Suggest wrapping the list elements across lines. -
scripts/fabricstat:205— block comment should start with#(E265).#If the stats was not cleared before...— add a space after#. (This one was added in the "Addressed review comments" commit; the-U0diff-based flake8 happens to miss it.) -
scripts/fabricstat:564— help-text typos.' (rows with N/A are always shown);. Nout used with capacity and reachability 'has "Nout" (→ "Not") and a stray;.. Minor, but user-visible infabricstat -h. -
Queue empty-table under
-nzis still inconsistent.FabricPortStat/FabricIsolation/FabricRatenow early-return and print nothing when the filtered result is empty, butFabricQueueStat.cnstat_printstill prints a bare header with no rows. Worth adding the sameif nonzero_only and len(table) == 0: returnbefore thetabulate(...)for uniformity.
All cosmetic/polish — no functional concerns. Happy to approve once these are cleaned up (or if you'd prefer to defer #4 as a follow-up, that's fine).
Signed-off-by: saksarav <sakthivadivu.saravanaraj@nokia.com>
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
Signed-off-by: saksarav <sakthivadivu.saravanaraj@nokia.com>
6645ec1 to
df0f4da
Compare
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
This PR has backport request label(s) for branch(es): msft-202601, 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., 202601: <test result>) in the Test result section as well in your PR description. ---Powered by SONiC BuildBot
|
Signed-off-by: saksarav <sakthivadivu.saravanaraj@nokia.com>
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
@vmittal-msft @kenneth-arista @jfeng-arista , Please review this and sign off |
vmittal-msft
left a comment
There was a problem hiding this comment.
Re-review (head 5b654f9) — following up on the Jul 15 round.
Thanks for the updates. 3 of the 4 items are resolved:
- ✅
capacity_headerline length (E501) — wrapped across lines; a fullflake8 --max-line-length=120 scripts/fabricstatnow flags no PR-added line. - ✅
# If the stats was not cleared...block comment (E265) — fixed (space after#). - ✅ Queue empty-table under
-nz—FabricQueueStat.cnstat_printnow early-returns when the filtered table is empty, consistent with Port/Isolation/Rate.
One item is only partially addressed:
⚠️ -nzhelp text (scripts/fabricstat~L570):Nout→Notis fixed, but the stray;.I flagged earlier is still present (...always shown);. Not used...), plus a double space where the two string fragments join. Both are user-visible infabricstat -h. Concrete suggestion inline.
Optional (non-blocking): in FabricRate.rate_print, rxData/txData/local_time are computed but unused now (body.append uses only rxRate/txRate) — leftover dead reads from the state-leak fix; drop if convenient.
No new lint or functional regressions in the latest commits. Happy to approve once the help string is cleaned up (or if you'd prefer to defer the cosmetic bits, that's fine too).
Signed-off-by: saksarav <sakthivadivu.saravanaraj@nokia.com>
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
The change is not in msft-202601 yet. @saksarav-nokia, please manually create the cherry pick PR for branch msft-202601. ---Powered by SONiC BuildBot
|
… cli commands (sonic-net#4641) * Add isolate reason, isolated link count, and -nz filtering for fabric cli commands Signed-off-by: saksarav <sakthivadivu.saravanaraj@nokia.com> * Fixed the flake8 errors Signed-off-by: saksarav <sakthivadivu.saravanaraj@nokia.com> * Addressed review comments Signed-off-by: saksarav <sakthivadivu.saravanaraj@nokia.com> * Fix the review comments Signed-off-by: saksarav <sakthivadivu.saravanaraj@nokia.com> * Fixed the review comment Signed-off-by: saksarav <sakthivadivu.saravanaraj@nokia.com> * Fix the UT failure Signed-off-by: saksarav <sakthivadivu.saravanaraj@nokia.com> * Address the review comment Signed-off-by: saksarav <sakthivadivu.saravanaraj@nokia.com> --------- Signed-off-by: saksarav <sakthivadivu.saravanaraj@nokia.com> Signed-off-by: Ramesh Raghupathy <ram@cisco.com>
|
PR for msft-202601 |
What I did
Made the following changes in fabric cli commands . The corresponding sonic-swss PR sonic-net/sonic-swss#4704
Display ISOLATE_REASON from STATE_DB in fabric isolation output.
show isolated_links in fabric capacity alongside operating/total links, matching new FabricPortsOrch telemetry.
Add -nz/--nonzero to fabricstat and show fabric for port, queue, rate, and isolation views to hide rows with zero counter deltas or rates.
Extend fabricstat unit tests and mock STATE_DB/COUNTERS_DB fixtures for the new fields and nonzero filtering.
sonic-mgmt PR: sonic-net/sonic-mgmt#25637
How I did it
How to verify it
Added unit tests
Also modifying existing sonic-mgmt tests.
Previous command output (if the output of a command-line utility has changed)
New command output (if the output of a command-line utility has changed)