Some timeout refactoring #3011
Some timeout refactoring #3011
Conversation
While investigating tektoncd#2905, I struggled to understand how the timeout handling works, especially with TimeoutSet having very little comments, so I've added some. I didn't add anything for backoffs yet because I'm hoping we can separate that into a separate structure since it has a very specific purpose that doesn't generalize to all timeouts. Also changed the name "finished" to consistently use "done" so the reader doesn't have to wonder about the difference between "finished" and "done" (there isn't one)
tekton-robot
added
the
release-note-none
tekton-robot
added
the
size/L
bobcatfish
added
the
kind/cleanup
|
The following is the coverage report on the affected files.
|
I'd like to move the "backoff" logic into its own file, separate from the other timeout logic, so it's clear which parts apply to what (i.e. the timeout handler is being used for 2 purposes: timing out Runs which take too long, and backing off when pod creation is failing - this is totally fine but it's hard to understand when reading the code) As a first step, I've moved the timeout handler into a separate package, so we can have a file and tests dedicated to the backoff logic separate from the other handling.
bobcatfish
force-pushed
the
some_timeout_refactoring
branch
from
2c4b0c4
to
7dfa315
Compare
|
The following is the coverage report on the affected files.
|
| @@ -124,7 +124,7 @@ var ( | |||
| _ pipelinerunreconciler.Interface = (*Reconciler)(nil) | |||
| ) | |||
|
|
|||
| // Reconcile compares the actual state with the desired, and attempts to | |||
| // ReconcileKind compares the actual state with the desired, and attempts to | |||
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dlorenc 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 |
tekton-robot
added
the
approved
| // This is usually set to the function that enqueues the taskRun for reconciling. | ||
| taskRunCallbackFunc func(interface{}) | ||
| // pipelineRunCallbackFunc is the function to call when a TaskRun has timed out | ||
| // This is usually set to the function that enqueues the taskRun for reconciling. |
There was a problem hiding this comment.
NIT:
s/when a TaskRun/when a PipelineRun/
s/enqueues the taskRun/enqueues the pipelineRun/
There was a problem hiding this comment.
whoops, thanks @pritidesai !
| @@ -274,7 +286,7 @@ func (t *TimeoutSet) waitRun(runObj StatusKey, timeout time.Duration, startTime | |||
| // the lifetime of the TaskRun no resources are released after the timer | |||
| // fires. It is the caller's responsibility to Release() the TaskRun when | |||
| // work with it has completed. | |||
| func (t *TimeoutSet) SetTaskRunTimer(tr *v1beta1.TaskRun, d time.Duration) { | |||
| func (t *Handler) SetTaskRunTimer(tr *v1beta1.TaskRun, d time.Duration) { | |||
There was a problem hiding this comment.
Random thought, SetTaskRunTimer but no SetPipelineRunTimer 🤔
| @@ -186,7 +198,7 @@ func (t *TimeoutSet) checkPipelineRunTimeouts(namespace string, pipelineclientse | |||
|
|
|||
| // CheckTimeouts function iterates through a given namespace or all namespaces | |||
| // (if empty string) and calls corresponding taskrun/pipelinerun timeout functions | |||
| func (t *TimeoutSet) CheckTimeouts(namespace string, kubeclientset kubernetes.Interface, pipelineclientset clientset.Interface) { | |||
| func (t *Handler) CheckTimeouts(namespace string, kubeclientset kubernetes.Interface, pipelineclientset clientset.Interface) { | |||
There was a problem hiding this comment.
its interesting that we are scrapping through all possible namespaces (if not specified) and checking all TaskRuns and PipelineRuns in those namespaces or at least in one specified namespace 😲
There was a problem hiding this comment.
And the same check CheckTimeouts is done in TaskRun controller and PipelineRun controller 🤔
There was a problem hiding this comment.
yeah!! the second means we're probably doing this twice as much as we need to 😅
and it sounds like we probably don't need to be doing it at all!! #2905 (comment)
|
@bobcatfish I learnt some timeout handler with this PR 😜 and excited to see more changes ... one minor NIT which can be addressed with next set of changes /lgtm |
Changes
Add more details about how the timeout handling works 🕒
While investigating #2905, I struggled to understand how the timeout
handling works, especially with TimeoutSet having very little comments,
so I've added some. I didn't add anything for backoffs yet because I'm
hoping we can separate that into a separate structure since it has a
very specific purpose that doesn't generalize to all timeouts.
Also changed the name "finished" to consistently use "done" so the
reader doesn't have to wonder about the difference between "finished"
and "done" (there isn't one)
Move timeout handler into its own package 📦
I'd like to move the "backoff" logic into its own file, separate from
the other timeout logic, so it's clear which parts apply to what (i.e.
the timeout handler is being used for 2 purposes: timing out Runs which
take too long, and backing off when pod creation is failing - this is
totally fine but it's hard to understand when reading the code)
As a first step, I've moved the timeout handler into a separate package,
so we can have a file and tests dedicated to the backoff logic separate
from the other handling.
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.
Double check this list of stuff that's easy to miss:
cmddir, please updatethe release Task to build and release this image.
Reviewer Notes
If API changes are included, additive changes must be approved by at least two OWNERS and backwards incompatible changes must be approved by more than 50% of the OWNERS, and they must first be added in a backwards compatible way.
Release Notes