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 HealthCheck #918

Merged
merged 6 commits into from Feb 6, 2017
Merged

Add HealthCheck #918

merged 6 commits into from Feb 6, 2017

Conversation

juliens
Copy link
Member

@juliens juliens commented Nov 29, 2016

No description provided.

emilevauge
emilevauge reviewed Nov 29, 2016
View reviewed changes
Copy link
Member

@emilevauge emilevauge left a comment •
edited

Choose a reason for hiding this comment

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

Hey @juliens thanks a lot for this :)
I made few comments.
I'm not entirely sure on the "one go routine" design. If one check takes a long time, it will defers other checks. We can't have different healthchecks for different backends (and we will need that). WDYT @containous/traefik ?

server.go Outdated
@@ -524,6 +525,9 @@ func (server *Server) loadConfig(configurations configs, globalConfiguration Glo
redirectHandlers := make(map[string]http.Handler)

backends := map[string]http.Handler{}

backendsHealcheck := map[string]*healthcheck.BackendHealthCheck{}
Copy link
Member

Choose a reason for hiding this comment

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

typo

healthcheck/healthcheck.go Outdated
singleton = newHealthCheck()
singleton.execute()
})
return singleton
Copy link
Member

Choose a reason for hiding this comment

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

Maybe an init function would be simpler ?

healthcheck/healthcheck.go Outdated
}

func testHealth(serverURL *url.URL, checkURL string) bool {
resp, err := http.Get(serverURL.String() + checkURL)
Copy link
Member

Choose a reason for hiding this comment

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

We need a timeout here

emilevauge
emilevauge reviewed Jan 25, 2017
View reviewed changes
Copy link
Member

@emilevauge emilevauge left a comment

Choose a reason for hiding this comment

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

Hey @juliens, I made a few comments in the code :)
Could you also update the documentation (doc/)?
Thanks a lot !
/cc @containous/traefik

healthcheck/healthcheck.go Outdated
}
}

func testHealth(serverURL *url.URL, checkURL string) bool {
Copy link
Member

Choose a reason for hiding this comment

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

Can you change this to checkHealth instead, we could confuse with a real test ;)

server.go Outdated
@@ -732,6 +742,7 @@ func (server *Server) loadConfig(configurations configs, globalConfiguration Glo
}
}
}
healthcheck.GetHealthCheck().SetBackendsConfiguration(backendsHealthcheck)
Copy link
Member

Choose a reason for hiding this comment

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

Could you add a parameter server.routinesPool.Ctx() here ?

healthcheck/healthcheck.go Outdated
if hc.cancel != nil {
hc.cancel()
}
ctx, cancel := context.WithCancel(context.Background())
Copy link
Member

Choose a reason for hiding this comment

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

Could you use the global context from server.routinesPool.Ctx() ?

healthcheck/healthcheck.go Outdated

func (hc *HealthCheck) execute(ctx context.Context) {
for backendID, backend := range hc.Backends {
go func(backendID string, backend *BackendHealthCheck) {
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 we should be safer here. We need to recover in case of a panic in those goroutines.
Could you use the safe.Go() wrapper here ?

types/types.go Outdated
@@ -35,6 +36,11 @@ type CircuitBreaker struct {
Expression string `json:"expression,omitempty"`
}

// HealthCheck holds healthchk configuration
Copy link
Member

Choose a reason for hiding this comment

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

HealthCheck holds healthchk

*HealthCheck

healthcheck/healthcheck.go Outdated
ticker := time.NewTicker(time.Second * 30)
select {
case <-ctx.Done():
log.Debugf("Refreshing All Healthcheck goroutines")
Copy link
Member

Choose a reason for hiding this comment

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

You mean stopping ?

healthcheck/healthcheck.go Outdated
var newDisabledURLs []*url.URL
for _, url := range backend.DisabledURLs {
if testHealth(url, backend.URL) {
log.Debugf("Upsert %s", url.String())
Copy link
Member

Choose a reason for hiding this comment

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

Could you log more info in this log ?

healthcheck/healthcheck.go Outdated

for _, url := range enabledURLs {
if !testHealth(url, backend.URL) {
log.Debugf("Remove %s", url.String())
Copy link
Member

Choose a reason for hiding this comment

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

Could you log more info in this log ?

@bakins
Copy link
Contributor

bakins commented Jan 31, 2017

@juliens are you still working on this? I'm interested in this feature and can take a look at addressing the comments, etc, if you don't have time.

@juliens
Copy link
Member Author

juliens commented Jan 31, 2017

@bakins I think that will be difficult to handle this before the next release (tomorrow, I guess). I'll try to do it before the end of the week.

@juliens juliens force-pushed the httpchk branch 2 times, most recently from 04f1dfd to e3ad4ff Compare January 31, 2017 21:58
emilevauge
emilevauge approved these changes Feb 4, 2017
View reviewed changes
Copy link
Member

@emilevauge emilevauge left a comment

Choose a reason for hiding this comment

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

@juliens Thank you for your corrections :)
I think we can merge this one. We will need another PR later to add tests and documentation during the RC phase.
/cc @containous/traefik

@emilevauge emilevauge added this to the 1.2 milestone Feb 5, 2017
@emilevauge emilevauge force-pushed the httpchk branch 2 times, most recently from 66b85d9 to 9d726c1 Compare February 5, 2017 21:20
@emilevauge emilevauge merged commit 39e6b16 into traefik:master Feb 6, 2017
@klausenbusk klausenbusk mentioned this pull request Feb 20, 2017
Closed
@klausenbusk
Copy link
Contributor

Seems like I can close my issue then :) Good work.
#824

@mattcollier mattcollier mentioned this pull request Apr 20, 2017
Closed
@ldez ldez changed the title (WIP) feat: HealthCheck feat: add HealthCheck Apr 29, 2017
@ldez ldez changed the title feat: add HealthCheck Add HealthCheck Apr 29, 2017
@ldez ldez added the kind/enhancement a new or improved feature. label Apr 29, 2017
@juliens juliens deleted the httpchk branch September 13, 2017 13:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emilevauge emilevauge emilevauge approved these changes

Assignees
No one assigned
Labels
kind/enhancement a new or improved feature.
Projects
None yet
Milestone
1.2
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@juliens @bakins @klausenbusk @emilevauge @ldez