-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Kubernetes metadata enhancements #22189
Conversation
Signed-off-by: chrismark <chrismarkou92@gmail.com>
930933e
to
f9a64cb
Compare
Pinging @elastic/integrations-platforms (Team:Platforms) |
Signed-off-by: chrismark <chrismarkou92@gmail.com>
Pinging @elastic/ingest-management (Team:Ingest Management) |
Signed-off-by: chrismark <chrismarkou92@gmail.com>
💚 Flaky test reportTests succeeded. Expand to view the summary
Test stats 🧪
|
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>
- name: node.hostname | ||
type: keyword | ||
description: > | ||
Kubernetes hostname as reported by the node’s kernel |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking about this comment #20434 (comment), perhaps we should store this value directly in host.name
(instead of storing it in kubernetes.node.hostname
). What I am not sure is if then we should also populate host.id
or other inventory fields and with what values.
Other option can be to postpone the decision and copy the field if we decide to use host.name
.
@exekias @kaiyan-sheng wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using host.name
sounds like a good idea to me, this complies with the current definition we use. It would be good to do some testing to see if what add_host_metadata
reports is the same thing reported by Kubernetes. For instance, when you change the hostname by hand, with and without setting a fqdn.
I think we need a deeper discussion around where host.id
should be set, what will happen when add_host_metadata
kicks in but host.name
is already set by add_kubernetes_metadata
? What will happen if add_host_metadata
happens before add_kubernetes_metadata
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds good to me! How about keeping the scope of this PR to just adding the node's metadata and opening a follow up issue so as to investigate further about host.name
in separate story?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about keeping the scope of this PR to just adding the node's metadata and opening a follow up issue so as to investigate further about host.name in separate story?
LGTM, worst case we will duplicate this field.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need a deeper discussion around where
host.id
should be set, what will happen whenadd_host_metadata
kicks in buthost.name
is already set byadd_kubernetes_metadata
? What will happen ifadd_host_metadata
happens beforeadd_kubernetes_metadata
?
Agree. This is definitely worth some time to test and discuss. When users have more than one processor enabled for Metricbeat, overwriting the same fields will be a problem. We also have this issue to discuss moving adding host.name
into add_host_metadata
. But then both host.id
and host.name
will have this same issue.
@@ -46,6 +48,7 @@ func (n *node) Generate(obj kubernetes.Resource, opts ...FieldOptions) common.Ma | |||
|
|||
meta := n.resource.Generate("node", obj, opts...) | |||
// TODO: Add extra fields in here if need be | |||
meta.Put("node.hostname", n.hostname) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we use the hostname in obj.Status.Addresses[...].Address
? I am concerned that when using cluster scope we can be setting here the hostname of the observer, and not the one the event belongs to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
++ good catch!
nodeWatcher kubernetes.Watcher, | ||
namespaceWatcher kubernetes.Watcher, | ||
metaConf *AddResourceMetadataConfig, | ||
hostname string) MetaGen { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are passing the hostname
in many places, I wonder if we may be using the wrong hostname when using cluster scope.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
++ good catch!
Signed-off-by: chrismark <chrismarkou92@gmail.com>
@jsoriano thanks for the suggestion! I made a change to this direction so feel free to have another look :) @exekias @kaiyan-sheng any thoughts about #22189 (comment)? |
👍🏼 Thanks for the review @jsoriano! I ended up removing |
Signed-off-by: chrismark <chrismarkou92@gmail.com>
I think we need to keep something to selectively include annotations, maybe we can keep |
Signed-off-by: chrismark <chrismarkou92@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Co-authored-by: Jaime Soriano Pastor <jaime.soriano@elastic.co>
c80b612
to
8002c9c
Compare
(cherry picked from commit 95a88a6)
What does this PR do?
kubernetes.host.hostname
on node metadata,add_resource_metadata
required to enabled) for both events coming from autodiscovered pods but also for events enriched byadd_kubernetes_metadata
add_resource_metadata
required to enabled) for both events coming from autodiscovered pods but also for events enriched byadd_kubernetes_metadata
add_resource_metadata
section for autodiscover and add_kubernetes_metadata.Why is it important?
This information is useful in various ways. See more on the respective issue: #20434
How to test this PR manually
Test with autodiscover provider
kubernetes.node.hostname
field is populated.Test with add_kubernetes_metadata processor
kubernetes.node.hostname
field is populated as well askubernetes.node.*
andkubernetes.namespace.*
.Related issues
kubernetes.node.hostname
field with the actual hostname of the node #20434Screenshots
Filebeat & add_kubernetes_metadata:
Autodiscover: