-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Add request accepting grace period delaying graceful shutdown. #1971
Changes from all commits
2901e69
b8c8d3d
50c6e1b
121d782
a3a102f
e74537b
9a2b814
8f84490
dd1a108
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,7 +3,9 @@ package integration | |
import ( | ||
"fmt" | ||
"net/http" | ||
"os" | ||
"strings" | ||
"syscall" | ||
"time" | ||
|
||
"github.com/containous/traefik/integration/try" | ||
|
@@ -101,3 +103,62 @@ func (s *SimpleSuite) TestPrintHelp(c *check.C) { | |
}) | ||
c.Assert(err, checker.IsNil) | ||
} | ||
|
||
func (s *SimpleSuite) TestRequestAcceptGraceTimeout(c *check.C) { | ||
s.createComposeProject(c, "reqacceptgrace") | ||
s.composeProject.Start(c) | ||
|
||
whoami := "http://" + s.composeProject.Container(c, "whoami").NetworkSettings.IPAddress + ":80" | ||
|
||
file := s.adaptFile(c, "fixtures/reqacceptgrace.toml", struct { | ||
Server string | ||
}{whoami}) | ||
defer os.Remove(file) | ||
cmd, display := s.traefikCmd(withConfigFile(file)) | ||
defer display(c) | ||
err := cmd.Start() | ||
c.Assert(err, checker.IsNil) | ||
defer cmd.Process.Kill() | ||
|
||
// Wait for Traefik to turn ready. | ||
err = try.GetRequest("http://127.0.0.1:8000/", 2*time.Second, try.StatusCodeIs(http.StatusNotFound)) | ||
c.Assert(err, checker.IsNil) | ||
|
||
// Make sure exposed service is ready. | ||
err = try.GetRequest("http://127.0.0.1:8000/service", 3*time.Second, try.StatusCodeIs(http.StatusOK)) | ||
c.Assert(err, checker.IsNil) | ||
|
||
// Send SIGTERM to Traefik. | ||
proc, err := os.FindProcess(cmd.Process.Pid) | ||
c.Assert(err, checker.IsNil) | ||
err = proc.Signal(syscall.SIGTERM) | ||
c.Assert(err, checker.IsNil) | ||
|
||
// Give Traefik time to process the SIGTERM and send a request half-way | ||
// into the request accepting grace period, by which requests should | ||
// still get served. | ||
time.Sleep(5 * time.Second) | ||
resp, err := http.Get("http://127.0.0.1:8000/service") | ||
c.Assert(err, checker.IsNil) | ||
defer resp.Body.Close() | ||
c.Assert(resp.StatusCode, checker.Equals, http.StatusOK) | ||
|
||
// Expect Traefik to shut down gracefully once the request accepting grace | ||
// period has elapsed. | ||
waitErr := make(chan error) | ||
go func() { | ||
waitErr <- cmd.Wait() | ||
}() | ||
|
||
select { | ||
case err := <-waitErr: | ||
c.Assert(err, checker.IsNil) | ||
case <-time.After(10 * time.Second): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 5s (sleep) + 10s (after) = 15s There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
// By now we are ~5 seconds out of the request accepting grace period | ||
// (start + 5 seconds sleep prior to the mid-grace period request + | ||
// 10 seconds timeout = 15 seconds > 10 seconds grace period). | ||
// Something must have gone wrong if we still haven't terminated at | ||
// this point. | ||
c.Fatal("Traefik did not terminate in time") | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
defaultEntryPoints = ["http"] | ||
|
||
logLevel = "DEBUG" | ||
|
||
[entryPoints] | ||
[entryPoints.http] | ||
address = ":8000" | ||
|
||
[lifeCycle] | ||
requestAcceptGraceTimeout = "10s" | ||
|
||
[file] | ||
[backends] | ||
[backends.backend] | ||
[backends.backend.servers.server] | ||
url = "{{.Server}}" | ||
|
||
[frontends] | ||
[frontends.frontend] | ||
backend = "backend" | ||
[frontends.frontend.routes.service] | ||
rule = "Path:/service" |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
whoami: | ||
image: emilevauge/whoami |
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.
Do you expect other settings than timeouts inside of the
LifeCycle
struct? If not do we want to rename it toLifeCycleTimeouts
for consistency reasons?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.
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.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.
I see your point. SGTM.