[teamd]: Clean teamd process if LAG creation fails #2888

theasianpianist · 2023-08-24T23:18:33Z

What I did

During LAG creation, if the teamd process fails for any reason, cleanup any leftover teamd processes for the LAG alias
Call kill directly in removeLag instead of using teamd -k

Why I did it

If the teamd process times out for any reason, subsequent attempts to create a LAG may fail if there is an orphaned process left running
As detailed in Removal of LAG with no members takes around 1 sec on 202012 sonic-buildimage#8071, team -k may block for some time.

How I verified it

Details if related

Signed-off-by: Lawrence Lee <[email protected]>

prsunny · 2023-08-24T23:22:55Z

Thanks for fix, can you please add a unit test to cover?

Signed-off-by: Lawrence Lee <[email protected]>

MaxXiao12 · 2023-08-25T21:27:32Z

cfgmgr/teammgr.cpp

    cmd << TEAMD_CMD << " -k -t " << shellquote(alias);
    EXEC_WITH_ERROR_THROW(cmd.str(), res);


This could be causing issues. IIUC if the process actually wasn't running, then teamd -k -t PortChannel.1 would fail. So in the case where addLag() fails, but no teamd ... running, then teamd -k -t PortChannel. would fail, as a result an exception will be thrown, could you check?

MaxXiao12 · 2023-08-25T21:31:57Z

cfgmgr/teammgr.cpp

+    try
+    {
+        pid = static_cast<pid_t>(std::stoul(res, nullptr, 10));
+        SWSS_LOG_INFO("Read port channel %s pid %d", alias.c_str(), pid);
+    }
+    catch (const std::exception &e)
+    {
+        SWSS_LOG_ERROR("Failed to read port channel %s pid: %s", alias.c_str(), e.what());
+        return false;
+    }
+
+    try
+    {
+        std::stringstream cmd;
+        cmd << "kill -TERM " << pid;
+        EXEC_WITH_ERROR_THROW(cmd.str(), res);
+    }
+    catch (const std::exception &e)
+    {
+        SWSS_LOG_ERROR("Failed to send SIGTERM to port channel %s pid %d: %s", alias.c_str(), pid, e.what());
+        return false;
+    }
+


Do we need to do this? It should be part of the teamd -k -t PortChannel1 IIUC.

Yes, since teamd -k can block for some time as per sonic-net/sonic-buildimage#8071

MaxXiao12 · 2023-08-25T21:35:42Z

cfgmgr/teammgr.cpp

+    try
+    {
+        std::stringstream cmd;
+        cmd << "cat " << shellquote("/var/run/teamd/" + alias + ".pid");
+        EXEC_WITH_ERROR_THROW(cmd.str(), res);
+    }
+    catch (const std::exception &e)
+    {
+        SWSS_LOG_NOTICE("Failed to remove non-existent port channel %s pid...", alias.c_str());
+        return false;
+    }


Thinking about this a bit more, IMO this is internal implementation details of teamd, it's better not assume the location and naming of the file.

Teammgrd already reads this file in cleanTeamProcesses, and I don't think there's another easy way to get the teamd PID.

cleanTeamProcesses has most part of the code. Is it good to reuse the same code by passing pid? if no pid passed, then original flow. Just a thought

Signed-off-by: Lawrence Lee <[email protected]>

MaxXiao12 · 2023-08-28T03:03:01Z

cfgmgr/teammgr.cpp

-    cmd << TEAMD_CMD << " -k -t " << shellquote(alias);
-    EXEC_WITH_ERROR_THROW(cmd.str(), res);
-


FWIW, can I suggest to the following:

bool TeamMgr::removeLag(const string &alias, bool assertSuccess) { ... if (asserSuccess) { EXEC_WITH_ERROR_THROW(cmd.str(), res); } else { exec( cmd.str(), res ); } ... }

In the case when the addLag() fails, removeLag() will be called with assertSuccess=false, otherwise the code path stays the same. IMO this would address the issue while still keeping the addLag() and removeLag() symmetrical... my 2cents.

prsunny · 2023-09-07T22:39:23Z

cfgmgr/teammgr.cpp

+    try
+    {
+        std::stringstream cmd;
+        cmd << "cat " << shellquote("/var/run/teamd/" + alias + ".pid");
+        EXEC_WITH_ERROR_THROW(cmd.str(), res);
+    }
+    catch (const std::exception &e)
+    {
+        SWSS_LOG_NOTICE("Failed to remove non-existent port channel %s pid...", alias.c_str());
+        return false;
+    }


cleanTeamProcesses has most part of the code. Is it good to reuse the same code by passing pid? if no pid passed, then original flow. Just a thought

prsunny · 2023-09-07T22:41:14Z

@judyjoseph , would you take a look at the PR?

* [teamd]: Clean teamd process if LAG creation fails

This reverts commit c7e5f10.

…onic-net#2888)"" This reverts commit 7097cf2.

* Revert "Revert "[teamd]: Clean teamd process if LAG creation fails (#2888)"" This reverts commit 7097cf2. During LAG creation, if the teamd process fails for any reason, cleanup any leftover teamd processes for the LAG alias Call kill directly in removeLag instead of using teamd -k

yxieca · 2023-10-12T22:30:50Z

@theasianpianist can you help create PR for 202205 branch? This change cannot be cherry-picked cleanly.

* [teamd]: Clean teamd process if LAG creation fails

theasianpianist · 2023-10-12T23:09:48Z

@theasianpianist can you help create PR for 202205 branch? This change cannot be cherry-picked cleanly.

#2932 PR here, will monitor the PR checks

…2932) What I did 202205 cherry-pick for [teamd]: Clean teamd process if LAG creation fails #2888 During LAG creation, if the teamd process fails for any reason, cleanup any leftover teamd processes for the LAG alias Call kill directly in removeLag instead of using teamd -k Why I did it If the teamd process times out for any reason, subsequent attempts to create a LAG may fail if there is an orphaned process left running As detailed in Removal of LAG with no members takes around 1 sec on 202012 sonic-buildimage#8071, team -k may block for some time. Signed-off-by: Lawrence Lee <[email protected]>

[teamd]: Clean teamd process if LAG creation fails

9c6877f

Signed-off-by: Lawrence Lee <[email protected]>

theasianpianist requested a review from judyjoseph as a code owner August 24, 2023 23:18

Merge branch 'master' into teammgrd-cleanup

111d91a

theasianpianist requested a review from prsunny as a code owner August 25, 2023 00:43

[tests]: Add teammgrd unit test

706b5e6

Signed-off-by: Lawrence Lee <[email protected]>

theasianpianist force-pushed the teammgrd-cleanup branch from 4a1b951 to 706b5e6 Compare August 25, 2023 01:22

[teammgrd]: Fix unit test setup

4bb1aba

Signed-off-by: Lawrence Lee <[email protected]>

MaxXiao12 reviewed Aug 25, 2023

View reviewed changes

theasianpianist added 4 commits August 26, 2023 03:10

[teammgr]: remove extra spaces from cli command

fb5176a

Signed-off-by: Lawrence Lee <[email protected]>

[teammgr]: Fix removeLag

2072d0c

Signed-off-by: Lawrence Lee <[email protected]>

update gitignore

5c88f99

Signed-off-by: Lawrence Lee <[email protected]>

update test expected commands

fc0582d

Signed-off-by: Lawrence Lee <[email protected]>

MaxXiao12 reviewed Aug 28, 2023

View reviewed changes

theasianpianist added Request for 202205 Branch Request for 202211 Branch Request for 202305 Branch labels Aug 29, 2023

MaxXiao12 approved these changes Aug 31, 2023

View reviewed changes

Merge branch 'master' into teammgrd-cleanup

87225ef

prsunny approved these changes Sep 7, 2023

View reviewed changes

prsunny merged commit 13ef25b into sonic-net:master Sep 13, 2023

StormLiangMS added the Included in 202305 Branch label Sep 21, 2023

StormLiangMS pushed a commit that referenced this pull request Sep 21, 2023

[teamd]: Clean teamd process if LAG creation fails (#2888)

c7e5f10

* [teamd]: Clean teamd process if LAG creation fails

StormLiangMS added a commit that referenced this pull request Sep 22, 2023

Revert "[teamd]: Clean teamd process if LAG creation fails (#2888)"

7097cf2

This reverts commit c7e5f10.

theasianpianist added a commit to theasianpianist/sonic-swss that referenced this pull request Sep 29, 2023

Revert "Revert "[teamd]: Clean teamd process if LAG creation fails (s…

a83737d

…onic-net#2888)"" This reverts commit 7097cf2.

theasianpianist mentioned this pull request Oct 3, 2023

[202305][teamd]: Clean teamd process if LAG creation fails #2918

Merged

yxieca added the Cherry Pick Conflict_202205 label Oct 12, 2023

theasianpianist added a commit to theasianpianist/sonic-swss that referenced this pull request Oct 12, 2023

[teamd]: Clean teamd process if LAG creation fails (sonic-net#2888)

01ff75e

* [teamd]: Clean teamd process if LAG creation fails

theasianpianist mentioned this pull request Oct 12, 2023

[202205][teamd]: Clean teamd process if LAG creation fails (#2888) #2932

Merged

yxieca added Included in 202205 Branch and removed Cherry Pick Conflict_202205 labels Nov 3, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[teamd]: Clean teamd process if LAG creation fails #2888

[teamd]: Clean teamd process if LAG creation fails #2888

theasianpianist commented Aug 24, 2023

prsunny commented Aug 24, 2023

MaxXiao12 Aug 25, 2023

MaxXiao12 Aug 25, 2023

theasianpianist Aug 28, 2023

MaxXiao12 Aug 25, 2023

theasianpianist Aug 30, 2023

prsunny Sep 7, 2023

MaxXiao12 Aug 28, 2023

prsunny Sep 7, 2023

prsunny commented Sep 7, 2023

yxieca commented Oct 12, 2023

theasianpianist commented Oct 12, 2023

		cmd << TEAMD_CMD << " -k -t " << shellquote(alias);
		EXEC_WITH_ERROR_THROW(cmd.str(), res);

[teamd]: Clean teamd process if LAG creation fails #2888

[teamd]: Clean teamd process if LAG creation fails #2888

Conversation

theasianpianist commented Aug 24, 2023

prsunny commented Aug 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prsunny commented Sep 7, 2023

yxieca commented Oct 12, 2023

theasianpianist commented Oct 12, 2023