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

[autodiscovery] annotations with multi instances bug #3478

Merged
merged 1 commit into from
Aug 21, 2017

Conversation

CharlyF
Copy link
Contributor

@CharlyF CharlyF commented Aug 18, 2017

What does this PR do?

Bug fix of #3341

Annotations with multi instances configuration leads to check level tags be added to the container level tags array.

While adding container tags with this method, performs a shallow copy of the container tag array to avoid adding tags from the previous instance ( which would come from this for loop), contains its check tags.

Motivation

Issue encountered by a customer.

Testing Guidelines

Instantiate a pod with annotations containing several instances (along the lines of this example)
The agent will detect the check but it will add the config level tags to the container level ones.
e.g.:

 (u'num.empl',
  1503048087,
  300024.0,
  {'hostname': 'i-0xxx',
   'tags': [u'kube_deployment:repl',
            u'kube_namespace:default',
            u'kube_replica_set:repl',
            u'env:prd',
            u'dbinstanceidentifier:dbi2',
            u'name:mysql',
            u'account:analytics',
            u'dbinstanceidentifier:dbi',
            u'team:data',
            u'image_tag:3.0',
            u'docker_image:gcr.io/google_containers/pause-amd64:3.0',
            u'image_name:gcr.io/google_containers/pause-amd64',
            u'table:playcount',
            u'dbinstanceidentifier:dbi2',
            u'env:prd',
            u'account:analytics',
            u'team:data'],
   'type': 'gauge'}),

As per the above pod yaml, this metric should only be tagged with dbinstanceidentifier:dbi2.
With the change, we have:

 (u'num.empl',
  1503050652,
  300024.0,
  {'hostname': 'i-0xxx',
   'tags': [u'kube_deployment:repl',
            u'kube_namespace:default',
            u'kube_replica_set:repl',
            u'env:prd',
            u'dbinstanceidentifier:dbi2',
            u'name:mysql',
            u'account:analytics',
            u'team:data',
            u'image_tag:3.0',
            u'docker_image:gcr.io/google_containers/pause-amd64:3.0',
            u'image_name:gcr.io/google_containers/pause-amd64',
            u'table:playcount',
            u'dbinstanceidentifier:dbi2',
            u'env:prd',
            u'account:analytics',
            u'team:data'],
   'type': 'gauge'}),

"""Add container tags to instance templates and build a
dict from template variable names and their values."""
var_values = {}
c_image = state.inspect_container(c_id).get('Config', {}).get('Image', '')

# add default tags to the instance
# add only default c_tags to the instance to avoid duplicate tags from conf
tags = copy.copy(c_tags)
Copy link
Member

Choose a reason for hiding this comment

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

Good catch! Thanks for digging in this issue.

If that's the only place we use it I'd rather not import copy and do tags = c_tags[:] instead. It will create a new list with all the elements of c_tags.

Copy link
Contributor

Choose a reason for hiding this comment

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

Tested with @hkaj's suggestion and it fixed the issue I had with way too much tags on my "query" metrics for the mysql integration configured in multi-instances mode via k8s annotations.

if c_tags:
tags = c_tags[:] # shallow copy of the c_tags array
else
tags = []
Copy link
Member

Choose a reason for hiding this comment

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

what about this?

def _fill_tpl(self, state, c_id, instance_tpl, variables, c_tags=[]):
    ...
    tags = c_tags[:]

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.

👌

@CharlyF CharlyF force-pushed the charly/sd_multiannotations_tags branch from 53a8ba9 to cf4f8a9 Compare August 21, 2017 13:49
@xvello xvello added this to the 5.17 milestone Aug 21, 2017
@CharlyF CharlyF force-pushed the charly/sd_multiannotations_tags branch 2 times, most recently from d7806af to 1ffbbc5 Compare August 21, 2017 15:25
…to check level tags be added to container level tags array
@CharlyF CharlyF force-pushed the charly/sd_multiannotations_tags branch from 1ffbbc5 to 0e6ff98 Compare August 21, 2017 16:10
@hkaj hkaj merged commit 8ac4fc9 into master Aug 21, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants