From fd1a7dfd477b9e0427347f0d268a4a073b4fb687 Mon Sep 17 00:00:00 2001 From: Stephen J Day Date: Wed, 7 Mar 2018 13:20:21 -0800 Subject: [PATCH 1/2] daemon/stats: more resilient cpu sampling To avoid noise in sampling CPU usage metrics, we now sample the system usage closer to the actual response from the underlying runtime. Because the response from the runtime may be delayed, this makes the sampling more resilient in loaded conditions. In addition to this, we also replace the tick with a sleep to avoid situations where ticks can backup under loaded conditions. The trade off here is slightly more load reading the system CPU usage for each container. There may be an optimization required for large amounts of containers but the cost is on the order of 15 ms per 1000 containers. If this becomes a problem, we can time slot the sampling, but the complexity may not be worth it unless we can test further. Unfortunately, there aren't really any good tests for this condition. Triggering this behavior is highly system dependent. As a matter of course, we should qualify the fix with the users that are affected. Signed-off-by: Stephen J Day (cherry picked from commit fd0e24b7189374e0fe7c55b6d26ee916d3ee1655) Signed-off-by: Sebastiaan van Stijn --- components/engine/daemon/stats/collector.go | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/components/engine/daemon/stats/collector.go b/components/engine/daemon/stats/collector.go index 39c76128b0..24d41a3d2e 100644 --- a/components/engine/daemon/stats/collector.go +++ b/components/engine/daemon/stats/collector.go @@ -57,7 +57,7 @@ func (s *Collector) Run() { // it will grow enough in first iteration var pairs []publishersPair - for range time.Tick(s.interval) { + for { // it does not make sense in the first iteration, // but saves allocations in further iterations pairs = pairs[:0] @@ -72,12 +72,6 @@ func (s *Collector) Run() { continue } - systemUsage, err := s.getSystemCPUUsage() - if err != nil { - logrus.Errorf("collecting system cpu usage: %v", err) - continue - } - onlineCPUs, err := s.getNumberOnlineCPUs() if err != nil { logrus.Errorf("collecting system online cpu count: %v", err) @@ -89,6 +83,14 @@ func (s *Collector) Run() { switch err.(type) { case nil: + // Sample system CPU usage close to container usage to avoid + // noise in metric calculations. + systemUsage, err := s.getSystemCPUUsage() + if err != nil { + logrus.WithError(err).WithField("container_id", pair.container.ID).Errorf("collecting system cpu usage") + continue + } + // FIXME: move to containerd on Linux (not Windows) stats.CPUStats.SystemUsage = systemUsage stats.CPUStats.OnlineCPUs = onlineCPUs @@ -106,6 +108,8 @@ func (s *Collector) Run() { logrus.Errorf("collecting stats for %s: %v", pair.container.ID, err) } } + + time.Sleep(s.interval) } } From 912261ed440634719457321476cfdec9eea970c9 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Thu, 15 Mar 2018 17:56:15 +0100 Subject: [PATCH 2/2] Fix stats collector spinning CPU if no stats are collected Commit fd0e24b7189374e0fe7c55b6d26ee916d3ee1655 changed the stats collection loop to use a `sleep()` instead of `time.Tick()` in the for-loop. This change caused a regression in situations where no stats are being collected, or an error is hit in the loop (in which case the loop would `continue`, and the `sleep()` is not hit). This patch puts the sleep at the start of the loop to guarantee it's always hit. This will delay the sampling, which is similar to the behavior before fd0e24b7189374e0fe7c55b6d26ee916d3ee1655. Signed-off-by: Sebastiaan van Stijn (cherry picked from commit 481b8e54b45955e40075f49a9af321afce439320) Signed-off-by: Sebastiaan van Stijn --- components/engine/daemon/stats/collector.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/components/engine/daemon/stats/collector.go b/components/engine/daemon/stats/collector.go index 24d41a3d2e..6aa030c57a 100644 --- a/components/engine/daemon/stats/collector.go +++ b/components/engine/daemon/stats/collector.go @@ -58,6 +58,10 @@ func (s *Collector) Run() { var pairs []publishersPair for { + // Put sleep at the start so that it will always be hit, + // preventing a tight loop if no stats are collected. + time.Sleep(s.interval) + // it does not make sense in the first iteration, // but saves allocations in further iterations pairs = pairs[:0] @@ -108,8 +112,6 @@ func (s *Collector) Run() { logrus.Errorf("collecting stats for %s: %v", pair.container.ID, err) } } - - time.Sleep(s.interval) } }