only requeue consumergroups from appropriate statefulset on change #3825

Cali0707 · 2024-04-05T19:55:33Z

Fixes #3824

Proposed Changes

Check the owner kind for the consumergroup and only requeue it if necessary for a given statefulset change

Signed-off-by: Calum Murray <cmurray@redhat.com>

Cali0707 · 2024-04-05T19:55:44Z

/cc @pierDipi @creydr @Leo6Leo

codecov · 2024-04-05T20:01:14Z

Codecov Report

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

Project coverage is 49.49%. Comparing base (551691c) to head (6814585).
Report is 38 commits behind head on main.

Files	Patch %	Lines
...l-plane/pkg/reconciler/consumergroup/controller.go	4.76%	18 Missing and 2 partials ⚠️
...apis/internals/kafka/eventing/v1alpha1/register.go	0.00%	9 Missing ⚠️
...ontrol-plane/pkg/reconciler/consumer/controller.go	0.00%	6 Missing and 1 partial ⚠️

Additional details and impacted files

@@              Coverage Diff              @@
##               main    #3825       +/-   ##
=============================================
- Coverage     73.90%   49.49%   -24.41%     
=============================================
  Files           100      246      +146     
  Lines          3407    14817    +11410     
  Branches        288        0      -288     
=============================================
+ Hits           2518     7334     +4816     
- Misses          716     6730     +6014     
- Partials        173      753      +580

Flag	Coverage Δ
java-unittests	`?`

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.

pierDipi · 2024-04-08T07:18:37Z

/retest-required

pierDipi · 2024-04-08T08:14:12Z

control-plane/pkg/apis/internals/kafka/eventing/v1alpha1/register.go

+func GetOwnerKindFromStatefulSetName(name string) (string, bool) {
+	switch name {
+	case SourceStatefulSetName:
+		return "KafkaSource", true
+	case ChannelStatefulSetName:
+		return "KafkaChannel", true
+	case BrokerStatefulSetName:
+		return "Trigger", true
+	}
+	return "", false
+}


Can't we use this one?

eventing-kafka-broker/control-plane/pkg/apis/internals/kafka/eventing/v1alpha1/consumer_group_types.go

Lines 199 to 211 in 551691c

// GetUserFacingResourceRef gets the resource reference to the user-facing resources

// that are backed by this ConsumerGroup using the OwnerReference list.

func (cg *ConsumerGroup) GetUserFacingResourceRef() *metav1.OwnerReference {

for i, or := range cg.OwnerReferences {

// TODO hardcoded resource kinds.

if strings.EqualFold(or.Kind, "trigger") ||

strings.EqualFold(or.Kind, "kafkasource") ||

strings.EqualFold(or.Kind, "kafkachannel") {

return &cg.OwnerReferences[i]

}

}

return nil

}

This one goes consumergroup -> ownerReference, we need to go statefulset -> ownerReference.Kind

pierDipi · 2024-04-08T12:43:32Z

control-plane/pkg/apis/internals/kafka/eventing/v1alpha1/register.go

+func GetOwnerKindFromStatefulSetName(name string) (string, bool) {
+	switch name {
+	case SourceStatefulSetName:
+		return "KafkaSource", true
+	case ChannelStatefulSetName:
+		return "KafkaChannel", true
+	case BrokerStatefulSetName:
+		return "Trigger", true
+	}
+	return "", false
+}


pierDipi · 2024-04-08T12:46:04Z

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

@@ -189,8 +189,30 @@ func NewController(ctx context.Context, watcher configmap.Watcher) *controller.I
 	})
 	consumerInformer.Informer().AddEventHandler(controller.HandleAll(enqueueConsumerGroupFromConsumer(impl.EnqueueKey)))

-	globalResync := func(interface{}) {
-		impl.GlobalResync(consumerGroupInformer.Informer())
+	globalResync := func(obj interface{}) {


Should we rename this variable?

This is not a globalResync anymore as it's dealing with statefulset changes vs the primary controlled objects

If we don't plan to reuse this anywhere I think we can inline this on the ResyncOnStatefulSetChange call as it's now very specific

Signed-off-by: Calum Murray <cmurray@redhat.com>

Cali0707 · 2024-04-11T14:50:07Z

/cc @pierDipi

Signed-off-by: Calum Murray <cmurray@redhat.com>

pierDipi · 2024-04-11T15:35:01Z

control-plane/pkg/reconciler/consumer/controller.go

@@ -90,7 +90,7 @@ func NewController(ctx context.Context, watcher configmap.Watcher) *controller.I
 		impl.GlobalResync(consumerInformer.Informer())
 	}

-	cgreconciler.ResyncOnStatefulSetChange(ctx, globalResync)
+	cgreconciler.ResyncOnStatefulSetChange(ctx, impl.FilteredGlobalResync, consumerInformer.Informer())


The provided informer here will have consumers but in the function we do:

filteredResync(func(i interface{}) bool { cg, ok := i.(*kafkainternals.ConsumerGroup) if !ok { return false }

that makes sense, thanks for catching this! (just saved me lots of debugging time)

Signed-off-by: Calum Murray <cmurray@redhat.com>

Cali0707 · 2024-04-11T19:54:50Z

/retest-required

Cali0707 · 2024-04-11T19:56:24Z

/retest-required

pierDipi · 2024-04-12T07:29:17Z

/retest

Cali0707 · 2024-04-12T11:36:21Z

/retest-required

Cali0707 · 2024-04-12T11:36:34Z

/cc @pierDipi

pierDipi · 2024-04-12T12:12:17Z

control-plane/pkg/reconciler/consumer/controller.go

+		}
+
+		for _, ref := range c.GetOwnerReferences() {
+			if ref.Kind == "ConsumerGroup" {


I'd use case insensitive match as it's more robust

Suggested change

if ref.Kind == "ConsumerGroup" {

if strings.EqualFold(ref.Kind, "ConsumerGroup") {

(Make sure the import is there)

Actually, there is this function

eventing-kafka-broker/control-plane/pkg/apis/internals/kafka/eventing/v1alpha1/consumer_types.go

Lines 258 to 267 in 0c5c2e5

// GetConsumerGroup gets the resource reference to the ConsumerGroup

// using the OwnerReference list.

func (c *Consumer) GetConsumerGroup() *metav1.OwnerReference {

for i, or := range c.OwnerReferences {

if strings.EqualFold(or.Kind, ConsumerGroupGroupVersionKind.Kind) {

return &c.OwnerReferences[i]

}

}

return nil

}

Signed-off-by: Calum Murray <cmurray@redhat.com>

Cali0707 · 2024-04-12T13:02:25Z

/cc @pierDipi

pierDipi

/lgtm
/approve

knative-prow · 2024-04-12T16:00:26Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Cali0707, pierDipi

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,pierDipi]

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

pierDipi · 2024-04-12T16:00:49Z

control-plane/pkg/reconciler/consumer/controller.go

+			return nil, false
+		}
+
+		cgRef := c.GetConsumerGroup()


nit: We would need to check for non nil here

pierDipi · 2024-04-12T16:01:06Z

/retest-required

Cali0707 · 2024-04-12T18:07:02Z

/test integration-tests

Cali0707 · 2024-04-18T08:27:54Z

/retest-required

Cali0707 · 2024-04-22T13:06:29Z

/retest-required

pierDipi · 2024-04-23T10:22:58Z

/retest-required

pierDipi · 2024-04-23T13:27:42Z

/retest-required

Cali0707 · 2024-04-25T14:45:44Z

/retest-required

Cali0707 · 2024-04-30T13:43:01Z

/retest

Cali0707 · 2024-04-30T16:19:05Z

/retest-required

pierDipi · 2024-05-03T08:43:15Z


    triggerv2_downgrade.go:54: failed to downgrade from triggerv2 failed to get deployment: client rate limiter Wait returned an error: context deadline exceeded

Cali0707 · 2024-05-03T14:41:07Z


    triggerv2_downgrade.go:54: failed to downgrade from triggerv2 failed to get deployment: client rate limiter Wait returned an error: context deadline exceeded

@pierDipi this should be fixed by #3868

/test upgrade-tests

pierDipi · 2024-05-06T06:31:44Z

/retest-required

pierDipi · 2024-05-21T12:58:50Z

/test upgrade-tests

knative-prow · 2024-05-21T16:37:23Z

@Cali0707: The following tests 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	`6814585`	link	unknown	`/test reconciler-tests-namespaced-broker`
reconciler-tests_eventing-kafka-broker_main	`6814585`	link	unknown	`/test reconciler-tests`

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.

only requeue consumergroups from appropriate statefulset on change

ffa7ee9

Signed-off-by: Calum Murray <cmurray@redhat.com>

knative-prow bot added approved Indicates a PR has been approved by an approver from all required OWNERS files. area/control-plane size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 5, 2024

knative-prow bot requested review from Leo6Leo and lionelvillard April 5, 2024 19:55

knative-prow bot requested review from creydr and pierDipi April 5, 2024 19:55

pierDipi reviewed Apr 8, 2024

View reviewed changes

inline resync logic

83cb99a

Signed-off-by: Calum Murray <cmurray@redhat.com>

knative-prow bot requested a review from pierDipi April 11, 2024 14:50

fix the consumer controller build error

81ceb79

Signed-off-by: Calum Murray <cmurray@redhat.com>

pierDipi reviewed Apr 11, 2024

View reviewed changes

Cali0707 added 2 commits April 11, 2024 15:24

fix: re-queue with correct informer

0c1e616

Signed-off-by: Calum Murray <cmurray@redhat.com>

cleanup: remove unused import

f092649

Signed-off-by: Calum Murray <cmurray@redhat.com>

knative-prow bot requested a review from pierDipi April 12, 2024 11:36

pierDipi reviewed Apr 12, 2024

View reviewed changes

use strings.EqualFold

6814585

Signed-off-by: Calum Murray <cmurray@redhat.com>

knative-prow bot requested a review from pierDipi April 12, 2024 13:02

pierDipi approved these changes Apr 12, 2024

View reviewed changes

knative-prow bot assigned pierDipi Apr 12, 2024

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

pierDipi reviewed Apr 12, 2024

View reviewed changes

	// GetUserFacingResourceRef gets the resource reference to the user-facing resources
	// that are backed by this ConsumerGroup using the OwnerReference list.
	func (cg ConsumerGroup) GetUserFacingResourceRef() metav1.OwnerReference {
	for i, or := range cg.OwnerReferences {
	// TODO hardcoded resource kinds.
	if strings.EqualFold(or.Kind, "trigger") \|\|
	strings.EqualFold(or.Kind, "kafkasource") \|\|
	strings.EqualFold(or.Kind, "kafkachannel") {
	return &cg.OwnerReferences[i]
	}
	}
	return nil
	}

	if ref.Kind == "ConsumerGroup" {
	if strings.EqualFold(ref.Kind, "ConsumerGroup") {

	// GetConsumerGroup gets the resource reference to the ConsumerGroup
	// using the OwnerReference list.
	func (c Consumer) GetConsumerGroup() metav1.OwnerReference {
	for i, or := range c.OwnerReferences {
	if strings.EqualFold(or.Kind, ConsumerGroupGroupVersionKind.Kind) {
	return &c.OwnerReferences[i]
	}
	}
	return nil
	}

only requeue consumergroups from appropriate statefulset on change #3825

Are you sure you want to change the base?

only requeue consumergroups from appropriate statefulset on change #3825

Conversation

Cali0707 commented Apr 5, 2024

Proposed Changes

Cali0707 commented Apr 5, 2024

codecov bot commented Apr 5, 2024 • edited

Codecov Report

pierDipi commented Apr 8, 2024

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

Cali0707 commented Apr 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Cali0707 commented Apr 11, 2024

Cali0707 commented Apr 11, 2024

pierDipi commented Apr 12, 2024

Cali0707 commented Apr 12, 2024

Cali0707 commented Apr 12, 2024

pierDipi Apr 12, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Cali0707 commented Apr 12, 2024

pierDipi left a comment

Choose a reason for hiding this comment

knative-prow bot commented Apr 12, 2024

Choose a reason for hiding this comment

pierDipi commented Apr 12, 2024

Cali0707 commented Apr 12, 2024

Cali0707 commented Apr 18, 2024

Cali0707 commented Apr 22, 2024

pierDipi commented Apr 23, 2024

pierDipi commented Apr 23, 2024

Cali0707 commented Apr 25, 2024

Cali0707 commented Apr 30, 2024

Cali0707 commented Apr 30, 2024

pierDipi commented May 3, 2024

Cali0707 commented May 3, 2024

pierDipi commented May 6, 2024

pierDipi commented May 21, 2024

knative-prow bot commented May 21, 2024

codecov bot commented Apr 5, 2024 •

edited

pierDipi Apr 12, 2024 •

edited