[muxorch] Fix handling mux neighbors learned after route (#3937)

Ndancejic · stepanblyschak · commit 1fa808a1574b · 2025-11-01T00:05:42.000+02:00
* [muxorch] Fix handling mux neighbors learned after route
Mux neighbors that were learned in standby state when a route entry is present were not being disabled properly due to still being referenced by the route.

This change calls updateNextHopRoutes and decreases the nexthop refcount before disabling the neighbor, allowing the neighbor to be removed.

Also applies fix that passes in the port_name on initial fdb add.

What I did

Update routes pointing to newly learned neighbor in standby state and update refcount
Update refcount for routes updated when neighbor is learned in active state
Moved update logic to after neighbor gets added to mux port
Pass port name when adding Fdb entry
Add vstest to test changes
Why I did it
Neighbors added while a route is present were not being disabled due to nonzero refcounts. This caused traffic to blackhole and dualtor_neighbor_check.py to fail due to mismatch between APPL and ASIC state.

How I verified it
Added vstest with test cases of adding neighbor while route exists in standby and active and toggling between the two states.
diff --git a/orchagent/fdborch.cpp b/orchagent/fdborch.cpp
@@ -855,6 +855,8 @@ void FdbOrch::doTask(Consumer& consumer)
                 }
             }
 
+            // set entry port_name, which is used in mux fdb update logic
+            entry.port_name = port;
 
             FdbData fdbData;
             fdbData.bridge_port_id = SAI_NULL_OBJECT_ID;
diff --git a/orchagent/muxorch.cpp b/orchagent/muxorch.cpp
@@ -658,7 +658,6 @@ void MuxCable::updateNeighbor(NextHopKey nh, bool add)
     SWSS_LOG_NOTICE("Processing update on neighbor %s for mux %s, add %d, state %d",
                      nh.ip_address.to_string().c_str(), mux_name_.c_str(), add, state_);
     sai_object_id_t tnh = mux_orch_->getNextHopTunnelId(MUX_TUNNEL, peer_ip4_);
-    nbr_handler_->update(nh, tnh, add, state_);
     if (add)
     {
         mux_orch_->addNexthop(nh, mux_name_);
@@ -667,7 +666,7 @@ void MuxCable::updateNeighbor(NextHopKey nh, bool add)
     {
         mux_orch_->removeNexthop(nh);
     }
-
+    nbr_handler_->update(nh, tnh, add, state_);
     updateRoutesForNextHop(nh);
 }
 
@@ -742,9 +741,12 @@ void MuxNbrHandler::update(NextHopKey nh, sai_object_id_t tunnelId, bool add, Mu
             neighbors_[nh.ip_address] = gNeighOrch->getLocalNextHopId(nh);
             gNeighOrch->enableNeighbor(nh);
             gRouteOrch->updateNextHopRoutes(nh, num_routes);
+            gNeighOrch->increaseNextHopRefCount(nh, num_routes);
             break;
         case MuxState::MUX_STATE_STANDBY:
             neighbors_[nh.ip_address] = tunnelId;
+            gRouteOrch->updateNextHopRoutes(nh, num_routes);
+            gNeighOrch->decreaseNextHopRefCount(nh, num_routes);
             gNeighOrch->disableNeighbor(nh);
             updateTunnelRoute(nh, true);
             create_route(pfx, tunnelId);
@@ -1561,6 +1563,7 @@ void MuxOrch::updateNeighbor(const NeighborUpdate& update)
         auto it = mux_nexthop_tb_.find(update.entry);
         if (it != mux_nexthop_tb_.end())
         {
+            SWSS_LOG_INFO("port %s, nexthop %s", port.c_str(), it->second.c_str());
             port = it->second;
             removeNexthop(update.entry);
         }
@@ -1770,6 +1773,26 @@ bool MuxOrch::handleMuxCfg(const Request& request)
                                    (MuxCable(port_name, srv_ip, srv_ip6, mux_peer_switch_, cable_type));
         addSkipNeighbors(skip_neighbors);
 
+        // Add neighbors that were learned before this mux port was configured.
+        NeighborTable m_neighbors;
+        gNeighOrch->getMuxNeighborsForPort(port_name, m_neighbors);
+        for (const auto &entry : m_neighbors)
+        {
+            bool nexthop_found = containsNextHop(entry.first);
+            bool is_skip_neighbor = isSkipNeighbor(entry.first.ip_address);
+            if (!nexthop_found && !is_skip_neighbor)
+            {
+                SWSS_LOG_NOTICE("Neighbor %s on %s learned before mux port %s configured. updating...",
+                    entry.first.ip_address.to_string().c_str(),
+                    entry.second.mac.to_string().c_str(),
+                    port_name.c_str()
+                );
+
+                NeighborUpdate neighbor_update = {entry.first, entry.second.mac, 1};
+                updateNeighbor(neighbor_update);
+            }
+        }
+
         SWSS_LOG_NOTICE("Mux entry for port '%s' was added, cable type %d", port_name.c_str(), cable_type);
     }
     else
diff --git a/orchagent/muxorch.h b/orchagent/muxorch.h
@@ -250,6 +250,7 @@ class MuxOrch : public Orch2, public Observer, public Subject
         enable_cache_neigh_updates_ = false;
     }
     void updateCachedNeighbors();
+    bool getMuxPort(const MacAddress&, const string&, string&);
 
 private:
     virtual bool addOperation(const Request& request);
@@ -261,8 +262,6 @@ class MuxOrch : public Orch2, public Observer, public Subject
     void updateNeighbor(const NeighborUpdate&);
     void updateFdb(const FdbUpdate&);
 
-    bool getMuxPort(const MacAddress&, const string&, string&);
-
     /***
      * Methods for managing tunnel routes for neighbor IPs not associated
      * with a specific mux cable
diff --git a/orchagent/neighorch.cpp b/orchagent/neighorch.cpp
@@ -1006,6 +1006,30 @@ void NeighOrch::doTask(Consumer &consumer)
     }
 }
 
+/* Gets all neighbor entries tied to a given mux port */
+void NeighOrch::getMuxNeighborsForPort(string port_name, NeighborTable& m_neighbors)
+{
+    SWSS_LOG_INFO("Getting mux neighbors on %s", port_name.c_str());
+
+    MuxOrch* mux_orch = gDirectory.get<MuxOrch*>();
+    string mux_port_name;
+    for (const auto &entry : m_syncdNeighbors)
+    {
+        // Check if mux port exists for given neighbor entry
+        mux_port_name = "";
+        if (!mux_orch->getMuxPort(entry.second.mac, entry.first.alias, mux_port_name) || mux_port_name.empty())
+        {
+            continue;
+        }
+
+        // Add to m_neighbors if entry found
+        if (mux_port_name == port_name)
+        {
+            m_neighbors.insert(entry);
+        }
+    }
+}
+
 bool NeighOrch::addNeighbor(NeighborContext& ctx)
 {
     SWSS_LOG_ENTER();
diff --git a/orchagent/neighorch.h b/orchagent/neighorch.h
@@ -109,6 +109,7 @@ class NeighOrch : public Orch, public Subject, public Observer
     void resolveNeighbor(const NeighborEntry &);
     void updateSrv6Nexthop(const NextHopKey &, const sai_object_id_t &);
     bool ifChangeInformRemoteNextHop(const string &, bool);
+    void getMuxNeighborsForPort(string port_name, NeighborTable &m_neighbors);
 
     void clearBulkers();
 
diff --git a/orchagent/routeorch.cpp b/orchagent/routeorch.cpp
@@ -1854,8 +1854,8 @@ bool RouteOrch::updateNextHopRoutes(const NextHopKey& nextHop, uint32_t& numRout
             continue;
         }
 
-        SWSS_LOG_INFO("Updating route %s", (*rt).prefix.to_string().c_str());
         next_hop_id = m_neighOrch->getNextHopId(nextHop);
+        SWSS_LOG_INFO("Updating route %s with nexthop %" PRIu64, (*rt).prefix.to_string().c_str(), (uint64_t)next_hop_id);
 
         route_entry.vr_id = (*rt).vrf_id;
         route_entry.switch_id = gSwitchId;
diff --git a/tests/test_mux.py b/tests/test_mux.py
@@ -223,11 +223,14 @@ def check_neigh_in_asic_db(self, asicdb, ip, expected=True):
 
         return ''
 
-    def check_tnl_nexthop_in_asic_db(self, asicdb, expected=1):
+    def check_tnl_nexthop_in_asic_db(self, asicdb, expected=None):
 
         global tunnel_nh_id
 
-        nh = asicdb.wait_for_n_keys(self.ASIC_NEXTHOP_TABLE, expected)
+        if expected:
+            nh = asicdb.wait_for_n_keys(self.ASIC_NEXTHOP_TABLE, expected)
+        else:
+            nh = asicdb.get_keys(self.ASIC_NEXTHOP_TABLE)
 
         for key in nh:
             fvs = asicdb.get_entry(self.ASIC_NEXTHOP_TABLE, key)
@@ -525,12 +528,15 @@ def create_and_test_fdb(self, appdb, asicdb, dvs, dvs_route):
 
         self.del_fdb(dvs, "00-00-00-00-00-11")
 
-    def create_and_test_route(self, appdb, asicdb, dvs, dvs_route):
-
+    def create_and_test_route(self, appdb, asicdb, dvs, dvs_route, mac_Ethernet0, mac_Ethernet4):
         self.set_mux_state(appdb, "Ethernet0", "active")
 
         rtprefix = "2.3.4.0/24"
 
+        # Make sure neighbor is present
+        self.add_neighbor(dvs, self.SERV1_IPV4, mac_Ethernet0)
+        self.add_neighbor(dvs, self.SERV2_IPV4, mac_Ethernet0)
+
         dvs.runcmd(
             "vtysh -c \"configure terminal\" -c \"ip route " + rtprefix +
             " " + self.SERV1_IPV4 + "\""
@@ -546,6 +552,7 @@ def create_and_test_route(self, appdb, asicdb, dvs, dvs_route):
         # Change Mux state to Standby and verify route pointing to Tunnel
         self.set_mux_state(appdb, "Ethernet0", "standby")
 
+        self.check_tnl_nexthop_in_asic_db(asicdb)
         self.check_nexthop_in_asic_db(asicdb, rtkeys[0], True)
 
         # Change Mux state back to Active and verify route is not pointing to Tunnel
@@ -581,6 +588,39 @@ def create_and_test_route(self, appdb, asicdb, dvs, dvs_route):
             " " + self.SERV1_IPV4 + "\""
         )
 
+    def create_and_test_route_learned_before_neighbor(self, appdb, asicdb, dvs, dvs_route, mac):
+        rtprefix = "2.3.4.0/24"
+        neigh = "192.168.0.110"
+        mux_port = "Ethernet0"
+
+        nexthop_map = {"active": neigh, "standby": tunnel_nh_id}
+        toggle_map = {"active": "standby", "standby": "active"}
+
+        for state in ["standby", "active"]:
+            try:
+                current_state = state
+                self.set_mux_state(appdb, mux_port, current_state)
+
+                self.add_route(dvs, rtprefix, [neigh])
+                time.sleep(1)
+                self.add_neighbor(dvs, neigh, mac)
+
+                # Confirm route is pointing to tunnel nh
+                self.check_route_nexthop(dvs_route, asicdb, rtprefix, nexthop_map[current_state], (current_state == "standby"))
+
+                # Toggle the mux a few times
+                current_state = toggle_map[current_state]
+                self.set_mux_state(appdb, mux_port, current_state)
+                self.check_route_nexthop(dvs_route, asicdb, rtprefix, nexthop_map[current_state], (current_state == "standby"))
+
+                current_state = toggle_map[current_state]
+                self.set_mux_state(appdb, mux_port, current_state)
+                self.check_route_nexthop(dvs_route, asicdb, rtprefix, nexthop_map[current_state], (current_state == "standby"))
+
+            finally:
+                self.del_route(dvs, rtprefix)
+                self.del_neighbor(dvs, neigh)
+
     def multi_nexthop_check(self, asicdb, dvs_route, route, nexthops, mux_states, non_mux_nexthop=None, expect_active=None):
         """
         Checks if multi-mux route points to an active nexthop or tunnel.
@@ -1574,7 +1614,6 @@ def setup(self, dvs):
         self.remove_qos_map(db, swsscommon.CFG_DSCP_TO_TC_MAP_TABLE_NAME, dscp_to_tc_map_oid)
         self.remove_qos_map(db, swsscommon.CFG_TC_TO_PRIORITY_GROUP_MAP_TABLE_NAME, tc_to_pg_map_oid)
 
-
     def test_Tunnel(self, dvs, setup_tunnel, restore_tunnel, testlog, setup):
         """ test IPv4 Mux tunnel creation """
         db = swsscommon.DBConnector(swsscommon.APPL_DB, dvs.redis_sock, 0)
@@ -1602,6 +1641,73 @@ def test_Peer(self, dvs, setup_peer_switch, setup_tunnel, setup, testlog):
 
         self.create_and_test_peer(asicdb, encap_tc_to_dscp_map_id, encap_tc_to_queue_map_id)
 
+    def test_neighbor_learned_before_mux_config(self, dvs, dvs_route, setup, setup_vlan, setup_peer_switch, setup_tunnel, testlog):
+        """ test neighbors learned before mux config """
+        test_ip_v4 = "192.168.0.110"
+        test_ip_v6 = "fc02:1000::110"
+
+        toggle_map = {"active": "standby", "standby": "active"}
+
+        asicdb = dvs.get_asic_db()
+        config_db = dvs.get_config_db()
+        appdb = swsscommon.DBConnector(swsscommon.APPL_DB, dvs.redis_sock, 0)
+
+        dvs.runcmd("ip neigh flush all")
+        for create_route in [False, True]:
+            for state in ["active", "standby"]:
+                try:
+                    current_state = state
+
+                    # Step 1.a: add neighbor on port
+                    self.add_fdb(dvs, "Ethernet4", "00-00-00-11-11-11")
+
+                    self.add_neighbor(dvs, test_ip_v4, "00:00:00:11:11:11")
+                    self.check_neigh_in_asic_db(asicdb, test_ip_v4, expected=True)
+
+                    self.add_neighbor(dvs, test_ip_v6, "00:00:00:11:11:11")
+                    self.check_neigh_in_asic_db(asicdb, test_ip_v6, expected=True)
+
+                    if create_route:
+                        # Step 1.b: Create a route pointing to the neighbor
+                        self.add_route(dvs, "11.11.11.11/32", ["192.168.0.100"])
+
+                    # Step 2: configure mux port and verify neighbor state.
+                    self.set_mux_state(appdb, "Ethernet4", current_state)
+                    fvs = {"server_ipv4": self.SERV2_IPV4+self.IPV4_MASK,
+                        "server_ipv6": self.SERV2_IPV6+self.IPV6_MASK}
+                    config_db.create_entry(self.CONFIG_MUX_CABLE, "Ethernet4", fvs)
+
+                    self.check_neigh_in_asic_db(asicdb, test_ip_v4, expected=(current_state != "standby"))
+                    self.check_tunnel_route_in_app_db(dvs, [test_ip_v4+self.IPV4_MASK], expected=(current_state == "standby"))
+                    self.check_neigh_in_asic_db(asicdb, test_ip_v6, expected=(current_state != "standby"))
+                    self.check_tunnel_route_in_app_db(dvs, [test_ip_v6+self.IPV6_MASK], expected=(current_state == "standby"))
+
+                    # Step 3: toggle mux state and verify neighbor state.
+                    current_state = toggle_map[current_state]
+                    self.set_mux_state(appdb, "Ethernet4", current_state)
+
+                    self.check_neigh_in_asic_db(asicdb, test_ip_v4, expected=(current_state != "standby"))
+                    self.check_tunnel_route_in_app_db(dvs, [test_ip_v4+self.IPV4_MASK], expected=(current_state == "standby"))
+                    self.check_neigh_in_asic_db(asicdb, test_ip_v6, expected=(current_state != "standby"))
+                    self.check_tunnel_route_in_app_db(dvs, [test_ip_v6+self.IPV6_MASK], expected=(current_state == "standby"))
+
+                    # Step 4: toggle mux state back to initial state and verify neighbor state.
+                    current_state = toggle_map[current_state]
+                    self.set_mux_state(appdb, "Ethernet4", current_state)
+
+                    self.check_neigh_in_asic_db(asicdb, test_ip_v4, expected=(current_state != "standby"))
+                    self.check_tunnel_route_in_app_db(dvs, [test_ip_v4+self.IPV4_MASK], expected=(current_state == "standby"))
+                    self.check_neigh_in_asic_db(asicdb, test_ip_v6, expected=(current_state != "standby"))
+                    self.check_tunnel_route_in_app_db(dvs, [test_ip_v6+self.IPV6_MASK], expected=(current_state == "standby"))
+
+                finally:
+                    if create_route:
+                        self.del_route(dvs, "11.11.11.11/32")
+                    self.del_neighbor(dvs, test_ip_v4)
+                    self.del_neighbor(dvs, test_ip_v6)
+                    config_db.delete_entry(self.CONFIG_MUX_CABLE, "Ethernet4")
+                    dvs.runcmd("ip neigh flush all")
+
     def test_Neighbor(self, dvs, dvs_route, setup_vlan, setup_mux_cable, testlog):
         """ test Neighbor entries and mux state change """
 
@@ -1619,13 +1725,16 @@ def test_Fdb(self, dvs, dvs_route, testlog):
 
         self.create_and_test_fdb(appdb, asicdb, dvs, dvs_route)
 
-    def test_Route(self, dvs, dvs_route, testlog):
+    def test_Route(self, dvs, intf_fdb_map, dvs_route, setup, setup_vlan, setup_peer_switch, setup_tunnel, setup_mux_cable, testlog):
         """ test Route entries and mux state change """
 
         appdb = swsscommon.DBConnector(swsscommon.APPL_DB, dvs.redis_sock, 0)
         asicdb = dvs.get_asic_db()
+        mac_Ethernet0 = intf_fdb_map["Ethernet0"]
+        mac_Ethernet4 = intf_fdb_map["Ethernet4"]
 
-        self.create_and_test_route(appdb, asicdb, dvs, dvs_route)
+        self.create_and_test_route(appdb, asicdb, dvs, dvs_route, mac_Ethernet0, mac_Ethernet4)
+        self.create_and_test_route_learned_before_neighbor(appdb, asicdb, dvs, dvs_route, mac_Ethernet0)
 
     def test_NH(self, dvs, dvs_route, intf_fdb_map, setup, setup_mux_cable,
                 setup_peer_switch, setup_tunnel, testlog):
@@ -1635,7 +1744,7 @@ def test_NH(self, dvs, dvs_route, intf_fdb_map, setup, setup_mux_cable,
         mac = intf_fdb_map["Ethernet0"]
 
         # get tunnel nexthop
-        self.check_tnl_nexthop_in_asic_db(asicdb, 5)
+        self.check_tnl_nexthop_in_asic_db(asicdb)
 
         self.create_and_test_NH_routes(appdb, asicdb, dvs, dvs_route, mac)
 

Original file line number	Diff line number	Diff line change
`@@ -855,6 +855,8 @@ void FdbOrch::doTask(Consumer& consumer)`
`855`	`855`	`}`
`856`	`856`	`}`
`857`	`857`
	`858`	`+ // set entry port_name, which is used in mux fdb update logic`
	`859`	`+ entry.port_name = port;`
`858`	`860`
`859`	`861`	`FdbData fdbData;`
`860`	`862`	`fdbData.bridge_port_id = SAI_NULL_OBJECT_ID;`
Original file line number	Diff line number	Diff line change
`@@ -1854,8 +1854,8 @@ bool RouteOrch::updateNextHopRoutes(const NextHopKey& nextHop, uint32_t& numRout`
`1854`	`1854`	`continue;`
`1855`	`1855`	`}`
`1856`	`1856`
`1857`		`- SWSS_LOG_INFO("Updating route %s", (*rt).prefix.to_string().c_str());`
`1858`	`1857`	`next_hop_id = m_neighOrch->getNextHopId(nextHop);`
	`1858`	`+ SWSS_LOG_INFO("Updating route %s with nexthop %" PRIu64, (*rt).prefix.to_string().c_str(), (uint64_t)next_hop_id);`
`1859`	`1859`
`1860`	`1860`	`route_entry.vr_id = (*rt).vrf_id;`
`1861`	`1861`	`route_entry.switch_id = gSwitchId;`