[Consolidated KafkaChannel] Set KafkaChannel consolidated dispatcher OwenrRef #798

devguyio · 2021-08-09T06:00:10Z

Proposed Changes

Set the consolidated KafkaChannel controller deployment as the owner of the dispatcher deployment. This means that the way to uninstall the KafkaChannes is by deleting the controller deployment only.

Release Note

- 🧽 The consolidated KafkaChannel dispatcher is now owned by the controller.

Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com>

matzew · 2021-08-09T06:02:12Z

pkg/channel/consolidated/reconciler/controller/controller.go

 )

+var trueConst = true


do we need this?

yes, wherever we need a ref to a true, which in this PR in line 121

how about https://pkg.go.dev/k8s.io/utils/pointer#Bool ?

good one! didn't know about it.

Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com>

codecov · 2021-08-09T06:42:37Z

Codecov Report

Merging #798 (1954a8b) into main (09a3149) will decrease coverage by 0.78%.
The diff coverage is 28.84%.

@@            Coverage Diff             @@
##             main     #798      +/-   ##
==========================================
- Coverage   75.41%   74.63%   -0.79%     
==========================================
  Files         144      139       -5     
  Lines        6497     6356     -141     
==========================================
- Hits         4900     4744     -156     
- Misses       1376     1393      +17     
+ Partials      221      219       -2

Impacted Files	Coverage Δ
...l/consolidated/reconciler/controller/controller.go	`0.00% <0.00%> (ø)`
...consolidated/reconciler/controller/kafkachannel.go	`48.77% <71.42%> (-0.27%)`	⬇️
...el/distributed/dispatcher/dispatcher/dispatcher.go	`96.77% <0.00%> (-0.04%)`	⬇️
test/rekt/resources/kafkatopic/topic.go	`31.57% <0.00%> (ø)`
test/rekt/resources/kafkacat/kafkacat.go	`66.66% <0.00%> (ø)`
test/rekt/resources/kafkasource/kafkasource.go	`74.19% <0.00%> (ø)`
test/rekt/resources/resetoffset/resetoffset.go	`42.30% <0.00%> (ø)`
test/rekt/resources/kafkachannel/kafkachannel.go	`26.66% <0.00%> (ø)`
pkg/channel/distributed/common/kafka/util/util.go	`100.00% <0.00%> (ø)`
...g/apis/messaging/v1beta1/kafka_channel_defaults.go	`100.00% <0.00%> (ø)`
... and 6 more

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 09a3149...1954a8b. Read the comment docs.

Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com>

devguyio · 2021-08-09T17:02:27Z

/retest

matzew · 2021-08-10T11:10:13Z

@devguyio
You need to delete this sym-link https://github.com/devguyio/eventing-kafka/blob/srvke-910/config/channel/consolidated/500-dispatcher.yaml

matzew · 2021-08-10T11:21:48Z

Works... see ownerReferences

apiVersion: apps/v1
kind: Deployment
metadata:
  annotations:
    deployment.kubernetes.io/revision: "1"
  creationTimestamp: "2021-08-10T11:18:02Z"
  generation: 1
  name: kafka-ch-dispatcher
  namespace: knative-eventing
  ownerReferences:
  - apiVersion: apps/v1
    controller: true
    kind: Deployment
    name: kafka-ch-controller
    uid: 738f6cb0-dd46-4fd9-856f-341bb35c4737

and therefore the clean up works...

matzew · 2021-08-10T12:48:45Z

pkg/channel/consolidated/reconciler/controller/resources/dispatcher.go

-			Namespace: args.DispatcherNamespace,
+			Name:            dispatcherName,
+			Namespace:       args.DispatcherNamespace,
+			OwnerReferences: []metav1.OwnerReference{args.OwnerRef},


Should we add the KafkaChannel itself to the Args, and do smth like:

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

we do that w/ the source, like here: https://github.com/knative-sandbox/eventing-kafka/blob/83e1b7a7c4353f129e83e39f137f4f662765d242/pkg/source/reconciler/source/resources/receive_adapter.go#L86

that would mean you're setting the ownerRef of the dispatcher deployment to be owned by that KafkaChannel, which is not what we want to do here. This true in the source case because the receive adapter is per KafkaSource, but here the dispatcher is shared across all KafkaChannel object.

Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com>

devguyio · 2021-08-11T00:15:39Z

/retest

matzew · 2021-08-11T06:59:59Z

/test pull-knative-sandbox-eventing-kafka-upgrade-tests

matzew · 2021-08-11T07:01:58Z

Aug 11 00:00:28.591 install_head_consolidated_channel [OUT] The Deployment "kafka-ch-controller" is invalid: spec.selector: Invalid value: v1.LabelSelector{MatchLabels:map[string]string{"app.kubernetes.io/component":"controller", "app.kubernetes.io/name":"eventing-kafka-channel", "messaging.knative.dev/channel":"kafka-channel", "messaging.knative.dev/role":"controller"}, MatchExpressions:[]v1.LabelSelectorRequirement(nil)}: field is immutable

matzew · 2021-08-11T07:45:06Z

@devguyio would it make sense to move the refactoring to a separate pull request?

matzew · 2021-08-11T07:49:03Z

That would make the fix a bit more portable to older versions. So the refactoring goes in after?

devguyio · 2021-08-11T07:56:12Z

@matzew I needed the refactoring to reason about that block which I need to touch and change with or without the refactoring. So I'm not very sure if without that refactoring it's gonna be more portable to older versions because that block needs to change anyway, and if it exists differently in older versions then even without the refactoring I'll have to manually fix the conflict in those versions.

knative-metrics-robot · 2021-08-11T08:51:25Z

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

File	Old Coverage	New Coverage	Delta
pkg/channel/consolidated/reconciler/controller/kafkachannel.go	55.5%	54.7%	-0.8

matzew

/lgtm
/approve

knative-prow-robot · 2021-08-11T10:02:43Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: devguyio, matzew

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~~ [devguyio,matzew]

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

matzew · 2021-08-11T10:03:40Z

/cherry-pick release-0.23

matzew · 2021-08-11T10:03:47Z

/cherry-pick release-0.24

knative-prow-robot · 2021-08-11T10:04:14Z

@matzew: #798 failed to apply on top of branch "release-0.23":

Applying: Set KafkaChannel consolidated dispatcher OwenrRef
Using index info to reconstruct a base tree...
M	pkg/channel/consolidated/reconciler/controller/kafkachannel.go
Falling back to patching base and 3-way merge...
Removing pkg/channel/consolidated/reconciler/controller/resources/dispatcher_test.go
Auto-merging pkg/channel/consolidated/reconciler/controller/kafkachannel.go
CONFLICT (content): Merge conflict in pkg/channel/consolidated/reconciler/controller/kafkachannel.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Set KafkaChannel consolidated dispatcher OwenrRef
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-0.23

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.

knative-prow-robot · 2021-08-11T10:04:22Z

@matzew: new pull request created: #802

In response to this:

/cherry-pick release-0.24

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.

…OwenrRef (knative-extensions#798) * Set KafkaChannel consolidated dispatcher OwenrRef Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Remove the rest of superficial tests Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Use a k8s go-client to fetch controller deployment Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Refactor dispatcher reconciliation logic for easier reasoning Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Remove apps.kubernetes.io labels * fix bad comment * Use apimachinary wait package instead of manual backoff * fix typo

…OwenrRef (#798) (#803) * Set KafkaChannel consolidated dispatcher OwenrRef Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Remove the rest of superficial tests Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Use a k8s go-client to fetch controller deployment Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Refactor dispatcher reconciliation logic for easier reasoning Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Remove apps.kubernetes.io labels * fix bad comment * Use apimachinary wait package instead of manual backoff * fix typo

…OwenrRef (knative-extensions#798) (knative-extensions#803) * Set KafkaChannel consolidated dispatcher OwenrRef Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Remove the rest of superficial tests Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Use a k8s go-client to fetch controller deployment Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Refactor dispatcher reconciliation logic for easier reasoning Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Remove apps.kubernetes.io labels * fix bad comment * Use apimachinary wait package instead of manual backoff * fix typo

…er OwenrRef (knative-extensions#798) (#299) * [Consolidated KafkaChannel] Set KafkaChannel consolidated dispatcher OwenrRef (knative-extensions#798) (knative-extensions#803) * Set KafkaChannel consolidated dispatcher OwenrRef Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Remove the rest of superficial tests Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Use a k8s go-client to fetch controller deployment Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Refactor dispatcher reconciliation logic for easier reasoning Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com> * Remove apps.kubernetes.io labels * fix bad comment * Use apimachinary wait package instead of manual backoff * fix typo * running 'make RELEASE=v0.23.3 generate-release' to (re)generate the manifest used on CI run. In our case that ensures the dispatcher deployment is gone Signed-off-by: Matthias Wessendorf <mwessend@redhat.com> Co-authored-by: Matthias Wessendorf <mwessend@redhat.com>

matzew · 2021-08-23T09:24:22Z

/cherry-pick release-0.25

knative-prow-robot · 2021-08-23T09:25:06Z

@matzew: new pull request created: #819

In response to this:

/cherry-pick release-0.25

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.

Set KafkaChannel consolidated dispatcher OwenrRef

df7324d

Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com>

knative-prow-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 9, 2021

google-cla bot added the cla: yes Indicates the PR's author has signed the CLA. label Aug 9, 2021

matzew reviewed Aug 9, 2021

View reviewed changes

Remove the rest of superficial tests

e47d87a

Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com>

knative-prow-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 9, 2021

Use a k8s go-client to fetch controller deployment

5f5b1a9

Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com>

matzew reviewed Aug 10, 2021

View reviewed changes

Refactor dispatcher reconciliation logic for easier reasoning

d4cadc2

Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com>

devguyio added 2 commits August 11, 2021 09:13

Remove apps.kubernetes.io labels

b23407d

fix bad comment

72eb3ab

devguyio added 2 commits August 11, 2021 10:43

Use apimachinary wait package instead of manual backoff

fe57989

fix typo

1954a8b

devguyio changed the title ~~[Consolidated KafkaChannel] [WIP] Set KafkaChannel consolidated dispatcher OwenrRef~~ [Consolidated KafkaChannel] Set KafkaChannel consolidated dispatcher OwenrRef Aug 11, 2021

matzew approved these changes Aug 11, 2021

View reviewed changes

knative-prow-robot assigned matzew Aug 11, 2021

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 11, 2021

knative-prow-robot merged commit d0d4599 into knative-extensions:main Aug 11, 2021

knative-prow-robot mentioned this pull request Aug 11, 2021

[release-0.24] [Consolidated KafkaChannel] Set KafkaChannel consolidated dispatcher OwenrRef #802

Merged

devguyio mentioned this pull request Aug 11, 2021

[release-0.23] [Consolidated KafkaChannel] Set KafkaChannel consolidated dispatcher OwenrRef #803

Merged

This was referenced Aug 11, 2021

[release-v0.23] [KafkaChannel] Set KafkaChannel consolidated dispatcher OwenrRef (#798) openshift-knative/eventing-kafka#299

Merged

Remove kafka-ch-disptacher from knativekafka manifests. openshift-knative/serverless-operator#1144

Merged

knative-prow-robot mentioned this pull request Aug 23, 2021

[release-0.25] [Consolidated KafkaChannel] Set KafkaChannel consolidated dispatcher OwenrRef #819

Merged

[Consolidated KafkaChannel] Set KafkaChannel consolidated dispatcher OwenrRef #798

[Consolidated KafkaChannel] Set KafkaChannel consolidated dispatcher OwenrRef #798

Uh oh!

Conversation

devguyio commented Aug 9, 2021

Proposed Changes

Uh oh!

matzew Aug 9, 2021

Choose a reason for hiding this comment

Uh oh!

devguyio Aug 9, 2021

Choose a reason for hiding this comment

Uh oh!

aliok Aug 9, 2021

Choose a reason for hiding this comment

Uh oh!

devguyio Aug 9, 2021

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Aug 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

devguyio commented Aug 9, 2021

Uh oh!

matzew commented Aug 10, 2021

Uh oh!

matzew commented Aug 10, 2021

Uh oh!

matzew Aug 10, 2021

Choose a reason for hiding this comment

Uh oh!

devguyio Aug 11, 2021

Choose a reason for hiding this comment

Uh oh!

devguyio commented Aug 11, 2021

Uh oh!

matzew commented Aug 11, 2021

Uh oh!

matzew commented Aug 11, 2021

Uh oh!

matzew commented Aug 11, 2021

Uh oh!

matzew commented Aug 11, 2021

Uh oh!

devguyio commented Aug 11, 2021

Uh oh!

knative-metrics-robot commented Aug 11, 2021

Uh oh!

matzew left a comment

Choose a reason for hiding this comment

Uh oh!

knative-prow-robot commented Aug 11, 2021

Uh oh!

matzew commented Aug 11, 2021

Uh oh!

matzew commented Aug 11, 2021

Uh oh!

knative-prow-robot commented Aug 11, 2021

Uh oh!

knative-prow-robot commented Aug 11, 2021

Uh oh!

matzew commented Aug 23, 2021

Uh oh!

knative-prow-robot commented Aug 23, 2021

Uh oh!

Uh oh!

codecov bot commented Aug 9, 2021 •

edited

Loading