[LEDD] Subscribe to PORT_TABLE in APPL_DB for port oper status#798
[LEDD] Subscribe to PORT_TABLE in APPL_DB for port oper status#798judyjoseph merged 2 commits intosonic-net:masterfrom
Conversation
Signed-off-by: Prince George <prgeor@microsoft.com>
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull request overview
Updates sonic-ledd to track port operational status from PORT_TABLE in APPL_DB (instead of STATE_DB) to reduce unnecessary event processing/log flooding caused by STATE_DB updates.
Changes:
- Switch PORT table subscription from
STATE_DBtoAPPL_DB(APP_PORT_TABLE_NAME). - Update the expected oper-status field name from
netdev_oper_statustooper_status.
Comments suppressed due to low confidence (1)
sonic-ledd/scripts/ledd:212
PORT_OP_FIELDwas changed tooper_status, butfindFrontPanelPorts()still reads initial port state fromSTATE_DBSTATE_PORT_TABLE_NAME(which historically usesnetdev_oper_status, as reflected by existing unit tests). This will likely default ports todownat startup and initialize LEDs incorrectly because the subscriber table only delivers changes, not the current snapshot. Consider fetching the initial oper status fromAPPL_DBAPP_PORT_TABLE_NAME(to match the new subscription) or handling different field names per DB.
for namespace in namespaces:
port_cfg_table = self.portObserver.getDatabaseTable("CONFIG_DB", swsscommon.CFG_PORT_TABLE_NAME, namespace)
port_st_table = self.portObserver.getDatabaseTable("STATE_DB", swsscommon.STATE_PORT_TABLE_NAME, namespace)
for key in port_cfg_table.getKeys():
| def __init__(self): | ||
| # Subscribe to PORT table notifications in the STATE DB | ||
| self.tables = {} | ||
| self.sel = swsscommon.Select() | ||
|
|
||
| def subscribePortTable(self, namespaces): | ||
| for namespace in namespaces: | ||
| self.subscribeDbTable("STATE_DB", swsscommon.STATE_PORT_TABLE_NAME, namespace) | ||
| self.subscribeDbTable("APPL_DB", swsscommon.APP_PORT_TABLE_NAME, namespace) |
There was a problem hiding this comment.
The comment in PortStateObserver.__init__ says it subscribes to PORT table notifications in STATE_DB, but subscribePortTable() now subscribes to APPL_DB. Updating this comment will avoid confusion for future maintainers/debugging.
There was a problem hiding this comment.
@copilot apply changes based on this feedback
| class Port(): | ||
| PORT_UP = "up" # All subports are up | ||
| PORT_DOWN = "down" # All subports are down | ||
| PORT_OP_FIELD = "netdev_oper_status" | ||
| PORT_OP_FIELD = "oper_status" |
There was a problem hiding this comment.
This change switches the subscribed table/field name (STATE_DB + netdev_oper_status -> APPL_DB + oper_status). The existing sonic-ledd/tests/test_ledd.py fixtures assert netdev_oper_status and mock STATE_DB behavior, so unit tests will fail and won’t validate the new behavior. Please update/add tests to cover APPL_DB PORT_TABLE events and the oper_status field.
There was a problem hiding this comment.
@copilot apply changes based on this feedback
|
can you update the tests too @prgeor |
Signed-off-by: Prince George <prgeor@microsoft.com>
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
@judyjoseph addressed |
Description
Subscribe to PORT_TABLE in APPL_DB for port oper status
Motivation and Context
Continuous log flooding from ledd daemon even if there is change in port operational status
This is because previously the ledd daemon had subscribed to PORT_TABLE of STATE_DB which can get updated by Orchagent which can unnecessarily trigger processing of port oper status by ledd
How Has This Been Tested?
Validated this in lab switch where the logs were getting flooded
Additional Information (Optional)