Merge #153048

153048: metric: reset net/disk counters on subtraction if new val is lower r=dhartunian a=dhartunian

Previously, we would occasionally see negative values for counters in tests when the OS would report lower values than initial or earlier snapshots. This can cause confusion for customers.

Now, when `subtractNetworkCounters` or `subtractDiskCounters` are invoked, we will reset the baseline values in order to prevent a negative counter.

Some reasons this could happen were pointed out in #147137.

Resolves: #147137
Resolves: #152484

Epic: None

Release note (observability change): network and disk metric counters collected from the operating system could occasionally report negative values in scenarios where unerlying hardware changes occurred. Certain counters will now proactively reset their values to a new baseline to prevent this.

Co-authored-by: David Hartunian <[email protected]>

Author: craig[bot]

pkg/server/status/runtime.go

@@ -1042,7 +1042,7 @@ func (rsr *RuntimeStatSampler) SampleEnvironment(ctx context.Context, cs *CGoMem
 		log.Ops.Warningf(ctx, "problem fetching disk stats: %s; disk stats will be empty.", err)
 	} else {
 		rsr.last.disk = diskCounters
-		subtractDiskCounters(&diskCounters, rsr.initialDiskCounters)
+		subtractDiskCounters(ctx, &diskCounters, &rsr.initialDiskCounters)
 
 		rsr.HostDiskReadBytes.Update(diskCounters.ReadBytes)
 		rsr.HostDiskReadCount.Update(diskCounters.readCount)
@@ -1063,11 +1063,11 @@ func (rsr *RuntimeStatSampler) SampleEnvironment(ctx context.Context, cs *CGoMem
 		}
 	} else {
 		deltaNet = nc // delta since *last* scrape
-		subtractNetworkCounters(&deltaNet, rsr.last.net)
+		subtractNetworkCounters(ctx, &deltaNet, &rsr.last.net)
 		rsr.last.net = nc
 
 		// `nc` will now be the delta since *first* scrape.
-		subtractNetworkCounters(&nc, rsr.initialNetCounters)
+		subtractNetworkCounters(ctx, &nc, &rsr.initialNetCounters)
 		// TODO(tbg): this is awkward: we're computing the delta above,
 		// why don't we increment the counters?
 		rsr.HostNetRecvBytes.Update(int64(nc.IOCounters.BytesRecv))
@@ -1443,19 +1443,38 @@ func sumAndFilterDiskCounters(disksStats []DiskStats) (DiskStats, error) {
 	return output, nil
 }
 
-// subtractDiskCounters subtracts the counters in `sub` from the counters in `from`,
-// saving the results in `from`.
-func subtractDiskCounters(from *DiskStats, sub DiskStats) {
-	from.writeCount -= sub.writeCount
-	from.WriteBytes -= sub.WriteBytes
-	from.writeTime -= sub.writeTime
-
-	from.readCount -= sub.readCount
-	from.ReadBytes -= sub.ReadBytes
-	from.readTime -= sub.readTime
-
-	from.ioTime -= sub.ioTime
-	from.weightedIOTime -= sub.weightedIOTime
+// subtractDiskCounters subtracts the counters in `baseline` from the
+// counters in `stats`, saving the results in `stats`. If any counter
+// in `stats` is lower than the corresponding counter in `baseline`
+// (indicating a reset), the value for all metrics in `baseline`
+// is updated to the current value in `stats` to establish a new
+// baseline.
+func subtractDiskCounters(ctx context.Context, stats *DiskStats, baseline *DiskStats) {
+	if stats.WriteBytes < baseline.WriteBytes ||
+		stats.writeCount < baseline.writeCount ||
+		stats.writeTime < baseline.writeTime ||
+		stats.ReadBytes < baseline.ReadBytes ||
+		stats.readCount < baseline.readCount ||
+		stats.readTime < baseline.readTime ||
+		stats.ioTime < baseline.ioTime ||
+		stats.weightedIOTime < baseline.weightedIOTime {
+		*baseline = *stats
+		*stats = DiskStats{}
+		log.Ops.Info(ctx, "runtime: new baseline in disk stats from host. disk metric counters have been reset.")
+		return
+	}
+
+	// Perform normal subtraction
+	stats.writeCount -= baseline.writeCount
+	stats.WriteBytes -= baseline.WriteBytes
+	stats.writeTime -= baseline.writeTime
+
+	stats.readCount -= baseline.readCount
+	stats.ReadBytes -= baseline.ReadBytes
+	stats.readTime -= baseline.readTime
+
+	stats.ioTime -= baseline.ioTime
+	stats.weightedIOTime -= baseline.weightedIOTime
 }
 
 // sumNetworkCounters returns a new net.IOCountersStat whose values are the sum of the
@@ -1475,22 +1494,46 @@ func sumNetworkCounters(netCounters []net.IOCountersStat) net.IOCountersStat {
 	return output
 }
 
-// subtractNetworkCounters subtracts the counters in `sub` from the counters in `from`,
-// saving the results in `from`.
-func subtractNetworkCounters(from *netCounters, sub netCounters) {
-	from.IOCounters.BytesRecv -= sub.IOCounters.BytesRecv
-	from.IOCounters.PacketsRecv -= sub.IOCounters.PacketsRecv
-	from.IOCounters.Errin -= sub.IOCounters.Errin
-	from.IOCounters.Dropin -= sub.IOCounters.Dropin
-	from.IOCounters.BytesSent -= sub.IOCounters.BytesSent
-	from.IOCounters.PacketsSent -= sub.IOCounters.PacketsSent
-	from.IOCounters.Errout -= sub.IOCounters.Errout
-	from.IOCounters.Dropout -= sub.IOCounters.Dropout
-	from.TCPRetransSegs -= sub.TCPRetransSegs
-	from.TCPFastRetrans -= sub.TCPFastRetrans
-	from.TCPTimeouts -= sub.TCPTimeouts
-	from.TCPSlowStartRetrans -= sub.TCPSlowStartRetrans
-	from.TCPLossProbes -= sub.TCPLossProbes
+// subtractNetworkCounters subtracts the counters in `baseline`
+// from the counters in `stats`, saving the results in `stats`. If
+// any counter in `stats` is lower than the corresponding counter
+// in `baseline` (indicating a reset), the value for all metrics in
+// `baseline` is updated to the current value in `stats` to establish
+// a new baseline.
+func subtractNetworkCounters(ctx context.Context, stats *netCounters, baseline *netCounters) {
+	if stats.IOCounters.BytesRecv < baseline.IOCounters.BytesRecv ||
+		stats.IOCounters.PacketsRecv < baseline.IOCounters.PacketsRecv ||
+		stats.IOCounters.Errin < baseline.IOCounters.Errin ||
+		stats.IOCounters.Dropin < baseline.IOCounters.Dropin ||
+		stats.IOCounters.BytesSent < baseline.IOCounters.BytesSent ||
+		stats.IOCounters.PacketsSent < baseline.IOCounters.PacketsSent ||
+		stats.IOCounters.Errout < baseline.IOCounters.Errout ||
+		stats.IOCounters.Dropout < baseline.IOCounters.Dropout ||
+		stats.TCPRetransSegs < baseline.TCPRetransSegs ||
+		stats.TCPFastRetrans < baseline.TCPFastRetrans ||
+		stats.TCPTimeouts < baseline.TCPTimeouts ||
+		stats.TCPSlowStartRetrans < baseline.TCPSlowStartRetrans ||
+		stats.TCPLossProbes < baseline.TCPLossProbes {
+		*baseline = *stats
+		*stats = netCounters{}
+		log.Ops.Info(ctx, "runtime: new baseline in network stats from host. network metric counters have been reset.")
+		return
+	}
+
+	// Perform normal subtraction
+	stats.IOCounters.BytesRecv -= baseline.IOCounters.BytesRecv
+	stats.IOCounters.PacketsRecv -= baseline.IOCounters.PacketsRecv
+	stats.IOCounters.Errin -= baseline.IOCounters.Errin
+	stats.IOCounters.Dropin -= baseline.IOCounters.Dropin
+	stats.IOCounters.BytesSent -= baseline.IOCounters.BytesSent
+	stats.IOCounters.PacketsSent -= baseline.IOCounters.PacketsSent
+	stats.IOCounters.Errout -= baseline.IOCounters.Errout
+	stats.IOCounters.Dropout -= baseline.IOCounters.Dropout
+	stats.TCPRetransSegs -= baseline.TCPRetransSegs
+	stats.TCPFastRetrans -= baseline.TCPFastRetrans
+	stats.TCPTimeouts -= baseline.TCPTimeouts
+	stats.TCPSlowStartRetrans -= baseline.TCPSlowStartRetrans
+	stats.TCPLossProbes -= baseline.TCPLossProbes
 }
 
 // GetProcCPUTime returns the cumulative user/system time (in ms) since the process start.

pkg/server/status/runtime_test.go

@@ -12,6 +12,7 @@ import (
 	"runtime"
 	"runtime/metrics"
 	"testing"
+	"time"
 
 	"github.com/cockroachdb/cockroach/pkg/testutils/skip"
 	"github.com/cockroachdb/cockroach/pkg/util/hlc"
@@ -129,12 +130,54 @@ func TestSubtractDiskCounters(t *testing.T) {
 		// Don't touch iops in progress; it is a gauge, not a counter.
 		iopsInProgress: 3,
 	}
-	subtractDiskCounters(&from, sub)
+	subtractDiskCounters(context.Background(), &from, &sub)
 	if !reflect.DeepEqual(from, expected) {
 		t.Fatalf("expected %+v; got %+v", expected, from)
 	}
 }
 
+func TestSubtractDiskCountersReset(t *testing.T) {
+	defer leaktest.AfterTest(t)()
+
+	// Test case where current values are lower than baseline (indicating counter reset)
+	stats := DiskStats{
+		ReadBytes:      2,                      // lower than sub (10)
+		readCount:      5,                      // higher than sub (3)
+		readTime:       time.Millisecond * 100, // lower than sub (200ms)
+		WriteBytes:     3,                      // lower than sub (8)
+		writeCount:     7,                      // higher than sub (4)
+		writeTime:      time.Millisecond * 150, // higher than sub (50ms)
+		ioTime:         time.Millisecond * 80,  // lower than sub (120ms)
+		weightedIOTime: time.Millisecond * 200, // higher than sub (180ms)
+		iopsInProgress: 5,                      // gauge - should not be affected
+	}
+	baseline := DiskStats{
+		ReadBytes:      10,
+		readCount:      3,
+		readTime:       time.Millisecond * 200,
+		WriteBytes:     8,
+		writeCount:     4,
+		writeTime:      time.Millisecond * 50,
+		ioTime:         time.Millisecond * 120,
+		weightedIOTime: time.Millisecond * 180,
+		iopsInProgress: 2,
+	}
+
+	// Expected: all values should be reset
+	expected := DiskStats{}
+
+	// Verify that baselines were updated correctly for reset fields
+	expectedBaseline := stats
+
+	subtractDiskCounters(context.Background(), &stats, &baseline)
+	if !reflect.DeepEqual(stats, expected) {
+		t.Fatalf("expected %+v; got %+v", expected, stats)
+	}
+	if !reflect.DeepEqual(baseline, expectedBaseline) {
+		t.Fatalf("expected sub %+v; got %+v", expectedBaseline, baseline)
+	}
+}
+
 func TestSubtractNetCounters(t *testing.T) {
 	defer leaktest.AfterTest(t)()
 
@@ -180,12 +223,69 @@ func TestSubtractNetCounters(t *testing.T) {
 		TCPRetransSegs: 3,
 		TCPFastRetrans: 6,
 	}
-	subtractNetworkCounters(&from, sub)
+	subtractNetworkCounters(context.Background(), &from, &sub)
 	if !reflect.DeepEqual(from, expected) {
 		t.Fatalf("expected %+v; got %+v", expected, from)
 	}
 }
 
+func TestSubtractNetCountersReset(t *testing.T) {
+	defer leaktest.AfterTest(t)()
+
+	// Test case where current values are lower than baseline (indicating counter reset)
+	stats := netCounters{
+		IOCounters: net.IOCountersStat{
+			PacketsRecv: 2, // lower than sub (5)
+			BytesRecv:   1, // lower than sub (10)
+			Errin:       3, // higher than sub (2)
+			Dropin:      1, // lower than sub (4)
+			BytesSent:   5, // lower than sub (8)
+			PacketsSent: 7, // higher than sub (3)
+			Errout:      2, // lower than sub (6)
+			Dropout:     4, // higher than sub (1)
+		},
+		TCPRetransSegs:      15, // lower than sub (20)
+		TCPFastRetrans:      8,  // higher than sub (5)
+		TCPTimeouts:         3,  // lower than sub (12)
+		TCPSlowStartRetrans: 9,  // higher than sub (7)
+		TCPLossProbes:       1,  // lower than sub (4)
+	}
+	baseline := netCounters{
+		IOCounters: net.IOCountersStat{
+			PacketsRecv: 5,
+			BytesRecv:   10,
+			Errin:       2,
+			Dropin:      4,
+			BytesSent:   8,
+			PacketsSent: 3,
+			Errout:      6,
+			Dropout:     1,
+		},
+		TCPRetransSegs:      20,
+		TCPFastRetrans:      5,
+		TCPTimeouts:         12,
+		TCPSlowStartRetrans: 7,
+		TCPLossProbes:       4,
+	}
+
+	// Expected: reset values should be set to sub values when from < sub,
+	// then normal subtraction occurs, resulting in 0 for reset fields
+	expected := netCounters{
+		IOCounters: net.IOCountersStat{},
+	}
+
+	// Verify that baselines were updated correctly for reset fields
+	expectedBaseilne := stats
+
+	subtractNetworkCounters(context.Background(), &stats, &baseline)
+	if !reflect.DeepEqual(stats, expected) {
+		t.Fatalf("expected %+v; got %+v", expected, stats)
+	}
+	if !reflect.DeepEqual(baseline, expectedBaseilne) {
+		t.Fatalf("expected sub %+v; got %+v", expectedBaseilne, baseline)
+	}
+}
+
 func TestFloat64HistogramSum(t *testing.T) {
 	defer leaktest.AfterTest(t)()
 	type testCase struct {