From e412c042d3af0a10fc7405d9f0b4627894182c67 Mon Sep 17 00:00:00 2001 From: Nan Deng Date: Tue, 17 Jun 2014 20:33:13 -0700 Subject: [PATCH 01/15] storage unit test percentiles --- storage/memory/memory_test.go | 8 ++++++- storage/test/storagetests.go | 41 +++++++++++++++++++++++++++++++++++ 2 files changed, 48 insertions(+), 1 deletion(-) diff --git a/storage/memory/memory_test.go b/storage/memory/memory_test.go index 98f9a6a8..ab75ad38 100644 --- a/storage/memory/memory_test.go +++ b/storage/memory/memory_test.go @@ -44,6 +44,12 @@ func TestSamplesWithoutSample(t *testing.T) { runStorageTest(test.StorageDriverTestSamplesWithoutSample, t) } -func TestPercentilessWithoutSample(t *testing.T) { +func TestPercentilesWithoutSample(t *testing.T) { runStorageTest(test.StorageDriverTestPercentilesWithoutSample, t) } + +func TestPercentiles(t *testing.T) { + N := 100 + driver := New(N, N) + test.StorageDriverTestPercentiles(driver, t) +} diff --git a/storage/test/storagetests.go b/storage/test/storagetests.go index e0f27f76..6253c482 100644 --- a/storage/test/storagetests.go +++ b/storage/test/storagetests.go @@ -168,3 +168,44 @@ func StorageDriverTestPercentilesWithoutSample(driver storage.StorageDriver, t * t.Errorf("There should be no percentiles") } } + +// The driver must be able to hold more than 100 samples +func StorageDriverTestPercentiles(driver storage.StorageDriver, t *testing.T) { + N := 100 + cpuTrace := make([]uint64, N) + memTrace := make([]uint64, N) + for i := 1; i < N+1; i++ { + cpuTrace[i-1] = uint64(i) + memTrace[i-1] = uint64(i) + } + + trace := buildTrace(cpuTrace, memTrace, 1*time.Second) + + ref := info.ContainerReference{ + Name: "container", + } + for _, stats := range trace { + driver.AddStats(ref, stats) + } + percentages := []int{ + 80, + 90, + 50, + } + percentiles, err := driver.Percentiles(ref.Name, percentages, percentages) + if err != nil { + t.Fatal(err) + } + for _, s := range percentiles.CpuUsagePercentiles { + // The value is out of the range of tolerance.. + if s.Value > uint64(s.Percentage+1) || s.Value < uint64(s.Percentage-1) { + t.Errorf("%v percentile data should be %v, but got %v", s.Percentage, s.Percentage, s.Value) + } + } + for _, s := range percentiles.MemoryUsagePercentiles { + // The value is out of the range of tolerance.. + if s.Value > uint64(s.Percentage+1) || s.Value < uint64(s.Percentage-1) { + t.Errorf("%v percentile data should be %v, but got %v", s.Percentage, s.Percentage, s.Value) + } + } +} From cf8257cd70f68665c32a7b236a6d4829d90afa3d Mon Sep 17 00:00:00 2001 From: Nan Deng Date: Tue, 17 Jun 2014 20:54:48 -0700 Subject: [PATCH 02/15] fix a bug in memory storage and add a new unit test --- storage/memory/memory.go | 4 ++-- storage/memory/memory_test.go | 4 ++++ storage/test/storagetests.go | 45 +++++++++++++++++++++++++++++++++++ 3 files changed, 51 insertions(+), 2 deletions(-) diff --git a/storage/memory/memory.go b/storage/memory/memory.go index dd353cc4..500ec3e0 100644 --- a/storage/memory/memory.go +++ b/storage/memory/memory.go @@ -75,9 +75,9 @@ func (self *containerStorage) AddStats(stats *info.ContainerStats) error { } } if self.recentStats.Len() >= self.maxNumStats { - self.recentStats.Remove(self.recentStats.Front()) + self.recentStats.Remove(self.recentStats.Back()) } - self.recentStats.PushBack(stats) + self.recentStats.PushFront(stats) self.updatePrevStats(stats) return nil } diff --git a/storage/memory/memory_test.go b/storage/memory/memory_test.go index ab75ad38..32809ce0 100644 --- a/storage/memory/memory_test.go +++ b/storage/memory/memory_test.go @@ -53,3 +53,7 @@ func TestPercentiles(t *testing.T) { driver := New(N, N) test.StorageDriverTestPercentiles(driver, t) } + +func TestRetrievePartialRecentStats(t *testing.T) { + runStorageTest(test.StorageDriverTestRetrievePartialRecentStats, t) +} diff --git a/storage/test/storagetests.go b/storage/test/storagetests.go index 6253c482..60985e67 100644 --- a/storage/test/storagetests.go +++ b/storage/test/storagetests.go @@ -16,6 +16,7 @@ package test import ( "math/rand" + "reflect" "testing" "time" @@ -171,6 +172,7 @@ func StorageDriverTestPercentilesWithoutSample(driver storage.StorageDriver, t * // The driver must be able to hold more than 100 samples func StorageDriverTestPercentiles(driver storage.StorageDriver, t *testing.T) { + defer driver.Close() N := 100 cpuTrace := make([]uint64, N) memTrace := make([]uint64, N) @@ -209,3 +211,46 @@ func StorageDriverTestPercentiles(driver storage.StorageDriver, t *testing.T) { } } } + +// The driver must be albe to hold more than 10 stats +func StorageDriverTestRetrievePartialRecentStats(driver storage.StorageDriver, t *testing.T) { + defer driver.Close() + N := 100 + memTrace := make([]uint64, N) + cpuTrace := make([]uint64, N) + for i := 0; i < N; i++ { + memTrace[i] = uint64(i + 1) + cpuTrace[i] = uint64(1) + } + + ref := info.ContainerReference{ + Name: "container", + } + + trace := buildTrace(cpuTrace, memTrace, 1*time.Second) + + for _, stats := range trace { + driver.AddStats(ref, stats) + } + + recentStats, err := driver.RecentStats(ref.Name, 10) + if err != nil { + t.Fatal(err) + } + + if len(recentStats) != 10 { + t.Fatalf("returned %v stats, not 10.", len(recentStats)) + } + + for _, r := range recentStats { + found := false + for _, s := range trace[len(trace)-10:] { + if reflect.DeepEqual(s, r) { + found = true + } + } + if !found { + t.Errorf("returned unexpected stats: %+v; %v", r, r.Memory.Usage) + } + } +} From 20b13c4cfc4bcc2f23ce8532de312fab8fd36fdc Mon Sep 17 00:00:00 2001 From: Nan Deng Date: Tue, 17 Jun 2014 20:59:33 -0700 Subject: [PATCH 03/15] add unit test --- storage/memory/memory_test.go | 4 ++++ storage/test/storagetests.go | 39 +++++++++++++++++++++++++++++++++++ 2 files changed, 43 insertions(+) diff --git a/storage/memory/memory_test.go b/storage/memory/memory_test.go index 32809ce0..6ecca408 100644 --- a/storage/memory/memory_test.go +++ b/storage/memory/memory_test.go @@ -57,3 +57,7 @@ func TestPercentiles(t *testing.T) { func TestRetrievePartialRecentStats(t *testing.T) { runStorageTest(test.StorageDriverTestRetrievePartialRecentStats, t) } + +func TestRetrieveAllRecentStats(t *testing.T) { + runStorageTest(test.StorageDriverTestRetrieveAllRecentStats, t) +} diff --git a/storage/test/storagetests.go b/storage/test/storagetests.go index 60985e67..27505f57 100644 --- a/storage/test/storagetests.go +++ b/storage/test/storagetests.go @@ -254,3 +254,42 @@ func StorageDriverTestRetrievePartialRecentStats(driver storage.StorageDriver, t } } } + +// The driver must be albe to hold more than 10 stats +func StorageDriverTestRetrieveAllRecentStats(driver storage.StorageDriver, t *testing.T) { + defer driver.Close() + N := 100 + memTrace := make([]uint64, N) + cpuTrace := make([]uint64, N) + for i := 0; i < N; i++ { + memTrace[i] = uint64(i + 1) + cpuTrace[i] = uint64(1) + } + + ref := info.ContainerReference{ + Name: "container", + } + + trace := buildTrace(cpuTrace, memTrace, 1*time.Second) + + for _, stats := range trace { + driver.AddStats(ref, stats) + } + + recentStats, err := driver.RecentStats(ref.Name, -1) + if err != nil { + t.Fatal(err) + } + + for _, r := range recentStats { + found := false + for _, s := range trace { + if reflect.DeepEqual(s, r) { + found = true + } + } + if !found { + t.Errorf("returned unexpected stats: %+v; %v", r, r.Memory.Usage) + } + } +} From 9c54221a54384b0cc73bb63734a164d8567c1721 Mon Sep 17 00:00:00 2001 From: Nan Deng Date: Tue, 17 Jun 2014 21:07:40 -0700 Subject: [PATCH 04/15] defines the order of the returned recent stats --- storage/test/storagetests.go | 32 ++++++++++++++------------------ 1 file changed, 14 insertions(+), 18 deletions(-) diff --git a/storage/test/storagetests.go b/storage/test/storagetests.go index 27505f57..1b23fd24 100644 --- a/storage/test/storagetests.go +++ b/storage/test/storagetests.go @@ -242,15 +242,14 @@ func StorageDriverTestRetrievePartialRecentStats(driver storage.StorageDriver, t t.Fatalf("returned %v stats, not 10.", len(recentStats)) } - for _, r := range recentStats { - found := false - for _, s := range trace[len(trace)-10:] { - if reflect.DeepEqual(s, r) { - found = true - } - } - if !found { - t.Errorf("returned unexpected stats: %+v; %v", r, r.Memory.Usage) + traceIdx := len(trace) - 1 + // Latest stats come first + for i := 0; i < 10; i++ { + r := recentStats[i] + s := trace[traceIdx] + traceIdx-- + if !reflect.DeepEqual(s, r) { + t.Errorf("The %vth item should be %+v with memory usage %v; got item %+v with memory usage %v", i, s, s.Memory.Usage, r, r.Memory.Usage) } } } @@ -281,15 +280,12 @@ func StorageDriverTestRetrieveAllRecentStats(driver storage.StorageDriver, t *te t.Fatal(err) } - for _, r := range recentStats { - found := false - for _, s := range trace { - if reflect.DeepEqual(s, r) { - found = true - } - } - if !found { - t.Errorf("returned unexpected stats: %+v; %v", r, r.Memory.Usage) + traceIdx := len(trace) - 1 + for i, r := range recentStats { + s := trace[traceIdx] + traceIdx-- + if !reflect.DeepEqual(s, r) { + t.Errorf("The %vth item should be %+v with memory usage %v; got item %+v with memory usage %v", i, s, s.Memory.Usage, r, r.Memory.Usage) } } } From 7258101720a646ac70aa94e55900ceb1840e8e9d Mon Sep 17 00:00:00 2001 From: Nan Deng Date: Tue, 17 Jun 2014 21:09:49 -0700 Subject: [PATCH 05/15] add doc --- storage/storage.go | 8 +++++--- storage/test/storagetests.go | 1 - 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/storage/storage.go b/storage/storage.go index 51f0b9c0..cd6af4c8 100644 --- a/storage/storage.go +++ b/storage/storage.go @@ -19,9 +19,11 @@ import "github.com/google/cadvisor/info" type StorageDriver interface { AddStats(ref info.ContainerReference, stats *info.ContainerStats) error - // Read most recent stats. numStats indicates max number of stats - // returned. The returned stats must be consecutive observed stats. If - // numStats < 0, then return all stats stored in the storage. + // Read most recent stats. The returned stats must be reverse + // chronological ordered. i.e. most recent comes first. numStats + // indicates max number of stats returned. The returned stats must be + // consecutive observed stats. If numStats < 0, then return all stats + // stored in the storage. RecentStats(containerName string, numStats int) ([]*info.ContainerStats, error) // Read the specified percentiles of CPU and memory usage of the container. diff --git a/storage/test/storagetests.go b/storage/test/storagetests.go index 1b23fd24..0476791e 100644 --- a/storage/test/storagetests.go +++ b/storage/test/storagetests.go @@ -254,7 +254,6 @@ func StorageDriverTestRetrievePartialRecentStats(driver storage.StorageDriver, t } } -// The driver must be albe to hold more than 10 stats func StorageDriverTestRetrieveAllRecentStats(driver storage.StorageDriver, t *testing.T) { defer driver.Close() N := 100 From 21cf8472a06c6566eb166b0e480ee75010fabd83 Mon Sep 17 00:00:00 2001 From: Nan Deng Date: Wed, 18 Jun 2014 11:17:35 -0700 Subject: [PATCH 06/15] remove constraints --- storage/test/storagetests.go | 65 +++++++++++++++++++++--------------- 1 file changed, 39 insertions(+), 26 deletions(-) diff --git a/storage/test/storagetests.go b/storage/test/storagetests.go index 0476791e..87ae39f7 100644 --- a/storage/test/storagetests.go +++ b/storage/test/storagetests.go @@ -52,7 +52,6 @@ func buildTrace(cpu, mem []uint64, duration time.Duration) []*info.ContainerStat return ret } -// The underlying driver must be able to hold more than 10 samples. func StorageDriverTestSampleCpuUsage(driver storage.StorageDriver, t *testing.T) { defer driver.Close() N := 10 @@ -170,7 +169,6 @@ func StorageDriverTestPercentilesWithoutSample(driver storage.StorageDriver, t * } } -// The driver must be able to hold more than 100 samples func StorageDriverTestPercentiles(driver storage.StorageDriver, t *testing.T) { defer driver.Close() N := 100 @@ -198,21 +196,25 @@ func StorageDriverTestPercentiles(driver storage.StorageDriver, t *testing.T) { if err != nil { t.Fatal(err) } - for _, s := range percentiles.CpuUsagePercentiles { - // The value is out of the range of tolerance.. - if s.Value > uint64(s.Percentage+1) || s.Value < uint64(s.Percentage-1) { - t.Errorf("%v percentile data should be %v, but got %v", s.Percentage, s.Percentage, s.Value) + for _, x := range percentiles.CpuUsagePercentiles { + for _, y := range percentiles.CpuUsagePercentiles { + // lower percentage, smaller value + if x.Percentage < y.Percentage && x.Value > y.Value { + t.Errorf("%v percent is %v; while %v percent is %v", + x.Percentage, x.Value, y.Percentage, y.Value) + } } } - for _, s := range percentiles.MemoryUsagePercentiles { - // The value is out of the range of tolerance.. - if s.Value > uint64(s.Percentage+1) || s.Value < uint64(s.Percentage-1) { - t.Errorf("%v percentile data should be %v, but got %v", s.Percentage, s.Percentage, s.Value) + for _, x := range percentiles.MemoryUsagePercentiles { + for _, y := range percentiles.MemoryUsagePercentiles { + if x.Percentage < y.Percentage && x.Value > y.Value { + t.Errorf("%v percent is %v; while %v percent is %v", + x.Percentage, x.Value, y.Percentage, y.Value) + } } } } -// The driver must be albe to hold more than 10 stats func StorageDriverTestRetrievePartialRecentStats(driver storage.StorageDriver, t *testing.T) { defer driver.Close() N := 100 @@ -238,18 +240,21 @@ func StorageDriverTestRetrievePartialRecentStats(driver storage.StorageDriver, t t.Fatal(err) } - if len(recentStats) != 10 { + if len(recentStats) > 10 { t.Fatalf("returned %v stats, not 10.", len(recentStats)) } - traceIdx := len(trace) - 1 - // Latest stats come first - for i := 0; i < 10; i++ { - r := recentStats[i] - s := trace[traceIdx] - traceIdx-- - if !reflect.DeepEqual(s, r) { - t.Errorf("The %vth item should be %+v with memory usage %v; got item %+v with memory usage %v", i, s, s.Memory.Usage, r, r.Memory.Usage) + actualRecentStats := trace[len(trace)-len(recentStats):] + + for _, r := range recentStats { + found := false + for _, s := range actualRecentStats { + if reflect.DeepEqual(s, r) { + found = true + } + } + if !found { + t.Errorf("unexpected stats %+v with memory usage %v", r, r.Memory.Usage) } } } @@ -278,13 +283,21 @@ func StorageDriverTestRetrieveAllRecentStats(driver storage.StorageDriver, t *te if err != nil { t.Fatal(err) } + if len(recentStats) > N { + t.Fatalf("returned %v stats, not 100.", len(recentStats)) + } - traceIdx := len(trace) - 1 - for i, r := range recentStats { - s := trace[traceIdx] - traceIdx-- - if !reflect.DeepEqual(s, r) { - t.Errorf("The %vth item should be %+v with memory usage %v; got item %+v with memory usage %v", i, s, s.Memory.Usage, r, r.Memory.Usage) + actualRecentStats := trace[len(trace)-len(recentStats):] + + for _, r := range recentStats { + found := false + for _, s := range actualRecentStats { + if reflect.DeepEqual(s, r) { + found = true + } + } + if !found { + t.Errorf("unexpected stats %+v with memory usage %v", r, r.Memory.Usage) } } } From bdc3ae7120b9ecc9315eb46634778977696f9d8e Mon Sep 17 00:00:00 2001 From: Nan Deng Date: Wed, 18 Jun 2014 11:18:21 -0700 Subject: [PATCH 07/15] docs --- storage/storage.go | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/storage/storage.go b/storage/storage.go index cd6af4c8..51f0b9c0 100644 --- a/storage/storage.go +++ b/storage/storage.go @@ -19,11 +19,9 @@ import "github.com/google/cadvisor/info" type StorageDriver interface { AddStats(ref info.ContainerReference, stats *info.ContainerStats) error - // Read most recent stats. The returned stats must be reverse - // chronological ordered. i.e. most recent comes first. numStats - // indicates max number of stats returned. The returned stats must be - // consecutive observed stats. If numStats < 0, then return all stats - // stored in the storage. + // Read most recent stats. numStats indicates max number of stats + // returned. The returned stats must be consecutive observed stats. If + // numStats < 0, then return all stats stored in the storage. RecentStats(containerName string, numStats int) ([]*info.ContainerStats, error) // Read the specified percentiles of CPU and memory usage of the container. From 7f542f29522bb0cb69e79a80e871913230c10276 Mon Sep 17 00:00:00 2001 From: Nan Deng Date: Wed, 18 Jun 2014 11:45:24 -0700 Subject: [PATCH 08/15] do not always allocate a new slice --- storage/memory/memory.go | 13 ++++++++++--- storage/test/storagetests.go | 1 + 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/storage/memory/memory.go b/storage/memory/memory.go index 500ec3e0..4af35c07 100644 --- a/storage/memory/memory.go +++ b/storage/memory/memory.go @@ -49,11 +49,18 @@ func (self *containerStorage) updatePrevStats(stats *info.ContainerStats) { } // make a deep copy. self.prevStats.Timestamp = stats.Timestamp + // copy the slice first. + percpuSlice := self.prevStats.Cpu.Usage.PerCpu *self.prevStats.Cpu = *stats.Cpu - self.prevStats.Cpu.Usage.PerCpu = make([]uint64, len(stats.Cpu.Usage.PerCpu)) - for i, perCpu := range stats.Cpu.Usage.PerCpu { - self.prevStats.Cpu.Usage.PerCpu[i] = perCpu + // If the old slice is enough to hold the new data, then don't allocate + // a new slice. + if len(percpuSlice) != len(stats.Cpu.Usage.PerCpu) { + percpuSlice = make([]uint64, len(stats.Cpu.Usage.PerCpu)) } + for i, perCpu := range stats.Cpu.Usage.PerCpu { + percpuSlice[i] = perCpu + } + self.prevStats.Cpu.Usage.PerCpu = percpuSlice *self.prevStats.Memory = *stats.Memory } diff --git a/storage/test/storagetests.go b/storage/test/storagetests.go index 87ae39f7..b27d9abf 100644 --- a/storage/test/storagetests.go +++ b/storage/test/storagetests.go @@ -44,6 +44,7 @@ func buildTrace(cpu, mem []uint64, duration time.Duration) []*info.ContainerStat stats.Cpu.Usage.Total = cpuTotalUsage stats.Cpu.Usage.User = stats.Cpu.Usage.Total stats.Cpu.Usage.System = 0 + stats.Cpu.Usage.PerCpu = []uint64{cpuUsage} stats.Memory.Usage = mem[i] From 83d12ed5fa11299a76ee090725e41257a5f26eea Mon Sep 17 00:00:00 2001 From: Nan Deng Date: Wed, 18 Jun 2014 12:01:24 -0700 Subject: [PATCH 09/15] check error on AddStats() --- storage/test/storagetests.go | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/storage/test/storagetests.go b/storage/test/storagetests.go index b27d9abf..1bef8610 100644 --- a/storage/test/storagetests.go +++ b/storage/test/storagetests.go @@ -74,7 +74,15 @@ func StorageDriverTestSampleCpuUsage(driver storage.StorageDriver, t *testing.T) trace := buildTrace(cpuTrace, memTrace, samplePeriod) for _, stats := range trace { - driver.AddStats(ref, stats) + err := driver.AddStats(ref, stats) + if err != nil { + t.Fatalf("unable to add stats: %v", err) + } + // set the trace to someting else. The stats stored in the + // storage should not be affected. + stats.Cpu.Usage.Total = 0 + stats.Cpu.Usage.System = 0 + stats.Cpu.Usage.User = 0 } samples, err := driver.Samples(ref.Name, N) @@ -115,7 +123,16 @@ func StorageDriverTestMaxMemoryUsage(driver storage.StorageDriver, t *testing.T) trace := buildTrace(cpuTrace, memTrace, 1*time.Second) for _, stats := range trace { - driver.AddStats(ref, stats) + err := driver.AddStats(ref, stats) + if err != nil { + t.Fatalf("unable to add stats: %v", err) + } + // set the trace to someting else. The stats stored in the + // storage should not be affected. + stats.Cpu.Usage.Total = 0 + stats.Cpu.Usage.System = 0 + stats.Cpu.Usage.User = 0 + stats.Memory.Usage = 0 } percentiles, err := driver.Percentiles(ref.Name, []int{50}, []int{50}) From 05ce0c01e32633fe296a30a6a66cc3be56cd2a5a Mon Sep 17 00:00:00 2001 From: Nan Deng Date: Wed, 18 Jun 2014 13:43:08 -0700 Subject: [PATCH 10/15] fix a deadlock bug along with unit tests --- storage/memory/memory.go | 3 +++ storage/memory/memory_test.go | 12 +++++++++++ storage/test/storagetests.go | 40 +++++++++++++++++++++++++++++++++++ 3 files changed, 55 insertions(+) diff --git a/storage/memory/memory.go b/storage/memory/memory.go index 4af35c07..f86dacb1 100644 --- a/storage/memory/memory.go +++ b/storage/memory/memory.go @@ -183,6 +183,7 @@ func (self *InMemoryStorage) Samples(name string, numSamples int) ([]*info.Conta var ok bool self.lock.RLock() if cstore, ok = self.containerStorageMap[name]; !ok { + self.lock.RUnlock() return nil, fmt.Errorf("unable to find data for container %v", name) } self.lock.RUnlock() @@ -195,6 +196,7 @@ func (self *InMemoryStorage) RecentStats(name string, numStats int) ([]*info.Con var ok bool self.lock.RLock() if cstore, ok = self.containerStorageMap[name]; !ok { + self.lock.RUnlock() return nil, fmt.Errorf("unable to find data for container %v", name) } self.lock.RUnlock() @@ -207,6 +209,7 @@ func (self *InMemoryStorage) Percentiles(name string, cpuPercentiles, memPercent var ok bool self.lock.RLock() if cstore, ok = self.containerStorageMap[name]; !ok { + self.lock.RUnlock() return nil, fmt.Errorf("unable to find data for container %v", name) } self.lock.RUnlock() diff --git a/storage/memory/memory_test.go b/storage/memory/memory_test.go index 6ecca408..394e48f6 100644 --- a/storage/memory/memory_test.go +++ b/storage/memory/memory_test.go @@ -61,3 +61,15 @@ func TestRetrievePartialRecentStats(t *testing.T) { func TestRetrieveAllRecentStats(t *testing.T) { runStorageTest(test.StorageDriverTestRetrieveAllRecentStats, t) } + +func TestNoRecentStats(t *testing.T) { + runStorageTest(test.StorageDriverTestNoRecentStats, t) +} + +func TestNoSamples(t *testing.T) { + runStorageTest(test.StorageDriverTestNoSamples, t) +} + +func TestPercentilesWithoutStats(t *testing.T) { + runStorageTest(test.StorageDriverTestPercentilesWithoutStats, t) +} diff --git a/storage/test/storagetests.go b/storage/test/storagetests.go index 1bef8610..311c4479 100644 --- a/storage/test/storagetests.go +++ b/storage/test/storagetests.go @@ -319,3 +319,43 @@ func StorageDriverTestRetrieveAllRecentStats(driver storage.StorageDriver, t *te } } } + +func StorageDriverTestNoRecentStats(driver storage.StorageDriver, t *testing.T) { + defer driver.Close() + nonExistContainer := "somerandomecontainer" + stats, _ := driver.RecentStats(nonExistContainer, -1) + if len(stats) > 0 { + t.Errorf("RecentStats() returns %v stats on non exist container", len(stats)) + } +} + +func StorageDriverTestNoSamples(driver storage.StorageDriver, t *testing.T) { + defer driver.Close() + nonExistContainer := "somerandomecontainer" + samples, _ := driver.Samples(nonExistContainer, -1) + if len(samples) > 0 { + t.Errorf("Samples() returns %v samples on non exist container", len(samples)) + } +} + +func StorageDriverTestPercentilesWithoutStats(driver storage.StorageDriver, t *testing.T) { + defer driver.Close() + nonExistContainer := "somerandomecontainer" + percentiles, _ := driver.Percentiles(nonExistContainer, []int{50, 80}, []int{50, 80}) + if percentiles == nil { + return + } + if percentiles.MaxMemoryUsage != 0 { + t.Errorf("Percentiles() reports max memory usage > 0 when there's no stats.") + } + for _, p := range percentiles.CpuUsagePercentiles { + if p.Value != 0 { + t.Errorf("Percentiles() reports cpu usage is %v when there's no stats.", p.Value) + } + } + for _, p := range percentiles.MemoryUsagePercentiles { + if p.Value != 0 { + t.Errorf("Percentiles() reports memory usage is %v when there's no stats.", p.Value) + } + } +} From 77dc470f542d4d5382f74e4e956b4f4f9a5f11b0 Mon Sep 17 00:00:00 2001 From: Nan Deng Date: Wed, 18 Jun 2014 13:49:23 -0700 Subject: [PATCH 11/15] increase the coverage --- storage/test/storagetests.go | 58 ++++++++++++++++++++++++++---------- 1 file changed, 43 insertions(+), 15 deletions(-) diff --git a/storage/test/storagetests.go b/storage/test/storagetests.go index 311c4479..cbaff9ac 100644 --- a/storage/test/storagetests.go +++ b/storage/test/storagetests.go @@ -53,9 +53,39 @@ func buildTrace(cpu, mem []uint64, duration time.Duration) []*info.ContainerStat return ret } +func samplesInTrace(samples []*info.ContainerStatsSample, cpuTrace, memTrace []uint64, samplePeriod time.Duration, t *testing.T) { + for _, sample := range samples { + if sample.Duration != samplePeriod { + t.Errorf("sample duration is %v, not %v", sample.Duration, samplePeriod) + } + cpuUsage := sample.Cpu.Usage + memUsage := sample.Memory.Usage + found := false + for _, u := range cpuTrace { + if u == cpuUsage { + found = true + break + } + } + if !found { + t.Errorf("unable to find cpu usage %v", cpuUsage) + } + found = false + for _, u := range memTrace { + if u == memUsage { + found = true + break + } + } + if !found { + t.Errorf("unable to find mem usage %v", memUsage) + } + } +} + func StorageDriverTestSampleCpuUsage(driver storage.StorageDriver, t *testing.T) { defer driver.Close() - N := 10 + N := 100 cpuTrace := make([]uint64, 0, N) memTrace := make([]uint64, 0, N) @@ -89,21 +119,19 @@ func StorageDriverTestSampleCpuUsage(driver storage.StorageDriver, t *testing.T) if err != nil { t.Errorf("unable to sample stats: %v", err) } - for _, sample := range samples { - if sample.Duration != samplePeriod { - t.Errorf("sample duration is %v, not %v", sample.Duration, samplePeriod) - } - cpuUsage := sample.Cpu.Usage - found := false - for _, u := range cpuTrace { - if u == cpuUsage { - found = true - } - } - if !found { - t.Errorf("unable to find cpu usage %v", cpuUsage) - } + samplesInTrace(samples, cpuTrace, memTrace, samplePeriod, t) + + samples, err = driver.Samples(ref.Name, -1) + if err != nil { + t.Errorf("unable to sample stats: %v", err) } + samplesInTrace(samples, cpuTrace, memTrace, samplePeriod, t) + + samples, err = driver.Samples(ref.Name, N-5) + if err != nil { + t.Errorf("unable to sample stats: %v", err) + } + samplesInTrace(samples, cpuTrace, memTrace, samplePeriod, t) } func StorageDriverTestMaxMemoryUsage(driver storage.StorageDriver, t *testing.T) { From 3f6fa477a46a43b3a9c63133f70669d25b5575b2 Mon Sep 17 00:00:00 2001 From: Nan Deng Date: Wed, 18 Jun 2014 16:34:47 -0700 Subject: [PATCH 12/15] defer unlock --- storage/memory/memory.go | 62 ++++++++++++++++++++++++++-------------- 1 file changed, 41 insertions(+), 21 deletions(-) diff --git a/storage/memory/memory.go b/storage/memory/memory.go index f86dacb1..28ed31ae 100644 --- a/storage/memory/memory.go +++ b/storage/memory/memory.go @@ -169,24 +169,34 @@ type InMemoryStorage struct { func (self *InMemoryStorage) AddStats(ref info.ContainerReference, stats *info.ContainerStats) error { var cstore *containerStorage var ok bool - self.lock.Lock() - if cstore, ok = self.containerStorageMap[ref.Name]; !ok { - cstore = newContainerStore(ref, self.maxNumSamples, self.maxNumStats) - self.containerStorageMap[ref.Name] = cstore - } - self.lock.Unlock() + + func() { + self.lock.Lock() + defer self.lock.Unlock() + if cstore, ok = self.containerStorageMap[ref.Name]; !ok { + cstore = newContainerStore(ref, self.maxNumSamples, self.maxNumStats) + self.containerStorageMap[ref.Name] = cstore + } + }() return cstore.AddStats(stats) } func (self *InMemoryStorage) Samples(name string, numSamples int) ([]*info.ContainerStatsSample, error) { var cstore *containerStorage var ok bool - self.lock.RLock() - if cstore, ok = self.containerStorageMap[name]; !ok { - self.lock.RUnlock() - return nil, fmt.Errorf("unable to find data for container %v", name) + + err := func() error { + self.lock.RLock() + defer self.lock.RUnlock() + if cstore, ok = self.containerStorageMap[name]; !ok { + return fmt.Errorf("unable to find data for container %v", name) + } + return nil + }() + + if err != nil { + return nil, err } - self.lock.RUnlock() return cstore.Samples(numSamples) } @@ -194,12 +204,17 @@ func (self *InMemoryStorage) Samples(name string, numSamples int) ([]*info.Conta func (self *InMemoryStorage) RecentStats(name string, numStats int) ([]*info.ContainerStats, error) { var cstore *containerStorage var ok bool - self.lock.RLock() - if cstore, ok = self.containerStorageMap[name]; !ok { - self.lock.RUnlock() - return nil, fmt.Errorf("unable to find data for container %v", name) + err := func() error { + self.lock.RLock() + defer self.lock.RUnlock() + if cstore, ok = self.containerStorageMap[name]; !ok { + return fmt.Errorf("unable to find data for container %v", name) + } + return nil + }() + if err != nil { + return nil, err } - self.lock.RUnlock() return cstore.RecentStats(numStats) } @@ -207,12 +222,17 @@ func (self *InMemoryStorage) RecentStats(name string, numStats int) ([]*info.Con func (self *InMemoryStorage) Percentiles(name string, cpuPercentiles, memPercentiles []int) (*info.ContainerStatsPercentiles, error) { var cstore *containerStorage var ok bool - self.lock.RLock() - if cstore, ok = self.containerStorageMap[name]; !ok { - self.lock.RUnlock() - return nil, fmt.Errorf("unable to find data for container %v", name) + err := func() error { + self.lock.RLock() + defer self.lock.RUnlock() + if cstore, ok = self.containerStorageMap[name]; !ok { + return fmt.Errorf("unable to find data for container %v", name) + } + return nil + }() + if err != nil { + return nil, err } - self.lock.RUnlock() return cstore.Percentiles(cpuPercentiles, memPercentiles) } From 43346e5f2947aa8311bd1c2580374307a26dcb31 Mon Sep 17 00:00:00 2001 From: Nan Deng Date: Thu, 19 Jun 2014 14:19:44 -0700 Subject: [PATCH 13/15] fix typo --- storage/test/storagetests.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/storage/test/storagetests.go b/storage/test/storagetests.go index cbaff9ac..7a45cc8a 100644 --- a/storage/test/storagetests.go +++ b/storage/test/storagetests.go @@ -108,7 +108,7 @@ func StorageDriverTestSampleCpuUsage(driver storage.StorageDriver, t *testing.T) if err != nil { t.Fatalf("unable to add stats: %v", err) } - // set the trace to someting else. The stats stored in the + // set the trace to something else. The stats stored in the // storage should not be affected. stats.Cpu.Usage.Total = 0 stats.Cpu.Usage.System = 0 @@ -155,7 +155,7 @@ func StorageDriverTestMaxMemoryUsage(driver storage.StorageDriver, t *testing.T) if err != nil { t.Fatalf("unable to add stats: %v", err) } - // set the trace to someting else. The stats stored in the + // set the trace to something else. The stats stored in the // storage should not be affected. stats.Cpu.Usage.Total = 0 stats.Cpu.Usage.System = 0 From e9111b5527dbed2ca8207746cbcd757e957648e7 Mon Sep 17 00:00:00 2001 From: Nan Deng Date: Thu, 19 Jun 2014 14:35:55 -0700 Subject: [PATCH 14/15] stats returned in chronological order --- storage/memory/memory.go | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/storage/memory/memory.go b/storage/memory/memory.go index 28ed31ae..539dbcd3 100644 --- a/storage/memory/memory.go +++ b/storage/memory/memory.go @@ -95,18 +95,15 @@ func (self *containerStorage) RecentStats(numStats int) ([]*info.ContainerStats, if self.recentStats.Len() < numStats || numStats < 0 { numStats = self.recentStats.Len() } - ret := make([]*info.ContainerStats, 0, numStats) + ret := make([]*info.ContainerStats, numStats) e := self.recentStats.Front() - for i := 0; i < numStats; i++ { + for i := numStats - 1; i >= 0; i-- { data, ok := e.Value.(*info.ContainerStats) if !ok { return nil, fmt.Errorf("The %vth element is not a ContainerStats", i) } - ret = append(ret, data) + ret[i] = data e = e.Next() - if e == nil { - break - } } return ret, nil } From a0071a7f7dabbbd58f774435654765c3fb524c6c Mon Sep 17 00:00:00 2001 From: Nan Deng Date: Thu, 19 Jun 2014 14:48:21 -0700 Subject: [PATCH 15/15] comment. --- storage/memory/memory.go | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/storage/memory/memory.go b/storage/memory/memory.go index 539dbcd3..63c6d552 100644 --- a/storage/memory/memory.go +++ b/storage/memory/memory.go @@ -95,6 +95,16 @@ func (self *containerStorage) RecentStats(numStats int) ([]*info.ContainerStats, if self.recentStats.Len() < numStats || numStats < 0 { numStats = self.recentStats.Len() } + + // Stats in the recentStats list are stored in reverse chronological + // order, i.e. most recent stats is in the front. + // numStats will always <= recentStats.Len() so that there will be + // always at least numStats available stats to retrieve. We traverse + // the recentStats list from its head and fill the ret slice in + // reverse order so that the returned slice will be in chronological + // order. The order of the returned slice is not specified by the + // StorageDriver interface, so it is not necessary for other storage + // drivers to return the slice in the same order. ret := make([]*info.ContainerStats, numStats) e := self.recentStats.Front() for i := numStats - 1; i >= 0; i-- {