Fix VRF update handling for loopback interfaces in IntfsOrch (#3461)

What I did Addressed the scenario where an interface exists in m_syncdIntfses, but the incoming VRF does not match the updated VRF for the loopback interface. The solution involves updating the loopback’s VRF ID, decrementing the reference count for the old VRF, and incrementing the reference count for the new VRF Why I did it Sometimes DEL is missing in the orchagent as mentioned in the issue (#3339), leading to a scenario where, when a loopback interface is added to a non-default VRF, an ip2me route is incorrectly added to the default VRF. This issue occurs because the IntfsOrch is not handling the case where the interface exists in m_syncdIntfses, but the VRF does not match the updated VRF configuration.
sonic-net · Jan 10, 2025 · 54a499b · 54a499b
1 parent 6f30ddd
commit 54a499b
Show file tree

Hide file tree

Showing 2 changed files with 71 additions and 0 deletions.
diff --git a/orchagent/intfsorch.cpp b/orchagent/intfsorch.cpp
@@ -846,6 +846,19 @@ void IntfsOrch::doTask(Consumer &consumer)
                         m_syncdIntfses[alias] = intfs_entry;
                         m_vrfOrch->increaseVrfRefCount(vrf_id);
                     }
+                    else if (m_syncdIntfses[alias].vrf_id != vrf_id)
+                    {
+                        if (m_syncdIntfses[alias].ip_addresses.size() == 0)
+                        {
+                            m_vrfOrch->decreaseVrfRefCount(m_syncdIntfses[alias].vrf_id);
+                            m_vrfOrch->increaseVrfRefCount(vrf_id);
+                            m_syncdIntfses[alias].vrf_id = vrf_id;
+                        }
+                        else
+                        {
+                            SWSS_LOG_ERROR("Failed to set interface '%s' to VRF ID '%d' because it has IP addresses associated with it.", alias.c_str(), vrf_id);
+                        }
+                    }
                 }
                 else
                 {

diff --git a/tests/mock_tests/intfsorch_ut.cpp b/tests/mock_tests/intfsorch_ut.cpp
@@ -330,5 +330,63 @@ namespace intfsorch_test
         static_cast<Orch *>(gIntfsOrch)->doTask();
         ASSERT_EQ(current_create_count + 1, create_rif_count);
         ASSERT_EQ(current_remove_count + 1, remove_rif_count);
+    };
+
+    TEST_F(IntfsOrchTest, IntfsOrchVrfUpdate)
+    {
+        //create a new vrf
+        std::deque<KeyOpFieldsValuesTuple> entries;
+        entries.push_back({"Vrf-Blue", "SET", { {"NULL", "NULL"}}});
+        auto consumer = dynamic_cast<Consumer *>(gVrfOrch->getExecutor(APP_VRF_TABLE_NAME));
+        consumer->addToSync(entries);
+        static_cast<Orch *>(gVrfOrch)->doTask(); 
+        ASSERT_TRUE(gVrfOrch->isVRFexists("Vrf-Blue"));
+        auto new_vrf_reference_count = gVrfOrch->getVrfRefCount("Vrf-Blue");
+        ASSERT_EQ(new_vrf_reference_count, 0);
+
+        // create an interface
+        entries.clear();
+        entries.push_back({"Loopback2", "SET", {}});
+        consumer = dynamic_cast<Consumer *>(gIntfsOrch->getExecutor(APP_INTF_TABLE_NAME));
+        consumer->addToSync(entries);
+        static_cast<Orch *>(gIntfsOrch)->doTask();
+        IntfsTable m_syncdIntfses = gIntfsOrch->getSyncdIntfses();
+        ASSERT_EQ(m_syncdIntfses["Loopback2"].vrf_id, gVirtualRouterId);
+
+        // change vrf and check if it worked
+        entries.clear();
+        entries.push_back({"Loopback2", "SET", { {"vrf_name", "Vrf-Blue"}}});
+        consumer = dynamic_cast<Consumer *>(gIntfsOrch->getExecutor(APP_INTF_TABLE_NAME));
+        consumer->addToSync(entries);
+        static_cast<Orch *>(gIntfsOrch)->doTask();
+        auto new_vrf_updated_reference_count = gVrfOrch->getVrfRefCount("Vrf-Blue");
+        ASSERT_EQ(new_vrf_reference_count + 1, new_vrf_updated_reference_count);
+        m_syncdIntfses = gIntfsOrch->getSyncdIntfses();
+        ASSERT_EQ(m_syncdIntfses["Loopback2"].vrf_id, gVrfOrch->getVRFid("Vrf-Blue"));
+
+        // create an interface
+        entries.clear();
+        entries.push_back({"Loopback3", "SET", {}});
+        consumer = dynamic_cast<Consumer *>(gIntfsOrch->getExecutor(APP_INTF_TABLE_NAME));
+        consumer->addToSync(entries);
+        static_cast<Orch *>(gIntfsOrch)->doTask();
+        m_syncdIntfses = gIntfsOrch->getSyncdIntfses();
+        ASSERT_EQ(m_syncdIntfses["Loopback3"].vrf_id, gVirtualRouterId);
+
+        // Add IP address to the interface
+        entries.clear();
+        entries.push_back({"Loopback3:3.3.3.3/32", "SET", {{"scope", "global"},{"family", "IPv4"}}});
+        consumer = dynamic_cast<Consumer *>(gIntfsOrch->getExecutor(APP_INTF_TABLE_NAME));
+        consumer->addToSync(entries);
+        static_cast<Orch *>(gIntfsOrch)->doTask();
+
+        // change vrf and check it doesn't affect the interface due to existing IP
+        entries.clear();
+        entries.push_back({"Loopback3", "SET", { {"vrf_name", "Vrf-Blue"}}});
+        consumer = dynamic_cast<Consumer *>(gIntfsOrch->getExecutor(APP_INTF_TABLE_NAME));
+        consumer->addToSync(entries);
+        static_cast<Orch *>(gIntfsOrch)->doTask();
+        m_syncdIntfses = gIntfsOrch->getSyncdIntfses();
+        ASSERT_EQ(m_syncdIntfses["Loopback3"].vrf_id, gVirtualRouterId);    
     }
 }