sonic-net / sonic-sairedis

SAI object interface to Redis database, as used in the SONiC project
Other
56 stars 259 forks source link

[syncd] Enable bulk api for neighbor entries #1373

Closed Ndancejic closed 2 months ago

Ndancejic commented 4 months ago

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

needed for ado: #25072867

kcudnik commented 3 months ago

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

Ndancejic commented 2 months ago

/azpw run

mssonicbld commented 2 months ago

/AzurePipelines run

azure-pipelines[bot] commented 2 months ago
Azure Pipelines successfully started running 1 pipeline(s).
Ndancejic commented 2 months ago

@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 commented 2 months ago

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 commented 2 months ago
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 commented 2 months ago
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 commented 2 months ago

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

Ndancejic commented 2 months ago

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

Ndancejic commented 2 months ago

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

Ndancejic commented 2 months ago

/azpw run

mssonicbld commented 2 months ago

/AzurePipelines run

azure-pipelines[bot] commented 2 months ago
Azure Pipelines successfully started running 1 pipeline(s).
kcudnik commented 2 months ago

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

Ndancejic commented 2 months ago

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

kcudnik commented 2 months ago
./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 commented 1 week ago

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

dprital commented 5 days ago

@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 commented 5 days ago

@dprital @nazariig Created the PR: https://github.com/sonic-net/sonic-sairedis/pull/1415