From 13a8ad12b8296c0360afe3f66218027dae6c1805 Mon Sep 17 00:00:00 2001 From: Patrick Ohly Date: Fri, 25 Aug 2023 10:42:17 +0200 Subject: [PATCH] apiserver: fix data race in etcd metrics 7a63997c8a1a9ba1 added a global variable which gets set multiple times by different goroutines in integration tests, leading to a data race: WARNING: DATA RACE Write at 0x00000a626928 by goroutine 87080: k8s.io/kubernetes/vendor/k8s.io/apiserver/pkg/storage/etcd3/metrics.SetStorageMonitorGetter() /home/prow/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/vendor/k8s.io/apiserver/pkg/storage/etcd3/metrics/metrics.go:231 +0x104 k8s.io/kubernetes/vendor/k8s.io/apiserver/pkg/server/options.(*EtcdOptions).ApplyWithStorageFactoryTo() /home/prow/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/vendor/k8s.io/apiserver/pkg/server/options/etcd.go:242 +0xbd k8s.io/kubernetes/pkg/controlplane/apiserver.BuildGenericConfig() /home/prow/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/pkg/controlplane/apiserver/config.go:124 +0x1c3d k8s.io/kubernetes/cmd/kube-apiserver/app.CreateKubeAPIServerConfig() /home/prow/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/cmd/kube-apiserver/app/server.go:218 +0xeb k8s.io/kubernetes/cmd/kube-apiserver/app.NewConfig() /home/prow/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/cmd/kube-apiserver/app/config.go:74 +0xd5 k8s.io/kubernetes/cmd/kube-apiserver/app/testing.StartTestServer() /home/prow/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/cmd/kube-apiserver/app/testing/testserver.go:299 +0x2e97 k8s.io/kubernetes/cmd/kube-apiserver/app/testing.StartTestServerOrDie() /home/prow/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/cmd/kube-apiserver/app/testing/testserver.go:423 +0xb2 k8s.io/kubernetes/test/integration/controlplane.testReconcilersAPIServerLease.func3() /home/prow/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/test/integration/controlplane/kube_apiserver_test.go:486 +0x1dd k8s.io/kubernetes/test/integration/controlplane.testReconcilersAPIServerLease.func7() /home/prow/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/test/integration/controlplane/kube_apiserver_test.go:488 +0x47 Previous write at 0x00000a626928 by goroutine 87079: k8s.io/kubernetes/vendor/k8s.io/apiserver/pkg/storage/etcd3/metrics.SetStorageMonitorGetter() /home/prow/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/vendor/k8s.io/apiserver/pkg/storage/etcd3/metrics/metrics.go:231 +0x104 k8s.io/kubernetes/vendor/k8s.io/apiserver/pkg/server/options.(*EtcdOptions).ApplyWithStorageFactoryTo() /home/prow/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/vendor/k8s.io/apiserver/pkg/server/options/etcd.go:242 +0xbd k8s.io/kubernetes/pkg/controlplane/apiserver.BuildGenericConfig() /home/prow/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/pkg/controlplane/apiserver/config.go:124 +0x1c3d k8s.io/kubernetes/cmd/kube-apiserver/app.CreateKubeAPIServerConfig() /home/prow/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/cmd/kube-apiserver/app/server.go:218 +0xeb k8s.io/kubernetes/cmd/kube-apiserver/app.NewConfig() /home/prow/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/cmd/kube-apiserver/app/config.go:74 +0xd5 k8s.io/kubernetes/cmd/kube-apiserver/app/testing.StartTestServer() /home/prow/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/cmd/kube-apiserver/app/testing/testserver.go:299 +0x2e97 k8s.io/kubernetes/cmd/kube-apiserver/app/testing.StartTestServerOrDie() /home/prow/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/cmd/kube-apiserver/app/testing/testserver.go:423 +0xb2 k8s.io/kubernetes/test/integration/controlplane.testReconcilersAPIServerLease.func3() /home/prow/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/test/integration/controlplane/kube_apiserver_test.go:486 +0x1dd k8s.io/kubernetes/test/integration/controlplane.testReconcilersAPIServerLease.func7() /home/prow/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/test/integration/controlplane/kube_apiserver_test.go:488 +0x47 Mutex locking avoids the data race. Whether this variable really can be used safely by those concurrent (?) tests is a different question... --- .../pkg/storage/etcd3/metrics/metrics.go | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/staging/src/k8s.io/apiserver/pkg/storage/etcd3/metrics/metrics.go b/staging/src/k8s.io/apiserver/pkg/storage/etcd3/metrics/metrics.go index ac023d55d8c..0192d754e26 100644 --- a/staging/src/k8s.io/apiserver/pkg/storage/etcd3/metrics/metrics.go +++ b/staging/src/k8s.io/apiserver/pkg/storage/etcd3/metrics/metrics.go @@ -228,7 +228,7 @@ func UpdateEtcdDbSize(ep string, size int64) { // SetStorageMonitorGetter sets monitor getter to allow monitoring etcd stats. func SetStorageMonitorGetter(getter func() ([]Monitor, error)) { - storageMonitor.monitorGetter = getter + storageMonitor.setGetter(getter) } // UpdateLeaseObjectCount sets the etcd_lease_object_counts metric. @@ -258,9 +258,22 @@ type StorageMetrics struct { type monitorCollector struct { compbasemetrics.BaseStableCollector + mutex sync.Mutex monitorGetter func() ([]Monitor, error) } +func (m *monitorCollector) setGetter(monitorGetter func() ([]Monitor, error)) { + m.mutex.Lock() + defer m.mutex.Unlock() + m.monitorGetter = monitorGetter +} + +func (m *monitorCollector) getGetter() func() ([]Monitor, error) { + m.mutex.Lock() + defer m.mutex.Unlock() + return m.monitorGetter +} + // DescribeWithStability implements compbasemetrics.StableColletor func (c *monitorCollector) DescribeWithStability(ch chan<- *compbasemetrics.Desc) { ch <- storageSizeDescription @@ -268,7 +281,7 @@ func (c *monitorCollector) DescribeWithStability(ch chan<- *compbasemetrics.Desc // CollectWithStability implements compbasemetrics.StableColletor func (c *monitorCollector) CollectWithStability(ch chan<- compbasemetrics.Metric) { - monitors, err := c.monitorGetter() + monitors, err := c.getGetter()() if err != nil { return }