Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add health check label to ECS #2421

Merged
merged 1 commit into from
Nov 21, 2017
Merged

Add health check label to ECS #2421

merged 1 commit into from
Nov 21, 2017

Conversation

oldmantaiter
Copy link
Contributor

What does this PR do?

Motivation

Even though ECS can provide status, it is (IMO) better to also check health from the load balancer to ensure the instance is up and running properly.

More

  • Added/updated tests
  • Added/updated documentation

juliens
juliens requested changes Nov 20, 2017
View reviewed changes
Copy link
Member

@juliens juliens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thx for this PR !

healthcheck/healthcheck.go Outdated
@@ -148,6 +150,12 @@ func checkHealth(serverURL *url.URL, backend *BackendHealthCheck) bool {
client := http.Client{
Timeout: backend.requestTimeout,
}
if backend.Options.IgnoreCertificateErrors {
tr := &http.Transport{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should be better to use the existing transport with TLS client configuration used for backend instead of add a new option to healthcheck.
https://github.com/containous/traefik/blob/be306d651e999feb50f1f126eb68073d607324f3/server/server.go#L68

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. I'll pass that through

@oldmantaiter oldmantaiter force-pushed the ecs-healthcheck branch 2 times, most recently from 676aa6e to 028103d Compare November 20, 2017 16:01
@oldmantaiter
Copy link
Contributor Author

@juliens Updated to use the default server roundtripper, thanks for the review!

@ldez
Copy link
Contributor

ldez commented Nov 20, 2017

@oldmantaiter Could you fix?

---> Making bundle: validate-autogen (in .)
checking autogen is up-to-date with templates...
The result of 'go generate' differs

 M autogen/gentemplates/gen.go

Please do "go generate" to update the `autogen` package.

@oldmantaiter oldmantaiter force-pushed the ecs-healthcheck branch 2 times, most recently from 62c8b9f to 0839aba Compare November 20, 2017 18:09
@oldmantaiter
Copy link
Contributor Author

@ldez Yep, locally the autogen validation test passes so just debugging why semaphore is not a fan

@oldmantaiter
Copy link
Contributor Author

@ldez - Fixed

mmatur
mmatur approved these changes Nov 21, 2017
View reviewed changes
Copy link
Member

@mmatur mmatur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mmatur mmatur added this to the 1.5 milestone Nov 21, 2017
@oldmantaiter @traefiker
Add health check label to ECS
a9aca46 
- Adds support for health checks of ECS containers where you can set
  the path and interval of the check for a given backend (service)
- Partially deals with traefik#1400
@traefiker traefiker merged commit 6d2f4a0 into traefik:master Nov 21, 2017
@oldmantaiter
Copy link
Contributor Author

Looks like using the default server round tripper in the health check has caused a connection leak. I'm still investigating but with health checks enabled my services start getting a ton of connections. I think this may need to be fixed to keep the client attached to the health check instead of making new requests each time.

@ldez ldez mentioned this pull request Dec 11, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ldez ldez ldez approved these changes

@mmatur mmatur mmatur approved these changes

@juliens juliens juliens approved these changes

Assignees
No one assigned
Labels
area/provider/ecs kind/enhancement a new or improved feature. size/M
Projects
None yet
Milestone
1.5
Development

Successfully merging this pull request may close these issues.
5 participants
@oldmantaiter @ldez @juliens @mmatur @traefiker