From 83fe97978020b1eff1defe4d2ad822eceb8395da Mon Sep 17 00:00:00 2001 From: Zach Aller Date: Thu, 5 Oct 2023 08:14:05 -0600 Subject: [PATCH 1/2] fix: sync notification controller configmaps/secrets first (#3075) sync notification controller configmaps/secrets first before starting other informers Signed-off-by: zachaller --- controller/controller.go | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/controller/controller.go b/controller/controller.go index 5429cfe743..afe4bc769b 100644 --- a/controller/controller.go +++ b/controller/controller.go @@ -463,6 +463,12 @@ func (c *Manager) startLeading(ctx context.Context, rolloutThreadiness, serviceT // Start the informer factories to begin populating the informer caches log.Info("Starting Controllers") + c.notificationConfigMapInformerFactory.Start(ctx.Done()) + c.notificationSecretInformerFactory.Start(ctx.Done()) + if ok := cache.WaitForCacheSync(ctx.Done(), c.configMapSynced, c.secretSynced); !ok { + log.Fatalf("failed to wait for configmap/secret caches to sync, exiting") + } + // notice that there is no need to run Start methods in a separate goroutine. (i.e. go kubeInformerFactory.Start(stopCh) // Start method is non-blocking and runs all registered informers in a dedicated goroutine. c.dynamicInformerFactory.Start(ctx.Done()) @@ -471,9 +477,6 @@ func (c *Manager) startLeading(ctx context.Context, rolloutThreadiness, serviceT } c.kubeInformerFactory.Start(ctx.Done()) - c.notificationConfigMapInformerFactory.Start(ctx.Done()) - c.notificationSecretInformerFactory.Start(ctx.Done()) - c.jobInformerFactory.Start(ctx.Done()) // Check if Istio installed on cluster before starting dynamicInformerFactory From ba7c9a5a1b18e83a2a5675f97d67347cd493e597 Mon Sep 17 00:00:00 2001 From: Zach Aller Date: Thu, 5 Oct 2023 10:30:50 -0600 Subject: [PATCH 2/2] fix: missing notification on error (#3076) * fix: missing notification on error Signed-off-by: zachaller * fix tests Signed-off-by: zachaller * aggregate errors Signed-off-by: zachaller * fix bad stat counts Signed-off-by: zachaller * return errors Signed-off-by: zachaller * fix tests on return of errors Signed-off-by: zachaller * change case on logs Signed-off-by: zachaller * missed a case Signed-off-by: zachaller --------- Signed-off-by: zachaller --- utils/record/record.go | 40 ++++++++++++++++++++++++------------- utils/record/record_test.go | 14 ++++++------- 2 files changed, 33 insertions(+), 21 deletions(-) diff --git a/utils/record/record.go b/utils/record/record.go index d2bd322acf..859b954390 100644 --- a/utils/record/record.go +++ b/utils/record/record.go @@ -218,9 +218,7 @@ func (e *EventRecorderAdapter) defaultEventf(object runtime.Object, warn bool, o err := e.sendNotifications(api, object, opts) if err != nil { logCtx.Errorf("Notifications failed to send for eventReason %s with error: %s", opts.EventReason, err) - e.NotificationFailedCounter.WithLabelValues(namespace, name, opts.EventType, opts.EventReason).Inc() } - e.NotificationSuccessCounter.WithLabelValues(namespace, name, opts.EventType, opts.EventReason).Inc() } } @@ -248,7 +246,7 @@ func NewAPIFactorySettings() api.Settings { } // Send notifications for triggered event if user is subscribed -func (e *EventRecorderAdapter) sendNotifications(notificationsAPI api.API, object runtime.Object, opts EventOptions) error { +func (e *EventRecorderAdapter) sendNotifications(notificationsAPI api.API, object runtime.Object, opts EventOptions) []error { logCtx := logutil.WithObject(object) _, namespace, name := logutil.KindNamespaceName(logCtx) startTime := timeutil.Now() @@ -259,7 +257,7 @@ func (e *EventRecorderAdapter) sendNotifications(notificationsAPI api.API, objec }() if notificationsAPI == nil { - return fmt.Errorf("notificationsAPI is nil") + return []error{fmt.Errorf("NotificationsAPI is nil")} } cfg := notificationsAPI.GetConfig() @@ -274,39 +272,53 @@ func (e *EventRecorderAdapter) sendNotifications(notificationsAPI api.API, objec objMap, err := toObjectMap(object) if err != nil { - return err + return []error{err} } emptyCondition := hash("") + // We should not return in these loops because we want other configured notifications to still send if they can. + errors := []error{} for _, destination := range destinations { res, err := notificationsAPI.RunTrigger(trigger, objMap) if err != nil { - log.Errorf("Failed to execute condition of trigger %s: %v", trigger, err) - return err + log.Errorf("Failed to run trigger, trigger: %s, destination: %s, namespace config: %s : %v", + trigger, destination, notificationsAPI.GetConfig().Namespace, err) + errors = append(errors, err) + continue } log.Infof("Trigger %s result: %v", trigger, res) for _, c := range res { - log.Infof("Res When Condition hash: %s, Templates: %s", c.Key, c.Templates) + log.Infof("Result when condition hash: %s, templates: %s", c.Key, c.Templates) s := strings.Split(c.Key, ".")[1] if s != emptyCondition && c.Triggered == true { err = notificationsAPI.Send(objMap, c.Templates, destination) if err != nil { - log.Errorf("notification error: %s", err.Error()) - return err + log.Errorf("Failed to execute the sending of notification on not empty condition, trigger: %s, destination: %s, namespace config: %s : %v", + trigger, destination, notificationsAPI.GetConfig().Namespace, err) + e.NotificationFailedCounter.WithLabelValues(namespace, name, opts.EventType, opts.EventReason).Inc() + errors = append(errors, err) + continue } + e.NotificationSuccessCounter.WithLabelValues(namespace, name, opts.EventType, opts.EventReason).Inc() } else if s == emptyCondition { err = notificationsAPI.Send(objMap, c.Templates, destination) if err != nil { - log.Errorf("notification error: %s", err.Error()) - return err + log.Errorf("Failed to execute the sending of notification on empty condition, trigger: %s, destination: %s, namespace config: %s : %v", + trigger, destination, notificationsAPI.GetConfig().Namespace, err) + e.NotificationFailedCounter.WithLabelValues(namespace, name, opts.EventType, opts.EventReason).Inc() + errors = append(errors, err) + continue } + e.NotificationSuccessCounter.WithLabelValues(namespace, name, opts.EventType, opts.EventReason).Inc() } } } - - return nil + if len(errors) == 0 { + return nil + } + return errors } // This function is copied over from notification engine to make sure we honour emptyCondition diff --git a/utils/record/record_test.go b/utils/record/record_test.go index 6c494a3ac7..97f4452441 100644 --- a/utils/record/record_test.go +++ b/utils/record/record_test.go @@ -113,7 +113,7 @@ func TestSendNotifications(t *testing.T) { rec.EventRecorderAdapter.apiFactory = apiFactory //ch := make(chan prometheus.HistogramVec, 1) err := rec.sendNotifications(mockAPI, &r, EventOptions{EventReason: "FooReason"}) - assert.NoError(t, err) + assert.Nil(t, err) } func TestSendNotificationsWhenCondition(t *testing.T) { @@ -140,7 +140,7 @@ func TestSendNotificationsWhenCondition(t *testing.T) { rec.EventRecorderAdapter.apiFactory = apiFactory //ch := make(chan prometheus.HistogramVec, 1) err := rec.sendNotifications(mockAPI, &r, EventOptions{EventReason: "FooReason"}) - assert.NoError(t, err) + assert.Nil(t, err) } func TestSendNotificationsWhenConditionTime(t *testing.T) { @@ -340,7 +340,7 @@ func TestSendNotificationsFails(t *testing.T) { rec.EventRecorderAdapter.apiFactory = apiFactory err := rec.sendNotifications(mockAPI, &r, EventOptions{EventReason: "FooReason"}) - assert.Error(t, err) + assert.Len(t, err, 1) }) t.Run("GetAPIError", func(t *testing.T) { @@ -349,7 +349,7 @@ func TestSendNotificationsFails(t *testing.T) { rec.EventRecorderAdapter.apiFactory = apiFactory err := rec.sendNotifications(nil, &r, EventOptions{EventReason: "FooReason"}) - assert.Error(t, err) + assert.NotNil(t, err) }) } @@ -380,7 +380,7 @@ func TestSendNotificationsFailsWithRunTriggerError(t *testing.T) { rec.EventRecorderAdapter.apiFactory = apiFactory err := rec.sendNotifications(mockAPI, &r, EventOptions{EventReason: "FooReason"}) - assert.Error(t, err) + assert.Len(t, err, 1) }) t.Run("GetAPIError", func(t *testing.T) { @@ -389,7 +389,7 @@ func TestSendNotificationsFailsWithRunTriggerError(t *testing.T) { rec.EventRecorderAdapter.apiFactory = apiFactory err := rec.sendNotifications(nil, &r, EventOptions{EventReason: "FooReason"}) - assert.Error(t, err) + assert.NotNil(t, err) }) } @@ -419,7 +419,7 @@ func TestSendNotificationsNoTrigger(t *testing.T) { rec.EventRecorderAdapter.apiFactory = apiFactory err := rec.sendNotifications(mockAPI, &r, EventOptions{EventReason: "MissingReason"}) - assert.Error(t, err) + assert.Len(t, err, 1) } func TestNewAPIFactorySettings(t *testing.T) {