Bug 1850237: Delete subscription metric when an operator is uninstalled

pkg/controller/operators/catalog/subscription/syncer.go

@@ -50,7 +50,7 @@ func (s *subscriptionSyncer) Sync(ctx context.Context, event kubestate.ResourceE
 		return err
 	}
 
-	s.recordMetrics(res)
+	metrics.EmitSubMetric(res)
 
 	logger := s.logger.WithFields(logrus.Fields{
 		"reconciling": fmt.Sprintf("%T", res),
@@ -68,8 +68,10 @@ func (s *subscriptionSyncer) Sync(ctx context.Context, event kubestate.ResourceE
 		initial = initial.Add()
 	case kubestate.ResourceUpdated:
 		initial = initial.Update()
+		metrics.UpdateSubsSyncCounterStorage(res)
 	case kubestate.ResourceDeleted:
 		initial = initial.Delete()
+		metrics.DeleteSubsMetric(res)
 	}
 
 	reconciled, err := s.reconcilers.Reconcile(ctx, initial)
@@ -85,15 +87,6 @@ func (s *subscriptionSyncer) Sync(ctx context.Context, event kubestate.ResourceE
 	return nil
 }
 
-func (s *subscriptionSyncer) recordMetrics(sub *v1alpha1.Subscription) {
-	// sub.Spec is not a required field.
-	if sub.Spec == nil {
-		return
-	}
-
-	metrics.CounterForSubscription(sub.GetName(), sub.Status.InstalledCSV, sub.Spec.Channel, sub.Spec.Package).Inc()
-}
-
 func (s *subscriptionSyncer) Notify(event kubestate.ResourceEvent) {
 	s.notify(event)
 }

pkg/controller/operators/catalog/subscription/syncer_test.go

@@ -6,11 +6,19 @@ import (
 
 	"github.com/sirupsen/logrus"
 	"github.com/stretchr/testify/require"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 
 	"github.com/operator-framework/api/pkg/operators/v1alpha1"
 	"github.com/operator-framework/operator-lifecycle-manager/pkg/lib/kubestate"
 )
 
+const (
+	name             = "test-subscription"
+	packageName      = "test-package"
+	channel          = "test-channel"
+	installedCSVName = "test-csv"
+)
+
 func TestSync(t *testing.T) {
 	type fields struct {
 		syncer kubestate.Syncer
@@ -39,7 +47,22 @@ func TestSync(t *testing.T) {
 			args: args{
 				event: kubestate.NewResourceEvent(
 					kubestate.ResourceAdded,
-					&v1alpha1.Subscription{},
+					&v1alpha1.Subscription{
+						TypeMeta: metav1.TypeMeta{
+							Kind:       v1alpha1.SubscriptionKind,
+							APIVersion: v1alpha1.SubscriptionCRDAPIVersion,
+						},
+						ObjectMeta: metav1.ObjectMeta{
+							Name: name,
+						},
+						Spec: &v1alpha1.SubscriptionSpec{
+							Package: packageName,
+							Channel: channel,
+						},
+						Status: v1alpha1.SubscriptionStatus{
+							InstalledCSV: installedCSVName,
+						},
+					},
 				),
 			},
 			want: want{

pkg/metrics/metrics.go

@@ -2,6 +2,7 @@ package metrics
 
 import (
 	"context"
+
 	"github.com/prometheus/client_golang/prometheus"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/labels"
@@ -168,8 +169,19 @@ var (
 		},
 		[]string{NAMESPACE_LABEL, NAME_LABEL, VERSION_LABEL, PHASE_LABEL, REASON_LABEL},
 	)
+
+	// subscriptionSyncCounters keeps a record of the promethues 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)
 )
 
+type subscriptionSyncLabelValues struct {
+	installedCSV string
+	pkg          string
+	channel      string
+}
+
 func RegisterOLM() {
 	prometheus.MustRegister(csvCount)
 	prometheus.MustRegister(csvSucceeded)
@@ -217,3 +229,45 @@ func EmitCSVMetric(oldCSV *olmv1alpha1.ClusterServiceVersion, newCSV *olmv1alpha
 		csvAbnormal.WithLabelValues(newCSV.Namespace, newCSV.Name, newCSV.Spec.Version.String(), string(newCSV.Status.Phase), string(newCSV.Status.Reason)).Set(1)
 	}
 }
+
+func EmitSubMetric(sub *olmv1alpha1.Subscription) {
+	if sub.Spec == nil {
+		return
+	}
+	SubscriptionSyncCount.WithLabelValues(sub.GetName(), sub.Status.InstalledCSV, sub.Spec.Channel, sub.Spec.Package).Inc()
+	if _, present := subscriptionSyncCounters[sub.GetName()]; !present {
+		subscriptionSyncCounters[sub.GetName()] = subscriptionSyncLabelValues{
+			installedCSV: sub.Status.InstalledCSV,
+			pkg:          sub.Spec.Package,
+			channel:      sub.Spec.Channel,
+		}
+	}
+}
+
+func DeleteSubsMetric(sub *olmv1alpha1.Subscription) {
+	if sub.Spec == nil {
+		return
+	}
+	SubscriptionSyncCount.DeleteLabelValues(sub.GetName(), sub.Status.InstalledCSV, sub.Spec.Channel, sub.Spec.Package)
+}
+
+func UpdateSubsSyncCounterStorage(sub *olmv1alpha1.Subscription) {
+	if sub.Spec == nil {
+		return
+	}
+	counterValues := subscriptionSyncCounters[sub.GetName()]
+
+	if sub.Spec.Channel != counterValues.channel ||
+		sub.Spec.Package != counterValues.pkg ||
+		sub.Status.InstalledCSV != counterValues.installedCSV {
+
+		// Delete metric will label values of old Subscription first
+		SubscriptionSyncCount.DeleteLabelValues(sub.GetName(), counterValues.installedCSV, counterValues.channel, counterValues.pkg)
+
+		counterValues.installedCSV = sub.Status.InstalledCSV
+		counterValues.pkg = sub.Spec.Package
+		counterValues.channel = sub.Spec.Channel
+
+		subscriptionSyncCounters[sub.GetName()] = counterValues
+	}
+}

test/e2e/metrics_e2e_test.go

@@ -6,6 +6,7 @@ import (
 	"context"
 	"fmt"
 	"regexp"
+	"time"
 
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
@@ -15,61 +16,172 @@ import (
 	"k8s.io/apimachinery/pkg/util/net"
 
 	"github.com/operator-framework/api/pkg/operators/v1alpha1"
+	"github.com/operator-framework/operator-lifecycle-manager/pkg/api/client/clientset/versioned"
 	"github.com/operator-framework/operator-lifecycle-manager/pkg/lib/operatorclient"
+	"github.com/operator-framework/operator-lifecycle-manager/pkg/metrics"
 )
 
 var _ = Describe("Metrics", func() {
-	It("endpoint", func() {
-
-		// TestMetrics tests the metrics endpoint of the OLM pod.
-
-		c := newKubeClient()
-		crc := newCRClient()
-
-		failingCSV := v1alpha1.ClusterServiceVersion{
-			TypeMeta: metav1.TypeMeta{
-				Kind:       v1alpha1.ClusterServiceVersionKind,
-				APIVersion: v1alpha1.ClusterServiceVersionAPIVersion,
-			},
-			ObjectMeta: metav1.ObjectMeta{
-				Name: genName("failing-csv-test-"),
-			},
-			Spec: v1alpha1.ClusterServiceVersionSpec{
-				InstallStrategy: v1alpha1.NamedInstallStrategy{
-					StrategyName: v1alpha1.InstallStrategyNameDeployment,
-					StrategySpec: strategy,
-				},
-			},
-		}
 
-		cleanupCSV, err := createCSV(GinkgoT(), c, crc, failingCSV, testNamespace, false, false)
-		Expect(err).ToNot(HaveOccurred())
+	var (
+		c   operatorclient.ClientInterface
+		crc versioned.Interface
+	)
 
-		_, err = fetchCSV(GinkgoT(), crc, failingCSV.Name, testNamespace, csvFailedChecker)
-		Expect(err).ToNot(HaveOccurred())
+	BeforeEach(func() {
+		c = newKubeClient()
+		crc = newCRClient()
+
+	})
+
+	Context("CSV metrics", func() {
+		It("endpoint", func() {
+
+			// TestMetrics tests the metrics endpoint of the OLM pod.
+
+			failingCSV := v1alpha1.ClusterServiceVersion{
+				TypeMeta: metav1.TypeMeta{
+					Kind:       v1alpha1.ClusterServiceVersionKind,
+					APIVersion: v1alpha1.ClusterServiceVersionAPIVersion,
+				},
+				ObjectMeta: metav1.ObjectMeta{
+					Name: genName("failing-csv-test-"),
+				},
+				Spec: v1alpha1.ClusterServiceVersionSpec{
+					InstallStrategy: v1alpha1.NamedInstallStrategy{
+						StrategyName: v1alpha1.InstallStrategyNameDeployment,
+						StrategySpec: strategy,
+					},
+				},
+			}
+
+			cleanupCSV, err := createCSV(GinkgoT(), c, crc, failingCSV, testNamespace, false, false)
+			Expect(err).ToNot(HaveOccurred())
+
+			_, err = fetchCSV(GinkgoT(), crc, failingCSV.Name, testNamespace, csvFailedChecker)
+			Expect(err).ToNot(HaveOccurred())
+
+			// Verify metrics have been emitted for packageserver csv
+			Expect(getMetricsFromPod(c, getPodWithLabel(c, "app=olm-operator"), "8081")).To(And(
+				ContainSubstring("csv_abnormal"),
+				ContainSubstring(fmt.Sprintf("name=\"%s\"", failingCSV.Name)),
+				ContainSubstring("phase=\"Failed\""),
+				ContainSubstring("reason=\"UnsupportedOperatorGroup\""),
+				ContainSubstring("version=\"0.0.0\""),
+				ContainSubstring("csv_succeeded"),
+			))
+
+			cleanupCSV()
+
+			// Verify that when the csv has been deleted, it deletes the corresponding CSV metrics
+			Expect(getMetricsFromPod(c, getPodWithLabel(c, "app=olm-operator"), "8081")).ToNot(And(
+				ContainSubstring("csv_abnormal{name=\"%s\"", failingCSV.Name),
+				ContainSubstring("csv_succeeded{name=\"%s\"", failingCSV.Name),
+			))
+		})
+	})
 
-		// Verify metrics have been emitted for packageserver csv
-		Expect(getMetricsFromPod(c, getOLMPod(c), "8081")).To(And(
-			ContainSubstring("csv_abnormal"),
-			ContainSubstring(fmt.Sprintf("name=\"%s\"", failingCSV.Name)),
-			ContainSubstring("phase=\"Failed\""),
-			ContainSubstring("reason=\"UnsupportedOperatorGroup\""),
-			ContainSubstring("version=\"0.0.0\""),
-			ContainSubstring("csv_succeeded"),
-		))
-
-		cleanupCSV()
-
-		// Verify that when the csv has been deleted, it deletes the corresponding CSV metrics
-		Expect(getMetricsFromPod(c, getOLMPod(c), "8081")).ToNot(And(
-			ContainSubstring("csv_abnormal{name=\"%s\"", failingCSV.Name),
-			ContainSubstring("csv_succeeded{name=\"%s\"", failingCSV.Name),
-		))
+	Context("Subscription Metric", func() {
+		var (
+			subscriptionCleanup cleanupFunc
+			subscription        *v1alpha1.Subscription
+		)
+		When("A subscription object is created", func() {
+
+			BeforeEach(func() {
+				subscriptionCleanup, subscription = createSubscription(GinkgoT(), crc, testNamespace, "metric-subscription", testPackageName, stableChannel, v1alpha1.ApprovalManual)
+			})
+
+			It("generates subscription_sync_total metric", func() {
+
+				// Verify metrics have been emitted for subscription
+				Eventually(func() string {
+					return getMetricsFromPod(c, getPodWithLabel(c, "app=catalog-operator"), "8081")
+				}, time.Minute, 5*time.Second).Should(And(
+					ContainSubstring("subscription_sync_total"),
+					ContainSubstring(fmt.Sprintf("%s=\"%s\"", metrics.NAME_LABEL, "metric-subscription")),
+					ContainSubstring(fmt.Sprintf("%s=\"%s\"", metrics.CHANNEL_LABEL, stableChannel)),
+					ContainSubstring(fmt.Sprintf("%s=\"%s\"", metrics.PACKAGE_LABEL, testPackageName))))
+			})
+
+			When("The subscription object is updated", func() {
+
+				BeforeEach(func() {
+					updatedSubscription, err := crc.OperatorsV1alpha1().Subscriptions(subscription.GetNamespace()).Get(context.TODO(), subscription.GetName(), metav1.GetOptions{})
+					Expect(err).ToNot(HaveOccurred())
+					updatedSubscription.Spec.Channel = betaChannel
+					updateSubscription(GinkgoT(), crc, updatedSubscription)
+				})
+
+				It("deletes the old Subscription metric and emits the new metric", func() {
+					Eventually(func() string {
+						return getMetricsFromPod(c, getPodWithLabel(c, "app=catalog-operator"), "8081")
+					}, time.Minute, 5*time.Second).ShouldNot(And(
+						ContainSubstring("subscription_sync_total{name=\"metric-subscription\""),
+						ContainSubstring(fmt.Sprintf("%s=\"%s\"", metrics.CHANNEL_LABEL, stableChannel))))
+
+					Eventually(func() string {
+						return getMetricsFromPod(c, getPodWithLabel(c, "app=catalog-operator"), "8081")
+					}, time.Minute, 5*time.Second).Should(And(
+						ContainSubstring("subscription_sync_total"),
+						ContainSubstring(fmt.Sprintf("%s=\"%s\"", metrics.NAME_LABEL, "metric-subscription")),
+						ContainSubstring(fmt.Sprintf("%s=\"%s\"", metrics.CHANNEL_LABEL, betaChannel)),
+						ContainSubstring(fmt.Sprintf("%s=\"%s\"", metrics.PACKAGE_LABEL, testPackageName))))
+				})
+
+				When("The Subscription object is updated again", func() {
+
+					BeforeEach(func() {
+						updatedSubscription, err := crc.OperatorsV1alpha1().Subscriptions(subscription.GetNamespace()).Get(context.TODO(), subscription.GetName(), metav1.GetOptions{})
+						Expect(err).ToNot(HaveOccurred())
+						updatedSubscription.Spec.Channel = alphaChannel
+						updateSubscription(GinkgoT(), crc, updatedSubscription)
+					})
+
+					It("deletes the old subscription metric and emits the new metric(there is only one metric for the subscription)", func() {
+						Eventually(func() string {
+							return getMetricsFromPod(c, getPodWithLabel(c, "app=catalog-operator"), "8081")
+						}, time.Minute, 5*time.Second).ShouldNot(And(
+							ContainSubstring("subscription_sync_total{name=\"metric-subscription-for-update\""),
+							ContainSubstring(fmt.Sprintf("%s=\"%s\"", metrics.CHANNEL_LABEL, stableChannel)),
+							ContainSubstring(fmt.Sprintf("%s=\"%s\"", metrics.CHANNEL_LABEL, betaChannel))))
+
+						Eventually(func() string {
+							return getMetricsFromPod(c, getPodWithLabel(c, "app=catalog-operator"), "8081")
+						}, time.Minute, 5*time.Second).Should(And(
+							ContainSubstring("subscription_sync_total"),
+							ContainSubstring(fmt.Sprintf("%s=\"%s\"", metrics.NAME_LABEL, "metric-subscription")),
+							ContainSubstring(fmt.Sprintf("%s=\"%s\"", metrics.CHANNEL_LABEL, alphaChannel)),
+							ContainSubstring(fmt.Sprintf("%s=\"%s\"", metrics.PACKAGE_LABEL, testPackageName))))
+					})
+				})
+			})
+
+			AfterEach(func() {
+				if subscriptionCleanup != nil {
+					subscriptionCleanup()
+				}
+			})
+		})
+		When("A Subscription object is deleted", func() {
+
+			BeforeEach(func() {
+				subscriptionCleanup, _ = createSubscription(GinkgoT(), crc, testNamespace, "metric-subscription-for-deletion", testPackageName, stableChannel, v1alpha1.ApprovalManual)
+				if subscriptionCleanup != nil {
+					subscriptionCleanup()
+				}
+			})
+			It("deletes the Subscription metric", func() {
+				Eventually(func() string {
+					return getMetricsFromPod(c, getPodWithLabel(c, "app=catalog-operator"), "8081")
+				}, time.Minute, 5*time.Second).ShouldNot(ContainSubstring("subscription_sync_total{name=\"metric-subscription-for-deletion\""))
+			})
+		})
 	})
 })
 
-func getOLMPod(client operatorclient.ClientInterface) *corev1.Pod {
-	listOptions := metav1.ListOptions{LabelSelector: "app=olm-operator"}
+func getPodWithLabel(client operatorclient.ClientInterface, label string) *corev1.Pod {
+	listOptions := metav1.ListOptions{LabelSelector: label}
 	var podList *corev1.PodList
 	Eventually(func() (err error) {
 		podList, err = client.KubernetesInterface().CoreV1().Pods(operatorNamespace).List(context.TODO(), listOptions)