-
Notifications
You must be signed in to change notification settings - Fork 993
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.
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
chore: Adds CoudEvents when a ScaledObject scales a workload from/to zero or one #5632
base: main
Are you sure you want to change the base?
chore: Adds CoudEvents when a ScaledObject scales a workload from/to zero or one #5632
Conversation
… zero or one replicas Signed-off-by: neelanjan00 <neelanjan.manna@harness.io>
Signed-off-by: neelanjan00 <neelanjan.manna@harness.io>
Signed-off-by: neelanjan00 <neelanjan.manna@harness.io>
Can you open a PR for our docs as well please? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some small suggestions, but I was wondering if:
- You could add e2e tests
- Share an example of the cloudevents being emitted
@@ -28,4 +28,8 @@ const ( | |||
ScaleTargetNotFoundMsg = "Target resource doesn't exist" | |||
|
|||
ScaleTargetNoSubresourceMsg = "Target resource doesn't expose /scale subresource" | |||
|
|||
ScaleTargetFromZero = "Target resource is scaling up from zero number of replicas" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ScaleTargetFromZero = "Target resource is scaling up from zero number of replicas" | |
ScaleTargetFromZero = "Target resource is scaling out from zero" |
|
||
ScaleTargetFromZero = "Target resource is scaling up from zero number of replicas" | ||
|
||
ScaleTargetToZero = "Target resource is scaling down to zero number of replicas" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ScaleTargetToZero = "Target resource is scaling down to zero number of replicas" | |
ScaleTargetToZero = "Target resource is scaling in to zero replicas" |
@neelanjan00 any update on this please? |
eventEmitter.Emit() will emit a k8s event at the same time. Replacing the recorder with eventEmitter and r.Recorder.Event() with eventEmitter.Emit() is better than passing an eventEmitter as a new parameter. Alternatively, we could pass an eventEmitter as a new parameter and delete the recorder once all r.Recorder.Event() calls have been replaced. |
@@ -272,6 +275,13 @@ func (e *scaleExecutor) scaleToZeroOrIdle(ctx context.Context, logger logr.Logge | |||
|
|||
e.recorder.Eventf(scaledObject, corev1.EventTypeNormal, eventreason.KEDAScaleTargetDeactivated, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one and the new one are duplicated. We should merge these two into one event. eventEmitter.Emit() includes emitting k8s event.
corev1.EventTypeNormal, eventemitter.ScaleFromZeroType, eventreason.KEDAScaleTargetActivated, | ||
message.ScaleTargetFromZero) | ||
} | ||
|
||
e.recorder.Eventf(scaledObject, corev1.EventTypeNormal, eventreason.KEDAScaleTargetActivated, "Scaled %s %s/%s from %d to %d", scaledObject.Status.ScaleTargetKind, scaledObject.Namespace, scaledObject.Spec.ScaleTargetRef.Name, currentReplicas, replicas) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one and the new one are duplicated. We should merge these two into one event.
@@ -80,7 +80,7 @@ func init() { | |||
|
|||
// SetupWithManager initializes the ScaledJobReconciler instance and starts a new controller managed by the passed Manager instance. | |||
func (r *ScaledJobReconciler) SetupWithManager(mgr ctrl.Manager, options controller.Options) error { | |||
r.scaleHandler = scaling.NewScaleHandler(mgr.GetClient(), nil, mgr.GetScheme(), r.GlobalHTTPTimeout, mgr.GetEventRecorderFor("scale-handler"), r.SecretsLister) | |||
r.scaleHandler = scaling.NewScaleHandler(mgr.GetClient(), nil, mgr.GetScheme(), r.GlobalHTTPTimeout, mgr.GetEventRecorderFor("scale-handler"), r.SecretsLister, nil) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
create and pass a new event emitter as well so that it doesn't need to check nil later.
We are planning release on Thursday, any chance we can incorporate the feedback? |
Hey @tomkerkhove, @zroubalik sorry for not being active on this! I am presently occupied with some office work so not able to put time away for this, not sure if I will be able to add the E2E tests on time for the release. |
No worries! |
Checklist
Relates to #3527