Do not remove containers from stats list on err

Before this patch, containers are silently removed from the stats list
on error. This patch instead will display `--` for all fields for the
container that had the error, allowing it to recover from errors.

Signed-off-by: Brian Goff <cpuguy83@gmail.com>

4 files changed, 26 insertions, 23 deletions

diff --git a/api/client/stats.go b/api/client/stats.go
index b84ac3e00e..943f189936 100644
--- a/api/client/stats.go
+++ b/api/client/stats.go
@@ -10,6 +10,7 @@ import (
 
 	"golang.org/x/net/context"
 
+	"github.com/Sirupsen/logrus"
 	Cli "github.com/docker/docker/cli"
 	"github.com/docker/engine-api/types"
 	"github.com/docker/engine-api/types/events"
@@ -169,20 +170,12 @@ func (cli *DockerCli) CmdStats(args ...string) error {
 
 	for range time.Tick(500 * time.Millisecond) {
 		printHeader()
-		toRemove := []int{}
 		cStats.mu.Lock()
-		for i, s := range cStats.cs {
+		for _, s := range cStats.cs {
 			if err := s.Display(w); err != nil && !*noStream {
-				toRemove = append(toRemove, i)
+				logrus.Debugf("stats: got error for %s: %v", s.Name, err)
 			}
 		}
-		for j := len(toRemove) - 1; j >= 0; j-- {
-			i := toRemove[j]
-			cStats.cs = append(cStats.cs[:i], cStats.cs[i+1:]...)
-		}
-		if len(cStats.cs) == 0 && !showAll {
-			return nil
-		}
 		cStats.mu.Unlock()
 		w.Flush()
 		if *noStream {
diff --git a/api/client/stats_helpers.go b/api/client/stats_helpers.go
index 697ee3e135..f757d9e527 100644
--- a/api/client/stats_helpers.go
+++ b/api/client/stats_helpers.go
@@ -2,12 +2,14 @@ package client
 
 import (
 	"encoding/json"
+	"errors"
 	"fmt"
 	"io"
 	"strings"
 	"sync"
 	"time"
 
+	"github.com/Sirupsen/logrus"
 	"github.com/docker/engine-api/client"
 	"github.com/docker/engine-api/types"
 	"github.com/docker/go-units"
@@ -25,7 +27,7 @@ type containerStats struct {
 	BlockRead        float64
 	BlockWrite       float64
 	PidsCurrent      uint64
-	mu               sync.RWMutex
+	mu               sync.Mutex
 	err              error
 }
 
@@ -62,6 +64,7 @@ func (s *stats) isKnownContainer(cid string) (int, bool) {
 }
 
 func (s *containerStats) Collect(cli client.APIClient, streamStats bool, waitFirst *sync.WaitGroup) {
+	logrus.Debugf("collecting stats for %s", s.Name)
 	var (
 		getFirst       bool
 		previousCPU    uint64
@@ -90,9 +93,11 @@ func (s *containerStats) Collect(cli client.APIClient, streamStats bool, waitFir
 	go func() {
 		for {
 			var v *types.StatsJSON
+
 			if err := dec.Decode(&v); err != nil {
+				dec = json.NewDecoder(io.MultiReader(dec.Buffered(), responseBody))
 				u <- err
-				return
+				continue
 			}
 
 			var memPercent = 0.0
@@ -139,6 +144,7 @@ func (s *containerStats) Collect(cli client.APIClient, streamStats bool, waitFir
 			s.BlockRead = 0
 			s.BlockWrite = 0
 			s.PidsCurrent = 0
+			s.err = errors.New("timeout waiting for stats")
 			s.mu.Unlock()
 			// if this is the first stat you get, release WaitGroup
 			if !getFirst {
@@ -150,8 +156,9 @@ func (s *containerStats) Collect(cli client.APIClient, streamStats bool, waitFir
 				s.mu.Lock()
 				s.err = err
 				s.mu.Unlock()
-				return
+				continue
 			}
+			s.err = nil
 			// if this is the first stat you get, release WaitGroup
 			if !getFirst {
 				getFirst = true
@@ -165,12 +172,20 @@ func (s *containerStats) Collect(cli client.APIClient, streamStats bool, waitFir
 }
 
 func (s *containerStats) Display(w io.Writer) error {
-	s.mu.RLock()
-	defer s.mu.RUnlock()
+	s.mu.Lock()
+	defer s.mu.Unlock()
+	// NOTE: if you change this format, you must also change the err format below!
+	format := "%s\t%.2f%%\t%s / %s\t%.2f%%\t%s / %s\t%s / %s\t%d\n"
 	if s.err != nil {
-		return s.err
+		format = "%s\t%s\t%s / %s\t%s\t%s / %s\t%s / %s\t%s\n"
+		errStr := "--"
+		fmt.Fprintf(w, format,
+			s.Name, errStr, errStr, errStr, errStr, errStr, errStr, errStr, errStr, errStr,
+		)
+		err := s.err
+		return err
 	}
-	fmt.Fprintf(w, "%s\t%.2f%%\t%s / %s\t%.2f%%\t%s / %s\t%s / %s\t%d\n",
+	fmt.Fprintf(w, format,
 		s.Name,
 		s.CPUPercentage,
 		units.BytesSize(s.Memory), units.BytesSize(s.MemoryLimit),
diff --git a/api/client/stats_unit_test.go b/api/client/stats_unit_test.go
index b67a41f03f..9040674240 100644
--- a/api/client/stats_unit_test.go
+++ b/api/client/stats_unit_test.go
@@ -2,7 +2,6 @@ package client
 
 import (
 	"bytes"
-	"sync"
 	"testing"
 
 	"github.com/docker/engine-api/types"
@@ -20,7 +19,6 @@ func TestDisplay(t *testing.T) {
 		BlockRead:        100 * 1024 * 1024,
 		BlockWrite:       800 * 1024 * 1024,
 		PidsCurrent:      1,
-		mu:               sync.RWMutex{},
 	}
 	var b bytes.Buffer
 	if err := c.Display(&b); err != nil {
diff --git a/integration-cli/docker_cli_stats_test.go b/integration-cli/docker_cli_stats_test.go
index e3c7a3e2e7..5cb1a3ea02 100644
--- a/integration-cli/docker_cli_stats_test.go
+++ b/integration-cli/docker_cli_stats_test.go
@@ -120,10 +120,7 @@ func (s *DockerSuite) TestStatsAllNoStream(c *check.C) {
 
 func (s *DockerSuite) TestStatsAllNewContainersAdded(c *check.C) {
 	// Windows does not support stats
-	// TODO: remove SameHostDaemon
-	//	The reason it was added is because, there seems to be some race that makes this test fail
-	//	for remote daemons (namely in the win2lin CI). We highly welcome contributions to fix this.
-	testRequires(c, DaemonIsLinux, SameHostDaemon)
+	testRequires(c, DaemonIsLinux)
 
 	id := make(chan string)
 	addedChan := make(chan struct{})