Add pod-uid support for add_kubernetes_metadata #7072

mariomechoulam · 2018-05-10T20:50:13Z

Hello everyone!

I am opening this pull request to gather some feedback on the approach.
There are a few things that can be cleaned up and improved for sure, as this is my first try with Go, but the basic should be working.
I was hesitating whether to create a completely new Matcher and duplicate a lot of the existing logic or extend the current one. Extending did not feel entirely right, because the LogPathMatcher was focused on container ID - nevertheless it is still a log_path matcher. Thus this middle-ground approach in which I am adding a new configuration parameter for the Matcher; it should be seamlessly backwards compatible.

I believe this is the configuration that should make it work...

processors:
  - add_kubernetes_metadata:
      in_cluster: true
      default_indexers.enabled: false
      default_matchers.enabled: false
      include_pod_uid: true
      indexers:
        - pod_uid: ~
      matchers:
        - logs_path:
            logs_path: /var/lib/kubelet/pods/
            resource_type: pod

My understanding is that, ideally, an integration test of some sort would be useful - at least to test that the above configuration will indeed produce the desired functionality. Will welcome any guidance :)

Cheers!

elasticmachine · 2018-05-10T20:50:14Z

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

houndci-bot · 2018-05-10T20:50:28Z

libbeat/processors/add_kubernetes_metadata/indexers_test.go

+func TestPodUidIndexer(t *testing.T) {
+	var testConfig = common.NewConfig()
+
+	podUidIndexer, err := NewPodUidIndexer(*testConfig, metagen)


var podUidIndexer should be podUIDIndexer

houndci-bot · 2018-05-10T20:50:28Z

libbeat/processors/add_kubernetes_metadata/indexers.go

+	}
+}
+
+func (p *PodUidIndexer) GetIndexes(pod *kubernetes.Pod) []string {


exported method PodUidIndexer.GetIndexes should have comment or be unexported

houndci-bot · 2018-05-10T20:50:28Z

libbeat/processors/add_kubernetes_metadata/indexers.go

+	return &PodUidIndexer{metaGen: metaGen}, nil
+}
+
+func (p *PodUidIndexer) GetMetadata(pod *kubernetes.Pod) []MetadataIndex {


exported method PodUidIndexer.GetMetadata should have comment or be unexported

houndci-bot · 2018-05-10T20:50:28Z

libbeat/processors/add_kubernetes_metadata/indexers.go

+	metaGen kubernetes.MetaGenerator
+}
+
+func NewPodUidIndexer(_ common.Config, metaGen kubernetes.MetaGenerator) (Indexer, error) {


exported function NewPodUidIndexer should have comment or be unexported
func NewPodUidIndexer should be NewPodUIDIndexer

houndci-bot · 2018-05-10T20:50:28Z

libbeat/processors/add_kubernetes_metadata/indexers.go

@@ -128,6 +129,28 @@ func (p *PodNameIndexer) GetIndexes(pod *kubernetes.Pod) []string {
 	return []string{fmt.Sprintf("%s/%s", pod.Metadata.Namespace, pod.Metadata.Name)}
 }

+type PodUidIndexer struct {


exported type PodUidIndexer should have comment or be unexported
type PodUidIndexer should be PodUIDIndexer

houndci-bot · 2018-05-10T20:50:29Z

libbeat/common/kubernetes/metadata.go

@@ -67,6 +70,21 @@ func (g *metaGenerator) PodMetadata(pod *Pod) common.MapStr {
 	return meta
 }

+func (g *metaGenerator) PodMetadataWithUid(pod *Pod) common.MapStr {


method PodMetadataWithUid should be PodMetadataWithUID

houndci-bot · 2018-05-10T20:50:29Z

filebeat/processor/add_kubernetes_metadata/matchers.go

+			// In case of the Kubernetes log path "/var/log/containers/",
+			// the container ID will be located right before the ".log" extension.
+			if strings.HasPrefix(f.LogsPath, "/var/log/containers/") && strings.HasSuffix(source, ".log") && sourceLen >= containerIdLen+4 {
+				containerIdEnd := sourceLen - 4


var containerIdEnd should be containerIDEnd

houndci-bot · 2018-05-10T20:50:29Z

filebeat/processor/add_kubernetes_metadata/matchers.go

+			if strings.HasPrefix(f.LogsPath, "/var/lib/kubelet/pods/") && strings.HasSuffix(source, ".log") {
+				pathDirs := strings.Split(source, "/")
+				if len(pathDirs) > podUidPos {
+					podUid := strings.Split(source, "/")[podUidPos]


var podUid should be podUID

houndci-bot · 2018-05-10T20:50:29Z

filebeat/processor/add_kubernetes_metadata/matchers.go

+// Pod UID is a 36-character-long string
+const podUidLen = 36
+// Pod UID is on the 5th index of the path directories
+const podUidPos = 5


const podUidPos should be podUIDPos

houndci-bot · 2018-05-10T20:50:29Z

filebeat/processor/add_kubernetes_metadata/matchers.go

 }

 // Docker container ID is a 64-character-long hexadecimal string
 const containerIdLen = 64
+// Pod UID is a 36-character-long string
+const podUidLen = 36


const podUidLen should be podUIDLen

mariomechoulam · 2018-05-11T00:01:03Z

I guess that the CI is failing because of the style and formatting. I have tried executing make fmt but I get an error. I am on Manjaro now, which ships with both Python 2 and 3, but the default is 3 =/
Have tried everything step by step to make sure that I have virtualenv with Python 2 and pip2 - and I have. Even manually source build/python-env/bin/activate-d before running the task. However every time I get:

make[1]: Entering directory '/home/mario/git/go/src/github.com/elastic/beats/libbeat'
Cache entry deserialization failed, entry ignored
  Cache entry deserialization failed, entry ignored
Command "python setup.py egg_info" failed with error code 1 in /tmp/pip-install-8n1zw1h_/functools32/
make[1]: *** [scripts/Makefile:232: python-env] Error 1
make[1]: Leaving directory '/home/mario/git/go/src/github.com/elastic/beats/libbeat'
make: *** [Makefile:92: fmt] Error 1

Probably those cache lines are a hint that it tries to run a different pip version. I am confident that executing sudo pip2 install --upgrade functools32 is working, not sure why it does not through make fmt.

Fetched goimports manually and after running it I am getting quite a lot of modifications, most in files I did not modify and several imports being removed altogether. Honestly, don't feel comfortable commiting that...
Can anyone shed some light and unblock me? I can send a patch with the goimports modifications.

exekias

Thank you for taking this!!

Overall this is looking good, what do you think about making pod.uid optional to the metadata generator (also add_kubernetes_metadata). You will need to add pod.id mapping to https://github.com/elastic/beats/blob/master/libbeat/processors/add_kubernetes_metadata/_meta/fields.yml

exekias · 2018-05-11T12:28:36Z

filebeat/processor/add_kubernetes_metadata/matchers.go

 }

 func newLogsPathMatcher(cfg common.Config) (add_kubernetes_metadata.Matcher, error) {
 	config := struct {
 		LogsPath string `config:"logs_path"`
+		ResourceType string `config:"resource_type"`


this config starts to get complicated, I guess we can live with this how it is now, but I will try to give it a thought on how to simplify this mechanism. Not a blocker for this PR :)

exekias · 2018-05-11T12:30:21Z

libbeat/common/kubernetes/metadata.go

@@ -67,6 +70,21 @@ func (g *metaGenerator) PodMetadata(pod *Pod) common.MapStr {
 	return meta
 }

+func (g *metaGenerator) PodMetadataWithUID(pod *Pod) common.MapStr {


I think this should be a setting instead, so pod.uid is added if requested (both by the user or your indexer

Hey!
Thanks for the feedback. So, just to make sure I am understand correctly, we are talking about adding the pod.uid from the configuration, anytime that

The pod_uid Indexer is used

pod.uid is specified as a field inside the fields element?

I was thinking more about doing something similar to the current include_annotations setting, for instance. Parameters are passed here: https://github.com/elastic/beats/blob/master/libbeat/common/kubernetes/metadata.go#L24

And the processor allows configuration for them here: https://github.com/elastic/beats/blob/master/libbeat/processors/add_kubernetes_metadata/config.go#L24

The indexer would pass the right parameter (enable pod.uid) without the user asking for it.

What do you think?

exekias · 2018-05-11T13:41:20Z

About the goimports issue, you can probably just use goimports -local github.com/elastic -l -w <file_to_review.go. Let me know if that works for you

mariomechoulam · 2018-05-15T11:06:55Z

Hi, just refactored the code as discussed and updated the unit-tests (also the autodiscover feature albeit a bit late, apologies).
Looks like the formatting is working now, thanks @exekias !
Just ran make docs and looks like it did stuff, but right after that I am not seeing any changes on the branch to be commited. Am I doing anything wrong?

exekias · 2018-05-15T11:09:14Z

You will need to:

rebase master
run make update in the root beats folder

…the Pod.UID as the key and inside the metadata. Extended Filebeat's LogPathMatcher with an additional configuration parameter (resource_type) which, when set to 'pod', uses a different logic to extract the MetadataIndex from the source field. Updated unit-tests accordingly.

… PodUIDIndexer being specified or by the "include_pod_uid" options in the configuration file. The result is used as a parameter for the metaGenerator which, in turn, adds the pod.uid to the existing metadata structure. Added and updated unit-tests accordingly.

…clude_pod_uid". Formatted code.

exekias · 2018-05-15T18:05:59Z

libbeat/processors/add_kubernetes_metadata/kubernetes.go

@@ -65,7 +66,10 @@ func newKubernetesAnnotator(cfg *common.Config) (processors.Processor, error) {
 		Indexing.RUnlock()
 	}

-	metaGen := kubernetes.NewMetaGenerator(config.IncludeAnnotations, config.IncludeLabels, config.ExcludeLabels)
+	//Checks whether the PodUIDIndexer has been added in order to turn on the pod uid gathering
+	_, hasPodUIDIndexer := Indexing.indexers["pod_uid"]


I think it's better if we don't do this automatically, the user must enable it. In any case, the indexer would still work without this, isn't it?

Hmmm the intention here was to enable the gathering of pod.uid if the indexer was specified in the config, even if the config bool include_pod_uid was not.
I thought this is what you meant with your previous comment: The indexer would pass the right parameter (enable pod.uid) without the user asking for it.
Or perhaps I am missing something...

EDIT:
Now that I check again, the Indexer would work even without any of the 2 being in the config as the key is not gotten from the metadata. Should I then remove it altogether?

Hi,

Yes, I apologize as that is what I said, then I realized the indexer doesn't need the pod uid in the metadata 😇

Still, I think having pod.uid is a good feature for some people, so I would keep it as something optional you can enable. I would only remove this auto-enable thingy and we are ready to go.

Awesome!
I'll remove the auto-enable and add the info in the changelog first thing tomorrow.
Thanks man! And no need to apologise 🙂

exekias

Thank you for the changes! I added a (hopefully) last thing :)

Please add a new entry to CHANGELOG.asciidoc

exekias · 2018-05-16T09:12:10Z

jenkins, test this please

yangyuw · 2018-05-16T09:14:53Z

@mariomechoulam Can I use this with emptyDir like this now?

processors:
  - add_kubernetes_metadata:
      in_cluster: true
      default_indexers.enabled: false
      default_matchers.enabled: false
      indexers:
        - pod_uid
      matchers:
        - logs_path:
            logs_path: /var/lib/kubelet/pods/
            resource_type: pod

exekias · 2018-05-16T09:45:08Z

Thank you @mariomechoulam for your contribution 🎉

It will go out in 6.4 release

mariomechoulam · 2018-05-16T09:47:40Z

Awesome! Many thanks @exekias for your support!
Do we know if it is working so I can update the initial post and the discuss topic? I was not able to test it beyond unit-test myself...

elasticmachine · 2018-05-16T09:57:02Z

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

mariomechoulam · 2018-05-16T10:03:29Z

Hi @yu-yang2
I have updated the original post with the configuration, I believe, would make it work (notice I added the include_pod_uid: true option). Nevertheless, I would wait myself until the confirmation that this is, indeed, the working setup.

exekias · 2018-05-16T10:29:35Z

I can confirm it works, both the indexer and adding pod.uid

yangyuw · 2018-05-16T15:34:02Z

@mariomechoulam You have missing a : after pod_uid beyond indexers, here is my config file, this is perfect work with emptyDir even in multi replicas deployment.

filebeat.prospectors:
- type: log
  paths:
    - /var/lib/kubelet/pods/*/volumes/kubernetes.io~empty-dir/*/*.log
processors:
  - add_kubernetes_metadata:
      in_cluster: true
      default_indexers.enabled: false
      default_matchers.enabled: false
      include_pod_uid: true
      indexers:
        - pod_uid:
      matchers:
        - logs_path:
            logs_path: /var/lib/kubelet/pods/
            resource_type: pod
output.elasticsearch:
  hosts: elasticsearch-logging:9200

mariomechoulam · 2018-05-16T20:45:29Z

Nice spot @yu-yang2 ! And many thanks for adding the correct configuration here; I'll update the original post.
Very happy to see that you found this helpful and are using it 🙂
Cheers!

Added PodUidIndexer to add_kubernetes_metadata processor, containing the Pod.UID as the key and inside the metadata. Extended Filebeat's LogPathMatcher with an additional configuration parameter (resource_type) which, when set to 'pod', uses a different logic to extract the MetadataIndex from the source field.

yubobo · 2018-08-18T09:19:53Z

@exekias
When is the 6.4 release? It's been over 100 days
thanks~

yubobo · 2018-09-28T10:22:25Z

unable to get kubernetes metadata to enrich log using filebeat 6.4.1. #8481
would you share deployment config with me？ @mariomechoulam @yu-yang2
thanks！！

houndci-bot reviewed May 10, 2018

View reviewed changes

jsoriano added in progress Pull request is currently in progress. review containers Related to containers use case Filebeat Filebeat libbeat labels May 11, 2018

exekias reviewed May 11, 2018

View reviewed changes

mariomechoulam added 5 commits May 15, 2018 14:24

Fixed reported styling issues.

f8e5051

Fixed autodiscover MetaGenerator to be compatible with the config "in…

f82b087

…clude_pod_uid". Formatted code.

Updated documentation after rebase.

1a0e10e

mariomechoulam force-pushed the improvement/add_kubernetes_metadata_supports_poduid branch from aa87349 to 1a0e10e Compare May 15, 2018 12:49

exekias reviewed May 15, 2018

View reviewed changes

Removed auto-enabling of pod uid metadata. Updated CHANGELOG document.

c4a19f5

exekias approved these changes May 16, 2018

View reviewed changes

exekias removed the in progress Pull request is currently in progress. label May 16, 2018

exekias merged commit 232ee20 into elastic:master May 16, 2018

exekias mentioned this pull request May 16, 2018

Add missing Pod UID registration #7118

Closed

exekias mentioned this pull request May 21, 2018

Add pod-uid support for add_kubernetes_metadata matchers #7029

Closed

ycombinator mentioned this pull request Jul 17, 2024

[k8s provider] Look up metadata based on file path elastic/elastic-agent#5164

Open

Add pod-uid support for add_kubernetes_metadata #7072

Add pod-uid support for add_kubernetes_metadata #7072

Conversation

mariomechoulam commented May 10, 2018 • edited by exekias Loading

elasticmachine commented May 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mariomechoulam commented May 11, 2018 • edited Loading

exekias left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

exekias commented May 11, 2018

mariomechoulam commented May 15, 2018

exekias commented May 15, 2018

Choose a reason for hiding this comment

mariomechoulam May 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

exekias left a comment

Choose a reason for hiding this comment

exekias commented May 16, 2018

yangyuw commented May 16, 2018

exekias commented May 16, 2018

mariomechoulam commented May 16, 2018

elasticmachine commented May 16, 2018

mariomechoulam commented May 16, 2018

exekias commented May 16, 2018

yangyuw commented May 16, 2018 • edited Loading

mariomechoulam commented May 16, 2018

yubobo commented Aug 18, 2018 • edited Loading

yubobo commented Sep 28, 2018 • edited Loading

mariomechoulam commented May 10, 2018 •

edited by exekias

Loading

mariomechoulam commented May 11, 2018 •

edited

Loading

exekias left a comment •

edited

Loading

mariomechoulam May 15, 2018 •

edited

Loading

yangyuw commented May 16, 2018 •

edited

Loading

yubobo commented Aug 18, 2018 •

edited

Loading

yubobo commented Sep 28, 2018 •

edited

Loading