Fix multi VLAN neighbor learning (sonic-net#3049)

What I did

When adding a new neighbor, check if the neighbor IP has already been learned on a different VLAN. If it has, remove the old neighbor entry before adding the new one.

Why I did it
On Gemini devices, if a neighbor IP moves from an active port in one VLAN to a second VLAN, then back to the first VLAN (with 3 different MAC addresses), orchagent will crash. Even though the MAC address of the last move is different from the first MAC address, orchagent believes the last MAC address to already be programmed in the hardware and tries to set an attribute of the entry which doesn't exist.

Author: theasianpianist

orchagent/neighorch.cpp

@@ -933,6 +933,27 @@ bool NeighOrch::addNeighbor(const NeighborEntry &neighborEntry, const MacAddress
         }
     }
 
+    PortsOrch* ports_orch = gDirectory.get<PortsOrch*>();
+    auto vlan_ports = ports_orch->getAllVlans();
+
+    for (auto vlan_port: vlan_ports)
+    {
+        if (vlan_port == alias)
+        {
+            continue;
+        }
+        NeighborEntry temp_entry = { ip_address, vlan_port };
+        if (m_syncdNeighbors.find(temp_entry) != m_syncdNeighbors.end())
+        {
+            SWSS_LOG_NOTICE("Neighbor %s on %s already exists, removing before adding new neighbor", ip_address.to_string().c_str(), vlan_port.c_str());
+            if (!removeNeighbor(temp_entry))
+            {
+                SWSS_LOG_ERROR("Failed to remove neighbor %s on %s", ip_address.to_string().c_str(), vlan_port.c_str());
+                return false;
+            }
+        }
+    }
+
     MuxOrch* mux_orch = gDirectory.get<MuxOrch*>();
     bool hw_config = isHwConfigured(neighborEntry);
 

orchagent/portsorch.cpp

@@ -1344,6 +1344,11 @@ map<string, Port>& PortsOrch::getAllPorts()
     return m_portList;
 }
 
+unordered_set<string>& PortsOrch::getAllVlans()
+{
+    return m_vlanPorts;
+}
+
 bool PortsOrch::getPort(string alias, Port &p)
 {
     SWSS_LOG_ENTER();
@@ -6047,6 +6052,7 @@ bool PortsOrch::addVlan(string vlan_alias)
     m_portList[vlan_alias] = vlan;
     m_port_ref_count[vlan_alias] = 0;
     saiOidToAlias[vlan_oid] =  vlan_alias;
+    m_vlanPorts.emplace(vlan_alias);
 
     return true;
 }
@@ -6113,6 +6119,7 @@ bool PortsOrch::removeVlan(Port vlan)
     saiOidToAlias.erase(vlan.m_vlan_info.vlan_oid);
     m_portList.erase(vlan.m_alias);
     m_port_ref_count.erase(vlan.m_alias);
+    m_vlanPorts.erase(vlan.m_alias);
 
     return true;
 }

orchagent/portsorch.h

@@ -2,6 +2,7 @@
 #define SWSS_PORTSORCH_H
 
 #include <map>
+#include <unordered_set>
 
 #include "acltable.h"
 #include "orch.h"
@@ -150,6 +151,8 @@ class PortsOrch : public Orch, public Subject
     bool createVlanHostIntf(Port& vl, string hostif_name);
     bool removeVlanHostIntf(Port vl);
 
+    unordered_set<string>& getAllVlans();
+
     bool createBindAclTableGroup(sai_object_id_t  port_oid,
                    sai_object_id_t  acl_table_oid,
                    sai_object_id_t  &group_oid,
@@ -311,6 +314,7 @@ class PortsOrch : public Orch, public Subject
     map<int, gearbox_port_t> m_gearboxPortMap;
     map<sai_object_id_t, tuple<sai_object_id_t, sai_object_id_t>> m_gearboxPortListLaneMap;
 
+    unordered_set<string> m_vlanPorts;
     port_config_state_t m_portConfigState = PORT_CONFIG_MISSING;
     sai_uint32_t m_portCount;
     map<set<uint32_t>, sai_object_id_t> m_portListLaneMap;

tests/mock_tests/Makefile.am

@@ -44,6 +44,7 @@ tests_SOURCES = aclorch_ut.cpp \
                 mock_table.cpp \
                 mock_hiredis.cpp \
                 mock_redisreply.cpp \
+                mock_sai_api.cpp \
                 bulker_ut.cpp \
                 portmgr_ut.cpp \
                 sflowmgrd_ut.cpp \
@@ -56,6 +57,7 @@ tests_SOURCES = aclorch_ut.cpp \
                 warmrestartassist_ut.cpp \
                 test_failure_handling.cpp \
                 warmrestarthelper_ut.cpp \
+                neighorch_ut.cpp \
                 $(top_srcdir)/warmrestart/warmRestartHelper.cpp \
                 $(top_srcdir)/lib/gearboxutils.cpp \
                 $(top_srcdir)/lib/subintf.cpp \

tests/mock_tests/flowcounterrouteorch_ut.cpp

@@ -135,6 +135,7 @@ namespace flowcounterrouteorch_test
 
             ASSERT_EQ(gPortsOrch, nullptr);
             gPortsOrch = new PortsOrch(m_app_db.get(), m_state_db.get(), ports_tables, m_chassis_app_db.get());
+            gDirectory.set(gPortsOrch);
 
             vector<string> vnet_tables = {
                 APP_VNET_RT_TABLE_NAME,