Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

httpserver: Configurable shutdown delay #4906

Merged
merged 5 commits into from
Aug 3, 2022
Merged

httpserver: Configurable shutdown delay #4906

merged 5 commits into from
Aug 3, 2022

Conversation

mholt
Copy link
Member

@mholt mholt commented Jul 23, 2022

This config setting allows scheduling a shutdown for the near future when a config is reloaded or the server is exiting. It blocks config reloads / signals while it waits and there is no cancelling it.

This is useful as it allows the server to continue operating normally before a config is unloaded. This delay happens before the grace period is initiated, meaning that new connections are still accepted during this time just like normal. After the delay completes, the grace period begins and new connections are no longer accepted and the graceful shutdown commences (idle connections closed, active connections closed once idle, etc).

Adds two new placeholders:

  • {http.shutting_down} will return true if the server is shutting down, or false otherwise.
  • {http.time_until_shutdown} will return the duration of the delay that remains if the server is scheduled to shut down. Otherwise it returns nil/empty.

This allows health check endpoints to announce that they will soon be going down so that this instance can be moved out of the rotation or a replacement instance can be spun up. For example:

{
	shutdown_delay 10s
}

...


	handle /health-check {
		@goingDown vars {http.shutting_down} true
		respond @goingDown "Bye-bye in {http.time_until_shutdown}" 503
		respond 200
	}

Note that this behavior is a little wonky for config reloads specifically. During a config change (as opposed to a config unload), two configs are running simultaneously for a time while the old one shuts down. Thus a request to a health check endpoint has the random chance of hitting the old server or the new one, since the grace period has not started yet. I'm trying to decide if that is correct behavior, or whether it should ONLY do this for a final shutdown (i.e. no new/replacement config).

@mxrlkn please try this out! :)

Closes #2843

@mholt mholt added feature ⚙️ New feature or request under review 🧐 Review is pending before merging labels Jul 23, 2022
@mholt mholt added this to the v2.6.0 milestone Jul 25, 2022
@mholt
Copy link
Member Author

mholt commented Aug 1, 2022

This should probably only be enforced for listeners that are not being used by the next configuration (i.e. server is shutting down, not reloading). This can be determined by accessing the count in the usagepool. I think this is currently very unexported but I wonder if we can access it.

@francislavoie
Copy link
Member

francislavoie commented Aug 1, 2022

FWIW I think it would be useful to have a built-in way to check if a shutdown is actually for a reload or not. Right now, I think plugins need to set up their own UsagePool just for that purpose, which is a lot of awkward boilerplate for something that should just be a simple boolean check.

@mholt
Copy link
Member Author

mholt commented Aug 1, 2022

I'm currently refactoring this for a more proper implementation: the placeholders will be per-server (if I can figure that part out), since a config reload might stop some listeners but not others. If any listener is being stopped, then the delay will occur, otherwise it will not.

What's the significance of a config reload? All the clients should care about is whether the listener is going away or not.

@francislavoie
Copy link
Member

For the case of the caddy-exec, it has startup and shutdown flags to indicate when a particular command should be run; you only want to start some attached server/process at startup and stop it on shutdown, no need to do that on every reload. https://github.com/abiosoft/caddy-exec

@mholt
Copy link
Member Author

mholt commented Aug 1, 2022

That's true, but this is only for HTTP server internals. Reload or shutdown will be irrelevant here (either way, it will report a shutdown if the listener is closing, which should be all that matters).

Instead of the entire app being scheduled for shutdown, shutdown will be
announced only for servers whose listeners are closing.
This is regardless of config reload or exit (listeners close during
reload if new config does not listen on that address).

Fix mapping of caddyhttp.Server -> http.Server and http3.Server types.

However this breaks HTTP/3 Alt-Svc because the http3 package uses a
hard-coded address explicitly in configuration, even though
the server can be used for multiple listeners.
Issue: quic-go/quic-go#3486

Overall this is a Very Good Change, I think.
@mholt
Copy link
Member Author

mholt commented Aug 1, 2022

Ok, refactor complete, and I'm actually quite happy with things... much better than before IMO. Now shutdowns are announced per-server and whether it's a process exit or a reload is irrelevant.

This does, unfortunately, break the http3 package setting the Alt-Svc header, because the http3 server is reused for potentially multiple listeners, yet it requires a single hard-coded address or port in its struct. So I opened an issue upstream: quic-go/quic-go#3486

(Edit: No it doesn't, I just made a mistake... fixed it.)

@mholt
Copy link
Member Author

mholt commented Aug 3, 2022

I'm a dum dum. HTTP/3 works just fine with this. The error was in my code, I didn't realize something got messed up in the refactor. Sorry about that Marten! 😅 Thanks for being patient and rigorous.

I'll merge this in a moment.

@mholt mholt removed the under review 🧐 Review is pending before merging label Aug 3, 2022
@mholt mholt merged commit 1960a0d into master Aug 3, 2022
@mholt mholt deleted the shutdown-delay branch August 3, 2022 17:04
@francislavoie francislavoie modified the milestones: v2.6.0, v2.6.0-beta.1 Aug 21, 2022
@mholt mholt modified the milestones: v2.6.0-beta.1, v2.6.0 Sep 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature ⚙️ New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New grace period to continue allowing new connections before shutting down
2 participants