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

Graceful shutdown #93

Merged
merged 2 commits into from Aug 17, 2017
Merged

Graceful shutdown #93

merged 2 commits into from Aug 17, 2017

Conversation

ghost
Copy link

@ghost ghost commented Aug 1, 2017

This change attempts to solve #92. Once the shell scripts have bootstrapped the container, they are no longer required. I've added exec to both the shell scripts so that the fluentd process replaces them instead of becoming a child process of them.

It seemed reasonable to add exec to entrypoint.sh. It didn't feel as good when adding the exec to the CMD. I hope if anyone is overriding the CMD that they are aware they might be breaking graceful shutdown. What do you think?

I repeated my tests with one of the containers I've built from this branch.

Did we get rid of those sh processes?

# docker build -t fluentd-graceful-shutdown-fix .
# docker run -d --name fluentd fluentd-graceful-shutdown-fix
# docker exec fluentd pstree -p
entrypoint.sh(1)---fluentd(7)---fluentd(16)

We can see that fluentd is a child of PID1 now, so it should be managed by dumb-init.

What happens when we stop it?

# strace -e trace=signal $(docker top fluentd | tail -n +2 | awk ' { x=x" -p "
$1 } END { print x }') 2>&1 | grep "+++" > /tmp/trace &
# docker stop fluentd
fluentd
[1]+  Done                       strace -e trace=signal $(...) 2>&1 | grep "+++" 1>/tmp/trace
# cat /tmp/trace 
[pid 31164] +++ exited with 0 +++
[pid 31156] +++ exited with 0 +++
+++ exited with 0 +++

We can see that the fluentd processes exited cleanly.

David Yeung added 2 commits August 2, 2017 00:57
Removing entrypoint.sh from the process tree helps us
to manage signals better.
Removing the command's one line script from the process
tree helps us to manage signals better.
@repeatedly
Copy link
Member

@ValFadeev @atombender Do you see any problem for this patch?

@ValFadeev
Copy link
Contributor

I think it's a great observation and a neat fix. The whole idea with using dumb-init was to achieve proper signal forwarding. However, it appears that the scripts are indeed getting in the way, and it would definitely not be appropriate to try to implement signal forwarding by hand (as it destroys the purpose of dumb-init wrapping)

@ghost
Copy link
Author

ghost commented Aug 11, 2017

Is there anything more I can do to help?

How about a picture of a cat in a sink?

cat sink drinking

@repeatedly repeatedly merged commit e5bc225 into fluent:master Aug 17, 2017
@repeatedly
Copy link
Member

Sorry for the delay. Just merged. Thanks 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants