From 027d43b4eba19f21e6d1161605010dc43a2570c8 Mon Sep 17 00:00:00 2001 From: Feng Ruohang Date: Wed, 16 Sep 2026 17:09:19 +0800 Subject: [PATCH] fix: synchronize CPU resource metrics reads Hold resourceMetricsMapMu.RLock while the v3 CPU collector reads both the subsystem map and its nested idle/iowait metrics. The unsynchronized reads were inherited from upstream commit f7b665347 (minio/minio#19560). Add value and concurrent Prometheus Gather regression tests, run them in the targeted race CI job, and record the existing CPU metric names in the compatibility inventory. Fixes #210 Signed-off-by: Feng Ruohang --- .github/workflows/go.yml | 3 + .../rebrand-guard/compat-baseline.json | 8 + cmd/metrics-v3-system-cpu.go | 2 + cmd/metrics-v3-system-cpu_test.go | 185 ++++++++++++++++++ 4 files changed, 198 insertions(+) create mode 100644 cmd/metrics-v3-system-cpu_test.go diff --git a/.github/workflows/go.yml b/.github/workflows/go.yml index 0baad8fe6..7b1f7c60c 100644 --- a/.github/workflows/go.yml +++ b/.github/workflows/go.yml @@ -96,6 +96,9 @@ jobs: - name: Run multipart listing and cancellation tests under race detector run: go test -race ./cmd -run '^Test(MultipartListing|MultipartAbort|PaginateMultipartUploads|ListMultipartUploads)' -count=1 -timeout=5m + - name: Run CPU metrics tests under race detector + run: go test -race ./cmd -run '^TestLoadCPUMetrics' -count=1 -timeout=5m + crosscompile: name: Cross Compile runs-on: ubuntu-latest diff --git a/buildscripts/rebrand-guard/compat-baseline.json b/buildscripts/rebrand-guard/compat-baseline.json index ff95268fd..2ff232340 100644 --- a/buildscripts/rebrand-guard/compat-baseline.json +++ b/buildscripts/rebrand-guard/compat-baseline.json @@ -571,6 +571,14 @@ "minio_resource_stats", "minio_s3", "minio_stats", + "minio_system_cpu_avg_idle", + "minio_system_cpu_avg_iowait", + "minio_system_cpu_load", + "minio_system_cpu_load_perc", + "minio_system_cpu_nice", + "minio_system_cpu_steal", + "minio_system_cpu_system", + "minio_system_cpu_user", "minio_test" ], "headers": [ diff --git a/cmd/metrics-v3-system-cpu.go b/cmd/metrics-v3-system-cpu.go index cb31b8358..dbfb802ae 100644 --- a/cmd/metrics-v3-system-cpu.go +++ b/cmd/metrics-v3-system-cpu.go @@ -69,6 +69,8 @@ func loadCPUMetrics(ctx context.Context, m MetricValues, c *metricsCache) error // metrics-resource.go runs a job to collect resource metrics including their Avg values and // stores them in resourceMetricsMap. We can use it to get the Avg values of CPU idle and IOWait. + resourceMetricsMapMu.RLock() + defer resourceMetricsMapMu.RUnlock() cpuResourceMetrics, found := resourceMetricsMap[cpuSubsystem] if found { if cpuIdleMetric, ok := cpuResourceMetrics[getResourceKey(cpuIdle, nil)]; ok { diff --git a/cmd/metrics-v3-system-cpu_test.go b/cmd/metrics-v3-system-cpu_test.go new file mode 100644 index 000000000..98ec95f57 --- /dev/null +++ b/cmd/metrics-v3-system-cpu_test.go @@ -0,0 +1,185 @@ +// Copyright (c) 2026 Ruohang Feng +// +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU Affero General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// This program is distributed in the hope that it will be useful +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU Affero General Public License for more details. +// +// You should have received a copy of the GNU Affero General Public License +// along with this program. If not, see . + +package cmd + +import ( + "context" + "fmt" + "runtime" + "sync" + "sync/atomic" + "testing" + "time" + + "github.com/minio/madmin-go/v3" + "github.com/minio/minio/internal/cachevalue" + "github.com/prometheus/client_golang/prometheus" + dto "github.com/prometheus/client_model/go" + cpustats "github.com/shirou/gopsutil/v3/cpu" + "github.com/shirou/gopsutil/v3/load" +) + +// These tests replace global resource metrics and must not run in parallel. +// Concurrent writers must finish before the fixture is restored. +func setCPUResourceMetricsForTest(t *testing.T, value map[MetricSubsystem]ResourceMetrics) { + t.Helper() + resourceMetricsMapMu.Lock() + saved := resourceMetricsMap + resourceMetricsMap = value + resourceMetricsMapMu.Unlock() + t.Cleanup(func() { + resourceMetricsMapMu.Lock() + resourceMetricsMap = saved + resourceMetricsMapMu.Unlock() + }) +} + +func newCPUMetricsTestRegistry() *prometheus.Registry { + c := &metricsCache{cpuMetrics: cachevalue.NewFromFunc(time.Hour, cachevalue.Opts{}, + func(context.Context) (madmin.CPUMetrics, error) { + return madmin.CPUMetrics{ + CPUCount: 4, + LoadStat: &load.AvgStat{Load1: 2}, + TimesStat: &cpustats.TimesStat{ + User: 10, System: 20, Idle: 60, Iowait: 5, Nice: 3, Steal: 2, + }, + }, nil + })} + _, _ = c.cpuMetrics.Get() // Warm the cache: the cache does not protect the resource map. + g := NewMetricsGroup(systemCPUCollectorPath, []MetricDescriptor{ + sysCPUAvgIdleMD, sysCPUAvgIOWaitMD, sysCPULoadMD, sysCPULoadPercMD, + sysCPUNiceMD, sysCPUStealMD, sysCPUSystemMD, sysCPUUserMD, + }, loadCPUMetrics) + g.SetCache(c) + r := prometheus.NewPedanticRegistry() + r.MustRegister(g) + return r +} + +func TestLoadCPUMetricsValues(t *testing.T) { + for _, tc := range []struct { + name string + data map[MetricSubsystem]ResourceMetrics + want map[string]float64 + }{ + {name: "nil-map"}, + {name: "empty-map", data: map[MetricSubsystem]ResourceMetrics{}}, + {name: "no-cpu", data: map[MetricSubsystem]ResourceMetrics{memSubsystem: {}}}, + {name: "nil-cpu", data: map[MetricSubsystem]ResourceMetrics{cpuSubsystem: nil}}, + {name: "empty-cpu", data: map[MetricSubsystem]ResourceMetrics{cpuSubsystem: {}}}, + {name: "idle-only", data: map[MetricSubsystem]ResourceMetrics{cpuSubsystem: { + getResourceKey(cpuIdle, nil): {Avg: 87.654}, + }}, want: map[string]float64{"minio_system_cpu_avg_idle": 87.65}}, + {name: "iowait-only", data: map[MetricSubsystem]ResourceMetrics{cpuSubsystem: { + getResourceKey(cpuIOWait, nil): {Avg: 1.236}, + }}, want: map[string]float64{"minio_system_cpu_avg_iowait": 1.24}}, + {name: "both-rounded", data: map[MetricSubsystem]ResourceMetrics{cpuSubsystem: { + getResourceKey(cpuIdle, nil): {Avg: 87.654}, + getResourceKey(cpuIOWait, nil): {Avg: 1.236}, + }}, want: map[string]float64{"minio_system_cpu_avg_idle": 87.65, "minio_system_cpu_avg_iowait": 1.24}}, + {name: "zero-keeps-existing-omission", data: map[MetricSubsystem]ResourceMetrics{cpuSubsystem: { + getResourceKey(cpuIdle, nil): {Avg: 0}, + getResourceKey(cpuIOWait, nil): {Avg: 0}, + }}}, + } { + t.Run(tc.name, func(t *testing.T) { + setCPUResourceMetricsForTest(t, tc.data) + families, err := newCPUMetricsTestRegistry().Gather() + if err != nil { + t.Fatal(err) + } + want := map[string]float64{ + "minio_system_cpu_load": 2, "minio_system_cpu_load_perc": 50, + "minio_system_cpu_nice": 3, "minio_system_cpu_steal": 2, + "minio_system_cpu_system": 20, "minio_system_cpu_user": 10, + } + for k, v := range tc.want { + want[k] = v + } + if len(families) != len(want) { + t.Fatalf("got %d families, want %d", len(families), len(want)) + } + for _, family := range families { + v, ok := want[family.GetName()] + if !ok || family.GetType() != dto.MetricType_GAUGE || len(family.Metric) != 1 || family.Metric[0].GetGauge().GetValue() != v { + t.Errorf("unexpected family: %v (want %v)", family, want) + } + } + }) + } +} + +func TestLoadCPUMetricsConcurrentUpdate(t *testing.T) { + for _, subsystem := range []MetricSubsystem{cpuSubsystem, memSubsystem} { + for _, readers := range []int{1, 4} { + t.Run(fmt.Sprintf("writer-%s/readers-%d", subsystem, readers), func(t *testing.T) { + setCPUResourceMetricsForTest(t, map[MetricSubsystem]ResourceMetrics{}) + updateResourceMetrics(cpuSubsystem, cpuIdle, 80, nil, false) + updateResourceMetrics(cpuSubsystem, cpuIOWait, 5, nil, false) + r := newCPUMetricsTestRegistry() + stop := make(chan struct{}) + done := make(chan struct{}) + ready := make(chan struct{}) + var updates atomic.Uint64 + go func() { + defer close(done) + if subsystem == cpuSubsystem { + updateResourceMetrics(subsystem, cpuIdle, 80, nil, false) + } else { + updateResourceMetrics(subsystem, memUsed, 1024, nil, false) + } + close(ready) + for { + select { + case <-stop: + return + default: + } + if subsystem == cpuSubsystem { + updateResourceMetrics(subsystem, cpuIdle, 80, nil, false) + updateResourceMetrics(subsystem, cpuIOWait, 5, nil, false) + } else { + updateResourceMetrics(subsystem, memUsed, 1024, nil, false) + } + updates.Add(1) + runtime.Gosched() + } + }() + <-ready + defer func() { close(stop); <-done }() + var wg sync.WaitGroup + for range readers { + wg.Add(1) + go func() { + defer wg.Done() + for range 1000 { + families, err := r.Gather() + if err != nil || len(families) != 8 { + t.Errorf("Gather: families=%d, error=%v", len(families), err) + return + } + } + }() + } + wg.Wait() + if updates.Load() == 0 { + t.Fatal("writer made no progress") + } + t.Logf("completed %d gathers with %d concurrent update iterations", readers*1000, updates.Load()) + }) + } + } +}