On-Demand container metrics #1779

dashpole · 2017-10-23T23:12:53Z

This is my take on implementing #1247.
This PR does the following:

Adds an UpdateStats field to v2 request options. Setting this to true causes cAdvisor to update stats for that container before returning.
Adds an OnDemandHousekeeping function to the containerData object, which allows housekeeping to be triggered before the interval.
This removes the check with instantaneous CPU usage if the difference is too small. This check could result in not getting instantaneous CPU usage if OnDemandHousekeeping is triggered right after the periodic collection.

There is still only a single thread which calls housekeepingTick().
The UpdateStats field is part of the v2 API in cAdvisor, but will be used only for node-level metrics in kubernetes. See kubernetes/kubernetes@master...dashpole:on_demand_metrics for the way this would be enabled in kubernetes.

dashpole · 2017-10-23T23:13:28Z

cc @derekwaynecarr

tallclair · 2017-11-07T21:45:48Z

info/v2/container.go

@@ -234,6 +234,8 @@ type RequestOptions struct {
 	Count int `json:"count"`
 	// Whether to include stats for child subcontainers.
 	Recursive bool `json:"recursive"`
+	// Whether to update stats before returning
+	UpdateStats bool `json:"update_stats"`


I wonder whether it would be more useful to specify this as MaxAge, and update the stats if they are older. 0 means always update, and something else (-1?) means never update.

I like that it is clearer. UpdateStats seemed vague, but I couldn't think of anything clearer. I have no plans to do anything other than trigger an update, mainly for responding when memcg thresholds are crossed. Do you think it is still worth doing MaxAge if we wont use it in k8s?

Yeah, I think it's still preferable. It's definitely useful outside of k8s, but even for the k8s usecase, there is probably some max age you could set for the memcg trigger (e.g. 10ms). It means that if a burst of requests came in simultaneously (for whatever reason), they can reuse the cached results.

Ok, I changed it to MaxAgeMS *int32. Although I was wondering if just using time.Duration directly would be better?

tallclair · 2017-11-07T21:54:53Z

manager/container.go

 	for {
+		houseKeepingTimer := time.NewTimer(c.nextHousekeepingInterval())


nit: create the timer outside the loop, and use Reset here.

tallclair · 2017-11-07T22:11:35Z

manager/container.go

+			// notify the calling function that housekeeping has completed
+			close(finishedChan)
+		case <-houseKeepingTimer.C:
+			c.housekeepingTick(longHousekeeping)


drain the finishedChan after this step. I.e.

for { select finishedChan := <-c.onDemandChan: close(finishedChan) default: break }

done, although I dont think we want a for loop around it.

why not? If there are multiple routines waiting for new stats and we just computed new stats, we should unblock them?

zSys-bot · 2017-11-07T23:26:30Z

Build finished.

zSys-bot · 2017-11-07T23:28:29Z

Build finished.

zSys-bot · 2017-11-08T22:31:32Z

Build finished.

zSys-bot · 2017-11-08T22:33:40Z

Build finished.

zSys-bot · 2017-11-08T22:36:35Z

Build finished.

zSys-bot · 2017-11-08T22:38:36Z

Build finished.

zSys-bot · 2017-11-08T22:46:29Z

Build finished.

zSys-bot · 2017-11-08T22:48:31Z

Build finished.

zSys-bot · 2017-11-08T22:51:28Z

Build finished.

zSys-bot · 2017-11-08T22:53:29Z

Build finished.

dashpole · 2017-11-13T18:18:48Z

FYI, I plan to cut cadvisor v0.28.1, which will correspond to k8s v1.9.0 this Friday, 11/17.

zSys-bot · 2017-11-14T21:06:34Z

Build finished.

zSys-bot · 2017-11-14T21:08:32Z

Build finished.

dashpole · 2017-11-14T23:08:46Z

info/v2/container.go

@@ -236,6 +236,9 @@ type RequestOptions struct {
 	Count int `json:"count"`
 	// Whether to include stats for child subcontainers.
 	Recursive bool `json:"recursive"`
+	// Update stats if they are older than MaxAge
+	// nil indicates no update, and 0 will always trigger an update.
+	MaxAge *time.Duration `json:"max_age"`


Duration is just a wrapper around int64, so this should be safe for the api
https://golang.org/src/time/time.go?s=21095:21114#L610

zSys-bot · 2017-11-14T23:16:32Z

Build finished.

zSys-bot · 2017-11-14T23:18:33Z

Build finished.

zSys-bot · 2017-11-18T02:16:20Z

Build finished.

zSys-bot · 2017-11-18T02:18:20Z

Build finished.

zSys-bot · 2017-11-20T00:48:23Z

Build finished.

zSys-bot · 2017-11-20T00:51:22Z

Build finished.

zSys-bot · 2017-11-20T02:38:38Z

Build finished.

zSys-bot · 2017-11-20T02:41:43Z

Build finished.

dashpole · 2017-11-20T02:53:20Z

Unit testing is complete. I had to plum through a clock. It should be fine to use apimachinery, right?

tallclair · 2017-11-20T21:15:25Z

manager/container_test.go

+			}
+			waitForHousekeeping.Done()
+		}
+	}


Correct me if I'm wrong, but I think you can get away with not mocking this:

onDemandCache := make(chan chan struct{}, numConcurrentCalls) for i := 0; i < numConcurrentCalls; i++ { go func() { c.OnDemandHousekeeping(0*time.Second) waitForHousekeeping.Done() } // Wait for work to be queued onDemandCache = append(onDemandCache, <-c.onDemandChan) } // Requeue work: for _, ch := range onDemandCache { c.OnDemandChan <- ch }

I can switch to that.

dashpole · 2017-11-20T21:16:04Z

I tested this in kubernetes/kubernetes and verified that it passes the summary API test as well

tallclair · 2017-11-20T21:16:16Z

Thanks for the tests. We've tried to avoid dependencies on Kuberneties to avoid the circular dependency, but I think it's ok in this case since it's a small utility package, and only used for testing.

zSys-bot · 2017-11-20T21:16:33Z

Build finished.

zSys-bot · 2017-11-20T21:18:35Z

Build finished.

zSys-bot · 2017-11-20T21:38:34Z

Build finished.

zSys-bot · 2017-11-20T21:41:35Z

Build finished.

dashpole · 2017-11-20T21:45:09Z

Ok, it should be ready

tallclair · 2017-11-20T22:44:58Z

manager/container_test.go

+			waitForHousekeeping.Done()
+		}()
+		// Wait for work to be queued
+		onDemandCache <- <-cd.onDemandChan


nit: I'd prefer a slice for the cache. It makes the requeue more straightforward.

yeah, but the double arrow looks really cool. Done

zSys-bot · 2017-11-20T22:53:34Z

Build finished.

zSys-bot · 2017-11-20T22:56:34Z

Build finished.

tallclair

/lgtm

thanks!

derekwaynecarr · 2017-11-21T05:01:50Z

@dashpole --- this is really awesome.

derekwaynecarr · 2017-11-21T05:02:00Z

fyi @sjenning xmas came early!

dashpole assigned tallclair Oct 23, 2017

dashpole force-pushed the on_demand_metrics branch from 6349de1 to a60138c Compare October 23, 2017 23:13

tallclair reviewed Nov 7, 2017

View reviewed changes

dashpole force-pushed the on_demand_metrics branch from a60138c to 812a1cf Compare November 7, 2017 23:23

dashpole force-pushed the on_demand_metrics branch from 812a1cf to 38790d5 Compare November 8, 2017 22:29

dashpole force-pushed the on_demand_metrics branch from 38790d5 to 3033ba8 Compare November 8, 2017 22:35

dashpole force-pushed the on_demand_metrics branch from 3033ba8 to 6df39e9 Compare November 8, 2017 22:43

dashpole force-pushed the on_demand_metrics branch from 6df39e9 to 473cc7d Compare November 8, 2017 22:49

dashpole mentioned this pull request Nov 13, 2017

Monitor /kubepods cgroup for allocatable metrics kubernetes/kubernetes#55638

Closed

dashpole force-pushed the on_demand_metrics branch from 473cc7d to fc516a2 Compare November 14, 2017 21:05

dashpole commented Nov 14, 2017

View reviewed changes

dashpole force-pushed the on_demand_metrics branch from fc516a2 to dfe268c Compare November 14, 2017 23:15

dashpole mentioned this pull request Nov 15, 2017

Memory Eviction Memcg notifications are not effective. kubernetes/kubernetes#51745

Closed

dashpole force-pushed the on_demand_metrics branch from dfe268c to 157b344 Compare November 16, 2017 23:13

dashpole force-pushed the on_demand_metrics branch from 311c646 to a8d0d2a Compare November 18, 2017 02:13

dashpole force-pushed the on_demand_metrics branch from a8d0d2a to 98d9614 Compare November 20, 2017 00:47

add apimachinery clock dependency

fd43dc1

tallclair reviewed Nov 20, 2017

View reviewed changes

dashpole force-pushed the on_demand_metrics branch from d68882f to b1b6227 Compare November 20, 2017 21:15

dashpole force-pushed the on_demand_metrics branch from b1b6227 to f54de49 Compare November 20, 2017 21:37

tallclair reviewed Nov 20, 2017

View reviewed changes

on demand metrics

3d6ad6d

dashpole force-pushed the on_demand_metrics branch from f54de49 to 3d6ad6d Compare November 20, 2017 22:51

tallclair approved these changes Nov 20, 2017

View reviewed changes

dashpole merged commit 17dcf1c into google:master Nov 20, 2017

dashpole deleted the on_demand_metrics branch November 20, 2017 23:06

		for {
		houseKeepingTimer := time.NewTimer(c.nextHousekeepingInterval())

On-Demand container metrics #1779

On-Demand container metrics #1779

Uh oh!

Conversation

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

Uh oh!

Uh oh!

dashpole commented Oct 23, 2017

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!

zSys-bot commented Nov 7, 2017

Uh oh!

zSys-bot commented Nov 7, 2017

Uh oh!

zSys-bot commented Nov 8, 2017

Uh oh!

zSys-bot commented Nov 8, 2017

Uh oh!

zSys-bot commented Nov 8, 2017

Uh oh!

zSys-bot commented Nov 8, 2017

Uh oh!

zSys-bot commented Nov 8, 2017

Uh oh!

zSys-bot commented Nov 8, 2017

Uh oh!

zSys-bot commented Nov 8, 2017

Uh oh!

zSys-bot commented Nov 8, 2017

Uh oh!

dashpole commented Nov 13, 2017

Uh oh!

zSys-bot commented Nov 14, 2017

Uh oh!

zSys-bot commented Nov 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zSys-bot commented Nov 14, 2017

Uh oh!

zSys-bot commented Nov 14, 2017

Uh oh!

zSys-bot commented Nov 18, 2017

Uh oh!

zSys-bot commented Nov 18, 2017

Uh oh!

zSys-bot commented Nov 20, 2017

Uh oh!

zSys-bot commented Nov 20, 2017

Uh oh!

zSys-bot commented Nov 20, 2017

Uh oh!

zSys-bot commented Nov 20, 2017

Uh oh!

dashpole commented Nov 20, 2017

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

dashpole commented Oct 23, 2017 •

edited

Loading