updated as per review comments

orchagent/orch.cpp

@@ -123,7 +123,14 @@ void Consumer::addToSync(const KeyOpFieldsValuesTuple &entry)
         }
         if (iter == ret.second)
         {
-            m_toSync.emplace(key, entry);
+            if(getTableName() == "VLAN_MEMBER_TABLE")
+            {
+                m_toSync.erase(key);
+            }
+            else
+            {
+                m_toSync.emplace(key, entry);
+            }
         }
         else
         {

orchagent/portsorch.cpp

@@ -741,8 +741,8 @@ bool PortsOrch::getPort(sai_object_id_t id, Port &port)
 {
     SWSS_LOG_ENTER();
 
-    auto itr = portOidToName.find(id);
-    if (itr == portOidToName.end())
+    auto itr = saiOidToAlias.find(id);
+    if (itr == saiOidToAlias.end())
     {
         return false;
     }
@@ -789,8 +789,8 @@ bool PortsOrch::getPortByBridgePortId(sai_object_id_t bridge_port_id, Port &port
 {
     SWSS_LOG_ENTER();
 
-    auto itr = portOidToName.find(bridge_port_id);
-    if (itr == portOidToName.end())
+    auto itr = saiOidToAlias.find(bridge_port_id);
+    if (itr == saiOidToAlias.end())
     {
         return false;
     }
@@ -2336,7 +2336,7 @@ bool PortsOrch::initPort(const string &alias, const string &role, const int inde
 
                 /* Add port to port list */
                 m_portList[alias] = p;
-                portOidToName[id] = alias;
+                saiOidToAlias[id] = alias;
                 m_port_ref_count[alias] = 0;
                 m_portOidToIndex[id] = index;
 
@@ -3526,23 +3526,24 @@ void PortsOrch::doVlanMemberTask(Consumer &consumer)
         }
         else if (op == DEL_COMMAND)
         {
-            int ret = true;
             if (vlan.m_members.find(port_alias) != vlan.m_members.end())
             {
-                ret = removeVlanMember(vlan, port);
-            }
-            if ((ret == true) && m_portVlanMember[port.m_alias].empty())
-            {
-                ret = removeBridgePort(port);
-            }
-            if (ret == true)
-            {
-                it = consumer.m_toSync.erase(it);
+                if (removeVlanMember(vlan, port))
+                {
+                    if (m_portVlanMember[port.m_alias].empty())
+                    {
+                        removeBridgePort(port);
+                    }
+                    it = consumer.m_toSync.erase(it);
+                }
+                else
+                {
+                    it++;
+                }
             }
             else
-            {
-                it++;
-            }
+                /* Cannot locate the VLAN */
+                it = consumer.m_toSync.erase(it);
         }
         else
         {
@@ -4217,36 +4218,6 @@ bool PortsOrch::addBridgePort(Port &port)
 
     if (port.m_bridge_port_id != SAI_NULL_OBJECT_ID)
     {
-        /* If the port is being added to the first VLAN,
-         * set bridge port admin status to UP.
-         * This can happen if the port was just removed from
-         * last VLAN and fdb flush is still in progress.
-         */
-        if (m_portVlanMember[port.m_alias].empty())
-        {
-            if (port.m_fdb_count > 0)
-            {
-                return false;
-            }
-            sai_attribute_t attr;
-            attr.id = SAI_BRIDGE_PORT_ATTR_ADMIN_STATE;
-            attr.value.booldata = true;
-            sai_status_t status = sai_bridge_api->set_bridge_port_attribute(port.m_bridge_port_id, &attr);
-            if (status != SAI_STATUS_SUCCESS)
-            {
-                SWSS_LOG_ERROR("Failed to set bridge port %s admin status to UP, rv:%d",
-                    port.m_alias.c_str(), status);
-                return false;
-            }
-            port.m_bridge_port_admin_state = true;
-            m_portList[port.m_alias] = port;
-            if (!setHostIntfsStripTag(port, SAI_HOSTIF_VLAN_TAG_KEEP))
-            {
-                SWSS_LOG_ERROR("Failed to set %s for hostif of port %s",
-                        hostif_vlan_tag[SAI_HOSTIF_VLAN_TAG_KEEP], port.m_alias.c_str());
-                return false;
-            }
-        }
         return true;
     }
 
@@ -4324,16 +4295,14 @@ bool PortsOrch::addBridgePort(Port &port)
         }
     }
 
-    port.m_bridge_port_admin_state = true;
-
     if (!setHostIntfsStripTag(port, SAI_HOSTIF_VLAN_TAG_KEEP))
     {
         SWSS_LOG_ERROR("Failed to set %s for hostif of port %s",
                 hostif_vlan_tag[SAI_HOSTIF_VLAN_TAG_KEEP], port.m_alias.c_str());
         return false;
     }
     m_portList[port.m_alias] = port;
-    portOidToName[port.m_bridge_port_id] = port.m_alias;
+    saiOidToAlias[port.m_bridge_port_id] = port.m_alias;
     SWSS_LOG_NOTICE("Add bridge port %s to default 1Q bridge", port.m_alias.c_str());
 
     PortUpdate update = { port, true };
@@ -4350,39 +4319,36 @@ bool PortsOrch::removeBridgePort(Port &port)
     {
         return true;
     }
-    if (port.m_bridge_port_admin_state)
-    {
-        /* Set bridge port admin status to DOWN */
-        sai_attribute_t attr;
-        attr.id = SAI_BRIDGE_PORT_ATTR_ADMIN_STATE;
-        attr.value.booldata = false;
+    /* Set bridge port admin status to DOWN */
+    sai_attribute_t attr;
+    attr.id = SAI_BRIDGE_PORT_ATTR_ADMIN_STATE;
+    attr.value.booldata = false;
 
-        sai_status_t status = sai_bridge_api->set_bridge_port_attribute(port.m_bridge_port_id, &attr);
-        if (status != SAI_STATUS_SUCCESS)
+    sai_status_t status = sai_bridge_api->set_bridge_port_attribute(port.m_bridge_port_id, &attr);
+    if (status != SAI_STATUS_SUCCESS)
+    {
+        SWSS_LOG_ERROR("Failed to set bridge port %s admin status to DOWN, rv:%d",
+            port.m_alias.c_str(), status);
+        task_process_status handle_status = handleSaiSetStatus(SAI_API_BRIDGE, status);
+        if (handle_status != task_success)
         {
-            SWSS_LOG_ERROR("Failed to set bridge port %s admin status to DOWN, rv:%d",
-                    port.m_alias.c_str(), status);
-            task_process_status handle_status = handleSaiSetStatus(SAI_API_BRIDGE, status);
-            if (handle_status != task_success)
-            {
-                return parseHandleSaiStatusFailure(handle_status);
-            }
+            return parseHandleSaiStatusFailure(handle_status);
         }
+    }
 
-        if (!setHostIntfsStripTag(port, SAI_HOSTIF_VLAN_TAG_STRIP))
-        {
-            SWSS_LOG_ERROR("Failed to set %s for hostif of port %s",
-                    hostif_vlan_tag[SAI_HOSTIF_VLAN_TAG_STRIP], port.m_alias.c_str());
-            return false;
-        }
+    if (!setHostIntfsStripTag(port, SAI_HOSTIF_VLAN_TAG_STRIP))
+    {
+        SWSS_LOG_ERROR("Failed to set %s for hostif of port %s",
+                hostif_vlan_tag[SAI_HOSTIF_VLAN_TAG_STRIP], port.m_alias.c_str());
+        return false;
     }
 
     //Flush the FDB entires corresponding to the port
     gFdbOrch->flushFDBEntries(port.m_bridge_port_id, SAI_NULL_OBJECT_ID);
     SWSS_LOG_INFO("Flush FDB entries for port %s", port.m_alias.c_str());
 
     /* Remove bridge port */
-    sai_status_t status = sai_bridge_api->remove_bridge_port(port.m_bridge_port_id);
+    status = sai_bridge_api->remove_bridge_port(port.m_bridge_port_id);
     if (status != SAI_STATUS_SUCCESS)
     {
         SWSS_LOG_ERROR("Failed to remove bridge port %s from default 1Q bridge, rv:%d",
@@ -4393,7 +4359,7 @@ bool PortsOrch::removeBridgePort(Port &port)
             return parseHandleSaiStatusFailure(handle_status);
         }
     }
-    portOidToName.erase(port.m_bridge_port_id);
+    saiOidToAlias.erase(port.m_bridge_port_id);
     port.m_bridge_port_id = SAI_NULL_OBJECT_ID;
 
     /* Remove bridge port */
@@ -4477,7 +4443,7 @@ bool PortsOrch::addVlan(string vlan_alias)
     vlan.m_members = set<string>();
     m_portList[vlan_alias] = vlan;
     m_port_ref_count[vlan_alias] = 0;
-    portOidToName[vlan_oid] =  vlan_alias;
+    saiOidToAlias[vlan_oid] =  vlan_alias;
 
     return true;
 }
@@ -4540,7 +4506,7 @@ bool PortsOrch::removeVlan(Port vlan)
     SWSS_LOG_NOTICE("Remove VLAN %s vid:%hu", vlan.m_alias.c_str(),
             vlan.m_vlan_info.vlan_id);
 
-    portOidToName.erase(vlan.m_vlan_info.vlan_oid);
+    saiOidToAlias.erase(vlan.m_vlan_info.vlan_oid);
     m_portList.erase(vlan.m_alias);
     m_port_ref_count.erase(vlan.m_alias);
 
@@ -4934,6 +4900,10 @@ bool PortsOrch::addLag(string lag_alias, uint32_t spa_id, int32_t switch_id)
     auto lagport = m_portList.find(lag_alias);
     if (lagport != m_portList.end())
     {
+        /* The deletion of bridgeport attached to the lag may still be 
+         * pending due to fdb entries still present on the lag. Wait
+         * until the cleanup is done.
+         */
         if (m_portList[lag_alias].m_bridge_port_id != SAI_NULL_OBJECT_ID)
         {
             return false;
@@ -4992,7 +4962,7 @@ bool PortsOrch::addLag(string lag_alias, uint32_t spa_id, int32_t switch_id)
     lag.m_members = set<string>();
     m_portList[lag_alias] = lag;
     m_port_ref_count[lag_alias] = 0;
-    portOidToName[lag_id] = lag_alias;
+    saiOidToAlias[lag_id] = lag_alias;
 
     PortUpdate update = { lag, true };
     notify(SUBJECT_TYPE_PORT_CHANGE, static_cast<void *>(&update));
@@ -5065,7 +5035,7 @@ bool PortsOrch::removeLag(Port lag)
 
     SWSS_LOG_NOTICE("Remove LAG %s lid:%" PRIx64, lag.m_alias.c_str(), lag.m_lag_id);
 
-    portOidToName.erase(lag.m_lag_id);
+    saiOidToAlias.erase(lag.m_lag_id);
     m_portList.erase(lag.m_alias);
     m_port_ref_count.erase(lag.m_alias);
 

orchagent/portsorch.h

@@ -232,7 +232,7 @@ class PortsOrch : public Orch, public Subject
      * retrieval of Port/VLAN from object ID for events
      * coming from SAI
      */
-    unordered_map<sai_object_id_t, string> portOidToName;
+    unordered_map<sai_object_id_t, string> saiOidToAlias;
     unordered_map<sai_object_id_t, int> m_portOidToIndex;
     map<string, uint32_t> m_port_ref_count;
     unordered_set<string> m_pendingPortSet;