From 1d1677ab30ff30d27c00e768a7e871c306ce28fb Mon Sep 17 00:00:00 2001 From: Daniel Franz Date: Mon, 9 Jan 2023 14:24:17 -0800 Subject: [PATCH 1/4] protected subscriptionSyncCounters access to prevent concurrent map writes Signed-off-by: Daniel Franz --- pkg/metrics/metrics.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/pkg/metrics/metrics.go b/pkg/metrics/metrics.go index 4bb2f4ae8a..ca04be761d 100644 --- a/pkg/metrics/metrics.go +++ b/pkg/metrics/metrics.go @@ -1,6 +1,7 @@ package metrics import ( + "sync" "time" "github.com/prometheus/client_golang/prometheus" @@ -203,6 +204,8 @@ var ( // 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) + + subscriptionSyncCountersLock sync.Mutex ) type subscriptionSyncLabelValues struct { @@ -280,6 +283,8 @@ func EmitSubMetric(sub *operatorsv1alpha1.Subscription) { if sub.Spec == nil { return } + subscriptionSyncCountersLock.Lock() + defer subscriptionSyncCountersLock.Unlock() 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{ @@ -302,6 +307,8 @@ func UpdateSubsSyncCounterStorage(sub *operatorsv1alpha1.Subscription) { if sub.Spec == nil { return } + subscriptionSyncCountersLock.Lock() + defer subscriptionSyncCountersLock.Unlock() counterValues := subscriptionSyncCounters[sub.GetName()] approvalStrategy := string(sub.Spec.InstallPlanApproval) From d783a030d4fc7bc7ff9d1680577b8a67409ac433 Mon Sep 17 00:00:00 2001 From: Daniel Franz Date: Tue, 10 Jan 2023 13:24:28 -0800 Subject: [PATCH 2/4] organize map and mutex into single struct Signed-off-by: Daniel Franz --- pkg/metrics/metrics.go | 44 ++++++++++++++++++++++++++++-------------- 1 file changed, 29 insertions(+), 15 deletions(-) diff --git a/pkg/metrics/metrics.go b/pkg/metrics/metrics.go index ca04be761d..df64e2d32e 100644 --- a/pkg/metrics/metrics.go +++ b/pkg/metrics/metrics.go @@ -200,14 +200,31 @@ 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) - - subscriptionSyncCountersLock sync.Mutex + subscriptionSyncCounters subscriptionSync ) +// 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. Read and Write access are +// protected by mutex. +type subscriptionSync struct { + counters map[string]subscriptionSyncLabelValues + countersLock sync.Mutex +} + +func (s *subscriptionSync) setValues(key string, val subscriptionSyncLabelValues) { + s.countersLock.Lock() + defer s.countersLock.Unlock() + s.counters[key] = val +} + +func (s *subscriptionSync) readValues(key string) (subscriptionSyncLabelValues, bool) { + s.countersLock.Lock() + defer s.countersLock.Unlock() + val, ok := s.counters[key] + return val, ok +} + type subscriptionSyncLabelValues struct { installedCSV string pkg string @@ -283,16 +300,15 @@ func EmitSubMetric(sub *operatorsv1alpha1.Subscription) { if sub.Spec == nil { return } - subscriptionSyncCountersLock.Lock() - defer subscriptionSyncCountersLock.Unlock() + 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), - } + }) } } @@ -307,9 +323,7 @@ func UpdateSubsSyncCounterStorage(sub *operatorsv1alpha1.Subscription) { if sub.Spec == nil { return } - subscriptionSyncCountersLock.Lock() - defer subscriptionSyncCountersLock.Unlock() - counterValues := subscriptionSyncCounters[sub.GetName()] + counterValues, _ := subscriptionSyncCounters.readValues(sub.GetName()) approvalStrategy := string(sub.Spec.InstallPlanApproval) if sub.Spec.Channel != counterValues.channel || @@ -324,7 +338,7 @@ func UpdateSubsSyncCounterStorage(sub *operatorsv1alpha1.Subscription) { counterValues.channel = sub.Spec.Channel counterValues.approvalStrategy = approvalStrategy - subscriptionSyncCounters[sub.GetName()] = counterValues + subscriptionSyncCounters.setValues(sub.GetName(), counterValues) } } From 720036f5848caead562a000cb02d5b361af53317 Mon Sep 17 00:00:00 2001 From: Daniel Franz Date: Tue, 10 Jan 2023 14:18:41 -0800 Subject: [PATCH 3/4] initialize struct Signed-off-by: Daniel Franz --- pkg/metrics/metrics.go | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/pkg/metrics/metrics.go b/pkg/metrics/metrics.go index df64e2d32e..ccd9dd54a8 100644 --- a/pkg/metrics/metrics.go +++ b/pkg/metrics/metrics.go @@ -200,25 +200,31 @@ var ( }, ) - subscriptionSyncCounters subscriptionSync + subscriptionSyncCounters = newSubscriptionSyncCounter() ) -// subscriptionSyncCounters keeps a record of the Prometheus counters emitted by +// 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 subscriptionSync struct { +type subscriptionSyncCounter struct { counters map[string]subscriptionSyncLabelValues countersLock sync.Mutex } -func (s *subscriptionSync) setValues(key string, val subscriptionSyncLabelValues) { +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 *subscriptionSync) readValues(key string) (subscriptionSyncLabelValues, bool) { +func (s *subscriptionSyncCounter) readValues(key string) (subscriptionSyncLabelValues, bool) { s.countersLock.Lock() defer s.countersLock.Unlock() val, ok := s.counters[key] From e0b794d186159a74cfd92421267fa2ce03f236b0 Mon Sep 17 00:00:00 2001 From: Daniel Franz Date: Thu, 12 Jan 2023 09:54:40 -0800 Subject: [PATCH 4/4] use RWMutex to allow concurrent reads Signed-off-by: Daniel Franz --- pkg/metrics/metrics.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/metrics/metrics.go b/pkg/metrics/metrics.go index ccd9dd54a8..7512d87f72 100644 --- a/pkg/metrics/metrics.go +++ b/pkg/metrics/metrics.go @@ -209,7 +209,7 @@ var ( // protected by mutex. type subscriptionSyncCounter struct { counters map[string]subscriptionSyncLabelValues - countersLock sync.Mutex + countersLock sync.RWMutex } func newSubscriptionSyncCounter() subscriptionSyncCounter { @@ -225,8 +225,8 @@ func (s *subscriptionSyncCounter) setValues(key string, val subscriptionSyncLabe } func (s *subscriptionSyncCounter) readValues(key string) (subscriptionSyncLabelValues, bool) { - s.countersLock.Lock() - defer s.countersLock.Unlock() + s.countersLock.RLock() + defer s.countersLock.RUnlock() val, ok := s.counters[key] return val, ok }