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

Flush ARP/neighbor entry on FDB flush when port L2-L3 #1506

Merged
merged 6 commits into from
Nov 25, 2020

Conversation

vasant17
Copy link
Contributor

@vasant17 vasant17 commented Nov 17, 2020

Signed-off-by: Vasant vapatil@linkedin.com

What I did
Flush ARP entries when FDB entries are flushed.

Why/How I did it
When we breakout port from 4x25G[10G] --> 1x100G[40G], ARP/neighbor entries via the deleted ports were NOT removed even though FDB entries were flushed. We considered the following two options to remove/flush the neighbor entries:

  1. Ping the neighbor entries from management framework/CLI after the breakout.
  2. Delete the neighbor entries corresponding to the FDB entries

We picked the #2 and here is the solution:

Neighbor orchagent attaches to FDB orchagent and observes.
When FDB orchagent removes/flushes a FDB entry, neighbor orchagent will be notified.
Neighbor orchagent will scan through all saved entries and flush the one's that matches the MAC address with flushed FDB entry.

How I verified it
Wrote and Ran VS test cases. Those test cases will raised as another PR

Details if related

$ sudo pytest --pdb -s -v --dvsname=vs-vp test_port_dpb_system.py
================================================================================== test session starts ==================================================================================
platform linux -- Python 3.6.9, pytest-6.1.2, py-1.9.0, pluggy-0.13.1 -- /usr/bin/python
cachedir: .pytest_cache
rootdir: /home/vapatil/workspace/AZURE_VS/sonic-buildimage/src/sonic-swss/tests
collected 29 items                                                                                                                                                                      

test_port_dpb_system.py::TestPortDPBSystem::test_port_breakout_simple[Ethernet0-2x50G0] remove extra link dummy
PASSED
test_port_dpb_system.py::TestPortDPBSystem::test_port_breakout_simple[Ethernet0-4x25G[10G]0] PASSED
test_port_dpb_system.py::TestPortDPBSystem::test_port_breakout_simple[Ethernet0-2x50G1] PASSED
test_port_dpb_system.py::TestPortDPBSystem::test_port_breakout_simple[Ethernet0-2x25G(2)+1x50G(2)0] PASSED
test_port_dpb_system.py::TestPortDPBSystem::test_port_breakout_simple[Ethernet0-2x50G2] PASSED
test_port_dpb_system.py::TestPortDPBSystem::test_port_breakout_simple[Ethernet0-1x50G(2)+2x25G(2)0] PASSED
test_port_dpb_system.py::TestPortDPBSystem::test_port_breakout_simple[Ethernet0-2x50G3] PASSED
test_port_dpb_system.py::TestPortDPBSystem::test_port_breakout_simple[Ethernet0-1x100G[40G]0] PASSED
test_port_dpb_system.py::TestPortDPBSystem::test_port_breakout_simple[Ethernet0-4x25G[10G]1] PASSED
test_port_dpb_system.py::TestPortDPBSystem::test_port_breakout_simple[Ethernet0-2x25G(2)+1x50G(2)1] PASSED
test_port_dpb_system.py::TestPortDPBSystem::test_port_breakout_simple[Ethernet0-4x25G[10G]2] PASSED
test_port_dpb_system.py::TestPortDPBSystem::test_port_breakout_simple[Ethernet0-1x50G(2)+2x25G(2)1] PASSED
test_port_dpb_system.py::TestPortDPBSystem::test_port_breakout_simple[Ethernet0-4x25G[10G]3] PASSED
test_port_dpb_system.py::TestPortDPBSystem::test_port_breakout_simple[Ethernet0-1x100G[40G]1] PASSED
test_port_dpb_system.py::TestPortDPBSystem::test_port_breakout_simple[Ethernet0-2x25G(2)+1x50G(2)2] PASSED
test_port_dpb_system.py::TestPortDPBSystem::test_port_breakout_simple[Ethernet0-1x50G(2)+2x25G(2)2] PASSED
test_port_dpb_system.py::TestPortDPBSystem::test_port_breakout_simple[Ethernet0-2x25G(2)+1x50G(2)3] PASSED
test_port_dpb_system.py::TestPortDPBSystem::test_port_breakout_simple[Ethernet0-1x100G[40G]2] PASSED
test_port_dpb_system.py::TestPortDPBSystem::test_port_breakout_simple[Ethernet0-1x50G(2)+2x25G(2)3] PASSED
test_port_dpb_system.py::TestPortDPBSystem::test_port_breakout_simple[Ethernet0-1x100G[40G]3] PASSED
test_port_dpb_system.py::TestPortDPBSystem::test_port_breakout_with_vlan PASSED
test_port_dpb_system.py::TestPortDPBSystem::test_port_breakout_with_acl SKIPPED
test_port_dpb_system.py::TestPortDPBSystem::test_cli_command_with_force_option SKIPPED
test_port_dpb_system.py::TestPortDPBSystem::test_cli_command_with_load_port_breakout_config_option SKIPPED
test_port_dpb_system.py::TestPortDPBSystem::test_cli_command_negative SKIPPED
test_port_dpb_system.py::TestPortDPBSystem::test_dpb_arp_flush PASSED
test_port_dpb_system.py::TestPortDPBSystem::test_dpb_arp_flush_vlan PASSED
test_port_dpb_system.py::TestPortDPBSystem::test_dpb_arp_flush_on_port_oper_shut PASSED
test_port_dpb_system.py::TestPortDPBSystem::test_dpb_arp_flush_on_vlan_member_remove PASSED

======================================================================= 25 passed, 4 skipped in 287.01s (0:04:47) =======================================================================
$

Signed-off-by: Vasant <vapatil@linkedin.com>
@daall daall requested review from daall and prsunny and removed request for daall November 17, 2020 22:24
@prsunny
Copy link
Collaborator

prsunny commented Nov 17, 2020

@anish-n for viz

orchagent/neighorch.cpp Outdated Show resolved Hide resolved
@prsunny
Copy link
Collaborator

prsunny commented Nov 20, 2020

IMO, this should be handled as below:

  1. FDB age/flush detected by orchagent
  2. Neighbor orch sends a notification to nbrmgr for refresh for the corresponding IP
  3. Nbrmgr sending request to kernel to refresh and cleanup the entry.

@vasant17
Copy link
Contributor Author

2. Neighbor orch sends a notification to nbrmgr for refresh for the corresponding IP

Could you please elaborate on what is the best way to achieve this #2?

@prsunny
Copy link
Collaborator

prsunny commented Nov 20, 2020

  1. Neighbor orch sends a notification to nbrmgr for refresh for the corresponding IP

Could you please elaborate on what is the best way to achieve this #2?

You could write an entry to APP_DB. Like NEIGH_RESOLVE:<if_name>:<ip_addr>

Vasant added 2 commits November 23, 2020 20:16
Signed-off-by: Vasant <vapatil@linkedin.com>
orchagent/neighorch.cpp Outdated Show resolved Hide resolved
orchagent/neighorch.cpp Outdated Show resolved Hide resolved
cfgmgr/nbrmgr.cpp Outdated Show resolved Hide resolved
orchagent/neighorch.cpp Outdated Show resolved Hide resolved
Copy link
Collaborator

@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.

Lgtm.. minor comments

cfgmgr/nbrmgr.cpp Outdated Show resolved Hide resolved
orchagent/orchdaemon.cpp Show resolved Hide resolved
@Str1ker17
Copy link

This seems to break #1275, since notifyObserversFDBFlush() depends on interface names.

daall pushed a commit to daall/sonic-swss that referenced this pull request Dec 7, 2020
Flush ARP entries when FDB entries are flushed.

*Neighbor orchagent attaches to FDB orchagent and observes.
*When FDB orchagent removes/flushes a FDB entry, neighbor orchagent will be notified.
*Neighbor orchagent will notifies nbrmgr to send a arp refresh. 

Co-authored-by: Vasant <vapatil@linkedin.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants