Skip to content

saiserver: default switch_id on neighbor/route entries when omitted #2300

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
nicholasching wants to merge 3 commits into
base: master
Choose a base branch
from
+46 −1
Open

saiserver: default switch_id on neighbor/route entries when omitted #2300

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3e94e74
fix: switch_id on route + neighbor entries
nicholasching Jun 15, 2026
51d2647
fix: -5 error, switch-id-NULL on neighbor/route create test
nicholasching Jun 19, 2026
1c9832a
fix: fall back only to RPC-scoped switch_id
nicholasching Jun 26, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 51 additions & 1 deletion meta/templates/sai_rpc_server_helper_functions.tt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,8 @@
}
[%- END -%]
[%- FOREACH struct IN apis.$api.structs %]

[%- NEXT IF struct.short_name == 'neighbor_entry' %]
[%- NEXT IF struct.short_name == 'route_entry' %]

[%- PROCESS struct_helper_function_header %] {

Expand Down Expand Up @@ -135,8 +136,57 @@ void sai_thrift_parse_[% struct.short_name %](const [% struct.thrift_name %] &th
[%- ######################################################################## -%]

[%- BLOCK special_helper_functions -%]
extern sai_object_id_t gSwitchId;

@yue-fred-gao yue-fred-gao Jun 26, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

In thrift v2, gSwitchId is never set, right? We probably can remove this line.

@nicholasching nicholasching Jun 26, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Yes, that is correct; gSwitchId is never set in v2. I will remove the line.

extern sai_object_id_t switch_id;

void sai_thrift_parse_buffer(const std::string &thrift_buffer,
void *buffer) {
/* not supported yet */
}

void sai_thrift_parse_neighbor_entry(const sai_thrift_neighbor_entry_t &thrift_neighbor_entry,
sai_neighbor_entry_t *neighbor_entry)
{
if (neighbor_entry == NULL) {
return;
}

// OCP sai_test often omits switch_id on neighbor_entry; fall back to the
// RPC-global switch_id (set by sai_thrift_create_switch) then gSwitchId.
if (thrift_neighbor_entry.switch_id != 0) {

@yue-fred-gao yue-fred-gao Jun 26, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

does sai_thrift_neighbor_entry_t have switch_id?

@nicholasching nicholasching Jun 26, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Yes, thesai_thrift_neighbor_entry_t in thriftv2 is generated from the SAI header sai_neighbor_entry_t, of which switch_id is a member.

SAI/inc/saineighbor.h

Lines 185 to 192 in c0203c0

typedef struct _sai_neighbor_entry_t
{
/**
* @brief Switch ID
*
* @objects SAI_OBJECT_TYPE_SWITCH
*/
sai_object_id_t switch_id;

@yue-fred-gao yue-fred-gao Jun 26, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I see. The previous change to sai_thrift_neighbor_entry_t is actually for v1. v2 always generate sai thrift from SAI headers.

neighbor_entry->switch_id = (sai_object_id_t)thrift_neighbor_entry.switch_id;
} else if (switch_id != SAI_NULL_OBJECT_ID) {
neighbor_entry->switch_id = switch_id;
} else if (gSwitchId != SAI_NULL_OBJECT_ID) {
neighbor_entry->switch_id = gSwitchId;

@yue-fred-gao yue-fred-gao Jun 26, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

same as above, we probably can remove this line.

} else {
neighbor_entry->switch_id = SAI_NULL_OBJECT_ID;
}

neighbor_entry->rif_id = (sai_object_id_t)thrift_neighbor_entry.rif_id;
sai_thrift_ip_address_t_parse(thrift_neighbor_entry.ip_address,
&neighbor_entry->ip_address);
}

void sai_thrift_parse_route_entry(const sai_thrift_route_entry_t &thrift_route_entry,
sai_route_entry_t *route_entry)
{
if (route_entry == NULL) {
return;
}

if (thrift_route_entry.switch_id != 0) {
route_entry->switch_id = (sai_object_id_t)thrift_route_entry.switch_id;
} else if (switch_id != SAI_NULL_OBJECT_ID) {
route_entry->switch_id = switch_id;
} else if (gSwitchId != SAI_NULL_OBJECT_ID) {
route_entry->switch_id = gSwitchId;
} else {
route_entry->switch_id = SAI_NULL_OBJECT_ID;
}

route_entry->vr_id = (sai_object_id_t)thrift_route_entry.vr_id;
sai_thrift_ip_prefix_t_parse(thrift_route_entry.destination,
&route_entry->destination);
}
[% END -%]
32 changes: 18 additions & 14 deletions test/sai_test/config/route_configer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -290,15 +290,17 @@ def create_default_v4_v6_route_entry(self):
addr=sai_thrift_ip_addr_t(
ip6=DEFAULT_IP_V6_PREFIX),
mask=sai_thrift_ip_addr_t(ip6=DEFAULT_IP_V6_PREFIX))
self.test_obj.dut.default_ipv6_route_entry = sai_thrift_route_entry_t(vr_id=self.test_obj.dut.default_vrf,
self.test_obj.dut.default_ipv6_route_entry = sai_thrift_route_entry_t(switch_id=self.test_obj.dut.switch_id,
vr_id=self.test_obj.dut.default_vrf,
destination=v6_default)
status = sai_thrift_create_route_entry(
self.client,
route_entry=self.test_obj.dut.default_ipv6_route_entry,
packet_action=SAI_PACKET_ACTION_DROP)
self.test_obj.assertEqual(status, SAI_STATUS_SUCCESS)

self.test_obj.dut.default_ipv4_route_entry = sai_thrift_route_entry_t(vr_id=self.test_obj.dut.default_vrf,
self.test_obj.dut.default_ipv4_route_entry = sai_thrift_route_entry_t(switch_id=self.test_obj.dut.switch_id,
vr_id=self.test_obj.dut.default_vrf,
destination=sai_ipprefix(DEFAULT_IP_V4_PREFIX))
status = sai_thrift_create_route_entry(
self.client,
Expand All @@ -323,22 +325,22 @@ def create_route_by_rif(
vr_id = self.choice_virtual_route(virtual_router)
if dest_device.ip_prefix:
net_routev4 = sai_thrift_route_entry_t(
vr_id=vr_id, destination=sai_ipprefix(dest_device.ipv4+'/'+dest_device.ip_prefix))
switch_id=self.test_obj.dut.switch_id, vr_id=vr_id, destination=sai_ipprefix(dest_device.ipv4+'/'+dest_device.ip_prefix))
else:
# destination cannot use sai_ipaddress
net_routev4 = sai_thrift_route_entry_t(
vr_id=vr_id, destination=sai_ipprefix(dest_device.ipv4+'/32'))
switch_id=self.test_obj.dut.switch_id, vr_id=vr_id, destination=sai_ipprefix(dest_device.ipv4+'/32'))
status = sai_thrift_create_route_entry(
self.client, net_routev4, next_hop_id=rif)
self.test_obj.assertEqual(status, SAI_STATUS_SUCCESS)

if dest_device.ip_prefix_v6:
net_routev6 = sai_thrift_route_entry_t(
vr_id=vr_id, destination=sai_ipprefix(dest_device.ipv6+'/'+dest_device.ip_prefix_v6))
switch_id=self.test_obj.dut.switch_id, vr_id=vr_id, destination=sai_ipprefix(dest_device.ipv6+'/'+dest_device.ip_prefix_v6))
else:
# destination cannot use sai_ipaddress
net_routev6 = sai_thrift_route_entry_t(
vr_id=vr_id, destination=sai_ipprefix(dest_device.ipv6+'/128'))
switch_id=self.test_obj.dut.switch_id, vr_id=vr_id, destination=sai_ipprefix(dest_device.ipv6+'/128'))
status = sai_thrift_create_route_entry(
self.client, net_routev6, next_hop_id=rif)
self.test_obj.assertEqual(status, SAI_STATUS_SUCCESS)
Expand Down Expand Up @@ -366,22 +368,22 @@ def create_route_by_nexthop(
vr_id = self.choice_virtual_route(virtual_router)
if dest_device.ip_prefix:
net_routev4 = sai_thrift_route_entry_t(
vr_id=vr_id, destination=sai_ipprefix(dest_device.ipv4+'/'+dest_device.ip_prefix))
switch_id=self.test_obj.dut.switch_id, vr_id=vr_id, destination=sai_ipprefix(dest_device.ipv4+'/'+dest_device.ip_prefix))
else:
# destination cannot use sai_ipaddress
net_routev4 = sai_thrift_route_entry_t(
vr_id=vr_id, destination=sai_ipprefix(dest_device.ipv4+'/32'))
switch_id=self.test_obj.dut.switch_id, vr_id=vr_id, destination=sai_ipprefix(dest_device.ipv4+'/32'))
status = sai_thrift_create_route_entry(
self.client, net_routev4, next_hop_id=nexthopv4.oid)
self.test_obj.assertEqual(status, SAI_STATUS_SUCCESS)

if dest_device.ip_prefix_v6:
net_routev6 = sai_thrift_route_entry_t(
vr_id=vr_id, destination=sai_ipprefix(dest_device.ipv6+'/'+dest_device.ip_prefix_v6))
switch_id=self.test_obj.dut.switch_id, vr_id=vr_id, destination=sai_ipprefix(dest_device.ipv6+'/'+dest_device.ip_prefix_v6))
else:
# destination cannot use sai_ipaddress
net_routev6 = sai_thrift_route_entry_t(
vr_id=vr_id, destination=sai_ipprefix(dest_device.ipv6+'/128'))
switch_id=self.test_obj.dut.switch_id, vr_id=vr_id, destination=sai_ipprefix(dest_device.ipv6+'/128'))
status = sai_thrift_create_route_entry(
self.client, net_routev6, next_hop_id=nexthopv6.oid)
self.test_obj.assertEqual(status, SAI_STATUS_SUCCESS)
Expand All @@ -406,22 +408,22 @@ def create_route_by_nexthop_group(
vr_id = self.choice_virtual_route(virtual_router)
if dest_device.ip_prefix:
net_routev4 = sai_thrift_route_entry_t(
vr_id=vr_id, destination=sai_ipprefix(dest_device.ipv4+'/'+dest_device.ip_prefix))
switch_id=self.test_obj.dut.switch_id, vr_id=vr_id, destination=sai_ipprefix(dest_device.ipv4+'/'+dest_device.ip_prefix))
else:
# destination cannot use sai_ipaddress
net_routev4 = sai_thrift_route_entry_t(
vr_id=vr_id, destination=sai_ipprefix(dest_device.ipv4+'/32'))
switch_id=self.test_obj.dut.switch_id, vr_id=vr_id, destination=sai_ipprefix(dest_device.ipv4+'/32'))
status = sai_thrift_create_route_entry(
self.client, net_routev4, next_hop_id=nexthop_groupv4.nhp_grp_id)
self.test_obj.assertEqual(status, SAI_STATUS_SUCCESS)

if dest_device.ip_prefix_v6:
net_routev6 = sai_thrift_route_entry_t(
vr_id=vr_id, destination=sai_ipprefix(dest_device.ipv6+'/'+dest_device.ip_prefix_v6))
switch_id=self.test_obj.dut.switch_id, vr_id=vr_id, destination=sai_ipprefix(dest_device.ipv6+'/'+dest_device.ip_prefix_v6))
else:
# destination cannot use sai_ipaddress
net_routev6 = sai_thrift_route_entry_t(
vr_id=vr_id, destination=sai_ipprefix(dest_device.ipv6+'/128'))
switch_id=self.test_obj.dut.switch_id, vr_id=vr_id, destination=sai_ipprefix(dest_device.ipv6+'/128'))
status = sai_thrift_create_route_entry(
self.client, net_routev6, next_hop_id=nexthop_groupv6.nhp_grp_id)
self.test_obj.assertEqual(status, SAI_STATUS_SUCCESS)
Expand All @@ -447,6 +449,7 @@ def create_neighbor_by_rif(self, nexthop_device: Device, rif, no_host=True):
"""
if nexthop_device.ipv4:
nbr_entry_v4 = sai_thrift_neighbor_entry_t(
switch_id=self.test_obj.dut.switch_id,
rif_id=rif,
ip_address=sai_ipaddress(nexthop_device.ipv4))
status = sai_thrift_create_neighbor_entry(
Expand All @@ -460,6 +463,7 @@ def create_neighbor_by_rif(self, nexthop_device: Device, rif, no_host=True):

if nexthop_device.ipv6:
nbr_entry_v6 = sai_thrift_neighbor_entry_t(
switch_id=self.test_obj.dut.switch_id,
rif_id=rif,
ip_address=sai_ipaddress(nexthop_device.ipv6))
status = sai_thrift_create_neighbor_entry(
Expand Down
5 changes: 3 additions & 2 deletions test/saithrift/src/switch_sai.thrift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,8 +198,9 @@ struct sai_thrift_route_entry_t {
}

struct sai_thrift_neighbor_entry_t {
1: sai_thrift_object_id_t rif_id;
2: sai_thrift_ip_address_t ip_address;
1: sai_thrift_object_id_t switch_id;

@yue-fred-gao yue-fred-gao Jun 26, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think this is not needed. We should keep the original design. let sai rpc server assign switch_id use the stored RPC scoped switch_id variable. Then the changes in route_configer.py are also not needed.

@nicholasching nicholasching Jun 26, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I agree, since issue was fixed by RPC-scoped switch_id fallback, no need for tests to pass in switch_id

2: sai_thrift_object_id_t rif_id;
3: sai_thrift_ip_address_t ip_address;
}

struct sai_thrift_attribute_list_t {
Expand Down
8 changes: 7 additions & 1 deletion test/saithrift/src/switch_sai_rpc_server.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,7 +241,13 @@ class switch_sai_rpcHandler : virtual public switch_sai_rpcIf {
}

void sai_thrift_parse_neighbor_entry(const sai_thrift_neighbor_entry_t &thrift_neighbor_entry, sai_neighbor_entry_t *neighbor_entry) {
neighbor_entry->switch_id = gSwitchId;

@yue-fred-gao yue-fred-gao Jun 24, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I don't quite understand about this change. From my reading of the code, saiserver stores switch_id in gSwitchId (

SAI/test/saithrift/src/saiserver.cpp

Line 438 in c0203c0

status = sai_switch_api->create_switch(&gSwitchId, attrSz, attr);
). Subsequent calls can use the gSwitchId if it is not in the RPC request, such as neighbor entries. Is there a case that gSwitchId is invalid?

@nicholasching nicholasching Jun 25, 2026 •
edited
Loading

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Yes, that is correct; for v1, gSwitchId is always valid so the change is not needed here. I made it to maintain feature parity w/saithriftv2.

Edits to saithriftv2 were needed because gSwitchId in saiserver.cpp is only declared and not assigned on the RPC path.

SAI/test/saithriftv2/src/saiserver.cpp

Line 43 in 9e90412

sai_object_id_t gSwitchId; ///< SAI switch global object ID.

Instead, sai_thrift_create_switch() stores the OID in the RPC-global switch_id.

So when tests created a neighbor or route over RPC, and did not send switch_id, saithriftv2 would pass switch_id=0 to SAI, resulting in switch id is NULL error (-5). The change to saithriftv2 adds a fallback for when switch_id is not passed in (0): first to the RPC path switch_id set by create_switch (which resolves the -5), then to gSwitchId as backup.

Should I keep this change for feature parity or remove to avoid touching v1?

@yue-fred-gao yue-fred-gao Jun 26, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I see. Didn't know there are 2 versions of saithrift. I think we can undo the change. Leave v1 as is

@nicholasching nicholasching Jun 26, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Will do!

// Python saithrift sends switch_id as field 1 (optional). Always fall back to
// gSwitchId when the client omits it (OCP sai_test often only passes rif_id).
if (thrift_neighbor_entry.switch_id != 0) {
neighbor_entry->switch_id = (sai_object_id_t) thrift_neighbor_entry.switch_id;
} else {
neighbor_entry->switch_id = gSwitchId;
}
neighbor_entry->rif_id = (sai_object_id_t) thrift_neighbor_entry.rif_id;
sai_thrift_parse_ip_address(thrift_neighbor_entry.ip_address, &neighbor_entry->ip_address);
}
Expand Down
Loading