Update taskrun/pipelinerun timeout logic to not rely on resync behavior #621

shashwathi · 2019-03-14T18:59:00Z

Changes

In this PR each new taskrun/pipelinerun starts goroutine that waits for either
stop signal, finish or timeout to occur. Once run times out handler adds
the object into respective controller queues.
When run controllers are restarted new goroutines are being created to
track existing timeouts. Mutexes added to safely update statuses.
Same timeout handler is used for pipelinerun / taskrun so mutex has
prefix "TaskRun" and "PipelineRun" to differentiate the keys.

why: As the number of taskruns and pipelineruns increase the controllers
cannot handle the number of reconciliations triggered. One of the
solutios to tackle this problems is to increase the resync period to 10h
instead of 30sec. This solution manifests a problem for
taskrun/pipelinerun timeouts because this implementation relied on the
resync behavior to update run status to "Timeout".

I drew inspiration from @tzununbekov PR in knative/build. Credit to
@pivotal-nader-ziada @dprotaso for suggesting level based reconciliation.

Fixes: #456

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.

Additional info

In previous PR I ran into e2e test error that I could not resolve. I spent couple of days and went nowhere with the debugging error. I started fresh again in this PR. @vdemeester I have taken your feedback from previous PR and addressed them. Please review again and let me know your thoughts.

I apologize for multiple PRs
3rd times the charm 🎆

googlebot · 2019-03-14T18:59:08Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

andrew-su · 2019-03-14T19:53:58Z

I signed it!

andrew-su · 2019-03-14T19:55:05Z

I'm good with having these commits contributed to this project. 👍

googlebot · 2019-03-14T21:30:57Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

bobcatfish · 2019-03-14T21:31:23Z

ive manually indicated that both contributors are good to go - i think if there are more commits we'll need to re-add this but happy to do that :)

vdemeester

Looks good to me 👍

nader-ziada · 2019-03-18T13:46:33Z

looks good to me too, @bobcatfish do you have any comments?

imjasonh

Basic direction LGTM, some style/readability nits but nothing serious.

imjasonh · 2019-03-19T15:08:50Z

cmd/controller/main.go

-	kubeconfig string
+	masterURL    string
+	kubeconfig   string
+	resyncPeriod = 10 * time.Hour


Could we make this a const instead? I don't see its value changed in tests or anything.

imjasonh · 2019-03-19T15:11:26Z

pkg/reconciler/timeout_handler.go

+	case <-finished:
+	case <-time.After(timeout):
+		t.StatusLock(tr)
+		if t.taskRuncallbackFunc == nil {


if t.taskRuncallbackFun != nil { t.taskRuncallbackFunc(tr, t.logger) }

Instead of calling a known-noop func

imjasonh · 2019-03-19T15:12:01Z

pkg/reconciler/timeout_handler.go

+	}
+}
+
+func (t *TimeoutSet) AddTrCallBackFunc(f func(interface{})) {


This name sounds like it will append a func, when in fact it replaces the func. How about SetTaskRunCallbackFunc ?

(and below for PipelineRun)

imjasonh · 2019-03-19T15:12:32Z

pkg/reconciler/timeout_handler.go

+	logger                  *zap.SugaredLogger
+	kubeclientset           kubernetes.Interface
+	pipelineclientset       clientset.Interface
+	taskRuncallbackFunc     func(interface{})


capitalization nit: taskRunCallbackFunc and pipelineRunCallbackFunc

imjasonh · 2019-03-19T15:13:38Z

pkg/reconciler/timeout_handler.go

+	}
+}
+
+func (t *TimeoutSet) AddTrCallBackFunc(f func(interface{})) {


These exported methods should have comments describing what they do.

imjasonh · 2019-03-19T15:15:48Z

pkg/reconciler/timeout_handler.go

+
+	select {
+	case <-t.stopCh:
+	case <-finished:


readability nit: empty case statements can make it hard to distinguish between "match any of these cases" and "match this case and do nothing"

I find it helpful to add a comment describing no-op cases, and/or explicitly return if you can, e.g.:

select { case <-t.stopCh: // we're stopping, give up return case <-finished: // it finished, we can stop watching return case <-time.After(timeout): ... }

imjasonh · 2019-03-19T15:16:32Z

pkg/reconciler/timeout_handler.go

+
+	defer t.Release(pr)
+
+	select {


googlebot · 2019-03-19T17:52:42Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

bobcatfish

Sorry for the delay in getting this reviewed! I left a bit of minor feedback - looks great!! Very nice work :D :D :D ❤️

bobcatfish · 2019-03-19T18:03:07Z

pkg/apis/pipeline/v1alpha1/pipelinerun_types_test.go

@@ -52,7 +52,7 @@ func TestPipelineRun_TaskRunref(t *testing.T) {
 	}

 	expectTaskRunRef := corev1.ObjectReference{
-		APIVersion: "build-tekton.dev/v1alpha1",
+		APIVersion: "tekton.dev/v1alpha1",


bobcatfish · 2019-03-19T18:04:44Z

pkg/reconciler/timeout_handler.go

+		finished = make(chan bool)
+	}
+	t.done[key] = finished
+	t.doneMut.Unlock()


so minor: curious why not defer t.doneMut.Unlock() in this function?

Behaviour remains the same either way.

ah kk, just a bit inconsistent since we're doing defer above, no big deal tho!

bobcatfish · 2019-03-19T18:09:05Z

pkg/reconciler/v1alpha1/pipelinerun/pipelinerun.go

@@ -350,6 +352,7 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1alpha1.PipelineRun) er

 	for _, rprt := range rprts {
 		if rprt != nil {
+			go c.timeoutHandler.WaitPipelineRun(pr)


i'm a bit confused, why do we need to call go WaitPipelineRun for every new taskrun created for the PipelineRun? I would assume we only needed to call go WaitPipelineRun once when the PipelineRun started. (I'm probably totally wrong, if so then maybe a comment here would help me understand!)

I would assume we only needed to call go WaitPipelineRun once when the PipelineRun started.

Yes. There is no specific place in reconciler code where it checks if its new or existing pipelinerun.

hm doesnt this mean we'll end up with more goroutines tracking pipelineruns than we need?

oh indeed, it should be just like task run below

go c.timeoutHandler.WaitPipelineRun(pr) for _, rprt := rang rprts { // […]

I updated this logic to use "hasstarted" function. WDYT @vdemeester ?

thanks @shashwathi !! ❤️

googlebot · 2019-03-19T18:13:26Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

imjasonh · 2019-03-20T14:50:04Z

LGTM, I'll let @bobcatfish give final sign-off. 🎉

bobcatfish · 2019-03-20T22:06:46Z

I'm happy to LGTM once that conflict is resolved 😅

I think that we might end up with more goroutines tracking pipelineruns than we need since we create one every time we create a taskrun for the pipelinerun (e.g. if a pipelinerun had 30 tasks we'd have 30 goroutines watching it by the end), but we can iterate on it if you want @shashwathi (or i might be misunderstanding the effect!)

googlebot · 2019-03-21T14:23:45Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

shashwathi · 2019-03-21T14:25:47Z

@bobcatfish @vdemeester : Both of you are not misunderstanding the effect. You are absolutely right that it was generating more goroutines to track pipelinerun and it would be inefficient and resource heavy when there are lot of tasks involved.

I have updated the code to check pipelinerun has started or not before creating goroutine. Resolved conflicts as well. Ready for another review.

googlebot · 2019-03-21T14:27:07Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

googlebot · 2019-03-21T14:57:46Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

googlebot · 2019-03-21T16:54:41Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

@tzununbekov

what: In this PR each new taskrun/pipelinerun starts goroutine that waits for either stop signal, finish or timeout to occur. Once run times out handler adds the object into respective controller queues. When run controllers are restarted new goroutines are being created to track existing timeouts. Mutexes added to safely update statuses. Same timeout handler is used for pipelinerun / taskrun so mutex has prefix "TaskRun" and "PipelineRun" to differentiate the keys. why: As the number of taskruns and pipelineruns increase the controllers cannot handle the number of reconciliations triggered. One of the solutios to tackle this problems is to increase the resync period to 10h instead of 30sec. This solution manifests a problem for taskrun/pipelinerun timeouts because this implementation relied on the resync behavior to update run status to "Timeout". I drew inspiration from @tzununbekov PR in knative/build. Credit to @pivotal-nader-ziada @dprotaso for suggesting level based reconciliation. Signed-off-by: Andrew Su <asu@pivotal.io> Co-authored-by: Andrew Su <asu@pivotal.io>

googlebot · 2019-03-21T17:34:40Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

googlebot · 2019-03-21T17:38:06Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

vdemeester

/lgtm

tekton-robot · 2019-03-21T17:41:44Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: shashwathi, 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~~ [vdemeester]

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

Fix resolve yaml script

tekton-robot requested a review from bobcatfish March 14, 2019 18:59

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 14, 2019

tekton-robot requested a review from dlorenc March 14, 2019 18:59

tekton-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Mar 14, 2019

googlebot added the cla: no label Mar 14, 2019

shashwathi force-pushed the new-run branch 2 times, most recently from 2b9ba16 to 12c6d92 Compare March 14, 2019 19:58

bobcatfish added cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit and removed cla: no labels Mar 14, 2019

vdemeester approved these changes Mar 15, 2019

View reviewed changes

imjasonh reviewed Mar 19, 2019

View reviewed changes

shashwathi force-pushed the new-run branch from 12c6d92 to 18af565 Compare March 19, 2019 17:52

googlebot added cla: no and removed cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit labels Mar 19, 2019

shashwathi force-pushed the new-run branch from 18af565 to d413008 Compare March 19, 2019 17:56

bobcatfish reviewed Mar 19, 2019

View reviewed changes

bobcatfish added cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit and removed cla: no labels Mar 19, 2019

shashwathi force-pushed the new-run branch from d413008 to 47f4080 Compare March 21, 2019 14:23

googlebot added cla: no and removed cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit labels Mar 21, 2019

dlorenc added cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit and removed cla: no labels Mar 21, 2019

shashwathi force-pushed the new-run branch from 47f4080 to 5582754 Compare March 21, 2019 14:57

googlebot added cla: no and removed cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit labels Mar 21, 2019

shashwathi force-pushed the new-run branch 2 times, most recently from 028d07f to 8f210ba Compare March 21, 2019 15:17

bobcatfish added cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit and removed cla: no labels Mar 21, 2019

shashwathi force-pushed the new-run branch from 8f210ba to 3f06e48 Compare March 21, 2019 17:34

googlebot added cla: no and removed cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit labels Mar 21, 2019

bobcatfish added cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit and removed cla: no labels Mar 21, 2019

vdemeester approved these changes Mar 21, 2019

View reviewed changes

tekton-robot assigned vdemeester Mar 21, 2019

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 21, 2019

tekton-robot merged commit c3910c3 into tektoncd:master Mar 21, 2019

bobcatfish mentioned this pull request Sep 9, 2020

The timeoutHandler is only instructed to wait when it creates pods #2905

Closed

pradeepitm12 pushed a commit to pradeepitm12/pipeline that referenced this pull request Mar 15, 2021

Merge pull request tektoncd#621 from piyush-garg/fix_jenkins_nightly

313072d

Fix resolve yaml script

Update taskrun/pipelinerun timeout logic to not rely on resync behavior #621

Update taskrun/pipelinerun timeout logic to not rely on resync behavior #621

Conversation

shashwathi commented Mar 14, 2019 • edited Loading

Changes

Submitter Checklist

Additional info

googlebot commented Mar 14, 2019

andrew-su commented Mar 14, 2019

andrew-su commented Mar 14, 2019

googlebot commented Mar 14, 2019

bobcatfish commented Mar 14, 2019

vdemeester left a comment

Choose a reason for hiding this comment

nader-ziada commented Mar 18, 2019

imjasonh 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

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

googlebot commented Mar 19, 2019

bobcatfish 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

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

googlebot commented Mar 19, 2019

imjasonh commented Mar 20, 2019

bobcatfish commented Mar 20, 2019

googlebot commented Mar 21, 2019

shashwathi commented Mar 21, 2019

googlebot commented Mar 21, 2019

googlebot commented Mar 21, 2019

googlebot commented Mar 21, 2019

googlebot commented Mar 21, 2019

googlebot commented Mar 21, 2019

vdemeester left a comment

Choose a reason for hiding this comment

tekton-robot commented Mar 21, 2019

shashwathi commented Mar 14, 2019 •

edited

Loading