Improve service discovery to only reload checks that need it #2702
Conversation
hkaj
force-pushed
the
haissam/fine-grained-reload
branch
5 times, most recently
from
ef2c7af
to
9d3d27b
Compare
| @@ -200,7 +200,7 @@ def get_hostname(config=None): | |||
| return gce_hostname | |||
|
|
|||
| # Try to get the docker hostname | |||
| docker_util = DockerUtil() | |||
| docker_util = DockerUtil(agentConfig=config) | |||
There was a problem hiding this comment.
Passing agentConfig doesn't change anything here, or am I missing something?
There was a problem hiding this comment.
it's used in DockerUtil to get the instance of the config_store. We could pass a subset of it instead since we basically just need the sd_config_backend field, but that would be more confusing than useful imho.
|
Added in a few comments, but looks nice overall! 📈 |
hkaj
force-pushed
the
haissam/fine-grained-reload
branch
from
2fae878
to
60933bd
Compare
|
Thanks @olivielpeau, that was a kick-ass review! |
hkaj
force-pushed
the
haissam/fine-grained-reload
branch
from
60933bd
to
ebf1fd9
Compare
|
Responded to one of your comments, but other than that LGTM Feel free to merge once that's addressed 🎆 |
|
👍 looking good, feel free to merge! (once the commit history is cleaned up a bit :) ) |
The `res.etcd_index` value that was previously used is the `X-Etcd-Index` header of etcd's http response. According to etcd's docs this index is the current index of the whole cluster, so any modification on any key increments this value. Using it as a reference to determine if the datastore has changed would trigger a config reload on every modification in the datastore (whereas we want a config reload only when the `sd_template_dir` changes). To fix this, we parse the `modifiedIndex` of all the returned keys instead, and we take the max index as the correct index.
hkaj
force-pushed
the
haissam/fine-grained-reload
branch
from
7441f84
to
98c6b40
Compare
|
It's failing on a cassandra timeout. Merging as it's unrelated. |
What does this PR do?
It makes configuration reload smarter.
The gist of it is that the agent now maintains a mapping
container_identifier <--> checksand only reloads checks for which a docker event associated with an identifier in this mapping happened since the last collector run.Motivation
In environments where a lot of docker events happen, full config reload on any container event is not a good design. We noticed several cases where the agent triggered a reload at each run, making rate collection impossible (calculating rates requires values from several runs, and these are forgotten on each check reload).
Individual reload is a better option.
Additional notes
In more details:
identifier_to_checkscache is built based on the content of the config store and theauto_conffolder. This cache is invalidated and refreshed when a template is modified in the K/V store.SIGHUPis received by the collector or the agent detects a change in the config store.auto_conffiles are not detected yet so an agent restart or a manualSIGHUPis still needed here.load_checkfunction that does the same thing asload_check_directorybut for a single check which name was passed as a third parameter.