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.

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 support for docker healthcheck 👼 #708

Merged
merged 1 commit into from
Oct 3, 2016

Conversation

vdemeester
Copy link
Contributor

@vdemeester vdemeester commented Oct 2, 2016

This add the simplest support for HEALTHCHECK for the docker provider.

Fixes #666.

  • React to health_status events
  • Filter container that have a health status and that are not healthy

It works like the other provider that support healthchecks (marathon for example) — if healthcheck are not supported, then it work as usual and if a container supports it, it filters the container if it's not healthy.

  • Update documentation (it's actually not documented on other providers, so…)
  • Add integration tests (too much extra work on that, I need to update libkermit and add support for a testRequire thingy with a check on docker >= 1.12 — will do in a follow up)

/cc @containous/traefik

🐸

Signed-off-by: Vincent Demeester vincent@sbr.pm

@vdemeester vdemeester added this to the 1.1 milestone Oct 2, 2016
@vdemeester vdemeester force-pushed the docker-support-healthcheck branch 2 times, most recently from 3b6f439 to 5530915 Compare October 2, 2016 16:48
@vdemeester vdemeester changed the title WIP: Add support for docker healthcheck 👼 Add support for docker healthcheck 👼 Oct 2, 2016
@@ -214,6 +215,9 @@ func (provider *Docker) Provide(configurationChan chan<- types.ConfigMessage, po
}
eventHandler.Handle("start", startStopHandle)
eventHandler.Handle("die", startStopHandle)
eventHandler.Handle("health_status: healthy", startStopHandle)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The way health_status event are sent via the docker api is a little buggy, see moby/moby#25800 👼

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.

thanks a lot @vdemeester :) Just a little comment.

@@ -378,6 +382,10 @@ func (provider *Docker) containerFilter(container dockerData) bool {
return false
}

if container.Health != "" && container.Health != "healthy" {
return false
Copy link
Member

Choose a reason for hiding this comment

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

I would add a debug log here to inform why the container has been filtered ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done 👼

@vhf
Copy link
Contributor

vhf commented Oct 2, 2016

Tested, it works nicely, thanks! 💯

Since the web UI shows all backend containers and not only the filteredContainers, I added some logging in the build I tested - this way I could see what was happening (otherwise my unhealthy containers were showing up along the healthy ones).

A nice addition would be to display the health status in the web UI. I think this PR is fine without this feature and I don't want to hijack it, but I might try to add this as a separate PR. [disregard this comment, I think I was wrong and unhealthy containers don't show up in the web UI]

- React to health_status events
- Filter container that have a health status *and* that are not healthy

Signed-off-by: Vincent Demeester <vincent@sbr.pm>
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.

LGTM

@emilevauge
Copy link
Member

Fixes #666

Copy link
Contributor

@errm errm left a comment

Choose a reason for hiding this comment

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

LGTM

I like how simple this is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants