✨ Add support for controller-manager webhook shutdown delay #2601
+13
−0
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -101,6 +101,10 @@ type Options struct { | |
|
|
||
| // WebhookMux is the multiplexer that handles different webhooks. | ||
| WebhookMux *http.ServeMux | ||
|
|
||
| // ShutdownDelay delays server shutdown to wait for clients to stop opening | ||
| // new connections before closing server listeners. Defaults to 0. | ||
| ShutdownDelay time.Duration | ||
| } | ||
|
|
||
| // NewServer constructs a new webhook.Server from the provided options. | ||
|
|
@@ -246,6 +250,15 @@ func (s *DefaultServer) Start(ctx context.Context) error { | |
| idleConnsClosed := make(chan struct{}) | ||
| go func() { | ||
| <-ctx.Done() | ||
|
|
||
| // Disable HTTP keep-alives to close persistent connections after next | ||
| // HTTP request. Clients may reconnect until routes are updated and | ||
| // server listeners are closed however this should start gradually | ||
| // migrating clients to server instances that are not about to shutdown | ||
| srv.SetKeepAlivesEnabled(false) | ||
|
Comment on lines
+254
to
+258
|
||
| // Wait before shutting down webhook server | ||
| time.Sleep(s.Options.ShutdownDelay) | ||
|
Comment on lines
+259
to
+260
There was a problem hiding this comment. Why is this needed? Reading the intent of the PR, there is a context with a timeout that will be given to There was a problem hiding this comment. Although This comment speaks more about how kube-proxy iptables are involved in webhook connectivity: #2601 (comment) |
||
|
|
||
| log.Info("Shutting down webhook server with timeout of 1 minute") | ||
|
|
||
| ctx, cancel := context.WithTimeout(context.Background(), 1*time.Minute) | ||
|
|
||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Open question:
Would it make sense to set a > 0 default, eg. 15s which is less than the 30 seconds default graceful shutdown delay?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a tricky one for sure, but because it's so specific to the Kubernetes cluster you're running on it doesn't feel right to align to any particular cluster or cloud provider (e.g. I'm not sure what value EKS and AKS use for example).
IMO we should keep it at 0 for backwards compatibility and to ensure that connection draining 'doesn't work' consistently across clusters by default. I think this is also more likely to encourage applications that care a lot about this (e.g. Gatekeeper) to expose a flag to change this to make it easier to reconfigure for the cluster you're running on (i.e. push the decision onto users of controller-runtime since an optimal default isn't clear).
No problems with changing to 15 seconds by default though if you were leaning towards that (that would be better for us anyway because we're using GKE 😄). We could alternatively align to the kube-proxy default of 1 second? But on GKE at least this won't help much.