Merge pull request #65 from monnand/storage-unit-tests-percentiles

More unit tests for storage drivers and a bug fix for memory storage

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))
+	// If the old slice is enough to hold the new data, then don't allocate
-	for i, perCpu := range stats.Cpu.Usage.PerCpu {
+	// a new slice.
-		self.prevStats.Cpu.Usage.PerCpu[i] = perCpu
+	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
 }
 
@@ -75,9 +82,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
 }
@@ -88,18 +95,25 @@ 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)
+
+	// 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 := 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
 }
@@ -162,23 +176,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 {
+	func() {
-		cstore = newContainerStore(ref, self.maxNumSamples, self.maxNumStats)
+		self.lock.Lock()
-		self.containerStorageMap[ref.Name] = cstore
+		defer self.lock.Unlock()
-	}
+		if cstore, ok = self.containerStorageMap[ref.Name]; !ok {
-	self.lock.Unlock()
+			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 {
+	err := func() error {
-		return nil, fmt.Errorf("unable to find data for container %v", name)
+		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)
 }
@@ -186,11 +211,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()
+	err := func() error {
-	if cstore, ok = self.containerStorageMap[name]; !ok {
+		self.lock.RLock()
-		return nil, fmt.Errorf("unable to find data for container %v", name)
+		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)
 }
@@ -198,11 +229,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()
+	err := func() error {
-	if cstore, ok = self.containerStorageMap[name]; !ok {
+		self.lock.RLock()
-		return nil, fmt.Errorf("unable to find data for container %v", name)
+		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)
 }

storage/memory/memory_test.go

@@ -44,6 +44,32 @@ 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)
+}
+
+func TestRetrievePartialRecentStats(t *testing.T) {
+	runStorageTest(test.StorageDriverTestRetrievePartialRecentStats, 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)
+}

storage/test/storagetests.go

@@ -16,6 +16,7 @@ package test
 
 import (
 	"math/rand"
+	"reflect"
 	"testing"
 	"time"
 
@@ -43,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]
 
@@ -51,10 +53,39 @@ 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 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)
 
@@ -73,28 +104,34 @@ 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 something 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)
 	if err != nil {
 		t.Errorf("unable to sample stats: %v", err)
 	}
-	for _, sample := range samples {
+	samplesInTrace(samples, cpuTrace, memTrace, samplePeriod, t)
-		if sample.Duration != samplePeriod {
+
-			t.Errorf("sample duration is %v, not %v", sample.Duration, samplePeriod)
+	samples, err = driver.Samples(ref.Name, -1)
-		}
+	if err != nil {
-		cpuUsage := sample.Cpu.Usage
+		t.Errorf("unable to sample stats: %v", err)
-		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, 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) {
@@ -114,7 +151,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 something 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})
@@ -168,3 +214,176 @@ func StorageDriverTestPercentilesWithoutSample(driver storage.StorageDriver, t *
 		t.Errorf("There should be no percentiles")
 	}
 }
+
+func StorageDriverTestPercentiles(driver storage.StorageDriver, t *testing.T) {
+	defer driver.Close()
+	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 _, 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 _, 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)
+			}
+		}
+	}
+}
+
+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))
+	}
+
+	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)
+		}
+	}
+}
+
+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)
+	}
+	if len(recentStats) > N {
+		t.Fatalf("returned %v stats, not 100.", len(recentStats))
+	}
+
+	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)
+		}
+	}
+}
+
+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)
+		}
+	}
+}