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

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

Merged
merged 1 commit into from
Jan 25, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions agent.py
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ def __init__(self, pidfile, autorestart, start_event=True, in_developer_mode=Fal
self._checksd = []
self.collector_profile_interval = DEFAULT_COLLECTOR_PROFILE_INTERVAL
self.check_frequency = None
# this flag can be set to True, False, or a list of checks (for partial reload)
# this flag can be set to True, False, or a set of checks (for partial reload)
self.reload_configs_flag = False
self.sd_backend = None
self.supervisor_proxy = None
Expand Down Expand Up @@ -161,7 +161,7 @@ def reload_configs(self, checks_to_reload=set()):
log.info("No checksd configs found")

def refresh_specific_checks(self, hostname, checksd, checks):
"""take a list of checks and for each of them:
"""take a set of checks and for each of them:
- remove it from the init_failed_checks if it was there
- load a fresh config for it
- replace its old config with the new one in initialized_checks if there was one
Expand Down Expand Up @@ -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', False)):
pipe_path = get_jmx_pipe_path()
if Platform.is_windows():
pipe_name = pipe_path.format(pipename=SD_PIPE_NAME)
Expand Down
2 changes: 1 addition & 1 deletion checks.d/docker_daemon.py
Original file line number Diff line number Diff line change
Expand Up @@ -855,7 +855,7 @@ def _parse_cgroup_file(self, stat_file):
except IOError:
# It is possible that the container got stopped between the API call and now.
# Some files can also be missing (like cpu.stat) and that's fine.
self.log.info("Can't open %s. Some metrics for this container may be missing." % stat_file)
self.log.debug("Can't open %s. Its metrics will be missing." % stat_file)

def _parse_blkio_metrics(self, stats):
"""Parse the blkio metrics."""
Expand Down
2 changes: 1 addition & 1 deletion config.py
Original file line number Diff line number Diff line change
Expand Up @@ -1108,7 +1108,7 @@ def load_check(agentConfig, hostname, checkname):

# try to load the check and return the result
load_success, load_failure = load_check_from_places(check_config, check_name, checks_places, agentConfig)
return load_success.values()[0] or load_failure
return load_success.values()[0] if load_success else load_failure

return None

Expand Down
111 changes: 94 additions & 17 deletions tests/core/test_service_discovery.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
import copy
import mock
import unittest
from collections import defaultdict

# 3p
from nose.plugins.attrib import attr
Expand All @@ -11,7 +12,8 @@
from utils.service_discovery.config_stores import get_config_store
from utils.service_discovery.consul_config_store import ConsulStore
from utils.service_discovery.etcd_config_store import EtcdStore
from utils.service_discovery.abstract_config_store import AbstractConfigStore, CONFIG_FROM_KUBE
from utils.service_discovery.abstract_config_store import AbstractConfigStore, \
_TemplateCache, CONFIG_FROM_KUBE, CONFIG_FROM_TEMPLATE, CONFIG_FROM_AUTOCONF
from utils.service_discovery.sd_backend import get_sd_backend
from utils.service_discovery.sd_docker_backend import SDDockerBackend, _SDDockerBackendConfigFetchState

Expand Down Expand Up @@ -66,11 +68,11 @@ def client_read(path, **kwargs):
if 'all' in kwargs:
return {}
else:
return TestServiceDiscovery.mock_tpls.get(image)[0][config_parts.index(config_part)]
return TestServiceDiscovery.mock_raw_templates.get(image)[0][config_parts.index(config_part)]


def issue_read(identifier):
return TestServiceDiscovery.mock_tpls.get(identifier)
return TestServiceDiscovery.mock_raw_templates.get(identifier)

@attr('unix')
class TestServiceDiscovery(unittest.TestCase):
Expand Down Expand Up @@ -122,7 +124,7 @@ class TestServiceDiscovery(unittest.TestCase):
}

# raw templates coming straight from the config store
mock_tpls = {
mock_raw_templates = {
# image_name: ('[check_name]', '[init_tpl]', '[instance_tpl]', expected_python_tpl_list)
'image_0': (
('["check_0"]', '[{}]', '[{"host": "%%host%%"}]'),
Expand Down Expand Up @@ -509,12 +511,12 @@ def test_fill_tpl(self, *args):
def test_get_auto_config(self):
"""Test _get_auto_config"""
expected_tpl = {
'redis': ('redisdb', None, {"host": "%%host%%", "port": "%%port%%"}),
'consul': ('consul', None, {
"url": "http://%%host%%:%%port%%", "catalog_checks": True, "new_leader_checks": True
}),
'redis:v1': ('redisdb', None, {"host": "%%host%%", "port": "%%port%%"}),
'foobar': None
'redis': [('redisdb', None, {"host": "%%host%%", "port": "%%port%%"})],
'consul': [('consul', None, {
"url": "http://%%host%%:%%port%%", "catalog_checks": True, "new_leader_checks": True
})],
'redis:v1': [('redisdb', None, {"host": "%%host%%", "port": "%%port%%"})],
'foobar': []
}

config_store = get_config_store(self.auto_conf_agentConfig)
Expand All @@ -529,10 +531,10 @@ def test_get_check_tpls(self, mock_client_read):
invalid_config = ['bad_image_0', 'bad_image_1']
config_store = get_config_store(self.auto_conf_agentConfig)
for image in valid_config:
tpl = self.mock_tpls.get(image)[1]
tpl = self.mock_raw_templates.get(image)[1]
self.assertEquals(tpl, config_store.get_check_tpls(image))
for image in invalid_config:
tpl = self.mock_tpls.get(image)[1]
tpl = self.mock_raw_templates.get(image)[1]
self.assertEquals(tpl, config_store.get_check_tpls(image))

@mock.patch.object(AbstractConfigStore, 'client_read', side_effect=client_read)
Expand All @@ -542,7 +544,7 @@ def test_get_check_tpls_kube(self, mock_client_read):
invalid_config = ['bad_image_0']
config_store = get_config_store(self.auto_conf_agentConfig)
for image in valid_config + invalid_config:
tpl = self.mock_tpls.get(image)[1]
tpl = self.mock_raw_templates.get(image)[1]
tpl = [(CONFIG_FROM_KUBE, t[1]) for t in tpl]
if tpl:
self.assertNotEquals(
Expand All @@ -558,7 +560,7 @@ def test_get_check_tpls_kube(self, mock_client_read):
['service-discovery.datadoghq.com/foo.check_names',
'service-discovery.datadoghq.com/foo.init_configs',
'service-discovery.datadoghq.com/foo.instances'],
self.mock_tpls[image][0]))))
self.mock_raw_templates[image][0]))))

def test_get_config_id(self):
"""Test get_config_id"""
Expand All @@ -570,8 +572,8 @@ def test_get_config_id(self):
expected_ident)
clear_singletons(self.auto_conf_agentConfig)

@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):
Copy link
Member

Choose a reason for hiding this comment

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

Big 🙇 for all these awesome tests!

"""Test read_config_from_store"""
valid_idents = [('nginx', 'nginx'), ('nginx:latest', 'nginx:latest'),
('custom-nginx', 'custom-nginx'), ('custom-nginx:latest', 'custom-nginx'),
Expand All @@ -582,7 +584,13 @@ def test_read_config_from_store(self, issue_read):
for ident, expected_key in valid_idents:
tpl = config_store.read_config_from_store(ident)
# source is added after reading from the store
self.assertEquals(tpl, ('template',) + self.mock_tpls.get(expected_key))
self.assertEquals(
tpl,
{
CONFIG_FROM_AUTOCONF: None,
CONFIG_FROM_TEMPLATE: self.mock_raw_templates.get(expected_key)
}
)
for ident in invalid_idents:
self.assertEquals(config_store.read_config_from_store(ident), [])

Expand All @@ -600,3 +608,72 @@ def test_read_jmx_config_from_store(self, *args):
for check in self.jmx_sd_configs:
key = '{}_0'.format(check)
self.assertEquals(jmx_configs[key], valid_configs[key])

# Template cache
@mock.patch('utils.service_discovery.abstract_config_store.get_auto_conf_images')
def test_populate_auto_conf(self, mock_get_auto_conf_images):
"""test _populate_auto_conf"""
auto_tpls = {
'foo': [['check0', 'check1'], [{}, {}], [{}, {}]],
'bar': [['check2', 'check3', 'check3'], [{}, {}, {}], [{}, {'foo': 'bar'}, {'bar': 'foo'}]],
}
cache = _TemplateCache(issue_read, '')
cache.auto_conf_templates = defaultdict(lambda: [[]] * 3)
mock_get_auto_conf_images.return_value = auto_tpls

cache._populate_auto_conf()
self.assertEquals(cache.auto_conf_templates['foo'], auto_tpls['foo'])
self.assertEquals(cache.auto_conf_templates['bar'],
[['check2', 'check3'], [{}, {}], [{}, {'foo': 'bar'}]])

@mock.patch.object(_TemplateCache, '_issue_read', return_value=None)
def test_get_templates(self, args):
"""test get_templates"""
kv_tpls = {
'foo': [['check0', 'check1'], [{}, {}], [{}, {}]],
'bar': [['check2', 'check3'], [{}, {}], [{}, {}]],
}
auto_tpls = {
'foo': [['check3', 'check5'], [{}, {}], [{}, {}]],
'bar': [['check2', 'check6'], [{}, {}], [{}, {}]],
'foobar': [['check4'], [{}], [{}]],
}
cache = _TemplateCache(issue_read, '')
cache.kv_templates = kv_tpls
cache.auto_conf_templates = auto_tpls
self.assertEquals(cache.get_templates('foo'),
{CONFIG_FROM_TEMPLATE: [['check0', 'check1'], [{}, {}], [{}, {}]],
CONFIG_FROM_AUTOCONF: [['check3', 'check5'], [{}, {}], [{}, {}]]}
)

self.assertEquals(cache.get_templates('bar'),
# check3 must come from template not autoconf
{CONFIG_FROM_TEMPLATE: [['check2', 'check3'], [{}, {}], [{}, {}]],
CONFIG_FROM_AUTOCONF: [['check6'], [{}], [{}]]}
)

self.assertEquals(cache.get_templates('foobar'),
{CONFIG_FROM_TEMPLATE: None,
CONFIG_FROM_AUTOCONF: [['check4'], [{}], [{}]]}
)

self.assertEquals(cache.get_templates('baz'), None)

def test_get_check_names(self):
"""Test get_check_names"""
kv_tpls = {
'foo': [['check0', 'check1'], [{}, {}], [{}, {}]],
'bar': [['check2', 'check3'], [{}, {}], [{}, {}]],
}
auto_tpls = {
'foo': [['check4', 'check5'], [{}, {}], [{}, {}]],
'bar': [['check2', 'check6'], [{}, {}], [{}, {}]],
'foobar': None,
}
cache = _TemplateCache(issue_read, '')
cache.kv_templates = kv_tpls
cache.auto_conf_templates = auto_tpls
self.assertEquals(cache.get_check_names('foo'), set(['check0', 'check1', 'check4', 'check5']))
self.assertEquals(cache.get_check_names('bar'), set(['check2', 'check3', 'check6']))
self.assertEquals(cache.get_check_names('foobar'), set())
self.assertEquals(cache.get_check_names('baz'), set())
21 changes: 17 additions & 4 deletions utils/checkfiles.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Copy link
Member

Choose a reason for hiding this comment

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

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

"""Return the yaml auto_config dict for a check name (None if it doesn't exist)."""
from config import PathNotFound, get_auto_confd_path

Expand All @@ -75,11 +75,11 @@ def get_auto_conf(agentConfig, check_name):
return auto_conf


def get_auto_conf_images(agentConfig):
def get_auto_conf_images(full_tpl=False):
"""Walk through the auto_config folder and build a dict of auto-configurable images."""
from config import PathNotFound, get_auto_confd_path
auto_conf_images = {
# image_name: check_name
# image_name: [check_names] or [[check_names], [init_tpls], [instance_tpls]]
}

try:
Expand All @@ -100,5 +100,18 @@ def get_auto_conf_images(agentConfig):
# extract the supported image list
images = auto_conf.get('docker_images', [])
for image in images:
auto_conf_images[image] = check_name
if full_tpl:
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]]
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

else:
for idx, item in enumerate([check_name, init_tpl, instance_tpl]):
auto_conf_images[image][idx].append(item)
else:
if image in auto_conf_images:
auto_conf_images[image].append(check_name)
else:
auto_conf_images[image] = [check_name]

return auto_conf_images
6 changes: 3 additions & 3 deletions utils/configcheck.py
Original file line number Diff line number Diff line change
Expand Up @@ -94,10 +94,10 @@ def print_templates(agentConfig):
except Exception as ex:
print("Failed to extract configuration templates from the backend:\n%s" % str(ex))

for img, tpl in templates.iteritems():
for ident, tpl in templates.iteritems():
print(
"- Image %s:\n\tcheck names: %s\n\tinit_configs: %s\n\tinstances: %s" % (
img,
"- Identifier %s:\n\tcheck names: %s\n\tinit_configs: %s\n\tinstances: %s" % (
ident,
tpl.get('check_names'),
tpl.get('init_configs'),
tpl.get('instances'),
Expand Down
Loading