Skip to content

[meta] Add opt-in saithrift RPCs for draining SAI notifications - #2335

Open
nicholasching wants to merge 2 commits into
opencomputeproject:masterfrom
nicholasching:sai_vpp_ut_notification_bridge
Open

[meta] Add opt-in saithrift RPCs for draining SAI notifications#2335
nicholasching wants to merge 2 commits into
opencomputeproject:masterfrom
nicholasching:sai_vpp_ut_notification_bridge

Conversation

@nicholasching

@nicholasching nicholasching commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

Context / motivation
Part of the SAIVPP unit-test framework. SAI delivers port state and BFD session state asynchronously through function pointers written into SAI_SWITCH_ATTR_PORT_STATE_CHANGE_NOTIFY and SAI_SWITCH_ATTR_BFD_SESSION_STATE_CHANGE_NOTIFY. A Thrift client cannot supply a function pointer, so there is currently no way for any saithrift-based test suite to observe that a notification was delivered. This blocks testing the notification path on any target, not just VPP.

What this change does

  • meta/templates/sai_thrift_utils.tt, meta/templates/sai.thrift.tt. Declare sai_thrift_enable_notifications() and sai_thrift_drain_notifications(), plus a sai_thrift_notification_event_t record carrying notification type, object id and state.
  • meta/templates/sai_rpc_server_functions.tt. Add the two method names to the sai_utils_functions regex so the generator emits stub bodies instead of trying to bind them to SAI metadata.
  • meta/sai_rpc_frontend.cpp. Implement both methods in sai_rpcHandlerFrontend, following the existing sai_thrift_object_type_get_availability pattern. The enable RPC installs server-owned callbacks and is idempotent; the drain RPC is non-blocking and atomically empties a mutex-protected bounded queue.
  • Post-create registration. The callbacks are installed on the already-created switch via set_switch_attribute, not at create time. SwitchStateBase::set_switch_default_attributes() writes NULL into both attributes during initialize_default_objects, so a pointer supplied at create time is silently discarded. Both attributes are CREATE_AND_SET and the senders re-read them when an event fires, so a post-create set is correct, simpler, and additionally suppresses the forced port-UP notifications raised during host-interface creation.

Scope / risk

  • Default-preserving: behaviour is unchanged unless a test explicitly calls the enable RPC. No existing RPC is modified.
  • No function pointer crosses Thrift. The callbacks live entirely inside saiserver; only notification type, object id and state are serialized.
  • Bounded: the queue holds 256 events and drops the oldest on overflow, so a client that never drains cannot grow memory without limit.
  • RAII containers and standard synchronization only. No new dependencies, no unsafe C memory or string calls, no credentials or certificates.
  • No SAI headers, metadata definitions, or backend behaviour change.
  • Validated behavior: gensairpc.pl completes and the generated service contains both methods; saithriftv2 and saiserver build and link; enable followed by drain from a throwaway client returns an empty list without crashing. End-to-end validation is in the notification test PR.

Dependencies
None. This is a prerequisite for the notification test cases, #2336.

Signed-off-by: Nicholas Ching <nicholaslching@gmail.com>
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
There may be pipelines that require an authorized user to comment /azp run to run.

Comment thread meta/templates/sai.thrift.tt Outdated
}

struct sai_thrift_notification_event_t {
1: i32 notification_type;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we define enum type for notification type?

@nicholasching

Copy link
Copy Markdown
Contributor Author

Hi @kcudnik @tjchadaga, when you have a chance, could you please invoke azure pipelines to run checks. Thanks!

Signed-off-by: Nicholas Ching <nicholaslching@gmail.com>

@yue-fred-gao yue-fred-gao left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

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.

2 participants