Bug 1809665: Start graceful shutdown on SIGTERM #94

smarterclayton · 2020-03-01T06:53:13Z

Graceful shutdown in Kubernetes requires:

Wait for the frontend to be taken out of rotation by kube-proxy (fast, 5s or so) and external load balancers (slow, 45s is reasonable across all clouds) so that new connections are not allowed
Stop accepting new connections
Drain existing connections within some timeout
Exit with 0 if drain before the timeout, exit with 1 after the timeout (connections still outstanding)

This PR updates the router to implement the above logic.

When the main process recieves SIGTERM or SIGINT, wait ROUTER_GRACEFUL_SHUTDOWN_DELAY (default: 45) seconds and then signal the reload script with ROUTER_SHUTDOWN=true that a graceful termination is requested. The reload-haproxy script then invokes USR1 on all child haproxy processes and waits ROUTER_MAX_SHUTDOWN_TIMEOUT or MAX_RELOAD_WAIT_TIME (default 30) seconds before invoking TERM on the child processes. If TERM is invoked the script exits with 1, indicating that not all processes completed their work.

Clients with long running requests should set ROUTER_MAX_SHUTDOWN_TIMEOUT as appropriate to ensure all connections exit cleanly.

smarterclayton · 2020-03-02T03:05:28Z

Depends on openshift/cluster-ingress-operator#366 (we're getting aborted early)

smarterclayton · 2020-03-02T17:07:46Z

Testing upgrade from this PR + 366 to itself (changed images only), https://prow.svc.ci.openshift.org/view/gcs/origin-ci-test/logs/release-openshift-origin-installer-e2e-aws-upgrade/19735 got 2s of disruption in the console, which should be fixed by openshift/console-operator#385

smarterclayton · 2020-03-02T19:12:59Z

Remaining change here, pod needs to start reporting not ready as soon as it is marked deleted. Otherwise I'm testing the logging and output.

knobunc · 2020-03-02T19:25:42Z

This approach, and the code, looks good to me. I'm avoiding saying lgtm in case you intend to add the not-ready reporting before it gets merged.

smarterclayton · 2020-03-02T21:42:56Z

Yeah, I want to see all three of the PRs working together before I merge any one.

smarterclayton · 2020-03-02T21:43:00Z

/hold

smarterclayton · 2020-03-03T03:27:32Z

/hold cancel

Verified the health check during shutdown with:

$ oc rsh -n openshift-ingress deploy/router-default
sh-4.2$ kill 1
sh-4.2$ curl http://localhost:1936/healthz/ready
[+]backend-http ok
[+]has-synced ok
[-]process-running failed: reason withheld
healthz check failed

Verified during upgrade route ingresses remain available https://prow.svc.ci.openshift.org/view/gcs/origin-ci-test/logs/release-openshift-origin-installer-e2e-aws-upgrade/19806 when testing all three PRs together.

This is ready for review.

pkg/router/shutdown/shutdown.go

Miciah · 2020-03-03T04:17:43Z

pkg/router/shutdown/shutdown.go

+// which is closed on one of these signals. If a second signal is caught, the program
+// is terminated with exit code 1.
+func SetupSignalHandler() <-chan struct{} {
+	close(onlyOneSignalHandler) // panics when called twice


What could call SetupSignalHandler twice? Is the purpose of this line to guard against future coding errors?

I would say so; and I ran into problems previously:

Revert "Make NewCommandStartNode() handle SIGTERM and SIGINT" origin#19986

Remove signal handler registration from pkg/kubelet kubernetes/kubernetes#63859

@smarterclayton any reason we don't pull an existing implementation?

This is the same code as genericapiserver. It's not appropriate for router to take a dependency on it, and this code is straightforward.

Yes, the panic guards against coding errors.

images/router/haproxy/reload-haproxy

Miciah · 2020-03-03T04:25:43Z

images/router/haproxy/reload-haproxy

+  fi
+  readonly wait_time=${ROUTER_MAX_SHUTDOWN_TIMEOUT:-${MAX_RELOAD_WAIT_TIME:-$max_wait_time}}
+  kill -USR1 $old_pids
+  for i in $( seq 1 $wait_time ); do


Not sure it matters enough to worry about, but the following would be more precise:

stop=$((SECONDS + wait_time)) while ((SECONDS < stop)); do # ...

Hrm, that construct looks much harder to understand for a casual reader. Why is it more precise?

Because the condition is true once $wait_time seconds has passed since the assignment. In contrast, sleeping for 1 second n times fails to account for execution time. It probably does not matter here as the only commands are builtins and pidof, which should be fast; it would matter more for a command like, say, curl, where the loop may spend more time executing commands than sleeping. That is why I say it probably does not matter enough to worry about, and if the for plus seq is easier to read, we can run with that.

Yeah, I'd prefer to use the simpler construct if we don't have a concrete reason to avoid it (waiting slightly longer should not be an issue).

pkg/cmd/infra/router/template.go

Miciah · 2020-03-03T05:55:45Z

pkg/cmd/infra/router/template.go

+		// endpoint out of rotation.
+		delay := getIntervalFromEnv("ROUTER_GRACEFUL_SHUTDOWN_DELAY", 45)
+		log.Info(fmt.Sprintf("Shutdown requested, waiting %s for new connections to cease", delay))
+		time.Sleep(delay)


The thing that sends the signal that triggers the graceful shutdown is the kubelet, when the pod is marked for deletion, right? As soon as a pod is marked for deletion, the pod is removed from endpoints, so it should not be receiving new connections. So once the endpoints controller updates the endpoints in response to the pod's deletion and the service proxy updates in response to the endpoints update, we're really just waiting for already established connections to drain, right? Where does the 45-second delay come from?

No, you're waiting for distributed load balancers to take you out of rotation.

Pod marked for deletion in etcd

Delete notification propagates to all consumers

Consumers stop directing new traffic to the endpoint

1 is fast. 2 may take up to 5-10s depending on load. 3 takes as long as any type of global load balancer in front of the service takes to detect a not ready service (which is (unhealthy checks + 1) * interval check or 32s for GCP). See https://docs.google.com/document/d/1BUmtdTth49V02UZ5EjRvJ92A5vjF8wMJMSdPb1Wz3wQ/edit# for an explanation (that will become part of openshift/enhancements)

pkg/router/template/router.go

openshift-ci-robot · 2020-03-03T16:17:33Z

@smarterclayton: This pull request references Bugzilla bug 1809665, which is invalid:

expected the bug to be in one of the following states: NEW, ASSIGNED, ON_DEV, POST, POST, but it is MODIFIED instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

Bug 1809665: Start graceful shutdown on SIGTERM

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

smarterclayton · 2020-03-03T16:19:21Z

4.4 bug is 1809667 and 4.3 bug is 1809668

smarterclayton · 2020-03-03T16:19:30Z

/cherry-pick release-4.4

openshift-cherrypick-robot · 2020-03-03T16:19:31Z

@smarterclayton: once the present PR merges, I will cherry-pick it on top of release-4.4 in a new PR and assign it to you.

In response to this:

/cherry-pick release-4.4

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

smarterclayton · 2020-03-03T16:19:36Z

/cherry-pick release-4.3

smarterclayton · 2020-03-03T18:12:39Z

Both jobs passed with zero disruption (just verifying I didn't break the graceful shutdown with the changes to reload-haproxy).

smarterclayton · 2020-03-03T18:12:57Z

/retest

smarterclayton · 2020-03-03T19:52:38Z

/retest

smarterclayton · 2020-03-03T21:12:09Z

Hrm, that's super weird. AWS install fails because it can't find the haproxy-config.template. That's scary.

smarterclayton · 2020-03-03T21:12:14Z

/retest

Miciah · 2020-03-03T21:47:37Z

/lgtm

smarterclayton · 2020-03-03T21:47:47Z

/retest

openshift-ci-robot · 2020-03-03T21:48:07Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Miciah, smarterclayton

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:

~~images/router/OWNERS~~ [Miciah,smarterclayton]
~~pkg/cmd/infra/router/OWNERS~~ [Miciah,smarterclayton]
~~pkg/router/OWNERS~~ [Miciah,smarterclayton]

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

openshift-bot · 2020-03-03T22:52:04Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-03-03T23:31:02Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-03-03T23:44:04Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-03-04T00:23:07Z

/retest

Please review the full test history for this PR and help us cut down flakes.

smarterclayton · 2020-03-04T00:28:16Z

/bugzilla refresh

openshift-ci-robot · 2020-03-04T00:28:20Z

@smarterclayton: This pull request references Bugzilla bug 1809665, which is valid. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.5.0) matches configured target release for branch (4.5.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

/bugzilla refresh

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-bot · 2020-03-04T09:56:19Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-03-04T10:08:10Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-03-04T11:52:33Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-03-04T14:55:25Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-cherrypick-robot · 2020-03-04T16:19:22Z

@smarterclayton: new pull request created: #97

In response to this:

/cherry-pick release-4.4

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-cherrypick-robot · 2020-03-04T16:19:29Z

@smarterclayton: new pull request created: #98

In response to this:

/cherry-pick release-4.3

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 1, 2020

openshift-ci-robot requested review from knobunc and pecameron March 1, 2020 06:53

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 1, 2020

smarterclayton force-pushed the signal_shutdown branch 2 times, most recently from 7fc4d56 to 1b7654c Compare March 2, 2020 00:00

knobunc requested review from ironcladlou, frobware, Miciah and danehans and removed request for pecameron March 2, 2020 19:23

smarterclayton force-pushed the signal_shutdown branch from 1b7654c to ecdbae6 Compare March 2, 2020 19:36

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 2, 2020

smarterclayton force-pushed the signal_shutdown branch from ecdbae6 to 636cecd Compare March 3, 2020 02:12

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 3, 2020

Miciah reviewed Mar 3, 2020

View reviewed changes

smarterclayton changed the title ~~Start graceful shutdown on SIGTERM~~ Bug 1809665: Start graceful shutdown on SIGTERM Mar 3, 2020

openshift-ci-robot added the bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. label Mar 3, 2020

openshift-ci-robot assigned Miciah Mar 3, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 3, 2020

openshift-ci-robot added bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. and removed bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. labels Mar 4, 2020

openshift-merge-robot merged commit 0355114 into openshift:master Mar 4, 2020

openshift-cherrypick-robot mentioned this pull request Mar 4, 2020

[release-4.4] Bug 1809667: Start graceful shutdown on SIGTERM #97

Merged

openshift-cherrypick-robot mentioned this pull request Mar 4, 2020

[release-4.3] Bug 1809668: Start graceful shutdown on SIGTERM #98

Closed

openshift-ci-robot mentioned this pull request Apr 13, 2020

Bug 1809665: Re-add pod disruption budget for ingress controllers openshift/cluster-ingress-operator#387

Merged

sgreene570 mentioned this pull request Dec 9, 2020

Bug 1903821: Pass stop channel to RouterControllers #233

Merged

Bug 1809665: Start graceful shutdown on SIGTERM #94

Bug 1809665: Start graceful shutdown on SIGTERM #94

Conversation

smarterclayton commented Mar 1, 2020 • edited

smarterclayton commented Mar 2, 2020 • edited

smarterclayton commented Mar 2, 2020

smarterclayton commented Mar 2, 2020

knobunc commented Mar 2, 2020

smarterclayton commented Mar 2, 2020

smarterclayton commented Mar 2, 2020

smarterclayton commented Mar 3, 2020

Choose a reason for hiding this comment

frobware Mar 3, 2020 • edited

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

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

openshift-ci-robot commented Mar 3, 2020

smarterclayton commented Mar 3, 2020

smarterclayton commented Mar 3, 2020

openshift-cherrypick-robot commented Mar 3, 2020

smarterclayton commented Mar 3, 2020

smarterclayton commented Mar 3, 2020 • edited

smarterclayton commented Mar 3, 2020

smarterclayton commented Mar 3, 2020

smarterclayton commented Mar 3, 2020

smarterclayton commented Mar 3, 2020

Miciah commented Mar 3, 2020

smarterclayton commented Mar 3, 2020

openshift-ci-robot commented Mar 3, 2020

openshift-bot commented Mar 3, 2020

openshift-bot commented Mar 3, 2020

openshift-bot commented Mar 3, 2020

openshift-bot commented Mar 4, 2020

smarterclayton commented Mar 4, 2020

openshift-ci-robot commented Mar 4, 2020

openshift-bot commented Mar 4, 2020

openshift-bot commented Mar 4, 2020

openshift-bot commented Mar 4, 2020

openshift-bot commented Mar 4, 2020

openshift-cherrypick-robot commented Mar 4, 2020

openshift-cherrypick-robot commented Mar 4, 2020

smarterclayton commented Mar 1, 2020 •

edited

smarterclayton commented Mar 2, 2020 •

edited

frobware Mar 3, 2020 •

edited

smarterclayton commented Mar 3, 2020 •

edited