Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[docker-syncd] Add timeout to force stop syncd container #4617

Merged
merged 3 commits into from
Jun 4, 2020
Merged

[docker-syncd] Add timeout to force stop syncd container #4617

merged 3 commits into from
Jun 4, 2020

Conversation

yozhao101
Copy link
Contributor

Signed-off-by: Yong Zhao yozhao@microsoft.com

- Why I did it
When I tested auto-restart feature of swss container by manually killing one of critical processes
in it, swss will be stopped. Then syncd container as the peer container should also be
stopped as expected. However, I found sometimes syncd container can be stopped, sometimes
it can not be stopped. The reason why syncd container can not be stopped is the process
(/usr/local/bin/syncd.sh stop) to execute the stop() function will be stuck between the lines 164 –
167. Systemd will wait for 90 seconds and then kill this process.

164 # wait until syncd quit gracefully
165 while docker top syncd$DEV | grep -q /usr/bin/syncd; do
166 sleep 0.1
167 done

The first thing I did is to profile how long this while loop will spin if syncd container can be
normally stopped after swss container is stopped. The result is 5 seconds or 6 seconds. If syncd
container can be normally stopped, two messages will be written into syslog:

str-a7050-acs-3 NOTICE syncd#dsserve: child /usr/bin/syncd exited status: 134
str-a7050-acs-3 INFO syncd#supervisord: syncd [5] child /usr/bin/syncd exited status: 134

The second thing I did was to add a timer in the condition of while loop to ensure this while loop
will be forced to exit after 20 seconds:

After that, the testing result is that syncd container can be normally stopped if swss is stopped
first. One more thing I want to mention is that if syncd container is stopped during 5 seconds or 6
second, then the two log messages can be still seen in syslog. However, if the execution time of
while loop is longer than 20 seconds and is forced to exit, although syncd container can be
stopped, I did not see these two messages in syslog. Further, although I observed the auto-restart
feature of swss container can work correctly right now, I can not make sure the issue which syncd
container can not stopped will occur in future.

- How I did it
I added a timer around the while loop in stop() function. This while loop will exit after spinning
20 seconds.

- How to verify it

after waiting for 20 seconds.

Signed-off-by: Yong Zhao <yozhao@microsoft.com>
files/scripts/syncd.sh Outdated Show resolved Hide resolved
Signed-off-by: Yong Zhao <yozhao@microsoft.com>
@lguohan
Copy link
Collaborator

lguohan commented May 21, 2020

retest mellanox please

while docker top syncd$DEV | grep -q /usr/bin/syncd; do
# wait until syncd quits gracefully or force syncd to exit after
# waiting for 20 seconds
start_in_secs=$(date +%s)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bash has a buildin variable ${SECONDS} for up seconds. You can use that instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great suggestion! Updated.

Signed-off-by: Yong Zhao <yozhao@microsoft.com>
sleep 0.1
end_in_secs=${SECONDS}
done
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you try timeout wait pgrep?

Copy link
Contributor Author

@yozhao101 yozhao101 May 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried this method. Actually pgrep command needs process name or partial process name as its argument. We are unable to use pgrep /usr/bin/syncd from host point of view since /usr/bin/syncd is the execution path in the syncd container.

@jleveque
Copy link
Contributor

Retest vsimage please

sleep 0.1
end_in_secs=${SECONDS}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not move this line to the top of the wile loop and remove line 169?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean we should modify the code like this?
start_in_secs=${SECONDS}
end_in_secs=${SECONDS}
timer_threshold=20
while docker top syncd$DEV | grep -q /usr/bin/syncd \ && [[ $((end_in_secs - start_in_secs)) -le $timer_threshold ]]; do
sleep 0.1

# waiting for 20 seconds
start_in_secs=${SECONDS}
end_in_secs=${SECONDS}
timer_threshold=20
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why 20 seconds? what is the rationale?

Copy link
Contributor Author

@yozhao101 yozhao101 May 31, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This threshold 20 seconds is what we discussed in our weekly meeting. In the meeting, we decided to set the timer to be 20 seconds and wait for enough long time such that syncd process has a chance to exit.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there must be some rationale to set it to 20 seconds, why not 10 seconds? can you explain it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually when I tested the auto-restart feature of swss container, syncd container sometimes can be normally stopped and sometimes can not be stopped. The first thing I did is to profile how long this while loop will spin if syncd container can be normally stopped after swss container is stopped. The result is 5 seconds or 6 seconds. After that, in our meeting, we talked about to set a 20 seconds timer for this while loop before forcing syncd to exit. Please review the section of "Why I did it" of this PR for more details.

Sure, I can also try to set a 10 seconds timer to check whether it works or not.

@lguohan
Copy link
Collaborator

lguohan commented May 31, 2020

retest vsimage please

1 similar comment
@lguohan
Copy link
Collaborator

lguohan commented Jun 1, 2020

retest vsimage please

@yozhao101 yozhao101 changed the title Add a timer around the while loop in stop() function of syncd.sh under /usr/local/bin [usr/local/bin/syncd.sh] Add timeout to force stop syncd container Jun 4, 2020
@yozhao101 yozhao101 changed the title [usr/local/bin/syncd.sh] Add timeout to force stop syncd container [docker-syncd] Add timeout to force stop syncd container Jun 4, 2020
@yozhao101 yozhao101 merged commit 4ea2e5e into sonic-net:master Jun 4, 2020
yxieca pushed a commit that referenced this pull request Jun 9, 2020
**- Why I did it**
When I tested auto-restart feature of swss container by manually killing one of critical processes in it, swss will be stopped. Then syncd container as the peer container should also be
stopped as expected. However, I found sometimes syncd container can be stopped, sometimes
it can not be stopped. The reason why syncd container can not be stopped is the process
(/usr/local/bin/syncd.sh stop) to execute the stop() function will be stuck between the lines 164 –167. Systemd will wait for 90 seconds and then kill this process.

164 # wait until syncd quit gracefully
165 while docker top syncd$DEV | grep -q /usr/bin/syncd; do
166 sleep 0.1
167 done

The first thing I did is to profile how long this while loop will spin if syncd container can be
normally stopped after swss container is stopped. The result is 5 seconds or 6 seconds. If syncd
container can be normally stopped, two messages will be written into syslog:

str-a7050-acs-3 NOTICE syncd#dsserve: child /usr/bin/syncd exited status: 134
str-a7050-acs-3 INFO syncd#supervisord: syncd [5] child /usr/bin/syncd exited status: 134

The second thing I did was to add a timer in the condition of while loop to ensure this while loop will be forced to exit after 20 seconds:

After that, the testing result is that syncd container can be normally stopped if swss is stopped
first. One more thing I want to mention is that if syncd container is stopped during 5 seconds or 6 seconds, then the two log messages can be still seen in syslog. However, if the execution
time of while loop is longer than 20 seconds and is forced to exit, although syncd container can be stopped, I did not see these two messages in syslog. Further, although I observed the auto-restart feature of swss container can work correctly right now, I can not make sure the issue which syncd container can not stopped will occur in future.

**- How I did it**
I added a timer around the while loop in stop() function. This while loop will exit after spinning
20 seconds.

Signed-off-by: Yong Zhao <yozhao@microsoft.com>
abdosi pushed a commit that referenced this pull request Jun 16, 2020
**- Why I did it**
When I tested auto-restart feature of swss container by manually killing one of critical processes in it, swss will be stopped. Then syncd container as the peer container should also be
stopped as expected. However, I found sometimes syncd container can be stopped, sometimes
it can not be stopped. The reason why syncd container can not be stopped is the process
(/usr/local/bin/syncd.sh stop) to execute the stop() function will be stuck between the lines 164 –167. Systemd will wait for 90 seconds and then kill this process.

164 # wait until syncd quit gracefully
165 while docker top syncd$DEV | grep -q /usr/bin/syncd; do
166 sleep 0.1
167 done

The first thing I did is to profile how long this while loop will spin if syncd container can be
normally stopped after swss container is stopped. The result is 5 seconds or 6 seconds. If syncd
container can be normally stopped, two messages will be written into syslog:

str-a7050-acs-3 NOTICE syncd#dsserve: child /usr/bin/syncd exited status: 134
str-a7050-acs-3 INFO syncd#supervisord: syncd [5] child /usr/bin/syncd exited status: 134

The second thing I did was to add a timer in the condition of while loop to ensure this while loop will be forced to exit after 20 seconds:

After that, the testing result is that syncd container can be normally stopped if swss is stopped
first. One more thing I want to mention is that if syncd container is stopped during 5 seconds or 6 seconds, then the two log messages can be still seen in syslog. However, if the execution 
time of while loop is longer than 20 seconds and is forced to exit, although syncd container can be stopped, I did not see these two messages in syslog. Further, although I observed the auto-restart feature of swss container can work correctly right now, I can not make sure the issue which syncd container can not stopped will occur in future.

**- How I did it**
I added a timer around the while loop in stop() function. This while loop will exit after spinning
20 seconds.

Signed-off-by: Yong Zhao <yozhao@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants