Skip to content

Bug 1822396: Delete subscription metric when an operator is uninstalled #1519

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jun 19, 2020
Merged

Bug 1822396: Delete subscription metric when an operator is uninstalled #1519

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 3 additions & 10 deletions pkg/controller/operators/catalog/subscription/syncer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand All @@ -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)
Expand All @@ -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)
}
Expand Down
52 changes: 52 additions & 0 deletions pkg/metrics/metrics.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -217,3 +229,43 @@ 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()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason the namespace isn't included? Since it's possible to have two subs with the same name, there's potential for collision.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@njhale I see your point. If we have consensus that this PR will be a good place to include the namespace label here I don't mind adding it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can potentially fix this in a separate bug.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

spec is not a required field, so this would panic without

	if sub.Spec == nil {
		return
	}

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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The should check for nil on sub.status and sub.spec

}

func UpdateSubsSyncCounterStorage(sub *olmv1alpha1.Subscription) {
if sub.Spec == nil {
return
}
counterValues := subscriptionSyncCounters[sub.GetName()]

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sub.spec and sub.status can be nil

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
}
}
86 changes: 80 additions & 6 deletions test/e2e/metrics_e2e_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,20 +5,23 @@ package e2e
import (
"context"
"fmt"
"regexp"
"time"

. "github.com/onsi/ginkgo"
. "github.com/onsi/gomega"
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/util/net"
"regexp"

"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"
"github.com/operator-framework/operator-lifecycle-manager/test/e2e/ctx"
)

var _ = Describe("Metrics are generated for OLM pod", func() {
var _ = Describe("Metrics are generated for OLM managed resources", func() {

var (
c operatorclient.ClientInterface
Expand Down Expand Up @@ -69,7 +72,7 @@ var _ = Describe("Metrics are generated for OLM pod", func() {
It("generates csv_abnormal metric for OLM pod", func() {

// Verify metrics have been emitted for packageserver csv
Expect(getMetricsFromPod(c, getOLMPod(c), "8081")).To(And(
Expect(getMetricsFromPod(c, getPodWithLabel(c, "app=olm-operator"), "8081")).To(And(
ContainSubstring("csv_abnormal"),
ContainSubstring(fmt.Sprintf("name=\"%s\"", failingCSV.Name)),
ContainSubstring("phase=\"Failed\""),
Expand All @@ -91,18 +94,89 @@ var _ = Describe("Metrics are generated for OLM pod", func() {

It("deletes its associated CSV metrics", func() {
// Verify that when the csv has been deleted, it deletes the corresponding CSV metrics
Expect(getMetricsFromPod(c, getOLMPod(c), "8081")).ToNot(And(
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),
))
})
})
})
})

Context("Subscription Metric", func() {
var (
subscriptionCleanup cleanupFunc
subscription *v1alpha1.Subscription
)
When("A subscription object is created", func() {

BeforeEach(func() {
subscriptionCleanup, _ = createSubscription(GinkgoT(), crc, testNamespace, "metric-subscription-for-create", 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-for-create")),
ContainSubstring(fmt.Sprintf("%s=\"%s\"", metrics.CHANNEL_LABEL, stableChannel)),
ContainSubstring(fmt.Sprintf("%s=\"%s\"", metrics.PACKAGE_LABEL, testPackageName))))
})
if subscriptionCleanup != nil {
subscriptionCleanup()
}
})
When("A subscription object is updated", func() {

BeforeEach(func() {
subscriptionCleanup, subscription = createSubscription(GinkgoT(), crc, testNamespace, "metric-subscription-for-update", testPackageName, stableChannel, v1alpha1.ApprovalManual)
subscription.Spec.Channel = "beta"
updateSubscription(GinkgoT(), crc, subscription)
})

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-for-update\""),
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-for-update")),
ContainSubstring(fmt.Sprintf("%s=\"%s\"", metrics.CHANNEL_LABEL, "beta")),
ContainSubstring(fmt.Sprintf("%s=\"%s\"", metrics.PACKAGE_LABEL, testPackageName))))
})
if subscriptionCleanup != nil {
subscriptionCleanup()
}
})

When("A subscription object is deleted", func() {

BeforeEach(func() {
subscriptionCleanup, subscription = createSubscription(GinkgoT(), crc, testNamespace, "metric-subscription-for-delete", 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-update\""))
})
})
})
})

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)
Expand Down
17 changes: 11 additions & 6 deletions test/e2e/subscription_e2e_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ var _ = Describe("Subscription", func() {
}()
require.NoError(GinkgoT(), initCatalog(GinkgoT(), c, crc))

cleanup := createSubscription(GinkgoT(), crc, testNamespace, testSubscriptionName, testPackageName, betaChannel, v1alpha1.ApprovalAutomatic)
cleanup, _ := createSubscription(GinkgoT(), crc, testNamespace, testSubscriptionName, testPackageName, betaChannel, v1alpha1.ApprovalAutomatic)
defer cleanup()

subscription, err := fetchSubscription(crc, testNamespace, testSubscriptionName, subscriptionStateAtLatestChecker)
Expand Down Expand Up @@ -80,7 +80,7 @@ var _ = Describe("Subscription", func() {
_, err := createCSV(c, crc, stableCSV, testNamespace, false, false)
require.NoError(GinkgoT(), err)

subscriptionCleanup := createSubscription(GinkgoT(), crc, testNamespace, testSubscriptionName, testPackageName, alphaChannel, v1alpha1.ApprovalAutomatic)
subscriptionCleanup, _ := createSubscription(GinkgoT(), crc, testNamespace, testSubscriptionName, testPackageName, alphaChannel, v1alpha1.ApprovalAutomatic)
defer subscriptionCleanup()

subscription, err := fetchSubscription(crc, testNamespace, testSubscriptionName, subscriptionStateAtLatestChecker)
Expand Down Expand Up @@ -188,7 +188,7 @@ var _ = Describe("Subscription", func() {
}()
require.NoError(GinkgoT(), initCatalog(GinkgoT(), c, crc))

subscriptionCleanup := createSubscription(GinkgoT(), crc, testNamespace, "manual-subscription", testPackageName, stableChannel, v1alpha1.ApprovalManual)
subscriptionCleanup, _ := createSubscription(GinkgoT(), crc, testNamespace, "manual-subscription", testPackageName, stableChannel, v1alpha1.ApprovalManual)
defer subscriptionCleanup()

subscription, err := fetchSubscription(crc, testNamespace, "manual-subscription", subscriptionStateUpgradePendingChecker)
Expand Down Expand Up @@ -1783,7 +1783,7 @@ func buildSubscriptionCleanupFunc(crc versioned.Interface, subscription *v1alpha
}
}

func createSubscription(t GinkgoTInterface, crc versioned.Interface, namespace, name, packageName, channel string, approval v1alpha1.Approval) cleanupFunc {
func createSubscription(t GinkgoTInterface, crc versioned.Interface, namespace, name, packageName, channel string, approval v1alpha1.Approval) (cleanupFunc, *v1alpha1.Subscription) {
subscription := &v1alpha1.Subscription{
TypeMeta: metav1.TypeMeta{
Kind: v1alpha1.SubscriptionKind,
Expand All @@ -1803,8 +1803,13 @@ func createSubscription(t GinkgoTInterface, crc versioned.Interface, namespace,
}

subscription, err := crc.OperatorsV1alpha1().Subscriptions(namespace).Create(context.TODO(), subscription, metav1.CreateOptions{})
require.NoError(t, err)
return buildSubscriptionCleanupFunc(crc, subscription)
Expect(err).ToNot(HaveOccurred())
return buildSubscriptionCleanupFunc(crc, subscription), subscription
}

func updateSubscription(t GinkgoTInterface, crc versioned.Interface, subscription *v1alpha1.Subscription) {
_, err := crc.OperatorsV1alpha1().Subscriptions(subscription.GetNamespace()).Update(context.TODO(), subscription, metav1.UpdateOptions{})
Expect(err).ToNot(HaveOccurred())
}

func createSubscriptionForCatalog(crc versioned.Interface, namespace, name, catalog, packageName, channel, startingCSV string, approval v1alpha1.Approval) cleanupFunc {
Expand Down