Initial k8s util implementation by zippolyte · Pull Request #285 · DataDog/datadog-agent

zippolyte · 2017-06-08T18:39:59Z

What does this PR do?

Initial k8s util implementation

hkaj · 2017-06-09T10:19:24Z

Nice! Could you try using https://github.com/ericchiang/k8s instead? It's way slimmer and should be more than enough for our use case. The interface is very similar so you shouldn't have to change many things.

masci · 2017-06-09T11:09:10Z

@zippolyte We're currently using the bloated k8s client in the metadata package, If the lightweight client works as expected, could you open another PR and use the new client for metadata too? (see https://github.com/DataDog/datadog-agent/blob/master/pkg/metadata/kubernetes/kubernetes.go)

Based on feedback in #285 we can use a smaller library that pulls in significantly fewer dependencies. The only trade-off is that we need to use getters in function calls to avoid pointers everywhere because this is using older style protobuf generated code.

hkaj · 2017-06-12T11:57:02Z

This method queries the API server for the list of pods (see here). There may be a use case for it, but I think we should avoid doing this by default, otherwise we'll put some heavy load on the apiserver.

Maybe rename GetPodList to GetGlobalPodList, and add to the comment that this hits the apiserver so we should use it with caution?

For GetNodeInfo we likely don't need to use this method. We can connect to the local kubelet directly (by making sure users give the agent access to it in the config - we also have some dark magic here in case they don't, ping me if you need more details) and request /pods/ (maybe create a GetLocalPodList that does that)?. This gives you a list of locally running pods.

Once you have that list, you can pick whichever pod you want and extract spec.nodeName and spec.hostIP (just make sure the pod doesn't have spec.hostNetwork set to true or you won't find an IP address). So this gives you the same result but by only querying the local kubelet instead of the global store.

masci · 2017-06-12T18:26:46Z

we merged #291 you might want to rebase there to resolve conflicts on glide yaml and lock file

hkaj

that looks good to me, although I would like to test it. Let's build a test package where it's used

hkaj · 2017-06-15T09:03:33Z

+}
+
+// PerformKubeletQuery performs a GET query against kubelet and return the response body
+// Supports auth


token-based auth. TLS is a TODO

masci

A couple of things to fix, then let's think about how could we test this (integration or system)

masci · 2017-06-15T12:52:52Z

+
+// Try and find the hostname to query the kubelet
+func locateKubelet(kubeletHost string, kubeletPort int) (string, error) {
+	host := os.Getenv("KUBERNETES_KUBELET_HOST")


Let's move this in a config option, we can always set it from the outside like DD_ KUBERNETES_KUBELET_HOST, this way seems more consistent to me

masci · 2017-06-15T12:58:15Z

+	host := os.Getenv("KUBERNETES_KUBELET_HOST")
+	var err error
+	if host == "" {
+		host = kubeletHost


Not sure an env var should take precedence over a function param.
Question: why NewKubeUtil takes the host as a param? If we want the caller to be able to set it, we should remove any local override. On the other end, if we want the caller to be agnostic of the host, let's remove the param from NewKubeUtil and access KUBERNETES_KUBELET_HOST from the settings (see previous comment) before passing it to locateKubelet

I was trying to replicate the order of precedence that we had in the kube_util of agent 5, where we would pass an instance to this function that could have kubelet_host and kubelet_port set in it, and the env var would take precedence.
My idea was that the check using this would pass whatever was in its config to the function, and let the utility do its thing.
But yeah I agree this isn't really intuitive, and a config option seems better

masci · 2017-06-15T13:00:09Z

+		}
+	}
+
+	port := kubeletPort


I'd move this to a setting as well

masci · 2017-06-15T13:00:53Z

+
+// PerformKubeletQuery performs a GET query against kubelet and return the response body
+// Supports auth
+func PerformKubeletQuery(url string) ([]byte, error) {


Not sure the module should export this func

We'll need to run Kubelet queries from other places as well, so either we export this method and build the URL where it's needed, or we don't export it and all the queries we need to run will be triggered by new functions we'll add here as needed. Honestly not sure what's the best solution here.

Maybe we should do both. I think concentrating wrapper functions here is part of the goal of the module itself but keeping this function exported might be a feature more than an issue. Let's keep it like this, ignore my comment.

masci

One last comment and we're there!

masci · 2017-06-16T11:16:10Z

+// Kubelet constants
+const (
+	AuthTokenPath           = "/var/run/secrets/kubernetes.io/serviceaccount/token"
+	DefaultHTTPKubeletPort  = 10255


could we set default port and host settings in init like we do for example here?

datadog-agent/pkg/metadata/scheduler.go

Line 17 in d9e459b

config.Datadog.SetDefault("metadata_interval", 4*time.Hour)

…utils_k8s

conorbranagan · 2017-06-19T14:20:49Z

Curious if we want consistency on kubernetes vs k8s? Right now the meta provider is using kubernetes while this uses k8s. I imagine it's ok to use interchangeably with commenting and such but it might be nice to have consistent pkg names.

masci · 2017-06-19T14:44:20Z

I think we should consolidate package and settings names to kubernetes, leaving k8s to informal communications, code comments and stuff like that

masci · 2017-07-09T09:28:51Z

We need integration tests for kubernetes, useful both for the util package and the k8s metadata collector. Let's merge this and do that in another PR.

Bumps [golang.org/x/oauth2](https://github.com/golang/oauth2) from 0.33.0 to 0.34.0. - [Commits](golang/oauth2@v0.33.0...v0.34.0) --- updated-dependencies: - dependency-name: golang.org/x/oauth2 dependency-version: 0.34.0 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

zippolyte mentioned this pull request Jun 8, 2017

[util][k8s] Get hostname from kubernetes #283

Closed

conorbranagan mentioned this pull request Jun 10, 2017

[metadata] Use slimmer k8s library for metadata. #291

Merged

hkaj requested changes Jun 12, 2017

View reviewed changes

zippolyte added 3 commits June 12, 2017 18:00

Initial k8s util implementation

1cdd13a

Use ericchian/k8s instead of k8s/client-go

40c50b0

Do not make calls to the apiserver

4b689e8

zippolyte force-pushed the hippo/utils_k8s branch from 97e2125 to 4b689e8 Compare June 13, 2017 23:25

Linting

81220a1

zippolyte force-pushed the hippo/utils_k8s branch from 8fb2cb5 to 9b01943 Compare June 13, 2017 23:41

More linting

95045ca

zippolyte force-pushed the hippo/utils_k8s branch from 9b01943 to 95045ca Compare June 13, 2017 23:49

hkaj reviewed Jun 15, 2017

View reviewed changes

masci suggested changes Jun 15, 2017

View reviewed changes

zippolyte added 3 commits June 15, 2017 16:30

Get host and port from config

e75ec50

Forgot one kubeletPort

019f544

Remove os import

5397c8a

masci suggested changes Jun 16, 2017

View reviewed changes

zippolyte added 5 commits June 16, 2017 15:00

Add error handling for unmarshalling

8654206

Allocate var where to unmarshal

a682289

Merge branch 'master' of github.com:DataDog/datadog-agent into hippo/…

d56e1a5

…utils_k8s

Add default config values

869e50f

remove unused const

675a763

Rename package & use local struct for unmarshalling

94e9c75

conorbranagan mentioned this pull request Jul 7, 2017

kubernetes: Collect local pod and container state from kubelet DataDog/datadog-process-agent#10

Merged

masci approved these changes Jul 9, 2017

View reviewed changes

zippolyte merged commit c093cf5 into master Jul 12, 2017

zippolyte deleted the hippo/utils_k8s branch July 12, 2017 13:12

Conversation

zippolyte commented Jun 8, 2017

What does this PR do?

Uh oh!

hkaj commented Jun 9, 2017

Uh oh!

masci commented Jun 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

masci commented Jun 12, 2017

Uh oh!

hkaj left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

masci left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

masci left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

conorbranagan commented Jun 19, 2017

Uh oh!

masci commented Jun 19, 2017

Uh oh!

masci commented Jul 9, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

masci commented Jun 9, 2017 •

edited

Loading