Use broker status annotations to stamp ConfigMap reference #1971

pierDipi · 2022-03-07T16:14:45Z

Instead of adding a finalizer to the ConfigMap, we can record
the ConfigMap data in the Broker status so that we can re-build
the ConfigMap even when the ConfigMap is deleted first.

Signed-off-by: Pierangelo Di Pilato pierdipi@redhat.com

Fixes #1970

Release Note

Support brokers with long namespace and name values.

Instead of adding a finalizer to the ConfigMap, we can record the ConfigMap data in the Broker status so that we can re-build the ConfigMap even when the ConfigMap is deleted first. Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>

matzew · 2022-03-07T16:16:09Z

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

@@ -348,6 +348,7 @@ func (r *Reconciler) finalizeKind(ctx context.Context, broker *eventing.Broker)

 	logger.Debug("Topic deleted", zap.String("topic", topic))

+	// TODO(pierDipi) remove after some releases (released in 1.4)


codecov · 2022-03-07T16:19:38Z

Codecov Report

Merging #1971 (0d23444) into main (250b4b4) will increase coverage by 0.07%.
The diff coverage is 90.90%.

@@             Coverage Diff              @@
##               main    #1971      +/-   ##
============================================
+ Coverage     66.96%   67.04%   +0.07%     
- Complexity      631      633       +2     
============================================
  Files           140      140              
  Lines          6557     6560       +3     
  Branches        179      179              
============================================
+ Hits           4391     4398       +7     
+ Misses         1798     1794       -4     
  Partials        368      368

Flag	Coverage Δ
java-unittests	`82.51% <ø> (+0.09%)`	⬆️

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

Impacted Files	Coverage Δ
control-plane/pkg/reconciler/broker/broker.go	`70.00% <90.90%> (+1.33%)`	⬆️
...lane/pkg/reconciler/consumergroup/consumergroup.go	`76.85% <0.00%> (ø)`
...dispatcher/impl/consumer/BaseConsumerVerticle.java	`93.75% <0.00%> (+6.25%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 250b4b4...0d23444. Read the comment docs.

Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>

pierDipi · 2022-03-07T16:29:53Z

/kind bug

knative-metrics-robot · 2022-03-07T16:32:11Z

The following is the coverage report on the affected files.
Say /test pull-knative-sandbox-eventing-kafka-broker-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
control-plane/pkg/reconciler/broker/broker.go	82.5%	83.2%	0.7

matzew · 2022-03-07T16:39:24Z

oc get broker matze-demo-kafka-broker -ojson | jq .status
{
  "address": {
    "url": "http://kafka-broker-ingress.knative-eventing.svc.cluster.local/default/matze-demo-kafka-broker"
  },
  "annotations": {
    "bootstrap.servers": "my-cluster-kafka-bootstrap.kafka:9092",
    "default.topic.partitions": "10",
    "default.topic.replication.factor": "3"
  },
  "conditions": [
....

I do like the solution to capture this - and the in-memory rebuild

matzew

/lgtm
/approve

/hold
in case one wants to do another review/test

knative-prow-robot · 2022-03-07T16:40:39Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: matzew, 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~~ [matzew,pierDipi]

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

pierDipi · 2022-03-07T17:05:38Z

since tomorrow is release day, I'm gonna cancel the 'hold', as always feedback is more than welcome!

cc @aliok @devguyio

/unhold

pierDipi · 2022-03-07T17:58:14Z

/test pull-knative-sandbox-eventing-kafka-broker-reconciler-tests

pierDipi · 2022-03-07T19:06:29Z

/test pull-knative-sandbox-eventing-kafka-broker-unit-tests

…xtensions#1971) * Use broker status annotations to stamp ConfigMap reference Instead of adding a finalizer to the ConfigMap, we can record the ConfigMap data in the Broker status so that we can re-build the ConfigMap even when the ConfigMap is deleted first. Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com> * Add test Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com> * Fix lint error Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>

matzew · 2022-03-08T06:36:55Z

/cherry-pick release-v1.2

knative-prow-robot · 2022-03-08T06:37:33Z

@matzew: cannot checkout release-v1.2: error checking out release-v1.2: exit status 1. output: error: pathspec 'release-v1.2' did not match any file(s) known to git

In response to this:

/cherry-pick release-v1.2

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.

pierDipi · 2022-03-08T07:03:02Z

/cherry-pick release-1.2

knative-prow-robot · 2022-03-08T07:03:39Z

@pierDipi: new pull request created: #1975

In response to this:

/cherry-pick release-1.2

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.

…xtensions#1971) (#142) * Use broker status annotations to stamp ConfigMap reference Instead of adding a finalizer to the ConfigMap, we can record the ConfigMap data in the Broker status so that we can re-build the ConfigMap even when the ConfigMap is deleted first. Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com> * Add test Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com> * Fix lint error Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com> Co-authored-by: Pierangelo Di Pilato <pierdipi@redhat.com>

We removed the "add finalizer to the ConfigMap logic" in knative-extensions#1971 and we kept the "remove the ConfigMap finalizer" in `FinalizeKind`. However, in order to remove the ConfigMap finalizer from every existing Broker that won't be deleted in the time span going from 1.3 to whatever release we remove the logic to remove the finalizer. Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>

We removed the "add finalizer to the ConfigMap logic" in knative-extensions#1971 and we kept the "remove the ConfigMap finalizer" in `FinalizeKind`. However, in order to remove the ConfigMap finalizer from every existing Broker that won't be deleted in the time span going from 1.3 to whatever release we remove the logic to remove the finalizer. Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com> Co-authored-by: Pierangelo Di Pilato <pierdipi@redhat.com>

We removed the "add finalizer to the ConfigMap logic" in #1971 and we kept the "remove the ConfigMap finalizer" in `FinalizeKind`. However, in order to remove the ConfigMap finalizer from every existing Broker that won't be deleted in the time span going from 1.3 to whatever release we remove the logic to remove the finalizer. Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>

We removed the "add finalizer to the ConfigMap logic" in knative-extensions#1971 and we kept the "remove the ConfigMap finalizer" in `FinalizeKind`. However, in order to remove the ConfigMap finalizer from every existing Broker that won't be deleted in the time span going from 1.3 to whatever release we remove the logic to remove the finalizer. Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>

We removed the "add finalizer to the ConfigMap logic" in knative-extensions#1971 and we kept the "remove the ConfigMap finalizer" in `FinalizeKind`. However, in order to remove the ConfigMap finalizer from every existing Broker that won't be deleted in the time span going from 1.3 to whatever release we remove the logic to remove the finalizer. Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com> Co-authored-by: Pierangelo Di Pilato <pierdipi@redhat.com>

knative-prow-robot added area/control-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 Mar 7, 2022

matzew reviewed Mar 7, 2022

View reviewed changes

pierDipi added 2 commits March 7, 2022 17:24

Add test

423d197

Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>

Fix lint error

0d23444

Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>

knative-prow-robot added the kind/bug Categorizes issue or PR as related to a bug. label Mar 7, 2022

matzew approved these changes Mar 7, 2022

View reviewed changes

knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 7, 2022

knative-prow-robot assigned matzew Mar 7, 2022

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 7, 2022

knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 7, 2022

knative-prow-robot merged commit 9606158 into knative-extensions:main Mar 7, 2022

pierDipi deleted the KNATIVE-1970_Use-broker-annotations-to-rebuild-CM branch March 7, 2022 21:30

knative-prow-robot mentioned this pull request Mar 8, 2022

[release-1.2] Use broker status annotations to stamp ConfigMap reference #1975

Merged

pierDipi mentioned this pull request Mar 9, 2022

Remove ConfigMap finalizer during reconcile #1987

Merged

matzew mentioned this pull request Mar 9, 2022

[release-v1.1] Backport of upstream 1987 openshift-knative/eventing-kafka-broker#144

Merged

pierDipi restored the KNATIVE-1970_Use-broker-annotations-to-rebuild-CM branch August 5, 2024 11:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Use broker status annotations to stamp ConfigMap reference #1971

Use broker status annotations to stamp ConfigMap reference #1971

Uh oh!

pierDipi commented Mar 7, 2022

Uh oh!

matzew Mar 7, 2022

Uh oh!

codecov bot commented Mar 7, 2022 •

edited

Loading

Uh oh!

pierDipi commented Mar 7, 2022

Uh oh!

knative-metrics-robot commented Mar 7, 2022

Uh oh!

matzew commented Mar 7, 2022

Uh oh!

matzew left a comment

Uh oh!

knative-prow-robot commented Mar 7, 2022

Uh oh!

pierDipi commented Mar 7, 2022

Uh oh!

pierDipi commented Mar 7, 2022

Uh oh!

pierDipi commented Mar 7, 2022

Uh oh!

matzew commented Mar 8, 2022

Uh oh!

knative-prow-robot commented Mar 8, 2022

Uh oh!

pierDipi commented Mar 8, 2022

Uh oh!

knative-prow-robot commented Mar 8, 2022

Uh oh!

Uh oh!

		@@ -348,6 +348,7 @@ func (r Reconciler) finalizeKind(ctx context.Context, broker eventing.Broker)

		logger.Debug("Topic deleted", zap.String("topic", topic))

		// TODO(pierDipi) remove after some releases (released in 1.4)

Use broker status annotations to stamp ConfigMap reference #1971

Use broker status annotations to stamp ConfigMap reference #1971

Uh oh!

Conversation

pierDipi commented Mar 7, 2022

Uh oh!

matzew Mar 7, 2022

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Mar 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

pierDipi commented Mar 7, 2022

Uh oh!

knative-metrics-robot commented Mar 7, 2022

Uh oh!

matzew commented Mar 7, 2022

Uh oh!

matzew left a comment

Choose a reason for hiding this comment

Uh oh!

knative-prow-robot commented Mar 7, 2022

Uh oh!

pierDipi commented Mar 7, 2022

Uh oh!

pierDipi commented Mar 7, 2022

Uh oh!

pierDipi commented Mar 7, 2022

Uh oh!

matzew commented Mar 8, 2022

Uh oh!

knative-prow-robot commented Mar 8, 2022

Uh oh!

pierDipi commented Mar 8, 2022

Uh oh!

knative-prow-robot commented Mar 8, 2022

Uh oh!

Uh oh!

codecov bot commented Mar 7, 2022 •

edited

Loading