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

Fix k8s metadata issue #16834

Merged
merged 11 commits into from
Mar 12, 2020
Merged

Fix k8s metadata issue #16834

merged 11 commits into from
Mar 12, 2020

Conversation

ChrsMark
Copy link
Member

@ChrsMark ChrsMark commented Mar 5, 2020

What does this PR do?

This PR fixes labels in k8s metadata. Issues found in 7.6 version: https://discuss.elastic.co/t/how-to-get-kubernetes-node-labels-with-metricbeat/220667/6

The proposed solution is to add kubernetes.labels.* in all resources (node, namespace etc) so as to cover cases like node metricset. Also when node, namespace, are part of other resources like pod then their labels are part of the sub-resource and not the main resource.

The proposed solution is to add kubernetes.labels.* in node resources so as to cover cases like node metricset. Also when node, is part of other resources like pod then their labels are part of the sub-resource and not the main resource.

Example with node as main resource :

{
    "node": {
        "name": "testnode",
        "uid":  "uid",
    },
    "labels": {
        "nodekey": "nodevalue",
    }
}

Example with sub-resources:

{
    "pod": {
        "name": "obj",
        "uid":  "uid",
    },
    "namespace": "default",
    "namespace_uid": "uid",
    "namespace_labels": {
            "nskey": "nsvalue",
    },
    "node": {
        "name": "testnode",
        "uid":  "uid",
        "labels": {
            "nodekey": "nodevalue",
        },
    },
    "labels": {
        "foo": "bar",
    },
    "annotations": {
        "app": "production",
    }
}

Why is it important?

Currently the metadata are not populated properly.

Checklist

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have made corresponding change to the default configuration files
  • I have added tests that prove my fix is effective or that my feature works

How to test this PR locally

TBA

Related issues

@ChrsMark ChrsMark added bug containers Related to containers use case Team:Integrations Label for the Integrations team autodiscovery Team:Platforms Label for the Integrations - Platforms team labels Mar 5, 2020
@ChrsMark ChrsMark self-assigned this Mar 5, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/integrations-platforms (Team:Platforms)

@ChrsMark ChrsMark added v7.6.2 v7.7.0 needs_backport PR is waiting to be backported to other branches. labels Mar 5, 2020
Signed-off-by: chrismark <chrismarkou92@gmail.com>
Copy link
Member

@jsoriano jsoriano left a comment

Choose a reason for hiding this comment

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

I am fine with this change for 8.0, but I am not sure of seeing the advantages of backporting this to 7.x versions.

@josharrington
Copy link

Has there been any update on this issue? This is a significant breaking change moving to 7.6.x and it makes it impossible for us to group nodes or pods by kubernetes cluster.

Signed-off-by: chrismark <chrismarkou92@gmail.com>
Signed-off-by: chrismark <chrismarkou92@gmail.com>
Signed-off-by: chrismark <chrismarkou92@gmail.com>
Signed-off-by: chrismark <chrismarkou92@gmail.com>
Signed-off-by: chrismark <chrismarkou92@gmail.com>
@ChrsMark
Copy link
Member Author

@jsoriano @exekias I did some tests with this approach and seems to fix the issue with labels in general while preserving namespace schema for now until we decide on a final schema along with a breaking change. Let me know what you think!

Signed-off-by: chrismark <chrismarkou92@gmail.com>
Copy link
Contributor

@blakerouse blakerouse left a comment

Choose a reason for hiding this comment

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

The approach seems acceptable to me, at least until 8.0.

@exekias
Copy link
Contributor

exekias commented Mar 11, 2020

LGTM!

@exekias
Copy link
Contributor

exekias commented Mar 11, 2020

I am fine with this change for 8.0, but I am not sure of seeing the advantages of backporting this to 7.x versions.

I understand that after the latest updates we are fine with backporting this, right @jsoriano?

@jsoriano
Copy link
Member

I am fine with this change for 8.0, but I am not sure of seeing the advantages of backporting this to 7.x versions.

I understand that after the latest updates we are fine with backporting this, right @jsoriano?

Looks good now, yes. Thanks!

Signed-off-by: chrismark <chrismarkou92@gmail.com>
@ChrsMark ChrsMark merged commit 034ee55 into elastic:master Mar 12, 2020
ChrsMark added a commit to ChrsMark/beats that referenced this pull request Mar 12, 2020
@ChrsMark ChrsMark removed the needs_backport PR is waiting to be backported to other branches. label Mar 12, 2020
ChrsMark added a commit to ChrsMark/beats that referenced this pull request Mar 12, 2020
ChrsMark added a commit that referenced this pull request Mar 12, 2020
* Fix k8s metadata issue (#16834)

(cherry picked from commit 034ee55)

* Update CHANGELOG.next.asciidoc
ChrsMark added a commit that referenced this pull request Mar 12, 2020
* Fix k8s metadata issue (#16834)


(cherry picked from commit 034ee55)

* Update CHANGELOG.next.asciidoc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autodiscovery bug containers Related to containers use case Team:Integrations Label for the Integrations team Team:Platforms Label for the Integrations - Platforms team v7.6.2 v7.7.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants