From 4b19165d40c1de935c71afb54bef3e77d0872d7c Mon Sep 17 00:00:00 2001 From: Avi Biton Date: Sun, 11 Aug 2024 13:30:25 +0300 Subject: [PATCH] chore(RHTAPWATCH-1207):Add metrics for total and failed notifications - Add metric for total notifications - Add metric for total failed notifications - Add tests Signed-off-by: Avi Biton --- internal/controller/metrics.go | 26 ++ .../notificationservice_controller.go | 2 + .../notificationservice_controller_test.go | 268 ++++++++++++------ internal/controller/suite_test.go | 30 +- 4 files changed, 223 insertions(+), 103 deletions(-) create mode 100644 internal/controller/metrics.go diff --git a/internal/controller/metrics.go b/internal/controller/metrics.go new file mode 100644 index 0000000..a3da0e9 --- /dev/null +++ b/internal/controller/metrics.go @@ -0,0 +1,26 @@ +package controller + +import ( + "github.com/prometheus/client_golang/prometheus" + "sigs.k8s.io/controller-runtime/pkg/metrics" +) + +var ( + notifications = prometheus.NewCounter( + prometheus.CounterOpts{ + Name: "notification_controller_notifications_total", + Help: "Number of total notification actions", + }, + ) + notificationsFailures = prometheus.NewCounter( + prometheus.CounterOpts{ + Name: "notification_controller_notifications_failures_total", + Help: "Number of failed notifications", + }, + ) +) + +func init() { + // Register custom metrics with the global prometheus registry + metrics.Registry.MustRegister(notifications, notificationsFailures) +} diff --git a/internal/controller/notificationservice_controller.go b/internal/controller/notificationservice_controller.go index a724a26..c576b29 100644 --- a/internal/controller/notificationservice_controller.go +++ b/internal/controller/notificationservice_controller.go @@ -78,8 +78,10 @@ func (r *NotificationServiceReconciler) Reconcile(ctx context.Context, req ctrl. return ctrl.Result{}, err } err = r.Notifier.Notify(ctx, string(results)) + notifications.Inc() if err != nil { logger.Error(err, "Failed to Notify") + notificationsFailures.Inc() return ctrl.Result{}, err } logger.Info("SNS Notified", "pipelinerun", pipelineRun.Name, "namespace", pipelineRun.Namespace) diff --git a/internal/controller/notificationservice_controller_test.go b/internal/controller/notificationservice_controller_test.go index 4894042..b93de73 100644 --- a/internal/controller/notificationservice_controller_test.go +++ b/internal/controller/notificationservice_controller_test.go @@ -22,6 +22,7 @@ import ( "github.com/konflux-ci/operator-toolkit/metadata" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" + "github.com/prometheus/client_golang/prometheus/testutil" tektonv1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1" "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -33,7 +34,8 @@ import ( var _ = Describe("NotificationService Controller", func() { var ( - pushPipelineRun, pullRequestPipelineRun *tektonv1.PipelineRun + pushPipelineRun, pullRequestPipelineRun *tektonv1.PipelineRun + notificationsCounter, notificationsFailuresCounter float64 ) const ( timeout = time.Second * 10 @@ -90,120 +92,198 @@ var _ = Describe("NotificationService Controller", func() { err := k8sClient.Get(ctx, pushPipelineRunLookupKey, createdPipelineRun) return err == nil }, timeout, interval).Should(BeTrue()) + + notificationsCounter = testutil.ToFloat64(notifications) + notificationsFailuresCounter = testutil.ToFloat64(notificationsFailures) }) Context("when a push pipelinerun is created and end successfully", func() { - It("should reconcile successfully - Add finalizer, Read the results, add annotation and remove the finalizer", func() { - By("Creating a new push pipelinerun and add finalizer") - - // The pipelinerun should be reconciled and the notification finalizer has been added successfully - Eventually(func() bool { - err := k8sClient.Get(ctx, pushPipelineRunLookupKey, createdPipelineRun) + It("should reconcile successfully - Add finalizer, Read the results, add annotation and remove the finalizer, update metrics", + func() { + By("Creating a new push pipelinerun and add finalizer") + err := nsr.SetupWithManager(k8sManager) Expect(err).ToNot(HaveOccurred()) - return controllerutil.ContainsFinalizer(createdPipelineRun, NotificationPipelineRunFinalizer) - }, timeout, interval).Should(BeTrue()) - Expect(controllerutil.ContainsFinalizer(createdPipelineRun, NotificationPipelineRunFinalizer)).To(BeTrue()) - // Check the Notify was not called - Expect(mn.Counter).To(BeZero()) + // The pipelinerun should be reconciled and the notification finalizer has been added successfully + Eventually(func() bool { + err := k8sClient.Get(ctx, pushPipelineRunLookupKey, createdPipelineRun) + Expect(err).ToNot(HaveOccurred()) + return controllerutil.ContainsFinalizer(createdPipelineRun, NotificationPipelineRunFinalizer) + }, timeout, interval).Should(BeTrue()) + Expect(controllerutil.ContainsFinalizer(createdPipelineRun, NotificationPipelineRunFinalizer)).To(BeTrue()) + // Check the Notify was not called + Expect(mn.Counter).To(BeZero()) - By("Updating status to completed successfully") - createdPipelineRun.Status = tektonv1.PipelineRunStatus{ - PipelineRunStatusFields: tektonv1.PipelineRunStatusFields{ - StartTime: &metav1.Time{Time: time.Now()}, - CompletionTime: &metav1.Time{Time: time.Now().Add(5 * time.Minute)}, - Results: []tektonv1.PipelineRunResult{ - { - Name: "IMAGE_DIGEST", - Value: *tektonv1.NewStructuredValues("image_digest_value"), - }, - { - Name: "IMAGE_URL", - Value: *tektonv1.NewStructuredValues("image"), - }, - { - Name: "CHAINS-GIT_URL", - Value: *tektonv1.NewStructuredValues("git_url_value"), - }, - { - Name: "CHAINS-GIT_COMMIT", - Value: *tektonv1.NewStructuredValues("git_commit_value"), + By("Updating status to completed successfully") + createdPipelineRun.Status = tektonv1.PipelineRunStatus{ + PipelineRunStatusFields: tektonv1.PipelineRunStatusFields{ + StartTime: &metav1.Time{Time: time.Now()}, + CompletionTime: &metav1.Time{Time: time.Now().Add(5 * time.Minute)}, + Results: []tektonv1.PipelineRunResult{ + { + Name: "IMAGE_DIGEST", + Value: *tektonv1.NewStructuredValues("image_digest_value"), + }, + { + Name: "IMAGE_URL", + Value: *tektonv1.NewStructuredValues("image"), + }, + { + Name: "CHAINS-GIT_URL", + Value: *tektonv1.NewStructuredValues("git_url_value"), + }, + { + Name: "CHAINS-GIT_COMMIT", + Value: *tektonv1.NewStructuredValues("git_commit_value"), + }, }, }, - }, - Status: v1.Status{ - Conditions: v1.Conditions{ - apis.Condition{ - Message: "Tasks Completed: 12 (Failed: 0, Cancelled 0), Skipped: 2", - Reason: "Completed", - Status: "True", - Type: apis.ConditionSucceeded, + Status: v1.Status{ + Conditions: v1.Conditions{ + apis.Condition{ + Message: "Tasks Completed: 12 (Failed: 0, Cancelled 0), Skipped: 2", + Reason: "Completed", + Status: "True", + Type: apis.ConditionSucceeded, + }, }, }, - }, - } - Expect(k8sClient.Status().Update(ctx, createdPipelineRun)).Should(Succeed()) + } + Expect(k8sClient.Status().Update(ctx, createdPipelineRun)).Should(Succeed()) + // The pipelinerun should be reconciled: + // Read the results, add the notification annotation, remove the finalizer + Eventually(func() bool { + err := k8sClient.Get(ctx, pushPipelineRunLookupKey, createdPipelineRun) + Expect(err).ToNot(HaveOccurred()) + return metadata.HasAnnotationWithValue(createdPipelineRun, NotificationPipelineRunAnnotation, NotificationPipelineRunAnnotationValue) + }, timeout, interval).Should(BeTrue()) + Expect(controllerutil.ContainsFinalizer(createdPipelineRun, NotificationPipelineRunFinalizer)).To(BeFalse()) + // Check the Notify was called only once + Expect(mn.Counter).To(Equal(1)) + // Check that notifications metric increased by 1 + Expect(testutil.ToFloat64(notifications)).To(Equal(notificationsCounter + 1)) + // Check that notificationsFailures metric did not change + Expect(testutil.ToFloat64(notificationsFailures)).To(Equal(notificationsFailuresCounter)) + }) + }) - // The pipelinerun should be reconciled: - // Read the results, add the notification annotation, remove the finalizer - Eventually(func() bool { - err := k8sClient.Get(ctx, pushPipelineRunLookupKey, createdPipelineRun) + Context("when a push pipelinerun is created and end successfully, but Notify fails", func() { + It("should reconcile successfully - Add finalizer, Fail sending results, Not add annotation, Update metrics", + func() { + By("Creating a new push pipelinerun and add finalizer") + err := fakeErrorNotifyNsr.SetupWithManager(k8sManager) Expect(err).ToNot(HaveOccurred()) - return metadata.HasAnnotationWithValue(createdPipelineRun, NotificationPipelineRunAnnotation, NotificationPipelineRunAnnotationValue) - }, timeout, interval).Should(BeTrue()) - Expect(controllerutil.ContainsFinalizer(createdPipelineRun, NotificationPipelineRunFinalizer)).To(BeFalse()) - // Check the Notify was called once - Expect(mn.Counter).To(Equal(1)) - }) - }) + // The pipelinerun should be reconciled and the notification finalizer has been added successfully + Eventually(func() bool { + err := k8sClient.Get(ctx, pushPipelineRunLookupKey, createdPipelineRun) + Expect(err).ToNot(HaveOccurred()) + return controllerutil.ContainsFinalizer(createdPipelineRun, NotificationPipelineRunFinalizer) + }, timeout, interval).Should(BeTrue()) + Expect(controllerutil.ContainsFinalizer(createdPipelineRun, NotificationPipelineRunFinalizer)).To(BeTrue()) + // Check the Notify was not called + Expect(mn.Counter).To(BeZero()) + By("Updating status to completed successfully") + createdPipelineRun.Status = tektonv1.PipelineRunStatus{ + PipelineRunStatusFields: tektonv1.PipelineRunStatusFields{ + StartTime: &metav1.Time{Time: time.Now()}, + CompletionTime: &metav1.Time{Time: time.Now().Add(5 * time.Minute)}, + Results: []tektonv1.PipelineRunResult{ + { + Name: "IMAGE_DIGEST", + Value: *tektonv1.NewStructuredValues("image_digest_value"), + }, + { + Name: "IMAGE_URL", + Value: *tektonv1.NewStructuredValues("image"), + }, + { + Name: "CHAINS-GIT_URL", + Value: *tektonv1.NewStructuredValues("git_url_value"), + }, + { + Name: "CHAINS-GIT_COMMIT", + Value: *tektonv1.NewStructuredValues("git_commit_value"), + }, + }, + }, + Status: v1.Status{ + Conditions: v1.Conditions{ + apis.Condition{ + Message: "Tasks Completed: 12 (Failed: 0, Cancelled 0), Skipped: 2", + Reason: "Completed", + Status: "True", + Type: apis.ConditionSucceeded, + }, + }, + }, + } + Expect(k8sClient.Status().Update(ctx, createdPipelineRun)).Should(Succeed()) + // Wait for the Notify method to be called + Eventually(func() bool { + return fakeErrorNotify.Counter != 0 + }, timeout, interval).Should(BeTrue()) + Expect(controllerutil.ContainsFinalizer(createdPipelineRun, NotificationPipelineRunFinalizer)).To(BeTrue()) + Expect(metadata.HasAnnotationWithValue(createdPipelineRun, NotificationPipelineRunAnnotation, NotificationPipelineRunAnnotationValue)).To(BeFalse()) + // Since returning an error will reconcile the resource indefinitely, and we cannot track the number of Notify calls and + // the number of times the metrics will be updated we can check that the notification and notificationFailures + // metrics after at least one failure were increased + Expect(testutil.ToFloat64(notifications)).To(BeNumerically(">", notificationsCounter)) + Expect(testutil.ToFloat64(notificationsFailures)).To(BeNumerically(">", notificationsFailuresCounter)) + }) + }) Context("when a push pipelinerun is created and end with failure", func() { - It("should reconcile successfully - Add finalizer, Not reading the results, Not adding annotation and remove the finalizer", func() { - By("Creating a new push pipelinerun and add finalizer") - - // The pipelinerun should be reconciled and the notification finalizer has been added successfully - Eventually(func() bool { - err := k8sClient.Get(ctx, pushPipelineRunLookupKey, createdPipelineRun) + It("should reconcile successfully - Add finalizer, Not reading the results, Not adding annotation and remove the finalizer, Not update metrics", + func() { + By("Creating a new push pipelinerun and add finalizer") + err := nsr.SetupWithManager(k8sManager) Expect(err).ToNot(HaveOccurred()) - return controllerutil.ContainsFinalizer(createdPipelineRun, NotificationPipelineRunFinalizer) - }, timeout, interval).Should(BeTrue()) - Expect(controllerutil.ContainsFinalizer(createdPipelineRun, NotificationPipelineRunFinalizer)).To(BeTrue()) + // The pipelinerun should be reconciled and the notification finalizer has been added successfully + Eventually(func() bool { + err := k8sClient.Get(ctx, pushPipelineRunLookupKey, createdPipelineRun) + Expect(err).ToNot(HaveOccurred()) + return controllerutil.ContainsFinalizer(createdPipelineRun, NotificationPipelineRunFinalizer) + }, timeout, interval).Should(BeTrue()) + Expect(controllerutil.ContainsFinalizer(createdPipelineRun, NotificationPipelineRunFinalizer)).To(BeTrue()) - By("Updating status to completed with failure") - createdPipelineRun.Status = tektonv1.PipelineRunStatus{ - PipelineRunStatusFields: tektonv1.PipelineRunStatusFields{ - StartTime: &metav1.Time{Time: time.Now()}, - CompletionTime: &metav1.Time{Time: time.Now().Add(5 * time.Minute)}, - }, - Status: v1.Status{ - Conditions: v1.Conditions{ - apis.Condition{ - Message: "Tasks Completed: 12 (Failed: 0, Cancelled 0), Skipped: 2", - Reason: "CouldntGetTask", - Status: "False", - Type: apis.ConditionSucceeded, + By("Updating status to completed with failure") + createdPipelineRun.Status = tektonv1.PipelineRunStatus{ + PipelineRunStatusFields: tektonv1.PipelineRunStatusFields{ + StartTime: &metav1.Time{Time: time.Now()}, + CompletionTime: &metav1.Time{Time: time.Now().Add(5 * time.Minute)}, + }, + Status: v1.Status{ + Conditions: v1.Conditions{ + apis.Condition{ + Message: "Tasks Completed: 12 (Failed: 0, Cancelled 0), Skipped: 2", + Reason: "CouldntGetTask", + Status: "False", + Type: apis.ConditionSucceeded, + }, }, }, - }, - } - Expect(k8sClient.Status().Update(ctx, createdPipelineRun)).Should(Succeed()) + } + Expect(k8sClient.Status().Update(ctx, createdPipelineRun)).Should(Succeed()) - // The pipelinerun should be reconciled: - // Remove the finalizer - Eventually(func() bool { - err := k8sClient.Get(ctx, pushPipelineRunLookupKey, createdPipelineRun) - Expect(err).ToNot(HaveOccurred()) - return controllerutil.ContainsFinalizer(createdPipelineRun, NotificationPipelineRunFinalizer) - }, timeout, interval).Should(BeFalse()) - Expect(metadata.HasAnnotationWithValue(createdPipelineRun, NotificationPipelineRunAnnotation, NotificationPipelineRunAnnotationValue)).To(BeFalse()) - // Check the Notify was not called - Expect(mn.Counter).To(BeZero()) - }) + // The pipelinerun should be reconciled: + // Remove the finalizer + Eventually(func() bool { + err := k8sClient.Get(ctx, pushPipelineRunLookupKey, createdPipelineRun) + Expect(err).ToNot(HaveOccurred()) + return controllerutil.ContainsFinalizer(createdPipelineRun, NotificationPipelineRunFinalizer) + }, timeout, interval).Should(BeFalse()) + Expect(metadata.HasAnnotationWithValue(createdPipelineRun, NotificationPipelineRunAnnotation, NotificationPipelineRunAnnotationValue)).To(BeFalse()) + // Check the Notify was not called + Expect(mn.Counter).To(BeZero()) + Expect(testutil.ToFloat64(notifications)).To(Equal(notificationsCounter)) + Expect(testutil.ToFloat64(notificationsFailures)).To(Equal(notificationsFailuresCounter)) + }) }) }) Describe("Testing No reconcile with non push pipelinerun", func() { Context("When a non push pipelineRun is created", func() { It("Reconcile should not run", func() { - + err := nsr.SetupWithManager(k8sManager) + Expect(err).ToNot(HaveOccurred()) // Create a pull_request pipelinerun pullRequestPipelineRun = &tektonv1.PipelineRun{ ObjectMeta: metav1.ObjectMeta{ @@ -238,7 +318,7 @@ var _ = Describe("NotificationService Controller", func() { }, }, } - err := k8sClient.Create(ctx, pullRequestPipelineRun) + err = k8sClient.Create(ctx, pullRequestPipelineRun) Expect(err).NotTo(HaveOccurred(), "failed to create test Pipelinerun resource") // Wait for the resource to be created @@ -295,6 +375,8 @@ var _ = Describe("NotificationService Controller", func() { Expect(metadata.HasAnnotationWithValue(createdPipelineRun, NotificationPipelineRunAnnotation, NotificationPipelineRunAnnotationValue)).To(BeFalse()) // Check the Notify was not called Expect(mn.Counter).To(BeZero()) + Expect(testutil.ToFloat64(notifications)).To(Equal(notificationsCounter)) + Expect(testutil.ToFloat64(notificationsFailures)).To(Equal(notificationsFailuresCounter)) }) }) }) diff --git a/internal/controller/suite_test.go b/internal/controller/suite_test.go index e1ec044..57ae0db 100644 --- a/internal/controller/suite_test.go +++ b/internal/controller/suite_test.go @@ -38,6 +38,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/envtest" logf "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/log/zap" + "sigs.k8s.io/controller-runtime/pkg/manager" // +kubebuilder:scaffold:imports ) @@ -46,12 +47,12 @@ import ( var cfg *rest.Config var k8sClient client.Client +var k8sManager manager.Manager var testEnv *envtest.Environment var ctx context.Context var cancel context.CancelFunc -var mn *MockNotifier -var nsr *NotificationServiceReconciler -var fakeErrorNsr *NotificationServiceReconciler +var mn, fakeErrorNotify *MockNotifier +var nsr, fakeErrorNotifyNsr, fakeErrorNsr *NotificationServiceReconciler func TestControllers(t *testing.T) { RegisterFailHandler(Fail) @@ -60,11 +61,15 @@ func TestControllers(t *testing.T) { } type MockNotifier struct { - Counter int + Counter int + shouldThroughError bool } func (mn *MockNotifier) Notify(ctx context.Context, message string) error { mn.Counter++ + if mn.shouldThroughError { + return errors.New("Failed to Notify") + } return nil } @@ -141,23 +146,28 @@ var _ = BeforeEach(func() { and it'd be easy to make mistakes. https://github.com/kubernetes-sigs/kubebuilder/blob/de1cc60900b896b2195e403a40c976a892df4921/docs/book/src/cronjob-tutorial/testdata/project/internal/controller/suite_test.go#L136 */ - k8sManager, err := ctrl.NewManager(cfg, ctrl.Options{ + k8sManager, err = ctrl.NewManager(cfg, ctrl.Options{ Scheme: clientsetscheme.Scheme, }) Expect(err).ToNot(HaveOccurred()) // Create a mock of notifier - mn = &MockNotifier{} - + mn = &MockNotifier{shouldThroughError: false} nsr = &NotificationServiceReconciler{ Client: k8sManager.GetClient(), Scheme: k8sManager.GetScheme(), Log: k8sManager.GetLogger(), Notifier: mn, } - err = nsr.SetupWithManager(k8sManager) - Expect(err).ToNot(HaveOccurred()) - + // err = nsr.SetupWithManager(k8sManager) + // Expect(err).ToNot(HaveOccurred()) + fakeErrorNotify = &MockNotifier{shouldThroughError: true} + fakeErrorNotifyNsr = &NotificationServiceReconciler{ + Client: k8sManager.GetClient(), + Scheme: k8sManager.GetScheme(), + Log: k8sManager.GetLogger(), + Notifier: fakeErrorNotify, + } fakeErrorPatchClient := &clientMock{shouldThroughError: true} fakeErrorNsr = &NotificationServiceReconciler{ Client: fakeErrorPatchClient,