Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
27930: server: fix crash when getting disk stats on mac r=vilterp a=vilterp

The hardware stats library, gopsutil, uses unguarded shared mutable state on darwin while gathering disk stats. Thus, it crashes during the tests, which use many servers concurrently. This change uses a different library to gather these stats on darwin.

Fixes cockroachdb#27867

Release note: None

Co-authored-by: Pete Vilter <[email protected]>
  • Loading branch information
craig[bot] and Pete Vilter committed Jul 25, 2018
2 parents 65f7aba + aabf1de commit 343e38c
Show file tree
Hide file tree
Showing 6 changed files with 191 additions and 84 deletions.
8 changes: 7 additions & 1 deletion Gopkg.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

47 changes: 47 additions & 0 deletions pkg/server/status/disk_counters.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
// Copyright 2018 The Cockroach Authors.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
// implied. See the License for the specific language governing
// permissions and limitations under the License.

// +build !darwin

package status

import (
"context"

"github.com/shirou/gopsutil/disk"
)

func getDiskCounters(ctx context.Context) ([]diskStats, error) {
driveStats, err := disk.IOCountersWithContext(ctx)
if err != nil {
return nil, err
}

output := make([]diskStats, len(driveStats))
i := 0
for _, counters := range driveStats {
output[i] = diskStats{
readBytes: int64(counters.ReadBytes),
readTimeMs: int64(counters.ReadTime),
readCount: int64(counters.ReadCount),
writeBytes: int64(counters.WriteBytes),
writeTimeMs: int64(counters.WriteTime),
writeCount: int64(counters.WriteCount),
iopsInProgress: int64(counters.IopsInProgress),
}
i++
}

return output, nil
}
45 changes: 45 additions & 0 deletions pkg/server/status/disk_counters_darwin.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
// Copyright 2018 The Cockroach Authors.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
// implied. See the License for the specific language governing
// permissions and limitations under the License.

// +build darwin

package status

import (
"context"

"github.com/lufia/iostat"
)

func getDiskCounters(context.Context) ([]diskStats, error) {
driveStats, err := iostat.ReadDriveStats()
if err != nil {
return nil, err
}

output := make([]diskStats, len(driveStats))
for i, counters := range driveStats {
output[i] = diskStats{
readBytes: counters.BytesRead,
readCount: counters.NumRead,
readTimeMs: int64(counters.TotalReadTime),
writeBytes: counters.BytesWritten,
writeCount: counters.NumWrite,
writeTimeMs: int64(counters.TotalWriteTime),
iopsInProgress: 0, // Not reported by this library. (#27927)
}
}

return output, nil
}
71 changes: 41 additions & 30 deletions pkg/server/status/runtime.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ import (
"github.com/cockroachdb/cockroach/pkg/util/hlc"
"github.com/cockroachdb/cockroach/pkg/util/log"
"github.com/cockroachdb/cockroach/pkg/util/metric"
"github.com/shirou/gopsutil/disk"
"github.com/shirou/gopsutil/net"
)

Expand Down Expand Up @@ -233,7 +232,7 @@ type RuntimeStatSampler struct {
lastCgoCall int64
lastNumGC uint32

initialDiskCounters disk.IOCountersStat
initialDiskCounters diskStats
initialNetCounters net.IOCountersStat

// Only show "not implemented" errors once, we don't need the log spam.
Expand Down Expand Up @@ -459,13 +458,25 @@ func (rsr *RuntimeStatSampler) SampleEnvironment(ctx context.Context) {
}
}

func getSummedDiskCounters(ctx context.Context) (disk.IOCountersStat, error) {
ioCounters, err := disk.IOCountersWithContext(ctx)
type diskStats struct {
readBytes int64
readTimeMs int64
readCount int64

writeBytes int64
writeTimeMs int64
writeCount int64

iopsInProgress int64
}

func getSummedDiskCounters(ctx context.Context) (diskStats, error) {
diskCounters, err := getDiskCounters(ctx)
if err != nil {
return disk.IOCountersStat{}, err
return diskStats{}, err
}

return sumDiskCounters(ioCounters), nil
return sumDiskCounters(diskCounters), nil
}

func (rsr *RuntimeStatSampler) sampleDiskStats(ctx context.Context) error {
Expand All @@ -476,13 +487,13 @@ func (rsr *RuntimeStatSampler) sampleDiskStats(ctx context.Context) error {

subtractDiskCounters(&summedDiskCounters, rsr.initialDiskCounters)

rsr.HostDiskReadBytes.Update(int64(summedDiskCounters.ReadBytes))
rsr.HostDiskReadTime.Update(int64(summedDiskCounters.ReadTime) * 1e6) // ms to ns
rsr.HostDiskReadCount.Update(int64(summedDiskCounters.ReadCount))
rsr.HostDiskWriteBytes.Update(int64(summedDiskCounters.WriteBytes))
rsr.HostDiskWriteTime.Update(int64(summedDiskCounters.WriteTime) * 1e6) // ms to ns
rsr.HostDiskWriteCount.Update(int64(summedDiskCounters.WriteCount))
rsr.IopsInProgress.Update(int64(summedDiskCounters.IopsInProgress))
rsr.HostDiskReadBytes.Update(summedDiskCounters.readBytes)
rsr.HostDiskReadTime.Update(summedDiskCounters.readTimeMs * 1e6) // ms to ns
rsr.HostDiskReadCount.Update(summedDiskCounters.readCount)
rsr.HostDiskWriteBytes.Update(summedDiskCounters.writeBytes)
rsr.HostDiskWriteTime.Update(summedDiskCounters.writeTimeMs * 1e6) // ms to ns
rsr.HostDiskWriteCount.Update(summedDiskCounters.writeCount)
rsr.IopsInProgress.Update(summedDiskCounters.iopsInProgress)

return nil
}
Expand Down Expand Up @@ -514,34 +525,34 @@ func (rsr *RuntimeStatSampler) sampleNetStats(ctx context.Context) error {

// sumDiskCounters returns a new disk.IOCountersStat whose values are the sum of the
// values in the slice of disk.IOCountersStats passed in.
func sumDiskCounters(disksStats map[string]disk.IOCountersStat) disk.IOCountersStat {
output := disk.IOCountersStat{}
func sumDiskCounters(disksStats []diskStats) diskStats {
output := diskStats{}
for _, stats := range disksStats {
output.ReadBytes += stats.ReadBytes
output.ReadTime += stats.ReadTime
output.ReadCount += stats.ReadCount
output.readBytes += stats.readBytes
output.readTimeMs += stats.readTimeMs
output.readCount += stats.readCount

output.WriteBytes += stats.WriteBytes
output.WriteTime += stats.WriteTime
output.WriteCount += stats.WriteCount
output.writeBytes += stats.writeBytes
output.writeTimeMs += stats.writeTimeMs
output.writeCount += stats.writeCount

output.IopsInProgress += stats.IopsInProgress
output.iopsInProgress += stats.iopsInProgress
}
return output
}

// subtractDiskCounters subtracts the counters in `sub` from the counters in `from`,
// saving the results in `from`.
func subtractDiskCounters(from *disk.IOCountersStat, sub disk.IOCountersStat) {
from.WriteCount -= sub.WriteCount
from.WriteTime -= sub.WriteTime
from.WriteBytes -= sub.WriteBytes
func subtractDiskCounters(from *diskStats, sub diskStats) {
from.writeCount -= sub.writeCount
from.writeTimeMs -= sub.writeTimeMs
from.writeBytes -= sub.writeBytes

from.ReadCount -= sub.ReadCount
from.ReadTime -= sub.ReadTime
from.ReadBytes -= sub.ReadBytes
from.readCount -= sub.readCount
from.readTimeMs -= sub.readTimeMs
from.readBytes -= sub.readBytes

from.IopsInProgress -= sub.IopsInProgress
from.iopsInProgress -= sub.iopsInProgress
}

// sumNetworkCounters returns a new net.IOCountersStat whose values are the sum of the
Expand Down
102 changes: 50 additions & 52 deletions pkg/server/status/runtime_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,48 +15,46 @@
package status

import (
"fmt"
"reflect"
"testing"

"fmt"

"github.com/cockroachdb/cockroach/pkg/util/leaktest"
"github.com/shirou/gopsutil/disk"
"github.com/shirou/gopsutil/net"
)

func TestSumDiskCounters(t *testing.T) {
defer leaktest.AfterTest(t)()

counters := map[string]disk.IOCountersStat{
"disk0": {
ReadBytes: 1,
ReadTime: 1,
ReadCount: 1,
IopsInProgress: 1,
WriteBytes: 1,
WriteTime: 1,
WriteCount: 1,
counters := []diskStats{
{
readBytes: 1,
readTimeMs: 1,
readCount: 1,
iopsInProgress: 1,
writeBytes: 1,
writeTimeMs: 1,
writeCount: 1,
},
"disk1": {
ReadBytes: 1,
ReadTime: 1,
ReadCount: 1,
IopsInProgress: 1,
WriteBytes: 1,
WriteTime: 1,
WriteCount: 1,
{
readBytes: 1,
readTimeMs: 1,
readCount: 1,
iopsInProgress: 1,
writeBytes: 1,
writeTimeMs: 1,
writeCount: 1,
},
}
summed := sumDiskCounters(counters)
expected := disk.IOCountersStat{
ReadBytes: 2,
ReadTime: 2,
ReadCount: 2,
WriteBytes: 2,
WriteTime: 2,
WriteCount: 2,
IopsInProgress: 2,
expected := diskStats{
readBytes: 2,
readTimeMs: 2,
readCount: 2,
writeBytes: 2,
writeTimeMs: 2,
writeCount: 2,
iopsInProgress: 2,
}
if !reflect.DeepEqual(summed, expected) {
t.Fatalf("expected %+v; got %+v", expected, summed)
Expand Down Expand Up @@ -95,32 +93,32 @@ func TestSumNetCounters(t *testing.T) {
func TestSubtractDiskCounters(t *testing.T) {
defer leaktest.AfterTest(t)()

from := disk.IOCountersStat{
ReadBytes: 3,
ReadTime: 3,
ReadCount: 3,
WriteBytes: 3,
WriteTime: 3,
WriteCount: 3,
IopsInProgress: 3,
from := diskStats{
readBytes: 3,
readTimeMs: 3,
readCount: 3,
writeBytes: 3,
writeTimeMs: 3,
writeCount: 3,
iopsInProgress: 3,
}
sub := disk.IOCountersStat{
ReadBytes: 1,
ReadTime: 1,
ReadCount: 1,
IopsInProgress: 1,
WriteBytes: 1,
WriteTime: 1,
WriteCount: 1,
sub := diskStats{
readBytes: 1,
readTimeMs: 1,
readCount: 1,
iopsInProgress: 1,
writeBytes: 1,
writeTimeMs: 1,
writeCount: 1,
}
expected := disk.IOCountersStat{
ReadBytes: 2,
ReadTime: 2,
ReadCount: 2,
WriteBytes: 2,
WriteTime: 2,
WriteCount: 2,
IopsInProgress: 2,
expected := diskStats{
readBytes: 2,
readTimeMs: 2,
readCount: 2,
writeBytes: 2,
writeTimeMs: 2,
writeCount: 2,
iopsInProgress: 2,
}
subtractDiskCounters(&from, sub)
if !reflect.DeepEqual(from, expected) {
Expand Down

0 comments on commit 343e38c

Please sign in to comment.