[WIP] Convert Flow controller to provider/reconcile model by vaikas · Pull Request #287 · knative/eventing

vaikas · 2018-07-25T18:23:58Z

Fixes #

Proposed Changes

change Flow controller to be like Feed controller.

google-prow-robot · 2018-07-25T18:24:03Z

[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

grantr

Love to see this @vaikas-google! I'm about halfway through, but have enough comments to submit early.

grantr · 2018-07-26T18:41:17Z

+	// MessageResourceSynced is the message used for an Event fired when a Flow
+	// is synced successfully
+	MessageResourceSynced = "Flow synced successfully"
+)


These constants don't seem to be used anymore, can they be removed?

grantr · 2018-07-26T18:42:42Z

+
+// Reconcile compares the actual state with the desired, and attempts to
+// converge the two. It then updates the Status block of the Flow resource
+// with the current status of the resource.


This is a better comment than feed.Reconcile() has, I'll steal it 😄

grantr · 2018-07-26T18:44:13Z

+// converge the two. It then updates the Status block of the Flow resource
+// with the current status of the resource.
+func (r *reconciler) Reconcile(request reconcile.Request) (reconcile.Result, error) {
+	glog.Infof("Reconciling flow %v", request)


Is this for debugging?

grantr · 2018-07-26T19:00:42Z

+		// If we didn't change anything then don't call updateStatus.
+		// This is important because the copy we loaded from the informer's
+		// cache may be stale and we don't want to overwrite a prior update
+		// to status with this stale state.


This update handling is different from the feed controller and I'm curious why. Did you run into issues, or is there a bug in the feed controller's update handling?

The feed controller does this:

Get the feed from the cache (feed A)

Reconcile feed A

Get the feed from the cache (feed B)

If the reconciled feed A status is equal to the just-retrieved feed B status, stop and do not update.

Copy feed A status into feed B.

Update feed B. This update will fail if the feed has been updated by something else since feed B was retrieved (because the apiserver will reject the feed B resourceVersion).

The flow controller does this:

Get the flow from the cache (flow A)

Copy flow A to original

Reconcile flow A

If the reconciled flow A status is equal to original status, stop and do not update.

Get the flow from the cache (flow B)

Copy flow A status into flow B

Update flow B. This update will fail if the feed has been updated by something else since feed B was retrieved (because the apiserver will reject the feed B resourceVersion).

grantr · 2018-07-26T19:04:03Z

+	}
+
+	// Requeue if the resource is not ready:
+	return reconcile.Result{Requeue: !flow.Status.IsReady()}, err


Instead of relying on requeue to progress to ready, the flow controller should watch the resources that, when changed, indicate progress toward readiness. My guess is that by watching Feeds, Channels, and Subscriptions, this Requeue will be unnecessary.

The feed controller does this by watching Jobs:

eventing/pkg/controller/feed/provider.go

Lines 58 to 62 in bc26d36

// Watch Jobs and enqueue owning Feed key.

if err := c.Watch(&source.Kind{Type: &batchv1.Job{}},

&handler.EnqueueRequestForOwner{OwnerType: &feedsv1alpha1.Feed{}, IsController: true}); err != nil {

return nil, err

}

FYI I think this is the only true blocker to LGTM. Everything else is minor and can be cleaned up later.

grantr · 2018-07-26T19:10:19Z

+
+// syncHandler compares the actual state with the desired, and attempts to
+// converge the two. It then updates the Status block of the Flow resource
+// with the current status of the resource.


This comment is about a different function 😄

grantr · 2018-07-26T19:10:29Z

+// converge the two. It then updates the Status block of the Flow resource
+// with the current status of the resource.
+func (r *reconciler) resolveActionTarget(namespace string, action v1alpha1.FlowAction) (string, error) {
+	glog.Infof("Resolving target: %+v", action)


Debug line?

grantr · 2018-07-26T19:11:16Z

+	return "", fmt.Errorf("No resolvable action target: %+v", action)
+}
+
+// resolveObjectReference fetches an object based on ObjectRefence. It assumes the


typo nit: ObjectReference

grantr · 2018-07-26T19:12:21Z

+		glog.Warningf("failed to get object: %v", err)
+		return "", err
+	}
+	status, ok := obj.Object["status"]


grantr · 2018-07-26T19:13:24Z

+	channel := &channelsv1alpha1.Channel{}
+	err := r.client.Get(context.TODO(), client.ObjectKey{Namespace: flow.Namespace, Name: channelName}, channel)
+	if errors.IsNotFound(err) {
+		channel, err = r.createChannel(flow)


For a future PR: might be useful to emit an event here

As in that we create one? I'd expect that Channel would send an event?

Oh I mean corev1.Event. Sorry for the ambiguity 😄

grantr

Here's the rest

grantr · 2018-07-26T20:06:58Z

+	glog.Infof("Resolved Target to: %q", target)
+
+	// Reconcile the Channel. Creates a channel that is the target that the Feed will use.
+	// TODO: We should reuse channels possibly.


Looks like the current code already reuses a channel with the same name. Is this TODO still needed?

sorry, it could re-use a channel (not by name but type or something like that) and only create a subscription. Right now it creates 1:1 (channel:subscription). Tried to clarify the comment.

grantr · 2018-07-26T20:07:20Z

+		return nil, err
+	}
+
+	// Should make sure channel is what it should be. For now, just assume it's fine


Might be clearer to add a TODO here.

grantr · 2018-07-26T20:08:11Z

+
+	// Should make sure channel is what it should be. For now, just assume it's fine
+	// if it exists.
+	return channel, err


Seems like err can only be nil here, so it might be clearer to return channel, nil.

grantr · 2018-07-26T20:12:05Z

+
+	// Should make sure subscription is what it should be. For now, just assume it's fine
+	// if it exists.
+	return subscription, err


Seems like err can only be nil here, so it might be clearer to return channel, nil.

grantr · 2018-07-26T20:12:19Z

+		return nil, err
+	}
+
+	// Should make sure subscription is what it should be. For now, just assume it's fine


Might be clearer to add a TODO here.

grantr · 2018-07-26T20:26:56Z

+	}
+
+	glog.Infof("Reconciled feed: %+v", feed)
+	// Should make sure feed is what it should be. For now, just assume it's fine


Might be clearer to add a TODO here.

grantr · 2018-07-26T20:27:05Z

+	glog.Infof("Reconciled feed: %+v", feed)
+	// Should make sure feed is what it should be. For now, just assume it's fine
+	// if it exists.
+	return feed, err


Seems like err can only be nil here, so it might be clearer to return channel, nil.

vaikas · 2018-07-27T02:12:30Z

Thanks for the review @grantr! Addressed all the other comments except one that I want to think about some more. PTAL.

knative-metrics-robot · 2018-07-27T02:14:00Z

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

File	Old Coverage	New Coverage	Delta
pkg/controller/flow/reconcile.go	Do not exist	45.2%

grantr

/lgtm

Backporting patch from 071 t0 08x line

vaikas added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 25, 2018

google-prow-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 25, 2018

google-prow-robot requested review from mattmoor and ultrasaurus July 25, 2018 18:24

first pass, convert to provider/reconcile model

1192e0f

vaikas force-pushed the flow-tests branch from 8cbee6c to 1192e0f Compare July 25, 2018 18:29

remove old cruft, dump feed on failure

188e6e5

google-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 Jul 25, 2018

Ville Aikas added 6 commits July 25, 2018 19:55

Add more unit tests

dd1ee03

dump controller logs in failures

f2e3132

moar logs

321b43e

add first unit test, more debug logs in e2e

a22bac6

requeue if flow is not ready

73c19f2

add a unit test

5e82d65

grantr reviewed Jul 26, 2018

View reviewed changes

Ville Aikas added 3 commits July 27, 2018 01:45

watch for channel,subscription and feed and reconcile if they change

98b56fe

fix unit tests

949cea2

address review comments

c82b13d

vaikas removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 27, 2018

grantr approved these changes Jul 27, 2018

View reviewed changes

google-prow-robot assigned grantr Jul 27, 2018

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

google-prow-robot merged commit 904f403 into knative:master Jul 27, 2018

vaikas deleted the flow-tests branch July 27, 2018 16:33

grantr mentioned this pull request Aug 21, 2018

Add grantr to list of approvers #387

Merged

matzew pushed a commit to matzew/eventing that referenced this pull request Oct 11, 2019

Merge pull request knative#287 from matzew/SRVKE-160

081cd5b

Backporting patch from 071 t0 08x line

	// Watch Jobs and enqueue owning Feed key.
	if err := c.Watch(&source.Kind{Type: &batchv1.Job{}},
	&handler.EnqueueRequestForOwner{OwnerType: &feedsv1alpha1.Feed{}, IsController: true}); err != nil {
	return nil, err
	}

Conversation

vaikas commented Jul 25, 2018

Proposed Changes

Uh oh!

google-prow-robot commented Jul 25, 2018

Uh oh!

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

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

vaikas commented Jul 27, 2018

Uh oh!

knative-metrics-robot commented Jul 27, 2018

Uh oh!

grantr left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers