Skip to content

Update kubernetes watcher to use official client-go libraries#13051

Merged
exekias merged 8 commits intoelastic:masterfrom
vjsamuel:client_go
Jul 31, 2019
Merged

Update kubernetes watcher to use official client-go libraries#13051
exekias merged 8 commits intoelastic:masterfrom
vjsamuel:client_go

Conversation

@vjsamuel
Copy link
Copy Markdown
Contributor

@vjsamuel vjsamuel commented Jul 24, 2019

This PR moves away from using the Eric Chang client-go for Kubernetes interaction in favor of the official kube client. As a result, we now use SharedInformers. This makes sure that at any given time there is only 1 connection to the API server per resource that is requested. Moving to the client-go also makes sure that we no longer need to check ResourceVersion while watching. It is implicitly done under the hood.

This change now allows the watcher to be testable and as a follow up we could add test cases.

@vjsamuel vjsamuel requested review from a team as code owners July 24, 2019 07:56
@elasticmachine
Copy link
Copy Markdown
Contributor

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?

1 similar comment
@elasticmachine
Copy link
Copy Markdown
Contributor

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?

@vjsamuel vjsamuel force-pushed the client_go branch 4 times, most recently from 3b2c86e to 878b3a7 Compare July 24, 2019 08:34
@exekias exekias added Team:Integrations Label for the Integrations team containers Related to containers use case review enhancement labels Jul 24, 2019
@exekias
Copy link
Copy Markdown
Contributor

exekias commented Jul 24, 2019

ok to test

@vjsamuel vjsamuel force-pushed the client_go branch 2 times, most recently from 6689e91 to 9b74f99 Compare July 24, 2019 16:06
@exekias exekias requested review from exekias and odacremolbap July 25, 2019 09:50
Comment thread libbeat/common/kubernetes/util.go Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

it sems you don't use inCluster anymore, how would this be used now? We would need to update the comment and some docs

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

the official kube client deduces if it runs in cluster or not and hence to get a kube client we dont really need it.

if kubeconfigPath == "" && masterUrl == "" {
		klog.Warningf("Neither --kubeconfig nor --master was specified.  Using the inClusterConfig.  This might not work.")
		kubeconfig, err := restclient.InClusterConfig()
		if err == nil {
			return kubeconfig, nil
		}
		klog.Warning("error creating inClusterConfig, falling back to default config: ", err)
	}

if kubeconfig path is provided then it just tries to create an out of cluster configuration.

That said. let me fix the discover node to just follow the same thing.

Comment thread libbeat/common/kubernetes/watcher.go Outdated
Copy link
Copy Markdown
Contributor

@exekias exekias Jul 26, 2019

Choose a reason for hiding this comment

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

what is this line for? a comment could help here

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

this just makes sure that it gets the namespace/name combination only for resources where state is not Unknown

Copy link
Copy Markdown
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 opening this @vjsamuel, this is REALLY appreciated! 🎉

I left a few comments and questions, I'm wondering about the changes in user configurations and backwards compatibility

@vjsamuel vjsamuel changed the title Update kubernetes watcher to use official client-go libraries [WIP] Update kubernetes watcher to use official client-go libraries Jul 27, 2019
@vjsamuel
Copy link
Copy Markdown
Contributor Author

not sure if the failures are related. I have done manual testing on all flows and things look file. Lets have action items to now have fake client based test cases on the autodiscover/processor paths so that it is easier to find failures. there were a few issues that i fixed in the last commit one of which is to increase the default resync period. with the new client, it is no longer necessary to sync every minute. the resync period forces all items on the cache to be replayed. this is undesirable at a 1 minute interval. i have also gone ahead and removed the in_cluster config parameter as it is deduced automatically. the PR is now in mergeable state now.

@vjsamuel vjsamuel changed the title [WIP] Update kubernetes watcher to use official client-go libraries Update kubernetes watcher to use official client-go libraries Jul 28, 2019
@exekias
Copy link
Copy Markdown
Contributor

exekias commented Jul 31, 2019

Thank you so much for this contribution @vjsamuel! it's a real enabler for future features

@exekias exekias merged commit e040d8d into elastic:master Jul 31, 2019
@vjsamuel vjsamuel deleted the client_go branch July 31, 2019 22:43
jmlrt added a commit to jmlrt/helm-charts that referenced this pull request Apr 8, 2020
mgreau pushed a commit to elastic/helm-charts that referenced this pull request Apr 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

containers Related to containers use case enhancement review Team:Integrations Label for the Integrations team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants