Skip to content

Commit

Permalink
[DPB:orchagent:portsorch] Use SAI REDIS return code to track dependen…
Browse files Browse the repository at this point in the history 
…cy 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 <[email protected]>
  • Loading branch information
@vasant17
vasant17 and Vasant Patil authored Mar 20, 2020
1 parent a1ff711 commit 29dc62c
Show file tree
Hide file tree
Showing 3 changed files with 42 additions and 58 deletions.
22 changes: 1 addition & 21 deletions orchagent/port.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {};
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -120,19 +113,6 @@ class Port

std::unordered_set<sai_object_id_t> m_ingress_acl_tables_uset;
std::unordered_set<sai_object_id_t> 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);
}
};

}
Expand Down
76 changes: 40 additions & 36 deletions orchagent/portsorch.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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());
}
Expand Down Expand Up @@ -1566,30 +1563,20 @@ bool PortsOrch::addPort(const set<int> &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<void *>(&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)
Expand Down Expand Up @@ -1646,6 +1633,8 @@ bool PortsOrch::initPort(const string &alias, const set<int> &lane_set)
PortUpdate update = { p, true };
notify(SUBJECT_TYPE_PORT_CHANGE, static_cast<void *>(&update));

m_portList[alias].m_init = true;

SWSS_LOG_NOTICE("Initialized port %s", alias.c_str());
}
else
Expand Down Expand Up @@ -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());
}

Expand Down Expand Up @@ -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.");
}
Expand Down Expand Up @@ -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<void *>(&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);
Expand Down Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion orchagent/portsorch.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,7 @@ class PortsOrch : public Orch, public Subject
void getLagMember(Port &lag, vector<Port> &portv);

bool addPort(const set<int> &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<int> &lane_set);
void deInitPort(string alias, sai_object_id_t port_id);

Expand Down

0 comments on commit 29dc62c

Please sign in to comment.