[Service Discovery] create a template cache to reduce calls to the KV store #3060

hkaj · 2016-12-05T18:16:09Z

Note: Please remember to review the Datadog Contribution Guidelines
if you have not yet done so.

What does this PR do?

Create a cache for configuration templates

it keeps auto_configuration templates and templates coming from the KV store
it invalides the KV templates when the agent detects an update in the config store
to reload auto_conf or JMX templates we still need to restart the collector or send it a SIGHUP.

Minor clean ups and test updates

Motivation

Using this cache avoids querying the config store at every container update.

Testing Guidelines

Tests were updated accordingly. A custom docker image using this build is also available at the TODO tag.

truthbk

This looks really good. Just a few minor comments/questions.

truthbk · 2016-12-20T18:27:43Z

utils/checkfiles.py

+                init_tpl = auto_conf.get('init_config') or {}
+                instance_tpl = auto_conf.get('instances', [])
+                if image not in auto_conf_images:
+                    auto_conf_images[image] = [[check_name], [init_tpl], [instance_tpl]]


Wouldn't it end up being less error-prone to have the init and instance configs/templates in a dict with check_name as the key?

you mean like auto_conf_images[(image, check_name)] = [init_tpl, instance_tpl]?

Problem with that is if users have several instances of the same check for the same image (and a customer actually asked how to do that yesterday so it happens). In this case we still need to wrap init_tpl and instance_tpl in lists and end up with the same logic as we have now.

But maybe I didn't understand correctly your suggestion

I think I originally meant auto_conf_images[image] = {'checkname': {'init': init_tpl, 'instances: [intance_tpl_0, instance_tpl_1,...]}}

But then I read on, and I saw it wasn't necessarily a good idea given the current structure/logic. I felt it would make the code a little more maintainable (perhaps). We can maybe just keep in mind for the future (it'd be a pretty big refactor), there's nothing wrong with the current logic.

truthbk · 2016-12-20T18:36:07Z

utils/service_discovery/abstract_config_store.py

+                        log.warning("Conflicting templates in auto_conf for image %s and check %s. "
+                                "Please check your template files." % (image, check_name))
+                        continue
+                    self.auto_conf_templates[image][0].append(check_name)


Likewise, I'm not sure if this wouldn't be better as self.auto_conf_templates[image][check_name] = (init_tpl, instance_tpl)

Oh I get it, we stick to lists for "compatibility" across the board when dealing with the configs. I think we could maybe refactor in the future, but it's a legit choice.

truthbk · 2016-12-20T20:36:48Z

utils/checkfiles.py

@@ -50,7 +50,7 @@ def get_check_class(agentConfig, check_name):
    return None


-def get_auto_conf(agentConfig, check_name):
+def get_auto_conf(check_name):


I feel like get_auto_conf() and get_auto_config() as names are a bit too close to each other...

truthbk · 2016-12-20T21:06:45Z

utils/service_discovery/abstract_config_store.py

+            # if _issue_read doesn't return anything we still create a key in the cache
+            # so that subsequent reads don't trigger issue_read every time
+            else:
+                self.kv_templates[identifier] = None


I know this is a NOOP but I noticed that we do include in the results templates[CONFIG_FROM_TEMPLATE] = self.kv_templates[identifier] even when self.kv_templates[identifier] is None if we have a hit, but we don't do that with a miss.

Just saying because it occured to me we could refactor this as:

if identifier not in self.kv_templates: tpls = self._issue_read(identifier) if tpls: templates[CONFIG_FROM_TEMPLATE] = tpls self.kv_templates[identifier] = tpls else: self.kv_templates[identifier] = None templates[CONFIG_FROM_TEMPLATE] = self.kv_templates[identifier]

because we always populate after a hit or fetch.

man that snippet is on point! 👌

truthbk · 2016-12-20T21:13:54Z

utils/service_discovery/abstract_config_store.py

-            # lastly fallback to auto_conf
-            return set(self.identifier_to_checks[self._get_image_ident(identifier)])
+            # lastly fallback to legacy name for auto-conf
+            return self.template_cache.get_check_names(self._get_image_ident(identifier))


Question: can we have checks under the legacy name that should also be applied? So a case where identifier and the legacy self._get_image_ident(identifier) would both apply to an image?

Good catch! That's a use case we didn't support before (if user-defined templates were found we skipped auto config) but I added logic for that here and forgot to update this part. Will do it now.

truthbk · 2016-12-20T21:35:52Z

tests/core/test_service_discovery.py

-    @mock.patch.object(AbstractConfigStore, '_issue_read', side_effect=issue_read)
-    def test_read_config_from_store(self, issue_read):
+    @mock.patch.object(_TemplateCache, '_issue_read', side_effect=issue_read)
+    def test_read_config_from_store(self, args):


Big 🙇 for all these awesome tests!

truthbk

Don't see anything wrong with this!

truthbk

Just a couple of nits, but I approve! ;)

truthbk · 2017-01-24T04:39:53Z

agent.py

@@ -245,7 +245,7 @@ def run(self, config=None):
        if self._agentConfig.get('service_discovery'):
            self.sd_backend = get_sd_backend(self._agentConfig)

-        if _is_affirmative(self._agentConfig.get('sd_jmx_enable')):
+        if _is_affirmative(self._agentConfig.get('sd_jmx_enable', '')):


This would probably be more readable as: _is_affirmative(self._agentConfig.get('sd_jmx_enable', False)).... Am I missing something?

truthbk · 2017-01-24T16:39:30Z

utils/service_discovery/abstract_config_store.py

+                log.exception('Failed to retrieve a template for %s.' % identifier)
+            # create a key in the cache even if _issue_read doesn't return a tpl
+            # so that subsequent reads don't trigger issue_read
+            self.kv_templates[identifier] = tpls or None


I'm not sure you need the or None? tpls will already be something if self._issue_read(identifier) returns either a list or None, or the None we've set it in the except blocks.

This cache is invalidated when a template change is detected. It doesn't detect changes to the auto_conf folder or JMX checks, only to the KV store. Refreshing configs from files still requires a SIGHUP or a restart.

hkaj · 2017-01-25T16:30:26Z

Thanks @truthbk ! I addressed your comments and squashed the commits. Will merge once the tests pass 🎉

hkaj force-pushed the haissam/sd-cache-kv-tpls branch from 9122643 to 6defada Compare December 5, 2016 18:23

hkaj assigned truthbk Dec 5, 2016

hkaj added checks improvement service discovery labels Dec 5, 2016

hkaj added this to the 5.11.0 milestone Dec 5, 2016

hkaj force-pushed the haissam/sd-cache-kv-tpls branch from 6defada to e3094d3 Compare December 6, 2016 14:34

hkaj removed the checks label Dec 7, 2016

truthbk approved these changes Dec 20, 2016

View reviewed changes

hkaj force-pushed the haissam/sd-cache-kv-tpls branch from e3094d3 to 999f7bd Compare December 21, 2016 14:41

truthbk approved these changes Dec 27, 2016

View reviewed changes

hkaj force-pushed the haissam/sd-cache-kv-tpls branch 8 times, most recently from 7543301 to 2eaab69 Compare January 10, 2017 00:05

truthbk approved these changes Jan 24, 2017

View reviewed changes

masci modified the milestones: 5.11.0, 5.12.0 Jan 24, 2017

[SD] create a template cache to reduce calls to the KV store

13d8a24

This cache is invalidated when a template change is detected. It doesn't detect changes to the auto_conf folder or JMX checks, only to the KV store. Refreshing configs from files still requires a SIGHUP or a restart.

hkaj force-pushed the haissam/sd-cache-kv-tpls branch from 2eaab69 to 13d8a24 Compare January 25, 2017 16:28

hkaj merged commit df1a506 into master Jan 25, 2017

hkaj deleted the haissam/sd-cache-kv-tpls branch January 25, 2017 16:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Service Discovery] create a template cache to reduce calls to the KV store #3060

[Service Discovery] create a template cache to reduce calls to the KV store #3060

hkaj commented Dec 5, 2016

truthbk left a comment

truthbk Dec 20, 2016

hkaj Dec 21, 2016

truthbk Dec 27, 2016

truthbk Dec 20, 2016

truthbk Dec 20, 2016

truthbk Dec 20, 2016

truthbk Dec 20, 2016

hkaj Dec 21, 2016

truthbk Dec 20, 2016

hkaj Dec 21, 2016

truthbk Dec 20, 2016

truthbk left a comment

truthbk left a comment

truthbk Jan 24, 2017

truthbk Jan 24, 2017

hkaj commented Jan 25, 2017

[Service Discovery] create a template cache to reduce calls to the KV store #3060

[Service Discovery] create a template cache to reduce calls to the KV store #3060

Conversation

hkaj commented Dec 5, 2016

What does this PR do?

Motivation

Testing Guidelines

truthbk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

truthbk left a comment

Choose a reason for hiding this comment

truthbk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hkaj commented Jan 25, 2017