From 371be653db7ceea2bb50245488b86760767fd9f1 Mon Sep 17 00:00:00 2001 From: Jimi Chen Date: Fri, 23 Jun 2023 16:46:59 +0000 Subject: [PATCH] Add delete SAG global MAC while VLAN is enabled test case * Fix test cases errors * Refine logic to keep it simple --- cfgmgr/intfmgr.cpp | 51 +++++++++++++++++++++++------- cfgmgr/intfmgr.h | 1 + orchagent/intfsorch.cpp | 56 +++++++++++++++++++++------------ tests/test_sag.py | 69 +++++++++++++++++++++++++++++++++++++++-- 4 files changed, 144 insertions(+), 33 deletions(-) diff --git a/cfgmgr/intfmgr.cpp b/cfgmgr/intfmgr.cpp index 47f576d8f3..99a1083ad6 100644 --- a/cfgmgr/intfmgr.cpp +++ b/cfgmgr/intfmgr.cpp @@ -198,6 +198,27 @@ bool IntfMgr::setIntfMpls(const string &alias, const string& mpls) return true; } +void IntfMgr::setIntfState(const string &alias, bool isUp) +{ + stringstream cmd; + string res; + + if (isUp) + { + cmd << IP_CMD << " link set " << shellquote(alias) << " up"; + } + else + { + cmd << IP_CMD << " link set " << shellquote(alias) << " down"; + } + + int ret = swss::exec(cmd.str(), res); + if (ret) + { + SWSS_LOG_ERROR("Command '%s' failed with rc %d", cmd.str().c_str(), ret); + } +} + void IntfMgr::addLoopbackIntf(const string &alias) { stringstream cmd; @@ -997,23 +1018,21 @@ bool IntfMgr::doIntfGeneralTask(const vector& keys, string gwmac = ""; if (m_cfgSagTable.hget("GLOBAL", "gateway_mac", gwmac)) { - // before change interface MAC, update sub-interface admin staus to down, - // it can prevent unwanted events received during interface changed in kernel - // bring it up after the changed + // before change interface MAC, set interface down and up to regenerate IPv6 LL by MAC if (sag == "true") { - updateSubIntfAdminStatus(alias, "down"); + setIntfState(alias, false); setIntfMac(alias, gwmac); - updateSubIntfAdminStatus(alias, "up"); + setIntfState(alias, true); FieldValueTuple fvTuple("mac_addr", gwmac); data.push_back(fvTuple); } else if (sag == "false") { - updateSubIntfAdminStatus(alias, "down"); + setIntfState(alias, false); setIntfMac(alias, gMacAddress.to_string()); - updateSubIntfAdminStatus(alias, "up"); + setIntfState(alias, true); FieldValueTuple fvTuple("mac_addr", MacAddress().to_string()); data.push_back(fvTuple); @@ -1326,16 +1345,26 @@ void IntfMgr::updateSagMac(const std::string &macAddr) { SWSS_LOG_NOTICE("set %s mac address to %s", key.c_str(), macAddr.c_str()); - // enable SAG - updateSubIntfAdminStatus(key, "down"); + // enable SAG, set device down and up to regenerate IPv6 LL by MAC + setIntfState(key, false); setIntfMac(key, macAddr); - updateSubIntfAdminStatus(key, "up"); + setIntfState(key, true); vector vlanIntFv; - FieldValueTuple fvTuple("mac_addr", macAddr); + + // keep consistent with default MAC 00:00:00:00:00:00 + string entryMac = MacAddress().to_string(); + if (macAddr != gMacAddress.to_string()) + { + entryMac = macAddr; + } + + FieldValueTuple fvTuple("mac_addr", entryMac); vlanIntFv.push_back(fvTuple); m_appIntfTableProducer.set(key, vlanIntFv); } + } else { + SWSS_LOG_INFO("can't get %s in VLAN_INTERFACE table", key.c_str()); } } } diff --git a/cfgmgr/intfmgr.h b/cfgmgr/intfmgr.h index 70cc9ff234..21ff856b08 100644 --- a/cfgmgr/intfmgr.h +++ b/cfgmgr/intfmgr.h @@ -43,6 +43,7 @@ class IntfMgr : public Orch void setIntfVrf(const std::string &alias, const std::string &vrfName); void setIntfMac(const std::string &alias, const std::string &macAddr); bool setIntfMpls(const std::string &alias, const std::string &mpls); + void setIntfState(const std::string &alias, bool isUp); bool doIntfGeneralTask(const std::vector& keys, std::vector data, const std::string& op); bool doIntfAddrTask(const std::vector& keys, const std::vector& data, const std::string& op); diff --git a/orchagent/intfsorch.cpp b/orchagent/intfsorch.cpp index 7237050413..16dfcfdd4a 100644 --- a/orchagent/intfsorch.cpp +++ b/orchagent/intfsorch.cpp @@ -499,8 +499,6 @@ bool IntfsOrch::setIntf(const string& alias, sai_object_id_t vrf_id, const IpPre else { intfs_entry.mac = gMacAddress; - port.m_mac = gMacAddress; - gPortsOrch->setPort(alias, port); } m_syncdIntfses[alias] = intfs_entry; m_vrfOrch->increaseVrfRefCount(vrf_id); @@ -979,31 +977,49 @@ void IntfsOrch::doTask(Consumer &consumer) } } - // the interface MAC is updated by setIntf(), it can get from PortsOrch - if (mac && gPortsOrch->getPort(alias, port)) + if (!mac) { - sai_attribute_t attr; - attr.id = SAI_ROUTER_INTERFACE_ATTR_SRC_MAC_ADDRESS; - - memcpy(attr.value.mac, port.m_mac.getMac(), sizeof(sai_mac_t)); - sai_status_t status = sai_router_intfs_api->set_router_interface_attribute(port.m_rif_id, &attr); + mac = gMacAddress; + } - if (status != SAI_STATUS_SUCCESS) + // update mac if it is changed + if (m_syncdIntfses.find(alias) != m_syncdIntfses.end()) + { + if (m_syncdIntfses[alias].mac != mac) { - SWSS_LOG_ERROR("Failed to set router interface mac %s for port %s, rv:%d", - port.m_mac.to_string().c_str(), port.m_alias.c_str(), status); - if (handleSaiSetStatus(SAI_API_ROUTER_INTERFACE, status) == task_need_retry) + // port.m_rif_id is set in setIntf(), need to get port again + if (gPortsOrch->getPort(alias, port)) { - it++; - continue; + sai_attribute_t attr; + attr.id = SAI_ROUTER_INTERFACE_ATTR_SRC_MAC_ADDRESS; + + memcpy(attr.value.mac, mac.getMac(), sizeof(sai_mac_t)); + sai_status_t status = sai_router_intfs_api->set_router_interface_attribute(port.m_rif_id, &attr); + + if (status != SAI_STATUS_SUCCESS) + { + SWSS_LOG_ERROR("Failed to set router interface mac %s for port %s, rv:%d", + mac.to_string().c_str(), port.m_alias.c_str(), status); + if (handleSaiSetStatus(SAI_API_ROUTER_INTERFACE, status) == task_need_retry) + { + it++; + continue; + } + } + else + { + SWSS_LOG_NOTICE("Set router interface mac %s for port %s success", + mac.to_string().c_str(), alias.c_str()); + m_syncdIntfses[alias].mac = mac; + } + } + else + { + SWSS_LOG_ERROR("Failed to set router interface mac %s for port %s, get port fail", + mac.to_string().c_str(), alias.c_str()); } } } - else - { - SWSS_LOG_ERROR("Failed to set router interface mac %s for port %s, getPort fail", - mac.to_string().c_str(), alias.c_str()); - } if (!proxy_arp.empty()) { diff --git a/tests/test_sag.py b/tests/test_sag.py index e894a0739c..075ec6371a 100644 --- a/tests/test_sag.py +++ b/tests/test_sag.py @@ -42,7 +42,7 @@ def remove_vrf(self, vrf_name): time.sleep(1) def add_sag_mac(self, mac): - self.cfg_db.create_entry(swsscommon.CFG_SAG_TABLE_NAME, "GLOBAL", {"gwmac": mac}) + self.cfg_db.create_entry(swsscommon.CFG_SAG_TABLE_NAME, "GLOBAL", {"gateway_mac": mac}) time.sleep(1) def remove_sag_mac(self): @@ -88,7 +88,7 @@ def check_kernel_intf_ipv6_addr(self, dvs, interface, addr): assert addr in result def check_app_db_sag_mac(self, fvs, mac): - assert fvs.get("gwmac") == mac + assert fvs.get("gateway_mac") == mac def check_app_db_intf(self, fvs, mac, sag): assert fvs.get("mac_addr") == mac and fvs.get("static_anycast_gateway") == sag @@ -191,6 +191,71 @@ def test_SagAddRemove(self, dvs): # reset interface self.reset_interface(dvs, interface, vlan) + def test_SagRemoveWhenSagVlanEnabled(self, dvs): + self.setup_db(dvs) + + default_mac = "00:00:00:00:00:00" + default_vrf_oid = self.get_asic_db_default_vrf_oid() + system_mac = self.get_system_mac(dvs) + + interface = "Ethernet0" + vlan = "100" + vlan_intf = "Vlan{}".format(vlan) + self.setup_interface(dvs, interface, vlan) + + ip = "1.1.1.1/24" + dvs.add_ip_address(vlan_intf, ip) + + # add SAG global MAC address + mac = "00:11:22:33:44:55" + self.add_sag_mac(mac) + fvs = dvs.get_app_db().wait_for_entry(swsscommon.APP_SAG_TABLE_NAME, "GLOBAL") + self.check_app_db_sag_mac(fvs, mac) + + ipv6_ll_route = self.generate_ipv6_link_local_addr(mac, 128) + self.check_asic_db_route_entry(str(ipv6_ll_route), default_vrf_oid, exist=True) + + # enable SAG on the VLAN interface + self.enable_sag(vlan) + fvs = self.app_db.wait_for_field_match( + swsscommon.APP_INTF_TABLE_NAME, + vlan_intf, + {"mac_addr": mac}) + + self.check_app_db_intf(fvs, mac, "true") + self.check_kernel_intf_mac(dvs, vlan_intf, mac) + + ipv6_ll = self.generate_ipv6_link_local_addr(mac, 64) + self.check_kernel_intf_ipv6_addr(dvs, vlan_intf, str(ipv6_ll)) + self.check_asic_db_router_interface(dvs.getVlanOid(vlan), mac, default_vrf_oid) + + # delete SAG global MAC address + self.remove_sag_mac() + self.app_db.wait_for_deleted_entry(swsscommon.APP_SAG_TABLE_NAME, "GLOBAL") + + ipv6_ll_route = self.generate_ipv6_link_local_addr(mac, 128) + self.check_asic_db_route_entry(str(ipv6_ll_route), default_vrf_oid, exist=False) + + fvs = self.app_db.wait_for_field_match( + swsscommon.APP_INTF_TABLE_NAME, + vlan_intf, + {"NULL": "NULL"} + ) + + self.check_app_db_intf(fvs, default_mac, "true") + self.check_kernel_intf_mac(dvs, vlan_intf, system_mac) + + ipv6_ll = self.generate_ipv6_link_local_addr(system_mac, 64) + self.check_kernel_intf_ipv6_addr(dvs, vlan_intf, str(ipv6_ll)) + self.check_asic_db_router_interface(dvs.getVlanOid(vlan), system_mac, default_vrf_oid) + + # remove ip + dvs.remove_ip_address(vlan_intf, ip) + + # reset interface + self.reset_interface(dvs, interface, vlan) + + def test_SagAddRemoveInVrf(self, dvs): self.setup_db(dvs)