[portsorch] remove port OID from saiOidToAlias map on port deletion (sonic-net#2483)

- What I did

I fixed an issue that on port deletion (port breakout scenario), the port OID is not removed from saiOidToAlias map, resulting in getPort returns true when querying non-existing port OID but the Port structure is not filled with correct values. Also lowered the log level on receiving non-existing port operational status update

- Why I did it

To fix errors in the log during breakout:

Oct  4 13:15:45.654396 r-bulldog-04 NOTICE swss#orchagent: :- updatePortOperStatus: Port  oper state set from unknown to down
Oct  4 13:15:45.654773 r-bulldog-04 ERR swss#orchagent: :- set: switch id oid:0x0 doesn't exist
Oct  4 13:15:45.654773 r-bulldog-04 WARNING swss#orchagent: :- setHostIntfsOperStatus: Failed to set operation status DOWN to host interface
Oct  4 13:15:45.654773 r-bulldog-04 ERR swss#orchagent: :- updatePortOperStatus: Failed to set host interface  operational status down
Oct  4 13:15:45.654773 r-bulldog-04 WARNING swss#orchagent: :- flushFDBEntries: Couldn't flush FDB. Bridge port OID: 0x0 bvid:0,

- How I verified it
Run UT, run manual port breakout tests.

Signed-off-by: Stepan Blyschak <[email protected]>

Author: stepanblyschak

orchagent/portsorch.cpp

@@ -828,7 +828,10 @@ bool PortsOrch::getPort(sai_object_id_t id, Port &port)
     }
     else
     {
-        getPort(itr->second, port);
+        if (!getPort(itr->second, port))
+        {
+            SWSS_LOG_THROW("Inconsistent saiOidToAlias map and m_portList map: oid=%" PRIx64, id);
+        }
         return true;
     }
 
@@ -1069,9 +1072,9 @@ void PortsOrch::getCpuPort(Port &port)
     port = m_cpuPort;
 }
 
-/* 
- * Create host_tx_ready field in PORT_TABLE of STATE-DB 
- * and set the field to false by default for the 
+/*
+ * Create host_tx_ready field in PORT_TABLE of STATE-DB
+ * and set the field to false by default for the
  * front<Ethernet> port.
  */
 void PortsOrch::initHostTxReadyState(Port &port)
@@ -1096,7 +1099,7 @@ void PortsOrch::initHostTxReadyState(Port &port)
     if (hostTxReady.empty())
     {
         m_portStateTable.hset(port.m_alias, "host_tx_ready", "false");
-        SWSS_LOG_INFO("initalize hostTxReady %s with status %s", 
+        SWSS_LOG_INFO("initalize hostTxReady %s with status %s",
                 port.m_alias.c_str(), hostTxReady.c_str());
     }
 }
@@ -1110,13 +1113,13 @@ bool PortsOrch::setPortAdminStatus(Port &port, bool state)
     attr.value.booldata = state;
 
     /* Update the host_tx_ready to false before setting admin_state, when admin state is false */
-    if (!state) 
+    if (!state)
     {
         m_portStateTable.hset(port.m_alias, "host_tx_ready", "false");
         SWSS_LOG_INFO("Set admin status DOWN host_tx_ready to false to port pid:%" PRIx64,
                 port.m_port_id);
     }
-    
+
     sai_status_t status = sai_port_api->set_port_attribute(port.m_port_id, &attr);
     if (status != SAI_STATUS_SUCCESS)
     {
@@ -1135,14 +1138,14 @@ bool PortsOrch::setPortAdminStatus(Port &port, bool state)
     {
         m_portStateTable.hset(port.m_alias, "host_tx_ready", "false");
     }
-   
+
     /* Update the state table for host_tx_ready*/
     if (state && (gbstatus == true) && (status == SAI_STATUS_SUCCESS) )
     {
         m_portStateTable.hset(port.m_alias, "host_tx_ready", "true");
         SWSS_LOG_INFO("Set admin status UP host_tx_ready to true to port pid:%" PRIx64,
                 port.m_port_id);
-    } 
+    }
 
     return true;
 }
@@ -1376,9 +1379,9 @@ bool PortsOrch::setPortPfcWatchdogStatus(sai_object_id_t portId, uint8_t pfcwd_b
         SWSS_LOG_ERROR("Failed to get port object for port id 0x%" PRIx64, portId);
         return false;
     }
-    
+
     p.m_pfcwd_sw_bitmask = pfcwd_bitmask;
-   
+
     m_portList[p.m_alias] = p;
 
     SWSS_LOG_INFO("Set PFC watchdog port id=0x%" PRIx64 ", bitmast=0x%x", portId, pfcwd_bitmask);
@@ -1396,9 +1399,9 @@ bool PortsOrch::getPortPfcWatchdogStatus(sai_object_id_t portId, uint8_t *pfcwd_
         SWSS_LOG_ERROR("Failed to get port object for port id 0x%" PRIx64, portId);
         return false;
     }
-    
+
     *pfcwd_bitmask = p.m_pfcwd_sw_bitmask;
-    
+
     return true;
 }
 
@@ -3778,10 +3781,10 @@ void PortsOrch::doPortTask(Consumer &consumer)
                         continue;
                     }
                 }
-                
+
                 /* create host_tx_ready field in state-db */
                 initHostTxReadyState(p);
-                
+
                 /* Last step set port admin status */
                 if (!admin_status.empty() && (p.m_admin_state_up != (admin_status == "up")))
                 {
@@ -3859,6 +3862,7 @@ void PortsOrch::doPortTask(Consumer &consumer)
 
             /* Delete port from port list */
             m_portList.erase(alias);
+            saiOidToAlias.erase(port_id);
         }
         else
         {
@@ -6371,7 +6375,7 @@ void PortsOrch::doTask(NotificationConsumer &consumer)
 
             if (!getPort(id, port))
             {
-                SWSS_LOG_ERROR("Failed to get port object for port id 0x%" PRIx64, id);
+                SWSS_LOG_NOTICE("Got port state change for port id 0x%" PRIx64 " which does not exist, possibly outdated event", id);
                 continue;
             }
 

tests/mock_tests/portsorch_ut.cpp

@@ -82,16 +82,16 @@ namespace portsorch_test
     {
         sai_port_api = pold_sai_port_api;
     }
-    
+
     sai_queue_api_t ut_sai_queue_api;
     sai_queue_api_t *pold_sai_queue_api;
     int _sai_set_queue_attr_count = 0;
-    
+
     sai_status_t _ut_stub_sai_set_queue_attribute(sai_object_id_t queue_id, const sai_attribute_t *attr)
     {
         if(attr->id == SAI_QUEUE_ATTR_PFC_DLR_INIT)
         {
-            if(attr->value.booldata == true) 
+            if(attr->value.booldata == true)
             {
                 _sai_set_queue_attr_count++;
             }
@@ -269,6 +269,46 @@ namespace portsorch_test
 
     };
 
+    /**
+     * Test that verifies PortsOrch::getPort() on a port that has been deleted
+     */
+    TEST_F(PortsOrchTest, GetPortTest)
+    {
+        Table portTable = Table(m_app_db.get(), APP_PORT_TABLE_NAME);
+        std::deque<KeyOpFieldsValuesTuple> entries;
+
+        // Get SAI default ports to populate DB
+        auto ports = ut_helper::getInitialSaiPorts();
+
+        for (const auto &it : ports)
+        {
+            portTable.set(it.first, it.second);
+        }
+
+        // Set PortConfigDone
+        portTable.set("PortConfigDone", { { "count", to_string(ports.size()) } });
+
+        // refill consumer
+        gPortsOrch->addExistingData(&portTable);
+
+        // Apply configuration :
+        //  create ports
+        static_cast<Orch *>(gPortsOrch)->doTask();
+
+        Port port;
+        ASSERT_TRUE(gPortsOrch->getPort("Ethernet0", port));
+        ASSERT_NE(port.m_port_id, SAI_NULL_OBJECT_ID);
+
+        // Delete port
+        entries.push_back({"Ethernet0", "DEL", {}});
+        auto consumer = dynamic_cast<Consumer *>(gPortsOrch->getExecutor(APP_PORT_TABLE_NAME));
+        consumer->addToSync(entries);
+        static_cast<Orch *>(gPortsOrch)->doTask();
+        entries.clear();
+
+        ASSERT_FALSE(gPortsOrch->getPort(port.m_port_id, port));
+    }
+
     TEST_F(PortsOrchTest, PortSupportedFecModes)
     {
         _hook_sai_port_api();
@@ -892,7 +932,7 @@ namespace portsorch_test
     * updated to DB.
     */
     TEST_F(PortsOrchTest, PortOperStatusIsUpAndOperSpeedIsZero)
-    {   
+    {
         Table portTable = Table(m_app_db.get(), APP_PORT_TABLE_NAME);
 
         // Get SAI default ports to populate DB
@@ -917,7 +957,7 @@ namespace portsorch_test
         Port port;
         gPortsOrch->getPort("Ethernet0", port);
         ASSERT_TRUE(port.m_oper_status != SAI_PORT_OPER_STATUS_UP);
-        
+
         // save original api since we will spy
         auto orig_port_api = sai_port_api;
         sai_port_api = new sai_port_api_t();
@@ -935,14 +975,14 @@ namespace portsorch_test
                     // Return 0 for port operational speed
                     attrs[0].value.u32 = 0;
                 }
-                
+
                 return (sai_status_t)SAI_STATUS_SUCCESS;
             }
         );
 
         auto exec = static_cast<Notifier *>(gPortsOrch->getExecutor("PORT_STATUS_NOTIFICATIONS"));
         auto consumer = exec->getNotificationConsumer();
-        
+
         // mock a redis reply for notification, it notifies that Ehernet0 is going to up
         mockReply = (redisReply *)calloc(sizeof(redisReply), 1);
         mockReply->type = REDIS_REPLY_ARRAY;
@@ -964,7 +1004,7 @@ namespace portsorch_test
         // trigger the notification
         consumer->readData();
         gPortsOrch->doTask(*consumer);
-        mockReply = nullptr; 
+        mockReply = nullptr;
 
         gPortsOrch->getPort("Ethernet0", port);
         ASSERT_TRUE(port.m_oper_status == SAI_PORT_OPER_STATUS_UP);