[intfsorch]: Fence new RIF dependencies during interface removal

orchagent/intfsorch.cpp

@@ -117,6 +117,11 @@ sai_object_id_t IntfsOrch::getRouterIntfsId(const string &alias)
     return port.m_rif_id;
 }
 
+bool IntfsOrch::isIntfRemovalPending(const string &alias) const
+{
+    return m_removingIntfses.find(alias) != m_removingIntfses.end();
+}
+
 bool IntfsOrch::isPrefixSubnet(const IpPrefix &ip_prefix, const string &alias)
 {
     if (m_syncdIntfses.find(alias) == m_syncdIntfses.end())
@@ -1191,11 +1196,19 @@ void IntfsOrch::doTask(Consumer &consumer)
 
                 if (vnet_orch->delIntf(alias, vnet_name, ip_prefix_in_key ? &ip_prefix : nullptr))
                 {
+                    if (!ip_prefix_in_key)
+                    {
+                        m_removingIntfses.erase(alias);
+                    }
                     m_vnetInfses.erase(alias);
                     it = consumer.m_toSync.erase(it);
                 }
                 else
                 {
+                    if (!ip_prefix_in_key)
+                    {
+                        m_removingIntfses.insert(alias);
+                    }
                     it++;
                     continue;
                 }
@@ -1204,12 +1217,18 @@ void IntfsOrch::doTask(Consumer &consumer)
             {
                 if (removeIntf(alias, port.m_vr_id, ip_prefix_in_key ? &ip_prefix : nullptr))
                 {
-                    m_removingIntfses.erase(alias);
+                    if (!ip_prefix_in_key)
+                    {
+                        m_removingIntfses.erase(alias);
+                    }
                     it = consumer.m_toSync.erase(it);
                 }
                 else
                 {
-                    m_removingIntfses.insert(alias);
+                    if (!ip_prefix_in_key)
+                    {
+                        m_removingIntfses.insert(alias);
+                    }
                     it++;
                     continue;
                 }

orchagent/intfsorch.h

@@ -39,6 +39,7 @@ class IntfsOrch : public Orch
     static const int intfsorch_pri;
 
     sai_object_id_t getRouterIntfsId(const string&);
+    bool isIntfRemovalPending(const string&) const;
     bool isPrefixSubnet(const IpPrefix&, const string&);
     bool isInbandIntfInMgmtVrf(const string& alias);
     string getRouterIntfsAlias(const IpAddress &ip, const string &vrf_name = "");

orchagent/neighorch.cpp

@@ -379,6 +379,13 @@ bool NeighOrch::addNextHop(NeighborContext& ctx)
     }
 
     assert(!hasNextHop(nexthop));
+    if (!m_intfsOrch->isRemoteSystemPortIntf(nh.alias) &&
+        m_intfsOrch->isIntfRemovalPending(nh.alias))
+    {
+        SWSS_LOG_INFO("Interface %s is pending removal", nh.alias.c_str());
+        return false;
+    }
+
     sai_object_id_t rif_id = m_intfsOrch->getRouterIntfsId(nh.alias);
 
     vector<sai_attribute_t> next_hop_attrs;
@@ -1324,6 +1331,13 @@ bool NeighOrch::addNeighbor(NeighborContext& ctx)
     string alias = neighborEntry.alias;
     bool bulk_op = ctx.bulk_op;
 
+    if (!m_intfsOrch->isRemoteSystemPortIntf(alias) &&
+        m_intfsOrch->isIntfRemovalPending(alias))
+    {
+        SWSS_LOG_INFO("Interface %s is pending removal", alias.c_str());
+        return false;
+    }
+
     sai_object_id_t rif_id = m_intfsOrch->getRouterIntfsId(alias);
     if (rif_id == SAI_NULL_OBJECT_ID)
     {
@@ -2065,6 +2079,7 @@ bool NeighOrch::enableNeighbors(std::list<NeighborContext>& bulk_ctx_list)
         if(!addNeighbor(*ctx))
         {
             SWSS_LOG_ERROR("Neighbor %s create entry failed.", neighborEntry.ip_address.to_string().c_str());
+            ret = false;
             continue;
         }
     }

orchagent/routeorch.cpp

@@ -1507,7 +1507,10 @@ bool RouteOrch::addNextHopGroup(const NextHopGroupKey &nexthops)
                  m_neighOrch->hasNextHop(NextHopKey(it.ip_address, it.alias)))
         {
             NeighborContext ctx = NeighborContext(it);
-            m_neighOrch->addNextHop(ctx);
+            if (!m_neighOrch->addNextHop(ctx))
+            {
+                return false;
+            }
             next_hop_id = m_neighOrch->getNextHopId(it);
         }
         else
@@ -2083,6 +2086,13 @@ bool RouteOrch::addRoute(RouteBulkContext& ctx, const NextHopGroupKey &nextHops)
                 return true;
             }
 
+            if (!m_intfsOrch->isRemoteSystemPortIntf(nexthop.alias) &&
+                m_intfsOrch->isIntfRemovalPending(nexthop.alias))
+            {
+                SWSS_LOG_INFO("Interface %s is pending removal", nexthop.alias.c_str());
+                return false;
+            }
+
             next_hop_id = m_intfsOrch->getRouterIntfsId(nexthop.alias);
             /* rif is not created yet */
             if (next_hop_id == SAI_NULL_OBJECT_ID)
@@ -2118,7 +2128,10 @@ bool RouteOrch::addRoute(RouteBulkContext& ctx, const NextHopGroupKey &nextHops)
             {
                 /* since IP neighbor NH exists, neighbor is resolved, add MPLS NH */
                 NeighborContext ctx = NeighborContext(nexthop);
-                m_neighOrch->addNextHop(ctx);
+                if (!m_neighOrch->addNextHop(ctx))
+                {
+                    return false;
+                }
                 next_hop_id = m_neighOrch->getNextHopId(nexthop);
             }
             /* IP neighbor is not yet resolved */

tests/mock_tests/intfsorch_ut.cpp

@@ -313,17 +313,30 @@ namespace intfsorch_test
         static_cast<Orch *>(gIntfsOrch)->doTask();
         ASSERT_EQ(current_create_count + 1, create_rif_count);
 
+        // Add a prefix so the whole-interface DEL is processed before its dependency DEL.
+        entries.clear();
+        entries.push_back({"Ethernet0:10.0.0.1/24", "SET", {
+            {"scope", "global"},
+            {"family", "IPv4"}
+        }});
+        consumer->addToSync(entries);
+        static_cast<Orch *>(gIntfsOrch)->doTask();
+
         // create dependency to the interface
         gIntfsOrch->increaseRouterIntfsRefCount("Ethernet0");
 
-        // delete the interface, expect retry because dependency exists
+        // Delete the interface and prefix in lexical order. The successful prefix
+        // DEL must not clear the whole-interface removal fence.
         entries.clear();
         entries.push_back({"Ethernet0", "DEL", { {} }});
+        entries.push_back({"Ethernet0:10.0.0.1/24", "DEL", { {} }});
         consumer = dynamic_cast<Consumer *>(gIntfsOrch->getExecutor(APP_INTF_TABLE_NAME));
         consumer->addToSync(entries);
         auto current_remove_count = remove_rif_count;
         static_cast<Orch *>(gIntfsOrch)->doTask();
         ASSERT_EQ(current_remove_count, remove_rif_count);
+        ASSERT_EQ(consumer->m_toSync.size(), 1u);
+        ASSERT_TRUE(gIntfsOrch->isIntfRemovalPending("Ethernet0"));
 
         // create the interface again, expect retry because interface is in removing
         entries.clear();

tests/mock_tests/neighorch_ut.cpp

@@ -189,6 +189,20 @@ namespace neighorch_test
         }
     };
 
+    TEST_F(NeighOrchTest, LocalNeighborDependenciesRetryWhileInterfaceRemovalPending)
+    {
+        gIntfsOrch->m_removingIntfses.insert(VLAN_1000);
+
+        NeighborContext neighbor_ctx(VLAN1000_NEIGH);
+        neighbor_ctx.mac = MacAddress(MAC1);
+        EXPECT_FALSE(gNeighOrch->addNeighbor(neighbor_ctx));
+
+        NeighborContext next_hop_ctx(VLAN1000_NEIGH);
+        EXPECT_FALSE(gNeighOrch->addNextHop(next_hop_ctx));
+
+        gIntfsOrch->m_removingIntfses.erase(VLAN_1000);
+    }
+
     TEST_F(NeighOrchTest, MultiVlanDuplicateNeighbor)
     {
         EXPECT_CALL(*mock_sai_neighbor_api, create_neighbor_entry);