-
Notifications
You must be signed in to change notification settings - Fork 117
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
feat: remove keda objects during reconciliation #3676
feat: remove keda objects during reconciliation #3676
Conversation
Hi @converge. Thanks for your PR. I'm waiting for a knative-extensions member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/cc @Cali0707 |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3676 +/- ##
============================================
- Coverage 61.99% 52.40% -9.60%
- Complexity 845 875 +30
============================================
Files 188 342 +154
Lines 12634 21449 +8815
Branches 273 284 +11
============================================
+ Hits 7833 11241 +3408
- Misses 4187 9296 +5109
- Partials 614 912 +298
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
// | ||
// When Keda creates a scaled object, it also generates a Horizontal Pod Autoscaler. This resource will also be removed | ||
// when the scaled object is deleted. | ||
func (r *Reconciler) deleteKedaObjects(ctx context.Context) error { |
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.
It looks like this will delete all KEDA scaled objects and trigger authentications in the cluster, not just those created by the kafka-controller. Maybe we could leverage the fact that the name of the scaledobject (in keda.GenerateScaledObject) and trigger authentication (in keda.GenerateTriggerAuthentication) for each consumergroup is deterministic and we can generate it each time?
Then, since the ReconcileKind
function will be called once per consumergroup when we update the configmap, we could:
- Get the name of the scaledobject associated with the consumergroup and then delete it
- Get the name of the trigger authentication associated with the consumergroup and then delete it
This way, we won't delete any extra KEDA resources
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.
In addition to that there is also a method we could leverage metav1.IsControlledBy(<keda-object>, <consumergroup>)
as we set the consumer group to be the controller of keda objects here:
eventing-kafka-broker/control-plane/pkg/autoscaler/keda/keda.go
Lines 132 to 134 in 2c8b901
OwnerReferences: []metav1.OwnerReference{ | |
*kmeta.NewControllerRef(cg), | |
}, |
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.
that sounds like a good idea. I tried to follow that on my last commit.
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.
@pierDipi sorry, I got your message just after I hit comment/send. could you double check my last commit? not sure if we still need the metav1.IsControlledBy(<keda-object>
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.
I would use that function as it's a more reliable and readable than the check we're anyway implemented here
var triggerAuthName string
if len(cg.ObjectMeta.OwnerReferences) > 0 && cg.ObjectMeta.OwnerReferences[0].UID != "" {
triggerAuthName = string(cg.ObjectMeta.OwnerReferences[0].UID)
}
// skip deleting trigger authentication if object reference doesn't exist
if triggerAuthName == "" {
return nil
}
For example, it's not necessarily guaranteed that first item is our object (even though it's the case currently) it would be more future proof to loom owner references
_, err := r.KedaClient.KedaV1alpha1().ScaledObjects(cg.Namespace).Get(ctx, scaledObjectName, metav1.GetOptions{}) | ||
if err != nil && !apierrors.IsNotFound(err) { | ||
return fmt.Errorf("failed to get Keda scaled object: %w", err) | ||
} | ||
if !apierrors.IsNotFound(err) { | ||
err = r.KedaClient.KedaV1alpha1().ScaledObjects(cg.Namespace).Delete(ctx, scaledObjectName, metav1.DeleteOptions{}) | ||
if err != nil { | ||
return fmt.Errorf("failed to delete Keda scaled object: %w", err) | ||
} | ||
logging.FromContext(ctx).Debugw("Keda scaled object deleted", | ||
"name", scaledObjectName, | ||
"namespace", cg.Namespace) | ||
} |
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.
Instead of GET
and then DELETE
, we can just call DELETE
and ignore "404", also because it's not guaranteed that DELETE will not return 404
(not found) as anything can happen between GET and DELETE
_, err := r.KedaClient.KedaV1alpha1().ScaledObjects(cg.Namespace).Get(ctx, scaledObjectName, metav1.GetOptions{}) | |
if err != nil && !apierrors.IsNotFound(err) { | |
return fmt.Errorf("failed to get Keda scaled object: %w", err) | |
} | |
if !apierrors.IsNotFound(err) { | |
err = r.KedaClient.KedaV1alpha1().ScaledObjects(cg.Namespace).Delete(ctx, scaledObjectName, metav1.DeleteOptions{}) | |
if err != nil { | |
return fmt.Errorf("failed to delete Keda scaled object: %w", err) | |
} | |
logging.FromContext(ctx).Debugw("Keda scaled object deleted", | |
"name", scaledObjectName, | |
"namespace", cg.Namespace) | |
} | |
err := r.KedaClient.KedaV1alpha1().ScaledObjects(cg.Namespace).Delete(ctx, scaledObjectName, metav1.DeleteOptions{}) | |
if err != nil && !apierrors.IsNotFound(err) { | |
return fmt.Errorf("failed to get Keda scaled object: %w", err) | |
} | |
logging.FromContext(ctx).Debugw("Keda scaled object deleted", "name", scaledObjectName, "namespace", cg.Namespace) | |
I don't know if the suggestion is properly formatted, it's mostly for describing the idea better
_, err = r.KedaClient.KedaV1alpha1().TriggerAuthentications(cg.Namespace).Get(ctx, triggerAuthName, metav1.GetOptions{}) | ||
if err != nil && !apierrors.IsNotFound(err) { | ||
return fmt.Errorf("failed to get Keda trigger authenticatoin") | ||
} | ||
if !apierrors.IsNotFound(err) { | ||
err = r.KedaClient.KedaV1alpha1().TriggerAuthentications(cg.Namespace).Delete(ctx, triggerAuthName, metav1.DeleteOptions{}) | ||
if err != nil { | ||
return fmt.Errorf("failed to delete Keda trigger authentication: %w", err) | ||
} | ||
logging.FromContext(ctx).Debugw("Keda trigger authentication deleted", | ||
"name", triggerAuthName, | ||
"namespace", cg.Namespace) | ||
} |
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.
similar idea here, we can just call Delete
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.
@@ -195,6 +195,9 @@ func (r *Reconciler) ReconcileKind(ctx context.Context, cg *kafkainternals.Consu | |||
} else { | |||
// If KEDA is not installed or autoscaler feature disabled, do nothing | |||
cg.MarkAutoscalerDisabled() | |||
if err := r.deleteKedaObjects(ctx); err != 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.
if err := r.deleteKedaObjects(ctx); err != nil { | |
if err := r.deleteKedaObjects(ctx, cg); err != 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.
@converge I think this set of changes looks good to me, would you be able to work on updating the unit tests so that they pass with the new changes?
If you want help with that (our unit tests can take some getting used to 😆 ), feel free to ping me on slack!
thanks xD |
/ok-to-test |
/retest |
/retest |
3 similar comments
/retest |
/retest |
/retest |
err = r.KedaClient.KedaV1alpha1().ScaledObjects(cg.Namespace).Delete(ctx, scaledObjectName, metav1.DeleteOptions{}) | ||
if err != nil && !apierrors.IsNotFound(err) { | ||
return fmt.Errorf("failed to delete Keda scaled object: %w", err) | ||
} | ||
if err == nil { | ||
logging.FromContext(ctx).Infoln("Keda scaled object deleted") | ||
} |
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.
@converge sorry I missed this earlier, but this should also be in the if metav1.IsControlledBy()
statement, as we don't want to delete the scaled object if we don't control it
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.
@converge sorry I missed this earlier, but this should also be in the
if metav1.IsControlledBy()
statement, as we don't want to delete the scaled object if we don't control it
make sense! I have just updated, tks!
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.
/lgtm
/approve
Thanks for all your persistence on this @converge 🎉
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Cali0707, converge The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/retest |
2 similar comments
/retest |
/retest |
/retest-required |
/retest |
/retest-required |
/test reconciler-tests-namespaced-broker |
1 similar comment
/test reconciler-tests-namespaced-broker |
/retest-required |
/test reconciler-tests-namespaced-broker |
1 similar comment
/test reconciler-tests-namespaced-broker |
/override reconciler-tests-namespaced-broker |
@pierDipi: pierDipi unauthorized: /override is restricted to Repo administrators. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
/retest-required |
97732b7
into
knative-extensions:main
Fixes #3669
Proposed Changes
Remove Keda objects during reconciliation if [configmap] config-kafka-features/controller-autoscaler-keda is set to
disabled
.