From 389142efb03b3d20be6d475a912948c226d5ef0e Mon Sep 17 00:00:00 2001 From: Daniel Franz Date: Mon, 16 Jan 2023 09:50:01 -0800 Subject: [PATCH 1/2] 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/2] 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) } }