[Discuss&POC] refactor k8s client to k8s.io/client-go #6117

walktall · 2018-01-19T20:11:04Z

I didn't update vendor and fully test it yet, just open this PR for discuss.

Will be very happy if you can take a look! @exekias @vjsamuel @ruflin

Maybe it's still too big for bin to change client, but I abstract the common/kubernetes pkg and make it easier to use hopefully.

And also, found 2 bugs while refactoring.

query pod meta won't refresh deleted map timeout
delete event for k8s autodiscovery will be delayed since watcher delay the event push

With one another enhancement: beat can auto discovery correct k8s node now. So host in config might only be useful when you are testing...

Now beat can detect host by itself when outside cluster It also fixes 2 bugs: * query pod meta won't refrese deleted map timeout * delete event for k8s autodiscovery will be delayed since watcher delay the event push

houndci-bot · 2018-01-19T20:11:13Z

libbeat/processors/add_kubernetes_metadata/cache.go

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


don't use an underscore in package name

houndci-bot · 2018-01-19T20:11:13Z

libbeat/common/kubernetes/types.go


-	return po
+func (e *Event) GetMetadata() *ObjectMeta {


exported method Event.GetMetadata should have comment or be unexported

houndci-bot · 2018-01-19T20:11:14Z

libbeat/common/kubernetes/types.go

-		logp.Warn("Unable to marshal %v", pod.String())
-		return nil
-	}
+type Event struct {


exported type Event should have comment or be unexported

houndci-bot · 2018-01-19T20:11:14Z

libbeat/common/kubernetes/types.go

@@ -108,6 +108,10 @@ type Pod struct {
 	Status     PodStatus  `json:"status"`
 }

+func (p *Pod) GetMetadata() *ObjectMeta {


exported method Pod.GetMetadata should have comment or be unexported

houndci-bot · 2018-01-19T20:11:14Z

libbeat/common/kubernetes/types.go

 )

+type Resource interface {


exported type Resource should have comment or be unexported

elasticmachine · 2018-01-19T20:30:09Z

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?

ruflin · 2018-01-21T21:58:32Z

@walktall Thanks for taking a stab at this. I haven't looked much at the code yet but I saw you mentioned that you discovered / solved two issue. I'm wondering if these could go into a separate PR to have them in rather soon or these depend on changing the library?

walktall · 2018-01-22T01:34:12Z

@ruflin Yes, I will make another PR about two issues only.

exekias

Overall I like very much how this is looking, thank you for taking this!! 🎉

These changes are tackling different concerns, so I think they should go on different PRs, at least 4:

[bugfix] query pod meta won't refresh deleted map timeout
Switch to client-go (is the size still +30MB?)
Watcher API refactor
Node discovery improvements

About: "delete event for k8s autodiscovery will be delayed since watcher delay the event push", this is not a bug but a feature :)

Especially when handling logs, but also with some metrics, you want to delay the shutdown of the module to ensure you get the latest events from the pod. If we stop reading the log just after the pod dies, last lines won't be read.

exekias · 2018-01-22T10:32:16Z

libbeat/common/kubernetes/util.go

+func inClusterNamespace() string {
+	// This way assumes you've set the POD_NAMESPACE environment variable using the downward API.
+	// This check has to be done first for backwards compatibility with the way InClusterConfig was originally set up
+	if ns := os.Getenv("POD_NAMESPACE"); ns != "" {


We stopped using this, in favor of client.Namespace, what's the motivation for this change?

Since k8s.io/client-go doesn't have a field Namespace in clientset, so client.Namespace is kind like a tricky in ericchiang/k8s I think. This func is borrowed from client-go, and after rethink about it, I think user should not need to set namespace by downward API since if beat is in cluster, then it should be auto detected.

exekias · 2018-01-22T10:33:59Z

libbeat/common/kubernetes/util.go

+		return defaultNode
+	}
+	for _, n := range nodes.Items {
+		if n.Status.NodeInfo.MachineID == mid {


Cool addition, I guess all the fallback mechanisms make node discovery bulletproof :)

exekias · 2018-01-22T10:37:22Z

libbeat/common/kubernetes/watcher.go

-
-	// ListenStop returns a bus listener to receive pod stopped events, with a `pod` key holding it
-	ListenStop() bus.Listener
+	AddEventHandler(ResourceEventHandler)


💯 to this approach, I've been wanting to change it to this since I wrote the current one 😅

walktall · 2018-01-23T01:45:58Z

@exekias Thank you for your comments! I will make PRs step by step. And sorry for misunderstanding the feature in auto discovery as a bug 😅

walktall · 2018-01-23T01:51:53Z

@exekias For switching client-go:

With go 1.9.2, build with go build -ldflags="-s -w", it grows 12mb, from 53mb to 65mb.

With go 1.9.2, build with go build, it grows 19mb. From 104mb to 123mb.

Anyway, I will put the switching pr the last one, since it is only imported by common/kubernetes.

ruflin · 2018-02-27T06:50:25Z

@walktall Did you ever investigate if we could sligthly tweak the import to massively reduce the dependency. I had a similar experience with docker.

vjsamuel · 2018-02-27T06:55:07Z

i think that might be a good exercise to see if it works but the Kube client as is does a lot of things in addition to just making API calls.

walktall · 2018-02-27T06:59:07Z

@ruflin I tried it before but I can not find it out...

@vjsamuel If we want to use k8s informer, then it is not just making API calls I think.

walktall · 2018-03-16T02:40:48Z

Close due to no further plan for this right now since library currently used works well.

walktall added 2 commits January 20, 2018 03:58

Using k8s.io/client-go instead of github.com/ericchiang/k8s

6433449

Now beat can detect host by itself when outside cluster It also fixes 2 bugs: * query pod meta won't refrese deleted map timeout * delete event for k8s autodiscovery will be delayed since watcher delay the event push

Metricbeat k8s event module refactor to use libbeat

fd8c825

houndci-bot reviewed Jan 19, 2018

View reviewed changes

ruflin added in progress Pull request is currently in progress. discuss Issue needs further discussion. libbeat labels Jan 21, 2018

ruflin requested a review from exekias January 21, 2018 21:58

exekias reviewed Jan 22, 2018

View reviewed changes

This was referenced Jan 23, 2018

libbeat: Add a new way to detect node for k8s which using machine-id #6146

Merged

kubernetes watcher refactor #6159

Merged

This was referenced Feb 27, 2018

[Discuss] port k8s client to kubernetes/client-go #6068

Closed

Avoid infinite loop in Kubernetes watcher #6353

Merged

walktall closed this Mar 16, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Discuss&POC] refactor k8s client to k8s.io/client-go #6117

[Discuss&POC] refactor k8s client to k8s.io/client-go #6117

walktall commented Jan 19, 2018 •

edited

Loading

houndci-bot Jan 19, 2018

houndci-bot Jan 19, 2018

houndci-bot Jan 19, 2018

houndci-bot Jan 19, 2018

houndci-bot Jan 19, 2018

elasticmachine commented Jan 19, 2018

ruflin commented Jan 21, 2018

walktall commented Jan 22, 2018

exekias left a comment

exekias Jan 22, 2018

walktall Jan 23, 2018 •

edited

Loading

exekias Jan 22, 2018

exekias Jan 22, 2018 •

edited

Loading

walktall commented Jan 23, 2018 •

edited

Loading

walktall commented Jan 23, 2018

ruflin commented Feb 27, 2018

vjsamuel commented Feb 27, 2018

walktall commented Feb 27, 2018

walktall commented Mar 16, 2018

[Discuss&POC] refactor k8s client to k8s.io/client-go #6117

[Discuss&POC] refactor k8s client to k8s.io/client-go #6117

Conversation

walktall commented Jan 19, 2018 • edited Loading

houndci-bot Jan 19, 2018

Choose a reason for hiding this comment

houndci-bot Jan 19, 2018

Choose a reason for hiding this comment

houndci-bot Jan 19, 2018

Choose a reason for hiding this comment

houndci-bot Jan 19, 2018

Choose a reason for hiding this comment

houndci-bot Jan 19, 2018

Choose a reason for hiding this comment

elasticmachine commented Jan 19, 2018

ruflin commented Jan 21, 2018

walktall commented Jan 22, 2018

exekias left a comment

Choose a reason for hiding this comment

exekias Jan 22, 2018

Choose a reason for hiding this comment

walktall Jan 23, 2018 • edited Loading

Choose a reason for hiding this comment

exekias Jan 22, 2018

Choose a reason for hiding this comment

exekias Jan 22, 2018 • edited Loading

Choose a reason for hiding this comment

walktall commented Jan 23, 2018 • edited Loading

walktall commented Jan 23, 2018

ruflin commented Feb 27, 2018

vjsamuel commented Feb 27, 2018

walktall commented Feb 27, 2018

walktall commented Mar 16, 2018

walktall commented Jan 19, 2018 •

edited

Loading

walktall Jan 23, 2018 •

edited

Loading

exekias Jan 22, 2018 •

edited

Loading

walktall commented Jan 23, 2018 •

edited

Loading