Use knative "adapter" to simplify event listener sink #1207

mattmoor · 2021-08-29T18:45:02Z

/kind cleanup

This simplifies the event listener sink by adopting the knative eventing "adapter" library, which is like our sharedmain for controlplanes, but for the eventing dataplane components. This pattern passes config data as JSON in env:, so it also dramatically simplifies the controller logic because we no longer need to copy around empty configmaps and clean them up, and net removes a lot of code.

This also takes advantage of the new dynamic client injection logic to reduce the overhead of each adapter on the API server, and potentially dramatically improves cold start time of ksvc based sinks (they no longer need to sync informers to handle traffic). If this proves troublesome, it should be relatively straightforward to switch things back.

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
Release notes block has been filled in or deleted (only if no user facing changes)

See the contribution guide for more details.

Release Notes

Adopt knative eventing adapter for event listener sink.
Change the way event listeners are configured to drop per-namespace configmaps.

tekton-robot · 2021-08-29T18:46: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/eventlistener/eventlistener.go	69.8%	67.2%	-2.6
pkg/reconciler/eventlistener/resources/custom.go	93.9%	93.8%	-0.2

mattmoor · 2021-08-29T19:04:44Z

Ha, it looks like this is actually consumed, by a little function in pkg/logging within triggers, so:

/hold

I may try to expand this PR to drop the config-logging propagation altogether, since we can probably get away with env/flags and it will greatly simplify the mechanics of EventListener.

tekton-robot · 2021-08-29T22:23:42Z

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/eventlistener/eventlistener.go	69.8%	67.9%	-2.0
pkg/reconciler/eventlistener/resources/custom.go	93.9%	93.8%	-0.2
test/controller.go	71.1%	71.7%	0.6

tekton-robot · 2021-08-29T22:50:42Z

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/eventlistener/eventlistener.go	69.8%	67.2%	-2.6
pkg/reconciler/eventlistener/resources/custom.go	93.9%	93.8%	-0.2
test/controller.go	71.1%	71.7%	0.6

tekton-robot · 2021-08-30T02:41: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/eventlistener/eventlistener.go	69.8%	67.2%	-2.6
pkg/reconciler/eventlistener/resources/custom.go	93.9%	93.8%	-0.2
test/controller.go	71.1%	71.7%	0.6

tekton-robot · 2021-08-30T02:50:09Z

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/eventlistener/eventlistener.go	69.8%	67.2%	-2.6
pkg/reconciler/eventlistener/resources/custom.go	93.9%	93.8%	-0.2
test/controller.go	71.1%	71.7%	0.6

cmd/eventlistenersink/main.go

mattmoor · 2021-08-30T03:14:32Z

/retest

tekton-robot · 2021-08-30T03:32:44Z

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/eventlistener/eventlistener.go	69.8%	67.8%	-2.0
pkg/reconciler/eventlistener/resources/custom.go	93.9%	93.8%	-0.2
test/controller.go	71.1%	71.7%	0.6

tekton-robot · 2021-08-30T03:49:15Z

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/eventlistener/eventlistener.go	69.8%	67.8%	-2.0
pkg/reconciler/eventlistener/resources/custom.go	93.9%	93.8%	-0.2
test/controller.go	71.1%	71.7%	0.6

mattmoor · 2021-08-30T04:15:41Z

/retest

mattmoor · 2021-08-30T04:31:06Z

/retest

mattmoor · 2021-08-30T04:49:37Z

The v1alpha1/github example keeps failing, but I think there is a race because I definitely see the pods for the task here in the post-failure dump 🤔

default            pod/github-run-7hfb6-pod-lp7qm                                       0/2     Init:0/2          0          1s

Adding a sleep to see if that helps...

tekton-robot · 2021-08-31T16:54:27Z

@n3wscott: changing LGTM is restricted to collaborators

In response to this:

/LGTM

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

cmd/eventlistenersink/main.go

khrm · 2021-08-31T17:33:36Z

pkg/client/dynamic/clientset/tekton/tekton.go

@@ -25,5 +32,15 @@ func WithClient(client dynamic.Interface) clientset.Option {
 				cs.Add(r, client)
 			}
 		}
+		for version, resources := range allowedTriggersTypes {


Do we need TriggersType here? @dibyom

This is needed to pass now, since everything is going over the dynamic client now.

If everything is going over the dynamic client now, can we get rid of the typed clientsets in the Sink struct?

https://github.com/tektoncd/triggers/blob/main/pkg/sink/sink.go#L50-L51

tekton-robot · 2021-08-31T17:41:18Z

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/eventlistener/eventlistener.go	68.0%	69.5%	1.5
pkg/reconciler/eventlistener/resources/custom.go	93.9%	93.8%	-0.2

mattmoor · 2021-09-02T20:28:21Z

/hold cancel
/assign @dibyom

dibyom

Thanks @mattmoor ! The EL initialization code looks much simpler!

I'm like halfway through this PR at the moment and will try this out on my local setup.
Are there any docs around the eventing adapter library that I can use to understand what it provides better?

Also, it looks like we've added a lott of files to vendor/ Are all of them needed? e.g. I found vendor/k8s.io/client-go/applyconfigurations/networking/v1/networkpolicyport.go etc.

cmd/eventlistenersink/main.go

pkg/adapter/adapter.go

cmd/eventlistenersink/main.go

savitaashture · 2021-09-06T07:00:39Z

pkg/client/dynamic/clientset/tekton/tekton.go

+		"v1beta1":  {"pipelineruns", "taskruns", "pipelines", "clustertasks", "tasks"},
+	}
+	allowedTriggersTypes = map[string][]string{
+		"v1alpha1": {"clusterinterceptors"},


Do we also need to add "clustertriggerbindings", "eventlisteners", "triggerbindings", "triggers", "triggertemplates" here ?
/cc @dibyom

I can for completeness, but given the way this access control works, it is not needed to pass tests. You only need the exact versions through which the code accesses things.

mattmoor · 2021-09-06T13:34:34Z

Are there any docs around the eventing adapter library that I can use to understand what it provides better?

@n3wscott or @lionelvillard might have a pointer, not sure.

I found vendor/k8s.io/client-go/applyconfigurations/networking/v1/networkpolicyport.go etc

I think this is related to the K8s API adding server-side apply in the latest client update that knative/pkg pulls in.

tekton-robot · 2021-09-06T14:32:58Z

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/eventlistener/eventlistener.go	67.8%	69.2%	1.4
pkg/reconciler/eventlistener/resources/custom.go	93.9%	93.8%	-0.2

pkg/reconciler/eventlistener/resources/container.go

lionelvillard · 2021-09-07T14:35:47Z

Are there any docs around the eventing adapter library that I can use to understand what it provides better?

@n3wscott or @lionelvillard might have a pointer, not sure.

This is a good starting point: https://github.com/knative/eventing/tree/main/docs/source#receive-adapters

tekton-robot · 2021-09-07T15:49:38Z

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/eventlistener/eventlistener.go	67.8%	69.2%	1.4
pkg/reconciler/eventlistener/resources/custom.go	93.9%	93.8%	-0.2

dibyom

Looking good. Could we also get rid of the typed clients now that everything is going over the dynamic clientset?

dibyom · 2021-09-08T00:53:02Z

test/e2e-tests-examples.sh

@@ -90,12 +90,16 @@ port_forward_and_curl() {
     exit 1
  fi

+  # Wait a few seconds for resources to show up
+  sleep 3


we already wait for the EL to be ready in the check_eventlistener function. Do we also need the sleep?

This is waiting for the EL to materialize the TaskRuns and PipelineRuns before querying them below.

i was seeing intermittent flakes without this.

tekton-robot · 2021-09-08T16:59:31Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dibyom

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

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

mattmoor · 2021-09-08T17:13:23Z

Could we also get rid of the typed clients now that everything is going over the dynamic clientset?

The typed clients (and objects) are much nicer to deal with directly, this just tunnels them through the dynamic client.

dlorenc · 2021-09-08T18:29:25Z

/lgtm

Nice!

tekton-robot added release-note-none Denotes a PR that doesnt merit a release note. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. labels Aug 29, 2021

tekton-robot requested review from dibyom and savitaashture August 29, 2021 18:45

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

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

mattmoor force-pushed the drop-config-logging branch from 7346475 to b512015 Compare August 29, 2021 22:21

mattmoor changed the title ~~Drop config-logging volume~~ [WIP] Drop config-logging volume Aug 29, 2021

tekton-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. 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 Aug 29, 2021

mattmoor force-pushed the drop-config-logging branch from b512015 to 1b85679 Compare August 29, 2021 22:48

mattmoor force-pushed the drop-config-logging branch from 1b85679 to 54f7063 Compare August 30, 2021 02:39

mattmoor force-pushed the drop-config-logging branch from 54f7063 to a3ad842 Compare August 30, 2021 02:47

mattmoor commented Aug 30, 2021

View reviewed changes

cmd/eventlistenersink/main.go Outdated Show resolved Hide resolved

mattmoor force-pushed the drop-config-logging branch from a3ad842 to 1b4c9c8 Compare August 30, 2021 03:30

mattmoor force-pushed the drop-config-logging branch from 1b4c9c8 to e16cb83 Compare August 30, 2021 03:46

mattmoor force-pushed the drop-config-logging branch from e16cb83 to f093de6 Compare August 30, 2021 04:49

khrm reviewed Aug 31, 2021

View reviewed changes

Add dropped !

148edb2

tekton-robot assigned dibyom Sep 2, 2021

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

dibyom reviewed Sep 2, 2021

View reviewed changes

cmd/eventlistenersink/main.go Show resolved Hide resolved

pkg/adapter/adapter.go Show resolved Hide resolved

cmd/eventlistenersink/main.go Show resolved Hide resolved

savitaashture reviewed Sep 6, 2021

View reviewed changes

tekton-robot requested a review from dibyom September 6, 2021 07:00

Plumb through propagation of the observability configs.

6dad1ed

dibyom reviewed Sep 7, 2021

View reviewed changes

pkg/reconciler/eventlistener/resources/container.go Outdated Show resolved Hide resolved

Drop commented code

125ad71

dibyom approved these changes Sep 8, 2021

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, 2021

tekton-robot assigned dlorenc Sep 8, 2021

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

tekton-robot merged commit 4794d88 into tektoncd:main Sep 8, 2021

mattmoor deleted the drop-config-logging branch September 8, 2021 18:36

dibyom mentioned this pull request Oct 21, 2021

Find root cause for why reconciler tests were not failing #1245

Closed

dibyom mentioned this pull request Nov 29, 2021

Unregister unnecessary configMap informer #1273

Merged

4 tasks

This was referenced May 15, 2023

Drop dynamic wrapped client generation knative/pkg#2739

Closed

FYI - knative/pkg will drop use of generated k8s client backed by a dynamic client #1593

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use knative "adapter" to simplify event listener sink #1207

Use knative "adapter" to simplify event listener sink #1207

mattmoor commented Aug 29, 2021 •

edited

Loading

tekton-robot commented Aug 29, 2021

mattmoor commented Aug 29, 2021

tekton-robot commented Aug 29, 2021

tekton-robot commented Aug 29, 2021

tekton-robot commented Aug 30, 2021

tekton-robot commented Aug 30, 2021

mattmoor commented Aug 30, 2021

tekton-robot commented Aug 30, 2021

tekton-robot commented Aug 30, 2021

mattmoor commented Aug 30, 2021

mattmoor commented Aug 30, 2021

mattmoor commented Aug 30, 2021

tekton-robot commented Aug 31, 2021

khrm Aug 31, 2021

mattmoor Aug 31, 2021

dibyom Sep 7, 2021 •

edited

Loading

tekton-robot commented Aug 31, 2021

mattmoor commented Sep 2, 2021

dibyom left a comment

savitaashture Sep 6, 2021 •

edited

Loading

mattmoor Sep 6, 2021

mattmoor commented Sep 6, 2021

tekton-robot commented Sep 6, 2021

lionelvillard commented Sep 7, 2021

tekton-robot commented Sep 7, 2021

dibyom left a comment

dibyom Sep 8, 2021

mattmoor Sep 8, 2021

tekton-robot commented Sep 8, 2021

mattmoor commented Sep 8, 2021

dlorenc commented Sep 8, 2021

Use knative "adapter" to simplify event listener sink #1207

Use knative "adapter" to simplify event listener sink #1207

Conversation

mattmoor commented Aug 29, 2021 • edited Loading

Submitter Checklist

Release Notes

tekton-robot commented Aug 29, 2021

mattmoor commented Aug 29, 2021

tekton-robot commented Aug 29, 2021

tekton-robot commented Aug 29, 2021

tekton-robot commented Aug 30, 2021

tekton-robot commented Aug 30, 2021

mattmoor commented Aug 30, 2021

tekton-robot commented Aug 30, 2021

tekton-robot commented Aug 30, 2021

mattmoor commented Aug 30, 2021

mattmoor commented Aug 30, 2021

mattmoor commented Aug 30, 2021

tekton-robot commented Aug 31, 2021

khrm Aug 31, 2021

Choose a reason for hiding this comment

mattmoor Aug 31, 2021

Choose a reason for hiding this comment

dibyom Sep 7, 2021 • edited Loading

Choose a reason for hiding this comment

tekton-robot commented Aug 31, 2021

mattmoor commented Sep 2, 2021

dibyom left a comment

Choose a reason for hiding this comment

savitaashture Sep 6, 2021 • edited Loading

Choose a reason for hiding this comment

mattmoor Sep 6, 2021

Choose a reason for hiding this comment

mattmoor commented Sep 6, 2021

tekton-robot commented Sep 6, 2021

lionelvillard commented Sep 7, 2021

tekton-robot commented Sep 7, 2021

dibyom left a comment

Choose a reason for hiding this comment

dibyom Sep 8, 2021

Choose a reason for hiding this comment

mattmoor Sep 8, 2021

Choose a reason for hiding this comment

tekton-robot commented Sep 8, 2021

mattmoor commented Sep 8, 2021

dlorenc commented Sep 8, 2021

mattmoor commented Aug 29, 2021 •

edited

Loading

dibyom Sep 7, 2021 •

edited

Loading

savitaashture Sep 6, 2021 •

edited

Loading