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

Is not exiting gracefully #3

Closed
luckydonald opened this issue Aug 25, 2016 · 13 comments
Closed

Is not exiting gracefully #3

luckydonald opened this issue Aug 25, 2016 · 13 comments

Comments

@luckydonald
Copy link

luckydonald commented Aug 25, 2016

Using compose, after docker-compose up.
I ask it to quit,ctrl+C when started without detaching,
or else docker-compose down.
I noticed, that my flask-docker instance always time outs after the 10 seconds.

I believe that could be some issue with the pid-1-zombie-process thing.

Some references:

Can that issue apply here?

@tiangolo
Copy link
Owner

tiangolo commented Oct 1, 2016

I don't think that the pid-1 zombie process problem applies here, as everything is controlled with Supervisor, as described in the official Docker docs: https://docs.docker.com/engine/admin/using_supervisord/

@luckydonald
Copy link
Author

But it doesn't shut down gracefully.

@tiangolo
Copy link
Owner

tiangolo commented Dec 5, 2016

Can you please elaborate?

@luckydonald
Copy link
Author

If you use this in an docker-compose project, when you then press ctrl-c, it will first said gracefully stopping and 10 seconds later stopping which means killing.
You can increase the default timeout:
docker-compose up -t 60
Which will wait 60 seconds before terminating

@tiangolo
Copy link
Owner

This should be fixed now.

The change was here in the base image: https://github.com/tiangolo/uwsgi-nginx-docker/releases/tag/v0.3.1

Could you confirm if it is working properly?


There were a lot of other changes too: https://github.com/tiangolo/uwsgi-nginx-flask-docker/releases/tag/v0.3.0

@luckydonald
Copy link
Author

I had replaced supervisord with a bash script I call überstart:

#!/usr/bin/env bash
#!/bin/bash

# check every $TIMEOUT seconds
TIMEOUT=10
# check every $TIMEOUT seconds
GRACE_TIMEOUT=3


function check_process() {
    if ! kill -s 0 $1; then
        wait $1
        CODE=$?
        echo "[überstart] PID $1 terminated with code $CODE.";
        exit $CODE;
    fi
}

# Start the first process
/usr/local/bin/uwsgi --ini /etc/uwsgi/uwsgi.ini --ini /app/uwsgi.ini --need-app --die-on-term &
PROCESS_ONE_PID=$!

echo "[überstart] uwsgi is running as PID $PROCESS_ONE_PID.";
check_process $PROCESS_ONE_PID

# Start the second process
/usr/sbin/nginx &
PROCESS_TWO_PID=$!

echo "[überstart] nginx is running as PID $PROCESS_TWO_PID.";
check_process $PROCESS_TWO_PID

# Naive check runs checks once a minute (actually $TIMEOUT seconds) to see if either of the processes exited.
# This illustrates part of the heavy lifting you need to do if you want to run
# more than one service in a container. The container will exit with an error
# if it detects that either of the processes has exited.
# Otherwise it will loop forever, waking up every $TIMEOUT seconds

# Kill children if we are killed https://stackoverflow.com/a/2173408/3423324

# ctrl-c / KeyboardInterrupt
trap "echo '[überstart] KeyboardInterrupt, going down';kill -SIGINT $PROCESS_ONE_PID $PROCESS_TWO_PID; wait;" SIGINT
# sigterm, kill -15
trap "echo '[überstart] Termination, going down';kill -SIGTERM $PROCESS_ONE_PID $PROCESS_TWO_PID; wait;" SIGTERM
# this script terminated
trap "echo '[überstart] Done, going down';kill -SIGINT $PROCESS_ONE_PID $PROCESS_TWO_PID; sleep $GRACE_TIMEOUT;echo \"[überstart] $GRACE_TIMEOUT timeout, going down for real.\";kill -SIGTERM $PROCESS_ONE_PID $PROCESS_TWO_PID; wait;"  EXIT

set -e;

while /bin/true; do
  check_process $PROCESS_ONE_PID
  check_process $PROCESS_TWO_PID
  sleep $TIMEOUT
done

Maybe you also should add --need-app to uwsgi, so it terminates right away if the python app couldn't start. Instead of ignoring errors and printing no python application found, check startup logs! on each request.

@tiangolo
Copy link
Owner

Thanks for the tip, I'll check it.

@ziru
Copy link

ziru commented Mar 26, 2018

It seems I do not have permission to create a PR. Here is the local patch (to let superviord becomes the front process of the container, and thus handle the SIGTERM):

diff --git a/python2.7-alpine3.7/start.sh b/python2.7-alpine3.7/start.sh
index 55099b1..8626c15 100644
--- a/python2.7-alpine3.7/start.sh
+++ b/python2.7-alpine3.7/start.sh
@@ -12,4 +12,4 @@ else
 fi

 # Start Supervisor, with Nginx and uWSGI
-/usr/bin/supervisord
+exec /usr/bin/supervisord
diff --git a/python2.7/start.sh b/python2.7/start.sh
index ed7d19c..87f02ba 100644
--- a/python2.7/start.sh
+++ b/python2.7/start.sh
@@ -7,9 +7,9 @@ echo "Checking for script in $PRE_START_PATH"
 if [ -f $PRE_START_PATH ] ; then
     echo "Running script $PRE_START_PATH"
     source $PRE_START_PATH
-else
+else
     echo "There is no script $PRE_START_PATH"
 fi

 # Start Supervisor, with Nginx and uWSGI
-/usr/bin/supervisord
+exec /usr/bin/supervisord
diff --git a/python3.6-alpine3.7/start.sh b/python3.6-alpine3.7/start.sh
index 55099b1..8626c15 100644
--- a/python3.6-alpine3.7/start.sh
+++ b/python3.6-alpine3.7/start.sh
@@ -12,4 +12,4 @@ else
 fi

 # Start Supervisor, with Nginx and uWSGI
-/usr/bin/supervisord
+exec /usr/bin/supervisord
diff --git a/python3.6/start.sh b/python3.6/start.sh
index ed7d19c..87f02ba 100644
--- a/python3.6/start.sh
+++ b/python3.6/start.sh
@@ -7,9 +7,9 @@ echo "Checking for script in $PRE_START_PATH"
 if [ -f $PRE_START_PATH ] ; then
     echo "Running script $PRE_START_PATH"
     source $PRE_START_PATH
-else
+else
     echo "There is no script $PRE_START_PATH"
 fi

 # Start Supervisor, with Nginx and uWSGI
-/usr/bin/supervisord
+exec /usr/bin/supervisord

Can you please help check?

@tiangolo
Copy link
Owner

@ziru thanks for debugging it. Good catch.

It is implemented in the latest commit: 30d01a7

If you check it, please let me know how it goes.

@luckydonald
Copy link
Author

luckydonald commented Apr 27, 2018

Could someone specify the format of the diff above as diff, so it get's syntax highlighting, please?

```diff
blah
```

The difference seems to be to put exec in front of /usr/bin/supervisord?

@tiangolo
Copy link
Owner

Yes, that's the difference @luckydonald .

And it's already in the latest commit.

@tiangolo
Copy link
Owner

There have been several improvements in the shutdown process, forcing uWSGI to have an "app", etc.

I'll close this issue now as I think you solved your problem, and your suggestion was actually implemented some time ago.

But feel free to add more comments, test it and check it, etc.

@luckydonald
Copy link
Author

👍

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

No branches or pull requests

3 participants