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.

Sign up for GitHub

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

Jump to bottom

[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

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c60ca91
Add a timer in stop() function around while loop to force syncd exit
yozhao101 May 19, 2020
71aad90
Remove extra blank line at 180.
yozhao101 May 20, 2020
0fa9da3
Use builtin variable $SECONDS to replace "$(date +%s)"
yozhao101 May 22, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 12 additions & 2 deletions files/scripts/syncd.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,11 +163,21 @@ stop() {
debug "${TYPE} shutdown syncd process ..."
/usr/bin/docker exec -i syncd$DEV /usr/bin/syncd_request_shutdown --${TYPE}

# wait until syncd quits gracefully
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=${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 •
edited
Loading

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.

while docker top syncd$DEV | grep -q /usr/bin/syncd \
&& [[ $((end_in_secs - start_in_secs)) -le $timer_threshold ]]; do
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

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 •
edited
Loading

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.


if [[ $((end_in_secs - start_in_secs)) -gt $timer_threshold ]]; then
debug "syncd process in container syncd$DEV did not exit gracefully"
fi

/usr/bin/docker exec -i syncd$DEV /bin/sync
debug "Finished ${TYPE} shutdown syncd process ..."
fi
Expand Down