[portsorch] fix errors when moving port from one lag to another. (#1797)

In scenario that is executed in sonic-mgmt in test_po_update.py a portchannel member is deleted from one portchannel and added to another portchannel.
It is possible that requests from teamsynd will arrive in different order
This reordering happens because teamsyncd has single event handler/selectable TeamSync::TeamPortSync::onChange() per team device so when two of them are ready it is swss::Select implementation detail in which order they are going to be returned.
This is a fundamental issue of Producer/ConsumerStateTable, thus orchagent must be aware of this and treat it as normal situation and figure out the right order and not crash or print an errors.

- What I did
Check if port is already a lag member beforehand.
Added an UT to cover this scenario, this UT verifies that SAI API is not called in this case.
Refactored portsorch_ut.cpp by moving out Orchs creation/deletion into SetUp()/TearDown()

- Why I did it
To fix errors in log.

- How I verified it
Ran test_po_update.py test.

Signed-off-by: Stepan Blyschak stepanb@nvidia.com

orchagent/portsorch.cpp

@@ -266,6 +266,12 @@ static char* hostif_vlan_tag[] = {
  [SAI_HOSTIF_VLAN_TAG_KEEP] = "SAI_HOSTIF_VLAN_TAG_KEEP",
  [SAI_HOSTIF_VLAN_TAG_ORIGINAL] = "SAI_HOSTIF_VLAN_TAG_ORIGINAL"
 };
+
+static bool isValidPortTypeForLagMember(const Port& port)
+{
+ return (port.m_type == Port::Type::PHY || port.m_type == Port::Type::SYSTEM);
+}
+
 /*
  * Initialize PortsOrch
  * 0) If Gearbox is enabled, then initialize the external PHYs as defined in
@@ -1758,7 +1764,7 @@ void PortsOrch::getPortSupportedSpeeds(const std::string& alias, sai_object_id_t
  }
 
  // if our guess was wrong, retry with the correct value
- speeds.resize(attr.value.u32list.count); 
+ speeds.resize(attr.value.u32list.count);
  }
 
  if (status == SAI_STATUS_SUCCESS)
@@ -2243,7 +2249,7 @@ sai_status_t PortsOrch::removePort(sai_object_id_t port_id)
 
  Port port;
 
- /* 
+ /*
  * Make sure to bring down admin state.
  * SET would have replaced with DEL
  */
@@ -2812,7 +2818,7 @@ void PortsOrch::doPortTask(Consumer &consumer)
  it = consumer.m_toSync.erase(it);
  continue;
  }
- 
+
  an = autoneg_mode_map[an_str];
  if (an != p.m_autoneg)
  {
@@ -2878,7 +2884,7 @@ void PortsOrch::doPortTask(Consumer &consumer)
  it++;
  continue;
  }
- 
+
  SWSS_LOG_NOTICE("Set port %s speed from %u to %u", alias.c_str(), p.m_speed, speed);
  p.m_speed = speed;
  m_portList[alias] = p;
@@ -2919,7 +2925,7 @@ void PortsOrch::doPortTask(Consumer &consumer)
  auto ori_adv_speeds = swss::join(',', p.m_adv_speeds.begin(), p.m_adv_speeds.end());
  if (!setPortAdvSpeeds(p.m_port_id, adv_speeds))
  {
- 
+
  SWSS_LOG_ERROR("Failed to set port %s advertised speed from %s to %s", alias.c_str(),
  ori_adv_speeds.c_str(),
  adv_speeds_str.c_str());
@@ -3713,6 +3719,15 @@ void PortsOrch::doLagMemberTask(Consumer &consumer)
  continue;
  }
 
+ /* Fail if a port type is not a valid type for being a LAG member port.
+ * Erase invalid entry, no need to retry in this case. */
+ if (!isValidPortTypeForLagMember(port))
+ {
+ SWSS_LOG_ERROR("LAG member port has to be of type PHY or SYSTEM");
+ it = consumer.m_toSync.erase(it);
+ continue;
+ }
+
  if (table_name == CHASSIS_APP_LAG_MEMBER_TABLE_NAME)
  {
  int32_t lag_switch_id = lag.m_system_lag_info.switch_id;
@@ -3746,8 +3761,12 @@ void PortsOrch::doLagMemberTask(Consumer &consumer)
 
  if (lag.m_members.find(port_alias) == lag.m_members.end())
  {
- /* Assert the port doesn't belong to any LAG already */
- assert(!port.m_lag_id && !port.m_lag_member_id);
+ if (port.m_lag_member_id != SAI_NULL_OBJECT_ID)
+ {
+ SWSS_LOG_INFO("Port %s is already a LAG member", port.m_alias.c_str());
+ it++;
+ continue;
+ }
 
  if (!addLagMember(lag, port, (status == "enabled")))
  {
@@ -5567,7 +5586,7 @@ void PortsOrch::getPortSerdesVal(const std::string& val_str,
  }
 }
 
-bool PortsOrch::getPortAdvSpeedsVal(const std::string &val_str, 
+bool PortsOrch::getPortAdvSpeedsVal(const std::string &val_str,
  std::vector<uint32_t> &speed_values)
 {
  SWSS_LOG_ENTER();
@@ -5581,7 +5600,7 @@ bool PortsOrch::getPortAdvSpeedsVal(const std::string &val_str,
  std::string speed_str;
  std::istringstream iss(val_str);
 
- try 
+ try
  {
  while (std::getline(iss, speed_str, ','))
  {
@@ -5598,31 +5617,31 @@ bool PortsOrch::getPortAdvSpeedsVal(const std::string &val_str,
  return true;
 }
 
-bool PortsOrch::getPortInterfaceTypeVal(const std::string &s, 
+bool PortsOrch::getPortInterfaceTypeVal(const std::string &s,
  sai_port_interface_type_t &interface_type)
 {
  SWSS_LOG_ENTER();
 
  auto iter = interface_type_map_for_an.find(s);
- if (iter != interface_type_map_for_an.end()) 
+ if (iter != interface_type_map_for_an.end())
  {
  interface_type = interface_type_map_for_an[s];
  return true;
  }
- else 
+ else
  {
  const std::string &validInterfaceTypes = getValidInterfaceTypes();
- SWSS_LOG_ERROR("Failed to parse interface_type value %s, valid interface type includes: %s", 
+ SWSS_LOG_ERROR("Failed to parse interface_type value %s, valid interface type includes: %s",
  s.c_str(), validInterfaceTypes.c_str());
  return false;
  }
 }
 
-bool PortsOrch::getPortAdvInterfaceTypesVal(const std::string &val_str, 
+bool PortsOrch::getPortAdvInterfaceTypesVal(const std::string &val_str,
  std::vector<uint32_t> &type_values)
 {
  SWSS_LOG_ENTER();
- if (val_str == "all") 
+ if (val_str == "all")
  {
  return true;
  }
@@ -5637,7 +5656,7 @@ bool PortsOrch::getPortAdvInterfaceTypesVal(const std::string &val_str,
  valid = getPortInterfaceTypeVal(type_str, interface_type);
  if (!valid) {
  const std::string &validInterfaceTypes = getValidInterfaceTypes();
- SWSS_LOG_ERROR("Failed to parse adv_interface_types value %s, valid interface type includes: %s", 
+ SWSS_LOG_ERROR("Failed to parse adv_interface_types value %s, valid interface type includes: %s",
  val_str.c_str(), validInterfaceTypes.c_str());
  return false;
  }