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.

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

52 changes: 52 additions & 0 deletions control-plane/pkg/reconciler/consumergroup/consumergroup.go
Original file line number Diff line number Diff line change
Expand Up @@ -195,6 +195,9 @@
} else {
// If KEDA is not installed or autoscaler feature disabled, do nothing
cg.MarkAutoscalerDisabled()
if err := r.deleteKedaObjects(ctx); err != nil {

Check failure on line 198 in control-plane/pkg/reconciler/consumergroup/consumergroup.go

View workflow job for this annotation

GitHub Actions / analyze / Go vulnerability Detection

not enough arguments in call to r.deleteKedaObjects

Check failure on line 198 in control-plane/pkg/reconciler/consumergroup/consumergroup.go

View workflow job for this annotation

GitHub Actions / build / Build

not enough arguments in call to r.deleteKedaObjects

Check failure on line 198 in control-plane/pkg/reconciler/consumergroup/consumergroup.go

View workflow job for this annotation

GitHub Actions / test / Unit Tests

not enough arguments in call to r.deleteKedaObjects

Check failure on line 198 in control-plane/pkg/reconciler/consumergroup/consumergroup.go

View workflow job for this annotation

GitHub Actions / analyze / Analyze CodeQL

not enough arguments in call to r.deleteKedaObjects
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if err := r.deleteKedaObjects(ctx); err != nil {
if err := r.deleteKedaObjects(ctx, cg); err != nil {

return err
}
}

if err := r.reconcileConsumers(ctx, cg); err != nil {
Expand Down Expand Up @@ -845,3 +848,52 @@
func keyOf(cg metav1.Object) string {
return types.NamespacedName{Namespace: cg.GetNamespace(), Name: cg.GetName()}.String()
}

// deleteKedaObjects is responsible for deleting Keda (Kubernetes-based Event Driven Autoscaling) objects, "scaled
// objects" and "trigger authentications" from all available namespaces.
//
// 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, cg *kafkainternals.ConsumerGroup) error {

// check if scaled object exist, if not ignore
scaledObjectName := keda.GenerateScaledObjectName(cg)
_, 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)
}
Copy link
Member

@pierDipi pierDipi Feb 8, 2024

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

Suggested change
_, 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


// check if trigger authentication exist , if not ignore
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
}

_, 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)
}
Copy link
Member

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

Copy link
Member Author

Choose a reason for hiding this comment

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

@pierDipi, @Cali0707 that's nice, so we don't need to care so much about validation before deleting. I have just pushed a new commit with the proposed changes.

return nil
}
Loading