refactor in-memory channel to support sync and async handlers by sbezverk · Pull Request #708 · knative/eventing

sbezverk · 2019-01-09T00:59:25Z

Signed-off-by: Serguei Bezverkhi sbezverk@cisco.com

This PR add support of async way of calling fanout handler and refactors in-memory-channel to support for both types, sync(old) and async(new)

sbezverk · 2019-01-09T00:59:49Z

/assign @Harwayne

Harwayne

Looks like a great start.

Harwayne · 2019-01-09T15:44:21Z

 		return err
 	}
-	config := multiChannelFanoutConfig(channels)
+	// Temporary adding logger


Is this intended to be committed?

This is a temporary, as I am debugging Async e2e case failure, will be removed before the final commit.

Harwayne · 2019-01-09T15:46:00Z

 // Configuration for a fanout.Handler.
 type Config struct {
 	Subscriptions []eventingduck.ChannelSubscriberSpec `json:"subscriptions"`
+	AsyncHandler  bool                                 `json:"asyncHandler"`


My preference is Async, rather than AsyncHandler.

It should probably omit on empty as well.

Async bool `json:"async,omitempty"`

I can change but just Async sounds a bit too ambiguous, where AsyncHandler clearly states what that is. Please let me know if you still want me to change it.

I have a minor preference for async, but I can see why you prefer asyncHandler. Unless someone else chimes in, leave it as-is.

Harwayne · 2019-01-09T15:47:21Z

 // the `sink` portions of the subscription.
 func (f *Handler) makeFanoutRequest(m provisioners.Message, sub eventingduck.ChannelSubscriberSpec) error {
+	if f.config.AsyncHandler {
+		f.logger.Info("><SB> Exercising ASYNC path in Fanout Request for", zap.Any("m = ", m), zap.Any("sub = ", sub))


Probably shouldn't be committed. If is committed, should be Debug level.

Also, zap.Any("m", m) is sufficient (as compared to "m = "), because the string is used as a key in the printed JSON log.

will be removed completely after debugging is concluded

Harwayne · 2019-01-09T15:52:12Z

 // Configuration for a fanout.Handler.
 type Config struct {
 	Subscriptions []eventingduck.ChannelSubscriberSpec `json:"subscriptions"`
+	AsyncHandler  bool                                 `json:"asyncHandler"`


Add documentation, something like:

// AsyncHandler controls whether the Subscriptions are called synchronous or asynchronously. It is expected to be false when used as a sidecar.

Harwayne · 2019-01-09T15:53:56Z

 		if c.Spec.Subscribable != nil {
+			// TODO After in-memory-channel is retired, this logic must be refactored.
+			asyncHandler := false
+			if c.Spec.Provisioner.Name == asyncProvisionerName {


Add a unit test to verify the state of asyncHandler.

n3wscott · 2019-01-16T18:46:33Z

 // the `sink` portions of the subscription.
 func (f *Handler) makeFanoutRequest(m provisioners.Message, sub eventingduck.ChannelSubscriberSpec) error {
+	if f.config.AsyncHandler {
+		go f.dispatcher.DispatchMessage(&m, sub.SubscriberURI, sub.ReplyURI, provisioners.DispatchDefaults{})


we will drop all errors with this. I think you could wrap in an anon function to log the error at least:

go func () { if err := f.dispatcher....; err != nil { log err } }

n3wscott · 2019-01-28T22:11:02Z


 **/zz_generated.*.go linguist-generated=true
 /pkg/client/** linguist-generated=true
+/contrib/*/pkg/client/** linguist-generated=true


what's this?

Please ignore, merging different branches issue. Just resolved now.

n3wscott · 2019-01-28T22:12:30Z

It looks like this PR needs to be rebased on master, it contains a lot of commits that are not related

Signed-off-by: Serguei Bezverkhi <sbezverk@cisco.com>

Harwayne

Looks good in general, some small comments before I LGTM.

However, we need to make sure to test this manually before submitting it. Our e2e tests should be good enough that we can rely on them, but I don't feel that is true right now.

Harwayne · 2019-01-29T22:35:55Z

+      echo "========== Collecting logs for pod: "${pod}" in $namespace ================="
+      for container in $(kubectl get pod "${pod}" -n $namespace -ojsonpath='{.spec.containers[*].name}'); do
+        echo "----------------------------------------------------------"
+        echo "Container: "${container}


My preference is to include all the pieces on one line (preferably in an easily searchable way). Something like:

echo "Namespace, Pod, Container: ${namespace}, ${pod}, ${container}"

Harwayne · 2019-01-29T22:37:15Z

+        echo "----------------------------------------------------------"
+        echo "Container: "${container}
+        kubectl logs -n $namespace "${pod}" -c "${container}" || true
+        echo "----------------------------------------------------------"


Also write out that this is a previous incarnation of a Pod. Something like:

echo "Namespace, Pod, Container -- Previous Instance: ${namespace}, ${pod}, ${container}"

Harwayne · 2019-01-29T22:40:00Z


 func createReceiverFunction(f *Handler) func(provisioners.ChannelReference, *provisioners.Message) error {
 	return func(_ provisioners.ChannelReference, m *provisioners.Message) error {
+		if f.config.AsyncHandler {


Modified existing unit test by adding one test case where AsyncHandler is set true. Let me know if you think it is sufficient.

I don't think anything has been pushed.

Harwayne · 2019-01-29T22:40:54Z

-		logger.Info("Error creating the Virtual Service for the Channel", zap.Error(err))
-		r.recorder.Eventf(c, corev1.EventTypeWarning, virtualServiceCreateFailed, "Failed to reconcile Virtual Service for the Channel: %v", err)
-		return err
+	if c.Spec.Provisioner.Name == asyncProvisionerName {


Are both paths of this 'if' statement unit tested? If not, do so.

I think this still isn't unit tested.

Right, but when I look at the code, in both cases, both branches of if, we create exactly the same virtual service (unless I was reading code incorrectly), even if a different from in-memory provisioner name is used we overwrite it to in-memory. So I am not sure what to test here, the only I could think of is if with a different than in-memory provisioner name, but that would test only golang's if. WDYT?

I want the unit test to test that the created VirtualService has the same destination regardless of whether or not in-memory or in-memory-channel are used.

As it is, I don't think we have any unit tests for the new in-memory behavior. As that is the provisioner we want going forward, I would like it tested. And once that is tested, I want to also test that in-memory-channel behaves correctly, namely that the created VirtualService points at the correct place.

Signed-off-by: Serguei Bezverkhi <sbezverk@cisco.com>

sbezverk · 2019-01-30T16:02:47Z

@Harwayne sorry, family emergency did not have time to push actual change.

Harwayne · 2019-01-30T16:18:38Z

@Harwayne sorry, family emergency did not have time to push actual change.

No problem, I've definitely forgotten to push in the past and wanted to let you know why the review wasn't going to move forward. Thanks for pushing, looking now.

Signed-off-by: Serguei Bezverkhi <sbezverk@cisco.com>

Harwayne · 2019-01-31T21:49:17Z

+			},
+			Mocks: controllertesting.Mocks{},
+			WantPresent: []runtime.Object{
+				makeVirtualService(),


Also makeK8sService("in-memory")

Harwayne · 2019-01-31T21:49:23Z

+			},
+			Mocks: controllertesting.Mocks{},
+			WantPresent: []runtime.Object{
+				makeVirtualService(),


Also makeK8sService("in-memory")

Sorry, I think this should have been makeK8sService("in-memory-channel"). Sorry for the confusion.

Harwayne · 2019-01-31T21:50:21Z


-func makeChannel() *eventingv1alpha1.Channel {
+func makeChannel(pn ...string) *eventingv1alpha1.Channel {
+	provisionerName := ccpName


Clever way to default. Move it to a separate function and add a comment saying what you are doing.

Signed-off-by: Serguei Bezverkhi <sbezverk@cisco.com>

Harwayne · 2019-02-01T22:16:55Z

-	svc, err := util.CreateK8sService(ctx, r.client, c)
+	cCopy := c.DeepCopy()
+	cCopy.Spec.Provisioner.Name = asyncProvisionerName
+	svc, err := util.CreateK8sService(ctx, r.client, cCopy)


Why are we always using in-memory here? Is it only because of my comment in the tests?

yep, fixed it

Harwayne · 2019-02-01T22:17:50Z

+			},
+			Mocks: controllertesting.Mocks{},
+			WantPresent: []runtime.Object{
+				makeVirtualService(),


Sorry, I think this should have been makeK8sService("in-memory-channel"). Sorry for the confusion.

Signed-off-by: Serguei Bezverkhi <sbezverk@cisco.com>

Harwayne

/lgtm
/hold

Please manually test in your cluster to see that both in-memory and in-memory-channel Channels work simultaneously. Once tested, feel free to cancel the hold yourself.

Also test the new in-memory with the e2e test:

export CLUSTER=<entry from ~/.kube/config file>
export NAMESPACE=<something random>

# This creates all the images. Without it, I was either seeing images that couldn't be pulled or stale images.
test/upload-test-images.sh e2e

# Actually running the tests.
go test -v -tags=e2e -count=1 ./test/e2e -dockerrepo=gcr.io/$PROJECT_ID -cluster=$CLUSTER -namespace=$NAMESPACE -clusterChannelProvisioner=in-memory

Harwayne · 2019-02-01T22:42:39Z

/assign @grantr

grantr

Thanks for seeing this through @sbezverk and @Harwayne!

grantr · 2019-02-01T22:51:59Z

 }

-func makeK8sService() *corev1.Service {
+func makeK8sService(pn ...string) *corev1.Service {


Suggestion for a different PR: Seems like this could benefit from the builder pattern for making fixtures with varying aspects. That might make it more clear to the reader what these arguments mean. See builder.go and SubscriptionBuilder.

Thank you for pointing out.

knative-prow-robot · 2019-02-01T23:02:08Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

sbezverk · 2019-02-02T00:19:03Z

@Harwayne Interesting problem :)
when old in memory channel type is used, we create k8s service with matching name knative-eventing in-memory-channel-dispatcher, but VirtualService uses:

  spec:
    hosts:
    - k8s-events-channel-s82gp.default.svc.sbezverk.cisco.com
    - k8s-events.default.channels.sbezverk.cisco.com
    http:
    - rewrite:
        authority: k8s-events.default.channels.sbezverk.cisco.com
      route:
      - destination:
          host: in-memory-dispatcher.knative-eventing.svc.sbezverk.cisco.com    <----- !!!!!!!
          port:
            number: 80
        weight: 0

VirtualService uses in destination new in-memory. That makes a breakage in istio. I checked route entries and it points to a non existing in-memory-dispatcher.knative-eventing.svc.sbezverk.cisco.com. I am afraid we might need to re-think this solution.

/hold

Signed-off-by: Serguei Bezverkhi <sbezverk@cisco.com>

Harwayne

/lgtm

sbezverk · 2019-02-02T01:59:34Z

/hold cancel

Signed-off-by: Serguei Bezverkhi <sbezverk@cisco.com>

knative-metrics-robot · 2019-02-02T04:59:22Z

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/channel/reconcile.go	97.9%	94.4%	-3.5
pkg/controller/eventing/inmemory/clusterchannelprovisioner/reconcile.go	93.1%	93.4%	0.3

sbezverk · 2019-02-02T12:32:18Z

@Harwayne please lgtm, it was lost due to one final tiny change,

Harwayne · 2019-02-03T23:44:57Z

  selector:
    matchLabels: &labels
-      clusterChannelProvisioner: in-memory
+      clusterChannelProvisioner: in-memory-channel


Why the change?

CI was failing e2e test

Harwayne

/lgtm

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 Jan 9, 2019

knative-prow-robot requested review from inlined and scothis January 9, 2019 00:59

knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 9, 2019

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Jan 9, 2019

knative-prow-robot assigned Harwayne Jan 9, 2019

Harwayne reviewed Jan 9, 2019

View reviewed changes

Harwayne mentioned this pull request Jan 10, 2019

Make in-memory-channel dispatcher non-blocking #713

Closed

sbezverk changed the title ~~WIP refactor in-memory channel to support sync and async handlers~~ refactor in-memory channel to support sync and async handlers Jan 11, 2019

knative-prow-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 11, 2019

n3wscott reviewed Jan 16, 2019

View reviewed changes

knative-prow-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 28, 2019

n3wscott reviewed Jan 28, 2019

View reviewed changes

Consolidating commits

031c09f

Signed-off-by: Serguei Bezverkhi <sbezverk@cisco.com>

knative-prow-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 28, 2019

Harwayne reviewed Jan 29, 2019

View reviewed changes

Addressing comments

6666e3e

Signed-off-by: Serguei Bezverkhi <sbezverk@cisco.com>

Adding 2 unit tests

7a8b7fc

Signed-off-by: Serguei Bezverkhi <sbezverk@cisco.com>

Harwayne reviewed Jan 31, 2019

View reviewed changes

Addressing comments

3894373

Signed-off-by: Serguei Bezverkhi <sbezverk@cisco.com>

Harwayne reviewed Feb 1, 2019

View reviewed changes

Addressing comments

a286f38

Signed-off-by: Serguei Bezverkhi <sbezverk@cisco.com>

Harwayne reviewed Feb 1, 2019

View reviewed changes

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 1, 2019

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

knative-prow-robot assigned grantr Feb 1, 2019

grantr approved these changes Feb 1, 2019

View reviewed changes

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 1, 2019

Final changes

370aa48

Signed-off-by: Serguei Bezverkhi <sbezverk@cisco.com>

knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Feb 2, 2019

Harwayne reviewed Feb 2, 2019

View reviewed changes

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 2, 2019

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

Adjusting yaml

079c109

Signed-off-by: Serguei Bezverkhi <sbezverk@cisco.com>

knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Feb 2, 2019

Harwayne reviewed Feb 3, 2019

View reviewed changes

Harwayne reviewed Feb 4, 2019

View reviewed changes

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 4, 2019

knative-prow-robot merged commit 9431427 into knative:master Feb 4, 2019

This was referenced Feb 7, 2019

Add Harwayne as an approver. #782

Merged

Rename in-memory-channel ClusterChannelProvisioner #676

Closed

Remove in-memory-channel ClusterChannelProvisioner in 0.7 #787

Closed

Harwayne mentioned this pull request Feb 14, 2019

rename in-memory channel #700

Closed

Conversation

sbezverk commented Jan 9, 2019

Uh oh!

sbezverk commented Jan 9, 2019

Uh oh!

Harwayne left a comment

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!

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!

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!

n3wscott commented Jan 28, 2019

Uh oh!

Harwayne left a comment

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sbezverk commented Jan 30, 2019

Uh oh!

Harwayne commented Jan 30, 2019

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Harwayne left a comment •

edited

Loading