From 2e325fd5d658e75f1bd06fd03e6ebbe037a30fca Mon Sep 17 00:00:00 2001 From: Sudharsan Dhamal Gopalarathnam Date: Tue, 21 Jul 2026 13:55:12 -0700 Subject: [PATCH] [202605][intfsorch]Fix race condition between VRF bind and ip address removal (#4758) * [intfsorch]Fix race condition between VRF bind and ip address removal Signed-off-by: dgsudharsan * Changing log level as per review feedback Signed-off-by: dgsudharsan --------- Signed-off-by: dgsudharsan --- orchagent/intfsorch.cpp | 5 ++- tests/mock_tests/intfsorch_ut.cpp | 55 ++++++++++++++++++++++++++++++- 2 files changed, 58 insertions(+), 2 deletions(-) diff --git a/orchagent/intfsorch.cpp b/orchagent/intfsorch.cpp index 22956c00b65..70febe1dbfa 100644 --- a/orchagent/intfsorch.cpp +++ b/orchagent/intfsorch.cpp @@ -857,7 +857,10 @@ void IntfsOrch::doTask(Consumer &consumer) } 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); + SWSS_LOG_NOTICE("Interface '%s' still has %zu IP address(es); deferring VRF '%s' bind until pending IP removals are processed.", + alias.c_str(), m_syncdIntfses[alias].ip_addresses.size(), vrf_name.c_str()); + it++; + continue; } } } diff --git a/tests/mock_tests/intfsorch_ut.cpp b/tests/mock_tests/intfsorch_ut.cpp index e3a7d2eccd5..7db360c9602 100644 --- a/tests/mock_tests/intfsorch_ut.cpp +++ b/tests/mock_tests/intfsorch_ut.cpp @@ -395,4 +395,57 @@ namespace intfsorch_test m_syncdIntfses = gIntfsOrch->getSyncdIntfses(); ASSERT_EQ(m_syncdIntfses["Loopback3"].vrf_id, gVirtualRouterId); } -} \ No newline at end of file + + // Regression test for the batched IP-removal + VRF-bind race. + // m_toSync is an ordered multimap, so within a single drain the bare interface + // key ("Loopback6") is processed before the per-IP key ("Loopback6:6.6.6.6/32"). + // When a config sequence removes the loopback IPs and rebinds the interface to a + // VRF back-to-back, both land in one batch and the VRF-change SET is evaluated + // while the IP is still present. The fix defers (retains) that SET instead of + // logging an error and dropping it, so the bind converges on a later drain. + TEST_F(IntfsOrchTest, IntfsOrchVrfBindDeferredUntilIpRemoved) + { + // create a new vrf + std::deque entries; + entries.push_back({"Vrf-Blue", "SET", { {"NULL", "NULL"}}}); + auto vrfConsumer = dynamic_cast(gVrfOrch->getExecutor(APP_VRF_TABLE_NAME)); + vrfConsumer->addToSync(entries); + static_cast(gVrfOrch)->doTask(); + ASSERT_TRUE(gVrfOrch->isVRFexists("Vrf-Blue")); + auto base_vrf_ref = gVrfOrch->getVrfRefCount("Vrf-Blue"); + + auto intfConsumer = dynamic_cast(gIntfsOrch->getExecutor(APP_INTF_TABLE_NAME)); + + // create a loopback in the default vrf and give it an IP address + entries.clear(); + entries.push_back({"Loopback6", "SET", {}}); + entries.push_back({"Loopback6:6.6.6.6/32", "SET", {{"scope", "global"}, {"family", "IPv4"}}}); + intfConsumer->addToSync(entries); + static_cast(gIntfsOrch)->doTask(); + auto syncd = gIntfsOrch->getSyncdIntfses(); + ASSERT_EQ(syncd["Loopback6"].vrf_id, gVirtualRouterId); + ASSERT_EQ(syncd["Loopback6"].ip_addresses.size(), static_cast(1)); + + // Single batch: remove the IP AND rebind the interface to Vrf-Blue. + // The bare "Loopback6" SET sorts before "Loopback6:6.6.6.6/32" DEL, so the + // bind is evaluated first (IP still present) and must be deferred, not dropped. + entries.clear(); + entries.push_back({"Loopback6", "SET", { {"vrf_name", "Vrf-Blue"}}}); + entries.push_back({"Loopback6:6.6.6.6/32", "DEL", {}}); + intfConsumer->addToSync(entries); + static_cast(gIntfsOrch)->doTask(); + + // After the first drain: IP removed, interface still present and still in the + // default vrf (the bind is pending, not lost). + syncd = gIntfsOrch->getSyncdIntfses(); + ASSERT_NE(syncd.find("Loopback6"), syncd.end()); + ASSERT_EQ(syncd["Loopback6"].ip_addresses.size(), static_cast(0)); + ASSERT_EQ(syncd["Loopback6"].vrf_id, gVirtualRouterId); + + // The deferred bind is retried on the next drain and now succeeds. + static_cast(gIntfsOrch)->doTask(); + syncd = gIntfsOrch->getSyncdIntfses(); + ASSERT_EQ(syncd["Loopback6"].vrf_id, gVrfOrch->getVRFid("Vrf-Blue")); + ASSERT_EQ(gVrfOrch->getVrfRefCount("Vrf-Blue"), base_vrf_ref + 1); + } +}