Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Timeout handler : WaitPipelineRun only if we're ready to create TaskRuns #680

Closed

Conversation

vdemeester
Copy link
Member

@vdemeester vdemeester commented Mar 26, 2019

Changes

We can save one goroutines per PipelineRun by starting the wait
goroutine just before creating TaskRuns.

It's a bit related to #674, but for pipelinerun, trying to reduce potential number of goroutines whenever we can. In the case of pipelinerun, we can wait a bit before starting the wait, as Wait will take the StartTime into account (which is created really early) — that way, if something fails to validate (Pipeline reference not found, …), we won't even create a goroutine.

/cc @shashwathi correct me if I'm wrong 👼 😝

Signed-off-by: Vincent Demeester [email protected]

Submitter Checklist

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

See the contribution guide
for more details.

We can save one goroutines per PipelineRun by starting the *wait*
goroutine just before creating TaskRuns.

Signed-off-by: Vincent Demeester <[email protected]>
@tekton-robot tekton-robot requested a review from shashwathi March 26, 2019 14:23
@googlebot googlebot added the cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit label Mar 26, 2019
@tekton-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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:

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

@tekton-robot tekton-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Mar 26, 2019
@shashwathi
Copy link
Contributor

shashwathi commented Mar 26, 2019

@vdemeester : This change will create race conditions. InitializeConditions updates StartTime in status and this needs to be accessed in mutex only

We can save one goroutines per PipelineRun by starting the wait
goroutine just before creating TaskRuns.

I do not understand how is this change saving goroutine. If pipelineRun has started then no goroutine will be created

@vdemeester
Copy link
Member Author

I do not understand how is this change saving goroutine. If pipelineRun has started then no goroutine will be created

@shashwathi oh good point, I forgot about that 😅

Right, so that won't work indeed… I was trying to not start the wait goroutine as long as possible (aka up until lots of validation happens). But yeah, as is, it won't work — I'll close, back to the drawing board 👼 😄

@vdemeester vdemeester closed this Mar 26, 2019
@vdemeester vdemeester deleted the pr-timeout-mini-refactoring branch March 26, 2019 14:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants