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

kubernetes watcher refactor #6159

Merged
merged 3 commits into from
Jan 26, 2018
Merged

Conversation

walktall
Copy link
Contributor

This PR comes from #6117

It does following items:

  1. Update k8s vendor to get an abstract api for watching and listing.

  2. Refactor watcher. The style is from client-go informer so that we can port to client-go without much effort in future. Also, it removes side effect in watcher, such as delay delete event and cache the resources, these side-effect should be implemented by caller who has that kind of requirement I think.

  3. Although as mentioned by @exekias , bug fix for querying pod should be put in another pr, but I found it is hard to do that since moving cache out from watcher is part of refactor...

  4. Event metricset in metricbeat now is using new refactored watcher.

@exekias PTAL~

@elasticmachine
Copy link
Collaborator

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?

@@ -0,0 +1,62 @@
package add_kubernetes_metadata

Choose a reason for hiding this comment

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

don't use an underscore in package name

@exekias exekias self-requested a review January 24, 2018 11:11
@walktall walktall force-pushed the watcher-refactor branch 3 times, most recently from 32dabfc to 87f19c2 Compare January 25, 2018 08:14
Copy link
Contributor

@exekias exekias left a comment

Choose a reason for hiding this comment

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

Thank you for the PR! I gave it a try and seems good :), the code is looking good too, v1 of k8s brought some sugar!, let's try to avoid that reflect, I left a comment for it

p.pods[pod.Metadata.UID] = pod
// ugly reflect since ResourceList does not exposed Items
// can be avoid by using k8s.io/client-go
list := reflect.ValueOf(w.resourceList).Elem().FieldByName("Items")
Copy link
Contributor

Choose a reason for hiding this comment

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

You can avoid this if you create a new interface for resource list, and write adapters to get the Items from PodList and EventList, what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea! I will make a try. Thanks!

@exekias
Copy link
Contributor

exekias commented Jan 25, 2018

ok to test

some changes on code to adjust the v1.0.0
  * be able to watch different resources
  * does not support cache now

Fix bug: query pod meta won't refresh deleted map timeout

metricbeat: using refactored watcher
@ruflin
Copy link
Contributor

ruflin commented Jan 26, 2018

@walktall This will need a rebase.

@walktall
Copy link
Contributor Author

@ruflin rebased

@exekias made some changes to avoid reflect, PTAL.

Copy link
Contributor

@exekias exekias left a comment

Choose a reason for hiding this comment

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

LGTM

@exekias exekias merged commit 0194427 into elastic:master Jan 26, 2018
@walktall walktall deleted the watcher-refactor branch February 4, 2018 07:57
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.

5 participants