BUG: The system restarts without graceful shutdown of the containers.

[Krystle]

Hi,

I discovered that executing the “reboot” or “poweroff” commands the host doesn’t wait until all the containers are correctly shutdown. I’ve analyzed the problem and the cause is the init script that manages the docker daemon. Here the description of problem:

• When you want to reboot or poweroff the host, all the daemon services require the be stoped.
• The current implementation does the correct behaviour calling “stop” to all scripts in “/etc/init.d”.
• However, some of the scripts use the “start-stop-daemon” tool. But this tool is from busybox. Therefore, this implementation doesn’t have any support for the “--retry” parameter when calling to “--stop”. So the current behaviour is that the script doesn’t block until the process ends. And after the system calls to the service stop, the init script finish and the system goes off.
• You can check this very easy: call to “/etc/init.d/S60dockerd stop” while running “top” and see that the script terminates immediately but the process is closing the running containers.

One solution is to chage from the busybox’s “start-stop-daemon” version to the full package in buildroot. And then add to the docker script the parameter “--retry TERM/120/QUIT/10/KILL” (in example) when doing the stop call.

Another solution is what I’ve implemented. A simple block to wait until completion inside the script. Here the patch:

diff --git a/S60dockerd b/S60dockerd
index 20419ad..958fe99 100644
--- a/S60dockerd
+++ b/S60dockerd
@@ -40,8 +40,25 @@ do_start() {

 do_stop() {
         echo -n "Stopping $NAME: "
-        start-stop-daemon --stop --quiet --pidfile $PIDFILE \
+        ppid=$(cat $PIDFILE)
+        start-stop-daemon --stop --quiet --retry TERM/120/QUIT/10/KILL --pidfile $PIDFILE \
                 && echo "OK" || echo "FAIL"
+
+        # Wait to exit until the process completes (workaround for missing --retry support)
+        if [ -f $PIDFILE ]; then
+                ppid=$(cat $PIDFILE)
+        fi
+        if [ -n "$ppid" ]; then
+                echo -n "Waiting to complete $NAME: "
+                cmd="awk '\$1==$ppid {print \$0}' <(top -n 1 -b)"
+                timeout=120
+                while [[ "$(eval "$cmd")" ]] && [[ $timeout -gt 0 ]]; do echo -n "$timeout,"; sleep 1; timeout=$((timeout-1)); done
+                if [[ $timeout -gt 0 ]]; then
+                        echo "OK"
+                else
+                        echo "FAIL"
+                fi
+        fi
 }

 case "$1" in

I’m using it without troubles. The timeout could be changed. I prefer 2 minutes beacuse I’m using heavy containers. But the scripts ends if the dockerd process quits early. So no problem at all. Futhermore, it never kills the process. The reason is simple: when you want to poweroff/reboot the system will complete when the timeout arrives (that’s a forced shutdown). And if you call to stop the service, then with the FAIL message you can do whatever actition that you prefer (force to kill?).

I hope you want to merge (or fix) this soon.

Regards.
K.

‌

[Krystle]

Hi,

In my previous patch you’ll see that the first ppid=$(cat $PIDFILE before the call to the start-stop-daemon is erroneous because it requires to be the initialization. This line needs to be ppid="". So please update the patch and add it to the next release.

Regards.
K.

[Krystle]

Hi Stephan,

This is my patch clean and ready to merge it:

diff --git a/S60dockerd b/S60dockerd
index 20419ad..ca3e8ec 100644
--- a/S60dockerd
+++ b/S60dockerd
@@ -40,8 +40,25 @@ do_start() {

 do_stop() {
         echo -n "Stopping $NAME: "
+        ppid=""
         start-stop-daemon --stop --quiet --pidfile $PIDFILE \
                 && echo "OK" || echo "FAIL"
+
+        # Wait to exit until the process completes (workaround for missing "--retry TERM/120/QUIT/10/KILL" support)
+        if [ -f $PIDFILE ]; then
+                ppid=$(cat $PIDFILE)
+        fi
+        if [ -n "$ppid" ]; then
+                echo -n "Waiting to complete $NAME: "
+                cmd="awk '\$1==$ppid {print \$0}' <(top -n 1 -b)"
+                timeout=120
+                while [[ "$(eval "$cmd")" ]] && [[ $timeout -gt 0 ]]; do echo -n "$timeout "; sleep 1; timeout=$((timeout-1)); done
+                if [[ $timeout -gt 0 ]]; then
+                        echo "OK"
+                else
+                        echo "FAIL"
+                fi
+        fi
 }

 case "$1" in

I hope you want to add it. ;-)

Regards.
K.

[Stephan Henningsen]

Crikey!

Thanks allot for the effort with the testing and the patch, K!

I’ll give this top priority!

[Krystle]

Nice!

[Krystle]

Hi Stephan,

Regarding your solution implemented in the commit https://bitbucket.org/asklandd/lightwhale/commits/da6c4afa1e12d66e6a466ccd78d0d715c0d1e28a

I’ve one comment:

• I don’t agree with this line:

while [ -n "$pid" ] && [ $timeout -gt 0 ] && pgrep -f $DAEMON>/dev/null; do

• In a previous line you get the pid value from the $DAEMON name, if the pid file doesn’t exist. And this is correct.
• However, you never have to check inside the loop (with pgrep -f $DAEMON) the existence of the daemon. You need to check for the existence of the process given by the pid. What happens if you’re running more than one daemon? For this reason you use the pid file!
• If you want to use the pgrep tool, then I recommend to use the command pgrep -P $pid that works with busybox’s pgrep.

So, please revise your script.
Regards.
K.

‌

‌

‌

[Stephan Henningsen]

Hi K,

Thanks for the review! =)

You caught me in unfinished work, but your feedback was still appreciated and valuable. You’re correct that I don’t handle multiple dockerd processes.

My new commit is still work-in-progress, and still completely untested, but you’re welcome to review with focus on my new strategy. I don’t use start-stop-daemon anymore, since I’d like to be a bit more robust in case the pidfile is stale. I still don’t handle multiple dockerd processes, but I intend to FAIL explicitly here. A perhaps better option would be to killall -TERM dockerd and at least give it a chance.

But I think that if multiple dockerd processes exist, people must have been tinkering a bit too much (delibirately started dockerd with a random --pidfile), and I think it's a fair define that as an unsupported state.

Come to think of it, perhaps I’m overthinking this. Docker on Lightwhale is supposed to be started and stopped with the S60docker script. That results in known states, ie. one dockerd process with a known and valid pidfile. Anything else is a result of carelessness and/or domestic Lightwhale violence =). I mean, I can’t safeguard against all possible ways things can break.

However, I do think it’s important to take good care of especially Docker and the containers; that’s the only important thing that runs on the server. Actually, I think it’s enough to just insert that wait-loop with the timeout after start-stop-daemon --stop.

What’s your opinion on this?

‌

Yours,
Stephan

[Stephan Henningsen]

There’s a dev6 build here if you’d like to test it in action: https://lightwhale.asklandd.dk/dev/

I’ve done some testing and it looks file so far. I’ll test multiple dockerd later, where the script will just FAIL.

[Krystle]

Hi Stephan,

As for my personal opinion... the KISS principle has advantages. Therefore:

• First of all all your init scripts seem to have common points between them. And that's a good thing. Therefore, I suggest keeping as much generalization as possible in them. So don't try to be too specific if you can reuse something. Based on this I suggest to keep using the "start-stop-daemon" tool. Why reinvent the wheel?
• The problem pointed out by me related to the use of the pgrep -f $DAEMON command is actually a generic problem. When you need to control a process, it could be considered an error if you look for the binary and not the process identifier. Who can guarantee that no other process is started from the same binary? This assumption is the problem to circumvent. So it's necessary to always use the pid file to retrieve the pid value. And only in extreme circumstances look for the pid value. But then use the pid value for everything else.
• And finally in the particular case of the S60docker script, I prefer your original version by adding only the wait-loop. And if the loop is simple, then it will be much better. For example, what action do you need to take if the process doesn't stop? In the end that's the only relevant question.

I hope this will help you to design the best solution you want to implement.
Thank you for your efforts with this very nice project.
K.

‌

[Stephan Henningsen]

Thanks for your feedback, I appreciate it.

I’ve simplified the script and just added the wait-loop. I agree that it’s a better to have a common form for the scripts and favor that.

I wonder how many other processes may potentially suffer from start-stop-daemon being so crude. Perhaps I’ll look into using the full-blown implementation at some point.

Btw, I think you misread my intent of using pgrep -f $DAEMON for $actual_pid. It’s only used as an extra safety in case the pidfile is missing, or the $expected_pid read from pidfile differs from $actual_pid; the sunshine scenario is still to kill $expected_pid (wich I assigned it to $actual_pid when it is the actual PID). So all in all it’s more robust than just counting on the pidfile to tell the truth. But also more (and perhaps unnecessary) complex to read…

[Stephan Henningsen]

Hi again.

I decided to replace start-stop-daemon with the full-blown version. This made the S60dockerd script so much better — shorter and more reliable. I’ll be dropping a dev11 build shortly. It also includes all the virtualization modules, you requested.

[Stephan Henningsen]

It’s looking good, so I’m closing it.

‌

[Stephan Henningsen]

fixed in 2.1.2