first cut at multi-tenant broker #2760

vaikas · 2020-03-17T01:21:47Z

Fixes #

Introduce a Multi Tenant Channel Based Broker that shares the running pieces of the code in knative-eventing namespace, instead of in the brokers namespace to make it easier, cheaper and simpler to operate brokers.
This uses a MTChannelBasedBroker class, and is not installed out of the box, so totes won't affect any existing code unless you want to play with it.

Proposed Changes

Introduce a multi-tenant channel based broker.
TL;DR here:
https://docs.google.com/document/d/1qYnmkRduWLUFQ3vEsaw7jU_mxS_nDHHkDkcGRf1_Fy4/edit#heading=h.ra3is5gwyzel

Release Note

- 🎁 Add new feature
Add multi tenant channel based broker.

Docs

mattmoor · 2020-03-17T01:26:39Z

cmd/mtbroker/ingress/main.go

+	configMapWatcher.Watch(loggingConfigMapName, logging.UpdateLevelFromConfigMap(sl, atomicLevel, component))
+
+	bin := tracing.BrokerIngressName(tracing.BrokerIngressNameArgs{
+		Namespace:  "knative-eventing",


Suggested change

Namespace: "knative-eventing",

Namespace: system.Namespace(),

config/300-broker-filter.yaml

config/300-broker-ingress.yaml

mattmoor · 2020-03-17T01:29:15Z

config/500-mt-broker-controller.yaml

@@ -0,0 +1,64 @@
+# Copyright 2020 The Knative Authors


These should all go under ./brokers/mt-channel-broker

mattmoor · 2020-03-17T01:31:08Z

cmd/mtbroker/filter/main.go

+	configMapWatcher.Watch(loggingConfigMapName, logging.UpdateLevelFromConfigMap(sl, atomicLevel, component))
+
+	bin := tracing.BrokerFilterName(tracing.BrokerFilterNameArgs{
+		Namespace:  env.Namespace,


This is the only use of Namespace, is this MT yet? Could it be system.Namespace() and drop that field?

mattmoor · 2020-03-17T01:33:06Z

pkg/reconciler/mtbroker/broker.go

+	// TODO(vaikas): Can we just add this into the controller as part of the filtering
+	// so they won't even get queued into my queue???
+	if r.brokerClass != "" {
+		if b.GetAnnotations()[eventing.BrokerClassKey] != r.brokerClass {


Didn't the current broker just drop this logic?

cmd/mtbroker/ingress/main.go

cmd/mtbroker/filter/main.go

mattmoor · 2020-03-17T04:15:09Z

pkg/reconciler/mtbroker/controller.go

+		triggerLister:      triggerInformer.Lister(),
+		brokerClass:        env.BrokerClass,
+	}
+	impl := brokerreconciler.NewImpl(ctx, r)


I believe this needs to pass ClassName

mattmoor · 2020-03-17T04:49:46Z

pkg/reconciler/mtbroker/broker.go

+		b.Status.MarkFilterFailed("DeploymentFailure", "%v", err)
+		return err
+	}
+	b.Status.PropagateFilterDeploymentAvailability(filterDeployment)


This assumes a particular anatomy, which isn't particularly flexible. Given that all you really care about is whether the network endpoint is "ready", I would change this (and the same below) to examine the Endpoints resources for non-empty addresses.

This approach should also be portable to the single-tenant Broker, and is actually (very slightly) more correct because even if the Deployment is "ready" it doesn't necessarily follow that the Endpoints have observed that and been updated to reflect it.

ack yes +1, just kept the existing behaviour for now as discussed offline :) I'll port your changes here.

Just to close the loop: #2766

pkg/mtbroker/filter/filter_handler.go

config/300-broker-filter.yaml

config/300-broker-ingress.yaml

Today the fact that the Broker lifecycle stuff propagates *Deployment* status for filter/ingress leaks implementation details into the API types. An example of where this leakage is bas would be if I were to run a multi-tenant broker (knative#2760) as something other than a Deployment. This change corrects one small part of this leaky interface by shifting from Deployment to Endpoints as the canonical resource for establishing readiness. This is also slightly more correct as it assesses not just whether the Deployment is ready, but whether that information has propagated to the Service / Endpoints.

mattmoor · 2020-03-18T03:29:05Z

/retest

This test is flaky on testgrid

Today the fact that the Broker lifecycle stuff propagates *Deployment* status for filter/ingress leaks implementation details into the API types. An example of where this leakage is bas would be if I were to run a multi-tenant broker (#2760) as something other than a Deployment. This change corrects one small part of this leaky interface by shifting from Deployment to Endpoints as the canonical resource for establishing readiness. This is also slightly more correct as it assesses not just whether the Deployment is ready, but whether that information has propagated to the Service / Endpoints.

mattmoor · 2020-03-18T17:37:15Z

config/brokers/mt-channel-broker/200-filter-clusterrole.yaml

@@ -0,0 +1 @@
+roles/filter-clusterrole.yaml


Add trailing newline:

Suggested change

roles/filter-clusterrole.yaml

roles/filter-clusterrole.yaml

mattmoor · 2020-03-18T17:37:16Z

config/brokers/mt-channel-broker/200-filter-serviceaccount.yaml

@@ -0,0 +1 @@
+roles/filter-serviceaccount.yaml


Add trailing newline:

Suggested change

roles/filter-serviceaccount.yaml

roles/filter-serviceaccount.yaml

mattmoor · 2020-03-18T17:37:16Z

config/brokers/mt-channel-broker/200-ingress-clusterrole.yaml

@@ -0,0 +1 @@
+roles/ingress-clusterrole.yaml


Add trailing newline:

Suggested change

roles/ingress-clusterrole.yaml

roles/ingress-clusterrole.yaml

mattmoor · 2020-03-18T17:37:16Z

config/brokers/mt-channel-broker/200-ingress-serviceaccount.yaml

@@ -0,0 +1 @@
+roles/ingress-serviceaccount.yaml


Add trailing newline:

Suggested change

roles/ingress-serviceaccount.yaml

roles/ingress-serviceaccount.yaml

mattmoor · 2020-03-18T17:37:17Z

config/brokers/mt-channel-broker/201-filter-clusterrolebinding.yaml

@@ -0,0 +1 @@
+roles/filter-clusterrolebinding.yaml


Add trailing newline:

Suggested change

roles/filter-clusterrolebinding.yaml

roles/filter-clusterrolebinding.yaml

mattmoor · 2020-03-18T17:37:17Z

config/brokers/mt-channel-broker/201-ingress-clusterrolebinding.yaml

@@ -0,0 +1 @@
+roles/ingress-clusterrolebinding.yaml


Add trailing newline:

Suggested change

roles/ingress-clusterrolebinding.yaml

roles/ingress-clusterrolebinding.yaml

mattmoor · 2020-03-18T17:37:17Z

config/brokers/mt-channel-broker/500-broker-filter.yaml

@@ -0,0 +1 @@
+deployments/broker-filter.yaml


Add trailing newline:

Suggested change

deployments/broker-filter.yaml

deployments/broker-filter.yaml

mattmoor · 2020-03-18T17:37:18Z

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

@@ -0,0 +1 @@
+deployments/broker-ingress.yaml


Add trailing newline:

Suggested change

deployments/broker-ingress.yaml

deployments/broker-ingress.yaml

mattmoor · 2020-03-18T17:37:18Z

config/brokers/mt-channel-broker/500-mt-broker-controller.yaml

@@ -0,0 +1 @@
+deployments/controller.yaml


Add trailing newline:

Suggested change

deployments/controller.yaml

deployments/controller.yaml

knative-metrics-robot · 2020-03-18T17:41:31Z

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
pkg/mtbroker/filter/filter_handler.go	Do not exist	77.8%
pkg/mtbroker/filter/stats_reporter.go	Do not exist	80.6%
pkg/mtbroker/ingress/ingress_handler.go	Do not exist	81.0%
pkg/mtbroker/ingress/stats_reporter.go	Do not exist	82.4%
pkg/mtbroker/ttl.go	Do not exist	85.0%
pkg/reconciler/mtbroker/broker.go	Do not exist	73.0%
pkg/reconciler/mtbroker/config.go	Do not exist	73.3%
pkg/reconciler/mtbroker/controller.go	Do not exist	89.5%
pkg/reconciler/mtbroker/resources/channel.go	Do not exist	90.0%
pkg/reconciler/mtbroker/resources/subscription.go	Do not exist	100.0%
pkg/reconciler/mtbroker/trigger.go	Do not exist	88.2%

vaikas · 2020-03-18T17:45:05Z

/test pull-knative-eventing-unit-tests

n3wscott · 2020-03-18T18:04:23Z

/lgtm
/approve

Awesome work.

knative-prow-robot · 2020-03-18T18:04:37Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

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 Mar 17, 2020

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

knative-prow-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Mar 17, 2020

knative-prow-robot requested review from matzew and n3wscott March 17, 2020 01:21