Merge pull request #440 from awgreene/429-to-release-4.12

[release-4.12] OCPBUGS-6260: Catalog, fatal error: concurrent map read and map write

Author: openshift-merge-robot

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)
 	}
 }
 

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)
+	}
+}