From 29dc62c0840913992541bde83de1bed70b24a63e Mon Sep 17 00:00:00 2001 From: Vasant Patil <36455926+vasant17@users.noreply.github.com> Date: Thu, 19 Mar 2020 19:52:40 -0700 Subject: [PATCH] [DPB:orchagent:portsorch] Use SAI REDIS return code to track dependency on port (#1219) What I did While deleting a port, if SAI call return OBJECT_IN_USE error code, we will continue to wait/loop in orchagent to delete the port. Once all the dependencies are removed, we will go-ahead remove the port. Note that dependency tracking itself is done in SAI REDIS layer. Also note that identifying dependencies and removing the dependencies is done in config mgmt layer. Why I did it As part Dynamic Port Breakout feature, we need to delete and re-create ports. During deletion of a port, we need to ensure that all dependencies are also removed. For example, port being part of ACL, VLAN, QoS, etc. Initial approach was to track these dependencies in orchagent itself. But after discussion with Microsoft team, we decided that we can use the dependency check that is already being done at the SAI REDIS layer. So, here we are going to check the return error code of SAI call to deduce if the port has any dependency. How I verified it Tested Dynamic Port Breakout code with port in VLAN and ACL tables. Verified from syslog that we SAI redis call to remove/delete the port succeeds when the dependencies are removed. Co-authored-by: Vasant Patil --- orchagent/port.h | 22 +----------- orchagent/portsorch.cpp | 76 ++++++++++++++++++++++------------------- orchagent/portsorch.h | 2 +- 3 files changed, 42 insertions(+), 58 deletions(-) diff --git a/orchagent/port.h b/orchagent/port.h index 9249c27654..963ee5688b 100644 --- a/orchagent/port.h +++ b/orchagent/port.h @@ -49,14 +49,6 @@ class Port UNKNOWN } ; - enum Dependency { - ACL_DEP, - FDB_DEP, - INTF_DEP, - LAG_DEP, - VLAN_DEP - }; - Port() {}; Port(std::string alias, Type type) : m_alias(alias), m_type(type) {}; @@ -84,6 +76,7 @@ class Port std::string m_learn_mode = "hardware"; bool m_autoneg = false; bool m_admin_state_up = false; + bool m_init = false; sai_object_id_t m_port_id = 0; sai_port_fec_mode_t m_fec_mode = SAI_PORT_FEC_MODE_NONE; VlanInfo m_vlan_info; @@ -120,19 +113,6 @@ class Port std::unordered_set m_ingress_acl_tables_uset; std::unordered_set m_egress_acl_tables_uset; - - inline void set_dependency(Dependency dep) - { - m_dependency_bitmap |= (1 << dep); - } - inline void clear_dependency(Dependency dep) - { - m_dependency_bitmap &= ~(1 << dep); - } - inline bool has_dependency() - { - return (m_dependency_bitmap != 0); - } }; } diff --git a/orchagent/portsorch.cpp b/orchagent/portsorch.cpp index a0f0c322de..4eadcf34f9 100755 --- a/orchagent/portsorch.cpp +++ b/orchagent/portsorch.cpp @@ -982,7 +982,6 @@ bool PortsOrch::unbindRemoveAclTableGroup(sai_object_id_t port_oid, return true; } - port.clear_dependency(Port::ACL_DEP); SWSS_LOG_NOTICE("Removing port OID %" PRIx64" ACL table group ID", port_oid); // Unbind ACL group @@ -1094,8 +1093,6 @@ bool PortsOrch::createBindAclTableGroup(sai_object_id_t port_oid, return false; } - port.set_dependency(Port::ACL_DEP); - SWSS_LOG_NOTICE("Create %s ACL table group and bind port %s to it", ingress ? "ingress" : "egress", port.m_alias.c_str()); } @@ -1566,30 +1563,20 @@ bool PortsOrch::addPort(const set &lane_set, uint32_t speed, int an, string return true; } -bool PortsOrch::removePort(sai_object_id_t port_id) +sai_status_t PortsOrch::removePort(sai_object_id_t port_id) { SWSS_LOG_ENTER(); - Port p; - if (getPort(port_id, p)) - { - PortUpdate update = {p, false }; - notify(SUBJECT_TYPE_PORT_CHANGE, static_cast(&update)); - } - sai_status_t status = sai_port_api->remove_port(port_id); if (status != SAI_STATUS_SUCCESS) { - SWSS_LOG_ERROR("Failed to remove port %" PRIx64 ", rv:%d", port_id, status); - return false; + return status; } - removeAclTableGroup(p); - m_portCount--; SWSS_LOG_NOTICE("Remove port %" PRIx64, port_id); - return true; + return status; } string PortsOrch::getQueueWatermarkFlexCounterTableKey(string key) @@ -1646,6 +1633,8 @@ bool PortsOrch::initPort(const string &alias, const set &lane_set) PortUpdate update = { p, true }; notify(SUBJECT_TYPE_PORT_CHANGE, static_cast(&update)); + m_portList[alias].m_init = true; + SWSS_LOG_NOTICE("Initialized port %s", alias.c_str()); } else @@ -1678,6 +1667,7 @@ void PortsOrch::deInitPort(string alias, sai_object_id_t port_id) RedisClient redisClient(m_counter_db.get()); redisClient.hdel(COUNTERS_PORT_NAME_MAP, alias); + m_portList[alias].m_init = false; SWSS_LOG_NOTICE("De-Initialized port %s", alias.c_str()); } @@ -1943,7 +1933,7 @@ void PortsOrch::doPortTask(Consumer &consumer) { if (m_lanesAliasSpeedMap.find(it->first) == m_lanesAliasSpeedMap.end()) { - if (!removePort(it->second)) + if (SAI_STATUS_SUCCESS != removePort(it->second)) { throw runtime_error("PortsOrch initialization failure."); } @@ -2264,26 +2254,47 @@ void PortsOrch::doPortTask(Consumer &consumer) SWSS_LOG_NOTICE("Deleting Port %s", alias.c_str()); auto port_id = m_portList[alias].m_port_id; auto hif_id = m_portList[alias].m_hif_id; + auto bridge_port_oid = m_portList[alias].m_bridge_port_id; - if (m_portList[alias].has_dependency()) + if (bridge_port_oid != SAI_NULL_OBJECT_ID) { - // Port has one or more dependencies, cannot remove - SWSS_LOG_WARN("Cannot to remove port because of dependency"); + // Bridge port OID is set on a port as long as + // port is part of at-least one VLAN. + // Ideally this should be tracked by SAI redis. + // Until then, let this snippet be here. + SWSS_LOG_WARN("Cannot remove port as bridge port OID is present %lx", bridge_port_oid); + it++; continue; - } - - deInitPort(alias, port_id); + } - SWSS_LOG_NOTICE("Removing hostif %lx for Port %s", hif_id, alias.c_str()); - sai_status_t status = sai_hostif_api->remove_hostif(hif_id); - if (status != SAI_STATUS_SUCCESS) + if (m_portList[alias].m_init) { - throw runtime_error("Remove hostif for the port failed"); + deInitPort(alias, port_id); + SWSS_LOG_NOTICE("Removing hostif %lx for Port %s", hif_id, alias.c_str()); + sai_status_t status = sai_hostif_api->remove_hostif(hif_id); + if (status != SAI_STATUS_SUCCESS) + { + throw runtime_error("Remove hostif for the port failed"); + } + + Port p; + if (getPort(port_id, p)) + { + PortUpdate update = {p, false }; + notify(SUBJECT_TYPE_PORT_CHANGE, static_cast(&update)); + } } - if (!removePort(port_id)) + sai_status_t status = removePort(port_id); + if (SAI_STATUS_SUCCESS != status) { - throw runtime_error("Delete port failed"); + if (SAI_STATUS_OBJECT_IN_USE != status) + { + throw runtime_error("Delete port failed"); + } + SWSS_LOG_WARN("Failed to remove port %" PRIx64 ", as the object is in use", port_id); + it++; + continue; } removePortFromLanesMap(alias); removePortFromPortListMap(port_id); @@ -2616,13 +2627,6 @@ void PortsOrch::doLagTask(Consumer &consumer) continue; } - if (m_portList[alias].has_dependency()) - { - // LAG has one or more dependencies, cannot remove - SWSS_LOG_WARN("Cannot to remove LAG because of dependency"); - continue; - } - if (removeLag(lag)) it = consumer.m_toSync.erase(it); else diff --git a/orchagent/portsorch.h b/orchagent/portsorch.h index d0b977e22b..34d7dfd8bf 100755 --- a/orchagent/portsorch.h +++ b/orchagent/portsorch.h @@ -194,7 +194,7 @@ class PortsOrch : public Orch, public Subject void getLagMember(Port &lag, vector &portv); bool addPort(const set &lane_set, uint32_t speed, int an=0, string fec=""); - bool removePort(sai_object_id_t port_id); + sai_status_t removePort(sai_object_id_t port_id); bool initPort(const string &alias, const set &lane_set); void deInitPort(string alias, sai_object_id_t port_id);