Emit kubernetes events from KEDA #1523

ahmelsayed · 2021-01-21T22:16:41Z

Signed-off-by: Ahmed ElSayed ahmels@microsoft.com

This PR adds the following events:

For both ScaledObjects and ScaledJobs:

Ready
CheckFailed
Deleted
ScalersStarted
ScalersRestarted
ScalersStopped

For ScaledObjects:

ScaleTargetActivated
ScaleTargetDeactivated
ScaleTargetActivationFailed
ScaleTargetDeactivationFailed

For ScaledJobs:

JobsCreated

If this list looks okay, I'll open a docs PR to list them in there.

Checklist

Commits are signed with Developer Certificate of Origin (DCO)
Tests have been added
A PR is opened to update the documentation on https://github.com/kedacore/keda-docs
Changelog has been updated

Fixes #530

coderanger · 2021-01-21T22:36:10Z

pkg/scaling/scale_handler.go

@@ -90,6 +94,9 @@ func (h *scaleHandler) HandleScalableObject(scalableObject interface{}) error {
 			cancelValue()
 		}
 		h.scaleLoopContexts.Store(key, cancel)
+		h.recorder.Event(withTriggers, corev1.EventTypeNormal, eventreason.ScalersRestarted, "Restarted scalers watch")
+	} else {
+		h.recorder.Event(withTriggers, corev1.EventTypeNormal, eventreason.ScalersStarted, "Started scalers watch")


Would this emit every time the controller pod restarts?

Yes. I wasn't sure if there is a best practice somewhere for when to emit events. I tried to add all the ones mentioned in #530 but if there is a best practices for this I'd love to check it

The basic guideline is it should only be when something actually occurs. So any situation where a reconcile happens and it takes no action, it should produce no events :) Otherwise they can get spammy and overwhelm Etcd.

But I don't suppose that controller pod restarts happen that often or it is something that we should consider as normal. Not sure what other way we can emit this kind of event?

coderanger · 2021-01-21T22:37:48Z

controllers/scaledobject_controller.go

 	} else {
 		reqLogger.V(1).Info(msg)
 		conditions.SetReadyCondition(metav1.ConditionTrue, "ScaledObjectReady", msg)
+		r.Recorder.Event(scaledObject, corev1.EventTypeNormal, eventreason.Ready, msg)


This isn't usually the kind of thing you would want in an event since it's not a specific action or event, it's a convergent state.

Agree, we should emit only once, for the first time.

tomkerkhove · 2021-01-22T09:45:45Z

Before we merge, can you PR a new sub-page to our Operate docs please?
https://keda.sh/docs/2.1/operate/

Would be good to list all event types and what scenario they represent.

zroubalik · 2021-01-22T10:19:59Z

controllers/scaledjob_controller.go

 		} else {
 			reqLogger.V(1).Info(msg)
 			conditions.SetReadyCondition(metav1.ConditionTrue, "ScaledJobReady", msg)
+			r.Recorder.Event(scaledJob, corev1.EventTypeNormal, eventreason.Ready, msg)


This should be emitted only for the first time (if at all).

zroubalik · 2021-01-22T10:21:51Z

controllers/scaledobject_controller.go

 	} else {
 		reqLogger.V(1).Info(msg)
 		conditions.SetReadyCondition(metav1.ConditionTrue, "ScaledObjectReady", msg)
+		r.Recorder.Event(scaledObject, corev1.EventTypeNormal, eventreason.Ready, msg)


Agree, we should emit only once, for the first time.

zroubalik · 2021-01-22T10:28:24Z

pkg/scaling/scale_handler.go

@@ -90,6 +94,9 @@ func (h *scaleHandler) HandleScalableObject(scalableObject interface{}) error {
 			cancelValue()
 		}
 		h.scaleLoopContexts.Store(key, cancel)
+		h.recorder.Event(withTriggers, corev1.EventTypeNormal, eventreason.ScalersRestarted, "Restarted scalers watch")
+	} else {
+		h.recorder.Event(withTriggers, corev1.EventTypeNormal, eventreason.ScalersStarted, "Started scalers watch")


But I don't suppose that controller pod restarts happen that often or it is something that we should consider as normal. Not sure what other way we can emit this kind of event?

CHANGELOG.md

zroubalik

The list of events looks okay!

pkg/eventreason/eventreason.go

tomkerkhove · 2021-01-22T13:41:44Z

What about these events:

Scaler failed (which is not stopped I guess?)
Scaler unauthorized
New Trigger Authentication Created
Deployment Scaled to Zero
Scaledobject/SCaledJob is linked to trigger authentication

Follow-up PR/issue is ok, just asking

Signed-off-by: Ahmed ElSayed <ahmels@microsoft.com>

ahmelsayed · 2021-01-29T20:31:14Z

Thanks @coderanger, @zroubalik, @tomkerkhove for the feedback.
I made the following changes in 2872b99

The *Ready events to only happen either on the first time a ScaledObject/ScaledJob is reconciled, or if it's previous Ready status was False/Unknown.
Removed ScalersRestarted since that would always happen on any ScaledObject/ScaledJob update
Renamed events to either include ScaledJob, ScaledObject, or KEDA prefix
Added a controller for TriggerAuthentication and events for a newly added TriggerAuthentication or deleted one.
Opened Provide documentation for Kubernetes events keda-docs#361

Some remarks:

ScalersStarted event will still fire for all scalers on KEDA restart.
@tomkerkhove, Scaler unauthorized will be KEDAScalerFailed, but that will also happen for any other errors other than Unauthorized. Scalers themselves don't get passed the event recorder object, so errors from them are opaque to KEDA itself. Initially I wanted to avoid scaler authers having to deal with Kubernetes APIs too much. do you think this is sufficient?
@tomkerkhove Regarding "Scaledobject/SCaledJob is linked to trigger authentication", I'm not sure how best to do this tbh. This can happen on any ScaledObject/ScaledJob update. I can diff them on every update and emit those events, but currently we don't really store an easily enumerable list of ScaledObjects/ScaledJobs anywhere (each just gets a context and keeps checking the scalers until the context is canceled) I'll need to store references to all of them to be able to diff the old vs new, @zroubalik what do you think about that?

Signed-off-by: Ahmed ElSayed <ahmels@microsoft.com>

coderanger

Overall +1 from me on the API plumbing side.

coderanger · 2021-01-29T21:08:03Z

controllers/triggerauthentication_controller.go

+	}
+
+	if triggerAuthentication.ObjectMeta.Generation == 1 {
+		r.Recorder.Event(triggerAuthentication, corev1.EventTypeNormal, eventreason.TriggerAuthenticationAdded, "New TriggerAuthentication configured")


This seems like a slightly weird one, but I don't think it will do any harm :)

Yeah, we can go with this for now :)

coderanger · 2021-01-29T21:10:24Z

pkg/scaling/scale_handler.go

@@ -90,6 +95,8 @@ func (h *scaleHandler) HandleScalableObject(scalableObject interface{}) error {
 			cancelValue()
 		}
 		h.scaleLoopContexts.Store(key, cancel)
+	} else {
+		h.recorder.Event(withTriggers, corev1.EventTypeNormal, eventreason.KEDAScalersStarted, "Started scalers watch")


I think this would display on every restart of the controller? Can probably drop this and the scalers-stopped events since they don't correspond to actual actions, just internal code state.

Yeah, but how often do we want to(or would like to see) restart the controller? Is this happening that often? And in fact, with a restart the scalers do start watch, so the event message is correct.

tomkerkhove · 2021-01-30T08:59:28Z

Renamed events to either include ScaledJob, ScaledObject, or KEDA prefix

I've had a look kedacore/keda-docs#361 and it's a bit odd since some have the KEDA prefix and others don't. I know I've requested this, but if others think it's stupid I would remove it or add the prefix to all of them. Thoughts @zroubalik?

Just for context: The reason why I suggested this was for consumers since they typically process the whole event stream and this would make it easier for them to understand where these come from.

Opened kedacore/keda-docs#361

Thanks 💘

@tomkerkhove, Scaler unauthorized will be KEDAScalerFailed, but that will also happen for any other errors other than Unauthorized. Scalers themselves don't get passed the event recorder object, so errors from them are opaque to KEDA itself. Initially I wanted to avoid scaler authers having to deal with Kubernetes APIs too much. do you think this is sufficient?

That's not ideal as you might want to filter out to detect authentication issues, but we can still split them later on if this is too much trouble now. Thoughts @zroubalik?

@tomkerkhove Regarding "Scaledobject/SCaledJob is linked to trigger authentication", I'm not sure how best to do this tbh. This can happen on any ScaledObject/ScaledJob update. I can diff them on every update and emit those events, but currently we don't really store an easily enumerable list of ScaledObjects/ScaledJobs anywhere (each just gets a context and keeps checking the scalers until the context is canceled) I'll need to store references to all of them to be able to diff the old vs new, @zroubalik what do you think about that?

Let's leave this out then, we can still add it later on if need be?

zroubalik

Looking good, I like the renamed event names!

we should probably add a controller for ClusterTriggerAuthentication and cover this new resource as well.
ad "Scaledobject/SCaledJob is linked to trigger authentication" discussion - yeah, we would need to track it as you suggest. But I don't think is necessary to add this complexity now. We could add later if there's a need from community. what you say @tomkerkhove?

zroubalik · 2021-02-01T11:29:45Z

controllers/scaledjob_controller.go

 	scaleHandler      scaling.ScaleHandler
 }

 // SetupWithManager initializes the ScaledJobReconciler instance and starts a new controller managed by the passed Manager instance.
 func (r *ScaledJobReconciler) SetupWithManager(mgr ctrl.Manager) error {
-	r.scaleHandler = scaling.NewScaleHandler(mgr.GetClient(), nil, mgr.GetScheme(), r.GlobalHTTPTimeout)
+	r.scaleHandler = scaling.NewScaleHandler(mgr.GetClient(), nil, mgr.GetScheme(), r.GlobalHTTPTimeout, mgr.GetEventRecorderFor("scale-handler"))


Event recorder for Metrics Adapter is named keda-metrics-adapter and this one is named scale-handler. For consistency, this one could be maybe named keda-operator/ keda-controller and sync them with those set in main.go WDYT?

Yeah, I think that makes sense. I'll change it to one recorder with the name keda-operator

zroubalik · 2021-02-01T11:30:00Z

CHANGELOG.md

@@ -43,6 +43,7 @@
 - Global authentication credentials can be managed using `ClusterTriggerAuthentication` objects ([#1452](https://github.com/kedacore/keda/pull/1452))
 - Introducing OpenStack Swift scaler ([#1342](https://github.com/kedacore/keda/issues/1342))
 - Introducing MongoDB scaler ([#1467](https://github.com/kedacore/keda/pull/1467))
+- Emit Kubernetes Events on KEDA events ([#1523](https://github.com/kedacore/keda/pull/1523)):wq


Nit: This should be moved to Unreleased section above.

And remove the vim suffix at the end :)

zroubalik · 2021-02-01T11:38:49Z

controllers/triggerauthentication_controller.go

+	}
+
+	if triggerAuthentication.ObjectMeta.Generation == 1 {
+		r.Recorder.Event(triggerAuthentication, corev1.EventTypeNormal, eventreason.TriggerAuthenticationAdded, "New TriggerAuthentication configured")


Yeah, we can go with this for now :)

zroubalik · 2021-02-01T11:43:22Z

pkg/scaling/scale_handler.go

@@ -90,6 +95,8 @@ func (h *scaleHandler) HandleScalableObject(scalableObject interface{}) error {
 			cancelValue()
 		}
 		h.scaleLoopContexts.Store(key, cancel)
+	} else {
+		h.recorder.Event(withTriggers, corev1.EventTypeNormal, eventreason.KEDAScalersStarted, "Started scalers watch")


Yeah, but how often do we want to(or would like to see) restart the controller? Is this happening that often? And in fact, with a restart the scalers do start watch, so the event message is correct.

zroubalik · 2021-02-01T11:51:14Z

Renamed events to either include ScaledJob, ScaledObject, or KEDA prefix

I've had a look kedacore/keda-docs#361 and it's a bit odd since some have the KEDA prefix and others don't. I know I've requested this, but if others think it's stupid I would remove it or add the prefix to all of them. Thoughts @zroubalik?

I personally don't have a problem with ScaledJob, ScaledObject, or KEDA prefix.

@tomkerkhove, Scaler unauthorized will be KEDAScalerFailed, but that will also happen for any other errors other than Unauthorized. Scalers themselves don't get passed the event recorder object, so errors from them are opaque to KEDA itself. Initially I wanted to avoid scaler authers having to deal with Kubernetes APIs too much. do you think this is sufficient?

That's not ideal as you might want to filter out to detect authentication issues, but we can still split them later on if this is too much trouble now. Thoughts @zroubalik?

+1 split later if needed

@tomkerkhove Regarding "Scaledobject/SCaledJob is linked to trigger authentication", I'm not sure how best to do this tbh. This can happen on any ScaledObject/ScaledJob update. I can diff them on every update and emit those events, but currently we don't really store an easily enumerable list of ScaledObjects/ScaledJobs anywhere (each just gets a context and keeps checking the scalers until the context is canceled) I'll need to store references to all of them to be able to diff the old vs new, @zroubalik what do you think about that?

Let's leave this out then, we can still add it later on if need be?

+1

tomkerkhove · 2021-02-02T06:33:42Z

I personally don't have a problem with ScaledJob, ScaledObject, or KEDA prefix.

So you're ok with how they are or would you use KEDA prefix for all?

zroubalik · 2021-02-02T09:05:09Z

I personally don't have a problem with ScaledJob, ScaledObject, or KEDA prefix.

So you're ok with how they are or would you use KEDA prefix for all?

I am ok with how they are now.

ahmelsayed · 2021-02-04T23:30:49Z

Regarding the naming, I was initially looking how the default kubernetes events are named, and they were all named as Created, Deleted, Killing, Pulling, etc. So I assumed the pattern is to have a verb for the name and drive the meaning from the object the event is on. The current names have the prefix ScaledObject or ScaledJob, but the ones for scalers are shared for both since scalers are the same regardless of the target.

Signed-off-by: Ahmed ElSayed <ahmels@microsoft.com>

tomkerkhove

Sounds good to me, it's a lot better than the default ones :D

LGTM, let me know when the docs are updated!

Signed-off-by: Ahmed ElSayed <ahmels@microsoft.com>

tomkerkhove · 2021-02-06T06:44:13Z

🚀

tomkerkhove · 2021-02-15T06:49:44Z

pkg/scaling/scale_handler.go

@@ -90,6 +95,8 @@ func (h *scaleHandler) HandleScalableObject(scalableObject interface{}) error {
 			cancelValue()
 		}
 		h.scaleLoopContexts.Store(key, cancel)
+	} else {
+		h.recorder.Event(withTriggers, corev1.EventTypeNormal, eventreason.KEDAScalersStarted, "Started scalers watch")


@ahmelsayed Is it possible to add the name of every scaler/trigger here?

tomkerkhove · 2021-02-15T06:49:52Z

pkg/scaling/scale_handler.go

@@ -115,6 +122,7 @@ func (h *scaleHandler) DeleteScalableObject(scalableObject interface{}) error {
 			cancel()
 		}
 		h.scaleLoopContexts.Delete(key)
+		h.recorder.Event(withTriggers, corev1.EventTypeNormal, eventreason.KEDAScalersStopped, "Stopped scalers watch")


@ahmelsayed Is it possible to add the name of every scaler/trigger here?

* Emit kubernetes events from KEDA Signed-off-by: Ahmed ElSayed <ahmels@microsoft.com> * CR comments Signed-off-by: Ahmed ElSayed <ahmels@microsoft.com> * Fix CI errors Signed-off-by: Ahmed ElSayed <ahmels@microsoft.com> * goimports Signed-off-by: Ahmed ElSayed <ahmels@microsoft.com> * Code review comments Signed-off-by: Ahmed ElSayed <ahmels@microsoft.com> * Fix CHANGELOG.md Signed-off-by: Ahmed ElSayed <ahmels@microsoft.com>

ahmelsayed requested a review from zroubalik as a code owner January 21, 2021 22:16

ahmelsayed requested a review from tomkerkhove January 21, 2021 22:16

ahmelsayed force-pushed the ahmels/events branch from 7b0ca5a to b943f36 Compare January 21, 2021 22:17

coderanger reviewed Jan 21, 2021

View reviewed changes

ahmelsayed force-pushed the ahmels/events branch from b943f36 to 42e9f8b Compare January 21, 2021 22:54

zroubalik reviewed Jan 22, 2021

View reviewed changes

tomkerkhove reviewed Jan 22, 2021

View reviewed changes

pkg/eventreason/eventreason.go Show resolved Hide resolved

Emit kubernetes events from KEDA

f1ee747

Signed-off-by: Ahmed ElSayed <ahmels@microsoft.com>

ahmelsayed force-pushed the ahmels/events branch 2 times, most recently from 52306cd to d3191b5 Compare January 29, 2021 19:51

CR comments

2872b99

Signed-off-by: Ahmed ElSayed <ahmels@microsoft.com>

ahmelsayed force-pushed the ahmels/events branch from d3191b5 to 2872b99 Compare January 29, 2021 20:14

Fix CI errors

ac1ebca

Signed-off-by: Ahmed ElSayed <ahmels@microsoft.com>

goimports

492406d

Signed-off-by: Ahmed ElSayed <ahmels@microsoft.com>

coderanger reviewed Jan 29, 2021

View reviewed changes

tomkerkhove added this to the v2.2 milestone Jan 30, 2021

zroubalik reviewed Feb 1, 2021

View reviewed changes

Code review comments

4f817c0

Signed-off-by: Ahmed ElSayed <ahmels@microsoft.com>

tomkerkhove approved these changes Feb 5, 2021

View reviewed changes

Fix CHANGELOG.md

eb54f73

Signed-off-by: Ahmed ElSayed <ahmels@microsoft.com>

zroubalik approved these changes Feb 5, 2021

View reviewed changes

tomkerkhove merged commit aac70e6 into kedacore:main Feb 6, 2021

tomkerkhove reviewed Feb 15, 2021

View reviewed changes

zroubalik mentioned this pull request Mar 2, 2021

Emit Events for ClusterTriggerAuthentication #1647

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Emit kubernetes events from KEDA #1523

Emit kubernetes events from KEDA #1523

ahmelsayed commented Jan 21, 2021 •

edited by tomkerkhove

Loading

coderanger Jan 21, 2021

ahmelsayed Jan 21, 2021

coderanger Jan 21, 2021

zroubalik Jan 22, 2021

coderanger Jan 21, 2021

zroubalik Jan 22, 2021

tomkerkhove commented Jan 22, 2021 •

edited

Loading

zroubalik Jan 22, 2021 •

edited

Loading

zroubalik Jan 22, 2021

zroubalik Jan 22, 2021

zroubalik left a comment

tomkerkhove commented Jan 22, 2021 •

edited

Loading

ahmelsayed commented Jan 29, 2021

coderanger left a comment

coderanger Jan 29, 2021

zroubalik Feb 1, 2021

coderanger Jan 29, 2021

zroubalik Feb 1, 2021

tomkerkhove commented Jan 30, 2021

zroubalik left a comment

zroubalik Feb 1, 2021

ahmelsayed Feb 4, 2021

zroubalik Feb 1, 2021

zroubalik Feb 1, 2021

zroubalik Feb 1, 2021

zroubalik Feb 1, 2021

zroubalik commented Feb 1, 2021 •

edited

Loading

tomkerkhove commented Feb 2, 2021

zroubalik commented Feb 2, 2021

ahmelsayed commented Feb 4, 2021

tomkerkhove left a comment

tomkerkhove commented Feb 6, 2021

tomkerkhove Feb 15, 2021

tomkerkhove Feb 15, 2021

Emit kubernetes events from KEDA #1523

Emit kubernetes events from KEDA #1523

Conversation

ahmelsayed commented Jan 21, 2021 • edited by tomkerkhove Loading

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomkerkhove commented Jan 22, 2021 • edited Loading

zroubalik Jan 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zroubalik left a comment

Choose a reason for hiding this comment

tomkerkhove commented Jan 22, 2021 • edited Loading

ahmelsayed commented Jan 29, 2021

coderanger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomkerkhove commented Jan 30, 2021

zroubalik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zroubalik commented Feb 1, 2021 • edited Loading

tomkerkhove commented Feb 2, 2021

zroubalik commented Feb 2, 2021

ahmelsayed commented Feb 4, 2021

tomkerkhove left a comment

Choose a reason for hiding this comment

tomkerkhove commented Feb 6, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahmelsayed commented Jan 21, 2021 •

edited by tomkerkhove

Loading

tomkerkhove commented Jan 22, 2021 •

edited

Loading

zroubalik Jan 22, 2021 •

edited

Loading

tomkerkhove commented Jan 22, 2021 •

edited

Loading

zroubalik commented Feb 1, 2021 •

edited

Loading