Skip to content
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.

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

[syncd] bulk OID remove requires RID #854

Merged
merged 1 commit into from
Jul 4, 2021
Merged

[syncd] bulk OID remove requires RID #854

merged 1 commit into from
Jul 4, 2021

Conversation

qbdwlr
Copy link
Contributor

@qbdwlr qbdwlr commented Jul 4, 2021

Syncd::processBulkOidRemove() is incorrectly passing objectVids.data() to m_vendorSai->bulkRemove() when it should be passing objectRids.data().
Only RID should be passed to m_vendorSai calls.
Issue can be seen when SAI bulk APIs are enabled in syncd and the following sequence of iproute2 commands are executed:
"ip route add <ip-prefix> nexthop via inet <ip-gateway-1> dev <port-1> nexthop via inet <ip-gateway-2> dev <port-2>"
"ip route del <ip-prefix> nexthop via inet <ip-gateway-1> dev <port-1> nexthop via inet <ip-gateway-2> dev <port-2>"

@kcudnik kcudnik added the Bug label Jul 4, 2021
@kcudnik
Copy link
Collaborator

kcudnik commented Jul 4, 2021

thanks for catching this bug !

this is interesting, since we have test case that is testing this scenario in tests/BCM56850.pl test_bulk_object, there is function in file /tests/BCM56850/bulk_object.rec

2017-06-14.01:55:56.555975|R|SAI_OBJECT_TYPE_NEXT_HOP_GROUP||oid:0x5000000000005||oid:0x5000000000006||oid:0x5000000000007

and that should fail and syncd should crash in this case, interesting that this is not happening, ahh, i think i know what's happening, it's falling back to 1 by 1 instead of executing bulk, here is fallback line: https://github.com/Azure/sonic-sairedis/blob/master/syncd/Syncd.cpp#L1579

Ok, i run the tests, and it actually not executes this, since "-l" param is missing in tests, so actual bulk code is not executed at all, but even then if it's executed, then if it fails it fallbacks, and it succeed (at least on virtual switch :D)

@prsunny
Copy link
Contributor

prsunny commented Sep 16, 2022

@kcudnik , looks like this needs to be cherry-picked to 202012.. can you please confirm?

yenlu-keith pushed a commit to yenlu-keith/sonic-sairedis that referenced this pull request Sep 16, 2022
Syncd::processBulkOidRemove() is incorrectly passing objectVids.data() to m_vendorSai->bulkRemove() when it should be passing objectRids.data(). Only RID should be passed to m_vendorSai calls.
@kcudnik
Copy link
Collaborator

kcudnik commented Sep 18, 2022

yes

pettershao-ragilenetworks pushed a commit to pettershao-ragilenetworks/sonic-sairedis that referenced this pull request Nov 18, 2022
Syncd::processBulkOidRemove() is incorrectly passing objectVids.data() to m_vendorSai->bulkRemove() when it should be passing objectRids.data(). Only RID should be passed to m_vendorSai calls.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants