Cadvisor now publishes per-container inode stats by dashpole · Pull Request #1489 · google/cadvisor

dashpole · 2016-10-03T16:36:44Z

Per-container stats available in the v2 api in the FileSystemStats.InodeUsage field. They are available in the v1 stats in the Filesystem.Inodes field. Per-container Inode stats collected using the command "find . -xdev | wc -l". The collection of inode stats uses the same back-off strategy as measuring per container bytes used, so that slow calls to find do not severely impact performance.

dashpole · 2016-10-03T19:46:38Z

Issue: kubernetes/kubernetes#33382

timstclair

I left a few suggestions around optimizing the file counting. I don't know if it's all necessary, but maybe worth considering.

timstclair · 2016-10-03T18:07:29Z

 type FsHandler interface {
 	Start()
-	Usage() (baseUsageBytes uint64, totalUsageBytes uint64)
+	Usage() (baseUsageBytes, totalUsageBytes, inodeUsage uint64)


nit: Can we create a struct to return with this info instead? IMHO there are very few situations where a method should return more than 2 values...

timstclair · 2016-10-03T18:54:01Z

+			}
+		}
+	}
+}


How about extracting something like a "BackoffPoller" that abstracts all the shared backoff & polling logic between trackInodesUsage & trackDiskUsage?

Actually, on further thought I'm wondering if this should be in the same routine as the du call. The advantages are that containers with very large filesystems wouldn't hog as many exec calls, and there might be less disk thrashing.

timstclair · 2016-10-03T18:55:13Z


 	if !ignoreMetrics.Has(container.DiskUsageMetrics) {
-		handler.fsHandler = common.NewFsHandler(time.Minute, rootfsStorageDir, "", fsInfo)
+		handler.fsHandler = common.NewFsHandler(time.Minute, time.Minute, rootfsStorageDir, "", fsInfo)


nit: make constants for the default periods

timstclair · 2016-10-03T19:57:45Z

+		err        error
+	)
+
+	if fh.rootfs != "" {


Under what conditions would the rootfs not be set? It should probably be an error, or exit the tracking loop...

timstclair · 2016-10-03T20:28:17Z

+	}
+	claimFindToken()
+	defer releaseFindToken()
+	findCmd := exec.Command("find", dir, "-xdev")


When I tried this, I found find $dir -xdev -printf '.' | wc -c to be about twice as fast. Might be a premature optimization though...

Ill go ahead and try it out. It shouldnt hurt to make it faster, and it should still work.

timstclair · 2016-10-03T20:36:50Z

+		glog.Errorf("failed to read from stdout for cmds: %v, %v - %v", findCmd.Args, wcCmd.Args, souterr)
+	}
+	// return 0, fmt.Errorf("cmd %v output %s", cmd.Args, stdout)
+	inodeUsage, err := strconv.ParseUint(strings.Fields(stdout)[0], 10, 64)


Is the fields call necessary?

stdout = int64 + \n, so the Fields call was just to remove the newline character.

timstclair · 2016-10-03T20:38:13Z

 func (fh *realFsHandler) Start() {
-	go fh.trackUsage()
+	go fh.trackDiskUsage()
+	go fh.trackInodeUsage()


Does it make sense to track inode usage for devicemapper? If not we should skip it.

AFAIK, inodes cannot be tracked for devicemapper.

See the note I made in the Issue. I was able to test this, and the find command works correctly (and takes almost no time, since it always finds 0 inodes) with devicemapper.

timstclair · 2016-10-03T20:41:32Z

+	}
+	claimFindToken()
+	defer releaseFindToken()
+	findCmd := exec.Command("find", dir, "-xdev")


I think this will double count inodes on base layers shared across containers. That's probably what we want, but it means killing a countainer will not reclaim all it's reported inodes.

It should not be doing that. dir here is expected to be the writable layer of a container. We need to test this.

I am very likely wrong, but I think the "rootfs" directory that it is pointed at is the container's modifiable directory, and shouldn't contain the base layers. At least in most of my tests, the containers start out reporting 0 inodes used, which would indicate to me that they are not counting any shared resources in base layers.

If that's the case, shouldn't it be zero for a new container? That's not what I'm seeing.

I tested this. It only includes the writeable layer.

timstclair · 2016-10-03T20:42:33Z

+	}
+	inodes, err := fsInfo.GetDirInodeUsage(dir, time.Minute)
+	as.NoError(err)
+	as.True(uint64(numFiles) <= inodes, "expected inodes in dir to be at-least %d; got inodes: %d", numFiles, inodes)


shouldn't numFiles == inodes? Under what circumstances would it be different?

I believe it is actually, numFiles + 1 == inodes, since the directory I created counts as well. I could change it to that, but I wasn't sure if it would consistently be the case.

Confirmed, I changed it to numFiles +1 == inodes

timstclair · 2016-10-03T20:44:06Z

 				stat.Filesystem = &FilesystemStats{
 					TotalUsageBytes: &val.Filesystem[0].Usage,
 					BaseUsageBytes:  &val.Filesystem[0].BaseUsage,
+					InodeUsage:      &val.Filesystem[0].Inodes,


If we skip inode collection for some storage drivers, then we should probably leave InodeUsage nil here when it's not collected.

I think that is what actually happens. &val.Filesystem[0].Inodes may be nil if we skipped collection. In that case, it should also set InodeUsage to nil as well?

I don't think so. If you're taking the address of a variable, it will always be non-nil. The value might be zero, but if possible we should distinguish between 0 and not set (nil).

Wouldn't it actually be more accurate to set InodesUsed to 0 in that case? We only skip collection for some storage drivers if they do not use inodes, which means they use 0 of the inodes shared by the rest of the pods.

Just to document, since these storage drivers do not use inodes, I am leaving this the way it is, since using 0 inodes is equivalent to using no inodes. Also, making a change here would require chaning the v1 api, or a large refactor of the code.

timstclair · 2016-10-04T20:22:38Z

ok to test

timstclair · 2016-10-04T20:28:07Z

@k8s-bot test this

ixdy · 2016-10-04T20:35:55Z

@k8s-bot ok to test

spxtr · 2016-10-04T20:42:54Z

@k8s-bot test this

It didn't have permissions to set the status, but the test ran :P

dashpole · 2016-10-04T22:53:16Z

pushed a commit addressing all comments except device-mapper related ones.

timstclair · 2016-10-07T23:24:12Z

+			TotalUsageBytes: 0,
+			BaseUsageBytes:  0,
+			InodeUsage:      0,
+		},


nit: no need to specify this (values default to 0)

timstclair · 2016-10-07T23:26:54Z

+	if extraDiskErr == nil && fh.extraDir != "" {
+		fh.usage.BaseUsageBytes = baseUsage
+	}
+	//combine errors into a single error to return


nit: space after //, start comments with a Capital.

timstclair · 2016-10-07T23:30:27Z

+const maxConcurrentDus = 20
+
+// The maximum number of `find` tasks that can be running at once.
+const maxConcurrentFinds = 20


Do we need a separate pool for each of these? On the one hand, if one operation is significantly slower, it prevents it from hogging the tokens. On the other hand, the operations happen in sequence, so the one operation will be blocking anyway. I'm leaning towards a single "exec" pool.

timstclair · 2016-10-07T23:36:10Z

+	wcCmd := exec.Command("wc", "-c")
+	wcCmd.Stdin, _ = findCmd.StdoutPipe()
+
+	stdoutp, err := wcCmd.StdoutPipe()


I think it would be better to assign a bytes.Buffer to stdout and stderr. I'm not positive, but I tihnk the process could hang if the pipe fills up, waiting for it to be drained.

timstclair · 2016-10-07T23:36:32Z

 	as.True(expectedSize <= size, "expected dir size to be at-least %d; got size: %d", expectedSize, size)
 }

+//make sure that the timeout is actually being triggered (found this bug in PR#1489)


nit: spaces & capitals, same below.

timstclair · 2016-10-07T23:37:13Z

+	dir, err := ioutil.TempDir(os.TempDir(), "")
+	as.NoError(err)
+	defer os.RemoveAll(dir)
+	dataSize := 1024 * 10000 //1000 KB bigger to make sure it triggers tihe timeout


s/tihe/the/

Did you mean 1,000 KB or 10,000 KB?

looks like you missed the typo

timstclair · 2016-10-07T23:38:48Z

+	numFiles := 1000
+	for i := 0; i < numFiles; i++ {
+		_, err := ioutil.TempFile(dir, "")
+		as.NoError(err)


I think you should use a require here, otherwise this could be a lot of error spam.

timstclair · 2016-10-07T23:39:07Z

+	numFiles := 100000 //make sure we actually trigger the timeout
+	for i := 0; i < numFiles; i++ {
+		_, err := ioutil.TempFile(dir, "")
+		as.NoError(err)


k8s-ci-robot · 2016-10-12T00:25:10Z

Jenkins GCE e2e failed for commit acb2426. Full PR test history.

The magic incantation to run this job again is @k8s-bot test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

timstclair · 2016-10-12T18:40:19Z

LGTM once you fix the typo and squash the commits.

…ind . -xdev printf '.'| wc -c' this is published in the v2 api using a new field

Closes: kubernetes#2896 According to the report, the google/cadvisor#1422 has been closed. However, the related issue has been fixed in google/cadvisor#1489 and merged a long time ago. We can safely remove the known issue now.

timstclair added the release-note label Oct 3, 2016

timstclair reviewed Oct 3, 2016

View reviewed changes

timstclair self-assigned this Oct 4, 2016

timstclair closed this Oct 4, 2016

timstclair reopened this Oct 4, 2016

dashpole force-pushed the per_container_inode_stats branch from d0aee10 to 9835f82 Compare October 6, 2016 17:40

timstclair reviewed Oct 7, 2016

View reviewed changes

dashpole force-pushed the per_container_inode_stats branch from eb60f6e to e92659c Compare October 11, 2016 20:12

dashpole force-pushed the per_container_inode_stats branch from ee2e96e to 633578d Compare October 12, 2016 20:13

timstclair added the lgtm label Oct 12, 2016

Cadvisor now publishes per-container inode stats using the command 'f…

9fdeefe

…ind . -xdev printf '.'| wc -c' this is published in the v2 api using a new field

dashpole force-pushed the per_container_inode_stats branch from 633578d to 9fdeefe Compare October 12, 2016 20:16

timstclair approved these changes Oct 13, 2016

View reviewed changes

timstclair merged commit e972272 into google:master Oct 13, 2016

dashpole deleted the per_container_inode_stats branch October 13, 2016 20:23

mdshuai mentioned this pull request Nov 21, 2016

Ability to know how many inodes used per container #1422

Closed

haegrr mentioned this pull request Dec 13, 2016

BusyBox find does not support -printf #1556

Closed

tengqm mentioned this pull request Jan 1, 2018

Fix out-of-resource future work kubernetes/website#6807

Merged

Conversation

dashpole commented Oct 3, 2016

Uh oh!

dashpole commented Oct 3, 2016

Uh oh!

timstclair 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!

Choose a reason for hiding this comment

Uh oh!

dashpole Oct 4, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

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!

timstclair commented Oct 4, 2016

Uh oh!

timstclair commented Oct 4, 2016

Uh oh!

ixdy commented Oct 4, 2016

Uh oh!

spxtr commented Oct 4, 2016

Uh oh!

dashpole commented Oct 4, 2016

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!

dashpole Oct 4, 2016 •

edited

Loading