Teamd :: fix for cleaning up the teamd processes correctly on teamd docker stop by judyjoseph · Pull Request #1159 · sonic-net/sonic-swss

judyjoseph · 2019-12-23T22:53:22Z

What I did

The changes implemented in the final fix are,
(1) In teammgrd: While adding LAG, save the mapping of lag <-> teamd PID
(2) In teammgrd: While removing LAG, remove the mapping of lag <-> teamd PID
(3) Introduce SIGTERM handlers in both teammgrd and teamsyncd

(a) In teammgrd, send the SIGTERM to all the teamd processes. It takes just a second for teamd processes + port channel interfaces to be cleaned up
(b) In teamsyncd, clean up the teamd resources like nl_socket etc.

Why I did it

There were multiple Issues seen, in random on doing a config reload/ config loadminigraph

(i) Teamsyncd segfault
(ii) Teamsyncd getting the NETLINK messages with the older IFINDEXs
(iii) "TeamPortSync: Unable to init team socket" error messages

Root cause

 With config reload command , the docker teamd stop and start is issued. I find that after "docker stop" the port channel interfaces remains in the linux kernel – it is not getting cleaned. There are indeed signal handlers in the teamd processes. But in this case teamd is started as a “daemon” with the “-d” option. The supervisord is not able to send the SIGTERM to the processes started in a daemon mode (http://supervisord.org/subprocess.html?highlight=daemon)

“docker stop” sends two signals

SIGTERM to the ENTRYPOINT process in our case it is supervisord process
After 10 sec send SIGKILL to all processes to stop the container.

So when docker stop is issued, supervisord send SIGTERM to all processes ( teamsyncd/temmgrd gets the signal correctly – but I find none of the teamd processes get the signal either because they are daemonized and running in background OR because teamd is not started directly by supervisord).

The teamd processes are killed later with SIGKILL send as a last resort to stop docker. SIGKILL cannot be handled and hence no cleanup – portchanel interfaces remains in kernel.

How I verified it

Tried the config reload back to back multiple times -- verified that the Portchannel interface are cleaned up in kernel, teamd processes are killed correctly.
When the teamd docker is started again, I don't see random NETLINK create/delete messages with the older Portchannel ifindices. ( which was seen earlier without this fix )

Other possible fixes which I tried out, but decided not to take

The other possible fix options which was tried were,

(1) Use the fork/exec way to spawn the teamd processes

(a) Seeing instabilities in all teamd instances NOT coming up when we do “config reload” back to back.
(b) Even with teamd staying in foreground, those processes were not getting the SIGTERM on “docker stop”. So doesn’t serve the purpose of trying NOT to daemonize the teamd.

(2) In the teammgrd Signal handler directly invoke an existing API teammgrd:removeLag() which
cleans up the teamd processes. This was working fine, but the cleanup was taking longer
duration like around 5sec for cleaning up all the teamd processes, which very high
considering it is a signal handler.

When the teamd docker receives a stop signal, only the processes started by supervisord gets the SIGTERM, so this fix is to propogate the signal to teamd processes via the signal handler in teamsyncd process.

…andler.

madhukar-kamarapu · 2019-12-24T00:14:28Z

@judyjoseph - this change would address bunch of issues we've been seeing in teamsyncd during config-reload and docker-restart. Thanks again.
One minor (log related) comment - can you please look into it.

teamsyncd/teamsync.cpp

pavel-shirshov

please check comments

teamsyncd/teamsync.cpp

judyjoseph · 2019-12-24T00:54:31Z

@judyjoseph - this change would address bunch of issues we've been seeing in teamsyncd during config-reload and docker-restart. Thanks again.
I have no major comments/concerns on this PR. One minor (log related) comment - can you please look into it.

I did testing with config reload and config loadminigraph and seen things are ok. These commands does docker stop and docker start explicitly in a sequence starting with swss docker.

What is the use case of docker restart ? are you referring to "docker restart teamd"? Is this command supported ? Currently I see swss, syncd, teamd tightly bound and think it will go to an inconsistent state if we just restart teamd alone.

madhukar-kamarapu · 2019-12-24T02:15:45Z

@judyjoseph - this change would address bunch of issues we've been seeing in teamsyncd during config-reload and docker-restart. Thanks again.
I have no major comments/concerns on this PR. One minor (log related) comment - can you please look into it.

@judyjoseph - this change would address bunch of issues we've been seeing in teamsyncd during config-reload and docker-restart. Thanks again.
I have no major comments/concerns on this PR. One minor (log related) comment - can you please look into it.

I did testing with config reload and config loadminigraph and seen things are ok. These commands does docker stop and docker start explicitly in a sequence starting with swss docker.

What is the use case of docker restart ? are you referring to "docker restart teamd"? Is this command supported ? Currently I see swss, syncd, teamd tightly bound and think it will go to an inconsistent state if we just restart teamd alone.

I was referring to "systemctl restart teamd". Teamd-docker upgrade (sonic_installer upgrade_docker teamd docker-teamd.gz --cleanup_image -y) issues this command internally.
Say, there are NO configurations on the port-channel (VLAN, IP, etc) => docker upgrade can be done (it wouldn't impact applications).
Note: If there is any application configuration - all applications need to register for port-channel events like (create/delete/up/down/etc) to handle the teamd-docker-upgrade OR teamd-service-restart. Reason - new port-channel netdevices (with new ifindex) are created in the kernel.

… and checking for the flag in the main loop. This way the cleanUp handler is not run in the signal Handler context and can add more Logs as we need not care for signal safety now.

Teammgrd tracks the PID's of the teamd processes and sents the SIGTERM signal when the teamd docker is stopped.

pavel-shirshov

Can you please answer on my questions?

cfgmgr/teammgr.h

cfgmgr/teammgr.cpp

cfgmgr/teammgrd.cpp

teamsyncd/teamsyncd.cpp

pavel-shirshov · 2020-01-11T01:22:18Z

cfgmgr/teammgr.cpp

+        }
+        else
+        {
+            SWSS_LOG_WARN("Cound not find PID corresponding to LaG %s ", it.c_str());


Can you please make it as an error?
Also It's better to say "Can't send TERM signal to LAG %s. PID wasn't found"

pavel-shirshov

looks good for me

qiluo-msft · 2020-01-16T19:56:00Z

cfgmgr/teammgrd.cpp


+bool received_sigterm = false;
+
+void sig_handler(int signo)


signo [](start = 21, length = 5)

check signo == SIGTERM first?

The Signal handler is only registered only for SIGTERM now. The handler will be called only on SIGTERM signal.

qiluo-msft · 2020-01-16T19:59:26Z

teamsyncd/teamsyncd.cpp


+bool received_sigterm = false;
+
+void sig_handler(int signo)


signo [](start = 21, length = 5)

the same

The Signal handler is only registered only for SIGTERM now. The handler will be called only on SIGTERM signal.

qiluo-msft · 2020-01-16T20:02:11Z

teamsyncd/teamsyncd.cpp

            {
+                if(received_sigterm)
+                {
+                  sync.cleanTeamSync();


[](start = 16, length = 2)

Inconsistent indentation.

…ocker stop (#1159) * Send explicit signal to the teamd processes whenthe teamd docker exits. When the teamd docker receives a stop signal, only the processes started by supervisord gets the SIGTERM, so this fix is to propogate the signal to teamd processes via the signal handler in teamsyncd process. * Updates to take care of boundary conditions in the teamsyncd signal handler. * Better way of signal Handling by setting a flag in the signal handler and checking for the flag in the main loop. This way the cleanUp handler is not run in the signal Handler context and can add more Logs as we need not care for signal safety now. * Updated the logic so that teammgrd controls the lifecycle of teamd. Teammgrd tracks the PID's of the teamd processes and sents the SIGTERM signal when the teamd docker is stopped. * Minor change in the function defenition * Updates based on the comments * Minor update in teammgr.cpp

Test make sure cleanup happens of Port-channel Kernel devices. This test case track the fixes done by PR: sonic-net/sonic-swss#1407 sonic-net/sonic-swss#1159 Signed-off-by: Abhishek Dosi <[email protected]>

* Added the test case for Port Channel cleanup. Test make sure cleanup happens of Port-channel Kernel devices. This test case track the fixes done by PR: sonic-net/sonic-swss#1407 sonic-net/sonic-swss#1159 Signed-off-by: Abhishek Dosi <[email protected]> * Address Review Comments Signed-off-by: Abhishek Dosi <[email protected]>

sonic-net#1159) * Global and Interface commands for IPv6 Link local feature * SONiC CLI per interface configuration command to enable and disable the IPv6 link-local address mode when addresses are not configured manually. Signed-off-by: Akhilesh Samineni <[email protected]>

…ocker stop (sonic-net#1159) * Send explicit signal to the teamd processes whenthe teamd docker exits. When the teamd docker receives a stop signal, only the processes started by supervisord gets the SIGTERM, so this fix is to propogate the signal to teamd processes via the signal handler in teamsyncd process. * Updates to take care of boundary conditions in the teamsyncd signal handler. * Better way of signal Handling by setting a flag in the signal handler and checking for the flag in the main loop. This way the cleanUp handler is not run in the signal Handler context and can add more Logs as we need not care for signal safety now. * Updated the logic so that teammgrd controls the lifecycle of teamd. Teammgrd tracks the PID's of the teamd processes and sents the SIGTERM signal when the teamd docker is stopped. * Minor change in the function defenition * Updates based on the comments * Minor update in teammgr.cpp

judyjoseph added 2 commits December 23, 2019 13:08

Send explicit signal to the teamd processes whenthe teamd docker exits.

581e873

When the teamd docker receives a stop signal, only the processes started by supervisord gets the SIGTERM, so this fix is to propogate the signal to teamd processes via the signal handler in teamsyncd process.

Updates to take care of boundary conditions in the teamsyncd signal h…

d4b473b

…andler.

judyjoseph requested a review from pavel-shirshov December 23, 2019 22:54

judyjoseph mentioned this pull request Dec 23, 2019

teamsyncd::Additional checks put inplace to make sure the netlink message is valid #1144

Closed

madhukar-kamarapu reviewed Dec 24, 2019

View reviewed changes

teamsyncd/teamsync.cpp Outdated Show resolved Hide resolved

pavel-shirshov suggested changes Dec 24, 2019

View reviewed changes

Better way of signal Handling by setting a flag in the signal handler…

b2c023e

… and checking for the flag in the main loop. This way the cleanUp handler is not run in the signal Handler context and can add more Logs as we need not care for signal safety now.

judyjoseph requested a review from pavel-shirshov January 2, 2020 18:55

judyjoseph added 2 commits January 9, 2020 22:55

Updated the logic so that teammgrd controls the lifecycle of teamd.

aa55cbb

Teammgrd tracks the PID's of the teamd processes and sents the SIGTERM signal when the teamd docker is stopped.

Minor change in the function defenition

d991280

pavel-shirshov reviewed Jan 10, 2020

View reviewed changes

Updates based on the comments

7264c8d

judyjoseph requested a review from pavel-shirshov January 11, 2020 00:31

pavel-shirshov reviewed Jan 11, 2020

View reviewed changes

Minor update in teammgr.cpp

c61135f

judyjoseph requested a review from pavel-shirshov January 11, 2020 03:16

pavel-shirshov approved these changes Jan 11, 2020

View reviewed changes

judyjoseph merged commit 80e01c0 into sonic-net:master Jan 15, 2020

yxieca added Request for 201811 Branch request_for_201911_branch labels Jan 16, 2020

prsunny mentioned this pull request Jan 16, 2020

Leftover portchannel found in the kernel after switch topology from T0 to T1-LAG sonic-net/sonic-buildimage#2760

Open

qiluo-msft reviewed Jan 16, 2020

View reviewed changes

yxieca added the Included in 201811 Branch label Jan 16, 2020

prsunny mentioned this pull request Jan 17, 2020

portchannel ip is lost after reboot, but config reload works fine sonic-net/sonic-buildimage#4015

Open

abdosi added the Included in 201911 Branch label Jan 21, 2020

prsunny added the Request_for_bmtor label Jan 28, 2020

judyjoseph deleted the teamd_fix branch January 30, 2020 15:07

prsunny mentioned this pull request Feb 8, 2020

"show ip interface" shows phantom PortChannel interface. sonic-net/sonic-utilities#774

Closed

pavel-shirshov mentioned this pull request Aug 20, 2020

[teamsyncd][teammgrd] Graceful exit after receiving SIGTERM #1407

Merged

abdosi mentioned this pull request Sep 3, 2020

Added test case to make sure when teamd feature/service is stop kernel device is cleanup and SIGTERM are handled gracefully sonic-net/sonic-mgmt#2186

Merged

abdosi mentioned this pull request Sep 8, 2020

Added the test case for Port Channel cleanup into runtest.sh Azure/sonic-build-tools#157

Merged

abdosi mentioned this pull request Sep 27, 2020

Some of Netdevice created by teamd are not cleaned up when teamd service is disabled. #1450

Merged

dgsudharsan mentioned this pull request Apr 14, 2021

ERR swss#orchagent: :- removeLag: Failed to remove ref count 1 LAG PortChannel0001 is seen during config reload sonic-net/sonic-buildimage#7317

Open

Conversation

judyjoseph commented Dec 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

madhukar-kamarapu commented Dec 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

pavel-shirshov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

judyjoseph commented Dec 24, 2019

Uh oh!

madhukar-kamarapu commented Dec 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pavel-shirshov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pavel-shirshov Jan 11, 2020

Choose a reason for hiding this comment

Uh oh!

pavel-shirshov left a comment

Choose a reason for hiding this comment

Uh oh!

qiluo-msft Jan 16, 2020

Choose a reason for hiding this comment

Uh oh!

judyjoseph Jan 16, 2020

Choose a reason for hiding this comment

Uh oh!

qiluo-msft Jan 16, 2020

Choose a reason for hiding this comment

Uh oh!

judyjoseph Jan 16, 2020

Choose a reason for hiding this comment

Uh oh!

qiluo-msft Jan 16, 2020

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

judyjoseph commented Dec 23, 2019 •

edited

Loading

madhukar-kamarapu commented Dec 24, 2019 •

edited

Loading

madhukar-kamarapu commented Dec 24, 2019 •

edited

Loading