Add kubernetes based events for triggers #1222

savitaashture · 2021-09-07T06:48:00Z

Changes

This PR adds changes to display k8s events for EventListener Pod when http request reaches to EL

kubectl get events -w | grep eventlistener

0s          Normal    dev.tekton.event.triggers.started.v1   eventlistener/github-listener              
0s          Normal    dev.tekton.event.triggers.successful.v1   eventlistener/github-listener

/cc @dibyom @khrm

Addresses #649

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

Triggers now support emitting Kubernetes events from EventListener Pod

tekton-robot · 2021-09-07T06:50:13Z

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%	68.7%	0.9
pkg/reconciler/events/event.go	Do not exist	75.0%

savitaashture · 2021-09-13T04:50:57Z

/cc @vdemeester

vaikas · 2021-09-13T10:23:21Z

FWIW, I'd caution against sending events for successful reconciliation and even for things that are non-terminal failures. This particular example emits 15 events in 78sec.

Creating events can be a problem in couple of scenarios:

Large number of EventListener objects get created in a short amount of time
Global resync, for example, when a controller restarts, or if HA is enabled, a leader switches.

In both cases there might be problems with the controller getting ratelimited due to large number of events and hilarity will ensue. We ran into this in Knative and just sharing our experience. In the end we ended up dialing back the number of events we emit because it ended up creating problems.

khrm · 2021-09-13T13:58:58Z

@savitaashture Based on #649, we should emit events when we trigger a resource successfully. This code should go inside EventListener's sink, not reconciliation.

savitaashture · 2021-11-23T13:01:03Z

@khrm @dibyom PR is ready for Review PTAL

tekton-robot · 2021-11-23T13:01:48Z

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/apis/triggers/v1alpha1/event_listener_types.go	80.6%	81.2%	0.6
pkg/apis/triggers/v1beta1/event_listener_types.go	80.6%	81.2%	0.6
pkg/reconciler/events/event.go	Do not exist	71.4%
pkg/sink/sink.go	75.8%	75.0%	-0.8

tekton-robot · 2021-11-24T05:38:48Z

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/apis/triggers/v1alpha1/event_listener_types.go	80.6%	81.2%	0.6
pkg/apis/triggers/v1beta1/event_listener_types.go	80.6%	81.2%	0.6
pkg/reconciler/events/event.go	Do not exist	71.4%
pkg/sink/sink.go	75.8%	75.0%	-0.8

vaikas · 2021-11-24T06:50:25Z

config/200-clusterrole.yaml

@@ -85,6 +85,9 @@ rules:
    resources: ["podsecuritypolicies"]
    resourceNames: ["tekton-triggers"]
    verbs: ["use"]
+  - apiGroups: [""]
+    resources: ["events"]
+    verbs: ["get", "list", "create", "update", "delete", "patch", "watch"]


Curious if here and below we need all these permissions? Seems like create would suffice because we only emit events?

Oh yes only create is required
Thanks for the heads up 👍

tekton-robot · 2021-11-25T07:06:11Z

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/apis/triggers/v1alpha1/event_listener_types.go	80.6%	81.2%	0.6
pkg/apis/triggers/v1beta1/event_listener_types.go	80.6%	81.2%	0.6
pkg/reconciler/events/event.go	Do not exist	71.4%
pkg/sink/sink.go	75.8%	75.0%	-0.8

savitaashture · 2021-11-25T07:25:36Z

/test pull-tekton-triggers-integration-tests

dibyom

Thanks @savitaashture

My main comment is around this PR and #1259 - I think we should be consistent in where and how we are emitting the event. As well as be consistent on the type of events that we are emitting (though the exact contents might be different).

config/200-clusterrole.yaml

dibyom · 2021-11-30T20:21:05Z

pkg/apis/triggers/v1alpha1/event_listener_types.go

@@ -261,6 +261,12 @@ func (els *EventListenerStatus) SetConditionsForDynamicObjects(conditions v1beta
 			Message: cond.Message,
 		})
 	}
+
+	els.SetCondition(&apis.Condition{


question: why do we need this in this PR?

Added as part of this PR because EL for Knative when succeed was not displaying any messages so added this change as part of this PR itself

pkg/reconciler/eventlistener/eventlistener.go

dibyom · 2021-11-30T20:24:07Z

pkg/reconciler/events/event.go

+)
+
+const (
+	// EventReasonSucceded is the reason set for events about successful completion of EventListener


Do we need to make sure the event reasons/types here are the same as the one in #1259?

Updated and used similar constant from #1259 PR

dibyom · 2021-11-30T20:27:24Z

pkg/sink/sink.go

+func (r Sink) emitEvents(ctx context.Context, el *triggersv1.EventListener, err error) {
+	var afterCondition *apis.Condition
+	if el != nil {
+		afterCondition = el.Status.GetCondition(apis.ConditionReady)


I don't think we should send the apis.ConditionReady as the message for the event. Maybe we can try to be consistent with the cloudevents PR here.

with el.Status.GetCondition(apis.ConditionReady) not sending message to event instead we are getting el Status

dibyom · 2021-11-30T20:34:20Z

pkg/sink/sink.go

@@ -111,6 +123,7 @@ func (r Sink) HandleEvent(response http.ResponseWriter, request *http.Request) {
 		log.Errorf("Error reading event body: %s", err)
 		r.recordCountMetrics(failTag)
 		response.WriteHeader(http.StatusInternalServerError)
+		r.emitEvents(r.ContextForEvents, el, err)


I think this should be co-ordinated with #1259 as well - can we have a single r.EmitEvents that sends both K8sEvents and CloudEvents (if enabled).

Yup i agree but i think we can do that once PRs merge

pkg/sink/sink.go

tekton-robot · 2021-12-03T10:57:13Z

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/apis/triggers/v1alpha1/event_listener_types.go	80.6%	81.2%	0.6
pkg/apis/triggers/v1beta1/event_listener_types.go	80.6%	81.2%	0.6
pkg/reconciler/events/event.go	Do not exist	75.0%
pkg/sink/sink.go	75.8%	75.2%	-0.5

tekton-robot · 2021-12-03T11:33:47Z

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/apis/triggers/v1alpha1/event_listener_types.go	80.6%	81.2%	0.6
pkg/apis/triggers/v1beta1/event_listener_types.go	80.6%	81.2%	0.6
pkg/reconciler/events/event.go	Do not exist	75.0%
pkg/sink/sink.go	75.8%	75.2%	-0.5

tekton-robot · 2021-12-03T12:41:33Z

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/apis/triggers/v1alpha1/event_listener_types.go	80.6%	81.2%	0.6
pkg/apis/triggers/v1beta1/event_listener_types.go	80.6%	81.2%	0.6
pkg/reconciler/events/event.go	Do not exist	75.0%
pkg/sink/sink.go	75.8%	75.1%	-0.7

savitaashture · 2021-12-03T12:41:34Z

pkg/sink/sink.go

@@ -91,17 +95,40 @@ type Response struct {
 	ErrorMessage string `json:"errorMessage,omitempty"`
 }

+func (r Sink) emitEvents(recorder record.EventRecorder, el *triggersv1.EventListener, err error) {
+	afterCondition := el.Status.GetCondition(apis.ConditionReady)
+	if el.Annotations[events.EnableEventListenerEvents] == "true" {


@dibyom emitting events only when user sets enable-eventlistener-events: "true" annotation
LMK your opinion on this.
If it looks okay i will update same info in Doc as well
/cc @khrm

Yeah that sounds good. This should be an option in the config-features.

@dibyom do you mean we no need to take from eventlistener annotation instead we should depend on config-features cm ?

my bad. I missed that this was an annotation. Just an annotation is fine for now as long as we document it.

dibyom · 2021-12-07T18:04:30Z

pkg/sink/sink.go

@@ -91,17 +95,40 @@ type Response struct {
 	ErrorMessage string `json:"errorMessage,omitempty"`
 }

+func (r Sink) emitEvents(recorder record.EventRecorder, el *triggersv1.EventListener, err error) {
+	afterCondition := el.Status.GetCondition(apis.ConditionReady)
+	if el.Annotations[events.EnableEventListenerEvents] == "true" {


Yeah that sounds good. This should be an option in the config-features.

dibyom · 2021-12-07T18:06:58Z

pkg/sink/sink.go

+				events.EnableEventListenerEvents: "true",
+			}}}
+
+	r.emitEvents(r.EventRecorder, &elTemp, nil)


Doesn't this mean that we will always emit an event due to elTemp always setting the annotation to true? i.e. the actual EL could have it set of false but due to elTemp defaulting to true, we are still emitting one event?

Yes because from this operation
el, err := r.EventListenerLister.EventListeners(r.EventListenerNamespace).Get(r.EventListenerName) there could be chances of error while doing Get
So added elTemp so that get operation failure captured

dibyom · 2021-12-07T18:08:41Z

pkg/reconciler/events/event.go

+	EnableEventListenerEvents = "enable-eventlistener-events"
+	// TriggerProcessingStarted is sent for Sink triggers received which
+	// are being processed
+	TriggerProcessingStartedV1 = "triggers.tekton.eventlistener.triggerprocessing.started.v1"


I'm glad that this is consistent with cloudevents but I had a comment that those names should be similar to the ones for Pipelines - see #1259 (comment)

Updated 👍

dibyom · 2021-12-07T18:20:19Z

pkg/reconciler/events/event.go

+	sendKubernetesEvents(recorder, afterCondition, object, err)
+}
+
+func sendKubernetesEvents(c record.EventRecorder, afterCondition *apis.Condition, object runtime.Object, err error) {


I'm still not sure if we need to rely on the status of the EventListener for emitting events in Triggers - if the EL is not ready, we are probably not going to receive the event at all i.e. the server is not up or something.

IMO it will be clearer emitEvents just sends the eventType (Successful, Failed etc.) as a param instead of the afterCondition.

We had a scenario with custom-resource example as part of 0.16.0 Triggers release we had an issue #1219 related to metadata.resourceVersion as part of Knative and because of that EL status was not True but pods were up and running so that is the reason i considered this way of using EL status in order to cover such scenarios also

But agree with you that the current PR focuses on events for Trigger process so we no need to consider eventlistener status instead we can send the type as you suggested

I have updated PR PTAL

tekton-robot · 2021-12-09T09:05: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/apis/triggers/v1alpha1/event_listener_types.go	80.6%	81.2%	0.6
pkg/apis/triggers/v1beta1/event_listener_types.go	80.6%	81.2%	0.6
pkg/reconciler/events/event.go	Do not exist	66.7%
pkg/sink/sink.go	75.9%	75.1%	-0.8

tekton-robot · 2021-12-09T09:18:03Z

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/apis/triggers/v1alpha1/event_listener_types.go	80.6%	81.2%	0.6
pkg/apis/triggers/v1beta1/event_listener_types.go	80.6%	81.2%	0.6
pkg/reconciler/events/event.go	Do not exist	66.7%
pkg/sink/sink.go	75.9%	75.1%	-0.8

dibyom · 2021-12-10T17:25:22Z

examples/v1alpha1/github/github-eventlistener-interceptor.yaml

@@ -3,6 +3,8 @@ apiVersion: triggers.tekton.dev/v1alpha1
 kind: EventListener
 metadata:
  name: github-listener
+  annotations:


Do we want to change this in our examples?

Its not needed but just as an example for emitting event added annotation

Reverted back for both v1alpha1 and v1beta1

pkg/reconciler/events/event.go

pkg/reconciler/events/event_test.go

tekton-robot · 2021-12-10T22:07:25Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dibyom, vdemeester

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

dibyom · 2021-12-10T22:07:38Z

Few small nits, but otherwise good to go!

tekton-robot · 2021-12-13T06:57: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/apis/triggers/v1alpha1/event_listener_types.go	80.6%	81.2%	0.6
pkg/apis/triggers/v1beta1/event_listener_types.go	80.6%	81.2%	0.6
pkg/reconciler/events/event.go	Do not exist	66.7%

tekton-robot · 2021-12-13T09:09:04Z

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/apis/triggers/v1alpha1/event_listener_types.go	80.6%	81.2%	0.6
pkg/apis/triggers/v1beta1/event_listener_types.go	80.6%	81.2%	0.6
pkg/reconciler/events/event.go	Do not exist	66.7%
pkg/sink/sink.go	75.9%	75.1%	-0.8

savitaashture · 2021-12-13T09:55:00Z

/test pull-tekton-triggers-integration-tests

savitaashture · 2021-12-13T11:03:22Z

/test pull-tekton-triggers-integration-tests

savitaashture · 2021-12-13T13:25:25Z

/test pull-tekton-triggers-integration-tests

savitaashture · 2021-12-14T08:13:30Z

@dibyom I have addressed all review comments PTAL

dibyom · 2021-12-14T15:28:47Z

/lgtm

tekton-robot requested review from dibyom and khrm September 7, 2021 06:48

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

tekton-robot requested a review from vdemeester September 13, 2021 04:50

vdemeester approved these changes Sep 13, 2021

View reviewed changes

tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 21, 2021

savitaashture force-pushed the k8s-emit-events branch from a023c6e to af49882 Compare November 23, 2021 13:00

tekton-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 23, 2021

savitaashture force-pushed the k8s-emit-events branch from af49882 to f7528ee Compare November 24, 2021 05:37

vaikas reviewed Nov 24, 2021

View reviewed changes

savitaashture force-pushed the k8s-emit-events branch from f7528ee to 66986be Compare November 25, 2021 07:04

dibyom reviewed Nov 30, 2021

View reviewed changes

savitaashture force-pushed the k8s-emit-events branch from 66986be to 829bce4 Compare December 3, 2021 10:55

savitaashture force-pushed the k8s-emit-events branch from 829bce4 to 91572be Compare December 3, 2021 11:32

savitaashture force-pushed the k8s-emit-events branch from 91572be to 8bfc4e1 Compare December 3, 2021 12:39

savitaashture commented Dec 3, 2021

View reviewed changes

dibyom reviewed Dec 7, 2021

View reviewed changes

savitaashture force-pushed the k8s-emit-events branch from 8bfc4e1 to eb8e26e Compare December 9, 2021 09:03

savitaashture force-pushed the k8s-emit-events branch from eb8e26e to 7317448 Compare December 9, 2021 09:15

dibyom approved these changes Dec 10, 2021

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 10, 2021

savitaashture force-pushed the k8s-emit-events branch from 7317448 to e57049a Compare December 13, 2021 06:55

Add kubernetes based events for triggers

89ecb1b

savitaashture force-pushed the k8s-emit-events branch from e57049a to 89ecb1b Compare December 13, 2021 09:06

tekton-robot assigned dibyom Dec 14, 2021

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

tekton-robot merged commit d9b14bf into tektoncd:main Dec 14, 2021

Add kubernetes based events for triggers #1222

Add kubernetes based events for triggers #1222

Conversation

savitaashture commented Sep 7, 2021 • edited Loading

Changes

Submitter Checklist

Release Notes

tekton-robot commented Sep 7, 2021

savitaashture commented Sep 13, 2021

vaikas commented Sep 13, 2021

khrm commented Sep 13, 2021

savitaashture commented Nov 23, 2021

tekton-robot commented Nov 23, 2021

tekton-robot commented Nov 24, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tekton-robot commented Nov 25, 2021

savitaashture commented Nov 25, 2021

dibyom 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 Dec 3, 2021 • 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

tekton-robot commented Dec 3, 2021

tekton-robot commented Dec 3, 2021

tekton-robot commented Dec 3, 2021

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

tekton-robot commented Dec 9, 2021

tekton-robot commented Dec 9, 2021

Choose a reason for hiding this comment

savitaashture Dec 13, 2021 • edited Loading

Choose a reason for hiding this comment

tekton-robot commented Dec 10, 2021

dibyom commented Dec 10, 2021

tekton-robot commented Dec 13, 2021

tekton-robot commented Dec 13, 2021

savitaashture commented Dec 13, 2021

savitaashture commented Dec 13, 2021

savitaashture commented Dec 13, 2021

savitaashture commented Dec 14, 2021

dibyom commented Dec 14, 2021

savitaashture commented Sep 7, 2021 •

edited

Loading

savitaashture Dec 3, 2021 •

edited

Loading

savitaashture Dec 13, 2021 •

edited

Loading