Fix for Rancher Service Discovery and adding custom tag support by AlexTawse · Pull Request #3366 · DataDog/dd-agent

AlexTawse · 2017-06-01T22:29:30Z

What does this PR do?

Restores Service Discovery functionality in Rancher in the event that Docker container metadata isn't populated as expected.
Incorporates functionality previously created by @janeczku at Rancher to automatically tag hosts based on their labels in Rancher.

Motivation

In our experience running a modified version of the Datadog agent in production, we've seen that Rancher is not guaranteed to populate Docker container metadata correctly as seen with the docker inspect command. We believe this is a bug with Rancher which we have separately raised with them. However, the result is that Service Discovery does not always work on the current release of Rancher, which is a major issue.

To address this, we build on the work recently contributed by @zippolyte and in our own fork to introduce an integration with the Rancher Metadata API. When resolving host IP address and exposed ports for the purposes of Service Discovery, this PR adds the ability to read these from the Rancher Metadata API should all previous attempts to resolve these values fail.

In addition, we've taken this opportunity to integrate some of the work previously carried out by @janeczku with this customisation of the agent: https://github.com/janeczku/datadog-rancher-init. We've added a new configuration file flag: collect_rancher_host_labels. With this flag set to true, the agent will check for connectivity to the Metadata API of a local Rancher environment, and if successful will read the metadata labels of the host from the API and set each entry as a tag on the host in Datadog. See the below example of tags automatically inferred in this way:

The net result is that Service Discovery is seen to be much more stable when running under Rancher, and Rancher users can deploy the "vanilla" agent directly from Datadog whilst retaining the functionality from datadog-rancher-init.

Additional Notes

Before this PR can be merged, the following PRs must be merged into their respective repos. These PRs constitute a single piece of work.

dd-agent-omnibus: [core] Adding rancher-metadata dependency dd-agent-omnibus#176
(adding the rancher-metadata dependency, source: https://github.com/m4ce/rancher-metadata-python)
omnibus-software: [core] Adding rancher-metadata-python dependency omnibus-software#147
(adding rancher-metadata dependency)

This PR has a co-dependency with this one:

integrations-core: Variable renaming in support of a PR into dd-agent integrations-core#455
(updating references to variables renamed and relocated in this PR)

Finally, this PR is dependent upon this one:

docker-dd-agent: Environment variable for Rancher custom tags docker-dd-agent#206
(adding environment variable RANCHER_HOST_LABELS to update the datadog.conf file)

hkaj

Hi @alextbrandwatch
That's a great addition, thanks a lot! I left a few comments but it's already in great shape. I'm happy to see that Rancher is getting some interest as well. Having a RancherUtil in utils/orchestrator will definitely make it easier to improve our support of it.

I'll have a look at the related PRs as well. Thanks again!

hkaj · 2017-06-02T11:05:33Z

-RANCHER_CONTAINER_NAME = 'io.rancher.container.name'
-RANCHER_CONTAINER_IP = 'io.rancher.container.ip'
-RANCHER_STACK_NAME = 'io.rancher.stack.name'
-RANCHER_SVC_NAME = 'io.rancher.stack_service.name'


nice, you can get rid of https://github.com/alextbrandwatch/dd-agent/blob/47002bae23f99c7b38697d91036ec8be453d88c4/utils/dockerutil.py#L78 as well then

hkaj · 2017-06-02T11:07:15Z

+# All rights reserved
+# Licensed under Simplified BSD License (see LICENSE)
+
+from .rancherutil import RancherUtil  # noqa: F401


could you move this to utils/orchestrator/__init__.py please?

hkaj · 2017-06-02T11:07:48Z

@@ -0,0 +1,69 @@
+import logging


and this file to utils/orchestrator/rancherutil.py

hkaj · 2017-06-02T11:12:12Z

+    def is_rancher():
+        if RancherUtil._is_rancher is None:
+            try:
+                response = requests.get(url=RancherUtil.METADATA_URL)


Could you add a timeout here? In some network configs that might not resolve but not refuse the connection immediately either. 1 second seems reasonable to me, don't know if you've seen it take longer than that?

1 second seems sensible, I'm fairly sure the request goes to another container on the same machine. I've never known it not respond immediately.

hkaj · 2017-06-02T12:20:51Z

+                    tags.append('rancher_host_docker_version:%s' % v)
+
+                elif k == RancherUtil.HOST_LINUX_KERNEL_VERSION_LABEL:
+                    tags.append('rancher_host_linux_kernel_version:%s' % v)


I don't think those 3 fit as tags in service discovery. They're good as host metadata but could you remove them from here?

Yes I wondered about this - will remove. Thanks!

AlexTawse · 2017-06-02T15:06:50Z

Hi @hkaj, thank you for the speedy review. I've made the changes as requested: relocating rancherutil.py into the orchestrator folder and removing the unnecessary container tags. I've built and re-deployed the agent internally and everything looks good from what I can see.

I've also pushed up changes based on your comments on the other PRs.

Thanks again - I appreciate your help!

hkaj

LGTM, waiting on 5.14.x to be branched off and we'll merge 🎉

AlexTawse · 2017-06-02T15:47:28Z

Thanks so much! 🎉

janeczku · 2017-06-10T18:35:15Z

@alextbrandwatch so great! 👏 🎉

hkaj · 2017-06-12T16:41:40Z

Hey @alextbrandwatch sorry we released a change that conflicts with your PR, I'll rebase your commits shortly.

AlexTawse · 2017-06-20T10:57:21Z

Thanks for the heads up @hkaj, looks like a bit of a change to how orchestration environments are detected and managed, a rebase would be appreciated though I will take a look myself when I'm next able to 🙇

AlexTawse · 2017-07-04T16:34:57Z

Hi @hkaj, I've rebased this PR against the changes that were made to the way that Orchestrator utility classes were handled. The result is that this PR is now a little simpler.

I've also removed the check_rancher_host_labels configuration file option that I proposed, given that a collector_orchestrator_tags option has now been added.

I'll get this tested and deployed to our test environment as soon as possible, but in the meantime I would very much appreciate a re-review to see if I've missed anything.

Thank you!

hkaj

thanks for taking care of it @alextbrandwatch and apologies, I've bee pretty swamped with other stuff. Left a few comments but it's looking good overall!

hkaj · 2017-07-06T11:48:22Z

+    def is_detected():
+        return RancherUtil.is_rancher()
+
+    def _get_cacheable_tags(self, cid=None, co=None):


cid is not optional

also maybe make the default value for co {}? Otherwise co.get will error out

hkaj · 2017-07-06T11:56:58Z

+
+    def __init__(self):
+        BaseUtil.__init__(self)
+        self.needs_inspect_config = True


it doesn't look like you need config since you only use labels. Can you look at how ecsutil does it?
Here are the interesting parts:

https://github.com/DataDog/dd-agent/blob/5.14.1/utils/orchestrator/ecsutil.py#L48-L70 for collecting the tags from a docker ps (faster than one inspect per container)

https://github.com/DataDog/dd-agent/blob/5.14.1/utils/orchestrator/ecsutil.py#L74-L87 for returning the tags

hkaj · 2017-07-06T12:17:39Z

+        return tags
+
+    @staticmethod
+    def is_rancher():


any reason not to rename it to is_detected and get rid of L29-31?

masci · 2018-04-12T13:43:24Z

Hi @alextbrandwatch,

thanks for your contribution. We've discussed this and while we agree the feature has merit, we decided to dedicate engineering resources to the new Agent thus limiting the work on this release line to bugfixes only.

AlexTawse force-pushed the supporting-rancher-for-service-discovery-using-api branch 2 times, most recently from 61e31d5 to 47002ba Compare June 1, 2017 22:59

hkaj self-requested a review June 2, 2017 09:20

hkaj self-assigned this Jun 2, 2017

hkaj requested changes Jun 2, 2017

View reviewed changes

AlexTawse force-pushed the supporting-rancher-for-service-discovery-using-api branch from e8b551b to 9f3a837 Compare June 2, 2017 14:36

hkaj approved these changes Jun 2, 2017

View reviewed changes

AlexTawse force-pushed the supporting-rancher-for-service-discovery-using-api branch from 9f3a837 to 8f29e6d Compare July 4, 2017 10:56

AlexTawse added 2 commits July 4, 2017 15:44

[core] Integrating Rancher Metadata API as Service Discovery fallback

9750708

[core] Refactoring after feedback: module relocation

1f9077f

AlexTawse force-pushed the supporting-rancher-for-service-discovery-using-api branch from a256410 to 83c62b6 Compare July 4, 2017 15:37

[core] Making RancherUtil subclass BaseUtil

c2beea0

AlexTawse force-pushed the supporting-rancher-for-service-discovery-using-api branch from 83c62b6 to c2beea0 Compare July 4, 2017 16:32

jeremy-lq added the community label Jul 5, 2017

hkaj requested changes Jul 6, 2017

View reviewed changes

hush-hush added this to the 5.16 milestone Jul 10, 2017

truthbk modified the milestones: 5.16, 5.17 Jul 25, 2017

olivielpeau added this to the 5.18.0 milestone Aug 18, 2017

olivielpeau removed this from the 5.17 milestone Aug 18, 2017

truthbk modified the milestones: 5.18.0, 5.19.0 Oct 2, 2017

olivielpeau modified the milestones: 5.19.0, 5.20.0 Oct 31, 2017

hush-hush modified the milestones: 5.20.0, 5.21.0 Nov 13, 2017

xvello modified the milestones: 5.21.0, Future Dec 27, 2017

masci closed this Apr 12, 2018

Uh oh!

Conversation

AlexTawse commented Jun 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Motivation

Additional Notes

Uh oh!

hkaj left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlexTawse commented Jun 2, 2017

Uh oh!

hkaj left a comment

Choose a reason for hiding this comment

Uh oh!

AlexTawse commented Jun 2, 2017

Uh oh!

janeczku commented Jun 10, 2017

Uh oh!

hkaj commented Jun 12, 2017

Uh oh!

AlexTawse commented Jun 20, 2017

Uh oh!

AlexTawse commented Jul 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hkaj left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

masci commented Apr 12, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

AlexTawse commented Jun 1, 2017 •

edited

Loading

AlexTawse commented Jul 4, 2017 •

edited

Loading