[Nexthop] Remove stale ports from portDisplayMap_ on switch state update - #1504
Open
raghavv-nexthop wants to merge 1 commit into
Open
[Nexthop] Remove stale ports from portDisplayMap_ on switch state update#1504raghavv-nexthop wants to merge 1 commit into
raghavv-nexthop wants to merge 1 commit into
Conversation
Signed-off-by: raghavv-nexthop <raghavv@nexthop.ai>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pre-submission checklist
pip install -r requirements-dev.txt && pre-commit installpre-commit runSummary
Overview
Remove ports from
portDisplayMap_when they are no longer present in the latest full agent switch-state update. This fixes an LED-state mismatch after breakout reconfiguration, where a previously active port remains in the LED manager’s software state and is incorrectly included when calculating the LED state of a surviving port that shares physical LEDs.Problem
On Wedge800, the
LedServiceTest.testTcvrLostest fails LED color equality checks and produces error messages that look like the following:Cause
LedManager::updateLedStatus()receives a complete view of the active agent ports through the FSDB switch-state subscriber. It updated or inserted entries inportDisplayMap_, but did not remove entries for ports that disappeared from the new switch state.After a breakout change, this left a stale port in
portDisplayMap_.BspLedManager::getCommonLedSwPorts()considers all switch ports supported by the transceiver and identifies ports that share LED IDs. As a result, the stale port continued to participate in the shared-LED aggregation for its breakout sibling.On Wedge800, this can produce the following mismatch:
BLUE+SLOW.YELLOW.The cached LED state and the hardware LED state then disagree, causing
LedServiceTest.testTcvrLosto fail.The stale-entry bug is generic, but Wedge800 makes the resulting inconsistency observable because its relevant transceiver mapping has four LEDs across eight lanes—two lanes per LED.
A 400G breakout port therefore spans multiple LEDs and uses the per-LED LOS handling path. On platforms where a 400G port maps to a single LED, the aggregate state is written directly to that LED, so the stale-port condition does not produce the same cache-versus-hardware mismatch.
Solution
After processing the latest switch-state entries, erase every
portDisplayMap_entry whose port ID is absent from the new switch state.The existing LED update flow then recalculates the state using only active ports. For the failing breakout case, the aggregate calculation correctly sees one active port with LOS and returns
YELLOW+SLOW, matching the LED state written by the existing hardware path.Follow-up Defects
While the change in this PR alone results in the
LedServiceTest.testTcvrLostest passing, there are two other defects that debugging this failure exposed:1. Per-LED LOS handling treats missing lane data as LOS
setLedBasedOnLOS()initializesallLanesLostotrueand only changes it when a lane explicitly reportsrxLos=false. If an LED lane is missing fromportLosMap_, the function logs the missing entry but leavesallLanesLosset totrue, causing that LED to be driven yellow instead of treating the data as unknown or incomplete.2. Per-lane LOS changes may not repaint the LEDs
triggerLedUpdate()callssetLedState()only when the aggregate LedState changes. On platforms where one port controls multiple LEDs, the aggregate state can remainBLUE+SLOWwhile the per-LED LOS pattern changes—for example, fromYELLOW/YELLOWtoYELLOW/BLUE.In that case, the cached aggregate state is unchanged, so
setLedState()is skipped and the hardware LEDs retain the previous per-LED pattern. A complete fix should trigger a hardware update when the per-lane LED representation changes, not only when the aggregate color or blink state changes.Test Plan
To verify the change, I ran the full LED service test suite on two platforms (Wedge800bact and Montblanc) using this command:
Result on Wedge800bact:
Result on Montblanc: