Add Timeout for EventListener Server #747

khrm · 2020-09-08T11:11:24Z

Added Timeout to avoid running out of file descriptor and leaking connections.

Changes

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

Add Read, Write, and Idle timeout for connections to the EventListener.

khrm · 2020-09-08T11:13:11Z

This pr might potentially resolve #687

khrm · 2020-09-08T16:52:57Z

/retest

tekton-robot · 2020-09-08T18:08: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/v1alpha1/eventlistener/eventlistener.go	75.5%	76.7%	1.2

khrm · 2020-09-08T18:42:52Z

/test pull-tekton-triggers-integration-tests

khrm · 2020-09-08T19:05:22Z

/test pull-tekton-triggers-integration-tests

tekton-robot · 2020-09-08T19:19:00Z

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	75.5%	76.7%	1.2

tekton-robot · 2020-09-08T19:43:14Z

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	75.5%	76.7%	1.2

dibyom · 2020-09-09T19:04:34Z

cmd/eventlistenersink/main.go

+
+	srv := &http.Server{
+		Addr:         fmt.Sprintf(":%s", sinkArgs.Port),
+		ReadTimeout:  sinkArgs.ELReadTimeOut * time.Second,


ReadTimeout includes reading both headers + body. Maybe we should use ReadHeaderTimeout instead?

Or may we set both? What would be the sane values for all these?

5 seconds sounds reasonable to me.

cmd/eventlistenersink/main.go

tekton-robot · 2020-09-10T06:29:35Z

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	75.5%	76.7%	1.2

dibyom · 2020-09-10T21:09:32Z

@khrm How do you feel about adding just a IdleTimeout for the v0.8.1 release. We can then add the additional timeouts + config options to change it for v0.9?

I opened #755 with just that subset of changes. Let me know what you think!

khrm · 2020-09-11T05:43:42Z

@dibyom Yea, that's good.

tekton-robot · 2020-09-17T07:33:24Z

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	76.2%	77.4%	1.2

tekton-robot · 2020-09-21T06:12:54Z

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	76.2%	77.4%	1.2

tekton-robot · 2020-09-21T06:29: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/v1alpha1/eventlistener/eventlistener.go	76.2%	77.4%	1.2

tekton-robot · 2020-09-21T06:44:54Z

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	76.2%	77.4%	1.2

dibyom · 2020-09-21T21:19:01Z

cmd/eventlistenersink/main.go

+		WriteTimeout: sinkArgs.ELWriteTimeOut * time.Second,
+		IdleTimeout:  sinkArgs.ELIdleTimeOut * time.Second,
+		Handler: http.TimeoutHandler(mux,
+			sinkArgs.ELTimeOutHandler*time.Second, "EventListener Timeout!\n"),


is there a way to add some extra info here e.g. the eventID or something?

No, unless we write our own custom thing.

dibyom · 2020-10-06T15:45:27Z

Summarizing discussion from last week's WG:

There are a lot of controls, which give users a lot of power...but we'd need docs to explain how these timeouts work with each other and when to use what.
As an alternative, maybe we can have a single timeout for the entire request processing time that the user can control (e.g. 30s) ? We'd use a context and cancel the context after this deadline. That might make this easier to explain/use.
At any rate, we should keep the IdleTimeout at least so that we are not leaking connections.

Added Timeout to avoid running out of file descriptor and leaking connections. These timeouts are configurable via config.

khrm · 2020-11-17T17:12:22Z

@dibyom @wlynch Can we merge this pr? I have added relevant doc in eventlistener.md

dibyom · 2020-11-17T20:42:49Z

/approve
/lgtm

(We should still consider adding a context based timeout to cancel trigger processing that takes more than x seconds)

tekton-robot · 2020-11-17T20:42:53Z

[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

tekton-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Sep 8, 2020

tekton-robot requested review from iancoffey and vtereso September 8, 2020 11:11

tekton-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Sep 8, 2020

khrm added the kind/feature Categorizes issue or PR as related to a new feature. label Sep 8, 2020

tekton-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Sep 8, 2020

khrm force-pushed the elServTimeout branch 2 times, most recently from 21c8471 to fe25c38 Compare September 8, 2020 14:57

tekton-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Sep 8, 2020

khrm force-pushed the elServTimeout branch from fe25c38 to 3977753 Compare September 8, 2020 16:06

dibyom added this to the Triggers v0.9 milestone Sep 8, 2020

khrm force-pushed the elServTimeout branch from 3977753 to 60bda57 Compare September 8, 2020 18:05

khrm force-pushed the elServTimeout branch from 60bda57 to acf33d3 Compare September 8, 2020 19:16

khrm force-pushed the elServTimeout branch from acf33d3 to 1149a3a Compare September 8, 2020 19:41

dibyom mentioned this pull request Sep 9, 2020

Event listener does not accept events after some time #687

Closed

dibyom reviewed Sep 9, 2020

View reviewed changes

khrm force-pushed the elServTimeout branch from 1149a3a to 69eda8d Compare September 10, 2020 06:27

tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 12, 2020

tekton-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 17, 2020

khrm force-pushed the elServTimeout branch from bc7ea8a to 8b22f56 Compare September 21, 2020 06:11

khrm force-pushed the elServTimeout branch from 8b22f56 to 15f70ea Compare September 21, 2020 06:28

khrm changed the title ~~WIP: Add Timeout for EventListener Server~~ Add Timeout for EventListener Server Sep 21, 2020

tekton-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 21, 2020

khrm force-pushed the elServTimeout branch from 15f70ea to e78af75 Compare September 21, 2020 06:43

dibyom reviewed Sep 28, 2020

View reviewed changes

dibyom modified the milestones: Triggers v0.9, Triggers v0.10 Oct 6, 2020

dibyom mentioned this pull request Nov 11, 2020

EventListeners stops receiving events randomly #824

Closed

dibyom linked an issue Nov 16, 2020 that may be closed by this pull request

EventListeners stops receiving events randomly #824

Closed

Add Timeout via Config for EventListener Server

e8cd876

Added Timeout to avoid running out of file descriptor and leaking connections. These timeouts are configurable via config.

khrm force-pushed the elServTimeout branch from e78af75 to e8cd876 Compare November 17, 2020 17:02

tekton-robot assigned dibyom Nov 17, 2020

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

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

tekton-robot merged commit 6657c79 into tektoncd:master Nov 17, 2020

khrm deleted the elServTimeout branch July 28, 2021 14:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Timeout for EventListener Server #747

Add Timeout for EventListener Server #747

khrm commented Sep 8, 2020 •

edited

Loading

khrm commented Sep 8, 2020 •

edited

Loading

khrm commented Sep 8, 2020

tekton-robot commented Sep 8, 2020

khrm commented Sep 8, 2020

khrm commented Sep 8, 2020

tekton-robot commented Sep 8, 2020

tekton-robot commented Sep 8, 2020

dibyom Sep 9, 2020

khrm Sep 10, 2020

dibyom Nov 17, 2020

tekton-robot commented Sep 10, 2020

dibyom commented Sep 10, 2020 •

edited

Loading

khrm commented Sep 11, 2020

tekton-robot commented Sep 17, 2020

tekton-robot commented Sep 21, 2020

tekton-robot commented Sep 21, 2020

tekton-robot commented Sep 21, 2020

dibyom Sep 21, 2020

khrm Sep 30, 2020

dibyom commented Oct 6, 2020

khrm commented Nov 17, 2020

dibyom commented Nov 17, 2020

tekton-robot commented Nov 17, 2020

Add Timeout for EventListener Server #747

Add Timeout for EventListener Server #747

Conversation

khrm commented Sep 8, 2020 • edited Loading

Changes

Submitter Checklist

Release Notes

khrm commented Sep 8, 2020 • edited Loading

khrm commented Sep 8, 2020

tekton-robot commented Sep 8, 2020

khrm commented Sep 8, 2020

khrm commented Sep 8, 2020

tekton-robot commented Sep 8, 2020

tekton-robot commented Sep 8, 2020

dibyom Sep 9, 2020

Choose a reason for hiding this comment

khrm Sep 10, 2020

Choose a reason for hiding this comment

dibyom Nov 17, 2020

Choose a reason for hiding this comment

tekton-robot commented Sep 10, 2020

dibyom commented Sep 10, 2020 • edited Loading

khrm commented Sep 11, 2020

tekton-robot commented Sep 17, 2020

tekton-robot commented Sep 21, 2020

tekton-robot commented Sep 21, 2020

tekton-robot commented Sep 21, 2020

dibyom Sep 21, 2020

Choose a reason for hiding this comment

khrm Sep 30, 2020

Choose a reason for hiding this comment

dibyom commented Oct 6, 2020

khrm commented Nov 17, 2020

dibyom commented Nov 17, 2020

tekton-robot commented Nov 17, 2020

khrm commented Sep 8, 2020 •

edited

Loading

khrm commented Sep 8, 2020 •

edited

Loading

dibyom commented Sep 10, 2020 •

edited

Loading