From 769e774719b258517e7a8641b38537abb7d830e5 Mon Sep 17 00:00:00 2001 From: Blaine Gardner Date: Tue, 14 Dec 2021 15:16:16 -0700 Subject: [PATCH] operator: always report events The original PR which added event reporting unnecessarily "optimized" to prevent spamming the API controller[1]. It is sometimes important to get events as they happen and not hide new events behind preexisting older events. For example, in integration tests, we may often want to wait for a controller to finish processing an update, and the best way to do that is to wait for the "ReconcileSucceeded" event. In order for this to be useful, the events must be reported each time. If we begin having problems with events being reported too often, then we should fix the underlying issue of reconciles happening too often instead of relying on a time-based "optimization" that hides recent event reports that may be useful. [1]: https://github.com/rook/rook/pull/7222 Signed-off-by: Blaine Gardner (cherry picked from commit da61ac1ae83b207ab2c1c0529e54c9529eab38a8) --- pkg/operator/ceph/cluster/controller.go | 5 +- pkg/operator/ceph/cluster/controller_test.go | 3 +- pkg/operator/ceph/object/controller.go | 5 +- pkg/operator/ceph/object/controller_test.go | 4 +- pkg/operator/ceph/reporting/reporting.go | 12 +- pkg/operator/k8sutil/events.go | 79 ---------- pkg/operator/k8sutil/events_test.go | 147 ------------------- 7 files changed, 15 insertions(+), 240 deletions(-) delete mode 100644 pkg/operator/k8sutil/events.go delete mode 100644 pkg/operator/k8sutil/events_test.go diff --git a/pkg/operator/ceph/cluster/controller.go b/pkg/operator/ceph/cluster/controller.go index 46754e2c385c..1467d765a77d 100644 --- a/pkg/operator/ceph/cluster/controller.go +++ b/pkg/operator/ceph/cluster/controller.go @@ -41,6 +41,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" apituntime "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" + "k8s.io/client-go/tools/record" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller" "sigs.k8s.io/controller-runtime/pkg/handler" @@ -88,7 +89,7 @@ type ClusterController struct { osdChecker *osd.OSDHealthMonitor client client.Client namespacedName types.NamespacedName - recorder *k8sutil.EventReporter + recorder record.EventRecorder OpManagerCtx context.Context } @@ -112,7 +113,7 @@ func newReconciler(mgr manager.Manager, ctx *clusterd.Context, clusterController // add "rook-" prefix to the controller name to make sure it is clear to all reading the events // that they are coming from Rook. The controller name already has context that it is for Ceph // and from the cluster controller. - clusterController.recorder = k8sutil.NewEventReporter(mgr.GetEventRecorderFor("rook-" + controllerName)) + clusterController.recorder = mgr.GetEventRecorderFor("rook-" + controllerName) return &ReconcileCephCluster{ client: mgr.GetClient(), diff --git a/pkg/operator/ceph/cluster/controller_test.go b/pkg/operator/ceph/cluster/controller_test.go index 477f36046ad8..0c5ea2219d3f 100644 --- a/pkg/operator/ceph/cluster/controller_test.go +++ b/pkg/operator/ceph/cluster/controller_test.go @@ -24,7 +24,6 @@ import ( cephv1 "github.com/rook/rook/pkg/apis/ceph.rook.io/v1" "github.com/rook/rook/pkg/client/clientset/versioned/scheme" "github.com/rook/rook/pkg/clusterd" - "github.com/rook/rook/pkg/operator/k8sutil" "github.com/stretchr/testify/assert" corev1 "k8s.io/api/core/v1" kerrors "k8s.io/apimachinery/pkg/api/errors" @@ -82,7 +81,7 @@ func TestReconcileDeleteCephCluster(t *testing.T) { // create the cluster controller and tell it that the cluster has been deleted controller := NewClusterController(clusterdCtx, "") fakeRecorder := record.NewFakeRecorder(5) - controller.recorder = k8sutil.NewEventReporter(fakeRecorder) + controller.recorder = fakeRecorder // Create a fake client to mock API calls // Make sure it has the fake CephCluster that is to be deleted in it diff --git a/pkg/operator/ceph/object/controller.go b/pkg/operator/ceph/object/controller.go index 570d1df8c4dd..e155d376f118 100644 --- a/pkg/operator/ceph/object/controller.go +++ b/pkg/operator/ceph/object/controller.go @@ -42,6 +42,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" + "k8s.io/client-go/tools/record" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller" "sigs.k8s.io/controller-runtime/pkg/handler" @@ -87,7 +88,7 @@ type ReconcileCephObjectStore struct { clusterSpec *cephv1.ClusterSpec clusterInfo *cephclient.ClusterInfo objectStoreContexts map[string]*objectStoreHealth - recorder *k8sutil.EventReporter + recorder record.EventRecorder opManagerContext context.Context opConfig opcontroller.OperatorConfig } @@ -113,7 +114,7 @@ func newReconciler(mgr manager.Manager, context *clusterd.Context, opManagerCont context: context, bktclient: bktclient.NewForConfigOrDie(context.KubeConfig), objectStoreContexts: make(map[string]*objectStoreHealth), - recorder: k8sutil.NewEventReporter(mgr.GetEventRecorderFor("rook-" + controllerName)), + recorder: mgr.GetEventRecorderFor("rook-" + controllerName), opManagerContext: opManagerContext, opConfig: opConfig, } diff --git a/pkg/operator/ceph/object/controller_test.go b/pkg/operator/ceph/object/controller_test.go index 0a19ada55b0d..06f0ca2475dc 100644 --- a/pkg/operator/ceph/object/controller_test.go +++ b/pkg/operator/ceph/object/controller_test.go @@ -349,7 +349,7 @@ func TestCephObjectStoreController(t *testing.T) { scheme: s, context: c, objectStoreContexts: make(map[string]*objectStoreHealth), - recorder: k8sutil.NewEventReporter(record.NewFakeRecorder(5)), + recorder: record.NewFakeRecorder(5), opManagerContext: context.TODO(), } @@ -684,7 +684,7 @@ func TestCephObjectStoreControllerMultisite(t *testing.T) { scheme: s, context: c, objectStoreContexts: make(map[string]*objectStoreHealth), - recorder: k8sutil.NewEventReporter(record.NewFakeRecorder(5)), + recorder: record.NewFakeRecorder(5), opManagerContext: ctx, } diff --git a/pkg/operator/ceph/reporting/reporting.go b/pkg/operator/ceph/reporting/reporting.go index 98ec711ab971..19d816a40ded 100644 --- a/pkg/operator/ceph/reporting/reporting.go +++ b/pkg/operator/ceph/reporting/reporting.go @@ -24,10 +24,10 @@ import ( "github.com/coreos/pkg/capnslog" "github.com/pkg/errors" cephv1 "github.com/rook/rook/pkg/apis/ceph.rook.io/v1" - "github.com/rook/rook/pkg/operator/k8sutil" "github.com/rook/rook/pkg/util/dependents" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/types" + "k8s.io/client-go/tools/record" "k8s.io/client-go/util/retry" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/reconcile" @@ -40,7 +40,7 @@ import ( // error returned by the reconcile. // The function is designed to return the appropriate values needed for the controller-runtime // framework's Reconcile() method. -func ReportReconcileResult(logger *capnslog.PackageLogger, recorder *k8sutil.EventReporter, +func ReportReconcileResult(logger *capnslog.PackageLogger, recorder record.EventRecorder, obj client.Object, reconcileResponse reconcile.Result, err error, ) (reconcile.Result, error) { kind := obj.GetObjectKind().GroupVersionKind().Kind @@ -51,7 +51,7 @@ func ReportReconcileResult(logger *capnslog.PackageLogger, recorder *k8sutil.Eve logger.Errorf("failed to reconcile %s %q. %v", kind, nsName, err) // 2. event - recorder.ReportIfNotPresent(obj, corev1.EventTypeWarning, string(cephv1.ReconcileFailed), err.Error()) + recorder.Event(obj, corev1.EventTypeWarning, string(cephv1.ReconcileFailed), err.Error()) if !reconcileResponse.IsZero() { // The framework will requeue immediately if there is an error. If we get an error with @@ -67,7 +67,7 @@ func ReportReconcileResult(logger *capnslog.PackageLogger, recorder *k8sutil.Eve logger.Debug(successMsg) // 2. event - recorder.ReportIfNotPresent(obj, corev1.EventTypeNormal, string(cephv1.ReconcileSucceeded), successMsg) + recorder.Event(obj, corev1.EventTypeNormal, string(cephv1.ReconcileSucceeded), successMsg) } return reconcileResponse, err @@ -116,7 +116,7 @@ func ReportDeletionBlockedDueToDependents( // 2. as an event on the object (via the given event recorder) // 3. as a condition on the object (added to the object's conditions list given) func ReportDeletionNotBlockedDueToDependents( - logger *capnslog.PackageLogger, client client.Client, recorder *k8sutil.EventReporter, obj cephv1.StatusConditionGetter, + logger *capnslog.PackageLogger, client client.Client, recorder record.EventRecorder, obj cephv1.StatusConditionGetter, ) { kind := obj.GetObjectKind().GroupVersionKind().Kind nsName := types.NamespacedName{ @@ -130,7 +130,7 @@ func ReportDeletionNotBlockedDueToDependents( logger.Infof("%s. %s", safeMsg, deletingMsg) // 2. event - recorder.ReportIfNotPresent(obj, corev1.EventTypeNormal, string(cephv1.DeletingReason), deletingMsg) + recorder.Event(obj, corev1.EventTypeNormal, string(cephv1.DeletingReason), deletingMsg) // 3. condition unblockedCond := dependents.DeletionBlockedDueToDependentsCondition(false, safeMsg) diff --git a/pkg/operator/k8sutil/events.go b/pkg/operator/k8sutil/events.go deleted file mode 100644 index 5d2486ee3d2f..000000000000 --- a/pkg/operator/k8sutil/events.go +++ /dev/null @@ -1,79 +0,0 @@ -/* -Copyright 2021 The Rook Authors. All rights reserved. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -// Package k8sutil for Kubernetes helpers. -package k8sutil - -import ( - "fmt" - "time" - - "k8s.io/apimachinery/pkg/api/meta" - "k8s.io/apimachinery/pkg/runtime" - "k8s.io/client-go/tools/record" -) - -// EventReporter is custom events reporter type which allows user to limit the events -type EventReporter struct { - recorder record.EventRecorder - - // lastReportedEvent will have a last captured event - lastReportedEvent map[string]string - - // lastReportedEventTime will be the time of lastReportedEvent - lastReportedEventTime map[string]time.Time -} - -// NewEventReporter returns EventReporter object -func NewEventReporter(recorder record.EventRecorder) *EventReporter { - return &EventReporter{ - recorder: recorder, - lastReportedEvent: make(map[string]string), - lastReportedEventTime: make(map[string]time.Time), - } -} - -// ReportIfNotPresent will report event if lastReportedEvent is not the same in last 60 minutes -func (rep *EventReporter) ReportIfNotPresent(instance runtime.Object, eventType, eventReason, msg string) { - - nameSpacedName, err := getNameSpacedName(instance) - if err != nil { - return - } - - eventKey := getEventKey(eventType, eventReason, msg) - - if rep.lastReportedEvent[nameSpacedName] != eventKey || rep.lastReportedEventTime[nameSpacedName].Add(time.Minute*60).Before(time.Now()) { - logger.Info("Reporting Event ", nameSpacedName, " ", eventKey) - rep.lastReportedEvent[nameSpacedName] = eventKey - rep.lastReportedEventTime[nameSpacedName] = time.Now() - rep.recorder.Event(instance, eventType, eventReason, msg) - } else { - logger.Debug("Not Reporting Event because event is same as the old one:", nameSpacedName, " ", eventKey) - } -} - -func getNameSpacedName(instance runtime.Object) (string, error) { - objMeta, err := meta.Accessor(instance) - if err != nil { - return "", err - } - return objMeta.GetNamespace() + ":" + objMeta.GetName(), nil -} - -func getEventKey(eventType, eventReason, msg string) string { - return fmt.Sprintf("%s:%s:%s", eventType, eventReason, msg) -} diff --git a/pkg/operator/k8sutil/events_test.go b/pkg/operator/k8sutil/events_test.go deleted file mode 100644 index fb25f5c1478e..000000000000 --- a/pkg/operator/k8sutil/events_test.go +++ /dev/null @@ -1,147 +0,0 @@ -/* -Copyright 2021 The Rook Authors. All rights reserved. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -// Package k8sutil for Kubernetes helpers. -package k8sutil - -import ( - "testing" - "time" - - "github.com/stretchr/testify/assert" - corev1 "k8s.io/api/core/v1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/client-go/tools/record" -) - -func getEventsOccurences(channel chan string) map[string]int { - - foundEvents := make(map[string]int) - - for len(channel) > 0 { - e := <-channel - foundEvents[e]++ - } - - return foundEvents -} - -func TestReportIfNotPresent(t *testing.T) { - pod1 := &corev1.Pod{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-pod1", - Namespace: "rook-ceph", - }, - } - - pod2 := &corev1.Pod{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-pod2", - Namespace: "rook-ceph", - }, - } - - testCases := []struct { - eventReported int - changeTime bool - ReportAnotherEvent bool - ReportEventForDifferentObject bool - }{ - { - // verify ReportIfNotPresent is called once and event is reported once - eventReported: 1, - }, - { - // verify ReportIfNotPresent is called twice and event is reported once - eventReported: 2, - }, - { - // verify ReportIfNotPresent report same event again when event is not present on cluster - eventReported: 1, - changeTime: true, - }, - { - // verify ReportIfNotPresent report same event again when event is not present on cluster - eventReported: 2, - changeTime: true, - }, - { - // verify it report event "a" both the times if events came like "a", "b", "a" - eventReported: 1, - ReportAnotherEvent: true, - }, - { - // verify it report event "a" both the times if events came like "a", "b", "a" - eventReported: 2, - ReportAnotherEvent: true, - }, - { - // verify it does not report the same event for same objects if multiple objects come into the picture - eventReported: 1, - ReportEventForDifferentObject: true, - }, - { - // verify it does not report the same event for same objects if multiple objects come into the picture - eventReported: 2, - ReportEventForDifferentObject: true, - }, - } - - for _, tc := range testCases { - eventType, eventReason, eventMsg := corev1.EventTypeNormal, "Created", "Pod has been created" - - frecorder := record.NewFakeRecorder(1024) - reporter := NewEventReporter(frecorder) - - for i := 0; i < tc.eventReported; i++ { - reporter.ReportIfNotPresent(pod1, eventType, eventReason, eventMsg) - } - - foundEvents := getEventsOccurences(frecorder.Events) - assert.Equal(t, 1, foundEvents[eventType+" "+eventReason+" "+eventMsg]) - - if tc.changeTime { - nameSpacedName, err := getNameSpacedName(pod1) - assert.NoError(t, err) - ftime := reporter.lastReportedEventTime[nameSpacedName].Add(time.Minute * -60) - reporter.lastReportedEventTime[nameSpacedName] = ftime - - reporter.ReportIfNotPresent(pod1, eventType, eventReason, eventMsg) - foundEvents := getEventsOccurences(frecorder.Events) - assert.Equal(t, 1, foundEvents[eventType+" "+eventReason+" "+eventMsg]) - } - - if tc.ReportAnotherEvent { - reporter.ReportIfNotPresent(pod1, corev1.EventTypeWarning, eventReason, eventMsg) - foundEvents := getEventsOccurences(frecorder.Events) - assert.Equal(t, 1, foundEvents[corev1.EventTypeWarning+" "+eventReason+" "+eventMsg]) - - reporter.ReportIfNotPresent(pod1, eventType, eventReason, eventMsg) - foundEvents = getEventsOccurences(frecorder.Events) - assert.Equal(t, 1, foundEvents[eventType+" "+eventReason+" "+eventMsg]) - } - - if tc.ReportEventForDifferentObject { - reporter.ReportIfNotPresent(pod2, eventType, eventReason, eventMsg) - foundEvents := getEventsOccurences(frecorder.Events) - assert.Equal(t, 1, foundEvents[eventType+" "+eventReason+" "+eventMsg]) - - reporter.ReportIfNotPresent(pod1, eventType, eventReason, eventMsg) - foundEvents = getEventsOccurences(frecorder.Events) - assert.Equal(t, 0, foundEvents[eventType+" "+eventReason+" "+eventMsg]) - } - } -}