Add request accepting grace period delaying graceful shutdown. #1971

timoreimann · 2017-08-18T22:24:12Z

The new grace period defines how long Traefik should continue accepting requests prior to shutting down all connection listeners. It preceeds the existing (HTTP shutdown) grace period and allows for a full-circle graceful termination by expecting downstream load-balancers to take Traefik out of rotation in time, without the need for rather complex and external request draining procedures.

Immediate beneficiaries are providers such as Kubernetes and Marathon which expect in-cluster applications to exhibit the described termination behavior.

This PR addresses one of the two necessary TODOs for #1942. However, it also stands useful on its own.

timoreimann · 2017-08-18T22:27:44Z

@emilevauge I remember you weren't completely convinced of this feature, so I went ahead and implemented it to help giving the full picture.

See the linked issue for some extra rationale.

emilevauge

Design review LGTM :)
Like what we did with RespondingTimeouts& ForwardingTimeouts, it may be interesting to group both reqAcceptGraceTimeOut & GraceTimeOut into a struct (something like LifeCyleTimeouts).

timoreimann · 2017-09-12T10:19:32Z

@emilevauge

Like what we did with RespondingTimeouts& ForwardingTimeouts, it may be interesting to group both reqAcceptGraceTimeOut & GraceTimeOut into a struct (something like LifeCyleTimeouts).

Sounds very reasonable to me. 👍 I'll work on making the necessary adjustments.

timoreimann · 2017-09-15T12:35:08Z

Still WIP, things aren't done yet.

timoreimann · 2017-09-15T16:02:57Z

Holding back the bot as we only want to merge this after #2114 landed in master, after we which we need to update the integration test this PR adds with the new generic show-logs-on-integration-test-failure approach.

m3co-code · 2017-09-19T08:00:49Z

configuration/configuration.go

 )

 // GlobalConfiguration holds global configuration (with providers, etc.).
 // It's populated from the traefik configuration file passed as an argument to the binary.
 type GlobalConfiguration struct {
-	GraceTimeOut              flaeg.Duration          `short:"g" description:"Duration to give active requests a chance to finish before Traefik stops"`
+	LifeCycle                 *LifeCycle              `description:"Timeouts influencing the server life cycle"`


Do you expect other settings than timeouts inside of the LifeCycle struct? If not do we want to rename it to LifeCycleTimeouts for consistency reasons?

We might, I don't know.

The likely more important point for me is that the other approach stutters: LifeCycleTimeouts.GraceTimeOut has one timeout too many, so it should either be removed from the struct name or the parameters IMHO. I decided for the former as that allows us to extend the struct in the future without the need for another name update.

I see your point. SGTM.

m3co-code

LGTM!

m3co-code · 2017-09-19T14:43:22Z

docs/configuration/commons.md

+
+# Duration to keep accepting requests prior to initiating the graceful
+# termination period (as defined by the `graceTimeOut` option). This
+# option is meant to give downstream load-balancer sufficient time to


It should be either: "a downstream load-balancer" or "downstream load-balancers".

Pluralized for the better.

timoreimann · 2017-09-19T15:28:59Z

This one is ready for review again (unless someone wants me to rebase first).

juliens

👏

juliens · 2017-09-21T07:13:54Z

integration/basic_test.go

+		Server string
+	}{whoami})
+	defer os.Remove(file)
+	cmd, output := s.cmdTraefik(withConfigFile(file))


Can you use the new version of traefikCmd with defer display (#2114)

juliens · 2017-09-21T07:21:12Z

configuration/configuration.go

+// LifeCycle contains configurations relevant to the lifecycle (such as the
+// shutdown phase) of Traefik.
+type LifeCycle struct {
+	ReqAcceptGraceTimeOut flaeg.Duration `description:"Duration to keep accepting requests before Traefik initiates the graceful shutdown procedure"`


Why not RequestAcceptGraceTimeout ?

Sure, why not -- done.

juliens · 2017-09-21T07:21:20Z

integration/basic_test.go

+	select {
+	case err := <-waitErr:
+		c.Assert(err, checker.IsNil)
+	case <-time.After(10 * time.Second):


5s (sleep) + 10s (after) = 15s
reqAcceptGraceTimeOut (10s) + defaultGraceTimeout(10s) = "20s"

The grace period shouldn't be used: We send the last request to the server at t =~ 5 when we are halfway into the request accept grace timeout. It should finish at t = 5 + delta, so at t = 10 no request should be left that needs to get served.

I left an extra comment to explain the chosen timeout.

The new grace period defines how long Traefik should continue accepting requests prior to shutting down all connection listeners. It preceeds the existing (HTTP shutdown) grace period and allows for a full-circle graceful termination by expecting downstream load-balancers to take Traefik out of rotation in time, without the need for rather complex and external request draining procedures. Immediate beneficiaries are providers such as Kubernetes and Marathon which expect in-cluster applications to exhibit the described termination behavior.

Former name stems from a previous (never published) naming schema.

Doesn't really make sense since we shut down the server for SIGINT gracefully as well.

timoreimann · 2017-09-25T16:10:20Z

@juliens et. al: This is ready for review again. I have also adjusted for #2114.

m3co-code · 2017-09-26T07:46:18Z

My LGTM stays 👍

nmengin

LGTM 🕙 👏

timoreimann added kind/enhancement a new or improved feature. status/1-needs-design-review labels Aug 18, 2017

traefiker added the size/M label Aug 18, 2017

timoreimann force-pushed the add-request-accepting-grace-period branch from 8904a1b to 882c473 Compare August 18, 2017 22:24

m3co-code requested review from ldez and removed request for ldez August 22, 2017 10:31

timoreimann force-pushed the add-request-accepting-grace-period branch from 882c473 to 40f6b39 Compare August 24, 2017 08:26

timoreimann mentioned this pull request Aug 24, 2017

Add readiness endpoint failing on shutdown. #2003

Closed

emilevauge reviewed Sep 12, 2017

View reviewed changes

emilevauge added status/2-needs-review and removed status/1-needs-design-review labels Sep 12, 2017

ldez added this to the 1.5 milestone Sep 12, 2017

timoreimann added the WIP label Sep 12, 2017

timoreimann force-pushed the add-request-accepting-grace-period branch from 4980c33 to 8b5030f Compare September 12, 2017 14:50

timoreimann force-pushed the add-request-accepting-grace-period branch 3 times, most recently from 7e27abf to d51fe84 Compare September 15, 2017 15:46

timoreimann added the bot/no-merge label Sep 15, 2017

ldez removed the status/2-needs-review label Sep 15, 2017

timoreimann force-pushed the add-request-accepting-grace-period branch from d51fe84 to edffad1 Compare September 15, 2017 22:13

m3co-code reviewed Sep 19, 2017

View reviewed changes

m3co-code approved these changes Sep 19, 2017

View reviewed changes

timoreimann added status/2-needs-review and removed WIP labels Sep 19, 2017

juliens requested changes Sep 21, 2017

View reviewed changes

traefiker added the contributor/waiting-for-corrections label Sep 21, 2017

timoreimann added 8 commits September 25, 2017 17:04

Rename variable req{Term,Accept}GraceTimeout.

b8c8d3d

Former name stems from a previous (never published) naming schema.

Remove restriction on syscall.SIGTERM.

50c6e1b

Doesn't really make sense since we shut down the server for SIGINT gracefully as well.

Group new parameter and graceful timeout into dedicated struct.

121d782

Update documentation.

a3a102f

Fix grammar deficiency.

e74537b

Use existing method to display Traefik log in case of test failure.

9a2b814

Add rationale to test timeout.

8f84490

timoreimann force-pushed the add-request-accepting-grace-period branch from 5ccbf12 to 11a6f02 Compare September 25, 2017 16:07

timoreimann removed bot/no-merge contributor/waiting-for-corrections labels Sep 25, 2017

juliens approved these changes Sep 26, 2017

View reviewed changes

nmengin approved these changes Sep 26, 2017

View reviewed changes

nmengin added status/3-needs-merge and removed status/2-needs-review labels Sep 26, 2017

traefiker added the bot/need-human-merge label Sep 26, 2017

Rename reqAcceptGraceTimeOut to requestAcceptGraceTimeout.

dd1a108

ldez force-pushed the add-request-accepting-grace-period branch from 11a6f02 to dd1a108 Compare September 26, 2017 08:06

ldez removed the bot/need-human-merge label Sep 26, 2017

traefiker merged commit 1c98a9a into traefik:master Sep 26, 2017

traefiker removed the status/3-needs-merge label Sep 26, 2017

ravilr mentioned this pull request Mar 6, 2018

Enable graceful rotation of traefik servers behind a load balancer #2969

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add request accepting grace period delaying graceful shutdown. #1971

Add request accepting grace period delaying graceful shutdown. #1971

timoreimann commented Aug 18, 2017

timoreimann commented Aug 18, 2017

emilevauge left a comment

timoreimann commented Sep 12, 2017

timoreimann commented Sep 15, 2017

timoreimann commented Sep 15, 2017

m3co-code Sep 19, 2017

timoreimann Sep 19, 2017

m3co-code Sep 19, 2017

m3co-code left a comment

m3co-code Sep 19, 2017

timoreimann Sep 19, 2017

timoreimann commented Sep 19, 2017 •

edited

juliens left a comment

juliens Sep 21, 2017

timoreimann Sep 25, 2017

juliens Sep 21, 2017

timoreimann Sep 25, 2017

juliens Sep 21, 2017

timoreimann Sep 25, 2017 •

edited

timoreimann commented Sep 25, 2017

m3co-code commented Sep 26, 2017

nmengin left a comment

Add request accepting grace period delaying graceful shutdown. #1971

Add request accepting grace period delaying graceful shutdown. #1971

Conversation

timoreimann commented Aug 18, 2017

timoreimann commented Aug 18, 2017

emilevauge left a comment

Choose a reason for hiding this comment

timoreimann commented Sep 12, 2017

timoreimann commented Sep 15, 2017

timoreimann commented Sep 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m3co-code left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timoreimann commented Sep 19, 2017 • edited

juliens left a 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

timoreimann Sep 25, 2017 • edited

Choose a reason for hiding this comment

timoreimann commented Sep 25, 2017

m3co-code commented Sep 26, 2017

nmengin left a comment

Choose a reason for hiding this comment

timoreimann commented Sep 19, 2017 •

edited

timoreimann Sep 25, 2017 •

edited