Fixed set admin_status for deleted subintf due to late notification (#…

…2659) * Fixed set admin_status for deleted subintf due to late notification
sonic-net · Mar 14, 2023 · b44692c · b44692c
1 parent 98a16cf
commit b44692c
Show file tree

Hide file tree

Showing 3 changed files with 40 additions and 8 deletions.
diff --git a/cfgmgr/intfmgr.cpp b/cfgmgr/intfmgr.cpp
@@ -491,13 +491,24 @@ void IntfMgr::updateSubIntfAdminStatus(const string &alias, const string &admin)
 std::string IntfMgr::setHostSubIntfAdminStatus(const string &alias, const string &admin_status, const string &parent_admin_status)
 {
     stringstream cmd;
-    string res;
+    string res, cmd_str;
 
     if (parent_admin_status == "up" || admin_status == "down")
     {
         SWSS_LOG_INFO("subintf %s admin_status: %s", alias.c_str(), admin_status.c_str());
         cmd << IP_CMD " link set " << shellquote(alias) << " " << shellquote(admin_status);
-        EXEC_WITH_ERROR_THROW(cmd.str(), res);
+        cmd_str = cmd.str();
+        int ret = swss::exec(cmd_str, res);
+        if (ret && !isIntfStateOk(alias))
+        {
+            // Can happen when a DEL notification is sent by portmgrd immediately followed by a new SET notification
+            SWSS_LOG_WARN("Setting admin_status to %s netdev failed with cmd:%s, rc:%d, error:%s",
+                          alias.c_str(), cmd_str.c_str(), ret, res.c_str());
+        }
+        else if (ret)
+        {
+            throw runtime_error(cmd_str + " : " + res);
+        }
         return admin_status;
     }
     else

diff --git a/tests/mock_tests/Makefile.am b/tests/mock_tests/Makefile.am
@@ -155,7 +155,7 @@ tests_portsyncd_LDADD = $(LDADD_GTEST) -lnl-genl-3 -lhiredis -lhiredis \
 
 ## intfmgrd unit tests
 
-tests_intfmgrd_SOURCES = intfmgrd/add_ipv6_prefix_ut.cpp \
+tests_intfmgrd_SOURCES = intfmgrd/intfmgr_ut.cpp \
                          $(top_srcdir)/cfgmgr/intfmgr.cpp \
                          $(top_srcdir)/lib/subintf.cpp \
                          $(top_srcdir)/orchagent/orch.cpp \

diff --git a/...ock_tests/intfmgrd/add_ipv6_prefix_ut.cpp → tests/mock_tests/intfmgrd/intfmgr_ut.cpp b/...ock_tests/intfmgrd/add_ipv6_prefix_ut.cpp → tests/mock_tests/intfmgrd/intfmgr_ut.cpp
@@ -1,6 +1,6 @@
 #include "gtest/gtest.h"
 #include <iostream>
-#include <fstream>  
+#include <fstream>
 #include <unistd.h>
 #include <sys/stat.h>
 #include "../mock_table.h"
@@ -20,29 +20,32 @@ int cb(const std::string &cmd, std::string &stdout){
     else if (cmd.find("/sbin/ip -6 address \"add\"") == 0) {
         return Ethernet0IPv6Set ? 0 : 2;
     }
+    else if (cmd == "/sbin/ip link set \"Ethernet64.10\" \"up\""){
+        return 1;
+    }
     else {
         return 0;
     }
     return 0;
 }
 
 // Test Fixture
-namespace add_ipv6_prefix_ut
+namespace intfmgr_ut
 {
     struct IntfMgrTest : public ::testing::Test
     {
         std::shared_ptr<swss::DBConnector> m_config_db;
         std::shared_ptr<swss::DBConnector> m_app_db;
         std::shared_ptr<swss::DBConnector> m_state_db;
         std::vector<std::string> cfg_intf_tables;
-        
+
         virtual void SetUp() override
-        {   
+        {
             testing_db::reset();
             m_config_db = std::make_shared<swss::DBConnector>("CONFIG_DB", 0);
             m_app_db = std::make_shared<swss::DBConnector>("APPL_DB", 0);
             m_state_db = std::make_shared<swss::DBConnector>("STATE_DB", 0);
-            
+
             swss::WarmStart::initialize("intfmgrd", "swss");
 
             std::vector<std::string> tables = {
@@ -106,4 +109,22 @@ namespace add_ipv6_prefix_ut
         }
         ASSERT_EQ(ip_cmd_called, 1);
     }
+
+    //This test except no runtime error when the set admin status command failed
+    //and the subinterface has not ok status (for example not existing subinterface)
+    TEST_F(IntfMgrTest, testSetAdminStatusFailToNotOkSubInt){
+        swss::IntfMgr intfmgr(m_config_db.get(), m_app_db.get(), m_state_db.get(), cfg_intf_tables);
+        intfmgr.setHostSubIntfAdminStatus("Ethernet64.10", "up", "up");
+    }
+
+    //This test except runtime error when the set admin status command failed
+    //and the subinterface has ok status
+    TEST_F(IntfMgrTest, testSetAdminStatusFailToOkSubInt){
+        swss::IntfMgr intfmgr(m_config_db.get(), m_app_db.get(), m_state_db.get(), cfg_intf_tables);
+        /* Set portStateTable */
+        std::vector<swss::FieldValueTuple> values;
+        values.emplace_back("state", "ok");
+        intfmgr.m_statePortTable.set("Ethernet64.10", values, "SET", "");
+        EXPECT_THROW(intfmgr.setHostSubIntfAdminStatus("Ethernet64.10", "up", "up"), std::runtime_error);
+    }
 }