Add support to disable diskIO metrics #2103

WanLinghao · 2018-11-22T03:28:02Z

Add support to disable diskIO metrics
ref:#2045

googlebot · 2018-11-22T03:28:06Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

WanLinghao · 2018-11-22T03:29:37Z

@dashpole @SamSaffron PTAL

WanLinghao · 2018-11-22T05:04:21Z

/test pull-cadvisor-e2e

WanLinghao · 2019-01-11T08:09:10Z

/test cla/google

WanLinghao · 2019-01-11T08:09:34Z

/retest

WanLinghao · 2019-01-11T08:12:03Z

I signed it!

WanLinghao · 2019-01-11T09:05:53Z

I signed it!

googlebot · 2019-01-11T09:05:55Z

CLAs look good, thanks!

WanLinghao · 2019-01-11T09:06:35Z

@dashpole CLA cert has been fixed, PTAL

dashpole

this is a great change to make. Just a couple comments

dashpole · 2019-01-11T18:47:06Z

container/raw/handler.go

@@ -117,7 +120,8 @@ func (self *rawContainerHandler) Cleanup() {}

 func (self *rawContainerHandler) GetSpec() (info.ContainerSpec, error) {
 	const hasNetwork = false
-	hasFilesystem := isRootCgroup(self.name) || len(self.externalMounts) > 0
+	hasFilesystem := (isRootCgroup(self.name) || len(self.externalMounts) > 0) &&


What is this fixing? It isn't related to diskIO AFAICT

It is a DiskUsageMetrics bug that people will see has_filesystem be true even they disable disk metrics. I will extract it and launch a new patch later if what I thought is correct @dashpole

dashpole · 2019-01-11T19:08:57Z

container/common/helpers.go

@@ -258,6 +258,13 @@ func assignDeviceNamesToPerDiskStats(namer DeviceNamer, diskStats ...[]info.PerD
 	}
 }

+// FilterCgroupPaths delete cgroup paths which we do't need base on includeMetrics
+func FilterCgroupPaths(cgroups map[string]string, includedMetrics container.MetricSet) {


rather than filtering out afterwards, I think it would be cleaner to do this when we first get the list of cgroup subsystems: https://github.com/google/cadvisor/blob/master/container/libcontainer/helpers.go#L37.

dashpole · 2019-01-11T19:09:54Z

container/common/helpers.go

@@ -258,6 +258,13 @@ func assignDeviceNamesToPerDiskStats(namer DeviceNamer, diskStats ...[]info.PerD
 	}
 }

+// FilterCgroupPaths delete cgroup paths which we do't need base on includeMetrics
+func FilterCgroupPaths(cgroups map[string]string, includedMetrics container.MetricSet) {
+	if !includedMetrics.Has(container.DiskIOMetrics) {


we don't currently do this for other ignored metrics. What is the benefit of removing the blkio cgroup from the list of cgroups?

As far as I can see, if we don't remove the blkio cgroup, it would keep extracting data from /sys/fs/cgroup/blkio/(ref:

cadvisor/vendor/github.com/opencontainers/runc/libcontainer/cgroups/fs/blkio.go

Line 115 in 45d3b77

func getBlkioStat(path string) ([]cgroups.BlkioStatEntry, error) {

). And the data it extracted would be just dropped cause we have disabled the diskIO metrics. So here I am trying to do is avoiding useless IO request when diskIO was disabled.

dashpole · 2019-01-11T19:11:18Z

container/libcontainer/handler.go

@@ -599,14 +600,16 @@ func setNetworkStats(libcontainerStats *libcontainer.Stats, ret *info.ContainerS
 	}
 }

-func newContainerStats(libcontainerStats *libcontainer.Stats, withPerCPU bool) *info.ContainerStats {
+func newContainerStats(libcontainerStats *libcontainer.Stats, withPerCPU, withDiskIO bool) *info.ContainerStats {


Now that we are passing in two of these, I would rather just pass ignoreMetrics to the function.

WanLinghao · 2019-01-14T08:19:22Z

@dashpole All comments addressed, PTAL

dashpole · 2019-01-14T18:48:01Z

container/raw/handler.go

@@ -117,7 +119,7 @@ func (self *rawContainerHandler) Cleanup() {}

 func (self *rawContainerHandler) GetSpec() (info.ContainerSpec, error) {
 	const hasNetwork = false
-	hasFilesystem := isRootCgroup(self.name) || len(self.externalMounts) > 0
+	hasFilesystem := (isRootCgroup(self.name) || len(self.externalMounts) > 0)


nit: remove extra parenthesis

dashpole

looks good after the nit

WanLinghao · 2019-01-15T01:46:37Z

comments addressed @dashpole

WanLinghao force-pushed the disable-diskio branch from 63e8463 to 201f2f7 Compare November 22, 2018 05:50

dashpole reviewed Jan 11, 2019

View reviewed changes

WanLinghao force-pushed the disable-diskio branch from 201f2f7 to 1ecbdd5 Compare January 14, 2019 08:04

dashpole reviewed Jan 14, 2019

View reviewed changes

dashpole mentioned this pull request Jan 14, 2019

The relationship between huge CPU usage and options #2144

Open

dashpole approved these changes Jan 14, 2019

View reviewed changes

Add support to disable diskIO metrics

4eab5b6

WanLinghao force-pushed the disable-diskio branch from 1ecbdd5 to 4eab5b6 Compare January 15, 2019 01:45

dashpole merged commit 8240c4a into google:master Jan 15, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support to disable diskIO metrics #2103

Add support to disable diskIO metrics #2103

WanLinghao commented Nov 22, 2018 •

edited

Loading

googlebot commented Nov 22, 2018

WanLinghao commented Nov 22, 2018

WanLinghao commented Nov 22, 2018

WanLinghao commented Jan 11, 2019

WanLinghao commented Jan 11, 2019

WanLinghao commented Jan 11, 2019

WanLinghao commented Jan 11, 2019

googlebot commented Jan 11, 2019

WanLinghao commented Jan 11, 2019

dashpole left a comment

dashpole Jan 11, 2019

WanLinghao Jan 14, 2019 •

edited

Loading

dashpole Jan 11, 2019

dashpole Jan 11, 2019

WanLinghao Jan 14, 2019

dashpole Jan 11, 2019

WanLinghao commented Jan 14, 2019

dashpole Jan 14, 2019

dashpole left a comment

WanLinghao commented Jan 15, 2019

Add support to disable diskIO metrics #2103

Add support to disable diskIO metrics #2103

Conversation

WanLinghao commented Nov 22, 2018 • edited Loading

googlebot commented Nov 22, 2018

What to do if you already signed the CLA

Individual signers

Corporate signers

WanLinghao commented Nov 22, 2018

WanLinghao commented Nov 22, 2018

WanLinghao commented Jan 11, 2019

WanLinghao commented Jan 11, 2019

WanLinghao commented Jan 11, 2019

WanLinghao commented Jan 11, 2019

googlebot commented Jan 11, 2019

WanLinghao commented Jan 11, 2019

dashpole left a comment

Choose a reason for hiding this comment

dashpole Jan 11, 2019

Choose a reason for hiding this comment

WanLinghao Jan 14, 2019 • edited Loading

Choose a reason for hiding this comment

dashpole Jan 11, 2019

Choose a reason for hiding this comment

dashpole Jan 11, 2019

Choose a reason for hiding this comment

WanLinghao Jan 14, 2019

Choose a reason for hiding this comment

dashpole Jan 11, 2019

Choose a reason for hiding this comment

WanLinghao commented Jan 14, 2019

dashpole Jan 14, 2019

Choose a reason for hiding this comment

dashpole left a comment

Choose a reason for hiding this comment

WanLinghao commented Jan 15, 2019

WanLinghao commented Nov 22, 2018 •

edited

Loading

WanLinghao Jan 14, 2019 •

edited

Loading