fix network value for stats summary for multiple network interfaces #52144

andyxning · 2017-09-08T04:14:15Z

The original reason is when there are more than one none lo, docker0, veth network interfaces instead of just one eth0, the network interface value is only partial and does not correct. For now, summary stats api only gets the eth0 network interface values.

The original issues about this can be find in Heapster #1058 and Cadvisor #1593.

Fix stats summary network value when multiple network interfaces are available.

/cc @DirectXMan12 @piosz @xiangpengzhao @vishh @timstclair

andyxning · 2017-09-08T04:20:47Z

/assign @dchen1107

DirectXMan12 · 2017-09-08T19:17:26Z

the actual code seems reasonable, but this probably needs a big bold release note if/when it gets merged, since it changes the meaning of the network stats (they might suddenly be much larger).

andyxning · 2017-09-08T23:40:11Z

this probably needs a big bold release note if/when it gets merged

IIUC, @DirectXMan12 you mean that we need to make the release note bold? Maybe we can surround release note by an internal ** which can not be parsed under "```" but will be correctly parsed when added to a change log markdown file.

PTAL.

andyxning · 2017-09-09T03:20:02Z

/test pull-kubernetes-e2e-gce-bazel

DirectXMan12 · 2017-09-12T18:07:12Z

IIUC, @DirectXMan12 you mean that we need to make the release note bold?

I meant that somewhat figuratively ;-).

DirectXMan12 · 2017-09-12T18:08:04Z

this has approval from me, I think, but I'd like SIG Node to chime in for the actual approval.

andyxning · 2017-09-12T18:46:25Z

/ping @vishh @timstclair @yujuhong

tallclair · 2017-09-12T22:07:54Z

pkg/kubelet/stats/helper.go

+		rxBytes += inter.RxBytes
+		rxErrors += inter.RxErrors
+		txBytes += inter.TxBytes
+		txErrors += inter.TxErrors


I don't think that summing these statistics over all interfaces is the right approach. For instance, you probably don't care about how much traffic is going over loopback. I think the right solution is to either make it possible to select the monitored interface (see #28407), or modify the API to include a list of network stats.

For instance, you probably don't care about how much traffic is going over loopback.

Actually, when requesting cadvisor to return network interface values, network interface with name contains lo, veth and docker will be ignored.. So, this should not be the problem.

@tallclair PTAL.

ping @tallclair

OK, I could be sold on this, but I would prefer to also offer the per-interface stats through another method. Perhaps someone from @kubernetes/sig-network-pr-reviews can chime in about how common multiple-interfaces are, and any other potential side effects to combining the stats.

Another example, I just looked at a random node on a GKE cluster and it has a cbr0 device (container bridge), which should definitely be counted separately from the eth0 stats.

I just looked at a random node on a GKE cluster and it has a cbr0 device (container bridge), which should definitely be counted separately from the eth0 stats.

@tallclair This should be done in cAdvisor.

tallclair · 2017-09-12T22:08:53Z

pkg/kubelet/stats/stats_provider_test.go

@@ -59,6 +59,8 @@ const (
 	offsetFsTotalUsageBytes
 	offsetFsBaseUsageBytes
 	offsetFsInodeUsage
+
+	cbr0NetworkSeed = 100


nit: call this something other than seed, since it's not seeding anything. Maybe value, since it's fixed?

Done. PTAL.

andyxning · 2017-09-13T09:29:13Z

/test pull-kubernetes-e2e-gce-etcd3
/test pull-kubernetes-e2e-gce-bazel

andyxning · 2017-09-16T01:33:40Z

ping @yujuhong @timstclair @xiangpengzhao @vishh

andyxning · 2017-09-20T15:50:43Z

@smarterclayton
/sig node

smarterclayton · 2017-09-20T15:58:03Z

You have made me a happy man.

tallclair · 2017-11-28T00:10:28Z

Yes, I think this should go in 1.9.

andyxning · 2017-11-28T10:17:47Z

Ping @thockin @dchen1107 Could you please help to add milestone related label for this to be merged in 1.9?

k8s-github-robot · 2017-11-28T21:59:06Z

[MILESTONENOTIFIER] Milestone Pull Request Current

@andyxning @dchen1107 @tallclair @thockin @yujuhong

Note: This pull request is marked as priority/critical-urgent, and must be updated every 1 day during code freeze.

Example update:

ACK.  In progress
ETA: DD/MM/YYYY
Risks: Complicated fix required

Pull Request Labels

sig/node: Pull Request will be escalated to these SIGs if needed.
priority/critical-urgent: Never automatically move pull request out of a release milestone; continually escalate to contributor and SIG through all available channels.
kind/bug: Fixes a bug discovered during the current release.

Help

dchen1107 · 2017-11-28T21:59:37Z

I bumped the priority to critical-urgent to make sure it is merged into v1.9 since it fixes the real production issues, and risk is very low.

dchen1107 · 2017-11-28T22:01:35Z

/lgtm

k8s-github-robot · 2017-11-28T22:02:02Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andyxning, dchen1107, tallclair

Associated issue: 1788

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~pkg/kubelet/OWNERS~~ [dchen1107,tallclair]
~~test/e2e_node/OWNERS~~ [dchen1107,tallclair]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

k8s-github-robot · 2017-11-28T22:13:47Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2017-11-28T22:59:08Z

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

andyxning · 2017-11-29T00:31:02Z

Thanks @dchen1107 @tallclair

k8s-ci-robot requested review from DirectXMan12, piosz, timstclair, vishh and xiangpengzhao September 8, 2017 04:14

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Sep 8, 2017

k8s-github-robot assigned yujuhong and dchen1107 Sep 8, 2017

andyxning mentioned this pull request Sep 8, 2017

fix network metric value for multiple interfaces kubernetes-retired/heapster#1788

Merged

andyxning force-pushed the fix_network_value_for_stats_summary branch from 3862eb9 to 6277040 Compare September 8, 2017 05:35

andyxning changed the title ~~fix network value for stats summary~~ fix network value for stats summary for multiple network interfaces Sep 8, 2017

andyxning force-pushed the fix_network_value_for_stats_summary branch from 6277040 to 3748f94 Compare September 9, 2017 00:15

tallclair reviewed Sep 12, 2017

View reviewed changes

andyxning force-pushed the fix_network_value_for_stats_summary branch from 3748f94 to bdf8b23 Compare September 13, 2017 04:07

k8s-ci-robot added the sig/node Categorizes an issue or PR as relevant to SIG Node. label Sep 20, 2017

k8s-github-robot added the milestone/incomplete-labels label Nov 28, 2017

k8s-github-robot added milestone/removed and removed milestone/incomplete-labels labels Nov 28, 2017

k8s-github-robot removed this from the v1.9 milestone Nov 28, 2017

dchen1107 added this to the v1.9 milestone Nov 28, 2017

dchen1107 removed the milestone/removed label Nov 28, 2017

k8s-github-robot added the milestone/removed label Nov 28, 2017

k8s-github-robot removed this from the v1.9 milestone Nov 28, 2017

dchen1107 added priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. and removed priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. milestone/removed labels Nov 28, 2017

dchen1107 added this to the v1.9 milestone Nov 28, 2017

k8s-github-robot added the milestone/needs-attention label Nov 28, 2017

dchen1107 added the status/in-progress label Nov 28, 2017

k8s-github-robot removed the milestone/needs-attention label Nov 28, 2017

k8s-github-robot merged commit 8226973 into kubernetes:master Nov 28, 2017

andyxning deleted the fix_network_value_for_stats_summary branch November 29, 2017 00:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix network value for stats summary for multiple network interfaces #52144

fix network value for stats summary for multiple network interfaces #52144

andyxning commented Sep 8, 2017 •

edited

andyxning commented Sep 8, 2017

DirectXMan12 commented Sep 8, 2017

andyxning commented Sep 8, 2017 •

edited

andyxning commented Sep 9, 2017

DirectXMan12 commented Sep 12, 2017

DirectXMan12 commented Sep 12, 2017

andyxning commented Sep 12, 2017

tallclair Sep 12, 2017

andyxning Sep 13, 2017

andyxning Sep 14, 2017

andyxning Sep 18, 2017

tallclair Sep 26, 2017

tallclair Sep 26, 2017

andyxning Sep 26, 2017

tallclair Sep 12, 2017

andyxning Sep 13, 2017

andyxning Sep 14, 2017

andyxning commented Sep 13, 2017

andyxning commented Sep 16, 2017

andyxning commented Sep 20, 2017

smarterclayton commented Sep 20, 2017

tallclair commented Nov 28, 2017

andyxning commented Nov 28, 2017

k8s-github-robot commented Nov 28, 2017

dchen1107 commented Nov 28, 2017

dchen1107 commented Nov 28, 2017

k8s-github-robot commented Nov 28, 2017

k8s-github-robot commented Nov 28, 2017

k8s-github-robot commented Nov 28, 2017

andyxning commented Nov 29, 2017

fix network value for stats summary for multiple network interfaces #52144

fix network value for stats summary for multiple network interfaces #52144

Conversation

andyxning commented Sep 8, 2017 • edited

andyxning commented Sep 8, 2017

DirectXMan12 commented Sep 8, 2017

andyxning commented Sep 8, 2017 • edited

andyxning commented Sep 9, 2017

DirectXMan12 commented Sep 12, 2017

DirectXMan12 commented Sep 12, 2017

andyxning commented Sep 12, 2017

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

Choose a reason for hiding this comment

andyxning commented Sep 13, 2017

andyxning commented Sep 16, 2017

andyxning commented Sep 20, 2017

smarterclayton commented Sep 20, 2017

tallclair commented Nov 28, 2017

andyxning commented Nov 28, 2017

k8s-github-robot commented Nov 28, 2017

dchen1107 commented Nov 28, 2017

dchen1107 commented Nov 28, 2017

k8s-github-robot commented Nov 28, 2017

k8s-github-robot commented Nov 28, 2017

k8s-github-robot commented Nov 28, 2017

andyxning commented Nov 29, 2017

andyxning commented Sep 8, 2017 •

edited

andyxning commented Sep 8, 2017 •

edited