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

using more sensible consul blocking query to detect health check changes #1241

Merged
merged 1 commit into from
May 4, 2017
Merged

using more sensible consul blocking query to detect health check changes #1241

merged 1 commit into from
May 4, 2017

Conversation

vholovko
Copy link
Contributor

@vholovko vholovko commented Mar 6, 2017

Possible fix for #1107

Using health.State only as a trigger for further actions.

@vholovko vholovko mentioned this pull request Mar 6, 2017
Closed
@rokka-n
Copy link

rokka-n commented Mar 6, 2017

Is it going to a release any time soon?

@vholovko
Copy link
Contributor Author

vholovko commented Mar 7, 2017 •
edited
Loading

@emilevauge, @rokka-n
Added required comments but not unit tests.
Don't have any experience writing go routines tests with channel communication (in particular for watchServices function).
It would be much appreciated if someone could write them out (at least one test).

@emilevauge emilevauge mentioned this pull request Mar 7, 2017
Closed
@jzvelc
Copy link

jzvelc commented Mar 10, 2017

Can we expect this in 1.2.0?

@timoreimann
Copy link
Contributor

@jzvelc we'd have to consider the issue that this PR fixes to be critical enough to warrant being merged into the 1.2 release branch, which has already reached candidate 2 status at this time.

@emilevauge thoughts?

@samart
Copy link

samart commented Mar 12, 2017 •
edited
Loading

Please merge this, as we really need it asap! This is critical for anyone using traefik with consul, since services that have healthchecks won't appear in traefik...

@rokka-n
Copy link

rokka-n commented Mar 12, 2017

Unit and integration tests needed for this pr.

@timoreimann
Copy link
Contributor

There are (at least) two routes to add unit tests here:

  1. Through watchServices(). This is probably the easiest route; however, it relies on testing an unexported method, whose behavior could change.
  2. Through Provide(). This probably takes a bit more effort but seems more robust in the long term.

I'm leaning towards 2. if the effort is reasonable.

I haven't looked at the integration tests yet.

@emilevauge
Copy link
Member

@vholovko It would be great to get this into v1.2. Could you rebase your PR against v1.2 branch? Do you think adding unit/integration tests on this is doable anytime soon?

@emilevauge emilevauge added this to the 1.2 milestone Mar 13, 2017
emilevauge
emilevauge reviewed Mar 15, 2017
View reviewed changes
provider/consul_catalog.go Outdated
data, meta, err := catalog.Services(opts)
// Listening to changes that leads to `passing` state or degrades from it.
// The call is used just as a trigger for further actions (intentionally there is no interest in the received data).
_, meta, err := health.State("passing", opts)
Copy link
Member

Choose a reason for hiding this comment

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

@vholovko I still have a doubt on this implementation in case we register services in Consul without healthcheck. Will these services trigger this listener ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@emilevauge
It is a valid point!
I can confirm that trigger wont work in this situation. Should rework it to listen both the catalog.Services and health.State at the same time

@vholovko vholovko changed the base branch from master to v1.2 March 16, 2017 08:41
@emilevauge
Copy link
Member

We are going to release v1.2 soon. Do you want your PR to be in that release @vholovko ?

@vholovko
Copy link
Contributor Author

vholovko commented Mar 17, 2017 •
edited
Loading

Hi @emilevauge,
Sorry for long hesitation, haven't got much time to look into it.

Apparently it doesn't work for case with services without health check defined.
I should find some time in the near future to correct this and finally make some unit tests.
But for now I'm afraid it cannot go to v1.2 in the current state...

@emilevauge
Copy link
Member

@vholovko ping ?

@vholovko
Copy link
Contributor Author

vholovko commented Apr 8, 2017 •
edited
Loading

@emilevauge,

Implemented version that should support both kinds of services: with health checks and without.
Unfortunately it goes without unit tests. Almost impossible for me to mock provider.client.Catalog() & provider.client.Health() and run unit test on method with safe.Go routine inside it. Would be great to receive some directions for possible solutions on that one.

However, I have an alternative draft proposal slightly different in terms of implementation, where the only clean way to test things was to use interfaces.

consul_catalog.go
consul_catalog_test.go

It can look ugly at first glance but it allows legal and not hacky approach for mocking struct methods inside tests via Interfacing. The one and only issue here is that + go test -cover -coverprofile=cover.out ./provider runs forever ...

Would be glad to receive any possible help for testing original 9501821 commit. Thanks

@venkataramanr
Copy link

@vholovko the code still checks only for passing health state, which would work just fine for services that have health checks defined, for those that don't the checks would only be serfHealth by default.. would there be a refresh and a change in index on the health state endpoint in such a case ? unless the node goes down or comes back up.

@vholovko
Copy link
Contributor Author

@venkataramanr
Seft Health Status is returning together with all the others checks from health state endpoint, so index should change and we should be able to detect it.

@venkataramanr
Copy link

thanks @vholovko for responding. SerfHeath does appear on the health endpoint, yet, unless there is a change in the agent's health status ( which is serf health ) would there be an update to the index ?

@ldez
Copy link
Contributor

ldez commented Apr 19, 2017

@vholovko Can you change the target branch of this PR from v1.2 to master ? Thanks.
(and rebase if it possible to you)

@vholovko vholovko changed the base branch from v1.2 to master April 24, 2017 19:25
@ldez ldez modified the milestones: 1.2, 1.3 Apr 25, 2017
@ldez ldez modified the milestone: 1.3 May 2, 2017
@emilevauge emilevauge force-pushed the healthcheck_changes branch 3 times, most recently from 5e2e3a0 to 3f6ad4e Compare May 4, 2017 14:52
emilevauge
emilevauge approved these changes May 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.

Thanks @vholovko
LGTM

@emilevauge emilevauge merged commit ba928dd into traefik:master May 4, 2017
@ldez ldez mentioned this pull request Jun 9, 2017
Closed
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

@ldez ldez ldez approved these changes

Assignees
No one assigned
Labels
area/healthcheck area/provider/consulcatalog kind/enhancement a new or improved feature.
Projects
None yet
Milestone
1.3
Development

Successfully merging this pull request may close these issues.
8 participants
@vholovko @rokka-n @jzvelc @timoreimann @samart @emilevauge @venkataramanr @ldez