Creating a shared job infrastructure for pubsub management. #24

n3wscott · 2019-06-17T14:37:07Z

No description provided.

… create topics and subscriptions.

n3wscott · 2019-06-17T23:49:50Z

The channel controller only reconciles the topic. There is large improvements to the PullSubscription controller. The invoker is not implemented.

Please take a look.

n3wscott · 2019-06-18T01:23:47Z

In the morning I will remove all the channel parts of this pr so just look at the pubsub rec and jobs work

vaikas · 2019-06-18T06:05:25Z

pkg/reconciler/pubsub/reconciler.go

+	return OpsOngoingState, nil
+}
+
+func (r *PubSubBase) getJob(ctx context.Context, owner metav1.Object, ls labels.Selector) (*v1.Job, error) {


Is there a reason why we can't use a well defined name and not have to list them using label selectors. I think there might be a race here. We had this same discussion with Channels and decided to use predictable naming and that also has been what serving has been doing.

I am using generate name, there should be no race because I am watching the job resource for jobs owned by the resource under reconcile

(without looking at any code) There's still a race if work is queued and is processed before the informer cache is updated.

@vagababov just created github.com/knative/serving/pkg/resources.ChildName(parent, suffix string) for generating suffixed child resource names in a way that automagically shortens the parent name to make the total fit in 63-characters. I'd be 👍 on moving to knative/pkg.

followup: #27

evankanderson

I'm still slightly concerned with the "Job for Subscription operations" model, but I'm not going to block this PR on it.

A few other concerns -- some may be resolvable with a TODO rather than a code change in this PR.

evankanderson · 2019-06-18T17:41:06Z

pkg/apis/pubsub/v1alpha1/channel_types.go

+
+	// ChannelConditionInvokerDeployed has status True when the Channel has had
+	// its invoker deployment created.
+	ChannelConditionInvokerDeployed apis.ConditionType = "InvokerDeployed"


This condition isn't in channelCondSet. Oversight?

not finished with this api yet.

evankanderson · 2019-06-18T17:59:55Z

pkg/pubsub/invoker/invoker.go

+}
+
+func (a *Invoker) receive(ctx context.Context, event cloudevents.Event, resp *cloudevents.EventResponse) error {
+	//logger := logging.FromContext(ctx).With(zap.Any("event.id", event.ID()))


this file is WIP

evankanderson · 2019-06-18T18:00:25Z

pkg/pubsub/invoker/invoker.go

+
+	// inbound is the cloudevents client to use to receive events.
+	inbound          cloudevents.Client
+	inboundTransport *cepubsub.Transport


Where is inboundTransport set?

it is removed in a followup. It was being used to delete subscriptions but I decided it was a bad idea.

evankanderson · 2019-06-18T18:02:43Z

pkg/pubsub/invoker/invoker.go

+		event.SetID(tx.ID)
+		event.SetTime(tx.PublishTime)
+		event.SetSource(v1alpha1.PubSubEventSource(tx.Project, tx.Topic))
+		event.SetDataContentType(*cloudevents.StringOfApplicationJSON())


Is this always JSON?

Ah, WIP file, sorry. No converter on the invoker. will delete.

evankanderson · 2019-06-18T18:25:20Z

pkg/pubsub/invoker/invoker.go

+		event.SetType(v1alpha1.PubSubEventType)
+		event.SetID(uuid.New().String())
+		event.Data = msg.Data
+		// TODO: this will drop the other metadata related to the the topic and subscription names.


Do you want to add a TODO to handle attributes as extensions?

It already does in the cloudevents sdk.

evankanderson · 2019-06-18T18:28:45Z

pkg/pubsub/operations/topic.go

+limitations under the License.
+*/
+
+package operations


I think you need a short README.md in pkg/pubsub/operations/ explaining the model and usage of these jobs. (Something similar to the description I overheard in person would be helpful.)

Good idea. Will do.

evankanderson · 2019-06-18T18:34:31Z

pkg/reconciler/pubsub/reconciler.go

+
+type OpsState string
+
+const (


I'm slightly concerned about usin a state machine (vs goal-based reconciliation).

On the other hand, these don't actually look to be used as a state machine, just a regular enum. Maybe OpStatus and remove State from the names?

Sure. I will do that in a later PR.

evankanderson · 2019-06-18T18:35:08Z

pkg/reconciler/pubsub/reconciler.go

+	OpsGetFailedState         OpsState = "JOB_GET_FAILED"
+	OpsCreatedState           OpsState = "JOB_CREATED"
+	OpsCreateFailedState      OpsState = "JOB_CREATE_FAILED"
+	OpsCompeteSuccessfulState OpsState = "JOB_SUCCESSFUL"


trying to save on Ls

evankanderson · 2019-06-18T18:43:38Z

pkg/reconciler/pullsubscription/pullsubscription.go

+			source.Status.SubscriptionID = ""
+			removeFinalizer(source)
+
+		case pubsub.OpsCreateFailedState, pubsub.OpsCompeteFailedState:


This leaves the finalizer hanging around -- should we give up on the finalizer at some point (for example, if we don't have permissions to delete the subscription, or if it is already deleted)?

I seem to recall that the GitHub source had issues where it would stick around forever if (for example) you created a source but it hadn't had a chance to register the webhook yet.

you can edit the resource and remove the finalizer.

evankanderson · 2019-06-18T18:46:57Z

pkg/reconciler/pullsubscription/pullsubscription.go

+		addFinalizer(source)
+		source.Status.MarkSubscriptionOperation("Creating",
+			"Created Job to create Subscription %q.",
+			source.Status.SubscriptionID)


Is there a possible race here where the SubscriptionID gets stored in status, the subscribe job gets created, and then the resource is deleted, and a delete job is created while the create job is still running?

At which point, does the job lookup in getJob work correctly, or does it return a random job?

oh interesting. and uug... I will make a bug for this one.

evankanderson

/lgtm
/approve

Thanks for including a "why" on the use of Jobs in the README.md

knative-prow-robot · 2019-06-18T20:06:01Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: evankanderson, n3wscott

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~~ [evankanderson,n3wscott]

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

evankanderson

/lgtm

knative-metrics-robot · 2019-06-18T20:11:20Z

The following is the coverage report on pkg/.
Say /test pull-googlecloudplatform-cloud-run-events-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pubsub/v1alpha1/channel_lifecycle.go	100.0%	83.3%	-16.7
pkg/reconciler/pullsubscription/controller.go	93.3%	93.8%	0.4
pkg/reconciler/pullsubscription/pullsubscription.go	43.9%	71.2%	27.3

akashrv · 2019-06-19T00:40:42Z

pkg/reconciler/pullsubscription/pullsubscription.go

@@ -172,19 +206,40 @@ func (c *Reconciler) reconcile(ctx context.Context, source *v1alpha1.PullSubscri
 		source.Status.MarkSink(sinkURI)


MarkTransformer? Same on line#203?

Scott Nichols added 2 commits June 17, 2019 07:32

adding start of controller.

e3402cd

Merge branch 'master' into add-controller-channel

3bdd39d

knative-prow-robot added the do-not-merge/work-in-progress label Jun 17, 2019

knative-prow-robot requested review from evankanderson and vaikas June 17, 2019 14:37

knative-prow-robot added approved size/XXL labels Jun 17, 2019

Scott Nichols added 5 commits June 17, 2019 08:23

making common job ops.

0448a09

working in a common pubsub reconcile layer to help manage the jobs to…

e82ced0

… create topics and subscriptions.

cleaned up job management.:

51d7f88

more cleanup around events.

0578002

update testing for channel reconciler.

2e2f819

n3wscott marked this pull request as ready for review June 17, 2019 23:49

knative-prow-robot removed the do-not-merge/work-in-progress label Jun 17, 2019

vaikas reviewed Jun 18, 2019

View reviewed changes

revert add of controller.

008776d

n3wscott changed the title ~~Add Channel controller~~ Creating a shared job infrastructure for pubsub management. Jun 18, 2019

This was referenced Jun 18, 2019

Adding Channel controller. #26

Merged

Jobs: Use well defined name and not list them using label selectors. #27

Closed

evankanderson reviewed Jun 18, 2019

View reviewed changes

Scott Nichols added 2 commits June 18, 2019 12:52

adding a readme for operations.

b796d2a

adding why.

ff142bf

evankanderson reviewed Jun 18, 2019

View reviewed changes

knative-prow-robot assigned evankanderson Jun 18, 2019

knative-prow-robot added the lgtm label Jun 18, 2019

remove link.

172c6a1

knative-prow-robot removed the lgtm label Jun 18, 2019

evankanderson reviewed Jun 18, 2019

View reviewed changes

knative-prow-robot added the lgtm label Jun 18, 2019

knative-prow-robot merged commit 3aea7f7 into google:master Jun 18, 2019

akashrv reviewed Jun 19, 2019

View reviewed changes

		@@ -172,19 +206,40 @@ func (c Reconciler) reconcile(ctx context.Context, source v1alpha1.PullSubscri
		source.Status.MarkSink(sinkURI)


		type OpsState string

		const (

Creating a shared job infrastructure for pubsub management. #24

Creating a shared job infrastructure for pubsub management. #24

Conversation

n3wscott commented Jun 17, 2019

n3wscott commented Jun 17, 2019 • edited

n3wscott commented Jun 18, 2019

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

evankanderson left a 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

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

evankanderson left a comment

Choose a reason for hiding this comment

knative-prow-robot commented Jun 18, 2019

evankanderson left a comment

Choose a reason for hiding this comment

knative-metrics-robot commented Jun 18, 2019

Choose a reason for hiding this comment

n3wscott commented Jun 17, 2019 •

edited