Skip to content
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

Jump to bottom

operator: always report events #9427

Merged
merged 1 commit into from Dec 20, 2021
Merged

operator: always report events #9427

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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 3 additions & 2 deletions pkg/operator/ceph/cluster/controller.go
View file
Open in desktop
Expand Up @@ -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"
Expand Down Expand Up @@ -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
}

Expand All @@ -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(),
Expand Down
3 changes: 1 addition & 2 deletions pkg/operator/ceph/cluster/controller_test.go
View file
Open in desktop
Expand Up @@ -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"
Expand Down Expand Up @@ -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
Expand Down
5 changes: 3 additions & 2 deletions pkg/operator/ceph/object/controller.go
View file
Open in desktop
Expand Up @@ -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"
Expand Down Expand Up @@ -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
}
Expand All @@ -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,
}
Expand Down
4 changes: 2 additions & 2 deletions pkg/operator/ceph/object/controller_test.go
View file
Open in desktop
Expand Up @@ -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(),
}

Expand Down Expand Up @@ -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,
}

Expand Down
12 changes: 6 additions & 6 deletions pkg/operator/ceph/reporting/reporting.go
View file
Open in desktop
Expand Up @@ -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"
Expand All @@ -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
Expand All @@ -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())
Copy link
Contributor

Choose a reason for hiding this comment

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

if this is happening on every retry, maybe we should keep the ReportIfNotPresent() call?

Copy link
Member Author

@BlaineEXE BlaineEXE Dec 15, 2021 •
edited

Choose a reason for hiding this comment

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

If there are multiple events, they will be collated together like so, with something similar to (x4 over 8m46s) indicating that they have happened multiple times and without adding loads of clutter:

  Warning  ReconcileFailed     8m11s               rook-ceph-object-controller  failed to create object store deployments: fail
ed to start rgw health checker for CephObjectStore "rook-ceph/my-store", will re-reconcile: failed to create bucket checker for
 CephObjectStore "rook-ceph/my-store": failed to create or retrieve rgw admin ops user: failed to create object user "rgw-admin
-ops-user". error code 1 for object store "my-store": failed to create s3 user. . : signal: interrupt
  Normal   ReconcileSucceeded  9s (x4 over 8m46s)  rook-ceph-object-controller  successfully configured CephObjectStore "rook-c
eph/my-store"

IMO, that is an okay trade-off for getting more insight into when things have happened most recently.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok .so this is happening regardless of whether we use ReportIfNotPresent or Event?
I guess we can just rely on this mechanism instead of adding another one.


if !reconcileResponse.IsZero() {
// The framework will requeue immediately if there is an error. If we get an error with
Expand All @@ -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
Expand Down Expand Up @@ -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{
Expand All @@ -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)
Expand Down
79 changes: 0 additions & 79 deletions pkg/operator/k8sutil/events.go
View file
Open in desktop

This file was deleted.

147 changes: 0 additions & 147 deletions pkg/operator/k8sutil/events_test.go
View file
Open in desktop

This file was deleted.