Add fabric port isolate reason isolated link count in the STATE_DB for fabric ports - #4704
Conversation
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@vmittal-msft @ysmanman @kenneth-arista , Please review. |
|
/azpw run |
|
Retrying failed(or canceled) jobs... |
|
Retrying failed(or canceled) stages in build 1147768: ✅Stage Test:
|
|
/azpw run |
|
Retrying failed(or canceled) jobs... |
|
Retrying failed(or canceled) stages in build 1147768: ✅Stage Test:
|
|
/azpw run |
|
Retrying failed(or canceled) jobs... |
Signed-off-by: saksarav <sakthivadivu.saravanaraj@nokia.com>
64adc46 to
7412a8c
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@kenneth-arista and @vmittal-msft to sign-off |
Signed-off-by: saksarav <sakthivadivu.saravanaraj@nokia.com>
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
@kenneth-arista : please sign off and @vmittal-msft : also please review/sign-off. |
kenneth-arista
left a comment
There was a problem hiding this comment.
Looks good to me. @jfeng-arista also took a look
vmittal-msft
left a comment
There was a problem hiding this comment.
Thanks for the enhancement — the intent (operator-friendly isolation debugging) is well served and the change is nicely scoped. All CI (incl. vstest) is green. A few points below; only the first is worth resolving before merge, the rest are minor.
What looks good
computeFabricIsolateReason()reuses the same threshold vars (isolationPollsCfg/fecIsolatedPolls) that drive the actual isolate trigger, so the reported reason stays consistent with the isolation decision within a poll.- The removed inner
for (cnt ...)loops inupdateFabricDebugCounters()/updateFabricRate()were genuinely dead —cntwas never used and the body only matched field names. Good simplification, and it de-spams the per-field log too. - Race-tolerant comparators for the ephemeral
link_event_counters_reset/admin_unisolatereasons are a nice touch. - Removing the unused
DVSDatabaseimport and the duplicateconfigKeyreassignment are both safe (configKeyis still defined earlier in the same scope).
Please align before merge — reason vocabulary vs sonic-utilities #4641
The companion CLI renders show fabric isolation by passing the STATE_DB string straight through, but its fixtures/expected output use reason strings this PR never emits:
- swss writes
config→ utilities examples showuser_config - utilities references
symbol_errors→ swss has no symbol-error isolation path (the monitor only tracks CRC + FEC-uncorrectable)
So the operator-facing examples won't match what's actually written. The set swss can produce is:
none, config, permanent, crc_errors, fec_uncorrectable, "crc_errors & fec_uncorrectable", auto, unknown (+ ephemeral link_event_counters_reset, admin_unisolate). Could we make that the single source of truth and align both PRs on one vocabulary?
Minor / non-blocking
&and spaces in a DB value —"crc_errors & fec_uncorrectable"works, but a machine-parseable form (e.g.crc_errors,fec_uncorrectable) is more conventional for a multi-valued STATE_DB field and easier for the CLI to split/render.configvspermanentprecedence — a port that is both config-isolated and permanently isolated reportsconfig(checked first). Is that intended?permanentis arguably the more important state to surface.- Redundant input — by the time the reason is computed,
permIsolateis already forced to 1 wheneverorigPermIsolated == 1, so thepermIsolate || origPermIsolatedcheck's second operand never changes the outcome. Similarly the finalunknownis unreachable (isolated == 1always implies cfg/auto/perm). Both are harmless/defensive — just flagging they're effectively dead. - Untested branches —
auto(auto-isolated while current counters are below the trigger, i.e. the recovery window) andunknownaren't covered by the new tests. Worth a short comment documenting when a port showsautovscrc_errors, since operators may be surprised to see it flip. - Style —
computeFabricIsolateReasontouches no member state; making itstatic/a free function would signal it has no side effects. - Lingering reason when monitoring is off —
admin_unisolate/link_event_counters_resetonly refresh on the next debug-counter poll, so withmonState=disablethey persist. Cosmetic.
Signed-off-by: saksarav <sakthivadivu.saravanaraj@nokia.com>
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
@vmittal-msft , i have addressed your comments. Please take a look at it and sign off. |
|
The label |
thank you for tagging me. about the review. the production code and test code look good to me as well. |
vmittal-msft
left a comment
There was a problem hiding this comment.
Re-reviewed db3ab60c ("Address the review comments") — thanks for the thorough turnaround. The one item I'd asked to resolve before merge is fully addressed, so this is good to go from my side.
Blocker resolved — reason vocabulary now aligned with sonic-utilities #4641
swss keeps config (and has no symbol-error isolation path), and the companion CLI PR was updated in lockstep: user_config and symbol_errors are gone, and both sides now share one vocabulary — none, config, permanent, crc_errors, fec_uncorrectable, crc_errors,fec_uncorrectable, auto, unknown plus the ephemeral link_event_counters_reset / admin_unisolate. The fabricstat header comment enumerates exactly this set and the mock fixtures match. 👍
Also addressed
- Multi-value reason is now machine-parseable
crc_errors,fec_uncorrectable(comma, no spaces). permanentis now evaluated beforeconfig, with a dedicated test.- The new comment documents
autovscrc_errorsand the ephemeral-persistence behavior whenmonState=disable; FEC-only and combined CRC+FEC cases now have tests.
Non-blocking nits (fine to leave)
computeFabricIsolateReason()touches no member state — could bestatic/free to signal that, but harmless as-is.- The final
unknownbranch is effectively unreachable (wheneverisolated==1, one of cfg/auto/perm is set); the addedSWSS_LOG_WARNis a fine defensive guard. ISOLATE_REASONisn't seeded inupdateFabricPortState(), so a freshly discovered port has no reason field until the first debug-counter poll. The CLI tolerates the missing field, so it's cosmetic — defaulting tononeat creation would round it out.
All 18 checks are green on this commit (incl. Test vstest / TestAsan vstest), so the new reason tests pass. LGTM.
|
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
|
|
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
|
|
|
PR for msft-201601 |
What I did
Added ISOLATE_REASON on each FABRIC_PORT_TABLE entry in STATE_DB so operators and CLI can see why a link is isolated (config, CRC/FEC auto-isolate, permanent, link-down counter reset, admin unisolate, etc.).
Added computeFabricIsolateReason() to derive the reason from isolation
state and error poll counters during fabric debug monitoring.
Added FABRIC_CAPACITY_DATA.isolated_links alongside existing
capacity fields for chassis-wide isolated link telemetry.
Also simplify (unnecessary) fabric counter parsing loops and fix minor log/comment
typos in FabricPortsOrch.
The corresponding sonic-utilities PR sonic-net/sonic-utilities#4641
sonic-mgmt PR : sonic-net/sonic-mgmt#25637
Why I did it
To make the debugging easier in the field when the crc/fec errors are seen and ports gets isolated
How I verified it
Induced the crc and fec errors in COUNTERS_DB and verified the fields are updated correctly in STATE_DB. Also updated the sonic-mgmt test and verified the tests are passing.
Details if related