Cleanup Bus/ClusterBus reconciler by scothis · Pull Request #420 · knative/eventing

scothis · 2018-09-07T19:43:23Z

Proposed Changes

only start the needed informer for either Bus or ClusterBus, we don't
need both
consolidate the createOrUpdateBus and createOrUpdateClusterBus methods
use BusReference to check if the updated bus is the reconciled bus

Release Note

NONE

/assign @greghaynes

- only start the needed informer for either Bus or ClusterBus, we don't need both - consolidate the createOrUpdateBus and createOrUpdateClusterBus methods - use BusReference to check if the updated bus is the reconciled bus Refs knative#418

neosab

Since Reconciler struct now has a bus reference, we should remove the busRef parameter in itsRun function L293

scothis · 2018-09-07T22:18:49Z

@neosab good catch, updated

neosab · 2018-09-07T22:28:21Z

/lgtm

knative-prow-robot · 2018-09-07T22:28:29Z

@neosab: changing LGTM is restricted to assignees, and only knative/eventing repo collaborators may be assigned issues.

Details

In response to this:

/lgtm

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.

greghaynes · 2018-09-07T22:48:09Z

-	r.bus = clusterBus.DeepCopy()
-	if !equality.Semantic.DeepEqual(r.bus.GetSpec(), previousBus.GetSpec()) {
+	// stash the new bus on the reconciler while retaining the old bus
+	bus, r.bus = r.bus, bus


r.bus is no longer a DeepCopy of the bus we get back from the busesLister, but during create we make a deepcopy in this same scenario (https://github.com/knative/eventing/pull/420/files#diff-d2cf608fb7c41df730d64ac95f391e7aR313). I don't see why the DeepCopy is needed (am I missing something?) but I imagine these two codepaths should operate the same way.

The bus resource on the reconciler should never be mutated. The general pattern is to create a copy before mutating a resource. The DeepCopy here was an extra layer of protected. With the switch to use GenericBus there is no longer a consistent implementation of DeepCopy to call.

I tried to add DeepCopy() GenericBus to the GenericBus interface, but couldn't get the type checker to accept the generated DeepCopy methods on Bus and ClusterBus since they don't return GenericBus but a struct that implements GenericBus.

To make the behavior more consistent we could call DeepCopy() before passing the bus into createOrUpdateBus(), or drop the other (hopefully) redundant DeepCopy calls so that's is obvious a copy needs to be created before mutation. Thoughts?

Ah, makes sense genericbus would cause issues there. Either way seems reasonable to me, as long as we end up with consistency.

Now calling .DeepCopy() before passing the bus to createOrUpdateBus().

neosab

Ah, I just meant to say this PR looks good to me 👍

greghaynes · 2018-09-07T22:50:04Z

Minor question but lgtm

scothis · 2018-09-10T20:30:18Z

/check-cla
/assign @grantr

greghaynes · 2018-09-10T20:39:09Z

/lgtm

grantr

Looks good @scothis! I'd like some clarification about the bus stashing since at first glance it seems like it might not be thread-safe (but I haven't looked at it too carefully).

grantr · 2018-09-11T17:34:41Z

-	r.bus = clusterBus.DeepCopy()
-	if !equality.Semantic.DeepEqual(r.bus.GetSpec(), previousBus.GetSpec()) {
+	// stash the new bus on the reconciler while retaining the old bus
+	bus, r.bus = r.bus, bus


Why is this stash needed? How does it work when there are multiple worker threads?

The workqueue guarantees that the same resource will never be emitted concurrently, so that should handle any race concerns.

We capture the bus resource for a few reasons:

in order to resolve parameters for Channels and Subscriptions

to know if a Channel is backed by the particular bus instance

to record Events against

The same resource will not be emitted concurrently, but IIUC there is still the case of threadiness different resources being reconciled simultaneously.

resource A enters worker thread A

resource B enters worker thread B

resource A and B race to set r.bus

What is the value of r.bus?

Is this scenario possible?

Serving and the Feed controller have context.Context objects plumbed through the entire Reconcile loop now, and it's reasonable to expect this will be a general pattern in Knative controllers. We may be able to eliminate the race (if there is one) by stashing the bus resource in a context.Context. That also makes the single-call scope of the bus stash clear. I'm intentionally ignoring any opinions on the design of context.Context for now. 😁

A reconciler is for a single bus resource. Resource A and B will never race to set r.bus because only A or B can be for the the bus that is being reconciled (otherwise there's a major bug in workqueue). When r.bus is updated, the kind, apiVersion, name and namespace of the previous and future value are identical. We set the value to pickup spec mutations.

#422 adds all provisioned Channels and/or Subscriptions (as needed) to the workqueue when the Bus params change. This will cause them to be reprocessed.

@scothis and I talked about this offline. Restating the above a bit: Reconciler will have all Bus update events in its workqueue, but discards update events for all Buses except one. That happens at https://github.com/knative/eventing/pull/420/files#diff-d2cf608fb7c41df730d64ac95f391e7aR542.

The Reconciler's workqueue has multiple object types in it. This is why the Reconciler needs threadiness even though it only ever reconciles a single Bus. This is not conventional. I don't know enough about the implementation to say whether the convention should apply, but I'd to see a comment in the code that acknowledges and briefly explains this break from convention.

So tl;dr my understanding is that this isn't a race condition even though it looks like one. I'd like to see a comment in the code acknowledging that as well.

grantr

/lgtm
/approve
/hold

Ok, we're on the same page now. Thanks for explaining your reasoning @scothis.

These comments describe the contents of workqueue keys incorrectly:
https://github.com/knative/eventing/pull/420/files#diff-d2cf608fb7c41df730d64ac95f391e7aR381
https://github.com/knative/eventing/pull/420/files#diff-d2cf608fb7c41df730d64ac95f391e7aR393
https://github.com/knative/eventing/pull/420/files#diff-d2cf608fb7c41df730d64ac95f391e7aR417

These are issues that are only related to this PR in the sense that I was confused by them while reviewing, so if you want to address them in a separate PR that's fine (you can cancel the hold in that case). Or if you want to address them now I'll lgtm again.

knative-prow-robot · 2018-09-11T23:21:13Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: grantr, scothis

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [grantr]

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

grantr

/lgtm

grantr · 2018-09-11T23:31:08Z

/hold cancel

Cleanup Bus/ClusterBus reconciler

a86786b

- only start the needed informer for either Bus or ClusterBus, we don't need both - consolidate the createOrUpdateBus and createOrUpdateClusterBus methods - use BusReference to check if the updated bus is the reconciled bus Refs knative#418

knative-prow-robot assigned greghaynes Sep 7, 2018

knative-prow-robot requested review from inlined and mattmoor September 7, 2018 19:43

knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 7, 2018

scothis mentioned this pull request Sep 7, 2018

Set (cluster)bus.Kind ourselves #418

Closed

neosab reviewed Sep 7, 2018

View reviewed changes

Remove redudant BusReference passed to Reconciler#Run()

df39751

greghaynes reviewed Sep 7, 2018

View reviewed changes

neosab approved these changes Sep 7, 2018

View reviewed changes

greghaynes mentioned this pull request Sep 10, 2018

Reprovision channels and resubscrube when a bus params are updated #422

Merged

knative-prow-robot assigned grantr Sep 10, 2018

Create copy of Bus before calling createOrUpdateBus

58e8304

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 10, 2018

grantr reviewed Sep 11, 2018

View reviewed changes

Merge remote-tracking branch 'upstream/master' into bus-informers

193c830

knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Sep 11, 2018

Review feedback

a31e6aa

grantr reviewed Sep 11, 2018

View reviewed changes

knative-prow-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels Sep 11, 2018

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 11, 2018

Clarify that the workqueue key is

5f4ffc0

knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Sep 11, 2018

grantr reviewed Sep 11, 2018

View reviewed changes

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

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

knative-prow-robot merged commit 94e79eb into knative:master Sep 11, 2018

scothis deleted the bus-informers branch September 11, 2018 23:39

greghaynes mentioned this pull request Sep 19, 2018

bus updates ignored by reconciler #417

Closed

Conversation

scothis commented Sep 7, 2018

Proposed Changes

Uh oh!

neosab left a comment

Choose a reason for hiding this comment

Uh oh!

scothis commented Sep 7, 2018

Uh oh!

neosab commented Sep 7, 2018

Uh oh!

knative-prow-robot commented Sep 7, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

neosab left a comment

Choose a reason for hiding this comment

Uh oh!

greghaynes commented Sep 7, 2018

Uh oh!

scothis commented Sep 10, 2018

Uh oh!

greghaynes commented Sep 10, 2018

Uh oh!

grantr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

grantr left a comment

Choose a reason for hiding this comment

Uh oh!

knative-prow-robot commented Sep 11, 2018

Uh oh!

grantr left a comment

Choose a reason for hiding this comment

Uh oh!

grantr commented Sep 11, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants