From 389142efb03b3d20be6d475a912948c226d5ef0e Mon Sep 17 00:00:00 2001 From: Daniel Franz Date: Mon, 16 Jan 2023 09:50:01 -0800 Subject: [PATCH 1/3] OCPBUGS-5523: Catalog, fatal error: concurrent map read and map write (#2913) * protected subscriptionSyncCounters access to prevent concurrent map writes Signed-off-by: Daniel Franz * organize map and mutex into single struct Signed-off-by: Daniel Franz * initialize struct Signed-off-by: Daniel Franz * use RWMutex to allow concurrent reads Signed-off-by: Daniel Franz Upstream-repository: operator-lifecycle-manager Upstream-commit: 2a49a4dddeb3e0fc38b44925bf9bd0d3931d4ff4 --- .../pkg/metrics/metrics.go | 45 +++++++++++++++---- 1 file changed, 36 insertions(+), 9 deletions(-) diff --git a/staging/operator-lifecycle-manager/pkg/metrics/metrics.go b/staging/operator-lifecycle-manager/pkg/metrics/metrics.go index 4bb2f4ae8a..7512d87f72 100644 --- a/staging/operator-lifecycle-manager/pkg/metrics/metrics.go +++ b/staging/operator-lifecycle-manager/pkg/metrics/metrics.go @@ -1,6 +1,7 @@ package metrics import ( + "sync" "time" "github.com/prometheus/client_golang/prometheus" @@ -199,12 +200,37 @@ var ( }, ) - // subscriptionSyncCounters keeps a record of the Prometheus counters emitted by - // Subscription objects. The key of a record is the Subscription name, while the value - // is struct containing label values used in the counter - subscriptionSyncCounters = make(map[string]subscriptionSyncLabelValues) + subscriptionSyncCounters = newSubscriptionSyncCounter() ) +// subscriptionSyncCounter keeps a record of the Prometheus counters emitted by +// Subscription objects. The key of a record is the Subscription name, while the value +// is struct containing label values used in the counter. Read and Write access are +// protected by mutex. +type subscriptionSyncCounter struct { + counters map[string]subscriptionSyncLabelValues + countersLock sync.RWMutex +} + +func newSubscriptionSyncCounter() subscriptionSyncCounter { + return subscriptionSyncCounter{ + counters: make(map[string]subscriptionSyncLabelValues), + } +} + +func (s *subscriptionSyncCounter) setValues(key string, val subscriptionSyncLabelValues) { + s.countersLock.Lock() + defer s.countersLock.Unlock() + s.counters[key] = val +} + +func (s *subscriptionSyncCounter) readValues(key string) (subscriptionSyncLabelValues, bool) { + s.countersLock.RLock() + defer s.countersLock.RUnlock() + val, ok := s.counters[key] + return val, ok +} + type subscriptionSyncLabelValues struct { installedCSV string pkg string @@ -280,14 +306,15 @@ func EmitSubMetric(sub *operatorsv1alpha1.Subscription) { if sub.Spec == nil { return } + SubscriptionSyncCount.WithLabelValues(sub.GetName(), sub.Status.InstalledCSV, sub.Spec.Channel, sub.Spec.Package, string(sub.Spec.InstallPlanApproval)).Inc() - if _, present := subscriptionSyncCounters[sub.GetName()]; !present { - subscriptionSyncCounters[sub.GetName()] = subscriptionSyncLabelValues{ + if _, present := subscriptionSyncCounters.readValues(sub.GetName()); !present { + subscriptionSyncCounters.setValues(sub.GetName(), subscriptionSyncLabelValues{ installedCSV: sub.Status.InstalledCSV, pkg: sub.Spec.Package, channel: sub.Spec.Channel, approvalStrategy: string(sub.Spec.InstallPlanApproval), - } + }) } } @@ -302,7 +329,7 @@ func UpdateSubsSyncCounterStorage(sub *operatorsv1alpha1.Subscription) { if sub.Spec == nil { return } - counterValues := subscriptionSyncCounters[sub.GetName()] + counterValues, _ := subscriptionSyncCounters.readValues(sub.GetName()) approvalStrategy := string(sub.Spec.InstallPlanApproval) if sub.Spec.Channel != counterValues.channel || @@ -317,7 +344,7 @@ func UpdateSubsSyncCounterStorage(sub *operatorsv1alpha1.Subscription) { counterValues.channel = sub.Spec.Channel counterValues.approvalStrategy = approvalStrategy - subscriptionSyncCounters[sub.GetName()] = counterValues + subscriptionSyncCounters.setValues(sub.GetName(), counterValues) } } From 79a0fff9cd0276f7a3e13576c0c783496d30e8ec Mon Sep 17 00:00:00 2001 From: dtfranz Date: Mon, 16 Jan 2023 14:07:21 -0800 Subject: [PATCH 2/3] update vendoring Signed-off-by: dtfranz --- .../pkg/metrics/metrics.go | 45 +++++++++++++++---- 1 file changed, 36 insertions(+), 9 deletions(-) diff --git a/vendor/github.com/operator-framework/operator-lifecycle-manager/pkg/metrics/metrics.go b/vendor/github.com/operator-framework/operator-lifecycle-manager/pkg/metrics/metrics.go index 4bb2f4ae8a..7512d87f72 100644 --- a/vendor/github.com/operator-framework/operator-lifecycle-manager/pkg/metrics/metrics.go +++ b/vendor/github.com/operator-framework/operator-lifecycle-manager/pkg/metrics/metrics.go @@ -1,6 +1,7 @@ package metrics import ( + "sync" "time" "github.com/prometheus/client_golang/prometheus" @@ -199,12 +200,37 @@ var ( }, ) - // subscriptionSyncCounters keeps a record of the Prometheus counters emitted by - // Subscription objects. The key of a record is the Subscription name, while the value - // is struct containing label values used in the counter - subscriptionSyncCounters = make(map[string]subscriptionSyncLabelValues) + subscriptionSyncCounters = newSubscriptionSyncCounter() ) +// subscriptionSyncCounter keeps a record of the Prometheus counters emitted by +// Subscription objects. The key of a record is the Subscription name, while the value +// is struct containing label values used in the counter. Read and Write access are +// protected by mutex. +type subscriptionSyncCounter struct { + counters map[string]subscriptionSyncLabelValues + countersLock sync.RWMutex +} + +func newSubscriptionSyncCounter() subscriptionSyncCounter { + return subscriptionSyncCounter{ + counters: make(map[string]subscriptionSyncLabelValues), + } +} + +func (s *subscriptionSyncCounter) setValues(key string, val subscriptionSyncLabelValues) { + s.countersLock.Lock() + defer s.countersLock.Unlock() + s.counters[key] = val +} + +func (s *subscriptionSyncCounter) readValues(key string) (subscriptionSyncLabelValues, bool) { + s.countersLock.RLock() + defer s.countersLock.RUnlock() + val, ok := s.counters[key] + return val, ok +} + type subscriptionSyncLabelValues struct { installedCSV string pkg string @@ -280,14 +306,15 @@ func EmitSubMetric(sub *operatorsv1alpha1.Subscription) { if sub.Spec == nil { return } + SubscriptionSyncCount.WithLabelValues(sub.GetName(), sub.Status.InstalledCSV, sub.Spec.Channel, sub.Spec.Package, string(sub.Spec.InstallPlanApproval)).Inc() - if _, present := subscriptionSyncCounters[sub.GetName()]; !present { - subscriptionSyncCounters[sub.GetName()] = subscriptionSyncLabelValues{ + if _, present := subscriptionSyncCounters.readValues(sub.GetName()); !present { + subscriptionSyncCounters.setValues(sub.GetName(), subscriptionSyncLabelValues{ installedCSV: sub.Status.InstalledCSV, pkg: sub.Spec.Package, channel: sub.Spec.Channel, approvalStrategy: string(sub.Spec.InstallPlanApproval), - } + }) } } @@ -302,7 +329,7 @@ func UpdateSubsSyncCounterStorage(sub *operatorsv1alpha1.Subscription) { if sub.Spec == nil { return } - counterValues := subscriptionSyncCounters[sub.GetName()] + counterValues, _ := subscriptionSyncCounters.readValues(sub.GetName()) approvalStrategy := string(sub.Spec.InstallPlanApproval) if sub.Spec.Channel != counterValues.channel || @@ -317,7 +344,7 @@ func UpdateSubsSyncCounterStorage(sub *operatorsv1alpha1.Subscription) { counterValues.channel = sub.Spec.Channel counterValues.approvalStrategy = approvalStrategy - subscriptionSyncCounters[sub.GetName()] = counterValues + subscriptionSyncCounters.setValues(sub.GetName(), counterValues) } } From 52d14cef1b705067ce4f569e3a3660ab19d085c0 Mon Sep 17 00:00:00 2001 From: Alexander Greene Date: Thu, 2 Feb 2023 10:13:08 -0800 Subject: [PATCH 3/3] Thread Safety test for UpdateSubsSyncCounterStorage (#2918) Introduces a test that ensure that the UpdateSubsSyncCounterStorage function is thread safe and avoids concurrent map writes. Signed-off-by: Alexander Greene Upstream-repository: operator-lifecycle-manager Upstream-commit: ed4444cfa87fef85a70314ca7c55024a287bc51a --- .../pkg/metrics/metrics_test.go | 34 +++++++++++++++++++ 1 file changed, 34 insertions(+) create mode 100644 staging/operator-lifecycle-manager/pkg/metrics/metrics_test.go diff --git a/staging/operator-lifecycle-manager/pkg/metrics/metrics_test.go b/staging/operator-lifecycle-manager/pkg/metrics/metrics_test.go new file mode 100644 index 0000000000..5987a38f21 --- /dev/null +++ b/staging/operator-lifecycle-manager/pkg/metrics/metrics_test.go @@ -0,0 +1,34 @@ +package metrics_test + +import ( + "fmt" + "testing" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + operatorsv1alpha1 "github.com/operator-framework/api/pkg/operators/v1alpha1" + "github.com/operator-framework/operator-lifecycle-manager/pkg/metrics" +) + +func TestUpdateSubsSyncCounterStorageThreadSafety(t *testing.T) { + for i := 0; i < 1000; i++ { + go func(ii int) { + sub := &operatorsv1alpha1.Subscription{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "foo", + Name: "foo", + }, + Spec: &operatorsv1alpha1.SubscriptionSpec{ + Channel: "foo", + Package: "foo", + InstallPlanApproval: "automatic", + }, + Status: operatorsv1alpha1.SubscriptionStatus{ + InstalledCSV: "foo", + }, + } + sub.Spec.Channel = fmt.Sprintf("bar-%v", ii) + metrics.UpdateSubsSyncCounterStorage(sub) + }(i) + } +}