Wait for zero in-flight requests before terminating realtime proxy

[deliahu]

No description provided.

[miguelvr]

I think we can achieve the same without adding an endpoint. You can simply wait for in-flight requests when the proxy gets the SIGTERM signal and before calling server.Shutdown()

[miguelvr]

I think we can achieve the same without adding an endpoint. You can simply wait for in-flight requests when the proxy gets the SIGTERM signal and before calling server.Shutdown()

Ref to the code:

cortex/cmd/proxy/main.go

Lines 172 to 189 in 16ffd08

	select {
	case err = <-errCh:
	exit(log, errors.Wrap(err, "failed to start proxy server"))
	case <-sigint:
	// We received an interrupt signal, shut down.
	log.Info("Received TERM signal, handling a graceful shutdown...")
	for name, server := range servers {
	log.Infof("Shutting down %s server", name)
	if err = server.Shutdown(context.Background()); err != nil {
	// Error from closing listeners, or context timeout:
	log.Warnw("HTTP server Shutdown Error", zap.Error(err))
	telemetry.Error(errors.Wrap(err, "HTTP server Shutdown Error"))
	}
	}
	log.Info("Shutdown complete, exiting...")
	}
	}

[deliahu]

@miguelvr yeah, that would be cleaner!

I just tried adding it there, but it didn't work as we expected. So I dug into it, and realized that we weren't handling the TERM signal (just INT). After listening for TERM too, checking the breaker's in-flight count was no longer necessary, since server.Shutdown() seems to handle this automatically.

What are your thoughts on the diff now?

[tfriedel]

Thanks for the PR! I built the images and tried my test (route which sleeps 1 sec). Contrary to the previous verion, deleting a pod did not result in 503s. Good job!

[deliahu]

@tfriedel awesome, thanks for bringing this to our attention, and for confirming that the fix is working as intended!

I'm working on a separate PR now to support custom preStop commands.