feat: channels use consumergroups #3816

Cali0707 · 2024-04-04T20:09:53Z

As part of the KEDA Kafka component scaling, we need to move channels to use consumergroups internally. This makes that change.

Proposed Changes

Use the channelv2 reconciler implementation
Use statefulsets instead of deployments for the channel

Release Note

Channels now use consumergroups internally. IMPORTANT: to downgrade from this release, you will need to delete any consumergroups owned by a channel, and delete the kafka-channel-receiver and kafka-channel-dispatcher statefulsets

Docs

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

Cali0707 · 2024-04-04T20:14:35Z

@pierDipi I'm planning on removing the channel v1 implementation in a follow up PR, so that it's easier to see any changes I make in this PR (rather than having a complicated diff)

codecov · 2024-04-04T20:15:19Z

Codecov Report

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

Project coverage is 48.31%. Comparing base (483904b) to head (1c21e04).
Report is 1 commits behind head on main.

❗ Current head 1c21e04 differs from pull request most recent head 29e5234

Please upload reports for the commit 29e5234 to get more accurate results.

Files	Patch %	Lines
control-plane/pkg/reconciler/channel/channel.go	73.47%	46 Missing and 15 partials ⚠️
...d/post-install/kafka_channel_deployment_deleter.go	0.00%	7 Missing ⚠️
control-plane/pkg/reconciler/channel/controller.go	88.00%	2 Missing and 1 partial ⚠️
control-plane/cmd/post-install/main.go	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3816      +/-   ##
==========================================
- Coverage   49.24%   48.31%   -0.94%     
==========================================
  Files         247      246       -1     
  Lines       14925    14528     -397     
==========================================
- Hits         7350     7019     -331     
+ Misses       6816     6800      -16     
+ Partials      759      709      -50

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

Cali0707 · 2024-04-04T20:40:11Z

Looks like everything is broken 😅
/hold

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

Cali0707 · 2024-04-12T18:27:52Z

/cc @creydr @pierDipi

I made some changes around OIDC/TrustBundles for channelv2, where I am a lot less familiar with the code. Would you mind taking a look at the changes and seeing if they make sense?

Cali0707 · 2024-04-12T18:42:52Z

/unhold

I think things should work now (the failing tests passed when I ran them locally, I think they are flaky in CI)

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

creydr · 2024-04-16T06:13:44Z

/retest

Cali0707 · 2024-05-14T17:56:31Z

/test integration-tests

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

Cali0707 · 2024-05-28T14:38:33Z

/test reconciler-tests

pierDipi · 2024-05-28T16:23:28Z

control-plane/cmd/kafka-controller/main.go

@@ -33,7 +33,7 @@ import (
 	"knative.dev/eventing-kafka-broker/control-plane/pkg/config"
 	"knative.dev/eventing-kafka-broker/control-plane/pkg/kafka/clientpool"
 	"knative.dev/eventing-kafka-broker/control-plane/pkg/reconciler/broker"
-	"knative.dev/eventing-kafka-broker/control-plane/pkg/reconciler/channel"
+	channelv2 "knative.dev/eventing-kafka-broker/control-plane/pkg/reconciler/channel"


channelv2 -> channel

whoops, looks like I missed this in the refactor 🤦

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

Cali0707 · 2024-05-28T16:58:14Z

/cc @pierDipi

Fixed the channelv2 name

pierDipi · 2024-05-29T08:39:15Z


    prober.go:78: There were 27029 events propagated, but 404 errors occurred. Listing them below.

/test upgrade-tests

pierDipi · 2024-05-29T08:39:32Z

/test reconciler-tests-namespaced-broker

pierDipi · 2024-05-29T10:02:08Z

Worked, retesting
/test upgrade-tests

pierDipi · 2024-05-29T10:02:14Z

/test reconciler-tests-namespaced-broker

pierDipi · 2024-05-29T10:05:07Z

control-plane/cmd/post-install/kafka_broker_deployment_deleter.go

should the file be called kafka_channel_... ?

test/upgrade/installation/channelv2_downgrade.go

pierDipi · 2024-05-29T12:37:34Z

/test upgrade-tests

pierDipi · 2024-05-30T09:08:27Z

control-plane/pkg/reconciler/channel/channel_test.go

How did you add / removed the tests here? did you move the channelv2_test.go here or did you make a one by one addition? I'm worried we're removing newly added tests

This is the result of moving channelv2_test.go

Co-authored-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com>

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

Cali0707 · 2024-05-30T17:31:59Z

/cc @pierDipi

Cali0707 · 2024-05-30T20:13:52Z

/test reconciler-tests-namespaced-broker

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

pierDipi

/lgtm
/approve

knative-prow · 2024-06-03T15:13:24Z

[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

Cali0707 · 2024-06-03T18:36:34Z

/unhold

I'll follow up on unit tests in a separate PR

feat: channels use consumergroups

5c77fd8

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

knative-prow bot requested review from aliok and pierDipi April 4, 2024 20:09

knative-prow bot added area/control-plane area/data-plane size/L Denotes a PR that changes 100-499 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Apr 4, 2024

fix: upgrade tests hopefully work now during downgrades

f39e4fb

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

knative-prow bot added the area/test label Apr 4, 2024

knative-prow bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 4, 2024

Cali0707 added 11 commits April 4, 2024 16:50

fix: e2e-common.sh should deploy statefulsets properly

8ca6796

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

fix: KafkaChannel lease name in test

2479f0e

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

track channel secret

fc13ff4

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

fix: channel sets reply on consumergroup

ec95a51

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

correctly handle delivery and reply specs in channelv2

2c6f437

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

fix: channel sets sa name for cg

04f1eeb

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

fix: set subscriber certs and audience

2136a38

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

fix: channelv2 has tls fixes from channelv1

519741e

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

feat: added trustbundles to channelv2

76f795c

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

fix: channelv2 sets volumeGeneration annotation on receiver pods

8419e75

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

fix: unit tests pass again

622e605

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

knative-prow bot requested a review from creydr April 12, 2024 18:27

knative-prow bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 12, 2024

fix: downgrade uses correct kind when deleting cgs

4f36859

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

Cali0707 added 2 commits May 16, 2024 10:52

Merge branch 'main' into use-channelv2

a6589ef

fix build issues and lint issues

b4f283e

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

pierDipi reviewed May 28, 2024

View reviewed changes

fix: channelv2 -> channel

6839402

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

knative-prow bot requested a review from pierDipi May 28, 2024 16:58

pierDipi reviewed May 29, 2024

View reviewed changes

control-plane/cmd/post-install/kafka_broker_deployment_deleter.go Outdated

Copy link

Member

pierDipi May 29, 2024

Choose a reason for hiding this comment

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

should the file be called kafka_channel_... ?

pierDipi reviewed May 29, 2024

View reviewed changes

test/upgrade/installation/channelv2_downgrade.go Outdated Show resolved Hide resolved

test/upgrade/installation/channelv2_downgrade.go Outdated Show resolved Hide resolved

pierDipi reviewed May 30, 2024

View reviewed changes

Cali0707 and others added 2 commits May 30, 2024 13:29

Apply suggestions from code review

ebed6d5

Co-authored-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com>

cleanup: renamed kafka_broker_deployment_deleter to kafka_channel_dep…

1c21e04

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

knative-prow bot requested a review from pierDipi May 30, 2024 17:32

Cali0707 added 2 commits May 31, 2024 14:37

fix merge conflicts

74f14a7

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

fix build issue

29e5234

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

pierDipi approved these changes Jun 3, 2024

View reviewed changes

knative-prow bot assigned pierDipi Jun 3, 2024

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

knative-prow bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 3, 2024

knative-prow bot merged commit 684d3e1 into knative-extensions:main Jun 3, 2024
34 of 35 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: channels use consumergroups #3816

feat: channels use consumergroups #3816

Cali0707 commented Apr 4, 2024 •

edited

Loading

Cali0707 commented Apr 4, 2024

codecov bot commented Apr 4, 2024 •

edited

Loading

Cali0707 commented Apr 4, 2024

Cali0707 commented Apr 12, 2024

Cali0707 commented Apr 12, 2024

creydr commented Apr 16, 2024

Cali0707 commented May 14, 2024

Cali0707 commented May 28, 2024

pierDipi May 28, 2024

Cali0707 May 28, 2024

Cali0707 commented May 28, 2024

pierDipi commented May 29, 2024

pierDipi commented May 29, 2024

pierDipi commented May 29, 2024

pierDipi commented May 29, 2024

pierDipi May 29, 2024

pierDipi commented May 29, 2024

pierDipi May 30, 2024

Cali0707 May 30, 2024

Cali0707 commented May 30, 2024

Cali0707 commented May 30, 2024

pierDipi left a comment

knative-prow bot commented Jun 3, 2024

Cali0707 commented Jun 3, 2024

feat: channels use consumergroups #3816

feat: channels use consumergroups #3816

Conversation

Cali0707 commented Apr 4, 2024 • edited Loading

Proposed Changes

Cali0707 commented Apr 4, 2024

codecov bot commented Apr 4, 2024 • edited Loading

Codecov Report

Cali0707 commented Apr 4, 2024

Cali0707 commented Apr 12, 2024

Cali0707 commented Apr 12, 2024

creydr commented Apr 16, 2024

Cali0707 commented May 14, 2024

Cali0707 commented May 28, 2024

pierDipi May 28, 2024

Choose a reason for hiding this comment

Cali0707 May 28, 2024

Choose a reason for hiding this comment

Cali0707 commented May 28, 2024

pierDipi commented May 29, 2024

pierDipi commented May 29, 2024

pierDipi commented May 29, 2024

pierDipi commented May 29, 2024

pierDipi May 29, 2024

Choose a reason for hiding this comment

pierDipi commented May 29, 2024

pierDipi May 30, 2024

Choose a reason for hiding this comment

Cali0707 May 30, 2024

Choose a reason for hiding this comment

Cali0707 commented May 30, 2024

Cali0707 commented May 30, 2024

pierDipi left a comment

Choose a reason for hiding this comment

knative-prow bot commented Jun 3, 2024

Cali0707 commented Jun 3, 2024

Cali0707 commented Apr 4, 2024 •

edited

Loading

codecov bot commented Apr 4, 2024 •

edited

Loading