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

[auto_conf] Allow multiple instances per check #3311

Merged
merged 1 commit into from
May 30, 2017

Conversation

olivielpeau
Copy link
Member

What does this PR do?

Allows configuring multiple instances of the same check with auto_conf.

Motivation

In some cases (and for some checks), it makes sense to have multiple
instances of the same check running against a container. Example:
php_fpm check when the user wants to run the check against multiple status_urls.

Testing Guidelines

I did some basic manual testing. Haven't written unit tests but if needed I can add some.

Additional Notes

Opening this PR after discussing with @sdwr98 on a use case for #3259 (the use case being: having the ability to define multiple instances of the same check with auto_conf, which is useful for php_fpm for example)

In some cases (and for some checks), it makes sense to have multiple
instances of the same check running against a container. Example:
`php_fpm` check when the user wants to check multiple `status_url`s.

So. instead of only taking into account the first instance, take into
account all of them.
@olivielpeau olivielpeau requested a review from hkaj April 19, 2017 19:47
@olivielpeau olivielpeau added this to the 5.14 milestone Apr 20, 2017
@aerostitch
Copy link
Contributor

Could this also affect service discovery? I'm currently experiencing a similar issue with service discovery / kubernetes annotations taking only the 1st instance defined in the annotations for a check.

@olivielpeau
Copy link
Member Author

@aerostitch This issue shouldn't be affecting kubernetes annotations (or, at least, what the present PR changes should have no effect on the way kubernetes annotations are pulled by the agent).

Feel free to contact our support team at [email protected] and send them a flare if you're having issues setting up kubernetes annotations for the Agent, they'll be able to ask for more specific information about your setup.

@aerostitch
Copy link
Contributor

aerostitch commented May 10, 2017

I've already done that, I was just checking, as the behavior seems very similar, if that was related.
And the fact that the get_check_tpls function was skipping to autoconf for the annotations here:

        # it makes the method skip directly to the auto_conf
        if kwargs.get('auto_conf') is True:
            # When not using a configuration store on kubernetes, check the pod
            # annotations for configs before falling back to autoconf.
            kube_annotations = kwargs.get(KUBE_ANNOTATIONS)
            kube_container_name = kwargs.get(KUBE_CONTAINER_NAME)

made me think that this could be what I was looking for as, when I use the info command, I get this message:
2017-05-10 20:35:05,125 | WARNING | dd.collector | utils.service_discovery.config(config.py:31) | No configuration backend provided for service discovery. Only auto config templates will be used.

I'm actually not sure if the issue is when the config is pulled from Kubernetes or when it's pushed in the datadog config.

@olivielpeau
Copy link
Member Author

Oh okay, hmm the comments on top of the snippet you copied are a bit misleading, but for what this PR fixes the issue lies in the function that's called here: https://github.com/DataDog/dd-agent/blob/5.13.0/utils/service_discovery/abstract_config_store.py#L287 , so it's definitely unrelated with your issue.

@aerostitch
Copy link
Contributor

Thanks for the quick answer.
I'll try to narrow down exactly to where my issue is in the code while talking to Stephen from the support.

@aerostitch
Copy link
Contributor

Just a note: once #3341 is merged you could use the image_3 and image_4 examples I added to the tests for this. Should be pretty straightforward.

Copy link
Contributor

@xvello xvello left a comment

Choose a reason for hiding this comment

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

LGTM, we could indeed extend the tests for that though, see previous comment

Copy link
Member

@hkaj hkaj left a comment

Choose a reason for hiding this comment

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

LGTM, fits well with #3341 (will also require some documentation)

@olivielpeau olivielpeau merged commit dff6845 into master May 30, 2017
@olivielpeau olivielpeau deleted the olivielpeau/allow-auto-conf-multiple-instances branch May 30, 2017 15:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants