Skip to content

Commit 929a77b

Browse files
boaz0boaz0
committed
Hide the mutex in formatter.ContainerStats
The formatter.ContainerStats struct exposes its Mutex. This is a bad design and should be fixed. To fix that, I separated the statistics attributes from ContainerStats to StatsEntry and hid the mutex. Notice that the mutex protects both the `err` field and the statistics attributes. Then, implemented SetStatistics, SetError, GetStatistics and GetError to avoid races. Moreover, to make this less granular, I decided to replace the read-write mutex with the regular mutex and to pass a StatsEntry slice to formatter.ContainerStatsWrite Signed-off-by: Boaz Shuster <ripcurld.github@gmail.com>
1 parent c5f4a1a commit 929a77b

File tree

3 files changed

+132
-81
lines changed

3 files changed

+132
-81
lines changed

‎cli/command/container/stats.go‎

Lines changed: 13 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -166,11 +166,10 @@ func runStats(dockerCli *command.DockerCli, opts *statsOptions) error {
166166
var errs []string
167167
cStats.mu.Lock()
168168
for _, c := range cStats.cs {
169-
c.Mu.Lock()
170-
if c.Err != nil {
171-
errs = append(errs, fmt.Sprintf("%s: %v", c.Name, c.Err))
169+
cErr := c.GetError()
170+
if cErr != nil {
171+
errs = append(errs, fmt.Sprintf("%s: %v", c.Name, cErr))
172172
}
173-
c.Mu.Unlock()
174173
}
175174
cStats.mu.Unlock()
176175
if len(errs) > 0 {
@@ -189,7 +188,7 @@ func runStats(dockerCli *command.DockerCli, opts *statsOptions) error {
189188
Format: formatter.NewStatsFormat(f, daemonOSType),
190189
}
191190

192-
cleanHeader := func() {
191+
cleanScreen := func() {
193192
if !opts.noStream {
194193
fmt.Fprint(dockerCli.Out(), "\033[2J")
195194
fmt.Fprint(dockerCli.Out(), "\033[H")
@@ -198,14 +197,17 @@ func runStats(dockerCli *command.DockerCli, opts *statsOptions) error {
198197

199198
var err error
200199
for range time.Tick(500 * time.Millisecond) {
201-
cleanHeader()
202-
cStats.mu.RLock()
203-
csLen := len(cStats.cs)
204-
if err = formatter.ContainerStatsWrite(statsCtx, cStats.cs); err != nil {
200+
cleanScreen()
201+
ccstats := []formatter.StatsEntry{}
202+
cStats.mu.Lock()
203+
for _, c := range cStats.cs {
204+
ccstats = append(ccstats, c.GetStatistics())
205+
}
206+
cStats.mu.Unlock()
207+
if err = formatter.ContainerStatsWrite(statsCtx, ccstats); err != nil {
205208
break
206209
}
207-
cStats.mu.RUnlock()
208-
if csLen == 0 && !showAll {
210+
if len(cStats.cs) == 0 && !showAll {
209211
break
210212
}
211213
if opts.noStream {

‎cli/command/container/stats_helpers.go‎

Lines changed: 23 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ import (
1717

1818
type stats struct {
1919
ostype string
20-
mu sync.RWMutex
20+
mu sync.Mutex
2121
cs []*formatter.ContainerStats
2222
}
2323

@@ -72,9 +72,7 @@ func collect(s *formatter.ContainerStats, ctx context.Context, cli client.APICli
7272

7373
response, err := cli.ContainerStats(ctx, s.Name, streamStats)
7474
if err != nil {
75-
s.Mu.Lock()
76-
s.Err = err
77-
s.Mu.Unlock()
75+
s.SetError(err)
7876
return
7977
}
8078
defer response.Body.Close()
@@ -88,6 +86,9 @@ func collect(s *formatter.ContainerStats, ctx context.Context, cli client.APICli
8886
cpuPercent = 0.0
8987
blkRead, blkWrite uint64 // Only used on Linux
9088
mem = 0.0
89+
memLimit = 0.0
90+
memPerc = 0.0
91+
pidsStatsCurrent uint64
9192
)
9293

9394
if err := dec.Decode(&v); err != nil {
@@ -113,26 +114,27 @@ func collect(s *formatter.ContainerStats, ctx context.Context, cli client.APICli
113114
cpuPercent = calculateCPUPercentUnix(previousCPU, previousSystem, v)
114115
blkRead, blkWrite = calculateBlockIO(v.BlkioStats)
115116
mem = float64(v.MemoryStats.Usage)
116-
117+
memLimit = float64(v.MemoryStats.Limit)
118+
memPerc = memPercent
119+
pidsStatsCurrent = v.PidsStats.Current
117120
} else {
118121
cpuPercent = calculateCPUPercentWindows(v)
119122
blkRead = v.StorageStats.ReadSizeBytes
120123
blkWrite = v.StorageStats.WriteSizeBytes
121124
mem = float64(v.MemoryStats.PrivateWorkingSet)
122125
}
123-
124-
s.Mu.Lock()
125-
s.CPUPercentage = cpuPercent
126-
s.Memory = mem
127-
s.NetworkRx, s.NetworkTx = calculateNetwork(v.Networks)
128-
s.BlockRead = float64(blkRead)
129-
s.BlockWrite = float64(blkWrite)
130-
if daemonOSType != "windows" {
131-
s.MemoryLimit = float64(v.MemoryStats.Limit)
132-
s.MemoryPercentage = memPercent
133-
s.PidsCurrent = v.PidsStats.Current
134-
}
135-
s.Mu.Unlock()
126+
netRx, netTx := calculateNetwork(v.Networks)
127+
s.SetStatistics(formatter.StatsEntry{
128+
CPUPercentage: cpuPercent,
129+
Memory: mem,
130+
MemoryPercentage: memPerc,
131+
MemoryLimit: memLimit,
132+
NetworkRx: netRx,
133+
NetworkTx: netTx,
134+
BlockRead: float64(blkRead),
135+
BlockWrite: float64(blkWrite),
136+
PidsCurrent: pidsStatsCurrent,
137+
})
136138
u <- nil
137139
if !streamStats {
138140
return
@@ -144,31 +146,18 @@ func collect(s *formatter.ContainerStats, ctx context.Context, cli client.APICli
144146
case <-time.After(2 * time.Second):
145147
// zero out the values if we have not received an update within
146148
// the specified duration.
147-
s.Mu.Lock()
148-
s.CPUPercentage = 0
149-
s.Memory = 0
150-
s.MemoryPercentage = 0
151-
s.MemoryLimit = 0
152-
s.NetworkRx = 0
153-
s.NetworkTx = 0
154-
s.BlockRead = 0
155-
s.BlockWrite = 0
156-
s.PidsCurrent = 0
157-
s.Err = errors.New("timeout waiting for stats")
158-
s.Mu.Unlock()
149+
s.SetErrorAndReset(errors.New("timeout waiting for stats"))
159150
// if this is the first stat you get, release WaitGroup
160151
if !getFirst {
161152
getFirst = true
162153
waitFirst.Done()
163154
}
164155
case err := <-u:
165156
if err != nil {
166-
s.Mu.Lock()
167-
s.Err = err
168-
s.Mu.Unlock()
157+
s.SetError(err)
169158
continue
170159
}
171-
s.Err = nil
160+
s.SetError(nil)
172161
// if this is the first stat you get, release WaitGroup
173162
if !getFirst {
174163
getFirst = true

‎cli/command/formatter/stats.go‎

Lines changed: 96 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -4,27 +4,27 @@ import (
44
"fmt"
55
"sync"
66

7-
"github.com/docker/go-units"
7+
units "src/github.com/docker/go-units"
88
)
99

1010
const (
11-
defaultStatsTableFormat = "table {{.Container}}\t{{.CPUPrec}}\t{{.MemUsage}}\t{{.MemPrec}}\t{{.NetIO}}\t{{.BlockIO}}\t{{.PIDs}}"
12-
winDefaultStatsTableFormat = "table {{.Container}}\t{{.CPUPrec}}\t{{{.MemUsage}}\t{.NetIO}}\t{{.BlockIO}}"
11+
winOSType = "windows"
12+
defaultStatsTableFormat = "table {{.Container}}\t{{.CPUPerc}}\t{{.MemUsage}}\t{{.MemPerc}}\t{{.NetIO}}\t{{.BlockIO}}\t{{.PIDs}}"
13+
winDefaultStatsTableFormat = "table {{.Container}}\t{{.CPUPerc}}\t{{{.MemUsage}}\t{.NetIO}}\t{{.BlockIO}}"
1314
emptyStatsTableFormat = "Waiting for statistics..."
1415

1516
containerHeader = "CONTAINER"
16-
cpuPrecHeader = "CPU %"
17+
cpuPercHeader = "CPU %"
1718
netIOHeader = "NET I/O"
1819
blockIOHeader = "BLOCK I/O"
19-
winMemPrecHeader = "PRIV WORKING SET" // Used only on Window
20-
memPrecHeader = "MEM %" // Used only on Linux
20+
winMemPercHeader = "PRIV WORKING SET" // Used only on Window
21+
memPercHeader = "MEM %" // Used only on Linux
2122
memUseHeader = "MEM USAGE / LIMIT" // Used only on Linux
2223
pidsHeader = "PIDS" // Used only on Linux
2324
)
2425

25-
// ContainerStatsAttrs represents the statistics data collected from a container.
26-
type ContainerStatsAttrs struct {
27-
Windows bool
26+
// StatsEntry represents represents the statistics data collected from a container
27+
type StatsEntry struct {
2828
Name string
2929
CPUPercentage float64
3030
Memory float64 // On Windows this is the private working set
@@ -35,19 +35,73 @@ type ContainerStatsAttrs struct {
3535
BlockRead float64
3636
BlockWrite float64
3737
PidsCurrent uint64 // Not used on Windows
38+
IsInvalid bool
39+
OSType string
3840
}
3941

40-
// ContainerStats represents the containers statistics data.
42+
// ContainerStats represents an entity to store containers statistics synchronously
4143
type ContainerStats struct {
42-
Mu sync.RWMutex
43-
ContainerStatsAttrs
44-
Err error
44+
mutex sync.Mutex
45+
StatsEntry
46+
err error
47+
}
48+
49+
// GetError returns the container statistics error.
50+
// This is used to determine whether the statistics are valid or not
51+
func (cs *ContainerStats) GetError() error {
52+
cs.mutex.Lock()
53+
defer cs.mutex.Unlock()
54+
return cs.err
55+
}
56+
57+
// SetErrorAndReset zeroes all the container statistics and store the error.
58+
// It is used when receiving time out error during statistics collecting to reduce lock overhead
59+
func (cs *ContainerStats) SetErrorAndReset(err error) {
60+
cs.mutex.Lock()
61+
defer cs.mutex.Unlock()
62+
cs.CPUPercentage = 0
63+
cs.Memory = 0
64+
cs.MemoryPercentage = 0
65+
cs.MemoryLimit = 0
66+
cs.NetworkRx = 0
67+
cs.NetworkTx = 0
68+
cs.BlockRead = 0
69+
cs.BlockWrite = 0
70+
cs.PidsCurrent = 0
71+
cs.err = err
72+
cs.IsInvalid = true
73+
}
74+
75+
// SetError sets container statistics error
76+
func (cs *ContainerStats) SetError(err error) {
77+
cs.mutex.Lock()
78+
defer cs.mutex.Unlock()
79+
cs.err = err
80+
if err != nil {
81+
cs.IsInvalid = true
82+
}
83+
}
84+
85+
// SetStatistics set the container statistics
86+
func (cs *ContainerStats) SetStatistics(s StatsEntry) {
87+
cs.mutex.Lock()
88+
defer cs.mutex.Unlock()
89+
s.Name = cs.Name
90+
s.OSType = cs.OSType
91+
cs.StatsEntry = s
92+
}
93+
94+
// GetStatistics returns container statistics with other meta data such as the container name
95+
func (cs *ContainerStats) GetStatistics() StatsEntry {
96+
cs.mutex.Lock()
97+
defer cs.mutex.Unlock()
98+
return cs.StatsEntry
4599
}
46100

47101
// NewStatsFormat returns a format for rendering an CStatsContext
48102
func NewStatsFormat(source, osType string) Format {
49103
if source == TableFormatKey {
50-
if osType == "windows" {
104+
if osType == winOSType {
51105
return Format(winDefaultStatsTableFormat)
52106
}
53107
return Format(defaultStatsTableFormat)
@@ -58,22 +112,16 @@ func NewStatsFormat(source, osType string) Format {
58112
// NewContainerStats returns a new ContainerStats entity and sets in it the given name
59113
func NewContainerStats(name, osType string) *ContainerStats {
60114
return &ContainerStats{
61-
ContainerStatsAttrs: ContainerStatsAttrs{
62-
Name: name,
63-
Windows: (osType == "windows"),
64-
},
115+
StatsEntry: StatsEntry{Name: name, OSType: osType},
65116
}
66117
}
67118

68119
// ContainerStatsWrite renders the context for a list of containers statistics
69-
func ContainerStatsWrite(ctx Context, containerStats []*ContainerStats) error {
120+
func ContainerStatsWrite(ctx Context, containerStats []StatsEntry) error {
70121
render := func(format func(subContext subContext) error) error {
71122
for _, cstats := range containerStats {
72-
cstats.Mu.RLock()
73-
cstatsAttrs := cstats.ContainerStatsAttrs
74-
cstats.Mu.RUnlock()
75123
containerStatsCtx := &containerStatsContext{
76-
s: cstatsAttrs,
124+
s: cstats,
77125
}
78126
if err := format(containerStatsCtx); err != nil {
79127
return err
@@ -86,50 +134,62 @@ func ContainerStatsWrite(ctx Context, containerStats []*ContainerStats) error {
86134

87135
type containerStatsContext struct {
88136
HeaderContext
89-
s ContainerStatsAttrs
137+
s StatsEntry
90138
}
91139

92140
func (c *containerStatsContext) Container() string {
93141
c.AddHeader(containerHeader)
94142
return c.s.Name
95143
}
96144

97-
func (c *containerStatsContext) CPUPrec() string {
98-
c.AddHeader(cpuPrecHeader)
145+
func (c *containerStatsContext) CPUPerc() string {
146+
c.AddHeader(cpuPercHeader)
147+
if c.s.IsInvalid {
148+
return fmt.Sprintf("--")
149+
}
99150
return fmt.Sprintf("%.2f%%", c.s.CPUPercentage)
100151
}
101152

102153
func (c *containerStatsContext) MemUsage() string {
103154
c.AddHeader(memUseHeader)
104-
if !c.s.Windows {
105-
return fmt.Sprintf("%s / %s", units.BytesSize(c.s.Memory), units.BytesSize(c.s.MemoryLimit))
155+
if c.s.IsInvalid || c.s.OSType == winOSType {
156+
return fmt.Sprintf("-- / --")
106157
}
107-
return fmt.Sprintf("-- / --")
158+
return fmt.Sprintf("%s / %s", units.BytesSize(c.s.Memory), units.BytesSize(c.s.MemoryLimit))
108159
}
109160

110-
func (c *containerStatsContext) MemPrec() string {
111-
header := memPrecHeader
112-
if c.s.Windows {
113-
header = winMemPrecHeader
161+
func (c *containerStatsContext) MemPerc() string {
162+
header := memPercHeader
163+
if c.s.OSType == winOSType {
164+
header = winMemPercHeader
114165
}
115166
c.AddHeader(header)
167+
if c.s.IsInvalid {
168+
return fmt.Sprintf("--")
169+
}
116170
return fmt.Sprintf("%.2f%%", c.s.MemoryPercentage)
117171
}
118172

119173
func (c *containerStatsContext) NetIO() string {
120174
c.AddHeader(netIOHeader)
175+
if c.s.IsInvalid {
176+
return fmt.Sprintf("--")
177+
}
121178
return fmt.Sprintf("%s / %s", units.HumanSizeWithPrecision(c.s.NetworkRx, 3), units.HumanSizeWithPrecision(c.s.NetworkTx, 3))
122179
}
123180

124181
func (c *containerStatsContext) BlockIO() string {
125182
c.AddHeader(blockIOHeader)
183+
if c.s.IsInvalid {
184+
return fmt.Sprintf("--")
185+
}
126186
return fmt.Sprintf("%s / %s", units.HumanSizeWithPrecision(c.s.BlockRead, 3), units.HumanSizeWithPrecision(c.s.BlockWrite, 3))
127187
}
128188

129189
func (c *containerStatsContext) PIDs() string {
130190
c.AddHeader(pidsHeader)
131-
if !c.s.Windows {
132-
return fmt.Sprintf("%d", c.s.PidsCurrent)
191+
if c.s.IsInvalid || c.s.OSType == winOSType {
192+
return fmt.Sprintf("--")
133193
}
134-
return fmt.Sprintf("-")
194+
return fmt.Sprintf("%d", c.s.PidsCurrent)
135195
}

0 commit comments

Comments
 (0)