From f8f676a4805cb76a815688550d9d2ec1467bec49 Mon Sep 17 00:00:00 2001 From: saksarav Date: Mon, 20 Jul 2026 11:14:54 -0400 Subject: [PATCH 1/2] teamsyncd: fix VoQ system LAG ID regression after teamdctl gate (#27245) Signed-off-by: saksarav --- teamsyncd/teamsync.cpp | 47 ++++++++++++++++-------------- tests/mock_tests/teamsync_ut.cpp | 50 +++++++++++++++++++++++++++++++- 2 files changed, 74 insertions(+), 23 deletions(-) diff --git a/teamsyncd/teamsync.cpp b/teamsyncd/teamsync.cpp index 8caf094dbe..d0e6e9795f 100644 --- a/teamsyncd/teamsync.cpp +++ b/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,7 +135,6 @@ 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 fvVector; FieldValueTuple a("admin_status", admin_state ? "up" : "down"); FieldValueTuple o("oper_status", oper_state ? "up" : "down"); @@ -145,10 +142,6 @@ void TeamSync::addLag(const string &lagName, int ifindex, bool admin_state, 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(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,14 +203,16 @@ 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 */ @@ -218,9 +220,10 @@ void TeamSync::removeLag(const string &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); diff --git a/tests/mock_tests/teamsync_ut.cpp b/tests/mock_tests/teamsync_ut.cpp index cec81ae3c2..db576ef76b 100644 --- a/tests/mock_tests/teamsync_ut.cpp +++ b/tests/mock_tests/teamsync_ut.cpp @@ -4,6 +4,7 @@ #include #include #include +#include "schema.h" #include "teamsync.h" #include "mock_table.h" @@ -207,13 +208,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 +271,51 @@ 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 appKeys; + appLagTable.getKeys(appKeys); + EXPECT_EQ(appKeys.size(), 1u); + EXPECT_EQ(appKeys[0], "testLag"); + + swss::Table stateLagTable(&stateDb, STATE_LAG_TABLE_NAME); + std::vector 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 keys; + appLagTable.getKeys(keys); + EXPECT_TRUE(keys.empty()); + } } From 902ed55a5205b82aa01cb1eec0f99957a1de8bc5 Mon Sep 17 00:00:00 2001 From: saksarav Date: Thu, 23 Jul 2026 10:41:41 -0400 Subject: [PATCH 2/2] Add Unit test to cover the code coverage gap Signed-off-by: saksarav --- tests/mock_tests/teamsync_ut.cpp | 47 ++++++++++++++++++++++++++++++++ 1 file changed, 47 insertions(+) diff --git a/tests/mock_tests/teamsync_ut.cpp b/tests/mock_tests/teamsync_ut.cpp index db576ef76b..82d895106c 100644 --- a/tests/mock_tests/teamsync_ut.cpp +++ b/tests/mock_tests/teamsync_ut.cpp @@ -5,7 +5,11 @@ #include #include #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; @@ -318,4 +322,47 @@ namespace teamsync_test 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 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 memberKeys; + appLagMemberTable.getKeys(memberKeys); + EXPECT_TRUE(memberKeys.empty()); + + swss::Table appLagTable(&db, APP_LAG_TABLE_NAME); + std::vector lagKeys; + appLagTable.getKeys(lagKeys); + EXPECT_TRUE(lagKeys.empty()); + + swss::Table stateLagTable(&stateDb, STATE_LAG_TABLE_NAME); + std::vector 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); + } }