add docker_only_prefix_whitelist #1926

andyxning · 2018-04-10T07:23:17Z

Partially fix kubernetes/kubernetes#61994

This PR adds docker_only_prefix_whitelist to cadvisor to specify A comma-separated list of cgroup path prefix that needs to be collected even docker-only is specified.

/cc @dashpole @tallclair

dashpole · 2018-04-10T16:32:48Z

container/raw/factory.go

@@ -29,6 +30,7 @@ import (
 )

 var dockerOnly = flag.Bool("docker_only", false, "Only report docker containers in addition to root stats")
+var dockerOnlyPrefixWhitelist = flag.String("docker_only_prefix_whitelist", "", "A comma-separated list of cgroup path prefix that needs to be collected when docker-only is specified")


can we call this rawPrefixWhitelist? I am hoping we can move away from dockerOnly, and instead use a whitelist of handlers in the future.

dashpole · 2018-04-10T16:33:10Z

lgtm other than naming nit

dashpole · 2018-04-10T16:40:37Z

Actually, how would you feel about making this an argument to manager.New()? The desire to whitelist particular cgroup trees seems fairly kubernetes specific, and I would rather not add another flag if we can avoid it. I think this would be a win for everyone in the community, as everyone would see better performance. It would also give us flexibility to change this interface in the future if we want a different model without having to deprecate flags and such.

The only downside is that if people are using cadvisor metrics for particular cgroups that are outside of what we plan to monitor, those would no longer be accessible.

andyxning · 2018-04-11T02:57:18Z

how would you feel about making this an argument to manager.New()?

I have also thought about this since rawPrefixWhitelist is a inter package variable. What i think about integrate this cAdvisor functionality into Kubernetes is to only pass this raw prefix whitelist when docker_only is specified automatically. Adding rawPrefixWhitelist should help us to easily integrate this functionality into Kubernetes.

The only downside is that if people are using cadvisor metrics for particular cgroups that are outside of what we plan to monitor, those would no longer be accessible.

Yep. That is the truth when make rawPrefixWhitelist an argument to manager.New(). IMHO, it should be fine as no users have complained about lacking the ability to customize the whitelist when docker_only has been specified. Since we have no clear plan about how to evolve the related settings, leaving customizing whitelist from command line seems fine. If we need this later, it should also not difficult to add the support.

andyxning · 2018-04-12T05:53:32Z

/retest

andyxning · 2018-04-12T06:28:08Z

@dashpole @tallclair PTAL. I have moved the configuration from command line to manager field. This should be fine for us to integrate cadvisor into Kubernetes.

dashpole · 2018-04-13T17:07:41Z

container/raw/factory.go

@@ -62,14 +66,24 @@ func (self *rawFactory) NewContainerHandler(name string, inHostNamespace bool) (
 // The raw factory can handle any container. If --docker_only is set to false, non-docker containers are ignored.
 func (self *rawFactory) CanHandleAndAccept(name string) (bool, bool, error) {
 	accept := name == "/" || !*dockerOnly
+
+	if *dockerOnly {


instead of having this only in effect when --docker_only is specified, can we make it always take effect, but have the default be [ "/" ], so all raw cgroups are collected? We can make setting --docker_only set this to [ ] during creation of the raw factory.

Sounds good to me. Will do.

We can make setting --docker_only set this to [ ] during creation of the raw factory.

We should not do this, imo. What we want is to respect with whitelist settings even docker_only is set and we should customize whitelist settings to at least []string{"/kubepods"} to allow pod level metrics in Kubernetes.

What i think is about this:

by default setting rawPrefixWhiteList to []string{"/"} to allow all raw cgroups to be collected or be compatible with backwards

always filter whitelist raw cgroups and respect with the whitelist settings.

andyxning · 2018-04-18T15:32:52Z

Ping @dashpole @tallclair

BTW, after this PR is merged, i will prepare an PR to bump cAdvisor dep to Kubernetes to utilizing this new change.

dashpole · 2018-04-18T17:45:49Z

This looks good to me, although I would really like to hear @tallclair 's opinion on this.
@andyxning have you brought this up at sig-instrumentation at all? It may be valuable to hear more opinions on limiting the cgroups that are scraped.

andyxning · 2018-04-19T08:20:59Z

have you brought this up at sig-instrumentation at all? It may be valuable to hear more opinions on limiting the cgroups that are scraped.

No. I have not talk about this in sig-instrumentation. Do we need to do a email to sig-instrumentation to describe this PR.

andyxning · 2018-04-22T00:57:27Z

@dashpole Seems @tallclair has temporarily no response. Do anyone else can help us to review
this?

andyxning · 2018-05-01T14:06:57Z

Ping @dashpole @tallclair

andyxning · 2018-05-07T10:01:08Z

Ping @dashpole @tallclair in case the notification emails are lost in your inbox. :)

andyxning · 2018-05-14T02:34:34Z

/cc @dashpole @tallclair

andyxning · 2018-05-18T05:35:58Z

Ping @dashpole @tallclair

andyxning · 2018-05-24T03:00:58Z

@dashpole @tallclair PTAL. 😆

andyxning · 2018-05-31T08:19:47Z

/cc @dashpole @tallclair PTAL.

andyxning · 2018-06-06T02:25:40Z

/cc @dashpole @tallclair PTAL.

dashpole

LGTM

andyxning · 2018-06-22T01:53:51Z

Will make a PR to Kubernetes once the dependency for cAdvisor is updated to make use of the new functionality. :)

serathius · 2018-07-16T12:23:29Z

@andyxning is this integrated into k8s?

andyxning · 2018-07-16T14:20:03Z

@serathius No. Will work on this later this week.

andyxning force-pushed the add_docker-only_whitelist branch 2 times, most recently from f6ce87c to 2259e16 Compare April 10, 2018 08:14

andyxning mentioned this pull request Apr 10, 2018

Incompatibility for kubelet stats api and cadvisor docker-only option kubernetes/kubernetes#61994

Closed

dashpole self-assigned this Apr 10, 2018

dashpole reviewed Apr 10, 2018

View reviewed changes

andyxning force-pushed the add_docker-only_whitelist branch from 2259e16 to 10b3f0c Compare April 11, 2018 02:39

andyxning force-pushed the add_docker-only_whitelist branch 2 times, most recently from 007b71d to 2f1250a Compare April 12, 2018 05:29

andyxning force-pushed the add_docker-only_whitelist branch from 2f1250a to 472c208 Compare April 12, 2018 06:20

dashpole reviewed Apr 13, 2018

View reviewed changes

add raw container cgroup path prefix whitelist

ed65210

andyxning force-pushed the add_docker-only_whitelist branch from 472c208 to ed65210 Compare April 16, 2018 04:06

dashpole approved these changes Jun 21, 2018

View reviewed changes

dashpole merged commit b1535b8 into google:master Jun 21, 2018

andyxning deleted the add_docker-only_whitelist branch June 22, 2018 01:52

dashpole mentioned this pull request Sep 18, 2018

Reducing cAdvisor memory usage #2045

Open

dashpole mentioned this pull request Oct 4, 2018

Exclude metric collection of certain cgroup groupnames #2048

Open

viberan mentioned this pull request Dec 17, 2018

exclude system.slice metrics from being exposed as prometheus metrics #2129

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add docker_only_prefix_whitelist #1926

add docker_only_prefix_whitelist #1926

andyxning commented Apr 10, 2018

dashpole Apr 10, 2018

dashpole commented Apr 10, 2018

dashpole commented Apr 10, 2018

andyxning commented Apr 11, 2018

andyxning commented Apr 12, 2018

andyxning commented Apr 12, 2018

dashpole Apr 13, 2018

andyxning Apr 15, 2018

andyxning Apr 16, 2018 •

edited

Loading

andyxning Apr 16, 2018

andyxning Apr 16, 2018

andyxning commented Apr 18, 2018

dashpole commented Apr 18, 2018

andyxning commented Apr 19, 2018

andyxning commented Apr 22, 2018

andyxning commented May 1, 2018

andyxning commented May 7, 2018

andyxning commented May 14, 2018

andyxning commented May 18, 2018

andyxning commented May 24, 2018

andyxning commented May 31, 2018

andyxning commented Jun 6, 2018

dashpole left a comment

andyxning commented Jun 22, 2018

serathius commented Jul 16, 2018

andyxning commented Jul 16, 2018

add docker_only_prefix_whitelist #1926

add docker_only_prefix_whitelist #1926

Conversation

andyxning commented Apr 10, 2018

dashpole Apr 10, 2018

Choose a reason for hiding this comment

dashpole commented Apr 10, 2018

dashpole commented Apr 10, 2018

andyxning commented Apr 11, 2018

andyxning commented Apr 12, 2018

andyxning commented Apr 12, 2018

dashpole Apr 13, 2018

Choose a reason for hiding this comment

andyxning Apr 15, 2018

Choose a reason for hiding this comment

andyxning Apr 16, 2018 • edited Loading

Choose a reason for hiding this comment

andyxning Apr 16, 2018

Choose a reason for hiding this comment

andyxning Apr 16, 2018

Choose a reason for hiding this comment

andyxning commented Apr 18, 2018

dashpole commented Apr 18, 2018

andyxning commented Apr 19, 2018

andyxning commented Apr 22, 2018

andyxning commented May 1, 2018

andyxning commented May 7, 2018

andyxning commented May 14, 2018

andyxning commented May 18, 2018

andyxning commented May 24, 2018

andyxning commented May 31, 2018

andyxning commented Jun 6, 2018

dashpole left a comment

Choose a reason for hiding this comment

andyxning commented Jun 22, 2018

serathius commented Jul 16, 2018

andyxning commented Jul 16, 2018

andyxning Apr 16, 2018 •

edited

Loading