Proof of concept GCP Pub Sub using a Receive Adapter by vaikas · Pull Request #57 · knative/eventing

vaikas · 2018-05-22T21:07:39Z

Fixes Issue #

Proposed Changes

This is a checkpoint to provide a proof of concept for unbind and handling more complex receivers. Follow on work will be to yank out the inproc event sources and move them into containers.

Introduce a notion of a Receive Adapter (still in proc creation)
Add Unbind + Context is stashed in the Bind.Status.BindContext so we have enough context to perform unbind and clean up (remove the github webhook, gcp pubsub subscription for example)
Fix the route target resolve so the examples work
Add a function handler that receives GCP PubSub events as a target

google-prow-robot · 2018-05-22T21:07:44Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vaikas-google

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~~ [vaikas-google]

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

…e target of the route

…hack) until we move to container model

mchmarny · 2018-05-22T22:56:38Z

+		if err != nil {
+			log.Printf("Failed to post message: %s", err)
+		}
+		m.Ack()


Should it Nack on postMessage error? If route was down (cold start takes too long) that event would never be processed.

yep, good call.

mchmarny · 2018-05-22T22:58:33Z

+	ctx := context.Background()
+
+	// Creates a client.
+	client, err := pubsub.NewClient(ctx, projectID)


Does the client creation assume the GOOGLE_APPLICATION_CREDENTIALS env var is defined with the gloabal service account here?

currently it uses whatever the GKE has, added a TODO for it.

mattmoor

I'm not really sure how picky I should be on this repo, especially given the "PoC" in the PR title. :)

I did a pass with some comments, will do another later.

mattmoor · 2018-05-22T22:13:07Z

+		m.Ack()
+	})
+	if err != nil {
+		// Handle error.


log.Fatalf?

mattmoor · 2018-05-22T22:13:52Z

+
+	URL := fmt.Sprintf("http://%s/", target)
+	log.Printf("Posting to %q", URL)
+	req, err := http.NewRequest("POST", URL, bytes.NewBuffer(jsonStr))


You don't check err

mattmoor · 2018-05-22T22:18:45Z

+        elafros.dev/type: container
+    spec:
+      container:
+        image: gcp_pubsub_function:latest


image: github.com/elafros/eventing/sample/gcp_pubsub_function (The BUILD file will also need this change)

mattmoor · 2018-05-22T22:23:51Z

+	URL := fmt.Sprintf("http://%s/", target)
+	log.Printf("Posting to %q", URL)
+	req, err := http.NewRequest("POST", URL, bytes.NewBuffer(jsonStr))
+	req.Header.Set("X-Ela-Header", "foovalue")


What is this?

mattmoor · 2018-05-22T22:28:29Z


+type BindContext struct {
+	Context map[string]interface{}
+}


Why the extra level? Why not: type BindContext map[string]interface{}?

I am under the impression that there might be some other context that controller will need that is in addition to the context that needs to be given to the event source (what it needs to unbind), but I think there's especially in the containerized workflow things like, what namespace the container running is. If you're ok with that reasoning, I'd like to leave it for now.

mattmoor · 2018-05-22T22:50:46Z

+	}
+
+	// TODO: Create ownerref to the binding so when the binding goes away deployment
+	// gets removed. Currently we manually do this.


"do this" meaning delete the deployment?

yes, clarified.

mattmoor · 2018-05-22T22:52:28Z

+	if err != nil {
+		return nil, err
+	}
+	finalizers := sets.NewString(accessor.GetFinalizers()...)


Will this roundtrip change ordering? IIUC initializers are ordered, so I'd guess the same is true for finalizers.

mattmoor · 2018-05-22T22:58:00Z

+
+				// Set the finalizer since the bind succeeded, we need to clean up...
+				// TODO: we should do this in the webhook instead...
+				bind.Finalizers = append(bind.ObjectMeta.Finalizers, controllerAgentName)


Won't the use of finalizers make it harder to move this out-of-process? Why use finalizers in place of DELETE-sensitive webhooks?

I don't think it will make it any harder. The controller should try to 'unbind' until it succeeds, then it will remove the finalizer. Why would the use of finalizers make it harder?

When we explored initializers for Build the problem was that doing long operations synchronously with delete was problematic. If the expectation is that this will ultimately launch a container to perform this, then doing it on finalization may become problematic. Not sure if finalizers are a perfect mirror of initializers here.

I don't think it applies here. We used the same pattern for Service Catalog, where deletion of resources can take awhile. Point of the finalizers is that we are telling k8s to not delete the real resources until we're done cleaning up (and then remove the finalizer), I think this is precisely what the finalizers are for?

Finalizers are explicitly asynchronous pre-delete hooks:
https://kubernetes.io/docs/tasks/access-kubernetes-api/extend-api-custom-resource-definitions/#advanced-topics

vaikas · 2018-05-23T19:52:57Z

Thanks for the reviews, PTAL.

mattmoor · 2018-05-23T22:04:20Z

/lgtm

I'm a little squeamish about PoCing in one of the main repos, but this one's young enough that it's a judgement call. I expect most of the code in this repo to turn over a handful more times before we call it good. :)

Includes the following changes 1e7c2e7 Update static_watcher.go d688349 Include note about embedding the ManualWatcher in the InformedWatcher 767d6a5 Few changes to the configmap package 0122abd The test logger will now log the correct caller (knative#63) e7a4b0d Dont call flag.parse in pkg/test (knative#62) b213523 Update test-infra dependency (knative#61) 382a2bf Make kube_checks generic so that it can be used in serving (knative#58) 760afb6 cleanup (knative#57) eedc0a9 Make verify-codegen.sh compatible with OS X (knative#54) 6eff182 Remove docker repo from e2e flags (knative#53) 4be5c07 Vendor the test-infra scripts (knative#52) 8c687df Update WaitForEndpointState to return response (knative#51) 8f6a3be Update knative/pkg/test (knative#50) 3ca4270 Add a logkey for the reconcile key. (knative#49) 62d2560 Add Istio DestinationRule and Policy into Istio apis and clients (knative#43) f4a77d7 Add a common test clients file (knative#46) 450739d Add common test logging module (knative#45)

Includes the following changes: 8fc80de Few changes to the configmap package (knative#59) 0122abd The test logger will now log the correct caller (knative#63) e7a4b0d Dont call flag.parse in pkg/test (knative#62) b213523 Update test-infra dependency (knative#61) 382a2bf Make kube_checks generic so that it can be used in serving (knative#58) 760afb6 cleanup (knative#57) eedc0a9 Make verify-codegen.sh compatible with OS X (knative#54) 6eff182 Remove docker repo from e2e flags (knative#53) 4be5c07 Vendor the test-infra scripts (knative#52) 8c687df Update WaitForEndpointState to return response (knative#51) 8f6a3be Update knative/pkg/test (knative#50) 3ca4270 Add a logkey for the reconcile key. (knative#49) 62d2560 Add Istio DestinationRule and Policy into Istio apis and clients (knative#43) f4a77d7 Add a common test clients file (knative#46) 450739d Add common test logging module (knative#45

* bump knative/pkg to 8fc80de Includes the following changes: 8fc80de Few changes to the configmap package (#59) 0122abd The test logger will now log the correct caller (#63) e7a4b0d Dont call flag.parse in pkg/test (#62) b213523 Update test-infra dependency (#61) 382a2bf Make kube_checks generic so that it can be used in serving (#58) 760afb6 cleanup (#57) eedc0a9 Make verify-codegen.sh compatible with OS X (#54) 6eff182 Remove docker repo from e2e flags (#53) 4be5c07 Vendor the test-infra scripts (#52) 8c687df Update WaitForEndpointState to return response (#51) 8f6a3be Update knative/pkg/test (#50) 3ca4270 Add a logkey for the reconcile key. (#49) 62d2560 Add Istio DestinationRule and Policy into Istio apis and clients (#43) f4a77d7 Add a common test clients file (#46) 450739d Add common test logging module (#45 * Use updated methods that indicate that the configmap.Watcher is using an informer

Update latest

These were originally added for the PubSub receive adapter in knative#57, but it's been a while and we probably don't need them anymore.

* Remove cobra and pflag overrides These overrides are very old, and cobra isn't imported anymore. This cleanup has already happened in serving. * Update pflag to 1.0.3 Tracking the serving bump in #2124. * Remove very old GCP PubSub-related overrides These were originally added for the PubSub receive adapter in #57, but it's been a while and we probably don't need them anymore.

* [main] Patches to make OCP 48 -> OCP 4.11 work (knative#1928) * Adding HPA patch Signed-off-by: Matthias Wessendorf <mwessend@redhat.com> * Adding SeccompProfile patch Signed-off-by: Matthias Wessendorf <mwessend@redhat.com> Signed-off-by: Matthias Wessendorf <mwessend@redhat.com> (cherry picked from commit 181f37b) * Apply HPA and SeccompProfile patch Signed-off-by: Christoph Stäbler <cstabler@redhat.com> Signed-off-by: Christoph Stäbler <cstabler@redhat.com> Co-authored-by: Matthias Wessendorf <mwessend@redhat.com>

google-prow-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels May 22, 2018

vaikas requested review from grantr and mattmoor May 22, 2018 21:07

Ville Aikas added 5 commits May 22, 2018 14:36

Add a Receive Adapter for GCP pubsub. In proc, but demonstrates concept

4ce24c9

Add ability to unbind and wire it through. Fix bug in figuring out th…

652028d

…e target of the route

Add gcp pubsub Receive Adapter and function

7562823

Wire the receive adapter in via command line flag for now (temporary …

c530270

…hack) until we move to container model

new vendor stuff

f44ce1e

vaikas force-pushed the master branch from 2f6a935 to f44ce1e Compare May 22, 2018 21:49

resolve merge conflicts

2d7e403

mchmarny reviewed May 22, 2018

View reviewed changes

mattmoor reviewed May 22, 2018

View reviewed changes

one image ref got lost in the move

6d2183a

mchmarny approved these changes May 23, 2018

View reviewed changes

address CR comments

8b8b6e9

google-prow-robot assigned mattmoor May 23, 2018

google-prow-robot added the lgtm Indicates that a PR is ready to be merged. label May 23, 2018

google-prow-robot merged commit e018ffd into knative:master May 23, 2018

matzew added a commit to matzew/eventing that referenced this pull request Apr 13, 2019

Merge pull request knative#57 from matzew/update_latest

2c267f4

Update latest

grantr added a commit to grantr/eventing that referenced this pull request Apr 23, 2019

Remove very old GCP PubSub-related overrides

172a59a

These were originally added for the PubSub receive adapter in knative#57, but it's been a while and we probably don't need them anymore.

grantr mentioned this pull request Apr 23, 2019

Remove a few obsolete overrides #1090

Merged

Conversation

vaikas commented May 22, 2018

Proposed Changes

Uh oh!

google-prow-robot commented May 22, 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!

mattmoor 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!

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!

vaikas commented May 23, 2018

Uh oh!

mattmoor commented May 23, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants