Skip to content

[Link Event Damping] Add RedisInterface plumbing for damping config#1935

Closed
DendroLabs wants to merge 2 commits into
sonic-net:masterfrom
DendroLabs:link-event-damping/redis-interface
Closed

[Link Event Damping] Add RedisInterface plumbing for damping config#1935
DendroLabs wants to merge 2 commits into
sonic-net:masterfrom
DendroLabs:link-event-damping/redis-interface

Conversation

@DendroLabs

Copy link
Copy Markdown
Contributor

Summary

Supersedes #1331, originally authored by @Ashish1805. The original PR has been inactive for ~2 years and has merge conflicts against current master.

This PR adds the Redis communication layer for link event damping configuration:

lib/ (orchagent -> Redis):

  • sairediscommon.h: New command strings link_event_damping_config_set / link_event_damping_config_response
  • RedisRemoteSaiInterface: New isRedisPortAttribute() (port analog to isRedisAttribute()), setRedisPortExtensionAttribute() dispatch, and setLinkEventDampingConfig() which is always synchronous (per @kcudnik review)
  • ClientSai::set(): Blocks port extension attrs in CLIENT mode
  • Sai::set(): Routes port extension attrs directly, bypassing metadata validation

syncd/ (Redis -> syncd handler):

  • Syncd: Dispatches link_event_damping_config_set to processLinkEventDampingConfigSet() stub
  • sendLinkEventDampingConfigResponse() always sends a response (no m_enableSyncMode guard), since the lib always waits

Fixes from @kcudnik's review of #1331

  1. Always synchronous: setLinkEventDampingConfig unconditionally calls m_communicationChannel->wait(). The original code skipped the wait in async mode, which @kcudnik said was wrong: "make this api always synchronous, async mode is just for create/remove/set operations all other apis should be synchronous."

  2. Simplified isRedisPortAttribute: Single boolean expression using reversed condition, per @kcudnik: "you can turn this to 1 single line by reversing condition."

  3. Syncd always sends response: sendLinkEventDampingConfigResponse omits the m_enableSyncMode guard, since the lib side always waits unconditionally.

Relation to other PRs

This is part of the Link Event Damping feature:

Test plan

  • 3 new unit tests in unittest/lib/TestClientServerSai.cpp (client mode rejection, null pointer rejection, invalid attribute rejection)
  • 2 new integration tests in syncd/tests/TestSyncdLinkEventDamping.cpp (full lib->Redis->syncd round trip for algorithm and AIED config, asserting NOT_IMPLEMENTED response from stub)
  • Existing tests unaffected -- normal port set operations (speed, MTU, admin state) have attr IDs far below SAI_PORT_ATTR_CUSTOM_RANGE_START (0x10000000)

Credits: Original implementation by @Ashish1805 (PR #1331).

Supersedes sonic-net#1331 by @Ashish1805. Adds the Redis communication layer
to carry link event damping configuration from orchagent to syncd.

Addresses @kcudnik review feedback from sonic-net#1331:
- setLinkEventDampingConfig always synchronous (never checks m_syncMode)
- sendLinkEventDampingConfigResponse always sends (no m_enableSyncMode guard)
- isRedisPortAttribute simplified to single boolean expression

lib/ changes:
- sairediscommon.h: DAMPING_CONFIG_SET / DAMPING_CONFIG_RESPONSE commands
- RedisRemoteSaiInterface: isRedisPortAttribute(), setRedisPortExtensionAttribute(),
  setLinkEventDampingConfig() with unconditional wait(), waitForLinkEventDampingConfigResponse()
- ClientSai::set(): block port extension attrs in CLIENT mode
- Sai::set(): route port extension attrs directly, bypass metadata validation

syncd/ changes:
- Syncd: dispatch DAMPING_CONFIG_SET in processSingleEvent()
- processLinkEventDampingConfigSet(): stub returning NOT_IMPLEMENTED
- sendLinkEventDampingConfigResponse(): always sends response

syncd handler is a stub -- actual algorithm in sonic-net#1334 supersede.

Signed-off-by: DendroLabs <info@dendrolabs.com>
@mssonicbld

Copy link
Copy Markdown
Collaborator

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@DendroLabs

Copy link
Copy Markdown
Contributor Author

@kcudnik @mikeberesford — this is the second PR in the link event damping series, following #1798 (SelectablesTracker, merged last week). It adds the Redis communication plumbing, superseding #1331.

All three review comments from #1331 are addressed — the key fix is making the damping config API always synchronous, matching the pattern used by flushFdbEntries and other non-QUAD APIs.

Would appreciate your review when you get a chance. The next PR (#1334 supersede, the actual damping algorithm) depends on this one.

Replace development-facing "stub" wording in the
processLinkEventDampingConfigSet NOTICE log with a professional
message suitable for operator syslog.

Signed-off-by: DendroLabs <info@dendrolabs.com>
@mssonicbld

Copy link
Copy Markdown
Collaborator

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@prsunny

prsunny commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

@DendroLabs , please review this PR - #1906. You can close #1935 in favor of 1906

@DendroLabs

Copy link
Copy Markdown
Contributor Author

Closing in favor of #1906 per @prsunny's suggestion. @sivat6's PR covers this scope and more (full Syncd wiring + damping algorithm + stats).

Our #1798 (SelectablesTracker) is already merged and provides the foundation both approaches build on. Happy to help review #1906 and get it across the finish line — we've done extensive testing of the damping algorithm and can offer some findings.

The CLI (sonic-net/sonic-utilities#4367) and YANG model (sonic-net/sonic-buildimage#26231) PRs remain open as they're independent of the sairedis implementation approach.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants