Move left over delivery helpers from pkg/buses to pkg/provisioners by neosab · Pull Request #626 · knative/eventing

neosab · 2018-11-16T20:38:25Z

Proposed Changes

Move message_dispatcher, message_receiver, logging helpers from pkg/buses to pkg/provisioners
Renames channel dispatcher k8s service from <provisioner>-clusterbus to <provisioner>-dispatcher. UPGRADE IMPACT Please follow the discussion below. Thanks @Harwayne

Release Note

Renamed channel dispatcher k8s service from `<provisioner>-clusterbus.svc.cluster.local` to `<provisioner`>-dispatcher.svc.cluster.local`

neosab · 2018-11-16T20:43:26Z

/cc @Harwayne @matzew @scothis

neosab · 2018-11-16T21:09:24Z

/test pull-knative-eventing-integration-tests

Harwayne · 2018-11-16T21:16:22Z

Renames channel dispatcher k8s service from <provisioner>-clusterbus to <provisioner>-dispatcher. Please suggest if there is a better naming to be followed. I don't think this has any impact on the end-user (now that we released 0.2.0) since this is not the Sink URL and it's behind the VirtualService.

I'm not sure this is a safe change. I don't think we validate the spec of the VirtualService during normal Channel reconciliation, just that something with the right name exists. So any Channels that exist before this update is applied will point at the old Service, which will likely work until some point in the future when that Service is deleted.

neosab · 2018-11-16T21:48:32Z

I'm not sure this is a safe change. I don't think we validate the spec of the VirtualService during normal Channel reconciliation, just that something with the right name exists. So any Channels that exist before this update is applied will point at the old Service, which will likely work until some point in the future when that Service is deleted.

You are right, I confused the VirtualService resource name and the DNS name and missed the fact we don't change the former.

Few ways to fix this:

Proposal 1 (the right fix)

As part of channel reconciliation, check the spec of VirtualService and replace the destination host to *-dispatcher.
Also, as part of the ClusterProvisioner controller during reconciliation remove the old dispatcher service *-clusterbus, if found.

Proposal 2 (compromise)

Create two dispatcher service's *-clusterbus and *-dispatcher and keep them around.

Proposal 3 (least impactful)
FWIW, only in-memory channel provisioner is impacted as the kafka channel provisioner was never released with 0.2.0.

So leave in-memory-clusterbus svc as-is.
Use the new service name for all new provisioners (kafka included)

Harwayne · 2018-11-19T17:43:46Z

I'm not sure this is a safe change. I don't think we validate the spec of the VirtualService during normal Channel reconciliation, just that something with the right name exists. So any Channels that exist before this update is applied will point at the old Service, which will likely work until some point in the future when that Service is deleted.

You are right, I confused the VirtualService resource name and the DNS name and missed the fact we don't change the former.

Few ways to fix this:

Proposal 1 (the right fix)

As part of channel reconciliation, check the spec of VirtualService and replace the destination host to *-dispatcher.

Also, as part of the ClusterProvisioner controller during reconciliation remove the old dispatcher service *-clusterbus, if found.

Proposal 2 (compromise)

Create two dispatcher service's *-clusterbus and *-dispatcher and keep them around.

Proposal 3 (least impactful)
FWIW, only in-memory channel provisioner is impacted as the kafka channel provisioner was never released with 0.2.0.

So leave in-memory-clusterbus svc as-is.

Use the new service name for all new provisioners (kafka included)

I guess one important thing we need to know is how backwards compatibility will work. I'm fairly sure we want to support 0.N -> 0.N+1 (upgrading directly from 0.N to 0.N+1) and 0.N+1 -> 0.N+2, without anything breaking. Do we also need to support 0.N -> 0.N+2 (upgrading directly from 0.N to 0.N+2)? @vaikas-google @evankanderson

Regardless of the backwards compatibility story, I think that we should do:

As part of channel reconciliation, check the spec of VirtualService and replace the destination host to *-dispatcher.

We can use a similar procedure as in eventing-sources.

And then, for the short-term, I think this is the proper fix until we determine how long we need to be compatible with the current in-memory-channel

Proposal 3 (least impactful)
FWIW, only in-memory channel provisioner is impacted as the kafka channel provisioner was never released with 0.2.0.

So leave in-memory-clusterbus svc as-is.

Use the new service name for all new provisioners (kafka included)

neosab · 2018-11-21T01:38:32Z

I guess one important thing we need to know is how backwards compatibility will work. I'm fairly sure we want to support 0.N -> 0.N+1 (upgrading directly from 0.N to 0.N+1) and 0.N+1 -> 0.N+2, without anything breaking. Do we also need to support 0.N -> 0.N+2 (upgrading directly from 0.N to 0.N+2)? @vaikas-google @evankanderson

I will raise this in the weekly meeting.

We can use a similar procedure as in eventing-sources.

And then, for the short-term, I think this is the proper fix until we determine how long we need to be compatible with the current in-memory-channel

Thanks! I just did that, please take a look.

Harwayne · 2018-11-27T19:27:27Z

I guess one important thing we need to know is how backwards compatibility will work. I'm fairly sure we want to support 0.N -> 0.N+1 (upgrading directly from 0.N to 0.N+1) and 0.N+1 -> 0.N+2, without anything breaking. Do we also need to support 0.N -> 0.N+2 (upgrading directly from 0.N to 0.N+2)?

Ping @vaikas-google @evankanderson - what are the requirements for backwards compatibility?

grantr · 2018-11-28T01:06:34Z

/milestone v0.2.1

grantr · 2018-11-28T01:13:29Z

/approve
I'll let @Harwayne lgtm.

The release note should mention that the controller will delete the previous service.

If we don't delete the -clusterbus service, will that break anything? What's the impact of leaving it around?

knative-prow-robot · 2018-11-28T01:13:35Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

Harwayne · 2018-11-28T01:18:42Z


+	// The name of the svc has changed since version 0.2.1. Hence, delete old dispatcher service (in-memory-channel-clusterbus)
+	// that was created previously in version 0.2.0 to ensure backwards compatibility.
+	err := r.deleteOldDispatcherService(ctx, ccp)


I'm not sure how much it matters because Istio won't notice right away, but move this after util.CreateDispatcherService, so that the correct VirtualService is updated before the old Service is deleted.

I don't think it matters, I changed the order.

Harwayne · 2018-11-28T01:20:07Z

+	// Update VirtualService if it has changed. This is possible since in version 0.2.0, the destinationHost in
+	// spec.HTTP.Route for the dispatcher was changed from *-clusterbus to *-dispatcher. Even otherwise, this
+	// reconciliation is useful for the future mutations to the object.
+	expected := newVirtualService(channel)


Let's make the same change to K8s Services as well (comparing and updating if needed).

Hmm, I did this but had to tweak a bit since service.spec.clusterIP is immutable. Take a look and let me know.

Harwayne · 2018-11-28T01:23:14Z

If we don't delete the -clusterbus service, will that break anything? What's the impact of leaving it around?

Nothing will break, but we will always have lingering doubts that something depends on it.

Harwayne

/lgtm

knative-metrics-robot · 2018-11-28T04:42:32Z

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

File	Old Coverage	New Coverage	Delta
pkg/controller/eventing/inmemory/clusterchannelprovisioner/reconcile.go	100.0%	94.2%	-5.8
pkg/provisioners/channel_util.go	85.1%	84.7%	-0.4
pkg/provisioners/logger.go	Do not exist	100.0%
pkg/provisioners/message.go	Do not exist	100.0%
pkg/provisioners/message_dispatcher.go	Do not exist	89.1%
pkg/provisioners/message_receiver.go	Do not exist	0.0%
pkg/provisioners/provisioner_util.go	77.3%	78.8%	1.5
pkg/provisioners/references.go	Do not exist	100.0%

Harwayne

/lgtm

* Initial work on the GCP PubSub Channel Controller. * Switch to the more standard model, a single global dispatcher. * Args are now defaulted on the controller, removed the ability to customize them per channel. We can add them back later if desired. * YAML and scaffold for the dispatcher. * Created the 'receive' side of the dispatcher. The dispatch side is totally unimplemented. * Initial work on the dispatcher. * More work on the dispatcher. * Dispatcher mostly working. * Handle the reconcileChan * Minor improvements. * Touch ups. * Rename Run->Start. * Fake PubSub. * Util unit tests. * Unit tests for the channel controller. * Beginning unit tests for dispatcher. * update-deps * Unit tests for dispatcher/reconcile.go * Unit test dispatcher/receiver. * Add a README. * DeepCopy the sub * Add the ServiceEntry to the Channel's config. * Typo * PR comments * buses -> provisioners * Switch to the common utils. * Add VirtualService.Update RBAC now that VirtualServices are updated if incorrect. * PR comments * Move the contoller packages inside the controller folder. * PR comments (mostly log levels). * Desugar to get the fields right. * Switch service name, to match #626.

Remove pkg/busses and refactor references

6769fe8

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Nov 16, 2018

knative-prow-robot requested review from evankanderson and inlined November 16, 2018 20:38

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

neosab changed the title ~~Movie left over delivery helpers from pkg/busses to pkg/provisioners~~ Movie left over delivery helpers from pkg/buses to pkg/provisioners Nov 16, 2018

neosab changed the title ~~Movie left over delivery helpers from pkg/buses to pkg/provisioners~~ Move left over delivery helpers from pkg/buses to pkg/provisioners Nov 16, 2018

knative-prow-robot requested review from Harwayne, matzew and scothis November 16, 2018 20:43

matzew reviewed Nov 19, 2018

View reviewed changes

Comment thread pkg/provisioners/message.go

matzew reviewed Nov 19, 2018

View reviewed changes

Comment thread pkg/provisioners/message_receiver.go

matzew reviewed Nov 19, 2018

View reviewed changes

Comment thread pkg/provisioners/logger_test.go Outdated

neosab added 2 commits November 20, 2018 11:30

Merge remote-tracking branch 'upstream/master' into refactor_busses

ccde57d

Compare virtualservice spec, delete dispatcher svc and add tests

406c415

knative-prow-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 21, 2018

knative-prow-robot added this to the v0.2.1 milestone Nov 28, 2018

Merge remote-tracking branch 'upstream/master' into refactor_busses

3e3451c

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

Harwayne reviewed Nov 28, 2018

View reviewed changes

neosab added 3 commits November 27, 2018 18:49

Address PR comments

bba895a

Update RBAC for kafka

271fc4b

Fix setting clusterIP

c598ef1

knative-prow-robot assigned Harwayne Nov 28, 2018

Harwayne reviewed Nov 28, 2018

View reviewed changes

knative-prow-robot added lgtm Indicates that a PR is ready to be merged. and removed lgtm Indicates that a PR is ready to be merged. labels Nov 28, 2018

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

Harwayne reviewed Nov 28, 2018

View reviewed changes

knative-prow-robot merged commit 5fb16a2 into knative:master Nov 28, 2018

Harwayne mentioned this pull request Nov 29, 2018

GCP PubSub Channel #618

Merged

neosab deleted the refactor_busses branch November 29, 2018 23:26

Harwayne added a commit to Harwayne/knative-eventing that referenced this pull request Dec 3, 2018

Switch service name, to match knative#626.

74a1b6f

Harwayne mentioned this pull request Dec 4, 2018

CreateK8sService diffs its expected spec against the actual spec and updates the K8s Service appropriately #666

Merged

Harwayne mentioned this pull request Feb 7, 2019

Add Harwayne as an approver. #782

Merged

creydr added a commit to creydr/knative-eventing that referenced this pull request Jun 17, 2024

Fix possible nil pointer dereference in event-dispatcher (knative#626)

3894539

Conversation

neosab commented Nov 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed Changes

Uh oh!

neosab commented Nov 16, 2018

Uh oh!

neosab commented Nov 16, 2018

Uh oh!

Harwayne commented Nov 16, 2018

Uh oh!

neosab commented Nov 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Harwayne commented Nov 19, 2018

Uh oh!

neosab commented Nov 21, 2018

Uh oh!

Harwayne commented Nov 27, 2018

Uh oh!

grantr commented Nov 28, 2018

Uh oh!

grantr commented Nov 28, 2018

Uh oh!

knative-prow-robot commented Nov 28, 2018

Uh oh!

Harwayne Nov 28, 2018

Choose a reason for hiding this comment

Uh oh!

neosab Nov 28, 2018

Choose a reason for hiding this comment

Uh oh!

Harwayne Nov 28, 2018

Choose a reason for hiding this comment

Uh oh!

neosab Nov 28, 2018

Choose a reason for hiding this comment

Uh oh!

Harwayne commented Nov 28, 2018

Uh oh!

Harwayne left a comment

Choose a reason for hiding this comment

Uh oh!

knative-metrics-robot commented Nov 28, 2018

Uh oh!

Harwayne left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

neosab commented Nov 16, 2018 •

edited

Loading

neosab commented Nov 16, 2018 •

edited

Loading