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] Enable bulk api for neighbor entries #1373

Merged
merged 3 commits into from
May 30, 2024

Conversation

Ndancejic
Copy link
Contributor

SAI 1.11.0 added support for bulk neighbor entries. Adding support for neighbor bulk operations to syncd.

needed for ado: #25072867

@Ndancejic Ndancejic marked this pull request as draft April 18, 2024 22:26
@Ndancejic Ndancejic force-pushed the neighbor_bulk branch 2 times, most recently from ef8a98e to 1e2a921 Compare April 19, 2024 17:37
@kcudnik
Copy link
Collaborator

kcudnik commented Apr 21, 2024

looks good, make code coverage and unnitests, and move out from draft then i will review it

@Ndancejic Ndancejic marked this pull request as ready for review May 14, 2024 19:35
@Ndancejic Ndancejic force-pushed the neighbor_bulk branch 5 times, most recently from 58a904a to 56c7728 Compare May 17, 2024 20:16
@Ndancejic
Copy link
Contributor Author

/azpw run

@mssonicbld
Copy link
Collaborator

/AzurePipelines run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@Ndancejic
Copy link
Contributor Author

@kcudnik Could you take a look? not sure what else I need for coverage, but the tests that are failing are common among other PRs out right now

@kcudnik
Copy link
Collaborator

kcudnik commented May 22, 2024

not passing tests you can ignore, those are flaky, but you can see that you don't have tests that cover your code, and here is exactly showed where: https://dev.azure.com/mssonic/build/_build/results?buildId=552818&view=codecoverage-tab

@kcudnik
Copy link
Collaborator

kcudnik commented May 22, 2024

main::test_bulk_neighbor: Fresh start
Killing syncd
Flushing redis
Starting syncd
Replay BCM56850/bulk_neighbor.rec
terminate called after throwing an instance of 'std::runtime_error'
  what():  :- translate_local_to_redis: failed to translate local RID oid:0x20000000006ee
player BCM56850/bulk_neighbor.rec: exitcode: 134:
FAIL: BCM56850.pl

you can make run tests locally on your dev machine by making "make check" and post when they pass

RID translation is problem that you used RID value of object that was not created previously in script or was not obtained by GET api

@Ndancejic
Copy link
Contributor Author

main::test_bulk_neighbor: Fresh start
Killing syncd
Flushing redis
Starting syncd
Replay BCM56850/bulk_neighbor.rec
terminate called after throwing an instance of 'std::runtime_error'
  what():  :- translate_local_to_redis: failed to translate local RID oid:0x20000000006ee
player BCM56850/bulk_neighbor.rec: exitcode: 134:
FAIL: BCM56850.pl

you can make run tests locally on your dev machine by making "make check" and post when they pass

RID translation is problem that you used RID value of object that was not created previously in script or was not obtained by GET api

I run into the following error when trying to run autogen.sh: configure.ac:32: error: AM_COND_IF: no such condition "CODE_COVERAGE_ENABLED"

@kcudnik
Copy link
Collaborator

kcudnik commented May 23, 2024

you need to install " autoconf-archive" packaged in ubuntu that contains that code coverage sctipt

@Ndancejic
Copy link
Contributor Author

you need to install " autoconf-archive" packaged in ubuntu that contains that code coverage sctipt

I see, I'll get that set up for next time then. I was able to get the coverage passing in the meantime, so this PR is ready for review/merge

@prsunny prsunny requested a review from kcudnik May 24, 2024 00:12
configure.ac Outdated Show resolved Hide resolved
Copy link
Contributor

@prsunny prsunny left a comment

Choose a reason for hiding this comment

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

Kamil, could you please review/signoff?

configure.ac Outdated Show resolved Hide resolved
configure.ac Outdated Show resolved Hide resolved
syncd/Syncd.cpp Outdated Show resolved Hide resolved
syncd/tests.cpp Outdated Show resolved Hide resolved
syncd/tests.cpp Outdated Show resolved Hide resolved
Ndancejic added a commit to Ndancejic/sonic-sairedis that referenced this pull request May 24, 2024
Ndancejic added a commit to Ndancejic/sonic-sairedis that referenced this pull request May 24, 2024
Ndancejic added a commit to Ndancejic/sonic-sairedis that referenced this pull request May 24, 2024
@Ndancejic
Copy link
Contributor Author

@kcudnik Coverage and tests are passing in the last commit, could you please review

@Ndancejic
Copy link
Contributor Author

/azpw run

@mssonicbld
Copy link
Collaborator

/AzurePipelines run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@kcudnik
Copy link
Collaborator

kcudnik commented May 29, 2024

@Ndancejic i made big refactor on master, removed a lot of unnecessary code, please resolve conflicts and check build

Ndancejic added 3 commits May 29, 2024 17:24
SAI 1.11.0 added support for bulk neighbor entries. Adding support for
neighbor bulk operations to syncd.
* added neighbor entry capability to bulk operations in syncd
* added unit tests for neighbor bulk operations
* added code coverage for neighbor bulk operations

Signed-off-by: Nikola Dancejic <[email protected]>
@Ndancejic
Copy link
Contributor Author

Ndancejic commented May 29, 2024

@kcudnik rebased onto the latest, please take a look and approve if no additional comments

@prsunny prsunny merged commit c8cede0 into sonic-net:master May 30, 2024
17 checks passed
@kcudnik
Copy link
Collaborator

kcudnik commented Jun 6, 2024

./unittest/vslib/test_sai_vs_neighbor.cpp
./unittest/lib/test_sai_redis_neighbor.cpp

those files are no longer needed, interface was refactored, and they are not connected into Makefile, can be removed

@nazariig
Copy link
Collaborator

nazariig commented Aug 6, 2024

@Ndancejic can you please create a separate cherry-pick to 202405 branch? We see Dual-ToR test failures due to missing implementation

@dprital
Copy link
Collaborator

dprital commented Aug 13, 2024

@Ndancejic can you please create a separate cherry-pick to 202405 branch? We see Dual-ToR test failures due to missing implementation

@Ndancejic , Do we have PR for 202405 ?

Ndancejic added a commit to Ndancejic/sonic-sairedis that referenced this pull request Aug 13, 2024
@Ndancejic
Copy link
Contributor Author

@dprital @nazariig Created the PR: #1415

@bingwang-ms
Copy link
Contributor

Cherry-pick is done by #1415.

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

Successfully merging this pull request may close these issues.

7 participants