diff --git a/internal/hostmetrics/collector.go b/internal/hostmetrics/collector.go index 16c275a912..340c0cb9a2 100644 --- a/internal/hostmetrics/collector.go +++ b/internal/hostmetrics/collector.go @@ -5,6 +5,7 @@ import ( "fmt" "sort" "strings" + "sync" "time" agentshost "github.com/rcourtman/pulse-go-rewrite/pkg/agents/host" @@ -21,6 +22,7 @@ import ( var ( cpuCounts = gocpu.CountsWithContext cpuPercent = gocpu.PercentWithContext + cpuTimes = gocpu.TimesWithContext loadAvg = goload.AvgWithContext virtualMemory = gomem.VirtualMemoryWithContext diskPartitions = godisk.PartitionsWithContext @@ -146,7 +148,66 @@ func memoryUsagePercent(usedBytes, totalBytes uint64) float64 { return percent } +// cpuUsageTracker computes CPU usage as the busy/total delta of the cumulative +// CPU time counters between consecutive collections, so the reported value +// averages over the whole report interval instead of a 1-second spot sample. +// Short spot samples badly overstate CPU on mostly-idle guests and block the +// collect path for a full second (issue #1648). +type cpuUsageTracker struct { + mu sync.Mutex + prev gocpu.TimesStat + hasPrev bool +} + +// defaultCPUUsage backs Collect. The state is process-wide by design: every +// consumer of Collect reports a busy average over the window since the +// previous collection, which stays correct even if collections interleave. +var defaultCPUUsage = &cpuUsageTracker{} + func collectCPUUsage(ctx context.Context) (float64, error) { + return defaultCPUUsage.collect(ctx) +} + +func (t *cpuUsageTracker) collect(ctx context.Context) (float64, error) { + times, err := cpuTimes(ctx, false) + if err != nil || len(times) == 0 { + return spotCPUUsage(ctx) + } + cur := times[0] + + t.mu.Lock() + prev, hasPrev := t.prev, t.hasPrev + t.prev, t.hasPrev = cur, true + t.mu.Unlock() + + if !hasPrev { + // No baseline yet on the very first collection. + return spotCPUUsage(ctx) + } + + prevTotal, prevBusy := cpuBusyTotal(prev) + curTotal, curBusy := cpuBusyTotal(cur) + if curBusy < prevBusy || curTotal <= prevTotal { + // Counters went backwards (reboot, VM migration) or did not advance. + return spotCPUUsage(ctx) + } + + return clampPercent((curBusy - prevBusy) / (curTotal - prevTotal) * 100), nil +} + +// cpuBusyTotal mirrors gopsutil's getAllBusy: guest time is already folded +// into user/nice by the Linux kernel, so it is subtracted from the total to +// avoid double counting (it is zero on other platforms), and iowait counts +// as idle. +func cpuBusyTotal(t gocpu.TimesStat) (total, busy float64) { + total = t.Total() - t.Guest - t.GuestNice + busy = total - t.Idle - t.Iowait + return total, busy +} + +// spotCPUUsage is the legacy blocking 1-second sample, kept as the fallback +// when no cross-interval delta is available. +func spotCPUUsage(ctx context.Context) (float64, error) { percentages, err := cpuPercent(ctx, time.Second, false) if err != nil { return 0, err @@ -154,15 +215,7 @@ func collectCPUUsage(ctx context.Context) (float64, error) { if len(percentages) == 0 { return 0, nil } - - usage := percentages[0] - if usage < 0 { - usage = 0 - } - if usage > 100 { - usage = 100 - } - return usage, nil + return clampPercent(percentages[0]), nil } func collectDisks(ctx context.Context, diskExclude []string) []agentshost.Disk { diff --git a/internal/hostmetrics/collector_memory_regression_test.go b/internal/hostmetrics/collector_memory_regression_test.go index 686eaf782a..4bc47510a5 100644 --- a/internal/hostmetrics/collector_memory_regression_test.go +++ b/internal/hostmetrics/collector_memory_regression_test.go @@ -7,6 +7,7 @@ import ( "testing" "time" + gocpu "github.com/shirou/gopsutil/v4/cpu" godisk "github.com/shirou/gopsutil/v4/disk" goload "github.com/shirou/gopsutil/v4/load" gomem "github.com/shirou/gopsutil/v4/mem" @@ -20,6 +21,7 @@ func TestCollectMemoryStability(t *testing.T) { origCPUCounts := cpuCounts origCPUPercent := cpuPercent + origCPUTimes := cpuTimes origLoadAvg := loadAvg origVirtualMemory := virtualMemory origDiskPartitions := diskPartitions @@ -32,6 +34,9 @@ func TestCollectMemoryStability(t *testing.T) { cpuPercent = func(ctx context.Context, interval time.Duration, percpu bool) ([]float64, error) { return []float64{5.0}, nil } + cpuTimes = func(ctx context.Context, percpu bool) ([]gocpu.TimesStat, error) { + return []gocpu.TimesStat{{CPU: "cpu-total", User: 100, System: 50, Idle: 1000}}, nil + } loadAvg = func(ctx context.Context) (*goload.AvgStat, error) { return &goload.AvgStat{Load1: 0.1, Load5: 0.2, Load15: 0.3}, nil } @@ -78,6 +83,7 @@ func TestCollectMemoryStability(t *testing.T) { t.Cleanup(func() { cpuCounts = origCPUCounts cpuPercent = origCPUPercent + cpuTimes = origCPUTimes loadAvg = origLoadAvg virtualMemory = origVirtualMemory diskPartitions = origDiskPartitions diff --git a/internal/hostmetrics/coverage_additional_test.go b/internal/hostmetrics/coverage_additional_test.go index cc1318a656..ca2632f4f7 100644 --- a/internal/hostmetrics/coverage_additional_test.go +++ b/internal/hostmetrics/coverage_additional_test.go @@ -11,15 +11,24 @@ import ( "testing" "time" + gocpu "github.com/shirou/gopsutil/v4/cpu" godisk "github.com/shirou/gopsutil/v4/disk" ) func TestCollectCPUUsageBranches(t *testing.T) { origCPUPercent := cpuPercent + origCPUTimes := cpuTimes t.Cleanup(func() { cpuPercent = origCPUPercent + cpuTimes = origCPUTimes }) + // Force the spot-sample fallback so the branches below stay deterministic + // regardless of the cross-interval delta state (issue #1648). + cpuTimes = func(ctx context.Context, percpu bool) ([]gocpu.TimesStat, error) { + return nil, errors.New("times unavailable") + } + cpuPercent = func(ctx context.Context, interval time.Duration, percpu bool) ([]float64, error) { return nil, errors.New("boom") } diff --git a/internal/hostmetrics/issue1648_cpu_interval_delta_test.go b/internal/hostmetrics/issue1648_cpu_interval_delta_test.go new file mode 100644 index 0000000000..9142e1504f --- /dev/null +++ b/internal/hostmetrics/issue1648_cpu_interval_delta_test.go @@ -0,0 +1,105 @@ +package hostmetrics + +import ( + "context" + "math" + "testing" + "time" + + gocpu "github.com/shirou/gopsutil/v4/cpu" +) + +// Issue #1648: a 1-second spot sample once per report interval overstates CPU +// on mostly-idle guests. CPU usage must be computed as the busy/total delta of +// the cumulative counters across the whole gap between collections, with the +// spot sample only as a fallback for the first collection and counter resets. +func TestIssue1648CPUUsageIntervalDelta(t *testing.T) { + origCPUPercent := cpuPercent + origCPUTimes := cpuTimes + t.Cleanup(func() { + cpuPercent = origCPUPercent + cpuTimes = origCPUTimes + }) + + // What a 1-second window would read on the idle 2-vCPU guest from the + // report: a short burst makes the instant view claim ~28% CPU. + const spotReading = 28.4 + spotCalls := 0 + cpuPercent = func(ctx context.Context, interval time.Duration, percpu bool) ([]float64, error) { + spotCalls++ + return []float64{spotReading}, nil + } + + // Cumulative counter samples 30s apart on a 2-vCPU guest (60 CPU-seconds + // per interval). Between samples[0] and samples[1] only 3 of those 60 + // seconds are busy, so the interval average is 5%. Guest time advances + // inside user and must not count as busy on top of it. + samples := []gocpu.TimesStat{ + {CPU: "cpu-total", User: 120, Nice: 1, System: 60, Idle: 5000, Iowait: 12, Irq: 2, Softirq: 3, Steal: 1, Guest: 10}, + {CPU: "cpu-total", User: 121.5, Nice: 1.25, System: 61, Idle: 5056.5, Iowait: 12.5, Irq: 2, Softirq: 3, Steal: 1.25, Guest: 10.5}, + // Counters restart far below the previous sample (reboot or migration). + {CPU: "cpu-total", User: 1, System: 0.5, Idle: 20}, + // One interval after the reset: 1 busy second out of 60. + {CPU: "cpu-total", User: 1.5, System: 1, Idle: 78, Iowait: 1}, + } + sampleIdx := 0 + cpuTimes = func(ctx context.Context, percpu bool) ([]gocpu.TimesStat, error) { + s := samples[sampleIdx] + sampleIdx++ + return []gocpu.TimesStat{s}, nil + } + + tracker := &cpuUsageTracker{} + ctx := context.Background() + + // First collection has no baseline and must fall back to the spot sample. + usage, err := tracker.collect(ctx) + if err != nil { + t.Fatalf("first collect: %v", err) + } + if usage != spotReading { + t.Fatalf("first collect should use spot fallback, got %v want %v", usage, spotReading) + } + if spotCalls != 1 { + t.Fatalf("expected 1 spot sample after first collect, got %d", spotCalls) + } + + // Second collection must average across the interval, not re-run the + // blocking spot sample that would have reported ~28%. + usage, err = tracker.collect(ctx) + if err != nil { + t.Fatalf("second collect: %v", err) + } + if math.Abs(usage-5.0) > 1e-9 { + t.Fatalf("interval delta usage = %v, want 5.0 (spot window would claim %v)", usage, spotReading) + } + if spotCalls != 1 { + t.Fatalf("interval delta must not invoke the blocking spot sample, got %d calls", spotCalls) + } + + // A counter reset makes the delta negative; that collection falls back to + // the spot sample instead of reporting garbage. + usage, err = tracker.collect(ctx) + if err != nil { + t.Fatalf("post-reset collect: %v", err) + } + if usage != spotReading { + t.Fatalf("counter reset should use spot fallback, got %v want %v", usage, spotReading) + } + if spotCalls != 2 { + t.Fatalf("expected 2 spot samples after counter reset, got %d", spotCalls) + } + + // The reset sample rebaselines the tracker, so the next collection + // resumes interval averaging: 1 busy CPU-second out of 60. + usage, err = tracker.collect(ctx) + if err != nil { + t.Fatalf("post-rebaseline collect: %v", err) + } + if math.Abs(usage-100.0/60.0) > 1e-9 { + t.Fatalf("post-rebaseline usage = %v, want %v", usage, 100.0/60.0) + } + if spotCalls != 2 { + t.Fatalf("rebaselined delta must not invoke the spot sample, got %d calls", spotCalls) + } +}