From 88e055059cd761098cad3ce3de90e967ff3fb971 Mon Sep 17 00:00:00 2001 From: Patrick Ohly Date: Wed, 29 Mar 2023 20:29:38 +0200 Subject: [PATCH] component-base: avoid data race in log format registry test/integration/controlplane.TestReconcilerAPIServerLeaseMultiMoreAPIServers creates two apiservers concurrently and both use the same registry. Normally that code runs during init in the main goroutine, so it doesn't need to be thread-safe. But adding locking is simpler than changing the test. Write at 0x00c00041ff28 by goroutine 58019: k8s.io/kubernetes/vendor/k8s.io/component-base/logs/api/v1.(*logFormatRegistry).freeze() /home/prow/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/vendor/k8s.io/component-base/logs/api/v1/registry.go:124 +0x110 k8s.io/kubernetes/vendor/k8s.io/component-base/logs/api/v1.addFlags() /home/prow/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/vendor/k8s.io/component-base/logs/api/v1/options.go:261 +0xeb k8s.io/kubernetes/vendor/k8s.io/component-base/logs/api/v1.AddFlags() /home/prow/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/vendor/k8s.io/component-base/logs/api/v1/options.go:223 +0x4f2 k8s.io/kubernetes/cmd/kube-apiserver/app/options.(*ServerRunOptions).Flags() /home/prow/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/cmd/kube-apiserver/app/options/options.go:156 +0x49e 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:138 +0x4d4 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:360 +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 0x00c00041ff28 by goroutine 58018: k8s.io/kubernetes/vendor/k8s.io/component-base/logs/api/v1.(*logFormatRegistry).freeze() /home/prow/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/vendor/k8s.io/component-base/logs/api/v1/registry.go:124 +0x110 k8s.io/kubernetes/vendor/k8s.io/component-base/logs/api/v1.addFlags() /home/prow/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/vendor/k8s.io/component-base/logs/api/v1/options.go:261 +0xeb k8s.io/kubernetes/vendor/k8s.io/component-base/logs/api/v1.AddFlags() /home/prow/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/vendor/k8s.io/component-base/logs/api/v1/options.go:223 +0x4f2 k8s.io/kubernetes/cmd/kube-apiserver/app/options.(*ServerRunOptions).Flags() /home/prow/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/cmd/kube-apiserver/app/options/options.go:156 +0x49e 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:138 +0x4d4 k8s.io/kubernetes/cmd/kube-apiserver/app/testing.StartTestServerOrDie() ... --- .../src/k8s.io/component-base/logs/api/v1/registry.go | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/staging/src/k8s.io/component-base/logs/api/v1/registry.go b/staging/src/k8s.io/component-base/logs/api/v1/registry.go index f8fc1f2cae1..6dc23ec1826 100644 --- a/staging/src/k8s.io/component-base/logs/api/v1/registry.go +++ b/staging/src/k8s.io/component-base/logs/api/v1/registry.go @@ -20,6 +20,7 @@ import ( "fmt" "sort" "strings" + "sync" "github.com/go-logr/logr" @@ -30,6 +31,7 @@ var logRegistry = newLogFormatRegistry() // logFormatRegistry stores factories for all supported logging formats. type logFormatRegistry struct { + mutex sync.Mutex registry map[string]logFormat frozen bool } @@ -83,6 +85,8 @@ func newLogFormatRegistry() *logFormatRegistry { // register adds a new log format. It's an error to modify an existing one. func (lfr *logFormatRegistry) register(name string, format logFormat) error { + lfr.mutex.Lock() + defer lfr.mutex.Unlock() if lfr.frozen { return fmt.Errorf("log format registry is frozen, unable to register log format %s", name) } @@ -98,6 +102,8 @@ func (lfr *logFormatRegistry) register(name string, format logFormat) error { // get specified log format factory func (lfr *logFormatRegistry) get(name string) (*logFormat, error) { + lfr.mutex.Lock() + defer lfr.mutex.Unlock() format, ok := lfr.registry[name] if !ok { return nil, fmt.Errorf("log format: %s does not exists", name) @@ -107,6 +113,8 @@ func (lfr *logFormatRegistry) get(name string) (*logFormat, error) { // list names of registered log formats, including feature gates (sorted) func (lfr *logFormatRegistry) list() string { + lfr.mutex.Lock() + defer lfr.mutex.Unlock() formats := make([]string, 0, len(lfr.registry)) for name, format := range lfr.registry { item := fmt.Sprintf(`"%s"`, name) @@ -121,5 +129,7 @@ func (lfr *logFormatRegistry) list() string { // freeze prevents further modifications of the registered log formats. func (lfr *logFormatRegistry) freeze() { + lfr.mutex.Lock() + defer lfr.mutex.Unlock() lfr.frozen = true }