[WIP] Heartbeat provisioner #513

n3wscott · 2018-10-10T22:50:26Z

This is a demo of a simple heartbeat container.

Steps to run:

install the in-memory channel.
ko publish github.com/knative/eventing/pkg/provisioners/heartbeats/cmd/
ko apply -f https://raw.githubusercontent.com/n3wscott/knative-playground/master/cmd/dumper/dumper.yaml
ko apply -f config/500-controller.yaml
kubectl apply -f pkg/provisioners/heartbeats/config/examples/

This will heartbeat into a generated channel, and then a subscription tie the channel to the message dumper.

TODO: there are changes I had to make to the eventing api, I will move those changes to another PR asap.

knative-prow-robot · 2018-10-10T22:50:31Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: n3wscott
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: inlined

If they are not already assigned, you can assign the PR to them by writing /assign @inlined in a comment when ready.

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

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

pkg/provisioners/heartbeats/config/heartbeat.yaml

matzew · 2018-10-12T11:27:58Z

pkg/provisioners/heartbeats/config/examples/subscription.yaml

+    target:
+      kind: Service
+      apiVersion: serving.knative.dev/v1alpha1
+      name: message-dumper


is it possible to also use an image here ?

I see you building your message-dumper Golang-container using ko, but was wondering if ko is required for building "sources" ? (and other artifacts in the new spec) ?

ko is just a nice way to push containers to a registry if you are using go.

matzew · 2018-10-12T11:31:32Z

pkg/provisioners/heartbeats/config/examples/source.yaml

+      name: heartbeats
+  arguments:
+    label: "<3"
+    period: 2


Could the kind: Source also reference to an image, backing the code (if not using ko )?

In general, the specific images to use should be an implementation detail of the Provisioner. If the provisioner chooses to expose an image, then it could be defined in the Source arguments.

Did this. I intended to learn something with the heartbeat source and I have... :D

matzew · 2018-10-12T13:43:50Z

Isn't that alot of code for an event-source and the provisioner ? 🤔

matzew · 2018-10-12T13:46:22Z

pkg/provisioners/heartbeats/cmd/main.go

+
+func send() {
+	hb.Sequence++
+	resp, err := http.Post(remote, "application/json", body())


here, the source invokes the target (aka ksvc), why would it directly invoke HTTP?

I would think think it uses a channel, that might have an http implementation (or something else)?

@matzew There's no indication here whether remote is a dns name for a Channel, KService or anything else. Which is good for this level. How the value for remote is set is a higher level concern.

I get the remote is somehow injected - fine.

I was more asking. Do we need (perhaps in the future) some higher level API ? that this would use a channel instead of an http client (e.g. when the spec supports other protocols as well). Perhaps out of scope for now...

This is assuming that the channel speaks http. So the source makes a channel, waits for it to resolve, and then gives the pod the resolved channel host as remote.

It is using a channel. The heartbeat code just does not know it. :D

matzew · 2018-10-15T09:58:06Z

Looking in the code/example, I see a subscription for a Channel(well Source atm) targeting a function. OK.

However, I wonder how this would look like, if the subscription would not include a call target, but instead the result to put it to a different channel.

Also, I see the source, in the example is doing direct HTTP invocation. Isn't there some "channel" class/object missing, that the source would use instead ? I'd think this is needed for better abstraction

Here is an example for a channel to channel routing, and than eventually that second channel has a subscription, which will trigger the HTTP ksvc function?!

Channel to Channel

apiVersion: eventing.knative.dev/v1alpha1
kind: Subscription
metadata:
  name: love-dumper1
  namespace: default
spec:
  from:
    kind: Channel
    apiVersion: eventing.knative.dev/v1alpha1
    name: love
  result:
    target:
      kind: Channel
      apiVersion: eventing.knative.dev/v1alpha1
      name: love-dumper-output

Receive from second Channel

apiVersion: eventing.knative.dev/v1alpha1
kind: Subscription
metadata:
  name: love-dumper2
  namespace: default
spec:
  from:
    kind: Channel
    apiVersion: eventing.knative.dev/v1alpha1
    name: love-dumper-output
  call:
    target:
      kind: Service
      apiVersion: serving.knative.dev/v1alpha1
      name: message-dumper

Is this a valid example? would it work with the proposed spec? Why does the example do direct HTTP invocations?

This assumes changes from @scothis in n3wscott#4 are accepted (e.g. only Channel is subscribable)

scothis · 2018-10-15T14:36:34Z

cmd/controller/controller-runtime-main.go

@@ -49,6 +50,7 @@ type ProvideFunc func(manager.Manager) (controller.Controller, error)
 // be added to the default providers list.
 var ExperimentalControllers = map[string]ProvideFunc{
 	"subscription.eventing.knative.dev": subscription.ProvideController,
+	"heartbeats-provisioner":            heartbeatcontroller.ProvideController,


A Source should be deployed independently. Otherwise, this encourages all other Source implementations to want to be included in this repo and compiled into the controller binary

This is just for experimenting with the model. Decouple the how deployment works with what the reconciling is like.

scothis · 2018-10-15T14:45:09Z

pkg/apis/eventing/v1alpha1/source_types.go

+	// +optional
+	// +patchMergeKey=name
+	// +patchStrategy=merge
+	Provisioned []ProvisionedObjectStatus `json:"provisioned,omitempty" patchStrategy:"merge" patchMergeKey:"name"`


name does not uniquely identify a resource since two resource can have the same name, but different types.

good catch. I moved this code to #517

Will fix there.

scothis · 2018-10-15T14:50:45Z

pkg/provisioners/heartbeats/cmd/main.go

+
+func send() {
+	hb.Sequence++
+	resp, err := http.Post(remote, "application/json", body())


@matzew There's no indication here whether remote is a dns name for a Channel, KService or anything else. Which is good for this level. How the value for remote is set is a higher level concern.

scothis · 2018-10-15T14:51:32Z

pkg/provisioners/heartbeats/cmd/main.go

+	defer resp.Body.Close()
+}
+
+func body() io.Reader {


Should this be a CloudEvent?

It does not need to be a CloudEvent. It can be though.

scothis · 2018-10-15T14:54:39Z

pkg/provisioners/heartbeats/config/examples/source.yaml

+      name: heartbeats
+  arguments:
+    label: "<3"
+    period: 2


In general, the specific images to use should be an implementation detail of the Provisioner. If the provisioner chooses to expose an image, then it could be defined in the Source arguments.

scothis · 2018-10-15T14:58:40Z

pkg/provisioners/heartbeats/controller/reconcile.go

+		return reconcile.Result{}, nil
+	}
+
+	original := source.DeepCopy()


This line will cause issue if there is any kind of cache within r.client since the "original" is the copy and the true original is being mutated.

scothis · 2018-10-15T15:06:23Z

pkg/provisioners/heartbeats/controller/resources/channel.go

+		Spec: v1alpha1.ChannelSpec{
+			Provisioner: &v1alpha1.ProvisionerReference{
+				Ref: &corev1.ObjectReference{
+					Name:       "in-memory-channel",


Should the heartbeat Source require the in-memory-channel Provisioner? It minimally needs to be documented as such.

This is demo code... :D

pkg/provisioners/heartbeats/controller/resources/channel.go

scothis · 2018-10-15T15:18:15Z

pkg/provisioners/heartbeats/controller/reconcile.go

+
+	}
+
+	pod, err := r.getPod(ctx, source)


A Deployment would offer better semantics around updates, which are not handled today.

Moved to a deployment.

pkg/provisioners/heartbeats/controller/resources/pod.go

n3wscott · 2018-10-16T13:57:38Z

Isn't that alot of code for an event-source and the provisioner ? 🤔

Look now, the source is 72 lines

And we don't intend average people to implement Provisioners. The hope is there will be an ecosystem of Provisioners where you can select the one that will do the job you need to do.

matzew · 2018-10-16T15:44:23Z

Look now, the source is 72 lines

And we don't intend average people to implement Provisioners. The hope is there will be an ecosystem of Provisioners where you can select the one that will do the job you need to do.

@n3wscott Thanks for the reply.

First of all, great: the source now is really simple. cool. I was also hoping for a rich eco-system of Source/Channel provisioners.

i like the changed version of this better than before your changes

matzew · 2018-10-16T15:45:37Z

@n3wscott question: the Source, instead of doing hard coded http. Could it write to some more abstract Channel type, e.g. like: https://github.com/knative/eventing/blob/master/pkg/apis/eventing/v1alpha1/channel_types.go#L34

deissnerk · 2018-10-16T16:07:51Z

@matzew My understanding is that so far between Source and Channel there is only http possible. If other protocols were supported, the (Source?) Provisioner would need to check, if the according Source and Channel are able to communicate. In #508 I was thinking of a Source that uses AMQP to deliver data to a Channel.

matzew · 2018-10-16T16:15:04Z

@deissnerk I think the source, now just uses direct HTTP. using the "remote" ass its URL

I think that a channel could be (in the future?) passed in too

cc @n3wscott

deissnerk · 2018-10-16T16:31:12Z

@matzew But remote seems to refer to the Channel that is used:

remote := fmt.Sprintf("--remote=http://%s", channel.Status.Sinkable.DomainInternal
containerArgs = append(containerArgs, remote)

n3wscott · 2018-10-16T17:28:28Z

I think that a channel could be (in the future?) passed in too

remote is misnamed, it should be called channel I think to make this clear.

n3wscott · 2018-10-16T17:29:23Z

I used remote because the sample was taken from another demo app that was used that knows nothing about eventing...

knative-metrics-robot · 2018-10-18T21:36:44Z

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/apis/eventing/v1alpha1/source_types.go	100.0%	38.5%	-61.5
pkg/provisioners/container/controller/provider.go	Do not exist	0.0%
pkg/provisioners/container/controller/reconcile.go	Do not exist	0.0%
pkg/provisioners/container/controller/resources/channel.go	Do not exist	0.0%
pkg/provisioners/container/controller/resources/deployment.go	Do not exist	0.0%
pkg/provisioners/sdk/provider.go	Do not exist	0.0%
pkg/provisioners/sdk/reconciler.go	Do not exist	0.0%
pkg/provisioners/sdk/status_accessor.go	Do not exist	0.0%
pkg/sources/heartbeats/cmd/main.go	Do not exist	13.6%

grantr · 2018-10-22T18:24:47Z

@n3wscott do we still want this in the repo or can it be moved to eventing-sources?

n3wscott · 2018-10-22T21:19:44Z

Closed in favor of https://github.com/knative/eventing-sources

Updating patches to latest

* Add TLS updates to main Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com> * Add TLS issuers Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com> --------- Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>

Scott Nichols added 5 commits October 10, 2018 11:44

checkpoint.

280adb5

checkpoint.

9d150e7

worked

db2a828

love wins.

439a24e

heart.

1f7d231

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 Oct 10, 2018

knative-prow-robot requested review from evankanderson and scothis October 10, 2018 22:50

knative-prow-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Oct 10, 2018

Scott Nichols added 2 commits October 11, 2018 08:14

Merge branch 'master' into heartbeat-provisioner

737b2a7

ambicontroller.

c43236a

neosab mentioned this pull request Oct 11, 2018

Adds Kafka Channel Provisioner Controllers #468

Merged

matzew reviewed Oct 12, 2018

View reviewed changes

pkg/provisioners/heartbeats/config/heartbeat.yaml Outdated Show resolved Hide resolved

matzew reviewed Oct 12, 2018

View reviewed changes

Looking at abstracting some of the reconciler out. Not quite there yet.

518b0df

scothis reviewed Oct 15, 2018

View reviewed changes

Scott Nichols added 4 commits October 15, 2018 10:21

broke heartbeats into container and a example.

7c6908c

workes with some mondo changes to channels.

d24e123

Update codegen.

109b59c

Merge branch 'master' into heartbeat-provisioner

3e3d833

n3wscott mentioned this pull request Oct 16, 2018

[WIP] RFC: applying controller sdk to reconciler for Subscriptions. #525

Closed

matzew mentioned this pull request Oct 17, 2018

using channel n3wscott/eventing#11

Closed

Merge branch 'master' into heartbeat-provisioner

ba913cb

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

n3wscott closed this Oct 22, 2018

matzew pushed a commit to matzew/eventing that referenced this pull request Apr 7, 2020

Merge pull request knative#513 from matzew/add_new_patches

e8ad294

Updating patches to latest

[WIP] Heartbeat provisioner #513

[WIP] Heartbeat provisioner #513

Conversation

n3wscott commented Oct 10, 2018

knative-prow-robot commented Oct 10, 2018

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

matzew commented Oct 12, 2018

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

matzew commented Oct 15, 2018 • edited Loading

Channel to Channel

Receive from second Channel

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

n3wscott commented Oct 16, 2018

matzew commented Oct 16, 2018

matzew commented Oct 16, 2018

deissnerk commented Oct 16, 2018

matzew commented Oct 16, 2018

deissnerk commented Oct 16, 2018

n3wscott commented Oct 16, 2018

n3wscott commented Oct 16, 2018

knative-metrics-robot commented Oct 18, 2018

grantr commented Oct 22, 2018

n3wscott commented Oct 22, 2018

matzew commented Oct 15, 2018 •

edited

Loading