Fix review comments

Author: liorghub

orchagent/intfsorch.cpp

@@ -57,18 +57,11 @@ static const vector<sai_router_interface_stat_t> rifStatIds =
     SAI_ROUTER_INTERFACE_STAT_OUT_ERROR_OCTETS,
 };
 
-// Translation of loopback action from string to sai type
-const unordered_map<string, loopback_action_e> 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<loopback_action_e, sai_packet_action_t> IntfsOrch::m_sai_loopback_action_map =
+const unordered_map<string, sai_packet_action_t> 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<IpPrefix> IntfsOrch:: getSubnetRoutes()
 {
     SWSS_LOG_ENTER();
@@ -504,19 +472,18 @@ set<IpPrefix> 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);

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<Table> m_vidToRidTable;
     unique_ptr<ProducerTable> m_flexCounterTable;
     unique_ptr<ProducerTable> m_flexCounterGroupTable;
-    static const unordered_map<string, loopback_action_e> m_loopback_action_map;
-    static const unordered_map<loopback_action_e, sai_packet_action_t> m_sai_loopback_action_map;
+    static const unordered_map<string, sai_packet_action_t> 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);

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<uint32_t> m_adv_interface_types;
     bool      m_mpls = false;
+    std::string m_loopback_action;
 
     /*
      * Following two bit vectors are used to lock

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