apiserver: graceful termination with new option shutdown-send-retry-after #101257

tkashem · 2021-04-19T20:17:12Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

Introduce a new server run option shutdown-send-retry-after

If true the HTTP Server will continue listening until all non longrunning request(s) in flight 
have been drained, during this window all incoming requests will be rejected with
a status code 429 and a 'Retry-After' response header.

The default value is set to false to maintain current behavior.

With this option, we now have two modes of graceful termination:

shutdown-send-retry-after=true: we initiate shutdown of the HTTP Server when all non longrunning requests in-flight have been drained.
shutdown-send-retry-after=false: we initiate shutdown of the HTTP Server as soon as shutdown-delay-duration has elapsed. (This is the current behavior)

The following happens with shutdown-send-retry-after=true:

Do not call http Server.Shutdown immediately after ShutdownDelayDuration elapses. Rather, wait for all non longrunning requests to drain. (the requests that are being counted by WithWaitGroup filter)
Immediately after ShutdownDelayDuration elapses, any new incoming request should be answered with a 429 and the following headers:
Connection: close and
Retry-After: N

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

a new server run option 'shutdown-send-retry-after'  has been introduced. If true the HTTP Server
will continue listening until all non longrunning request(s) in flight have been drained, during this window all 
incoming requests will be rejected with a status code 429 and a 'Retry-After' response header.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot · 2021-04-19T20:17:14Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

tkashem · 2021-04-19T20:17:43Z

/test all

tkashem · 2021-04-19T20:19:06Z

/assign @smarterclayton

smarterclayton · 2021-04-19T21:34:45Z

Hrm, I'd prefer 429 - try again (every request after shutdown we reject is " come back later) and connection close ensures that.

tkashem · 2021-04-19T22:01:32Z

/test pull-kubernetes-e2e-gce-ubuntu-containerd

fedebongio · 2021-04-20T20:12:52Z

/triage accepted

sttts · 2021-07-15T08:29:20Z

staging/src/k8s.io/apiserver/pkg/server/filters/with_retry_after.go

+
+func (f retryAfterConditionFunc) ShouldRespondWithRetryAfter() (*retryAfterParams, bool) {
+	return f()
+}


odd construct. Just use retryAfterConditionFunc() directly. No need for a method.

sttts · 2021-07-15T08:29:34Z

staging/src/k8s.io/apiserver/pkg/server/filters/with_retry_after.go

+
+func withRetryAfter(handler http.Handler, excludeFn excludeFunc, retryAfterFn retryAfterConditionFunc) http.Handler {
+	return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
+		retryAfterParams, sendRetryAfter := retryAfterFn.ShouldRespondWithRetryAfter()


retryAfterFn()

tkashem · 2021-07-16T02:46:53Z

/retest

p0lyn0mial · 2021-07-19T14:02:49Z

staging/src/k8s.io/apiserver/pkg/server/genericapiserver.go

+		//   net/http waits for 1s for the peer to respond to a GO_AWAY frame, so
+		//   we should wait for a minimum of 2s
+		stopHttpServerCh = drainedCh.Signaled()
+		shutdownTimeout = 2 * time.Second


the long-running connections should have a chance to terminate gracefully.
with shutdownTimeout = 2 * time.Second this is impossible.

sttts · 2021-08-10T14:51:44Z

staging/src/k8s.io/apiserver/pkg/server/filters/with_retry_after.go

+// NOTE: both WithRetryAfter and WithWaitGroup must use the same exact isRequestExemptFunc 'isRequestExemptFromRetryAfter,
+// otherwise SafeWaitGroup might wait indefinitely and will prevent the server from shutting down gracefully.
+func WithRetryAfter(handler http.Handler, shutdownDelayDurationElapsedCh <-chan struct{}) http.Handler {
+	return withRetryAfter(handler, isRequestExemptFromRetryAfter, newRetryAfterFunc(shutdownDelayDurationElapsedCh))


nit: move the isRequestExemptFromRetryAfter comment above this line, out of the func doc

sttts · 2021-08-10T14:55:16Z

staging/src/k8s.io/apiserver/pkg/server/filters/with_retry_after.go

+
+func withRetryAfter(handler http.Handler, isRequestExemptFn isRequestExemptFunc, shouldRespondWithRetryAfterFn shouldRespondWithRetryAfterFunc) http.Handler {
+	return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
+		retryAfterParams, sendRetryAfter := shouldRespondWithRetryAfterFn()


params, send := .... The func is about retryAfter. No need for fully qualified var names.

sttts · 2021-08-10T14:56:57Z

staging/src/k8s.io/apiserver/pkg/server/filters/with_retry_after.go

+// newRetryAfterFunc builds a shouldRespondWithRetryAfterFunc that starts
+// sending Retry-After response to all incoming requests once the shut down
+// delay duration (ShutdownDelayDuration) has elapsed.
+func newRetryAfterFunc(shutdownDelayDurationElapsedCh <-chan struct{}) shouldRespondWithRetryAfterFunc {


haven't checked where this is used outside of WithRetryAfter. Just inline this there. This abstraction does not help, but rather distributes complexity.

sttts · 2021-08-10T14:57:30Z

staging/src/k8s.io/apiserver/pkg/server/filters/with_retry_after.go

+// NOTE: both 'WithRetryAfter' and 'WithWaitGroup' filters should use this function
+// to exempt the set of requests from being rejected or tracked.
+func isRequestExemptFromRetryAfter(r *http.Request) bool {
+	if isKubeApiserverUserAgent(r) || hasExemptPathPrefix(r) {


return isKubeApiserverUserAgent(r) || hasExemptPathPrefix(r)

add a new mode for graceful termination with the new server run option 'shutdown-send-retry-after' - shutdown-send-retry-after=true: we initiate shutdown of the HTTP Server when all in-flight request(s) have been drained. during this window all incoming requests are rejected with status code 429 and the following response headers: - 'Retry-After: N' - client should retry after N seconds - 'Connection: close' - tear down the TCP connection - shutdown-send-retry-after=false: we initiate shutdown of the HTTP Server as soon as shutdown-delay-duration has elapsed. This is in keeping with the current behavior.

sttts · 2021-08-10T20:09:44Z

/lgtm
/approve

k8s-ci-robot · 2021-08-10T20:10:20Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sttts, tkashem

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~staging/src/k8s.io/apiserver/OWNERS~~ [sttts]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/bug Categorizes issue or PR as related to a bug. labels Apr 19, 2021

k8s-ci-robot added area/apiserver sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Apr 19, 2021

k8s-ci-robot requested review from jpbetz and smarterclayton April 19, 2021 20:18

k8s-ci-robot assigned smarterclayton Apr 19, 2021

tkashem changed the title ~~[WIP]: graceful termination~~ [WIP]: graceful termination with delayed server shutdown Apr 19, 2021

k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Apr 20, 2021

tkashem force-pushed the graceful-termintaion branch from c8f9918 to 6aff959 Compare May 6, 2021 21:42

tkashem changed the title ~~[WIP]: graceful termination with delayed server shutdown~~ [WIP]: [DO NOT REVIEW] graceful termination with delayed server shutdown May 11, 2021

tkashem force-pushed the graceful-termintaion branch from 6aff959 to a08c822 Compare May 11, 2021 20:24

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 11, 2021

tkashem force-pushed the graceful-termintaion branch 2 times, most recently from ffc30cb to f8c0a38 Compare May 12, 2021 18:32

sttts reviewed Jul 15, 2021

View reviewed changes

tkashem force-pushed the graceful-termintaion branch from a735bb2 to 15f66e4 Compare July 15, 2021 18:58

tkashem force-pushed the graceful-termintaion branch 4 times, most recently from 962e00d to e305345 Compare July 16, 2021 19:54

tkashem changed the title ~~apiserver: graceful termination with new option keep-listening-during-shutdown~~ apiserver: graceful termination with new option shutdown-send-retry-after Jul 16, 2021

tkashem mentioned this pull request Jul 16, 2021

terminate long-running requests on termination #103296

Closed

p0lyn0mial reviewed Jul 19, 2021

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 6, 2021

tkashem force-pushed the graceful-termintaion branch from e305345 to 082e0e5 Compare August 9, 2021 15:01

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 9, 2021

sttts reviewed Aug 10, 2021

View reviewed changes

apiserver: rename test variables

e838173

tkashem force-pushed the graceful-termintaion branch from fdd44c8 to a8a90f9 Compare August 10, 2021 19:50

tkashem force-pushed the graceful-termintaion branch from a8a90f9 to 3182b69 Compare August 10, 2021 19:53

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 10, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 10, 2021

k8s-ci-robot merged commit 938d31e into kubernetes:master Aug 10, 2021

k8s-ci-robot added this to the v1.23 milestone Aug 10, 2021

openshift-ci-robot mentioned this pull request Aug 17, 2021

Bug 1994643: UPSTREAM: 104281: send retry-after until the apiserver is ready openshift/kubernetes#889

Merged

tkashem mentioned this pull request Dec 2, 2021

Bug 2019215: enable shutdown-send-retry-after for apiserver openshift/cluster-openshift-apiserver-operator#486

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

apiserver: graceful termination with new option shutdown-send-retry-after #101257

apiserver: graceful termination with new option shutdown-send-retry-after #101257

tkashem commented Apr 19, 2021 •

edited

k8s-ci-robot commented Apr 19, 2021

tkashem commented Apr 19, 2021

tkashem commented Apr 19, 2021

smarterclayton commented Apr 19, 2021

tkashem commented Apr 19, 2021

fedebongio commented Apr 20, 2021

sttts Jul 15, 2021

tkashem Aug 9, 2021

sttts Jul 15, 2021

tkashem commented Jul 16, 2021

p0lyn0mial Jul 19, 2021

sttts Aug 10, 2021

sttts Aug 10, 2021

sttts Aug 10, 2021

sttts Aug 10, 2021

sttts commented Aug 10, 2021

k8s-ci-robot commented Aug 10, 2021

apiserver: graceful termination with new option shutdown-send-retry-after #101257

apiserver: graceful termination with new option shutdown-send-retry-after #101257

Conversation

tkashem commented Apr 19, 2021 • edited

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot commented Apr 19, 2021

tkashem commented Apr 19, 2021

tkashem commented Apr 19, 2021

smarterclayton commented Apr 19, 2021

tkashem commented Apr 19, 2021

fedebongio commented Apr 20, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkashem commented Jul 16, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sttts commented Aug 10, 2021

k8s-ci-robot commented Aug 10, 2021

tkashem commented Apr 19, 2021 •

edited