Skip to content

Commit 4b357e5

Browse files
authored
Fix VRF update handling for loopback interfaces in IntfsOrch (#3512)
<!-- Please make sure you have read and understood the contribution guildlines: https://github.com/Azure/SONiC/blob/gh-pages/CONTRIBUTING.md 1. Make sure your commit includes a signature generted with `git commit -s` 2. Make sure your commit title follows the correct format: [component]: description 3. Make sure your commit message contains enough details about the change and related tests 4. Make sure your pull request adds related reviewers, asignees, labels Please also provide the following information in this pull request: --> **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. **How I verified it** Reproduced the issue with the fix applied and verified in the SAI Redis log that the ip2me route was correctly added to the non-default VRF 2025-01-07.20:48:12.797119|c|SAI_OBJECT_TYPE_ROUTE_ENTRY:{"dest":"8.8.8.8/32","switch_id":"oid:0x21000000000000","vr":"oid:0x3000000000427"}|SAI_ROUTE_ENTRY_ATTR_PACKET_ACTION=SAI_PACKET_ACTION_FORWARD|SAI_ROUTE_ENTRY_ATTR_NEXT_HOP_ID=oid:0x1000000000001 Added Unit Tests **Details if related**
1 parent fe98176 commit 4b357e5

File tree

2 files changed

+71
-0
lines changed

2 files changed

+71
-0
lines changed

orchagent/intfsorch.cpp

+13
Original file line numberDiff line numberDiff line change
@@ -846,6 +846,19 @@ void IntfsOrch::doTask(Consumer &consumer)
846846
m_syncdIntfses[alias] = intfs_entry;
847847
m_vrfOrch->increaseVrfRefCount(vrf_id);
848848
}
849+
else if (m_syncdIntfses[alias].vrf_id != vrf_id)
850+
{
851+
if (m_syncdIntfses[alias].ip_addresses.size() == 0)
852+
{
853+
m_vrfOrch->decreaseVrfRefCount(m_syncdIntfses[alias].vrf_id);
854+
m_vrfOrch->increaseVrfRefCount(vrf_id);
855+
m_syncdIntfses[alias].vrf_id = vrf_id;
856+
}
857+
else
858+
{
859+
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);
860+
}
861+
}
849862
}
850863
else
851864
{

tests/mock_tests/intfsorch_ut.cpp

+58
Original file line numberDiff line numberDiff line change
@@ -330,5 +330,63 @@ namespace intfsorch_test
330330
static_cast<Orch *>(gIntfsOrch)->doTask();
331331
ASSERT_EQ(current_create_count + 1, create_rif_count);
332332
ASSERT_EQ(current_remove_count + 1, remove_rif_count);
333+
};
334+
335+
TEST_F(IntfsOrchTest, IntfsOrchVrfUpdate)
336+
{
337+
//create a new vrf
338+
std::deque<KeyOpFieldsValuesTuple> entries;
339+
entries.push_back({"Vrf-Blue", "SET", { {"NULL", "NULL"}}});
340+
auto consumer = dynamic_cast<Consumer *>(gVrfOrch->getExecutor(APP_VRF_TABLE_NAME));
341+
consumer->addToSync(entries);
342+
static_cast<Orch *>(gVrfOrch)->doTask();
343+
ASSERT_TRUE(gVrfOrch->isVRFexists("Vrf-Blue"));
344+
auto new_vrf_reference_count = gVrfOrch->getVrfRefCount("Vrf-Blue");
345+
ASSERT_EQ(new_vrf_reference_count, 0);
346+
347+
// create an interface
348+
entries.clear();
349+
entries.push_back({"Loopback2", "SET", {}});
350+
consumer = dynamic_cast<Consumer *>(gIntfsOrch->getExecutor(APP_INTF_TABLE_NAME));
351+
consumer->addToSync(entries);
352+
static_cast<Orch *>(gIntfsOrch)->doTask();
353+
IntfsTable m_syncdIntfses = gIntfsOrch->getSyncdIntfses();
354+
ASSERT_EQ(m_syncdIntfses["Loopback2"].vrf_id, gVirtualRouterId);
355+
356+
// change vrf and check if it worked
357+
entries.clear();
358+
entries.push_back({"Loopback2", "SET", { {"vrf_name", "Vrf-Blue"}}});
359+
consumer = dynamic_cast<Consumer *>(gIntfsOrch->getExecutor(APP_INTF_TABLE_NAME));
360+
consumer->addToSync(entries);
361+
static_cast<Orch *>(gIntfsOrch)->doTask();
362+
auto new_vrf_updated_reference_count = gVrfOrch->getVrfRefCount("Vrf-Blue");
363+
ASSERT_EQ(new_vrf_reference_count + 1, new_vrf_updated_reference_count);
364+
m_syncdIntfses = gIntfsOrch->getSyncdIntfses();
365+
ASSERT_EQ(m_syncdIntfses["Loopback2"].vrf_id, gVrfOrch->getVRFid("Vrf-Blue"));
366+
367+
// create an interface
368+
entries.clear();
369+
entries.push_back({"Loopback3", "SET", {}});
370+
consumer = dynamic_cast<Consumer *>(gIntfsOrch->getExecutor(APP_INTF_TABLE_NAME));
371+
consumer->addToSync(entries);
372+
static_cast<Orch *>(gIntfsOrch)->doTask();
373+
m_syncdIntfses = gIntfsOrch->getSyncdIntfses();
374+
ASSERT_EQ(m_syncdIntfses["Loopback3"].vrf_id, gVirtualRouterId);
375+
376+
// Add IP address to the interface
377+
entries.clear();
378+
entries.push_back({"Loopback3:3.3.3.3/32", "SET", {{"scope", "global"},{"family", "IPv4"}}});
379+
consumer = dynamic_cast<Consumer *>(gIntfsOrch->getExecutor(APP_INTF_TABLE_NAME));
380+
consumer->addToSync(entries);
381+
static_cast<Orch *>(gIntfsOrch)->doTask();
382+
383+
// change vrf and check it doesn't affect the interface due to existing IP
384+
entries.clear();
385+
entries.push_back({"Loopback3", "SET", { {"vrf_name", "Vrf-Blue"}}});
386+
consumer = dynamic_cast<Consumer *>(gIntfsOrch->getExecutor(APP_INTF_TABLE_NAME));
387+
consumer->addToSync(entries);
388+
static_cast<Orch *>(gIntfsOrch)->doTask();
389+
m_syncdIntfses = gIntfsOrch->getSyncdIntfses();
390+
ASSERT_EQ(m_syncdIntfses["Loopback3"].vrf_id, gVirtualRouterId);
333391
}
334392
}

0 commit comments

Comments
 (0)