From aabf1de740b730c903a8a66dff6fe1565d68db1a Mon Sep 17 00:00:00 2001 From: Pete Vilter Date: Tue, 24 Jul 2018 13:46:55 -0400 Subject: [PATCH] server: fix crash when getting disk stats on mac 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 #27867 Release note: None --- Gopkg.lock | 8 +- pkg/server/status/disk_counters.go | 47 ++++++++++ pkg/server/status/disk_counters_darwin.go | 45 ++++++++++ pkg/server/status/runtime.go | 71 ++++++++------- pkg/server/status/runtime_test.go | 102 +++++++++++----------- vendor | 2 +- 6 files changed, 191 insertions(+), 84 deletions(-) create mode 100644 pkg/server/status/disk_counters.go create mode 100644 pkg/server/status/disk_counters_darwin.go diff --git a/Gopkg.lock b/Gopkg.lock index 9d28057465cb..d10af7202b61 100644 --- a/Gopkg.lock +++ b/Gopkg.lock @@ -771,6 +771,12 @@ revision = "ba38bae1f0ec1ece9092d35bbecbb497ee344cbc" version = "v0.15.0" +[[projects]] + branch = "master" + name = "github.com/lufia/iostat" + packages = ["."] + revision = "9f7362b77ad333b26c01c99de52a11bdb650ded2" + [[projects]] name = "github.com/marstr/guid" packages = ["."] @@ -1318,6 +1324,6 @@ [solve-meta] analyzer-name = "dep" analyzer-version = 1 - inputs-digest = "7356b2f481042b2b79a78bd5541e8c7d40e8073cbb5d6c6f0854c9dcfd9455b0" + inputs-digest = "24f1631a158f62585d7805d142405d71312f2609b00cb7a3d268b18b9f5f7f32" solver-name = "gps-cdcl" solver-version = 1 diff --git a/pkg/server/status/disk_counters.go b/pkg/server/status/disk_counters.go new file mode 100644 index 000000000000..2b4c445a78cc --- /dev/null +++ b/pkg/server/status/disk_counters.go @@ -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 +} diff --git a/pkg/server/status/disk_counters_darwin.go b/pkg/server/status/disk_counters_darwin.go new file mode 100644 index 000000000000..08b0c5a276d0 --- /dev/null +++ b/pkg/server/status/disk_counters_darwin.go @@ -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 +} diff --git a/pkg/server/status/runtime.go b/pkg/server/status/runtime.go index 79282a029205..a6c1fc8aeebf 100644 --- a/pkg/server/status/runtime.go +++ b/pkg/server/status/runtime.go @@ -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" ) @@ -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. @@ -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 { @@ -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 } @@ -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 diff --git a/pkg/server/status/runtime_test.go b/pkg/server/status/runtime_test.go index 829397998079..0429e693ea81 100644 --- a/pkg/server/status/runtime_test.go +++ b/pkg/server/status/runtime_test.go @@ -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) @@ -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) { diff --git a/vendor b/vendor index ac6e8eec9939..d7cf42bc8289 160000 --- a/vendor +++ b/vendor @@ -1 +1 @@ -Subproject commit ac6e8eec9939751c053035dcb04e122daab6f00b +Subproject commit d7cf42bc8289e8e082f0650223cc58474adb6848