Speeding up consul catalog health change detection

[vholovko]

Implemented long polling for services and for the health in the separate co-routines and react on either trigger independently by using same channel for both.

Basically two things improved:

1. Decreased Health change detection time.
2. Reduced amount of false triggering for traefik configuration rebuild by doing extra checking for actually changed services.

Detailed description: #1107 (comment)

Improvement for #1107
Relates to #1514

[keis]

Cool stuff :)

[keis]

Rather than doing a merge make the channel a parameter of both watch* and send the same one to both.

[vholovko]

Thanks, good catch!
Corrected.

[keis]

By only looking at the service name wouldn't that mean that health changes in 1 node in a bigger pool would not trigger a message. Or maybe I'm confused what ServiceName is here.

[vholovko]

I was just filtering only services with real names without consul agents. But now I adjusted the code to work only with Catalog Services response. Should be more cleaner approach now.

[ldez]

The order in Go (for stdlib testing) is got first, want second; both in the comparison expression and the error message.

[vholovko]

Changed the order. Thanks.

[ldez]

The order in Go (for stdlib testing) is got first, want second; both in the comparison expression and the error message.

[vholovko]

The order was changed.

[ldez]

log.Errorf("Failed to list services: %s", err)

[vholovko]

Fixed.

[ldez]

log.Errorf("Failed to list services: %s", err)

[vholovko]

Done it in appropriate way.

[emilevauge]

Great @vholovko !
I left few comments but overall, seems good :)

[emilevauge]

Shouldn't we add a else after this block ?

[vholovko]

I'm not seeing here any functionality that can be placed in the else block.
This branch statement was copied from original watchServices function and there wasn't any else block for it.

From the source it is clear that there are two places when nil for data is possible
https://github.com/hashicorp/consul/blob/master/api/catalog.go#L140
https://github.com/hashicorp/consul/blob/master/api/catalog.go#L150
it is when we get an error which is handled at earlier phase in the code above.

Thus it seems that this if statement is just a precaution in case json.NewDecoder can bring nil without notifying us with an error (the actual JSON null in the consul response).
I doubt consul response body will be null within 200 OK GET response.
Even in case there are not services at all the JSON response should include the empty map and this situation is handled already.

[emilevauge]

Thanks for you detailed explanation 👍

[emilevauge]

Same comment, shouldn't we add a else block ?

[vholovko]

I don't think anything should be placed in the 'else' block. The explanation is given in the earlier comment.

[emilevauge]

Thanks @vholovko
LGTM

[ldez]

@vholovko could you squash your commits?

[vholovko]

@ldez
Sorry for inconvenience, but how do I do the squash of my commits in terms of git commands ?
What sequence and in what order should I run to not mess up things ?

[ldez]

@vholovko No problem 😃

You are in a specific case, because you have merge master several times in your branch.

In the following commands, I suppose your git local repository setup is:

$ git remote -v

origin	git@github.com:vholovko/traefik.git (fetch)
origin	git@github.com:vholovko/traefik.git (push)
upstream	git@github.com:containous/traefik.git (fetch)
upstream	git@github.com:containous/traefik.git (push)

The commands:

# Update your repo
git fetch --all

# Delete your local branch
git branch -D consul_catalog_hot_detect_newly_added_containers

# Create a branch with the same name as the previous.
# This branch must be created on the HEAD of the containous/traefik master branch.
git checkout -b consul_catalog_hot_detect_newly_added_containers upstream/master

# Squash all your branch in this new branch
git merge --squash origin/consul_catalog_hot_detect_newly_added_containers

# Create a new commit
git commit -m "Speeding up health change detection by separating it from catalog services check."

# Force push the branch.
git push --force-with-lease origin consul_catalog_hot_detect_newly_added_containers

Next time, when you want to sync your branch with the master, use rebase instead merge 😉.
In this case the squash is more simple and can be done by only one command git rebase -i <HASH_OF_THE_COMMIT_BEFORE_YOUR_FIRST_COMMIT>.

[vholovko]

thanks @ldez , your hints were very helpful
all done as you described

[ldez]

Thanks 👍
LGTM