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

Fix for [EVPN] When MAC moves from remote end point to local, ASIC DB fields are not updated properly for the mac #11503Update NotificationProcessor.cpp #1118

Merged
merged 9 commits into from
Dec 14, 2022

Conversation

anilkpan
Copy link
Contributor

Issue:
When a mac moves from remote to local, the local mac is set in ASIC db using hset, which updates common fields, but does not remove fields specific to remote mac.

Fix:
In case of a mac move event, remove the existing FDB entry from ASIC DB first and then set the new one.

In case of a mac move event, remove the existing FDB entry from ASIC DB first and then set the new one.
Copy link
Collaborator

@dgsudharsan dgsudharsan left a comment

Choose a reason for hiding this comment

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

@anilkpan Can you please add UT for the coverage?

@dgsudharsan
Copy link
Collaborator

@kcudnik Can you please review?

@anilkpan
Copy link
Contributor Author

anilkpan commented Sep 2, 2022

I will work on adding the PTF test for mac move. ETA end of the month.

dgsudharsan
dgsudharsan previously approved these changes Sep 10, 2022
kcudnik
kcudnik previously approved these changes Sep 12, 2022
@kcudnik
Copy link
Collaborator

kcudnik commented Sep 13, 2022

please satisfy code coverage via unittests

@dgsudharsan
Copy link
Collaborator

I will work on adding the PTF test for mac move. ETA end of the month.

Hi @anilkpan Not sure if PTF tests will satisfy the coverage. I believe you need to add gtest here. Can you please check?

@kcudnik
Copy link
Collaborator

kcudnik commented Sep 15, 2022

yes i was suggesting to add tests unittests here

@anilkpan
Copy link
Contributor Author

I will work on adding the PTF test for mac move. ETA end of the month.

Hi @anilkpan Not sure if PTF tests will satisfy the coverage. I believe you need to add gtest here. Can you please check?

@dgsudharsan, i will start working on adding the unittest next week.

@dgsudharsan
Copy link
Collaborator

@anilkpan Can you please provide ETA for UT?

@anilkpan
Copy link
Contributor Author

@dgsudharsan, I have coded the gtest for this scenario. Is there a document that describes how to run the tests locally?

@dgsudharsan
Copy link
Collaborator

@dgsudharsan, I have coded the gtest for this scenario. Is there a document that describes how to run the tests locally?

@kcudnik Can you please help here?

@kcudnik
Copy link
Collaborator

kcudnik commented Oct 9, 2022

@dgsudharsan, I have coded the gtest for this scenario. Is there a document that describes how to run the tests locally?

@kcudnik Can you please help here?

to run locally go to the desired directory and type "make check" and it will run all tests

@anilkpan
Copy link
Contributor Author

@dgsudharsan, I have coded the gtest for this scenario. Is there a document that describes how to run the tests locally?

@kcudnik Can you please help here?

to run locally go to the desired directory and type "make check" and it will run all tests

Thanks @kcudnik, i will try this.

@anilkpan
Copy link
Contributor Author

@kcudnik, 'make check' seems to need libsaimetadata.so.0, which was not built. Any ideas what I am missing?

make check-TESTS
make[1]: Entering directory '/home/anilkpandey/sonic-buildimage/src/sonic-sairedis/lib'
/home/anilkpandey/sonic-buildimage/src/sonic-sairedis/lib/.libs/tests: error while loading shared libraries: libsaimetadata.so.0: cannot open shared object file: No such file or directory
FAIL: tests

1 of 1 test failed

Makefile:1592: recipe for target 'check-TESTS' failed

@kcudnik
Copy link
Collaborator

kcudnik commented Oct 17, 2022

hmm, it should be build, can you type "make" first in top directory to build everyting ?

@dgsudharsan
Copy link
Collaborator

@anilkpan Can you please provide ETA of adding the coverage?

@anilkpan
Copy link
Contributor Author

@dgsudharsan, I plan to add the test by end of next week.

Added coverage for mac move.
@anilkpan anilkpan dismissed stale reviews from kcudnik and dgsudharsan via 279dc26 October 31, 2022 22:04
@anilkpan
Copy link
Contributor Author

anilkpan commented Nov 4, 2022

need to check why the test is failing, will take few more days.

@dgsudharsan
Copy link
Collaborator

@anilkpan Is there any update on this PR?

@anilkpan
Copy link
Contributor Author

@dgsudharsan, The test is failing. Is the syslog or the core dump available for the test run so that I can debug the failure?

@dgsudharsan
Copy link
Collaborator

@dgsudharsan, The test is failing. Is the syslog or the core dump available for the test run so that I can debug the failure?

@vivekrnv Can you please help here?

@vivekrnv
Copy link
Contributor

vivekrnv commented Nov 23, 2022

@dgsudharsan, The test is failing. Is the syslog or the core dump available for the test run so that I can debug the failure?

Hi, There must be a test binary/executable found after you run make. invoke the binary with gdb to drop into a shell. Set breakpoints in you code and debug.
Additionally you can use --gtest_filter to only run the test you want to check.

@dgsudharsan
Copy link
Collaborator

@anilkpan Were you able to proceed? Can you please provide ETA?

@anilkpan
Copy link
Contributor Author

anilkpan commented Dec 2, 2022

Thanks @vivekrnv.
@dgsudharsan, I have added the fix, checks look good so far.

@dgsudharsan
Copy link
Collaborator

@kcudnik Can we merge this?

@dgsudharsan
Copy link
Collaborator

Request for 202211 branch

@prsunny prsunny merged commit 5887d31 into sonic-net:master Dec 14, 2022
@dgsudharsan
Copy link
Collaborator

@prsunny @yxieca Can we add label for 202205 and 202211 for this PR?

@yxieca
Copy link
Contributor

yxieca commented Dec 15, 2022

@anilkpan, @dgsudharsan this change cannot be cherry-picked cleanly. Please open new PR.

@dgsudharsan
Copy link
Collaborator

@anilkpan Can you please raise a separate PR for 202205?

@anilkpan
Copy link
Contributor Author

@dgsudharsan, working on it, ETA 12/22

@anilkpan
Copy link
Contributor Author

@dgsudharsan, created PR #1173 on 202205 branch.

StormLiangMS pushed a commit that referenced this pull request Feb 10, 2023
… fields are not updated properly for the mac #11503Update NotificationProcessor.cpp (#1118)

In case of a mac move event, remove the existing FDB entry from ASIC DB first and then set the new one.
lukasstockner pushed a commit to genesiscloud/sonic-sairedis that referenced this pull request Mar 31, 2023
… fields are not updated properly for the mac #11503Update NotificationProcessor.cpp (sonic-net#1118)

In case of a mac move event, remove the existing FDB entry from ASIC DB first and then set the new one.
lukasstockner pushed a commit to genesiscloud/sonic-sairedis that referenced this pull request Mar 31, 2023
… fields are not updated properly for the mac #11503Update NotificationProcessor.cpp (sonic-net#1118)

In case of a mac move event, remove the existing FDB entry from ASIC DB first and then set the new one.
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.

8 participants