Update Broker and Controller to use topic name templates #3162

Cali0707 · 2023-06-20T21:19:33Z

Proposed Changes

Update the broker and controller to use the topic templates added in Allow defining topic prefixes #3157
Use the Status.Annotations field on the Channel and Broker to make sure that if the template gets changed, the Channel/Broker topic name is not changed on existing Brokers/Channels.

Release Note

Brokers and Channels now support custom Kafka Topic name templates.

Docs

knative/docs#5607

knative-prow · 2023-06-20T21:19:34Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

codecov · 2023-06-20T21:26:18Z

Codecov Report

Merging #3162 (7a5c4a8) into main (20a3799) will increase coverage by 0.01%.
The diff coverage is 75.28%.

@@             Coverage Diff              @@
##               main    #3162      +/-   ##
============================================
+ Coverage     63.51%   63.53%   +0.01%     
  Complexity      757      757              
============================================
  Files           167      167              
  Lines         11660    11726      +66     
  Branches        239      239              
============================================
+ Hits           7406     7450      +44     
- Misses         3701     3716      +15     
- Partials        553      560       +7

Flag	Coverage Δ
java-unittests	`80.14% <ø> (ø)`

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

Impacted Files	Coverage Δ
...l-plane/pkg/reconciler/broker/namespaced_broker.go	`69.75% <ø> (ø)`
...ane/pkg/reconciler/broker/namespaced_controller.go	`0.00% <0.00%> (ø)`
control-plane/pkg/reconciler/broker/broker.go	`71.81% <53.84%> (-0.63%)`	⬇️
control-plane/pkg/reconciler/channel/channel.go	`69.05% <57.14%> (-0.68%)`	⬇️
...ntrol-plane/pkg/reconciler/channel/v2/channelv2.go	`70.91% <62.50%> (-0.34%)`	⬇️
control-plane/pkg/apis/config/features.go	`86.45% <100.00%> (+0.14%)`	⬆️
control-plane/pkg/reconciler/broker/controller.go	`84.53% <100.00%> (+1.39%)`	⬆️
control-plane/pkg/reconciler/channel/controller.go	`90.42% <100.00%> (+0.77%)`	⬆️
...ol-plane/pkg/reconciler/channel/v2/controllerv2.go	`92.10% <100.00%> (+0.92%)`	⬆️

pierDipi

How do we make sure that if I change the template while I have existing brokers/channels, that change is not applied to existing objects?

Cali0707 · 2023-06-21T13:42:13Z

How do we make sure that if I change the template while I have existing brokers/channels, that change is not applied to existing objects?

Good catch! I hadn't considered that. Do you think that checking if there is a broker.Status.Annotations[kafka.TopicAnnotation] and using that if it exists would be sufficient?

…isting topic name for broker

Cali0707 · 2023-06-21T20:07:08Z

@pierDipi there seem to be 2 main things left to work on before this PR is ready, and I am not sure what to do for either.

I managed to use the broker.Status.Annotations[kafka.TopicAnnotation] to check for if a broker had an existing topic, as we currently set that field in the broker reconcileKind function. However, we don't do this for the channel (v1 or v2) so I am not sure how to check if there is an existing topic for a channel.
We should add unit tests to check if the templates are being applied properly for a custom template, but I am not sure how to write these tests as the features of the KafkaFeatureFlags are private, so I can't directly change them. Should we make the features public or is there some way to patch the resource so that the correct features will be seen as the broker/channel is reconciled?

Any ideas/input would be appreciated :)

pierDipi · 2023-06-22T13:12:52Z

@pierDipi there seem to be 2 main things left to work on before this PR is ready, and I am not sure what to do for either.

I managed to use the broker.Status.Annotations[kafka.TopicAnnotation] to check for if a broker had an existing topic, as we currently set that field in the broker reconcileKind function. However, we don't do this for the channel (v1 or v2) so I am not sure how to check if there is an existing topic for a channel.

Can't we set it on the channel and if it's not there we assume it will use the old pattern?

We should add unit tests to check if the templates are being applied properly for a custom template, but I am not sure how to write these tests as the features of the KafkaFeatureFlags are private, so I can't directly change them. Should we make the features public or is there some way to patch the resource so that the correct features will be seen as the broker/channel is reconciled?

Can't we make public newFeaturesConfigFromMap and a create KafkaFeatureFlags for the test?

Any ideas/input would be appreciated :)

Cali0707 · 2023-06-22T21:53:17Z

/retest-required

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

test/rekt/features/kafka_sink.go

test/rekt/features/broker_config.go

control-plane/pkg/apis/config/features.go

control-plane/pkg/reconciler/broker/broker_test.go

…properly

Cali0707 · 2023-06-23T21:08:01Z

/retest-required

Cali0707 · 2023-06-26T13:50:51Z

/retest-required

Cali0707 · 2023-06-26T19:29:52Z

/retest-required

Cali0707 · 2023-06-26T20:48:23Z

@pierDipi I've addressed your earlier comments and fixed the broken tests. Let me know if there is anything else I should change on this PR!

control-plane/pkg/apis/config/features.go

Cali0707 · 2023-06-28T14:12:52Z

/retest-required

Cali0707 · 2023-06-28T14:17:29Z

/retest-required

Cali0707 · 2023-06-28T15:18:22Z

/retest-required

Cali0707 · 2023-06-28T15:56:26Z

/retest-required

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

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

Cali0707 · 2023-06-28T19:39:27Z

/retest-required

pierDipi

/lgtm
/approve

knative-prow · 2023-06-29T09:18:33Z

[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~~ [pierDipi]

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

Cali0707 · 2023-06-29T12:26:14Z

/retest-required

Cali0707 · 2023-06-29T13:39:07Z

/retest-required

Cali0707 · 2023-06-29T14:06:03Z

/retest-required

…ensions#3162) * Updated broker to use broker topic name template * Updated broker reconciliation to onlyuse topic name template if no existing topic name for broker * Fixed import styling * Updated channel to use topic template * Updated channels to use topic annotations * Fixed import formatting * Added tests for reconciling with custom topic templates * Fixed go imports * use annotated topic names when deleting topics * switched from failing tests to using panics if topic names not setup properly * updated kafka feature yaml files to match channel topic template * updated kafka feature default vars to be private * updated tests * Fixed goimports * Fixed broker finalizer tests * Updated unit tests * Fixed unit tests * Switched templates to not be pointers * Updated broker and channel controllers to watch kafka feature config maps Signed-off-by: Calum Murray <cmurray@redhat.com> --------- Signed-off-by: Calum Murray <cmurray@redhat.com>

* Allow defining topic prefixes (knative-extensions#3157) * Added kafka feature for broker and channel topic templates * Added tests for broker and channel features * Updated codegen * Update Broker and Controller to use topic name templates (knative-extensions#3162) * Updated broker to use broker topic name template * Updated broker reconciliation to onlyuse topic name template if no existing topic name for broker * Fixed import styling * Updated channel to use topic template * Updated channels to use topic annotations * Fixed import formatting * Added tests for reconciling with custom topic templates * Fixed go imports * use annotated topic names when deleting topics * switched from failing tests to using panics if topic names not setup properly * updated kafka feature yaml files to match channel topic template * updated kafka feature default vars to be private * updated tests * Fixed goimports * Fixed broker finalizer tests * Updated unit tests * Fixed unit tests * Switched templates to not be pointers * Updated broker and channel controllers to watch kafka feature config maps Signed-off-by: Calum Murray <cmurray@redhat.com> --------- Signed-off-by: Calum Murray <cmurray@redhat.com> --------- Signed-off-by: Calum Murray <cmurray@redhat.com>

Updated broker to use broker topic name template

5088f85

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 area/test size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 20, 2023

knative-prow bot requested review from lionelvillard and matzew June 20, 2023 21:19

pierDipi reviewed Jun 21, 2023

View reviewed changes

Cali0707 added 2 commits June 21, 2023 11:14

Updated broker reconciliation to onlyuse topic name template if no ex…

d1efe2d

…isting topic name for broker

Fixed import styling

1a72d4c

Cali0707 force-pushed the broker-and-channel-use-topic-template branch from eb368c3 to 1a72d4c Compare June 21, 2023 17:58

Updated channel to use topic template

588b85b

Cali0707 added 4 commits June 22, 2023 14:34

Updated channels to use topic annotations

eaabca2

Fixed import formatting

5efbe15

Added tests for reconciling with custom topic templates

2419d38

Fixed go imports

c2c8591

Cali0707 mentioned this pull request Jun 22, 2023

Document Custom Kafka Topics knative/docs#5607

Closed

Cali0707 changed the title ~~[WIP]: Update Broker and Controller to use topic name templates~~ Update Broker and Controller to use topic name templates Jun 22, 2023

Cali0707 marked this pull request as ready for review June 22, 2023 20:18

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 Jun 22, 2023

knative-prow bot requested a review from aliok June 22, 2023 20:19

pierDipi reviewed Jun 23, 2023

View reviewed changes

Cali0707 added 2 commits June 23, 2023 13:27

use annotated topic names when deleting topics

6bb551c

switched from failing tests to using panics if topic names not setup …

88c5cad

…properly

Fixed broker finalizer tests

fad1041

Updated unit tests

e41e196

Cali0707 added 2 commits June 26, 2023 14:41

merged main into branch to fix conflicts

74d902a

Fixed unit tests

a9d4ee9

pierDipi reviewed Jun 28, 2023

View reviewed changes

control-plane/pkg/apis/config/features.go Outdated Show resolved Hide resolved

Switched templates to not be pointers

d7206cd

pierDipi reviewed Jun 28, 2023

View reviewed changes

control-plane/pkg/reconciler/broker/controller.go Show resolved Hide resolved

Updated broker and channel controllers to watch kafka feature config …

7a5c4a8

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

pierDipi approved these changes Jun 29, 2023

View reviewed changes

knative-prow bot assigned pierDipi Jun 29, 2023

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

knative-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 29, 2023

knative-prow bot merged commit e9792d0 into knative-extensions:main Jun 29, 2023

Cali0707 mentioned this pull request Jul 4, 2023

Cherry pick custom kafka topics changes openshift-knative/eventing-kafka-broker#729

Merged

matzew mentioned this pull request Aug 22, 2023

Fix channel finalizer logic #3295

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update Broker and Controller to use topic name templates #3162

Update Broker and Controller to use topic name templates #3162

Cali0707 commented Jun 20, 2023 •

edited

Loading

knative-prow bot commented Jun 20, 2023

codecov bot commented Jun 20, 2023 •

edited

Loading

pierDipi left a comment

Cali0707 commented Jun 21, 2023

Cali0707 commented Jun 21, 2023

pierDipi commented Jun 22, 2023

Cali0707 commented Jun 22, 2023

Cali0707 commented Jun 23, 2023

Cali0707 commented Jun 26, 2023

Cali0707 commented Jun 26, 2023

Cali0707 commented Jun 26, 2023

Cali0707 commented Jun 28, 2023

Cali0707 commented Jun 28, 2023

Cali0707 commented Jun 28, 2023

Cali0707 commented Jun 28, 2023

Cali0707 commented Jun 28, 2023

pierDipi left a comment

knative-prow bot commented Jun 29, 2023

Cali0707 commented Jun 29, 2023

Cali0707 commented Jun 29, 2023

Cali0707 commented Jun 29, 2023

Update Broker and Controller to use topic name templates #3162

Update Broker and Controller to use topic name templates #3162

Conversation

Cali0707 commented Jun 20, 2023 • edited Loading

Proposed Changes

knative-prow bot commented Jun 20, 2023

codecov bot commented Jun 20, 2023 • edited Loading

Codecov Report

pierDipi left a comment

Choose a reason for hiding this comment

Cali0707 commented Jun 21, 2023

Cali0707 commented Jun 21, 2023

pierDipi commented Jun 22, 2023

Cali0707 commented Jun 22, 2023

Cali0707 commented Jun 23, 2023

Cali0707 commented Jun 26, 2023

Cali0707 commented Jun 26, 2023

Cali0707 commented Jun 26, 2023

Cali0707 commented Jun 28, 2023

Cali0707 commented Jun 28, 2023

Cali0707 commented Jun 28, 2023

Cali0707 commented Jun 28, 2023

Cali0707 commented Jun 28, 2023

pierDipi left a comment

Choose a reason for hiding this comment

knative-prow bot commented Jun 29, 2023

Cali0707 commented Jun 29, 2023

Cali0707 commented Jun 29, 2023

Cali0707 commented Jun 29, 2023

Cali0707 commented Jun 20, 2023 •

edited

Loading

codecov bot commented Jun 20, 2023 •

edited

Loading