Reprovision channels and resubscrube when a bus params are updated by greghaynes · Pull Request #422 · knative/eventing

greghaynes · 2018-09-10T19:24:59Z

Fixes #392

Proposed Changes

When parameters for a bus are updated we need to reprovision and
resubscribe all channels for that bus.

Release Note

NONE

googlebot · 2018-09-10T19:25:03Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

greghaynes · 2018-09-10T19:25:38Z

/hold This is based on PR #420 , that needs to merge first

greghaynes · 2018-09-10T19:26:06Z

/cc @scothis

knative-metrics-robot · 2018-09-10T19:26:45Z

The following is the coverage report on pkg/.
Say /test pull-knative-eventing-go-coverage to run the coverage report again

File	Old Coverage	New Coverage	Delta
pkg/buses/cache.go	100.0%	75.8%	-24.2

scothis · 2018-09-10T20:05:10Z

Thanks @greghaynes

/hold
/assign @scothis

scothis

Direction looks good. A few comments inline

scothis · 2018-09-10T20:08:22Z

 				if oldBus.ResourceVersion == newBus.ResourceVersion {
 					// Periodic resync will send update events for all known Buses.
 					// Two different versions of the same Bus will always have different RVs.
+					logger.Info("Identical resource version, ignoring update")


If this log message is useful, we should probably also apply it to the other informer event handlers. Otherwise, delete it.

scothis · 2018-09-10T20:23:16Z

+		// If parameters changed we need to reprovision / resubscribe
+		if !equality.Semantic.DeepEqual(r.bus.GetSpec().Parameters, bus.GetSpec().Parameters) {
+			r.logger.Infof("Bus parameters changed. Reprovisioning channels.")
+			for _, channel := range r.cache.AllChannels() {


The Bus Parameters are subdivided into Channel and Subscription params. We could go a step further and check that the channel params have changed before requeueing all channels and that the subscription params have changed before requeueing all subscriptions.

scothis · 2018-09-10T20:24:16Z

 	delete(c.subscriptions, subscriptionRef)
 }
+
+func (c *Cache) AllChannels() []*channelsv1alpha1.Channel {


the cache.go file currently has good test coverage, can we add a couple tests?

scothis · 2018-09-11T15:36:11Z

/lgtm

Remaining steps are to get #420 merged, rebase this PR on top of the new master and get this PR approved.

@evankanderson

scothis · 2018-09-11T23:40:58Z

@greghaynes ready to be reconciled with master

When parameters for a bus are updated we need to reprovision and resubscribe all channels for that bus. Fixes knative#392

googlebot · 2018-09-12T00:05:20Z

CLAs look good, thanks!

greghaynes · 2018-09-12T00:06:49Z

/hold cancel

scothis · 2018-09-12T00:30:03Z

/lgtm

for approval:
/assign @grantr

@evankanderson

From [ROLES.MD](https://github.com/knative/docs/blob/dfc53c67c8e80d30b8863353c9e9b4ad00c41fa0/community/ROLES.md#approver): > Reviewer of the codebase for at least 3 months or 50% of project lifetime, whichever is shorter - [First Issue](#80). Opened 6/11 - [First PR](#66). Opened 5/31 - [First Review](#79 (review)) 6/11 > Primary reviewer for at least 10 substantial PRs to the codebase - #422 (review) - #414 (review) - #325 (review) - #225 (review) - #189 (review) - #168 (review) - #165 (review) - #99 (review) - #79 (review) - #111 (review) > Reviewed or merged at least 30 PRs to the codebase - [Reviewed 23 PRs](https://github.com/knative/eventing/pulls?utf8=✓&q=is%3Apr+reviewed-by%3Ascothis) - [Authored 34 merged PRs](https://github.com/knative/eventing/pulls?utf8=✓&q=is%3Apr+author%3Ascothis+is%3Amerged) - [Authored 5 open PRs](https://github.com/knative/eventing/pulls/scothis) > Nominated by an area lead From [WORKING_GROUPS.MD](https://github.com/knative/docs/blob/dfc53c67c8e80d30b8863353c9e9b4ad00c41fa0/community/WORKING-GROUPS.md#events) /assign @vaikas-google > With no objections from other leads 🤞 /cc @evankanderson @grantr @inlined @mattmoor

scothis · 2018-09-12T18:08:11Z

/approve

knative-prow-robot · 2018-09-12T18:08:16Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

@evankanderson

knative-prow-robot requested review from evankanderson and vaikas September 10, 2018 19:25

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

knative-prow-robot requested a review from scothis September 10, 2018 19:26

greghaynes changed the title ~~Feature/reprovision resub bus update~~ Reprovision channels and resubscrube when a bus params are updated Sep 10, 2018

knative-prow-robot assigned scothis Sep 10, 2018

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

scothis reviewed Sep 10, 2018

View reviewed changes

greghaynes force-pushed the feature/reprovision-resub-bus-update branch from 4fb1666 to ae2a2b3 Compare September 10, 2018 21:17

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

This was referenced Sep 11, 2018

Add scothis as an approver #426

Merged

Cleanup Bus/ClusterBus reconciler #420

Merged

Reprovision/Resub on bus param update

61976ae

When parameters for a bus are updated we need to reprovision and resubscribe all channels for that bus. Fixes knative#392

greghaynes force-pushed the feature/reprovision-resub-bus-update branch from ae2a2b3 to 61976ae Compare September 12, 2018 00:05

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

knative-prow-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 12, 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 12, 2018

knative-prow-robot assigned grantr Sep 12, 2018

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

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

knative-prow-robot merged commit 6585b66 into knative:master Sep 12, 2018

Conversation

greghaynes commented Sep 10, 2018

Proposed Changes

Uh oh!

googlebot commented Sep 10, 2018

Uh oh!

greghaynes commented Sep 10, 2018

Uh oh!

greghaynes commented Sep 10, 2018

Uh oh!

knative-metrics-robot commented Sep 10, 2018

Uh oh!

scothis commented Sep 10, 2018

Uh oh!

scothis left a comment

Choose a reason for hiding this comment

Uh oh!

scothis Sep 10, 2018

Choose a reason for hiding this comment

Uh oh!

scothis Sep 10, 2018

Choose a reason for hiding this comment

Uh oh!

scothis Sep 10, 2018

Choose a reason for hiding this comment

Uh oh!

scothis commented Sep 11, 2018

Uh oh!

scothis commented Sep 11, 2018

Uh oh!

googlebot commented Sep 12, 2018

Uh oh!

greghaynes commented Sep 12, 2018

Uh oh!

scothis commented Sep 12, 2018

Uh oh!

scothis commented Sep 12, 2018

Uh oh!

knative-prow-robot commented Sep 12, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants