Expose network stats for pods #852

jimmidyson · 2016-01-04T23:51:59Z

Fixes #368

Dirty: using leaky infra container name, regex for container name to pod mapping, but seems to work OK...

/cc @vishh @akash010 @smarterclayton @simon3z

k8s-bot · 2016-01-04T23:53:12Z

Jenkins GCE e2e

Build/test failed for commit b5c136e.

Build Log

k8s-bot · 2016-01-05T00:23:24Z

Jenkins GCE e2e

Build/test failed for commit 5552fce.

Build Log

k8s-bot · 2016-01-05T00:38:35Z

Jenkins GCE e2e

Build/test failed for commit f1bc44b.

Build Log

smarterclayton · 2016-01-05T03:07:07Z

If leaky is imported just to get pod infra container name, it's not worth it. I'd just have it be a constant or parameter in this code.

jimmidyson · 2016-01-05T07:20:54Z

retest this please

jimmidyson · 2016-01-05T08:47:36Z

Removed leaky package dependency.

k8s-bot · 2016-01-05T09:05:30Z

Jenkins GCE e2e

Build/test failed for commit cf6f29b.

Build Log

jimmidyson · 2016-01-05T09:11:28Z

Got more work to do on this to expose via the API so changing to WIP - please don't merge.

@vishh @mwielgus Do you know if the Jenkins GCE e2e should pass? Seeing unrelated flake in logs:

No error is expected but got expected [gcm] sinks, found []

mwielgus · 2016-01-05T09:20:18Z

Yeah, the problem seems to be unrelated. BTW, we will have to redo this work in heapster-scalability branch.

jimmidyson · 2016-01-05T09:24:55Z

@mwielgus Are you targeting the new metrics APIs in the scalability branch? Sorry I've not kept up to speed with it.

mwielgus · 2016-01-05T09:34:45Z

If the new API is delivered on time then yes we will switch, if not we will keep using the old one.

k8s-bot · 2016-01-05T14:28:30Z

Jenkins GCE e2e

Build/test failed for commit 42d3c59.

Build Log

jimmidyson · 2016-01-05T14:28:52Z

Ready for review please. Jenkins GCE e2e is flaky & needs to either disabled or fixed separately to this PR.

vishh · 2016-01-05T14:31:29Z

api/v1/types/model_types.go

 // A model entity can be a Pod, a Container, a Namespace or a Node.
 type ExternalEntityListEntry struct {
 	Name     string `json:"name"`
 	CPUUsage uint64 `json:"cpuUsage"`
 	MemUsage uint64 `json:"memUsage"`
+	RxBytes  uint64 `json:"rx_bytes"`


Do we need network metrics in the model or is it necessary only for monitoring purposes?
The cost for adding metrics to the model is kindda high for now.

So are you saying it shouldn't be returned by API queries, only passed to sinks? I'd prefer it to be exposed via the REST API & that means using the model, doesn't it?

Yeah. Is it required to be exposed via APIs as well?

Point me to the requirements :) Don't we make it up as we go along? ;)

Honestly, if it's too expensive to store a couple of extra values per pod then I don't mind dropping it from there & just leave it as passed to sinks.

I'd personally prefer exposing this and filesystem stats as well. Its just that the default resource limits in Kube has to change once these metrics are added. So as long as we can go fix the limits, then adding these metrics is OK by me :)

OK how about I remove from the model for this version & revisit it for @mwielgus' rewritten version?

SGTM. So is monitoring the primary use case?

Monitoring & accounting, yes.

Can't think of a use for autoscaling on network traffic right now.

k8s-bot · 2016-01-05T15:17:31Z

Jenkins GCE e2e

Build/test failed for commit 44cd513.

Build Log

k8s-bot · 2016-01-05T15:44:34Z

Jenkins GCE e2e

Build/test failed for commit b445c4f.

Build Log

vishh · 2016-01-05T15:58:33Z

manager/manager.go

+				pod := &sd.data.Pods[podIndex]
+				// If we find a matching pod then add the container to the pod's containers slice.
+				if pod.Name == podName && pod.Namespace == podNamespace {
+					cont.Hostname = pod.Hostname


Ideally, the pod infra container should be hidden inside heapster. That way if we collect metrics from rocket for example, which does not need an infra container, we will not break users of heapster.

Suggestions on how to do that? Right now I can't think of one tbh.

We need stats at the pod level in addition to container level.

Bit confused by this. We're adding the infra container to the appropriate pod so this is done.

vishh · 2016-01-05T16:01:14Z

General structure LGTM. Thanks @jimmidyson !!

Fixes kubernetes-retired#368

k8s-bot · 2016-01-05T18:16:22Z

Jenkins GCE e2e

Build/test failed for commit e412d1a.

Build Log

jimmidyson · 2016-01-05T18:41:51Z

Network stats are now only sent to sinks, not retrievable via Heapster API as discussed with @vishh.

jimmidyson · 2016-01-12T14:02:37Z

@vishh Please can you let me know what is outstanding for this PR to be merged?

vishh · 2016-01-12T14:58:00Z

The only issue with this PR is that of exposing the infra container. I'd prefer adding pod level metrics and exposing network as a pod level metrics.

jimmidyson · 2016-01-12T16:00:20Z

This is how's it's been done in heapster-scalability branch & I'm not going to duplicate that functionality as this is just a quick fix until heapster-scalability branch becomes master.

vishh · 2016-01-12T21:08:18Z

Ok then. No issues in that case.

On Tue, Jan 12, 2016 at 8:00 AM, Jimmi Dyson notifications@github.com
wrote:

This is how's it's been done in heapster-scalability branch & I'm not
going to duplicate that functionality as this is just a quick fix until
heapster-scalability branch becomes master.

—
Reply to this email directly or view it on GitHub
#852 (comment).

jimmidyson · 2016-01-12T21:11:12Z

Thanks @vishh.

Merging.

Expose network stats for pods

googlebot added the cla: yes label Jan 4, 2016

jimmidyson force-pushed the network-stats branch from 5552fce to f1bc44b Compare January 5, 2016 00:01

jimmidyson force-pushed the network-stats branch from f1bc44b to cf6f29b Compare January 5, 2016 08:45

jimmidyson changed the title ~~Expose network stats for pods.~~ [WIP] Expose network stats for pods. Jan 5, 2016

jimmidyson force-pushed the network-stats branch from cf6f29b to 42d3c59 Compare January 5, 2016 14:26

jimmidyson changed the title ~~[WIP] Expose network stats for pods.~~ Expose network stats for pods Jan 5, 2016

vishh reviewed Jan 5, 2016
View reviewed changes

jimmidyson force-pushed the network-stats branch 2 times, most recently from 44cd513 to b445c4f Compare January 5, 2016 14:58

vishh reviewed Jan 5, 2016
View reviewed changes

Expose network stats for pods.

e412d1a

Fixes kubernetes-retired#368

jimmidyson force-pushed the network-stats branch from b445c4f to e412d1a Compare January 5, 2016 18:02

jimmidyson added a commit that referenced this pull request Jan 12, 2016

Merge pull request #852 from jimmidyson/network-stats

78ff89c

Expose network stats for pods

jimmidyson merged commit 78ff89c into kubernetes-retired:master Jan 12, 2016

ghost mentioned this pull request Jan 22, 2016

cpu usage metrics openshift/origin-metrics#27

Closed

Expose network stats for pods #852

Expose network stats for pods #852

Conversation

jimmidyson commented Jan 4, 2016

k8s-bot commented Jan 4, 2016

k8s-bot commented Jan 5, 2016

k8s-bot commented Jan 5, 2016

smarterclayton commented Jan 5, 2016

jimmidyson commented Jan 5, 2016

jimmidyson commented Jan 5, 2016

k8s-bot commented Jan 5, 2016

jimmidyson commented Jan 5, 2016

mwielgus commented Jan 5, 2016

jimmidyson commented Jan 5, 2016

mwielgus commented Jan 5, 2016

k8s-bot commented Jan 5, 2016

jimmidyson commented Jan 5, 2016

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

k8s-bot commented Jan 5, 2016

k8s-bot commented Jan 5, 2016

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

vishh commented Jan 5, 2016

k8s-bot commented Jan 5, 2016

jimmidyson commented Jan 5, 2016

jimmidyson commented Jan 12, 2016

vishh commented Jan 12, 2016

jimmidyson commented Jan 12, 2016

vishh commented Jan 12, 2016

jimmidyson commented Jan 12, 2016