From 9700711188a38181866cf01f6d8e69ee6f7d7202 Mon Sep 17 00:00:00 2001 From: Vivek Reddy Karri Date: Wed, 31 Aug 2022 20:46:57 +0000 Subject: [PATCH 1/4] Wait until all the attribs are recieved Signed-off-by: Vivek Reddy Karri --- orchagent/portsorch.cpp | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/orchagent/portsorch.cpp b/orchagent/portsorch.cpp index 0d4177c6d5..28d28cc30f 100755 --- a/orchagent/portsorch.cpp +++ b/orchagent/portsorch.cpp @@ -2615,6 +2615,15 @@ bool PortsOrch::addPort(const set &lane_set, uint32_t speed, int an, string { SWSS_LOG_ENTER(); + if (!speed || lane_set.empty()) + { + /* + speed and lane list are mandatory attributes for the initial create_port call + This check is required because the incoming notifs may not be atomic + */ + return true; + } + vector lanes(lane_set.begin(), lane_set.end()); sai_attribute_t attr; From eb7abf6d9f94082c06147d210381868097a88687 Mon Sep 17 00:00:00 2001 From: Vivek Reddy Karri Date: Tue, 6 Sep 2022 09:18:57 +0000 Subject: [PATCH 2/4] fixed the del after set race in portmgrd Signed-off-by: Vivek Reddy Karri --- cfgmgr/portmgr.cpp | 60 ++++++++++++++++++++++++++++++++++++---------- cfgmgr/portmgr.h | 1 + tests/port_dpb.py | 4 ++++ 3 files changed, 52 insertions(+), 13 deletions(-) diff --git a/cfgmgr/portmgr.cpp b/cfgmgr/portmgr.cpp index 38c0418a7a..40b2611205 100644 --- a/cfgmgr/portmgr.cpp +++ b/cfgmgr/portmgr.cpp @@ -23,27 +23,55 @@ PortMgr::PortMgr(DBConnector *cfgDb, DBConnector *appDb, DBConnector *stateDb, c bool PortMgr::setPortMtu(const string &alias, const string &mtu) { stringstream cmd; - string res; + string res, cmd_str; // ip link set dev mtu cmd << IP_CMD << " link set dev " << shellquote(alias) << " mtu " << shellquote(mtu); - EXEC_WITH_ERROR_THROW(cmd.str(), res); - - // Set the port MTU in application database to update both - // the port MTU and possibly the port based router interface MTU - return writeConfigToAppDb(alias, "mtu", mtu); + cmd_str = cmd.str(); + int ret = swss::exec(cmd_str, res); + if (!ret) + { + // Set the port MTU in application database to update both + // the port MTU and possibly the port based router interface MTU + return writeConfigToAppDb(alias, "mtu", mtu); + } + else if (!isPortStateOk(alias)) + { + // Can happen when a DEL notification is sent by portmgrd followed by a pending mtu SET notification + SWSS_LOG_WARN("Setting mtu to alias:%s netdev failed with cmd:%s, rc:%d, error:%s", alias.c_str(), cmd_str.c_str(), ret, res.c_str()); + return false; + } + else + { + throw runtime_error(cmd_str + " : " + res); + } + return true; } bool PortMgr::setPortAdminStatus(const string &alias, const bool up) { stringstream cmd; - string res; + string res, cmd_str; // ip link set dev [up|down] cmd << IP_CMD << " link set dev " << shellquote(alias) << (up ? " up" : " down"); - EXEC_WITH_ERROR_THROW(cmd.str(), res); - - return writeConfigToAppDb(alias, "admin_status", (up ? "up" : "down")); + cmd_str = cmd.str(); + int ret = swss::exec(cmd_str, res); + if (!ret) + { + return writeConfigToAppDb(alias, "admin_status", (up ? "up" : "down")); + } + else if (!isPortStateOk(alias)) + { + // Can happen when a DEL notification is sent by portmgrd followed by a pending admin_status SET notification + SWSS_LOG_WARN("Setting admin_status to alias:%s netdev failed with cmd%s, rc:%d, error:%s", alias.c_str(), cmd_str.c_str(), ret, res.c_str()); + return false; + } + else + { + throw runtime_error(cmd_str + " : " + res); + } + return true; } bool PortMgr::isPortStateOk(const string &alias) @@ -124,10 +152,9 @@ void PortMgr::doTask(Consumer &consumer) } } - for (auto &entry : field_values) + if (field_values.size()) { - writeConfigToAppDb(alias, fvField(entry), fvValue(entry)); - SWSS_LOG_NOTICE("Configure %s %s to %s", alias.c_str(), fvField(entry).c_str(), fvValue(entry).c_str()); + writeConfigToAppDb(alias, field_values); } if (!portOk) @@ -136,6 +163,7 @@ void PortMgr::doTask(Consumer &consumer) writeConfigToAppDb(alias, "mtu", mtu); writeConfigToAppDb(alias, "admin_status", admin_status); + /* Retry setting these params after the netdev is created */ field_values.clear(); field_values.emplace_back("mtu", mtu); field_values.emplace_back("admin_status", admin_status); @@ -176,3 +204,9 @@ bool PortMgr::writeConfigToAppDb(const std::string &alias, const std::string &fi return true; } + +bool PortMgr::writeConfigToAppDb(const std::string &alias, std::vector &field_values) +{ + m_appPortTable.set(alias, field_values); + return true; +} diff --git a/cfgmgr/portmgr.h b/cfgmgr/portmgr.h index dde346bfe1..683aacc488 100644 --- a/cfgmgr/portmgr.h +++ b/cfgmgr/portmgr.h @@ -30,6 +30,7 @@ class PortMgr : public Orch void doTask(Consumer &consumer); bool writeConfigToAppDb(const std::string &alias, const std::string &field, const std::string &value); + bool writeConfigToAppDb(const std::string &alias, std::vector &field_values); bool setPortMtu(const std::string &alias, const std::string &mtu); bool setPortAdminStatus(const std::string &alias, const bool up); bool isPortStateOk(const std::string &alias); diff --git a/tests/port_dpb.py b/tests/port_dpb.py index f17a9c48eb..1891177bb1 100644 --- a/tests/port_dpb.py +++ b/tests/port_dpb.py @@ -242,8 +242,10 @@ def breakin(self, dvs, port_names): for cp in child_ports: assert(cp.exists_in_config_db() == False) + time.sleep(1) for cp in child_ports: assert(cp.exists_in_app_db() == False) + time.sleep(1) for cp in child_ports: assert(cp.exists_in_asic_db() == False) #print "Verified child ports are deleted from all DBs" @@ -275,9 +277,11 @@ def create_child_ports(self, dvs, p, num_child_ports): assert(cp.exists_in_config_db() == True) cp.verify_config_db() #print "Config DB verification passed" + time.sleep(1) for cp in child_ports: assert(cp.exists_in_app_db() == True) cp.verify_app_db() + time.sleep(1) #print "APP DB verification passed" for cp in child_ports: assert(cp.exists_in_asic_db() == True) From e9c75e1f296b7ad8a680179dfb928edbc067cbe9 Mon Sep 17 00:00:00 2001 From: Vivek Reddy Karri Date: Wed, 7 Sep 2022 05:26:46 +0000 Subject: [PATCH 3/4] Removed sleep intervals in the test Signed-off-by: Vivek Reddy Karri --- tests/port_dpb.py | 4 ---- 1 file changed, 4 deletions(-) diff --git a/tests/port_dpb.py b/tests/port_dpb.py index 1891177bb1..f17a9c48eb 100644 --- a/tests/port_dpb.py +++ b/tests/port_dpb.py @@ -242,10 +242,8 @@ def breakin(self, dvs, port_names): for cp in child_ports: assert(cp.exists_in_config_db() == False) - time.sleep(1) for cp in child_ports: assert(cp.exists_in_app_db() == False) - time.sleep(1) for cp in child_ports: assert(cp.exists_in_asic_db() == False) #print "Verified child ports are deleted from all DBs" @@ -277,11 +275,9 @@ def create_child_ports(self, dvs, p, num_child_ports): assert(cp.exists_in_config_db() == True) cp.verify_config_db() #print "Config DB verification passed" - time.sleep(1) for cp in child_ports: assert(cp.exists_in_app_db() == True) cp.verify_app_db() - time.sleep(1) #print "APP DB verification passed" for cp in child_ports: assert(cp.exists_in_asic_db() == True) From 6ea8be8cdb530f42f842174607290344abb807c2 Mon Sep 17 00:00:00 2001 From: Vivek Reddy Karri Date: Wed, 7 Sep 2022 05:29:58 +0000 Subject: [PATCH 4/4] comment updated Signed-off-by: Vivek Reddy Karri --- cfgmgr/portmgr.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cfgmgr/portmgr.cpp b/cfgmgr/portmgr.cpp index 40b2611205..5134b31861 100644 --- a/cfgmgr/portmgr.cpp +++ b/cfgmgr/portmgr.cpp @@ -37,7 +37,7 @@ bool PortMgr::setPortMtu(const string &alias, const string &mtu) } else if (!isPortStateOk(alias)) { - // Can happen when a DEL notification is sent by portmgrd followed by a pending mtu SET notification + // Can happen when a DEL notification is sent by portmgrd immediately followed by a new SET notif SWSS_LOG_WARN("Setting mtu to alias:%s netdev failed with cmd:%s, rc:%d, error:%s", alias.c_str(), cmd_str.c_str(), ret, res.c_str()); return false; } @@ -63,7 +63,7 @@ bool PortMgr::setPortAdminStatus(const string &alias, const bool up) } else if (!isPortStateOk(alias)) { - // Can happen when a DEL notification is sent by portmgrd followed by a pending admin_status SET notification + // Can happen when a DEL notification is sent by portmgrd immediately followed by a new SET notification SWSS_LOG_WARN("Setting admin_status to alias:%s netdev failed with cmd%s, rc:%d, error:%s", alias.c_str(), cmd_str.c_str(), ret, res.c_str()); return false; }