[kubernetes] Only use kube annotations for sd once per pod #2901

mikekap · 2016-10-08T21:57:33Z

What does this PR do?

Currently the kube annotations are used for service discovery once per docker container. This causes "duplicate" checks to be instantiated if you have sidecar containers in your kubernetes pod.

This PR changes the annotations to annotate a specific container to monitor (by name).

Motivation

Seeing dup checks in my cluster :X.

Testing Guidelines

Unit tests. I will also be deploying this to my internal cluster and monitoring.

Additional Notes

The various tuples flying around between config & backend are getting a bit old. AFAICT there are 4 distinct sets:

Config->Backend: [(source, (check_name, init_config, instance_tpl))]
_get_config_templates -> _get_check_configs: [(source, (check_name, init_config, instance_tpl, variables))]
_get_check_configs -> get_configs: [(source, (check_name, init_config, instance))]

It's getting a bit weird. I removed trace_configs as a variation because that was getting to be a bit too much. I think there was also a bug where some code didn't handle this case.

hkaj · 2016-10-13T12:20:05Z

thanks @mikekap i'll review it shortly

hkaj · 2016-10-18T13:28:03Z

Hi @mikekap
Thanks for contributing (again!) 🎉

That's an issue I should have caught before. Our lack of integration testing for k8s is getting punishing.
Also, thank you for removing the trace_configs thing. It really didn't age well.

Your approach in this PR works, but I wonder if we shouldn't modify the annotation format instead of ignoring all but one container per pod. Running checks against several containers in the same pod should be an option. What do you think of changing annotation keys like so?

com.datadoghq.sd/<full_image_name>/check_names
com.datadoghq.sd/<full_image_name>/init_configs
com.datadoghq.sd/<full_image_name>/instances

AbstractConfigStore._get_kube_config has both annotations and the identifier (which should already be full_image_name in this case) so we can easily do the matching there.

This feature is not documented yet so it shouldn't be a problem.
What do you think?

mikekap · 2016-10-19T06:29:52Z

We could definitely add support for monitoring all containers. It would probably be more intuitive to use the kubernetes container name rather than the image name though. One thing I'm not sure of - is the multi-container model more flexible? Currently, even though we only init checks for one of the containers, you can easily monitor the other containers in a pod by just changing the port (since all containers in a pod share the same ip).

On a higher level, the kubernetes model is a bit shoehorned into the docker model here. The "unit" of kubernetes is a pod. Ideally we'd want to monitor pods no matter how many containers are running or not running in those pods. That is, if some container fails to start, or docker is down, the monitoring should still apply. That's a bit outside the scope of this PR to fix though.

hkaj · 2016-10-19T11:03:34Z

Using the kubernetes container name is fine since it's also quick to extract and guaranteed to exist. Go for it 👍
The reason for the multi-container model is that to resolve template variables we use the docker inspect of the container (by looking through all ports used by the container and selecting the right one for %%port%% for instance). So tying the template to a container rather than a pod seems to make more sense (but I might be wrong).
The thing is service discovery is not targeting only kubernetes but container environments at large. So it is likely to stay container-centric and not pod-centric, at least for now. We have some other work in progress to make pod/service/deployment monitoring easier and more powerful though. Notably improvements to the kubernetes integration. So stay tuned ;)

mikekap · 2016-10-21T06:41:15Z

Done. I haven't tested this outside of unit tests yet since I would need to migrate my existing usage.

hkaj

Thanks for the update @mikekap
I left one small comment in the code, looks good otherwise. I'll give it a try tomorrow.

hkaj · 2016-10-24T17:11:42Z

utils/service_discovery/sd_docker_backend.py

@@ -223,6 +230,14 @@ def _get_additional_tags(self, container_inspect, *args):
            tags.append('pod_name:%s' % pod_metadata.get('name'))
        return tags

+    def _get_kube_container_name(self, c_id):
+        pods = self.kubeutil.retrieve_pods_list().get('items', [])


not super excited about calling this for every container, can we maybe build the mapping c_id -> c_name once before entering this loop https://github.com/mikekap/dd-agent/blob/4790dd8df8417900a62fb776f0e10fa3491fb749/utils/service_discovery/sd_docker_backend.py#L258 to reduce api calls? It should be fine since it's coming from the kubelet but with nodes running hundreds of containers if there's a hiccups we'll feel it.

hkaj · 2016-10-27T16:52:41Z

Looks like this scheme is not going to work:

The Pod "redis-django" is invalid:
* metadata.annotations: Invalid value: "com.datadoghq.sd/frontend/check_names": must match the regex ([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9] (e.g. 'MyName' or 'my.name' or '123-abc') with an optional DNS subdomain prefix and '/' (e.g. 'example.com/MyName'

Let's use dots instead of slashes?

mikekap · 2016-10-27T20:17:19Z

Sounds good. I'm a bit swamped this week so I'll get around to making those changes on the weekend if you don't mind. Sorry for the delay

hkaj · 2016-10-28T12:17:51Z

utils/service_discovery/abstract_config_store.py

@@ -26,7 +26,8 @@
 INIT_CONFIGS = 'init_configs'
 INSTANCES = 'instances'
 KUBE_ANNOTATIONS = 'kube_annotations'
-KUBE_ANNOTATION_PREFIX = 'com.datadoghq.sd/'
+KUBE_CONTAINER_NAME = 'kube_container_name'
+KUBE_ANNOTATION_PREFIX = 'sd.datadoghq.com'


typo I guess, it's com.datadoghq.sd

So I may have snuck this one in there. I changed the annotation to be service-discovery.datadoghq.com/checks.XXXX. I've realized that kube annotations don't follow the "reverse dns" scheme at all and I didn't want to deviate from that. Let me know if you'd prefer to change it back to what you were suggesting.

…r the first container in a pod. Otherwise you could end up instantiating several instances of the same check when the pod contains more than one container. Also get rid of the trace_config madness

Particularly this goes from O(4-5N) calls to the kubernetes API to 1.

hkaj

Hey @mikekap sorry for the delay here. I left 2 comments, once they're fixed we'll merge this. Don't worry about the conflicts I'll resolve them at merge time. It's about code I changed recently.

Thanks a bunch!

hkaj · 2016-11-18T19:17:57Z

utils/service_discovery/abstract_config_store.py

-            check_names = json.loads(kube_annotations[KUBE_ANNOTATION_PREFIX + CHECK_NAMES])
-            init_config_tpls = json.loads(kube_annotations[KUBE_ANNOTATION_PREFIX + INIT_CONFIGS])
-            instance_tpls = json.loads(kube_annotations[KUBE_ANNOTATION_PREFIX + INSTANCES])
+            prefix = '{}/{}.'.format(KUBE_ANNOTATION_PREFIX, kube_container_name)


We shouldn't require the container name here, it's already defined in the metadata.
prefix = '{}.'.format(KUBE_ANNOTATION_PREFIX) is enough.
and the annotations would look like

annotations: service-discovery.datadoghq.com.check_names: '["supervisord"]' service-discovery.datadoghq.com.init_configs: '[{}]' service-discovery.datadoghq.com.instances: '[{"name": "super", "host": "%%host%%"}]'

HK - I think you might have forgotten some of the context from above. The main point of doing this PR is so we instantiate the checks above once per pod rather than once per container (since pods can have a few containers and you can't annotate containers directly in kubernetes; this was previously creating N instances of the checks above where N=containers in a pod).

welp, yes indeed.

hkaj · 2016-11-22T16:30:36Z

utils/service_discovery/sd_docker_backend.py

        """Get the list of checks applied to a container from the identifier_to_checks cache in the config store.
        Use the DATADOG_ID label or the image."""
+        inspect = state.inspect_container(c_id)


So we actually have another issue to take care of here, which is that if the container which the ID is from is already removed at this point, inspect will be None and we won't reload anything although there's a check that now fails on a missing container.

To be clear it's a pre-existing bug, not yours, but let's fix it here. In this situation we can't really decide which check to reload since the container isn't here anymore, so I think we should trigger a full reload.
Adding this block here does the job:

# if the container was removed we can't tell which check is concerned # so we have to reload everything if not inspect: self.reload_check_configs = True return

hkaj · 2016-11-28T15:05:48Z

🎉 thanks again @mikekap

mikekap · 2016-11-28T18:45:40Z

Wow - thanks for getting this in @hkaj! I was just coming back from the break and getting ready to tackle merging this. Sorry you had to go through that.

hkaj · 2016-11-28T18:55:22Z

No worries, it was an easy conflict. I'll finish polishing a documentation update for it and we should be good to go!

mikekap closed this Oct 8, 2016

mikekap reopened this Oct 8, 2016

mikekap closed this Oct 9, 2016

mikekap reopened this Oct 9, 2016

mikekap force-pushed the kube-sd-only-one branch 2 times, most recently from e70e5b5 to 118db09 Compare October 9, 2016 04:57

irabinovitch assigned hkaj Oct 10, 2016

irabinovitch added triage community labels Oct 10, 2016

mikekap force-pushed the kube-sd-only-one branch from 118db09 to d86da7e Compare October 21, 2016 06:40

olivielpeau added this to the 5.10.0 milestone Oct 21, 2016

hkaj reviewed Oct 24, 2016

View reviewed changes

hkaj reviewed Oct 28, 2016

View reviewed changes

mikekap added 6 commits October 31, 2016 22:36

[kubernetes] Only use kubernetes annotations for service discovery fo…

dd235a8

…r the first container in a pod. Otherwise you could end up instantiating several instances of the same check when the pod contains more than one container. Also get rid of the trace_config madness

Change the annotation style

8b677c0

Kill dedup key

85bacde

Slight CR

527accc

Lint

fb0ea7c

Cache all calls to kubernetes and docker when doing service discovery

3ae3d38

Particularly this goes from O(4-5N) calls to the kubernetes API to 1.

mikekap force-pushed the kube-sd-only-one branch from 4790dd8 to 3ae3d38 Compare November 1, 2016 06:36

mikekap closed this Nov 2, 2016

mikekap reopened this Nov 2, 2016

remh added this to the 5.10.1 milestone Nov 2, 2016

remh removed this from the 5.10.0 milestone Nov 2, 2016

mikekap mentioned this pull request Nov 3, 2016

[kubernetes] Use pod annotations for service discovery #2794

Closed

truthbk modified the milestones: 5.11.0, 5.10.1 Nov 17, 2016

hkaj requested changes Nov 22, 2016

View reviewed changes

hkaj approved these changes Nov 25, 2016

View reviewed changes

hkaj merged commit 3ae3d38 into DataDog:master Nov 28, 2016

hkaj added bugfix service discovery and removed triage labels Nov 28, 2016

mikekap deleted the kube-sd-only-one branch November 28, 2016 18:45

sjenriquez mentioned this pull request Jan 20, 2017

[service_discovery][jmx] fix yaml parsing #3139

Merged

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[kubernetes] Only use kube annotations for sd once per pod #2901

[kubernetes] Only use kube annotations for sd once per pod #2901

mikekap commented Oct 8, 2016 •

edited

Loading

hkaj commented Oct 13, 2016

hkaj commented Oct 18, 2016

mikekap commented Oct 19, 2016

hkaj commented Oct 19, 2016

mikekap commented Oct 21, 2016 •

edited

Loading

hkaj left a comment

hkaj Oct 24, 2016

hkaj commented Oct 27, 2016

mikekap commented Oct 27, 2016

hkaj Oct 28, 2016

mikekap Nov 1, 2016

hkaj left a comment

hkaj Nov 18, 2016

mikekap Nov 24, 2016

hkaj Nov 25, 2016

hkaj Nov 22, 2016

hkaj commented Nov 28, 2016

mikekap commented Nov 28, 2016

hkaj commented Nov 28, 2016

[kubernetes] Only use kube annotations for sd once per pod #2901

[kubernetes] Only use kube annotations for sd once per pod #2901

Conversation

mikekap commented Oct 8, 2016 • edited Loading

What does this PR do?

Motivation

Testing Guidelines

Additional Notes

hkaj commented Oct 13, 2016

hkaj commented Oct 18, 2016

mikekap commented Oct 19, 2016

hkaj commented Oct 19, 2016

mikekap commented Oct 21, 2016 • edited Loading

hkaj left a comment

Choose a reason for hiding this comment

hkaj Oct 24, 2016

Choose a reason for hiding this comment

hkaj commented Oct 27, 2016

mikekap commented Oct 27, 2016

hkaj Oct 28, 2016

Choose a reason for hiding this comment

mikekap Nov 1, 2016

Choose a reason for hiding this comment

hkaj left a comment

Choose a reason for hiding this comment

hkaj Nov 18, 2016

Choose a reason for hiding this comment

mikekap Nov 24, 2016

Choose a reason for hiding this comment

hkaj Nov 25, 2016

Choose a reason for hiding this comment

hkaj Nov 22, 2016

Choose a reason for hiding this comment

hkaj commented Nov 28, 2016

mikekap commented Nov 28, 2016

hkaj commented Nov 28, 2016

mikekap commented Oct 8, 2016 •

edited

Loading

mikekap commented Oct 21, 2016 •

edited

Loading