feat: remove keda objects during reconciliation #3676

converge · 2024-02-06T17:03:44Z

Proposed Changes

Remove Keda objects during reconciliation if [configmap] config-kafka-features/controller-autoscaler-keda is set to disabled.

- remove Keda objects during reconciliation

early feedback welcome / will add unit tests before marking it as ready

knative-prow · 2024-02-06T17:03:53Z

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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

converge · 2024-02-06T17:06:00Z

/cc @Cali0707

codecov · 2024-02-06T18:00:29Z

Codecov Report

Attention: Patch coverage is 31.42857% with 24 lines in your changes are missing coverage. Please review.

Project coverage is 52.40%. Comparing base (02a879b) to head (ffcae53).
Report is 118 commits behind head on main.

Files	Patch %	Lines
...lane/pkg/reconciler/consumergroup/consumergroup.go	31.42%	23 Missing and 1 partial ⚠️

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

Flag	Coverage Δ
java-unittests	`74.16% <ø> (-0.30%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Cali0707 · 2024-02-06T18:23:43Z

control-plane/pkg/reconciler/consumergroup/consumergroup.go

+//
+// 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 {


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

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),

},

that sounds like a good idea. I tried to follow that on my last commit.

@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>

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

pierDipi · 2024-02-08T13:09:12Z

control-plane/pkg/reconciler/consumergroup/consumergroup.go

+	_, 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)
+	}


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

pierDipi · 2024-02-08T13:09:33Z

control-plane/pkg/reconciler/consumergroup/consumergroup.go

+	_, 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)
+	}


similar idea here, we can just call Delete

@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.

pierDipi · 2024-02-08T13:09:50Z

control-plane/pkg/reconciler/consumergroup/consumergroup.go

@@ -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 {


Suggested change

if err := r.deleteKedaObjects(ctx); err != nil {

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

Cali0707

@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!

…snt exist

converge · 2024-02-10T20:44:11Z

@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
I pushed one commit to fix the unit test. are the unit test auto generated somehow? should I write more unit tests?

pierDipi · 2024-02-14T16:23:10Z

/ok-to-test

converge · 2024-02-14T17:20:24Z

/retest

converge · 2024-02-15T15:33:48Z

/retest

converge · 2024-02-17T19:46:31Z

/retest

converge · 2024-02-23T17:41:04Z

/retest

converge · 2024-02-26T11:47:20Z

/retest

Cali0707 · 2024-02-26T13:18:33Z

control-plane/pkg/reconciler/consumergroup/consumergroup.go

+	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")
+	}


@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

@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!

Cali0707

/lgtm
/approve

Thanks for all your persistence on this @converge 🎉

knative-prow · 2024-02-28T15:07:27Z

[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:

~~OWNERS~~ [Cali0707]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

converge · 2024-02-28T17:03:27Z

/retest

converge · 2024-02-28T18:53:17Z

/retest

converge · 2024-03-01T14:14:31Z

/retest

pierDipi · 2024-04-03T15:37:31Z

/retest-required

pierDipi · 2024-04-04T11:42:12Z

/retest

pierDipi · 2024-04-30T13:10:20Z

/retest-required

Cali0707 · 2024-05-01T15:24:05Z

/test reconciler-tests-namespaced-broker

pierDipi · 2024-05-09T15:22:49Z

/test reconciler-tests-namespaced-broker

pierDipi · 2024-05-14T14:43:07Z

/retest-required

pierDipi · 2024-05-15T07:04:17Z

/test reconciler-tests-namespaced-broker

knative-prow · 2024-05-15T08:16:51Z

@converge: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
reconciler-tests-namespaced-broker_eventing-kafka-broker_main	`ffcae53`	link	true	`/test reconciler-tests-namespaced-broker`

Your PR dashboard.

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. I understand the commands that are listed here.

converge added 2 commits February 6, 2024 13:57

feat: remove keda objects during reconciliation

ed14fa0

chore: add better error description

9e62fdd

knative-prow bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. area/control-plane labels Feb 6, 2024

knative-prow bot requested review from Leo6Leo and pierDipi February 6, 2024 17:03

knative-prow bot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 6, 2024

knative-prow bot requested a review from Cali0707 February 6, 2024 17:06

Cali0707 reviewed Feb 6, 2024

View reviewed changes

feat: remove only keda objects created by knative eventing

c94a6b4

pierDipi reviewed Feb 8, 2024

View reviewed changes

chore: delete scaled objects and ignore 404 errors

36e3b3a

converge requested review from Cali0707 and pierDipi February 8, 2024 20:36

Cali0707 reviewed Feb 9, 2024

View reviewed changes

chore: skip trigger authentication deletion in case scaled object doe…

2099976

…snt exist

converge marked this pull request as ready for review February 10, 2024 20:46

knative-prow bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 10, 2024

knative-prow bot requested review from aliok and lionelvillard February 10, 2024 20:46

knative-prow bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 14, 2024

converge mentioned this pull request Feb 14, 2024

[unit tests] reconciliation removal of keda objects #3701

Open

Cali0707 reviewed Feb 26, 2024

View reviewed changes

converge added 2 commits February 27, 2024 10:02

chore: delete scaled object only if its controlled by knative eventing

1ae09da

chore: add extra validation

ffcae53

Cali0707 approved these changes Feb 28, 2024

View reviewed changes

knative-prow bot assigned Cali0707 Feb 28, 2024

knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Feb 28, 2024

knative-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 28, 2024

	OwnerReferences: []metav1.OwnerReference{
	*kmeta.NewControllerRef(cg),
	},

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

feat: remove keda objects during reconciliation #3676

Are you sure you want to change the base?

feat: remove keda objects during reconciliation #3676

Conversation

converge commented Feb 6, 2024 • edited

Proposed Changes

knative-prow bot commented Feb 6, 2024

converge commented Feb 6, 2024

codecov bot commented Feb 6, 2024 • edited

Codecov Report

Cali0707 Feb 6, 2024 • edited

Choose a reason for hiding this comment

pierDipi Feb 7, 2024 • edited

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

pierDipi Feb 8, 2024 • edited

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

Cali0707 left a comment

Choose a reason for hiding this comment

converge commented Feb 10, 2024

pierDipi commented Feb 14, 2024

converge commented Feb 14, 2024

converge commented Feb 15, 2024

converge commented Feb 17, 2024

converge commented Feb 23, 2024

converge commented Feb 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Cali0707 left a comment

Choose a reason for hiding this comment

knative-prow bot commented Feb 28, 2024

converge commented Feb 28, 2024

converge commented Feb 28, 2024

converge commented Mar 1, 2024

pierDipi commented Apr 3, 2024

pierDipi commented Apr 4, 2024

pierDipi commented Apr 30, 2024

Cali0707 commented May 1, 2024

pierDipi commented May 9, 2024

pierDipi commented May 14, 2024

pierDipi commented May 15, 2024

knative-prow bot commented May 15, 2024

converge commented Feb 6, 2024 •

edited

codecov bot commented Feb 6, 2024 •

edited

Cali0707 Feb 6, 2024 •

edited

pierDipi Feb 7, 2024 •

edited

pierDipi Feb 8, 2024 •

edited