From ea5a175611f5ebae7b93dcc258edf251548d1f4d Mon Sep 17 00:00:00 2001 From: liora Date: Tue, 31 May 2022 03:55:29 +0000 Subject: [PATCH 01/10] Add IP interface loopback action support --- cfgmgr/intfmgr.cpp | 12 ++++ orchagent/intfsorch.cpp | 133 ++++++++++++++++++++++++++++++++++++++-- orchagent/intfsorch.h | 8 +++ orchagent/port.h | 8 +++ 4 files changed, 156 insertions(+), 5 deletions(-) diff --git a/cfgmgr/intfmgr.cpp b/cfgmgr/intfmgr.cpp index 93281dbcd96..105f4f4cdd2 100644 --- a/cfgmgr/intfmgr.cpp +++ b/cfgmgr/intfmgr.cpp @@ -708,6 +708,7 @@ bool IntfMgr::doIntfGeneralTask(const vector& keys, string grat_arp = ""; string mpls = ""; string ipv6_link_local_mode = ""; + string loopback_action = ""; for (auto idx : data) { @@ -750,6 +751,10 @@ bool IntfMgr::doIntfGeneralTask(const vector& keys, { vlanId = value; } + else if (field == "loopback_action") + { + loopback_action = value; + } } if (op == SET_COMMAND) @@ -791,6 +796,13 @@ bool IntfMgr::doIntfGeneralTask(const vector& keys, data.push_back(fvTuple); } + /* Set loopback action */ + if (!loopback_action.empty()) + { + FieldValueTuple fvTuple("loopback_action", loopback_action); + data.push_back(fvTuple); + } + /* Set mpls */ if (!setIntfMpls(alias, mpls)) { diff --git a/orchagent/intfsorch.cpp b/orchagent/intfsorch.cpp index 587e04e140f..cb2b4fe6edc 100644 --- a/orchagent/intfsorch.cpp +++ b/orchagent/intfsorch.cpp @@ -57,6 +57,21 @@ static const vector rifStatIds = SAI_ROUTER_INTERFACE_STAT_OUT_ERROR_OCTETS, }; +// Translation of loopback action from string to sai type +const unordered_map IntfsOrch::m_loopback_action_map = +{ + {"none", LOOPBACK_ACTION_NONE}, + {"drop", LOOPBACK_ACTION_DROP}, + {"forward", LOOPBACK_ACTION_FORWARD}, +}; + +// Translation of loopback action from sonic to sai type +const unordered_map IntfsOrch::m_sai_loopback_action_map = +{ + {LOOPBACK_ACTION_DROP, SAI_PACKET_ACTION_DROP}, + {LOOPBACK_ACTION_FORWARD, SAI_PACKET_ACTION_FORWARD}, +}; + IntfsOrch::IntfsOrch(DBConnector *db, string tableName, VRFOrch *vrf_orch, DBConnector *chassisAppDb) : Orch(db, tableName, intfsorch_pri), m_vrfOrch(vrf_orch) { @@ -416,6 +431,62 @@ bool IntfsOrch::setIntfProxyArp(const string &alias, const string &proxy_arp) return true; } +bool IntfsOrch::setIntfLoopbackAction(const Port &port) +{ + sai_attribute_t attr; + attr.id = SAI_ROUTER_INTERFACE_ATTR_LOOPBACK_PACKET_ACTION; + attr.value.s32 = m_sai_loopback_action_map.at(port.m_loopback_action); + + string action_str = getIntfLoopbackActionStr(port.m_loopback_action); + + 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("Loopback action [%s] set failed, interface [%s], rc [%d]", + action_str.c_str(), port.m_alias.c_str(), status); + + task_process_status handle_status = handleSaiSetStatus(SAI_API_ROUTER_INTERFACE, status); + if (handle_status != task_success) + { + return parseHandleSaiStatusFailure(handle_status); + } + } + + SWSS_LOG_NOTICE("Loopback action [%s] set success, interface [%s]", + action_str.c_str(), port.m_alias.c_str()); + return true; +} + +bool IntfsOrch::getIntfLoopbackAction(const std::string &actionStr, loopback_action_e &action) +{ + auto it = m_loopback_action_map.find(actionStr); + if (it != m_loopback_action_map.end()) + { + action = m_loopback_action_map.at(actionStr); + return true; + } + else + { + SWSS_LOG_WARN("Unsupported loopback action [%s]", actionStr.c_str()); + return false; + } +} + +string IntfsOrch::getIntfLoopbackActionStr(loopback_action_e action) +{ + for (auto it = m_loopback_action_map.begin(); it != m_loopback_action_map.end(); ++it) + { + if (it->second == action) + { + return it->first; + } + } + + SWSS_LOG_WARN("Failed to fetch action as string for action [%u]", action); + return string(); +} + + set IntfsOrch:: getSubnetRoutes() { SWSS_LOG_ENTER(); @@ -433,12 +504,10 @@ set IntfsOrch:: getSubnetRoutes() return subnet_routes; } -bool IntfsOrch::setIntf(const string& alias, sai_object_id_t vrf_id, const IpPrefix *ip_prefix, const bool adminUp, const uint32_t mtu) +bool IntfsOrch::doSetIntf(Port port, sai_object_id_t vrf_id, const IpPrefix *ip_prefix, const bool adminUp, const uint32_t mtu) { SWSS_LOG_ENTER(); - - Port port; - gPortsOrch->getPort(alias, port); + string alias = port.m_alias; auto it_intfs = m_syncdIntfses.find(alias); if (it_intfs == m_syncdIntfses.end()) @@ -551,6 +620,27 @@ bool IntfsOrch::setIntf(const string& alias, sai_object_id_t vrf_id, const IpPre return true; } +bool IntfsOrch::setIntf(const string& alias, loopback_action_e loopbackAction, + sai_object_id_t vrf_id, const IpPrefix *ip_prefix, + const bool adminUp, const uint32_t mtu) +{ + Port port; + gPortsOrch->getPort(alias, port); + port.m_loopback_action = loopbackAction; + + return doSetIntf(port, vrf_id, ip_prefix, adminUp, mtu); +} + +bool IntfsOrch::setIntf(const string& alias, sai_object_id_t vrf_id, + const IpPrefix *ip_prefix, const bool adminUp, + const uint32_t mtu) +{ + Port port; + gPortsOrch->getPort(alias, port); + + return doSetIntf(port, vrf_id, ip_prefix, adminUp, mtu); +} + bool IntfsOrch::removeIntf(const string& alias, sai_object_id_t vrf_id, const IpPrefix *ip_prefix) { SWSS_LOG_ENTER(); @@ -665,6 +755,7 @@ void IntfsOrch::doTask(Consumer &consumer) string inband_type = ""; bool mpls = false; string vlan = ""; + loopback_action_e loopbackAction = LOOPBACK_ACTION_NONE; for (auto idx : data) { @@ -757,6 +848,13 @@ void IntfsOrch::doTask(Consumer &consumer) { vlan = value; } + else if (field == "loopback_action") + { + if(!getIntfLoopbackAction(value, loopbackAction)) + { + continue; + } + } } if (alias == "eth0" || alias == "docker0") @@ -874,7 +972,8 @@ void IntfsOrch::doTask(Consumer &consumer) { adminUp = port.m_admin_state_up; } - if (!setIntf(alias, vrf_id, ip_prefix_in_key ? &ip_prefix : nullptr, adminUp, mtu)) + + if (!setIntf(alias, loopbackAction, vrf_id, ip_prefix_in_key ? &ip_prefix : nullptr, adminUp, mtu)) { it++; continue; @@ -906,6 +1005,19 @@ void IntfsOrch::doTask(Consumer &consumer) setRouterIntfsMpls(port); gPortsOrch->setPort(alias, port); } + + /* Set loopback action */ + string cache_action_str = getIntfLoopbackActionStr(port.m_loopback_action); + string got_action_str = getIntfLoopbackActionStr(loopbackAction); + + if ((loopbackAction != LOOPBACK_ACTION_NONE) and (port.m_loopback_action != loopbackAction)) + { + port.m_loopback_action = loopbackAction; + if(setIntfLoopbackAction(port)) + { + gPortsOrch->setPort(alias, port); + } + } } } @@ -1067,6 +1179,17 @@ bool IntfsOrch::addRouterIntfs(sai_object_id_t vrf_id, Port &port) attr.value.oid = vrf_id; attrs.push_back(attr); + /* Set loopback action only for non virtual rifs */ + if((port.m_loopback_action != LOOPBACK_ACTION_NONE) and + (m_vnetInfses.find(port.m_alias) == m_vnetInfses.end())) + { + string action_str = getIntfLoopbackActionStr(port.m_loopback_action); + + attr.id = SAI_ROUTER_INTERFACE_ATTR_LOOPBACK_PACKET_ACTION; + attr.value.s32 = m_sai_loopback_action_map.at( port.m_loopback_action); + attrs.push_back(attr); + } + attr.id = SAI_ROUTER_INTERFACE_ATTR_SRC_MAC_ADDRESS; if (port.m_mac) { diff --git a/orchagent/intfsorch.h b/orchagent/intfsorch.h index 5605abf1338..941411cd4f9 100644 --- a/orchagent/intfsorch.h +++ b/orchagent/intfsorch.h @@ -54,6 +54,12 @@ class IntfsOrch : public Orch void addRifToFlexCounter(const string&, const string&, const string&); void removeRifFromFlexCounter(const string&, const string&); + bool setIntfLoopbackAction(const Port &port); + bool getIntfLoopbackAction(const std::string &action_str, loopback_action_e &action); + string getIntfLoopbackActionStr(loopback_action_e action); + bool doSetIntf(Port port, sai_object_id_t vrf_id, const IpPrefix *ip_prefix, const bool adminUp, const uint32_t mtu); + bool setIntf(const string& alias, loopback_action_e loopbackAction, sai_object_id_t vrf_id = gVirtualRouterId, + const IpPrefix *ip_prefix = nullptr, const bool adminUp = true, const uint32_t mtu = 0); bool setIntf(const string& alias, sai_object_id_t vrf_id = gVirtualRouterId, const IpPrefix *ip_prefix = nullptr, const bool adminUp = true, const uint32_t mtu = 0); bool removeIntf(const string& alias, sai_object_id_t vrf_id = gVirtualRouterId, const IpPrefix *ip_prefix = nullptr); @@ -88,6 +94,8 @@ class IntfsOrch : public Orch unique_ptr m_vidToRidTable; unique_ptr m_flexCounterTable; unique_ptr m_flexCounterGroupTable; + static const unordered_map m_loopback_action_map; + static const unordered_map m_sai_loopback_action_map; std::string getRifFlexCounterTableKey(std::string s); diff --git a/orchagent/port.h b/orchagent/port.h index 83f61e1b1c7..78f7d6032f5 100644 --- a/orchagent/port.h +++ b/orchagent/port.h @@ -71,6 +71,13 @@ struct SystemLagInfo int32_t spa_id = 0; }; +typedef enum loopback_action +{ + LOOPBACK_ACTION_NONE, + LOOPBACK_ACTION_DROP, + LOOPBACK_ACTION_FORWARD, +} loopback_action_e; + class Port { public: @@ -107,6 +114,7 @@ class Port } std::string m_alias; + loopback_action_e m_loopback_action; Type m_type; int m_index = 0; // PHY_PORT: index uint32_t m_mtu = DEFAULT_MTU; From 5dc7668d8700919ff26a7a78d8e1578f4069e095 Mon Sep 17 00:00:00 2001 From: liora Date: Tue, 31 May 2022 11:52:41 +0000 Subject: [PATCH 02/10] Add VS test for loopback action --- tests/test_interface.py | 78 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 78 insertions(+) diff --git a/tests/test_interface.py b/tests/test_interface.py index a57970b1e5a..fae92f3df5a 100644 --- a/tests/test_interface.py +++ b/tests/test_interface.py @@ -1,9 +1,12 @@ import time import json import pytest +import random from swsscommon import swsscommon +VLAN_SUB_INTERFACE_SEPARATOR = '.' + class TestRouterInterface(object): def setup_db(self, dvs): self.pdb = swsscommon.DBConnector(0, dvs.redis_sock, 0) @@ -2193,6 +2196,81 @@ def test_VLanInterfaceIpv6LinkLocalOnly(self, dvs, testlog): # one loopback router interface assert len(intf_entries) == 1 + def set_loopback_action(self, interface, action): + if interface.startswith("PortChannel"): + tbl_name = "PORTCHANNEL_INTERFACE" + elif interface.startswith("Vlan"): + tbl_name = "VLAN_INTERFACE" + else: + sub_intf_sep_idx = interface.find(VLAN_SUB_INTERFACE_SEPARATOR) + if sub_intf_sep_idx != -1: + tbl_name = "VLAN_SUB_INTERFACE" + else: + tbl_name = "INTERFACE" + + fvs = swsscommon.FieldValuePairs([("loopback_action", action)]) + tbl = swsscommon.Table(self.cdb, tbl_name) + tbl.set(interface, fvs) + time.sleep(1) + + def loopback_action_test(self, iface): + # create interface + self.create_l3_intf(iface, "") + + # set interface loopback action in config database + action_map = {"drop": "SAI_PACKET_ACTION_DROP", "forward": "SAI_PACKET_ACTION_FORWARD"} + action = random.choice(list(action_map.keys())) + self.set_loopback_action(iface, action) + + # check application database + tbl = swsscommon.Table(self.pdb, "INTF_TABLE") + (status, fvs) = tbl.get(iface) + assert status == True + + action_found = False + for fv in fvs: + if fv[0] == "loopback_action": + action_found = True + assert fv[1] == action + assert action_found == True + + # check asic database + tbl = swsscommon.Table(self.adb, "ASIC_STATE:SAI_OBJECT_TYPE_ROUTER_INTERFACE") + intf_entries = tbl.getKeys() + + action_found = False + for key in intf_entries: + (status, fvs) = tbl.get(key) + assert status == True + + for fv in fvs: + if fv[0] == "SAI_ROUTER_INTERFACE_ATTR_LOOPBACK_PACKET_ACTION": + action_found = True + assert fv[1] == action_map[action] + assert action_found == True + + # remove interface + self.remove_l3_intf(iface) + + def test_interfaceLoopbackAction(self, dvs, testlog): + self.setup_db(dvs) + self.loopback_action_test("Ethernet8") + + def test_subInterfaceLoopbackAction(self, dvs, testlog): + self.setup_db(dvs) + self.loopback_action_test("Ethernet8.1") + + def test_vlanInterfaceLoopbackAction(self, dvs, testlog): + self.setup_db(dvs) + self.create_vlan("10") + self.loopback_action_test("Vlan10") + self.remove_vlan("10") + + def test_portChannelInterfaceLoopbackAction(self, dvs, testlog): + self.setup_db(dvs) + self.create_port_channel("PortChannel009") + self.loopback_action_test("PortChannel009") + self.remove_port_channel("PortChannel009") # Add Dummy always-pass test at end as workaroud # for issue when Flaky fail on final test it invokes module tear-down before retrying From bfb61a2694599db1b12d03c245ffbddee1451519 Mon Sep 17 00:00:00 2001 From: liora Date: Tue, 31 May 2022 11:58:58 +0000 Subject: [PATCH 03/10] Remove debug prints leftovers --- orchagent/intfsorch.cpp | 3 --- 1 file changed, 3 deletions(-) diff --git a/orchagent/intfsorch.cpp b/orchagent/intfsorch.cpp index cb2b4fe6edc..80992bffe09 100644 --- a/orchagent/intfsorch.cpp +++ b/orchagent/intfsorch.cpp @@ -1007,9 +1007,6 @@ void IntfsOrch::doTask(Consumer &consumer) } /* Set loopback action */ - string cache_action_str = getIntfLoopbackActionStr(port.m_loopback_action); - string got_action_str = getIntfLoopbackActionStr(loopbackAction); - if ((loopbackAction != LOOPBACK_ACTION_NONE) and (port.m_loopback_action != loopbackAction)) { port.m_loopback_action = loopbackAction; From f871125261630cb7a2c9c3f4b7b0690d700b095f Mon Sep 17 00:00:00 2001 From: liora Date: Tue, 31 May 2022 12:00:44 +0000 Subject: [PATCH 04/10] Remove more debug prints leftovers --- orchagent/intfsorch.cpp | 2 -- 1 file changed, 2 deletions(-) diff --git a/orchagent/intfsorch.cpp b/orchagent/intfsorch.cpp index 80992bffe09..0493989894f 100644 --- a/orchagent/intfsorch.cpp +++ b/orchagent/intfsorch.cpp @@ -1180,8 +1180,6 @@ bool IntfsOrch::addRouterIntfs(sai_object_id_t vrf_id, Port &port) if((port.m_loopback_action != LOOPBACK_ACTION_NONE) and (m_vnetInfses.find(port.m_alias) == m_vnetInfses.end())) { - string action_str = getIntfLoopbackActionStr(port.m_loopback_action); - attr.id = SAI_ROUTER_INTERFACE_ATTR_LOOPBACK_PACKET_ACTION; attr.value.s32 = m_sai_loopback_action_map.at( port.m_loopback_action); attrs.push_back(attr); From 25a6cae6c130c28e65ec85336fa5d7636f6759e9 Mon Sep 17 00:00:00 2001 From: liora Date: Tue, 31 May 2022 19:33:46 +0000 Subject: [PATCH 05/10] Remove unneeded function oveloading --- orchagent/intfsorch.cpp | 37 +++++++++---------------------------- orchagent/intfsorch.h | 5 +---- 2 files changed, 10 insertions(+), 32 deletions(-) diff --git a/orchagent/intfsorch.cpp b/orchagent/intfsorch.cpp index 0493989894f..0d1cca94f5e 100644 --- a/orchagent/intfsorch.cpp +++ b/orchagent/intfsorch.cpp @@ -504,10 +504,14 @@ set IntfsOrch:: getSubnetRoutes() return subnet_routes; } -bool IntfsOrch::doSetIntf(Port port, sai_object_id_t vrf_id, const IpPrefix *ip_prefix, const bool adminUp, const uint32_t mtu) +bool IntfsOrch::setIntf(const string& alias, sai_object_id_t vrf_id, const IpPrefix *ip_prefix, const bool adminUp, const uint32_t mtu, loopback_action_e loopbackAction) + { SWSS_LOG_ENTER(); - string alias = port.m_alias; + Port port; + gPortsOrch->getPort(alias, port); + + port.m_loopback_action = loopbackAction; auto it_intfs = m_syncdIntfses.find(alias); if (it_intfs == m_syncdIntfses.end()) @@ -620,27 +624,6 @@ bool IntfsOrch::doSetIntf(Port port, sai_object_id_t vrf_id, const IpPrefix *ip_ return true; } -bool IntfsOrch::setIntf(const string& alias, loopback_action_e loopbackAction, - sai_object_id_t vrf_id, const IpPrefix *ip_prefix, - const bool adminUp, const uint32_t mtu) -{ - Port port; - gPortsOrch->getPort(alias, port); - port.m_loopback_action = loopbackAction; - - return doSetIntf(port, vrf_id, ip_prefix, adminUp, mtu); -} - -bool IntfsOrch::setIntf(const string& alias, sai_object_id_t vrf_id, - const IpPrefix *ip_prefix, const bool adminUp, - const uint32_t mtu) -{ - Port port; - gPortsOrch->getPort(alias, port); - - return doSetIntf(port, vrf_id, ip_prefix, adminUp, mtu); -} - bool IntfsOrch::removeIntf(const string& alias, sai_object_id_t vrf_id, const IpPrefix *ip_prefix) { SWSS_LOG_ENTER(); @@ -973,7 +956,7 @@ void IntfsOrch::doTask(Consumer &consumer) adminUp = port.m_admin_state_up; } - if (!setIntf(alias, loopbackAction, vrf_id, ip_prefix_in_key ? &ip_prefix : nullptr, adminUp, mtu)) + if (!setIntf(alias, vrf_id, ip_prefix_in_key ? &ip_prefix : nullptr, adminUp, mtu, loopbackAction)) { it++; continue; @@ -1176,12 +1159,10 @@ bool IntfsOrch::addRouterIntfs(sai_object_id_t vrf_id, Port &port) attr.value.oid = vrf_id; attrs.push_back(attr); - /* Set loopback action only for non virtual rifs */ - if((port.m_loopback_action != LOOPBACK_ACTION_NONE) and - (m_vnetInfses.find(port.m_alias) == m_vnetInfses.end())) + if(port.m_loopback_action != LOOPBACK_ACTION_NONE) { attr.id = SAI_ROUTER_INTERFACE_ATTR_LOOPBACK_PACKET_ACTION; - attr.value.s32 = m_sai_loopback_action_map.at( port.m_loopback_action); + attr.value.s32 = m_sai_loopback_action_map.at(port.m_loopback_action); attrs.push_back(attr); } diff --git a/orchagent/intfsorch.h b/orchagent/intfsorch.h index 941411cd4f9..e34f98f6a46 100644 --- a/orchagent/intfsorch.h +++ b/orchagent/intfsorch.h @@ -57,10 +57,7 @@ class IntfsOrch : public Orch bool setIntfLoopbackAction(const Port &port); bool getIntfLoopbackAction(const std::string &action_str, loopback_action_e &action); string getIntfLoopbackActionStr(loopback_action_e action); - bool doSetIntf(Port port, sai_object_id_t vrf_id, const IpPrefix *ip_prefix, const bool adminUp, const uint32_t mtu); - bool setIntf(const string& alias, loopback_action_e loopbackAction, sai_object_id_t vrf_id = gVirtualRouterId, - const IpPrefix *ip_prefix = nullptr, const bool adminUp = true, const uint32_t mtu = 0); - bool setIntf(const string& alias, sai_object_id_t vrf_id = gVirtualRouterId, const IpPrefix *ip_prefix = nullptr, const bool adminUp = true, const uint32_t mtu = 0); + bool setIntf(const string& alias, sai_object_id_t vrf_id = gVirtualRouterId, const IpPrefix *ip_prefix = nullptr, const bool adminUp = true, const uint32_t mtu = 0, loopback_action_e loopbackAction = LOOPBACK_ACTION_NONE); bool removeIntf(const string& alias, sai_object_id_t vrf_id = gVirtualRouterId, const IpPrefix *ip_prefix = nullptr); void addIp2MeRoute(sai_object_id_t vrf_id, const IpPrefix &ip_prefix); From bc7e20aaed950a85a2ec2985ee5e823cfa7b150b Mon Sep 17 00:00:00 2001 From: liora Date: Tue, 31 May 2022 19:41:52 +0000 Subject: [PATCH 06/10] Add newlines in setIntf signature --- orchagent/intfsorch.cpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/orchagent/intfsorch.cpp b/orchagent/intfsorch.cpp index 0d1cca94f5e..8d3210c7078 100644 --- a/orchagent/intfsorch.cpp +++ b/orchagent/intfsorch.cpp @@ -504,13 +504,14 @@ set IntfsOrch:: getSubnetRoutes() return subnet_routes; } -bool IntfsOrch::setIntf(const string& alias, sai_object_id_t vrf_id, const IpPrefix *ip_prefix, const bool adminUp, const uint32_t mtu, loopback_action_e loopbackAction) +bool IntfsOrch::setIntf(const string& alias, sai_object_id_t vrf_id, const IpPrefix *ip_prefix, + const bool adminUp, const uint32_t mtu, loopback_action_e loopbackAction) { SWSS_LOG_ENTER(); + Port port; gPortsOrch->getPort(alias, port); - port.m_loopback_action = loopbackAction; auto it_intfs = m_syncdIntfses.find(alias); From ac63f6ff3aabda5a6000a7b96f78d53c2865f28e Mon Sep 17 00:00:00 2001 From: liora Date: Wed, 1 Jun 2022 13:18:25 +0000 Subject: [PATCH 07/10] Fix review comments --- orchagent/intfsorch.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/orchagent/intfsorch.cpp b/orchagent/intfsorch.cpp index 8d3210c7078..d2a36d99e0b 100644 --- a/orchagent/intfsorch.cpp +++ b/orchagent/intfsorch.cpp @@ -60,7 +60,6 @@ static const vector rifStatIds = // Translation of loopback action from string to sai type const unordered_map IntfsOrch::m_loopback_action_map = { - {"none", LOOPBACK_ACTION_NONE}, {"drop", LOOPBACK_ACTION_DROP}, {"forward", LOOPBACK_ACTION_FORWARD}, }; From abe328d90ca4b18869786936550f2d8da96bfc0e Mon Sep 17 00:00:00 2001 From: liora Date: Tue, 14 Jun 2022 03:45:34 +0000 Subject: [PATCH 08/10] Fix review comments --- orchagent/intfsorch.cpp | 108 +++++++++++++++++----------------------- orchagent/intfsorch.h | 12 ++--- orchagent/port.h | 9 +--- tests/test_interface.py | 45 ++++++++++++----- 4 files changed, 84 insertions(+), 90 deletions(-) diff --git a/orchagent/intfsorch.cpp b/orchagent/intfsorch.cpp index d2a36d99e0b..1be80d836b9 100644 --- a/orchagent/intfsorch.cpp +++ b/orchagent/intfsorch.cpp @@ -57,18 +57,11 @@ static const vector rifStatIds = SAI_ROUTER_INTERFACE_STAT_OUT_ERROR_OCTETS, }; -// Translation of loopback action from string to sai type -const unordered_map IntfsOrch::m_loopback_action_map = -{ - {"drop", LOOPBACK_ACTION_DROP}, - {"forward", LOOPBACK_ACTION_FORWARD}, -}; - // Translation of loopback action from sonic to sai type -const unordered_map IntfsOrch::m_sai_loopback_action_map = +const unordered_map IntfsOrch::m_sai_loopback_action_map = { - {LOOPBACK_ACTION_DROP, SAI_PACKET_ACTION_DROP}, - {LOOPBACK_ACTION_FORWARD, SAI_PACKET_ACTION_FORWARD}, + {"drop", SAI_PACKET_ACTION_DROP}, + {"forward", SAI_PACKET_ACTION_FORWARD}, }; IntfsOrch::IntfsOrch(DBConnector *db, string tableName, VRFOrch *vrf_orch, DBConnector *chassisAppDb) : @@ -430,19 +423,24 @@ bool IntfsOrch::setIntfProxyArp(const string &alias, const string &proxy_arp) return true; } -bool IntfsOrch::setIntfLoopbackAction(const Port &port) +bool IntfsOrch::setIntfLoopbackAction(const Port &port, string actionStr) { sai_attribute_t attr; - attr.id = SAI_ROUTER_INTERFACE_ATTR_LOOPBACK_PACKET_ACTION; - attr.value.s32 = m_sai_loopback_action_map.at(port.m_loopback_action); + sai_packet_action_t action; + + if (!getSaiLoopbackAction(actionStr, action)) + { + return false; + } - string action_str = getIntfLoopbackActionStr(port.m_loopback_action); + attr.id = SAI_ROUTER_INTERFACE_ATTR_LOOPBACK_PACKET_ACTION; + attr.value.s32 = action; 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("Loopback action [%s] set failed, interface [%s], rc [%d]", - action_str.c_str(), port.m_alias.c_str(), status); + actionStr.c_str(), port.m_alias.c_str(), status); task_process_status handle_status = handleSaiSetStatus(SAI_API_ROUTER_INTERFACE, status); if (handle_status != task_success) @@ -452,40 +450,10 @@ bool IntfsOrch::setIntfLoopbackAction(const Port &port) } SWSS_LOG_NOTICE("Loopback action [%s] set success, interface [%s]", - action_str.c_str(), port.m_alias.c_str()); + actionStr.c_str(), port.m_alias.c_str()); return true; } -bool IntfsOrch::getIntfLoopbackAction(const std::string &actionStr, loopback_action_e &action) -{ - auto it = m_loopback_action_map.find(actionStr); - if (it != m_loopback_action_map.end()) - { - action = m_loopback_action_map.at(actionStr); - return true; - } - else - { - SWSS_LOG_WARN("Unsupported loopback action [%s]", actionStr.c_str()); - return false; - } -} - -string IntfsOrch::getIntfLoopbackActionStr(loopback_action_e action) -{ - for (auto it = m_loopback_action_map.begin(); it != m_loopback_action_map.end(); ++it) - { - if (it->second == action) - { - return it->first; - } - } - - SWSS_LOG_WARN("Failed to fetch action as string for action [%u]", action); - return string(); -} - - set IntfsOrch:: getSubnetRoutes() { SWSS_LOG_ENTER(); @@ -504,19 +472,18 @@ set IntfsOrch:: getSubnetRoutes() } bool IntfsOrch::setIntf(const string& alias, sai_object_id_t vrf_id, const IpPrefix *ip_prefix, - const bool adminUp, const uint32_t mtu, loopback_action_e loopbackAction) + const bool adminUp, const uint32_t mtu, string loopbackAction) { SWSS_LOG_ENTER(); Port port; gPortsOrch->getPort(alias, port); - port.m_loopback_action = loopbackAction; auto it_intfs = m_syncdIntfses.find(alias); if (it_intfs == m_syncdIntfses.end()) { - if (!ip_prefix && addRouterIntfs(vrf_id, port)) + if (!ip_prefix && addRouterIntfs(vrf_id, port, loopbackAction)) { gPortsOrch->increasePortRefCount(alias); IntfsEntry intfs_entry; @@ -738,7 +705,7 @@ void IntfsOrch::doTask(Consumer &consumer) string inband_type = ""; bool mpls = false; string vlan = ""; - loopback_action_e loopbackAction = LOOPBACK_ACTION_NONE; + string loopbackAction = ""; for (auto idx : data) { @@ -833,10 +800,7 @@ void IntfsOrch::doTask(Consumer &consumer) } else if (field == "loopback_action") { - if(!getIntfLoopbackAction(value, loopbackAction)) - { - continue; - } + loopbackAction = value; } } @@ -990,11 +954,11 @@ void IntfsOrch::doTask(Consumer &consumer) } /* Set loopback action */ - if ((loopbackAction != LOOPBACK_ACTION_NONE) and (port.m_loopback_action != loopbackAction)) + if ((!loopbackAction.empty()) && (port.m_loopback_action != loopbackAction)) { - port.m_loopback_action = loopbackAction; - if(setIntfLoopbackAction(port)) + if (setIntfLoopbackAction(port, loopbackAction)) { + port.m_loopback_action = loopbackAction; gPortsOrch->setPort(alias, port); } } @@ -1139,7 +1103,22 @@ void IntfsOrch::doTask(Consumer &consumer) } } -bool IntfsOrch::addRouterIntfs(sai_object_id_t vrf_id, Port &port) +bool IntfsOrch::getSaiLoopbackAction(const string &actionStr, sai_packet_action_t &action) +{ + auto it = m_sai_loopback_action_map.find(actionStr); + if (it != m_sai_loopback_action_map.end()) + { + action = m_sai_loopback_action_map.at(actionStr); + return true; + } + else + { + SWSS_LOG_WARN("Unsupported loopback action [%s]", actionStr.c_str()); + return false; + } +} + +bool IntfsOrch::addRouterIntfs(sai_object_id_t vrf_id, Port &port, string loopbackActionStr) { SWSS_LOG_ENTER(); @@ -1159,11 +1138,15 @@ bool IntfsOrch::addRouterIntfs(sai_object_id_t vrf_id, Port &port) attr.value.oid = vrf_id; attrs.push_back(attr); - if(port.m_loopback_action != LOOPBACK_ACTION_NONE) + if (!loopbackActionStr.empty()) { - attr.id = SAI_ROUTER_INTERFACE_ATTR_LOOPBACK_PACKET_ACTION; - attr.value.s32 = m_sai_loopback_action_map.at(port.m_loopback_action); - attrs.push_back(attr); + sai_packet_action_t loopbackAction; + if (getSaiLoopbackAction(loopbackActionStr, loopbackAction)) + { + attr.id = SAI_ROUTER_INTERFACE_ATTR_LOOPBACK_PACKET_ACTION; + attr.value.s32 = loopbackAction; + attrs.push_back(attr); + } } attr.id = SAI_ROUTER_INTERFACE_ATTR_SRC_MAC_ADDRESS; @@ -1276,6 +1259,7 @@ bool IntfsOrch::addRouterIntfs(sai_object_id_t vrf_id, Port &port) } port.m_vr_id = vrf_id; + port.m_loopback_action = loopbackActionStr; gPortsOrch->setPort(port.m_alias, port); m_rifsToAdd.push_back(port); diff --git a/orchagent/intfsorch.h b/orchagent/intfsorch.h index e34f98f6a46..07ee1e01939 100644 --- a/orchagent/intfsorch.h +++ b/orchagent/intfsorch.h @@ -54,10 +54,9 @@ class IntfsOrch : public Orch void addRifToFlexCounter(const string&, const string&, const string&); void removeRifFromFlexCounter(const string&, const string&); - bool setIntfLoopbackAction(const Port &port); - bool getIntfLoopbackAction(const std::string &action_str, loopback_action_e &action); - string getIntfLoopbackActionStr(loopback_action_e action); - bool setIntf(const string& alias, sai_object_id_t vrf_id = gVirtualRouterId, const IpPrefix *ip_prefix = nullptr, const bool adminUp = true, const uint32_t mtu = 0, loopback_action_e loopbackAction = LOOPBACK_ACTION_NONE); + bool setIntfLoopbackAction(const Port &port, string actionStr); + bool getSaiLoopbackAction(const string &actionStr, sai_packet_action_t &action); + bool setIntf(const string& alias, sai_object_id_t vrf_id = gVirtualRouterId, const IpPrefix *ip_prefix = nullptr, const bool adminUp = true, const uint32_t mtu = 0, string loopbackAction = ""); bool removeIntf(const string& alias, sai_object_id_t vrf_id = gVirtualRouterId, const IpPrefix *ip_prefix = nullptr); void addIp2MeRoute(sai_object_id_t vrf_id, const IpPrefix &ip_prefix); @@ -91,12 +90,11 @@ class IntfsOrch : public Orch unique_ptr
m_vidToRidTable; unique_ptr m_flexCounterTable; unique_ptr m_flexCounterGroupTable; - static const unordered_map m_loopback_action_map; - static const unordered_map m_sai_loopback_action_map; + static const unordered_map m_sai_loopback_action_map; std::string getRifFlexCounterTableKey(std::string s); - bool addRouterIntfs(sai_object_id_t vrf_id, Port &port); + bool addRouterIntfs(sai_object_id_t vrf_id, Port &port, string loopbackAction); bool removeRouterIntfs(Port &port); void addDirectedBroadcast(const Port &port, const IpPrefix &ip_prefix); diff --git a/orchagent/port.h b/orchagent/port.h index 78f7d6032f5..51bb2d15be5 100644 --- a/orchagent/port.h +++ b/orchagent/port.h @@ -71,13 +71,6 @@ struct SystemLagInfo int32_t spa_id = 0; }; -typedef enum loopback_action -{ - LOOPBACK_ACTION_NONE, - LOOPBACK_ACTION_DROP, - LOOPBACK_ACTION_FORWARD, -} loopback_action_e; - class Port { public: @@ -114,7 +107,6 @@ class Port } std::string m_alias; - loopback_action_e m_loopback_action; Type m_type; int m_index = 0; // PHY_PORT: index uint32_t m_mtu = DEFAULT_MTU; @@ -158,6 +150,7 @@ class Port sai_port_interface_type_t m_interface_type; std::vector m_adv_interface_types; bool m_mpls = false; + std::string m_loopback_action; /* * Following two bit vectors are used to lock diff --git a/tests/test_interface.py b/tests/test_interface.py index fae92f3df5a..4e1be189499 100644 --- a/tests/test_interface.py +++ b/tests/test_interface.py @@ -2213,13 +2213,11 @@ def set_loopback_action(self, interface, action): tbl.set(interface, fvs) time.sleep(1) - def loopback_action_test(self, iface): + def loopback_action_test(self, iface, action): # create interface self.create_l3_intf(iface, "") - # set interface loopback action in config database - action_map = {"drop": "SAI_PACKET_ACTION_DROP", "forward": "SAI_PACKET_ACTION_FORWARD"} - action = random.choice(list(action_map.keys())) + # set interface loopback action in config db self.set_loopback_action(iface, action) # check application database @@ -2234,10 +2232,11 @@ def loopback_action_test(self, iface): assert fv[1] == action assert action_found == True - # check asic database + # check asic db tbl = swsscommon.Table(self.adb, "ASIC_STATE:SAI_OBJECT_TYPE_ROUTER_INTERFACE") intf_entries = tbl.getKeys() + action_map = {"drop": "SAI_PACKET_ACTION_DROP", "forward": "SAI_PACKET_ACTION_FORWARD"} action_found = False for key in intf_entries: (status, fvs) = tbl.get(key) @@ -2252,25 +2251,45 @@ def loopback_action_test(self, iface): # remove interface self.remove_l3_intf(iface) - def test_interfaceLoopbackAction(self, dvs, testlog): + def test_interfaceLoopbackActionDrop(self, dvs, testlog): + self.setup_db(dvs) + self.loopback_action_test("Ethernet8", "drop") + + def test_interfaceLoopbackActionForward(self, dvs, testlog): self.setup_db(dvs) - self.loopback_action_test("Ethernet8") + self.loopback_action_test("Ethernet8", "forward") - def test_subInterfaceLoopbackAction(self, dvs, testlog): + def test_subInterfaceLoopbackActionDrop(self, dvs, testlog): self.setup_db(dvs) - self.loopback_action_test("Ethernet8.1") + self.loopback_action_test("Ethernet8.1", "drop") + + def test_subInterfaceLoopbackActionForward(self, dvs, testlog): + self.setup_db(dvs) + self.loopback_action_test("Ethernet8.1", "forward") - def test_vlanInterfaceLoopbackAction(self, dvs, testlog): + def test_vlanInterfaceLoopbackActionDrop(self, dvs, testlog): self.setup_db(dvs) self.create_vlan("10") - self.loopback_action_test("Vlan10") + self.loopback_action_test("Vlan10", "drop") self.remove_vlan("10") + + def test_vlanInterfaceLoopbackActionForward(self, dvs, testlog): + self.setup_db(dvs) + self.create_vlan("20") + self.loopback_action_test("Vlan20", "forward") + self.remove_vlan("20") - def test_portChannelInterfaceLoopbackAction(self, dvs, testlog): + def test_portChannelInterfaceLoopbackActionDrop(self, dvs, testlog): self.setup_db(dvs) self.create_port_channel("PortChannel009") - self.loopback_action_test("PortChannel009") + self.loopback_action_test("PortChannel009", "drop") self.remove_port_channel("PortChannel009") + + def test_portChannelInterfaceLoopbackActionForward(self, dvs, testlog): + self.setup_db(dvs) + self.create_port_channel("PortChannel010") + self.loopback_action_test("PortChannel010", "forward") + self.remove_port_channel("PortChannel010") # Add Dummy always-pass test at end as workaroud # for issue when Flaky fail on final test it invokes module tear-down before retrying From 5e571ec6a34e0f06b1f9f49f73ec2c96d2badfda Mon Sep 17 00:00:00 2001 From: liora Date: Wed, 15 Jun 2022 04:05:39 +0000 Subject: [PATCH 09/10] Remove unused import --- tests/test_interface.py | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/test_interface.py b/tests/test_interface.py index 4e1be189499..98f15271520 100644 --- a/tests/test_interface.py +++ b/tests/test_interface.py @@ -1,7 +1,6 @@ import time import json import pytest -import random from swsscommon import swsscommon From 484903019e07c50651a00c93be26572175cfc880 Mon Sep 17 00:00:00 2001 From: liora Date: Thu, 23 Jun 2022 06:35:29 +0000 Subject: [PATCH 10/10] Fix review comments --- orchagent/intfsorch.cpp | 28 +++++++++++----------------- orchagent/intfsorch.h | 1 - orchagent/port.h | 2 -- 3 files changed, 11 insertions(+), 20 deletions(-) diff --git a/orchagent/intfsorch.cpp b/orchagent/intfsorch.cpp index 1be80d836b9..2fb329199d9 100644 --- a/orchagent/intfsorch.cpp +++ b/orchagent/intfsorch.cpp @@ -57,13 +57,6 @@ static const vector rifStatIds = SAI_ROUTER_INTERFACE_STAT_OUT_ERROR_OCTETS, }; -// Translation of loopback action from sonic to sai type -const unordered_map IntfsOrch::m_sai_loopback_action_map = -{ - {"drop", SAI_PACKET_ACTION_DROP}, - {"forward", SAI_PACKET_ACTION_FORWARD}, -}; - IntfsOrch::IntfsOrch(DBConnector *db, string tableName, VRFOrch *vrf_orch, DBConnector *chassisAppDb) : Orch(db, tableName, intfsorch_pri), m_vrfOrch(vrf_orch) { @@ -954,13 +947,9 @@ void IntfsOrch::doTask(Consumer &consumer) } /* Set loopback action */ - if ((!loopbackAction.empty()) && (port.m_loopback_action != loopbackAction)) + if (!loopbackAction.empty()) { - if (setIntfLoopbackAction(port, loopbackAction)) - { - port.m_loopback_action = loopbackAction; - gPortsOrch->setPort(alias, port); - } + setIntfLoopbackAction(port, loopbackAction); } } } @@ -1105,10 +1094,16 @@ void IntfsOrch::doTask(Consumer &consumer) bool IntfsOrch::getSaiLoopbackAction(const string &actionStr, sai_packet_action_t &action) { - auto it = m_sai_loopback_action_map.find(actionStr); - if (it != m_sai_loopback_action_map.end()) + const unordered_map loopbackActionMap = + { + {"drop", SAI_PACKET_ACTION_DROP}, + {"forward", SAI_PACKET_ACTION_FORWARD}, + }; + + auto it = loopbackActionMap.find(actionStr); + if (it != loopbackActionMap.end()) { - action = m_sai_loopback_action_map.at(actionStr); + action = loopbackActionMap.at(actionStr); return true; } else @@ -1259,7 +1254,6 @@ bool IntfsOrch::addRouterIntfs(sai_object_id_t vrf_id, Port &port, string loopba } port.m_vr_id = vrf_id; - port.m_loopback_action = loopbackActionStr; gPortsOrch->setPort(port.m_alias, port); m_rifsToAdd.push_back(port); diff --git a/orchagent/intfsorch.h b/orchagent/intfsorch.h index 07ee1e01939..246474c0ee2 100644 --- a/orchagent/intfsorch.h +++ b/orchagent/intfsorch.h @@ -90,7 +90,6 @@ class IntfsOrch : public Orch unique_ptr
m_vidToRidTable; unique_ptr m_flexCounterTable; unique_ptr m_flexCounterGroupTable; - static const unordered_map m_sai_loopback_action_map; std::string getRifFlexCounterTableKey(std::string s); diff --git a/orchagent/port.h b/orchagent/port.h index 51bb2d15be5..561f9df022a 100644 --- a/orchagent/port.h +++ b/orchagent/port.h @@ -150,8 +150,6 @@ class Port sai_port_interface_type_t m_interface_type; std::vector m_adv_interface_types; bool m_mpls = false; - std::string m_loopback_action; - /* * Following two bit vectors are used to lock * the PG/queue from being changed in BufferOrch.