teamsyncd: fix VoQ system LAG ID regression after teamdctl gate (#27245)

teamsyncd/teamsync.cpp

@@ -120,11 +120,9 @@ void TeamSync::onMsg(int nlmsg_type, struct nl_object *obj)
 
     if (nlmsg_type == RTM_DELLINK)
     {
-        if (m_teamSelectables.find(lagName) != m_teamSelectables.end())
-        {
-            /* Remove LAG ports and delete LAG */
-            removeLag(lagName);
-        }
+        /* Always remove APP_LAG (and STATE_LAG if tracked), even when
+         * TeamPortSync was never created because teamdctl failed on add. */
+        removeLag(lagName);
         return;
     }
 
@@ -137,18 +135,13 @@ void TeamSync::onMsg(int nlmsg_type, struct nl_object *obj)
 void TeamSync::addLag(const string &lagName, int ifindex, bool admin_state,
                       bool oper_state, unsigned int mtu)
 {
-    /* Set the LAG */
     std::vector<FieldValueTuple> fvVector;
     FieldValueTuple a("admin_status", admin_state ? "up" : "down");
     FieldValueTuple o("oper_status", oper_state ? "up" : "down");
     FieldValueTuple m("mtu", std::to_string(mtu));
     fvVector.push_back(a);
     fvVector.push_back(o);
     fvVector.push_back(m);
-    m_lagTable.set(lagName, fvVector);
-
-    SWSS_LOG_INFO("Add %s admin_status:%s oper_status:%s, mtu: %d",
-                   lagName.c_str(), admin_state ? "up" : "down", oper_state ? "up" : "down", mtu);
 
     bool lag_update = true;
     /* Return when the team instance has already been tracked */
@@ -165,6 +158,12 @@ void TeamSync::addLag(const string &lagName, int ifindex, bool admin_state,
 
     FieldValueTuple s("state", "ok");
     fvVector.push_back(s);
+
+    /* Publish APP_LAG immediately so orchagent can allocate and free LAG ids on
+     * rapid add/del; RTM_DELLINK always removes APP_LAG even when TeamPortSync
+     * never succeeded.  STATE_LAG is written only after teamd is ready (#3984). */
+    m_lagTable.set(lagName, fvVector);
+
     if (lag_update)
     {
         /* Create the team instance.
@@ -177,16 +176,17 @@ void TeamSync::addLag(const string &lagName, int ifindex, bool admin_state,
          * which would abort the entire netlink processing loop.  When teamd finishes
          * recreating the device the kernel emits a fresh RTM_NEWLINK with the correct
          * new ifindex and addLag() is called again, at which point initialization
-         * succeeds.
-         * STATE_DB is written only after the team instance is successfully created
-         * to prevent dependent services (e.g. intfmgrd) from acting on a LAG that
-         * teamd has not yet finished setting up. */
+         * succeeds. */
         try
         {
             auto sync = make_shared<TeamPortSync>(lagName, ifindex, &m_lagMemberTable);
             m_stateLagTable.set(lagName, fvVector);
             m_teamSelectables[lagName] = sync;
             m_selectablesToAdd.insert(lagName);
+
+            SWSS_LOG_INFO("Add %s admin_status:%s oper_status:%s, mtu: %d",
+                           lagName.c_str(), admin_state ? "up" : "down",
+                           oper_state ? "up" : "down", mtu);
         }
         catch (const system_error& e)
         {
@@ -203,24 +203,27 @@ void TeamSync::addLag(const string &lagName, int ifindex, bool admin_state,
 
 void TeamSync::removeLag(const string &lagName)
 {
-    /* Delete all members */
-    auto selectable = m_teamSelectables[lagName];
-    for (auto it : selectable->m_lagMembers)
+    auto it = m_teamSelectables.find(lagName);
+    if (it != m_teamSelectables.end())
     {
-        m_lagMemberTable.del(lagName + ":" + it.first);
+        for (auto member : it->second->m_lagMembers)
+        {
+            m_lagMemberTable.del(lagName + ":" + member.first);
 
-        SWSS_LOG_INFO("Remove member %s before removing LAG %s",
-                it.first.c_str(), lagName.c_str());
+            SWSS_LOG_INFO("Remove member %s before removing LAG %s",
+                    member.first.c_str(), lagName.c_str());
+        }
     }
 
     /* Delete the LAG */
     m_lagTable.del(lagName);
 
     SWSS_LOG_INFO("Remove LAG %s", lagName.c_str());
 
-    /* Return when the team instance hasn't been tracked before */
-    if (m_teamSelectables.find(lagName) == m_teamSelectables.end())
+    if (it == m_teamSelectables.end())
+    {
         return;
+    }
 
     m_stateLagTable.del(lagName);
 

tests/mock_tests/teamsync_ut.cpp

@@ -4,7 +4,12 @@
 #include <stdexcept>
 #include <team.h>
 #include <teamdctl.h>
+#include "schema.h"
+#define protected public
+#define private public
 #include "teamsync.h"
+#undef protected
+#undef private
 #include "mock_table.h"
 
 static unsigned int (*callback_sleep)(unsigned int seconds) = NULL;
@@ -207,13 +212,13 @@ namespace teamsync_test
         callback_teamdctl_disconnect = cb_teamdctl_disconnect;
     }
 
-    /* Subclass to expose the protected addLag() for unit testing. */
     class TeamSyncUnderTest : public swss::TeamSync
     {
     public:
         TeamSyncUnderTest(swss::DBConnector *db, swss::DBConnector *stateDb, swss::Select *sel)
             : swss::TeamSync(db, stateDb, sel) {}
         using swss::TeamSync::addLag;
+        using swss::TeamSync::removeLag;
     };
 
     struct TeamSyncTest : public ::testing::Test
@@ -270,4 +275,94 @@ namespace teamsync_test
 
         ts.addLag("testLag", 4, true, true, 1500);
     }
+
+    /* When teamd is not ready, APP_LAG is still published for orchagent but
+     * STATE_LAG is deferred until TeamPortSync succeeds. */
+    TEST_F(TeamSyncTest, AddLagTeamdctlFailsNoStateLag)
+    {
+        callback_team_init = cb_team_init;
+        callback_team_change_handler = cb_team_change_handler;
+        callback_teamdctl_connect = cb_teamdctl_connect;
+        callback_sleep = cb_sleep;
+
+        swss::DBConnector db(0, "localhost", 0, 0);
+        swss::DBConnector stateDb(1, "localhost", 0, 0);
+        TeamSyncUnderTest ts(&db, &stateDb, nullptr);
+
+        ts.addLag("testLag", 4, true, true, 1500);
+
+        swss::Table appLagTable(&db, APP_LAG_TABLE_NAME);
+        std::vector<std::string> appKeys;
+        appLagTable.getKeys(appKeys);
+        EXPECT_EQ(appKeys.size(), 1u);
+        EXPECT_EQ(appKeys[0], "testLag");
+
+        swss::Table stateLagTable(&stateDb, STATE_LAG_TABLE_NAME);
+        std::vector<std::string> stateKeys;
+        stateLagTable.getKeys(stateKeys);
+        EXPECT_TRUE(stateKeys.empty());
+    }
+
+    TEST_F(TeamSyncTest, RemoveLagWithoutTeamPortSync)
+    {
+        callback_team_init = cb_team_init;
+        callback_team_change_handler = cb_team_change_handler;
+        callback_teamdctl_connect = cb_teamdctl_connect;
+        callback_sleep = cb_sleep;
+
+        swss::DBConnector db(0, "localhost", 0, 0);
+        swss::DBConnector stateDb(1, "localhost", 0, 0);
+        TeamSyncUnderTest ts(&db, &stateDb, nullptr);
+
+        ts.addLag("testLag", 4, true, true, 1500);
+        ts.removeLag("testLag");
+
+        swss::Table appLagTable(&db, APP_LAG_TABLE_NAME);
+        std::vector<std::string> keys;
+        appLagTable.getKeys(keys);
+        EXPECT_TRUE(keys.empty());
+    }
+
+    /* When TeamPortSync exists with tracked members, removeLag() must delete
+     * each APP_LAG_MEMBER entry before removing the LAG itself. */
+    TEST_F(TeamSyncTest, RemoveLagWithTeamPortSyncMembers)
+    {
+        setTeamSyncSuccessMocks();
+
+        swss::DBConnector db(0, "localhost", 0, 0);
+        swss::DBConnector stateDb(1, "localhost", 0, 0);
+        TeamSyncUnderTest ts(&db, &stateDb, nullptr);
+
+        ts.addLag("testLag", 4, true, true, 1500);
+
+        ASSERT_NE(ts.m_teamSelectables.find("testLag"), ts.m_teamSelectables.end());
+
+        ts.m_teamSelectables["testLag"]->m_lagMembers["Ethernet0"] = true;
+        ts.m_teamSelectables["testLag"]->m_lagMembers["Ethernet4"] = false;
+
+        std::vector<swss::FieldValueTuple> memberFv;
+        memberFv.emplace_back("status", "enabled");
+        ts.m_lagMemberTable.set("testLag:Ethernet0", memberFv);
+        ts.m_lagMemberTable.set("testLag:Ethernet4", memberFv);
+
+        ts.removeLag("testLag");
+
+        swss::Table appLagMemberTable(&db, APP_LAG_MEMBER_TABLE_NAME);
+        std::vector<std::string> memberKeys;
+        appLagMemberTable.getKeys(memberKeys);
+        EXPECT_TRUE(memberKeys.empty());
+
+        swss::Table appLagTable(&db, APP_LAG_TABLE_NAME);
+        std::vector<std::string> lagKeys;
+        appLagTable.getKeys(lagKeys);
+        EXPECT_TRUE(lagKeys.empty());
+
+        swss::Table stateLagTable(&stateDb, STATE_LAG_TABLE_NAME);
+        std::vector<std::string> stateKeys;
+        stateLagTable.getKeys(stateKeys);
+        EXPECT_TRUE(stateKeys.empty());
+
+        EXPECT_EQ(ts.m_teamSelectables.find("testLag"), ts.m_teamSelectables.end());
+        EXPECT_EQ(ts.m_selectablesToRemove.count("testLag"), 1u);
+    }
 }