Non-fatal signals break restart policies

[tonistiigi]

Sending a signal with docker kill that is handled by the application will turn off restart policies for that container.

> docker run -d --restart=always --name s0 busybox top
> kill -SIGTERM $(docker inspect -f "{{.State.Pid}}" s0)
> sleep 1
> docker ps | grep s0 | wc -l
1
> docker kill -s CONT s0
> docker ps | grep s0 | wc -l
1
> kill -SIGTERM $(docker inspect -f "{{.State.Pid}}" s0)
> sleep 1
> docker ps | grep s0 | wc -l
0

Not sure how to fix this without changing the behavior for docker kill as there is no way to predict if a signal will cause the container to exit or not.

To me its kind of weird that docker kill -s TERM foo and kill -SIGTERM $(docker inspect -f "{{.State.Pid}}" foo) have different meaning for the restart logic, but I can see it being useful for some situations.

Tested with v1.5.0 and master

[tonistiigi]

/cc @crosbymichael

[crosbymichael]

Yep, this looks like a bug, thanks for the ping.

[tonistiigi]

@crosbymichael Any ideas how to fix it? Seems more like a logic issue than a code bug.

[crosbymichael]

logic but it's kinda hard to identify what is a "non-fatal" signal compared to something else. Like what happens if a non-fatal signal actually crashes the app?

[tonistiigi]

Yes, for anything other than STOP and KILL (afaik) its for the application to decide. There are no rules for the others. Some, like HUP or CONT are quite obvious of course but I don't think mind reading is a way to go. I'm more thinking about something like docker kill -s TERM --no-restart/--stop edit: docker kill --stop-timeout=10

[crosbymichael]

What about things like SIGINT? I usually consider it a terminating signal in most apps

[tonistiigi]

@crosbymichael Yes, thats the default. But application can choose to ignore it or do something else.

[ble]

I think this is where it happens:
Kill command calls into container.KillSig if there is a signal and it is not SIGKILL: https://github.com/docker/docker/blob/267bd51adbf9e344a5d18bb0c7df4e09bcb60668/daemon/kill.go#L25

container.KillSig calls monitor.ExitOnNext()
https://github.com/docker/docker/blob/267bd51adbf9e344a5d18bb0c7df4e09bcb60668/daemon/container.go#L714

[ble]

But it is more a "decide what the behavior should be" kinda bug than a "definitely wrong" bug.

[tonistiigi]

@ble @crosbymichael

In response to #12727, I'm proposing the following syntax:

docker kill -s SIGNAL --stop-timeout=N

If the signal is one of SIGKILL, SIGTERM, SIGINT then the --stop-timeout will default to 10 seconds. For any other signal it will default to 0.

If stop-timeout is 0, the command will be equal to sending kill -s to the containers PID1. So no extra logic for handling the restarts. If you send docker kill -s KILL --stop-timeout=0 the container will be killed and restart monitor will take over and restart the container.

If stop-timeout is greater than 0 then after sending the signal, command will wait for the container to stop(docker kill command will not return instantly as it does now). If the container stops before the timeout is reached the restart monitor will not take over and docker kill command returns cleanly. Should the container not stop within the timeout, docker kill will return with an error code and message. If container stops after that, the restart monitor will take over again.

Problem with the current solution is that although it provides defaults that work most of the time it leaves edge-cases that are impossible to handle correctly. #12727 provides better defaults but still leaves cases that can't be handled. Not only the problem I brought up in the comments there - also for example when my container uses a custom signal to do a clean shutdown, I wouldn't be able to shut down a container with any signal except the ones that are whitelisted.

This solution still maintains good defaults and adds a way to handle all the edge-cases(I hope). Of course, as a counter-argument, the logic is quite complicated to grasp.

[ble]

@tonistiigi +1

I'm not super knowledgeable about the project, but it certainly seems like one might want to be able to determine whether or not a container had stopped on the basis of docker kill exit code.

[visualphoenix]

Can we get this moved back to 1.7.x?

[cpuguy83]

We now have stopsignal support, we can probably look at this to determine if the restart policy should be disabled.

[markgoddard]

I am still seeing this issue in Docker CE 19.0.3:

docker run --stop-signal SIGTERM --restart=unless-stopped -d --name sleepy centos:7 bash -c "trap \"\" SIGHUP; sleep infinity"
docker ps -a | grep sleepy
docker kill --signal HUP sleepy
71f79e8b7c5d        centos:7                                           "bash -c 'trap \"\" SI…"   5 seconds ago       Up 4 seconds                            sleepy
systemctl restart docker
docker ps -a | grep sleepy
4e14d7f92eff        centos:7                                           "bash -c 'trap \"\" SI…"   51 seconds ago      Exited (137) 31 seconds ago                       sleepy

I would expect the container to be running after restarting docker.