Bind healthcheck to backend by entryPointName #1868
Conversation
Fixes: #1866 Conflicts: #1867 If we need to have one lb instance per entrypoint for some reasons, I don't see, this commit registers healtchcheck with `entryPointName+frontend.Backend` as a key. So the healthcheck will be registered as often as there are entrypoints. Because of this, I would prefer #1867.
ldez
added
kind/enhancement
|
Thanks for your contribution, design LGTM |
There was a problem hiding this comment.
Hey @chrigl, great fix! We would love to see an integration test on this use case. Do you think you could do this ?
|
@emilevauge added an integration test. |
integration/healthcheck_test.go
Outdated
| c.Assert(err, checker.IsNil) | ||
| } | ||
| // Test http1 is healthy and only whoami2 responds | ||
| for i := 0; i < 4; i++ { |
There was a problem hiding this comment.
Could you replace this loop by a try:
// Check if whoami1 never respond
err = try.Request(frontend1Req, 2*time.Second, try.BodyContains(whoami1Host))
c.Assert(err, checker.Not(checker.IsNil))Instead of testing the presence of whoami2 you can test the absence of whoami1.
integration/healthcheck_test.go
Outdated
| } | ||
| // Test http1 is healthy and only whoami2 responds | ||
| for i := 0; i < 4; i++ { | ||
| frontendReq, err := http.NewRequest(http.MethodGet, "http://127.0.0.1:8000/", nil) |
There was a problem hiding this comment.
You can extract the request from the loop and rename like that frontend2Req
integration/healthcheck_test.go
Outdated
| // Test http2 is healthy and only whoami2 responds | ||
| // Looping to give traefik the chance to forward to a broken server | ||
| for i := 0; i < 4; i++ { | ||
| frontendReq, err := http.NewRequest(http.MethodGet, "http://127.0.0.1:9000/", nil) |
There was a problem hiding this comment.
You can extract the request from the loop and rename like that frontend1Req
integration/healthcheck_test.go
Outdated
|
|
||
| // Test http2 is healthy and only whoami2 responds | ||
| // Looping to give traefik the chance to forward to a broken server | ||
| for i := 0; i < 4; i++ { |
There was a problem hiding this comment.
Could you replace this loop by a try:
// Check if whoami1 never respond
err = try.Request(frontend2Req, 2*time.Second, try.BodyContains(whoami1Host))
c.Assert(err, checker.Not(checker.IsNil))Instead of testing the presence of whoami2 you can test the absence of whoami1.
integration/healthcheck_test.go
Outdated
| @@ -92,3 +92,89 @@ func (s *HealthCheckSuite) TestSimpleConfiguration(c *check.C) { | |||
| c.Assert(err, checker.IsNil) | |||
| c.Assert(resp.StatusCode, checker.Equals, http.StatusNotFound) | |||
| } | |||
|
|
|||
| func (s *HealthCheckSuite) TestMultipleEntrypoints(c *check.C) { | |||
| whoami1Host := s.composeProject.Container(c, "whoami1").NetworkSettings.IPAddress | |||
There was a problem hiding this comment.
Could you refactor tests like that:
type HealthCheckSuite struct{
BaseSuite
whoami1IP string
whoami2IP string
}
func (s *HealthCheckSuite) SetUpSuite(c *check.C) {
s.createComposeProject(c, "healthcheck")
s.composeProject.Start(c)
s.whoami1IP = s.composeProject.Container(c, "whoami1").NetworkSettings.IPAddress
s.whoami2IP = s.composeProject.Container(c, "whoami2").NetworkSettings.IPAddress
}and replace all occurrence of whoami1Host and whoami2Host by s.whoami1IP and s.whoami2IP
* replaced unnecessary for loops by Try * replaced whoamiHosts by s.whoamiIPs * added additional test case to also cover drr backends
|
@ldez Thanks for your feedback. Changed code to your recommendations, and added a new test case to also cover healthchecks on drr backends. |
Description
Fixes: #1866
Conflicts: #1867
If we need to have one lb instance per entrypoint for some reasons, I
don't see, this commit registers healtchcheck with
entryPointName+frontend.Backendas a key. So the healthcheck will beregistered as often as there are entrypoints. Because of this, I would
prefer #1867.