Remove channel based broker #3212

vaikas · 2020-05-27T17:58:21Z

Addresses #3139

Proposed Changes

Remove ChannelBasedBroker implementation.

Release Note

- 🗑️ Remove feature or internal logic
Remove ChannelBasedBroker

Docs
knative/docs#2516

knative-prow-robot · 2020-05-27T17:58:28Z

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

vaikas · 2020-05-27T18:07:14Z

/test all

vaikas · 2020-05-27T18:34:24Z

/test pull-knative-eventing-integration-tests

vaikas · 2020-05-27T19:42:03Z

/test all

vaikas · 2020-05-27T20:43:29Z

/test all

vaikas · 2020-05-27T21:07:13Z

/test all

matzew · 2020-05-28T05:32:22Z

/assign

matzew · 2020-05-28T05:34:53Z

let's remove this too:

https://github.com/vaikas/eventing/blob/remove-channel-based-broker/hack/release.sh#L24

vaikas · 2020-05-28T06:15:35Z

Follow up tomorrow ok? Sent from Outlook Mobile<https://aka.ms/blhgte>

…

________________________________ From: Matthias Wessendorf <notifications@github.com> Sent: Wednesday, May 27, 2020 10:35:05 PM To: knative/eventing <eventing@noreply.github.com> Cc: Ville Aikas <vaikas@vmware.com>; Mention <mention@noreply.github.com> Subject: Re: [knative/eventing] Remove channel based broker (#3212) let's remove this too: https://github.com/vaikas/eventing/blob/remove-channel-based-broker/hack/release.sh#L24<https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fvaikas%2Feventing%2Fblob%2Fremove-channel-based-broker%2Fhack%2Frelease.sh%23L24&data=02%7C01%7Cvaikas%40vmware.com%7C62e78aa60aa34e0104ba08d802c8e12e%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637262409082176543&sdata=CojmSdbVlx30is4u%2Fo%2FE17wtC68JfW4xk9ZwRSE9LCQ%3D&reserved=0> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fknative%2Feventing%2Fpull%2F3212%23issuecomment-635113979&data=02%7C01%7Cvaikas%40vmware.com%7C62e78aa60aa34e0104ba08d802c8e12e%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637262409082186535&sdata=VWe%2FefUpU0Lryplg60ODy5LWrjghtpkbEuFuBaW%2B1xc%3D&reserved=0>, or unsubscribe<https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FACWB45BXGFIQ5Q5AIVLQWJTRTXZYTANCNFSM4NMLXXAQ&data=02%7C01%7Cvaikas%40vmware.com%7C62e78aa60aa34e0104ba08d802c8e12e%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637262409082186535&sdata=YgqtFmxyrzA6Rpr6I%2FeiJhz71mtKhSXsJm8P9pRznUc%3D&reserved=0>.

matzew · 2020-05-28T08:01:10Z

config/brokers/channel-broker/deployments/controller.yaml

-  labels:
-    eventing.knative.dev/release: devel
-spec:
-  replicas: 1


should we keep this with replicas: 0 ?

We have done this before with other "deprecated" PRs.

Thoughts?

So, if we do this, we can't remove this?
https://github.com/vaikas/eventing/blob/remove-channel-based-broker/hack/release.sh#L24

Just have the deployment there, set to 0 replicas?

so remove this file and move to mt-channel-broker? Or? :) Confused :)

I think we want to delete the file outright and add a followup to do a cluster update for deleting a deployment in the knative-eventing namespace called broker-controller

matzew · 2020-05-28T08:06:55Z

I've tested this PR, using: https://github.com/matzew/knative-eventing-samples/tree/master/03-source_broker

worked fine!

A few nits?

the filter/ingress are called broker-xyz, e.g.:
https://github.com/vaikas/eventing/blob/remove-channel-based-broker/config/brokers/mt-channel-broker/deployments/broker-ingress.yaml#L18

while the controller is mt-broker-controller, see:
https://github.com/vaikas/eventing/blob/remove-channel-based-broker/config/brokers/mt-channel-broker/deployments/controller.yaml#L18

so, than the three deployments would all start with mt-xxx ?

knative-metrics-robot · 2020-05-28T19:18:19Z

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

File	Old Coverage	New Coverage	Delta
pkg/mtbroker/filter/filter_handler.go	78.4%	78.3%	-0.1
pkg/mtbroker/ingress/ingress_handler.go	83.3%	80.9%	-2.5
pkg/reconciler/mtbroker/trigger.go	87.7%	87.6%	-0.1

vaikas · 2020-05-28T19:20:04Z

Thanks, I think I addressed all your comments, PTAL :)

matzew · 2020-05-28T19:23:21Z

config/brokers/mt-channel-broker/deployments/broker-ingress.yaml

@@ -20,6 +22,7 @@ metadata:
  labels:
    eventing.knative.dev/release: devel
 spec:
+  replicas: 0


ahem... in the "MT broker" ?

I think we should keep the bits for the ST broker ? (at replica 0)

Or am I missing something here ?

Sorry, are you saying that we have this deployment in the config/brokers/mt-channel-broker/ where we set the ChannelBroker deployment to 0?

I thought that when we rename the deployment, we should bring the old MT deployments to replicas: 0. Then the one below creates a new deployment?

So, you end up with something like this:
note broker-filter, broker-ingress have been brought to 0 and mt-broker-filter and mt-broker-ingress are up and running.

broker-controller is similarly brought to 0.

vaikas-a01:eventing vaikas$ kubectl -n knative-eventing get deployments NAME READY UP-TO-DATE AVAILABLE AGE broker-controller 0/0 0 0 43m broker-filter 0/0 0 0 34d broker-ingress 0/0 0 0 34d eventing-controller 1/1 1 1 34d eventing-webhook 1/1 1 1 34d imc-controller 1/1 1 1 34d imc-dispatcher 1/1 1 1 34d kafka-ch-controller 1/1 1 1 16d kafka-webhook 1/1 1 1 16d mt-broker-controller 1/1 1 1 34d mt-broker-filter 1/1 1 1 77m mt-broker-ingress 1/1 1 1 76m

vaikas · 2020-05-28T19:28:41Z

/test pull-knative-eventing-unit-tests

vaikas · 2020-05-28T19:45:49Z

For the UT tests above, filed:
#3222

n3wscott · 2020-05-28T20:07:53Z

I think this would be fine to do as a followup, but we should fold in the mt-broker-controller into the eventing controller and keep all the class filtering it does to avoid the deployment. It costs almost nothing to be idle...

vaikas · 2020-05-28T20:10:09Z

I was under the impression that we mosdef wanted the broker to be a separately installable component which means it wouldn't be part of the core eventing controller. But maybe I misunderstand what you mean?

vaikas · 2020-05-28T21:01:58Z

I created an issue to track the clean up of the old brokers. Seems like a bigger issue, and perhaps we should have a job that cleans up the old Brokers, service accounts, etc.? In any case, bigger than this PR imho?
#3225

matzew

/lgtm
/approve

knative-prow-robot · 2020-05-29T08:00:10Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

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

vaikas added 6 commits May 27, 2020 09:27

remove configs

76dd268

remove the binaries

8ce7ee9

and the libraries

c61d6cd

fix import paths to correct location

f7701f1

remove ChannelBasedBroker constants, no tests for ChannelBasedBroker

568b6ad

update tests

2da8398

knative-prow-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 27, 2020

knative-prow-robot requested review from matzew and yt3liu May 27, 2020 17:58

knative-prow-robot added the area/test-and-release Test infrastructure, tests or release label May 27, 2020

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label May 27, 2020

knative-prow-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels May 27, 2020

mattmoor reviewed May 27, 2020

View reviewed changes

one more place

be12215

mattmoor reviewed May 27, 2020

View reviewed changes

dump failed channels too

1657ed4

mattmoor reviewed May 27, 2020

View reviewed changes

remove namespace reconciler, configmappropagation

cfa1fd3

mattmoor reviewed May 27, 2020

View reviewed changes

remove trailing space

7d37a44

few more namespace->mtnamespace

198f0ef

vaikas marked this pull request as ready for review May 27, 2020 21:07

knative-prow-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 27, 2020

filter out default filter channel

12b520d

knative-prow-robot assigned matzew May 28, 2020

matzew reviewed May 28, 2020

View reviewed changes

vaikas added 3 commits May 28, 2020 11:53

rename deployments, set older deployments to replicas=0

c349a98

bring old broker controller replicas=0

12ed90a

fix the env variables to v0.15 release

dc16625

matzew reviewed May 28, 2020

View reviewed changes

vaikas mentioned this pull request May 28, 2020

Add a post-install step that deletes the Deprecated deployments. #3225

Closed

matzew approved these changes May 29, 2020

View reviewed changes

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label May 29, 2020

knative-prow-robot merged commit 14b5201 into knative:master May 29, 2020

vaikas deleted the remove-channel-based-broker branch May 29, 2020 15:12

vaikas mentioned this pull request May 29, 2020

Using MTChannelBasedBroker and v1beta1 APIs #3228

Merged

matzew mentioned this pull request Jun 2, 2020

Remove the Single tenant Broker #3139

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove channel based broker #3212

Remove channel based broker #3212

vaikas commented May 27, 2020

knative-prow-robot commented May 27, 2020

vaikas commented May 27, 2020

vaikas commented May 27, 2020

vaikas commented May 27, 2020

vaikas commented May 27, 2020

vaikas commented May 27, 2020

matzew commented May 28, 2020

matzew commented May 28, 2020

vaikas commented May 28, 2020 via email

matzew May 28, 2020

vaikas May 28, 2020

vaikas May 28, 2020

n3wscott May 28, 2020

matzew commented May 28, 2020

knative-metrics-robot commented May 28, 2020

vaikas commented May 28, 2020

matzew May 28, 2020

vaikas May 28, 2020

vaikas May 28, 2020

vaikas May 28, 2020

vaikas commented May 28, 2020

vaikas commented May 28, 2020

n3wscott commented May 28, 2020

vaikas commented May 28, 2020

vaikas commented May 28, 2020

matzew left a comment

knative-prow-robot commented May 29, 2020

Remove channel based broker #3212

Remove channel based broker #3212

Conversation

vaikas commented May 27, 2020

Proposed Changes

knative-prow-robot commented May 27, 2020

vaikas commented May 27, 2020

vaikas commented May 27, 2020

vaikas commented May 27, 2020

vaikas commented May 27, 2020

vaikas commented May 27, 2020

matzew commented May 28, 2020

matzew commented May 28, 2020

vaikas commented May 28, 2020 via email

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

matzew commented May 28, 2020

knative-metrics-robot commented May 28, 2020

vaikas commented May 28, 2020

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

vaikas commented May 28, 2020

vaikas commented May 28, 2020

n3wscott commented May 28, 2020

vaikas commented May 28, 2020

vaikas commented May 28, 2020

matzew left a comment

Choose a reason for hiding this comment

knative-prow-robot commented May 29, 2020