Add trigger controller #1094

liu-cong · 2020-05-19T16:48:35Z

I tried to reuse existing code as much as I can to reduce the amount of changes. I will have follow up PRs to do some cleanup and refactoring.

Proposed Changes

Add a separate trigger controller that reconciles the retry topic/pull subscription for Triggers
The controller reconciles/finalizes triggers if their brokers have the googlecloud brokerclass, or they have the googlecloud finalizer.
Changes to targets-config reconciliation logic
- The controller deletes broker entry if a broker is deleted, previously the entry still exists, which creates garbage
- Previously the broker entry and each trigger entry is individually updated, now every time a broker is reconciled, the entire broker entry is reconstructed. This has the following benefits:
  - The trigger controller doesn't need to manipulate trigger entries in the configmap
  - It's more reliable to always reconstruct compared to incremental edits

Release Note

Docs

knative-prow-robot · 2020-05-19T16:48:37Z

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

…rigger-controller

ian-mi · 2020-05-20T18:18:09Z

pkg/reconciler/broker/broker.go

+	// TODO Maybe get rid of BrokerMutation and add Delete() and Upsert(broker) methods to TargetsConfig. Now we always
+	//  delete or update the entire broker entry and we don't need partial updates per trigger.
+	// The code can be simplified to r.targetsConfig.Upsert(brokerConfigEntry)
+	// First delete the broker entry.


I had written a targets config reconciler which supports the interface you want here and allows directly constructing the broker config proto rather than working with the mutation interface: #914. Directly constructing the proto is a bit nicer since we are not constrained to using the mutation interface and individual mutations do not need to be threadsafe. I had closed that since the brokercell reconciler was meant to replace this but maybe it is worth reviving after this PR is merged since the brokercell reconciler is not yet ready.

Once we do #1054, we can start moving the target-config reconciliation to brokercell reconciler (assuming a static brokercell).

I disagree that updating proto is better. Reconciler shouldn't worry about the internals of targets config. Again, I'm likely biased but just giving my voice.

I am not sure I follow what do you mean by "internals" of targets config. Currently the UpsertTargets interface already takes in a config.Target object. What I am suggesting here is to add an interface to take in config.Broker because now we don't need to upsert each trigger one by one, it's always all or nothing. If we think the config.Broker proto is "internal", we can always add another abstraction but I don't think it's needed.

config.Broker is bit different from config.Target IMO. The "internal" I'm referring to is the map in the config.Broker. To construct a config.Broker, you will have to "manually" create the map rather than having the lib handle that. On the other side, config.Target is pure data.

ian-mi · 2020-05-20T18:22:03Z

pkg/reconciler/trigger/controller.go

+var filterBroker = pkgreconciler.AnnotationFilterFunc(eventingv1beta1.BrokerClassAnnotationKey, brokerv1beta1.BrokerClass, false /*allowUnset*/)
+
+func NewController(ctx context.Context, cmw configmap.Watcher) *controller.Impl {
+	// TODO initialize project ID here via env var or metadata.


Do we want to support reconciling across multiple projects?

I would say no for now unless there are some compelling use cases. Do you have any use case in mind ?

I haven't heard of a need for reconciling across multiple projects.

Notes for a future PR: Using an env var here makes tests more complicated. One option for setting project ID in this and other controllers is to add it to the context created in cmd/controller/main.go. Then controllers have a uniform way of getting the global project ID if they need it, and there's only one piece of code responsible for retrieving the project id from metadata or env var.

…rigger-controller

pkg/reconciler/broker/broker.go

grantr · 2020-05-27T01:25:57Z

pkg/reconciler/trigger/controller.go

+var filterBroker = pkgreconciler.AnnotationFilterFunc(eventingv1beta1.BrokerClassAnnotationKey, brokerv1beta1.BrokerClass, false /*allowUnset*/)
+
+func NewController(ctx context.Context, cmw configmap.Watcher) *controller.Impl {
+	// TODO initialize project ID here via env var or metadata.


I haven't heard of a need for reconciling across multiple projects.

Notes for a future PR: Using an env var here makes tests more complicated. One option for setting project ID in this and other controllers is to add it to the context created in cmd/controller/main.go. Then controllers have a uniform way of getting the global project ID if they need it, and there's only one piece of code responsible for retrieving the project id from metadata or env var.

pkg/reconciler/trigger/trigger.go

grantr · 2020-05-27T01:54:30Z

pkg/reconciler/trigger/trigger.go

+		// Call Finalizer anyway in case the Trigger still holds GCP Broker related resources.
+		// If a Trigger used to point to a GCP Broker but now has a Broker with a different brokerclass,
+		// we should clean up resources related to GCP Broker.
+		return r.FinalizeKind(ctx, t)


If a trigger is created for a broker that doesn't have this broker class, will the googlecloud finalizer be added anyway? I think it's automatically added in genreconciler when FinalizeKind is present, before Reconcile is even called. So most likely every Trigger will have the googlecloud finalizer even if they use another broker class.

I think that means that every trigger with a different broker class will be finalized on every update. The unit test of a trigger with a not found broker seems to confirm this (because it finalizes that trigger). Might want to confirm with a real-world test.

If that behavior is confirmed then we should disable the automatic finalizer logic in genreconciler (by renaming FinalizeKind). Then we can add the finalizer only to Triggers that point to a googlecloud broker.

Your understanding is correct. I forgot to add a TODO here to explain the finalizer scenario and future improvements.

I think that means that every trigger with a different broker class will be finalized on every update.

Correct. This is unnecessary and it will also cause read operation on the topic and pull subscription.

we should disable the automatic finalizer logic in genreconciler (by renaming FinalizeKind).

Unfortunately if we do this, the FinalizeKind logic won't be called when the trigger object is deleted. The genreconciler will do nothing because it thinks there is no finalizer to call if the reconciler doesn't implement FinalizeKind.

I have thought about this and came up with a few options:

The current behavior. Pros: least effort; Cons: unnecessary FinalizeKind calls for non-GCP broker.

Use a custom controller instead of genreconciler. This essentially means we need to copy most of the genreconciler boil plate code and make some modifications.

Upstream genreconciler changes to make it more flexible, specifically, add a FilterKind interface to let the genreconciler skip a trigger if it doesn't point to a GCP broker. I have proposed this interface before genreconciler should support Filtering knative/pkg#1149

I personally prefer Option 3. If this sounds good to you, I will add a TODO for that.

Dumb question: why would we reconcile such a trigger (trigger not pointing a gcp broker) in the first place? Broker in trigger spec is immutable.

Also for option 2, there might not be that much boilerplate code from just the FinalizeKind. We can still use the majority of the genreconciler.

why would we reconcile such a trigger (trigger not pointing a gcp broker) in the first place? Broker in trigger spec is immutable.

Consider this scenario. A trigger was pointed to a gcp broker. Then the broker was updated to use a different brokerclass. Now we want to tear down trigger pubsub resources. We don't know if users will actually do this. But in theory they can.

We could do option 2 but I really don't like the trigger controller to be "special".
I don't think it's very high priority because: 1. users are likely to only use gcp broker. 2. even if they use a different broker, this results in a couple of unnecessary calls to pubsub. not great but doesn't really hurt too much.
For the above reasons I prefer to have a clean solution.

A broker could be deleted and replaced with a broker with a different class. From the trigger controller's perspective, this will be the same as a broker changing class.

Right it's immutable. But you can still delete a broker and recreate it with a different brokerclass.

It's fine to defer this for later. I'd love to end up at option 3, but realistically I'd probably attempt option 2 first, then move the new code upstream later. It will be easier to justify as an upstream option once we have a demonstrated solution.

SG. Created an issue to follow up #1164

Nothing blocking. Just an idea: if we can (or already?) label the trigger with the initial broker class, then even if the broker got recreated with a different class we can still handle that deterministically, right?

pkg/reconciler/broker/broker.go

…rigger-controller

This reverts commit 0c17859.

yolocs

Having a couple of questions

yolocs · 2020-05-28T16:37:01Z

pkg/reconciler/trigger/controller.go

+
+	r.Logger.Info("Setting up event handlers")
+
+	triggerInformer.Informer().AddEventHandlerWithResyncPeriod(controller.HandleAll(impl.Enqueue), reconciler.DefaultResyncPeriod)


Do we need to filter on trigger label eventing.BrokerLabelKey?

No because we need to be able to reconcile orphaned triggers without a broker.

When would that happen? It seems trigger spec broker and broker class are both immutable.

Sorry let me clarify. Do mean filter on the existence of the label or actually check the broker is GCP broker?

Let me assume you meant the latter . If you delete a broker, then its triggers become orphaned. We'd like to be able to still delete the triggers (in this case reconcile is a noop).

pkg/reconciler/trigger/controller_test.go

yolocs · 2020-05-28T16:49:29Z

pkg/reconciler/trigger/trigger.go

+		// Call Finalizer anyway in case the Trigger still holds GCP Broker related resources.
+		// If a Trigger used to point to a GCP Broker but now has a Broker with a different brokerclass,
+		// we should clean up resources related to GCP Broker.
+		return r.FinalizeKind(ctx, t)


Dumb question: why would we reconcile such a trigger (trigger not pointing a gcp broker) in the first place? Broker in trigger spec is immutable.

Also for option 2, there might not be that much boilerplate code from just the FinalizeKind. We can still use the majority of the genreconciler.

pkg/reconciler/broker/broker.go

grantr

/lgtm

liu-cong · 2020-05-29T00:13:21Z

/hold for @yolocs to take a look

liu-cong · 2020-05-29T00:14:04Z

Thanks @yolocs and @grantr for the review. There are many tricky bits and thanks for your comments.

…ller should handle this

knative-metrics-robot · 2020-05-29T17:17:15Z

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

File	Old Coverage	New Coverage	Delta
pkg/reconciler/broker/broker.go	66.3%	63.5%	-2.8
pkg/reconciler/broker/controller.go	81.6%	78.1%	-3.5
pkg/reconciler/trigger/controller.go	Do not exist	69.7%
pkg/reconciler/trigger/trigger.go	Do not exist	59.2%

liu-cong · 2020-05-29T18:20:26Z

/retest

grantr

/lgtm

knative-prow-robot · 2020-05-29T18:35:23Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: grantr, liu-cong

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~~ [grantr,liu-cong]

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

yolocs · 2020-05-29T18:36:51Z

/lgtm

Dismissing request changes after lgtm

liu-cong · 2020-05-29T18:42:17Z

/unhold

liu-cong · 2020-05-29T20:39:28Z

/retest

knative-test-reporter-robot · 2020-05-29T21:08:51Z

The following jobs failed:

Test name	Triggers	Retries
pull-google-knative-gcp-integration-tests	pull-google-knative-gcp-integration-tests pull-google-knative-gcp-integration-tests pull-google-knative-gcp-integration-tests pull-google-knative-gcp-integration-tests pull-google-knative-gcp-integration-tests	3/3

Job pull-google-knative-gcp-integration-tests expended all 3 retries without success.

grantr · 2020-05-29T23:39:44Z

/retest

Add trigger controller

0d493ac

liu-cong requested a review from grantr May 19, 2020 16:48

liu-cong assigned grantr May 19, 2020

knative-prow-robot added the do-not-merge/work-in-progress label May 19, 2020

googlebot added the cla: yes (override cla status due to multiple authors bug) label May 19, 2020

knative-prow-robot requested review from ericlem and ian-mi May 19, 2020 16:48

knative-prow-robot added approved needs-rebase size/XL labels May 19, 2020

liu-cong and others added 2 commits May 19, 2020 20:55

Add ut

aa4b22a

Merge branch 'master' of https://github.com/google/knative-gcp into t…

6d85208

…rigger-controller

knative-prow-robot added size/XXL and removed needs-rebase size/XL labels May 20, 2020

liu-cong marked this pull request as ready for review May 20, 2020 17:24

knative-prow-robot removed the do-not-merge/work-in-progress label May 20, 2020

ian-mi reviewed May 20, 2020

View reviewed changes

Merge branch 'master' of https://github.com/google/knative-gcp into t…

07e54f3

…rigger-controller

yolocs previously requested changes May 26, 2020

View reviewed changes

pkg/reconciler/broker/broker.go Outdated Show resolved Hide resolved

pkg/reconciler/broker/broker.go Show resolved Hide resolved

pkg/reconciler/broker/broker.go Show resolved Hide resolved

liu-cong mentioned this pull request May 26, 2020

Create brokercell on demand #1132

Merged

grantr reviewed May 27, 2020

View reviewed changes

Mutate broker config in one transaction

2f960d7

yolocs reviewed May 27, 2020

View reviewed changes

pkg/reconciler/broker/broker.go Show resolved Hide resolved

liu-cong added 4 commits May 27, 2020 09:36

Move Delete into a separate MutateBroker call

0c17859

Do not override trigger status message

fa29969

Merge branch 'master' of https://github.com/google/knative-gcp into t…

0fb370d

…rigger-controller

Revert "Move Delete into a separate MutateBroker call"

067a7dd

This reverts commit 0c17859.

liu-cong requested a review from yolocs May 27, 2020 20:39

yolocs reviewed May 28, 2020

View reviewed changes

liu-cong added 2 commits May 28, 2020 10:52

Remove unused serving fakes

b1914d5

Add a TODO to skip non-gcp related triggers

eed3710

grantr approved these changes May 29, 2020

View reviewed changes

knative-prow-robot added the lgtm label May 29, 2020

liu-cong mentioned this pull request May 29, 2020

Don't add finalizers to triggers that are not managed by GCP broker #1164

Closed

knative-prow-robot added the do-not-merge/hold label May 29, 2020

Do not update status if broker is not found or error, eventing contro…

f54f83b

…ller should handle this

knative-prow-robot removed the lgtm label May 29, 2020

grantr approved these changes May 29, 2020

View reviewed changes

knative-prow-robot added the lgtm label May 29, 2020

knative-prow-robot assigned yolocs May 29, 2020

knative-prow-robot removed the do-not-merge/hold label May 29, 2020

knative-prow-robot merged commit 123a9fe into google:master May 30, 2020


		r.Logger.Info("Setting up event handlers")

		triggerInformer.Informer().AddEventHandlerWithResyncPeriod(controller.HandleAll(impl.Enqueue), reconciler.DefaultResyncPeriod)

Add trigger controller #1094

Add trigger controller #1094

Conversation

liu-cong commented May 19, 2020

Proposed Changes

knative-prow-robot commented May 19, 2020

Choose a reason for hiding this comment

liu-cong May 20, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liu-cong May 27, 2020 • edited Loading

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

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

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

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

yolocs left a comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grantr left a comment

Choose a reason for hiding this comment

liu-cong commented May 29, 2020

liu-cong commented May 29, 2020

knative-metrics-robot commented May 29, 2020

liu-cong commented May 29, 2020

grantr left a comment

Choose a reason for hiding this comment

knative-prow-robot commented May 29, 2020

yolocs commented May 29, 2020

liu-cong commented May 29, 2020

liu-cong commented May 29, 2020

knative-test-reporter-robot commented May 29, 2020

grantr commented May 29, 2020

liu-cong May 20, 2020 •

edited

Loading

liu-cong May 27, 2020 •

edited

Loading