Add '-deregister-on-stop' option

[mcclurmc]

Currently registrator deregisters the service once the container dies, but in some cases that might be too late. The -deregister-on-stop option tells registrator to deregister the service once docker receives either the stop or kill commands, but before the container actually dies.

This gives the service in the container a chance to clean up after itself after receiving a SIGTERM, without the chance that it will receive any new requests before it actually dies.

Note that I think this is probably a bug in the Docker events documentation, and that the intended behavior of registrator is to deregister the service as soon as Docker receives the instruction to stop the container. This would be the "kill" event, not the "die" event, which is the opposite of what the documentation says.

If my change does bring registrator inline with the intended behavior, then it might be better to make it the default behavior, and not guard it behind a flag. I only added the flag to keep this change backwards compatible.

[mcclurmc]

Sorry for raising this PR without any prior development/design discussion. I'll be happy to work with the maintainers to get this PR in good shape to merge if there are any code review comments.

[JanAhrens]

Thanks for implementing this @mcclurmc. We're facing the same problem at the moment and would love to have this feature.

Our applications are deployed using Amazon EC2 Container Service (ECS). On updates, ECS will shutdown the docker container (using docker stop). Our applications then use the SIGTERM signal to clean up. This takes on average 500 ms. During this time the instance is registered in Consul and still receives requests. Those requests fail of course, because the application is already busy shutting down. Having the "deregister on docker-stop" would fix this for us.

[mcclurmc]

Thanks for the thumbsup, @JanAhrens. We're going to be carrying this fork until we can merge this change upstream. We've also updated dumb-init to help us rewrite SIGTERM to SIGQUIT (or any arbitrary signals, for that matter) for our Unicorn workers, which don't handle SIGTERM gracefully. I figure other registrator users might be interested in this: Yelp/dumb-init#83

[mattatcha]

This code will cause unexpected results with a command like docker kill -s=SIGHUP some-container. docker kill will emit a kill event regardless of the signal sent.

Here is an issue regarding the events documentation moby/moby#21331.

[mcclurmc]

Oh I see, @MattAitchison. So it sounds like this could still be a good addition as an option, but it shouldn't be made the default behavior. Running docker kill -s=SIGHUP is a bit of a corner case, and I would imagine that the way many people run containers with registrator (especially inside orchestration frameworks), that this isn't something we'd need to worry about 90% of the time.

@MattAitchison, given that this option could be very useful to people who aren't sending arbitrary signals to their containers via docker kill, which is likely the majority case, do you think this is still a valid feature request?

[mcclurmc]

Alternatively, we could further restrict deregistration in this case by checking whether the signal was 15, and only deregistering then. Thoughts?

[mcclurmc]

For the above suggestion, note that we can only determine the signal in Docker API version 1.22, which corresponds to Docker version 1.10, so this might not be a viable solution if we need to support previous Docker versions.

[mcclurmc]

Hi @MattAitchison. Do you have any additional feedback for this PR, or any replies to my comments above?

[JanAhrens]

@josegonzalez: I saw that you added some labels to this PR and believe that's a good sign 😃
Obviously there were some discussions about this. Is there anything that you can share? We're also very interested in having this issue solved.

[josegonzalez]

Right now im just categorizing things so that this weekend I can go through and figure out whats can be merged, is duplicate, etc.

@MattAitchison I'm 👍 on requiring a new docker version as well as making this default, thoughts?

[shidel-dev]

Hi. We are trying to use consul and registrator to rewrite nginx configs using consul template. I do not think that registrator is doing what it advertises, as it is actually waiting till the container exits to deregister.

This pull request would solve our issue, and make registrator work as expected for us.

[shidel-dev]

I do think that this should be the default, and not behind a flag. I can understand using a flag if people are relying on the old behavior though.

[sbuettner]

@josegonzalez Are there any plans for a merge of this important PR?

[mattatcha]

Would it be possible to add an option to the existing deregister flag instead of adding a new one?

[yteraoka]

This change also break compatibility ?

diff --git a/registrator.go b/registrator.go
index b76dc94..c7827e0 100644
--- a/registrator.go
+++ b/registrator.go
@@ -172,6 +172,10 @@ func main() {
                        go b.Add(msg.ID)
                case "die":
                        go b.RemoveOnExit(msg.ID)
+               case "kill":
+                       if signal, ok := msg.Actor.Attributes["signal"]; ok && signal == "15" {
+                               go b.RemoveOnExit(msg.ID)
+                       }
                }
        }

[kronos]

Hello, any plan on this issue? We really need this feature in our project.