fabiolb / fabio

Consul Load-Balancing made simple
https://fabiolb.net
MIT License
7.26k stars 617 forks source link

Change the shutdown procedure to deregister fabio from the registry and then shutdown the proxy #908

Closed martinivanov closed 1 year ago

martinivanov commented 1 year ago

The current shutdown procedure shuts down all proxies and then de-registers Fabio from the service registry. This may cause some requests to fail while a Fabio instance is shutting down. This PR reverses the order and introduces a de-register grace period, causing Fabio to wait for a configurable period after de-registering from a registry and then shutting down the proxies. This way no new requests will be sent to the stopping instances, while giving time to running requests to finish.

CLAassistant commented 1 year ago

CLA assistant check
All committers have signed the CLA.

nathanejohnson commented 1 year ago

I like the idea of changing the shutdown procedure order. As for the configurable grace period, there is already a proxy.shutdownwait .

https://fabiolb.net/ref/proxy.shutdownwait/

Would that solve your issue?

martinivanov commented 1 year ago

It doesn't serve the same purpose. The shutdownwait gives time to currently in-flight requests/connections finish, while new ones are declined. In the best case proxy.shutdownwait and the grace period will work together for pretty much 0 dropped requests. The main point is that some requests may still reach Fabio after it de-registers from Consul for example and by not waiting for some time they would be declined due to the proxy being shut down.

nathanejohnson commented 1 year ago

So after digging through the code, it looks like tcp, grpc, and https+tcp+sni proxies will all wait until the context expires before returning. However, the http proxy does not, it will wait at most up to the context deadline but if all listeners finish with connections before that, it returns early. So as much as I hate the inconsistency here in fabio, I don't think it's worth it to change existing behavior.

So if you could add some documentation to

1) the fabio.properties file in the comments, following the other examples in there 2) add a markdown document with similar documentation inside of docs/content/ref

I will get this merged. Thank you!

martinivanov commented 1 year ago

Done!

nathanejohnson commented 1 year ago

Thank you for your contribution!