Migrate to using generated reconcilers #733

dibyom · 2020-08-28T18:23:26Z

Changes

Using genreconciler from knative/pkg should reduce the amount of boilerplate we
need to write a reconciler. One change I had to make to use genreconciler
is to add a Finalizer for deleting the logging config maps when an
EventListener is deleted.

Fixes #635

/kind misc

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

Includes tests (if functionality changed/added)
Includes docs (if user facing)
Commit messages follow commit message best practices

See the contribution guide for more details.

Release Notes

    Triggers reconciler now emits K8s events on reconciliation failures.

tekton-robot · 2020-08-28T18:25:43Z

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/v1alpha1/eventlistener/eventlistener.go	73.7%	74.1%	0.4

tekton-robot · 2020-08-28T19:10:59Z

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/v1alpha1/eventlistener/eventlistener.go	73.7%	75.3%	1.6

dibyom · 2020-08-28T19:28:52Z

/assign @khrm
/cc @n3wscott

tekton-robot · 2020-08-28T19:29:57Z

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/v1alpha1/eventlistener/eventlistener.go	73.7%	75.3%	1.6

dibyom · 2020-08-28T20:37:45Z

/test pull-tekton-triggers-integration-tests

dibyom · 2020-08-28T20:56:31Z

/hold

Two potential issues:

Sometimes we never set the ServiceExists status.Condition
Some logs with the following message:
{"level":"warn","logger":"controller","caller":"eventlistener/reconciler.go:190","msg":"Failed to update resource status","knative.dev/traceid":"461c4c63-7ea9-4def-86f6-13faba05254c","knative.dev/key":"arrakis-49bxn/my-eventlistener","targetMethod":"FinalizeKind","error":"resource name may not be empty"}

n3wscott · 2020-08-28T21:03:42Z

pkg/reconciler/v1alpha1/eventlistener/eventlistener.go

-
-	// Don't modify the informer's copy
-	el := original.DeepCopy()
+func (r *Reconciler) ReconcileKind(ctx context.Context, el *v1alpha1.EventListener) pkgreconciler.Event {
 	// Initial reconciliation
 	if equality.Semantic.DeepEqual(el.Status, v1alpha1.EventListenerStatus{}) {


this will never happen with the genreconciler, there is a pre-step (you can opt out) that will call InitializeConditions for you and it marks the generation after you return. It also will expose an error if you reconcile and do nothing (you need to call a mark method)

Interesting, I was following the generated stub: https://github.com/tektoncd/triggers/pull/733/files#diff-2518496f4b5a085db3dc51c69af781c2R52

Is there a newer version which calls the IntializeConditions? (This is the older version of genreconciler I think, before leader elections etc.)

fixed. calling InitializeConditions in the ReconcileKind each time now

n3wscott · 2020-08-28T21:06:47Z

pkg/reconciler/v1alpha1/eventlistener/eventlistener.go

 			},
 		},
 	}
-	existingService, err := c.KubeClientSet.CoreV1().Services(el.Namespace).Get(el.Status.Configuration.GeneratedResourceName, metav1.GetOptions{})
+	existingService, err := r.KubeClientSet.CoreV1().Services(el.Namespace).Get(el.Status.Configuration.GeneratedResourceName, metav1.GetOptions{})


this is a direct api call, you likely want to avoid doing this and use a lister and informer instead.

tekton-robot · 2020-08-31T21:28:28Z

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/v1alpha1/eventlistener/eventlistener.go	73.7%	75.5%	1.8

tekton-robot · 2020-08-31T22:05:21Z

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/v1alpha1/eventlistener/eventlistener.go	73.7%	75.5%	1.8

tekton-robot · 2020-08-31T22:20:12Z

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/v1alpha1/eventlistener/eventlistener.go	73.7%	75.5%	1.8

tekton-robot · 2020-09-01T01:57:08Z

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/v1alpha1/eventlistener/eventlistener.go	74.1%	75.9%	1.8

dibyom · 2020-09-01T14:54:31Z

/hold cancel

tekton-robot · 2020-09-01T19:59:53Z

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/v1alpha1/eventlistener/eventlistener.go	74.1%	76.0%	1.9

savitaashture

PR looks good to me 👍

I have few minor comments

savitaashture · 2020-09-02T17:29:30Z

.golangci.yml

@@ -4,6 +4,7 @@ run:
  skip-dirs:
  - vendor
  - pkg/client/clientset/(.*)/fake
+  - pkg/client/injection


Any reason to skip only injection 🤔

yes, so injection contains generated code container boilerplate for the reconciler. There were some lint errors I think that was causing the build tests to fail. It doesn't really make sense to fix generated code since it will be overwritten the next time we generate the code so I marked it as ignored from the linter.

Right i totally agree with you.
One more point is along with injection i see most of the code from pkg/client contains generated code so is that okay if we skip all like pipeline https://github.com/tektoncd/pipeline/blob/master/.golangci.yml#L33 so that it gives clear information why we are skipping

Good question: So pkg/client is mostly generated code but one folder is not: https://github.com/tektoncd/triggers/tree/master/pkg/client/dynamic/clientset

ah i see the will leave the way you did 👍

pkg/reconciler/v1alpha1/eventlistener/eventlistener.go

savitaashture · 2020-09-02T17:31:03Z

pkg/reconciler/v1alpha1/eventlistener/eventlistener.go

+	TriggersClientSet triggersclientset.Interface
+
+	// CachingClientSet allows us to instantiate Image objects
+	CachingClientSet cachingclientset.Interface


I don't see any usage of CachingClientSet can we remove this?

Good catch :) I think this is a left over from copying over from the Pipelines controller

Turns out removing this also removes some dependencies on knative/pkg/caching!

pkg/reconciler/v1alpha1/eventlistener/eventlistener.go

Using genreconciler from knative/pkg should reduce the amount of boilerplate we need to write a reconciler. * Migrates the logic for deleting logging config maps to a finalizer * Uses listers to get existing deployment, service, configMap instead of making direct API calls. Fixes tektoncd#635 Signed-off-by: Dibyo Mukherjee <dibyo@google.com>

tekton-robot · 2020-09-02T20:53:06Z

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/v1alpha1/eventlistener/eventlistener.go	74.1%	75.5%	1.3

wlynch

LGTM! One minor comment.

/lgtm

wlynch · 2020-09-02T20:58:42Z

pkg/reconciler/v1alpha1/eventlistener/controller.go

-	c.Logger.Info("Setting up event handlers")
+	impl := eventlistenerreconciler.NewImpl(ctx, c, func(impl *controller.Impl) controller.Options {
+		return controller.Options{
+			AgentName: ControllerName,


Knative docs are a bit unclear here- is this the HTTP user agent that's used? If so, should we make this a little bit more specific so we can ID specific event listeners (e.g. incl namespace/EL name?)

It is used by EventRecorder for EventSource's Component field: https://github.com/knative/eventing/blob/a7223d8e9ec421e92e13da952fe18884ab8ee749/vendor/k8s.io/api/core/v1/types.go#L5057

Yeah, this is just for the controller and not the actual HTTP agent name for the sinks.

savitaashture · 2020-09-04T08:37:17Z

/lgtm

tekton-robot · 2020-09-08T06:48:51Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: khrm

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~~ [khrm]

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

tekton-robot added the kind/misc Categorizes issue or PR as a miscellaneuous one. label Aug 28, 2020

tekton-robot requested review from iancoffey and vtereso August 28, 2020 18:23

tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 28, 2020

dibyom force-pushed the gen3 branch from 067360c to 63e2430 Compare August 28, 2020 19:09

dibyom force-pushed the gen3 branch from 63e2430 to 7351b12 Compare August 28, 2020 19:27

tekton-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 28, 2020

tekton-robot assigned khrm Aug 28, 2020

tekton-robot requested a review from n3wscott August 28, 2020 19:28

tekton-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 28, 2020

n3wscott reviewed Aug 28, 2020

View reviewed changes

dibyom force-pushed the gen3 branch from e50f652 to 198a4dc Compare August 31, 2020 22:18

dibyom force-pushed the gen3 branch from 198a4dc to 603691d Compare September 1, 2020 01:54

tekton-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 1, 2020

dibyom force-pushed the gen3 branch from 603691d to 5ecb469 Compare September 1, 2020 19:57

savitaashture reviewed Sep 2, 2020

View reviewed changes

dibyom force-pushed the gen3 branch from 5ecb469 to 713f93f Compare September 2, 2020 20:50

tekton-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Sep 2, 2020

wlynch reviewed Sep 2, 2020

View reviewed changes

tekton-robot assigned wlynch Sep 2, 2020

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 2, 2020

dibyom added kind/feature Categorizes issue or PR as related to a new feature. and removed kind/misc Categorizes issue or PR as a miscellaneuous one. labels Sep 3, 2020

tekton-robot added the kind/misc Categorizes issue or PR as a miscellaneuous one. label Sep 3, 2020

tekton-robot assigned savitaashture Sep 4, 2020

khrm approved these changes Sep 8, 2020

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 8, 2020

tekton-robot merged commit 6372f4f into tektoncd:master Sep 8, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate to using generated reconcilers #733

Migrate to using generated reconcilers #733

dibyom commented Aug 28, 2020 •

edited

Loading

tekton-robot commented Aug 28, 2020

tekton-robot commented Aug 28, 2020

dibyom commented Aug 28, 2020

tekton-robot commented Aug 28, 2020

dibyom commented Aug 28, 2020

dibyom commented Aug 28, 2020

n3wscott Aug 28, 2020

dibyom Aug 31, 2020

dibyom Sep 1, 2020

n3wscott Aug 28, 2020

dibyom Sep 1, 2020

tekton-robot commented Aug 31, 2020

tekton-robot commented Aug 31, 2020

tekton-robot commented Aug 31, 2020

tekton-robot commented Sep 1, 2020

dibyom commented Sep 1, 2020

tekton-robot commented Sep 1, 2020

savitaashture left a comment •

edited

Loading

savitaashture Sep 2, 2020

dibyom Sep 2, 2020

savitaashture Sep 3, 2020

dibyom Sep 3, 2020

savitaashture Sep 4, 2020

savitaashture Sep 2, 2020

dibyom Sep 2, 2020

dibyom Sep 2, 2020

tekton-robot commented Sep 2, 2020

wlynch left a comment

wlynch Sep 2, 2020

khrm Sep 3, 2020

dibyom Sep 3, 2020

savitaashture commented Sep 4, 2020

tekton-robot commented Sep 8, 2020

Migrate to using generated reconcilers #733

Migrate to using generated reconcilers #733

Conversation

dibyom commented Aug 28, 2020 • edited Loading

Changes

Submitter Checklist

Release Notes

tekton-robot commented Aug 28, 2020

tekton-robot commented Aug 28, 2020

dibyom commented Aug 28, 2020

tekton-robot commented Aug 28, 2020

dibyom commented Aug 28, 2020

dibyom commented Aug 28, 2020

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

tekton-robot commented Aug 31, 2020

tekton-robot commented Aug 31, 2020

tekton-robot commented Aug 31, 2020

tekton-robot commented Sep 1, 2020

dibyom commented Sep 1, 2020

tekton-robot commented Sep 1, 2020

savitaashture left a comment • edited Loading

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

tekton-robot commented Sep 2, 2020

wlynch 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

savitaashture commented Sep 4, 2020

tekton-robot commented Sep 8, 2020

dibyom commented Aug 28, 2020 •

edited

Loading

savitaashture left a comment •

edited

Loading