Skip to content

Commit be2f20c

Browse files
wendaniqiluo-msft
wendani
authored and
qiluo-msft
committed
[sub intf] Fix kernel side processing to enslave sub interface to non-default vrf (#1521)
Signed-off-by: Wenda Ni <[email protected]> What I did Change in IntfMgr: Use alias (instead of subIntfAlias before the change) to hold complete sub interface name; use parentAlias (instead of alias before the change) to hold parent port name. Move sub interface creation to the first place before processing fields vrf, proxy arp, and garp. In doing above, a sub interface can receive the same treatment as other types of interfaces (interface type port, type vlan) on vrf, proxy arp, and garp fields, while maintaining an indicator (parentAlias not empty) to receive treatment specific to a sub interface (creation, deletion). Why I did it Fix #1277, #1510 How I verified it vs test. Extended all test cases to test a sub interface ingress linked to a non-default vrf.
1 parent 05f0438 commit be2f20c

4 files changed

Lines changed: 521 additions & 158 deletions

File tree

‎cfgmgr/intfmgr.cpp‎

Lines changed: 67 additions & 67 deletions
Original file line numberDiff line numberDiff line change
@@ -428,16 +428,15 @@ bool IntfMgr::doIntfGeneralTask(const vector<string>& keys,
428428

429429
string alias(keys[0]);
430430
string vlanId;
431-
string subIntfAlias;
431+
string parentAlias;
432432
size_t found = alias.find(VLAN_SUB_INTERFACE_SEPARATOR);
433433
if (found != string::npos)
434434
{
435435
// This is a sub interface
436-
// subIntfAlias holds the complete sub interface name
437-
// while alias becomes the parent interface
438-
subIntfAlias = alias;
436+
// alias holds the complete sub interface name
437+
// while parentAlias holds the parent port name
439438
vlanId = alias.substr(found + 1);
440-
alias = alias.substr(0, found);
439+
parentAlias = alias.substr(0, found);
441440
}
442441
bool is_lo = !alias.compare(0, strlen(LOOPBACK_PREFIX), LOOPBACK_PREFIX);
443442
string mac = "";
@@ -482,7 +481,7 @@ bool IntfMgr::doIntfGeneralTask(const vector<string>& keys,
482481

483482
if (op == SET_COMMAND)
484483
{
485-
if (!isIntfStateOk(alias))
484+
if (!isIntfStateOk(parentAlias.empty() ? alias : parentAlias))
486485
{
487486
SWSS_LOG_DEBUG("Interface is not ready, skipping %s", alias.c_str());
488487
return false;
@@ -520,74 +519,28 @@ bool IntfMgr::doIntfGeneralTask(const vector<string>& keys,
520519
}
521520
}
522521

523-
if (!vrf_name.empty())
524-
{
525-
setIntfVrf(alias, vrf_name);
526-
}
527-
528-
/*Set the mac of interface*/
529-
if (!mac.empty())
530-
{
531-
setIntfMac(alias, mac);
532-
}
533-
else
534-
{
535-
FieldValueTuple fvTuple("mac_addr", MacAddress().to_string());
536-
data.push_back(fvTuple);
537-
}
538-
539-
if (!proxy_arp.empty())
522+
if (!parentAlias.empty())
540523
{
541-
if (!setIntfProxyArp(alias, proxy_arp))
542-
{
543-
SWSS_LOG_ERROR("Failed to set proxy ARP to \"%s\" state for the \"%s\" interface", proxy_arp.c_str(), alias.c_str());
544-
return false;
545-
}
546-
547-
if (!alias.compare(0, strlen(VLAN_PREFIX), VLAN_PREFIX))
548-
{
549-
FieldValueTuple fvTuple("proxy_arp", proxy_arp);
550-
data.push_back(fvTuple);
551-
}
552-
}
553-
554-
if (!grat_arp.empty())
555-
{
556-
if (!setIntfGratArp(alias, grat_arp))
557-
{
558-
SWSS_LOG_ERROR("Failed to set ARP accept to \"%s\" state for the \"%s\" interface", grat_arp.c_str(), alias.c_str());
559-
return false;
560-
}
561-
562-
if (!alias.compare(0, strlen(VLAN_PREFIX), VLAN_PREFIX))
563-
{
564-
FieldValueTuple fvTuple("grat_arp", grat_arp);
565-
data.push_back(fvTuple);
566-
}
567-
}
568-
569-
if (!subIntfAlias.empty())
570-
{
571-
if (m_subIntfList.find(subIntfAlias) == m_subIntfList.end())
524+
if (m_subIntfList.find(alias) == m_subIntfList.end())
572525
{
573526
try
574527
{
575-
addHostSubIntf(alias, subIntfAlias, vlanId);
528+
addHostSubIntf(parentAlias, alias, vlanId);
576529
}
577530
catch (const std::runtime_error &e)
578531
{
579532
SWSS_LOG_NOTICE("Sub interface ip link add failure. Runtime error: %s", e.what());
580533
return false;
581534
}
582535

583-
m_subIntfList.insert(subIntfAlias);
536+
m_subIntfList.insert(alias);
584537
}
585538

586539
if (!mtu.empty())
587540
{
588541
try
589542
{
590-
setHostSubIntfMtu(subIntfAlias, mtu);
543+
setHostSubIntfMtu(alias, mtu);
591544
}
592545
catch (const std::runtime_error &e)
593546
{
@@ -609,7 +562,7 @@ bool IntfMgr::doIntfGeneralTask(const vector<string>& keys,
609562
}
610563
try
611564
{
612-
setHostSubIntfAdminStatus(subIntfAlias, adminStatus);
565+
setHostSubIntfAdminStatus(alias, adminStatus);
613566
}
614567
catch (const std::runtime_error &e)
615568
{
@@ -618,10 +571,57 @@ bool IntfMgr::doIntfGeneralTask(const vector<string>& keys,
618571
}
619572

620573
// set STATE_DB port state
621-
setSubIntfStateOk(subIntfAlias);
574+
setSubIntfStateOk(alias);
622575
}
623-
m_appIntfTableProducer.set(subIntfAlias.empty() ? alias : subIntfAlias, data);
624-
m_stateIntfTable.hset(subIntfAlias.empty() ? alias : subIntfAlias, "vrf", vrf_name);
576+
577+
if (!vrf_name.empty())
578+
{
579+
setIntfVrf(alias, vrf_name);
580+
}
581+
582+
/*Set the mac of interface*/
583+
if (!mac.empty())
584+
{
585+
setIntfMac(alias, mac);
586+
}
587+
else
588+
{
589+
FieldValueTuple fvTuple("mac_addr", MacAddress().to_string());
590+
data.push_back(fvTuple);
591+
}
592+
593+
if (!proxy_arp.empty())
594+
{
595+
if (!setIntfProxyArp(alias, proxy_arp))
596+
{
597+
SWSS_LOG_ERROR("Failed to set proxy ARP to \"%s\" state for the \"%s\" interface", proxy_arp.c_str(), alias.c_str());
598+
return false;
599+
}
600+
601+
if (!alias.compare(0, strlen(VLAN_PREFIX), VLAN_PREFIX))
602+
{
603+
FieldValueTuple fvTuple("proxy_arp", proxy_arp);
604+
data.push_back(fvTuple);
605+
}
606+
}
607+
608+
if (!grat_arp.empty())
609+
{
610+
if (!setIntfGratArp(alias, grat_arp))
611+
{
612+
SWSS_LOG_ERROR("Failed to set ARP accept to \"%s\" state for the \"%s\" interface", grat_arp.c_str(), alias.c_str());
613+
return false;
614+
}
615+
616+
if (!alias.compare(0, strlen(VLAN_PREFIX), VLAN_PREFIX))
617+
{
618+
FieldValueTuple fvTuple("grat_arp", grat_arp);
619+
data.push_back(fvTuple);
620+
}
621+
}
622+
623+
m_appIntfTableProducer.set(alias, data);
624+
m_stateIntfTable.hset(alias, "vrf", vrf_name);
625625
}
626626
else if (op == DEL_COMMAND)
627627
{
@@ -640,16 +640,16 @@ bool IntfMgr::doIntfGeneralTask(const vector<string>& keys,
640640
m_loopbackIntfList.erase(alias);
641641
}
642642

643-
if (!subIntfAlias.empty())
643+
if (!parentAlias.empty())
644644
{
645-
removeHostSubIntf(subIntfAlias);
646-
m_subIntfList.erase(subIntfAlias);
645+
removeHostSubIntf(alias);
646+
m_subIntfList.erase(alias);
647647

648-
removeSubIntfState(subIntfAlias);
648+
removeSubIntfState(alias);
649649
}
650650

651-
m_appIntfTableProducer.del(subIntfAlias.empty() ? alias : subIntfAlias);
652-
m_stateIntfTable.del(subIntfAlias.empty() ? alias : subIntfAlias);
651+
m_appIntfTableProducer.del(alias);
652+
m_stateIntfTable.del(alias);
653653
}
654654
else
655655
{

‎orchagent/intfsorch.cpp‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -660,6 +660,7 @@ void IntfsOrch::doTask(Consumer &consumer)
660660
{
661661
IntfsEntry intfs_entry;
662662
intfs_entry.ref_count = 0;
663+
intfs_entry.proxy_arp = false;
663664
intfs_entry.vrf_id = vrf_id;
664665
m_syncdIntfses[alias] = intfs_entry;
665666
m_vrfOrch->increaseVrfRefCount(vrf_id);

‎tests/conftest.py‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -979,16 +979,19 @@ def set_interface_status(self, interface, admin_status):
979979
tbl.set(interface, fvs)
980980
time.sleep(1)
981981

982-
# deps: acl, fdb_update, fdb, mirror_port_erspan, vlan
983-
def add_ip_address(self, interface, ip):
982+
# deps: acl, fdb_update, fdb, mirror_port_erspan, vlan, sub port intf
983+
def add_ip_address(self, interface, ip, vrf_name=None):
984984
if interface.startswith("PortChannel"):
985985
tbl_name = "PORTCHANNEL_INTERFACE"
986986
elif interface.startswith("Vlan"):
987987
tbl_name = "VLAN_INTERFACE"
988988
else:
989989
tbl_name = "INTERFACE"
990990
tbl = swsscommon.Table(self.cdb, tbl_name)
991-
fvs = swsscommon.FieldValuePairs([("NULL", "NULL")])
991+
pairs = [("NULL", "NULL")]
992+
if vrf_name:
993+
pairs = [("vrf_name", vrf_name)]
994+
fvs = swsscommon.FieldValuePairs(pairs)
992995
tbl.set(interface, fvs)
993996
tbl.set(interface + "|" + ip, fvs)
994997
time.sleep(1)

0 commit comments

Comments
 (0)