Proposal: step timeout

[imjasonh]

It could be nice to augment the Step type with a timeout duration, after which time the step fails with some unique status/message.

To support this we could pass the timeout value to the entrypoint binary, which would start a timer when step execution starts, and stop executing and raise a timeout error if execution doesn't finish in time.

If this is interesting to people it should be a pretty good first issue for a new contributor.

/kind feature

[chengjingtao]

I am watching changes: #1722. It provides an ability to retrieve information from taskstep container.
According the changes in PR, We could set PipelineResourceResult to

[
  {
     "key": "terminatedReason",
     "value" "TimeOut"
  }
]

and set StepState.Terminated.Reason="TaskStepTimeOut", So we can get TimeOut error of specific step.
I will provide a more deailed design, and begin to implement it after the PR above merged.

[bobcatfish]

Seems reasonable to me! @imjasonh do you have an example use case where you'd need this? (just wanna understand why the overall task timeout wouldn't be enough)

[imjasonh]

Task authors might want to have one step that should take just a few seconds, to set up the execution environment for instance, followed by a second step that might take a long time.

If the setup step takes longer than expected it might indicate some larger issue and you'd want to fail fast rather than exhaust the TaskRun's timeout waiting for setup.

[chengjingtao]

Hi, @imjasonh I begin to work on it . Can you assign it to me ?

My plan is

• Changes on cmd entrypoint
  • add a flag timeout to the cmd entrypoint and pass it to exec.CommandContext. If so, the entrypoint will return error when times out.
  • write fields named Reason and Message into terminationMessage as PipelineResourceResult. Reason will be TaskStepTimeout, Message will be timeout more than %s
• Changes on controller
  • append arg -timeout when changes args of step container and pass the value as step.timeout.
  • assign Reason to TaskStepTimeout when convert ContainerState to StepState and assign Message as value of that returned in terminationMessage.
  • assign Reason to TaskStepTimeout in Condition of Succeeded of TaskRunStatus

[bobcatfish]

Hey @chengjingtao sorry we didn't reply sooner! Are you still working on this? Your approach sounds solid.

Adding Reason and Message into the terminationMessage is a nice addition - it might actually be interesting to tackle this first i.e. in a separate PR - seems useful even outside the context of time outs!

[chengjingtao]

@bobcatfish yes, I will work on this and sorry for my slow action.
I will split it to two PR

• Add Reason and Message into terminationMessage #2401 The fist one: add Reason and Message into terminationMessage
• [WIP] The next on is: add timeout argument to entrypoint cmd and assign Reason= TaskStepTimeout and Message into StepState according terminationMessage in ContainerState

[Peaorl]

Hi @chengjingtao, are you still working on this issue?
I recently picked up this proposal and got the timeout working.
Is it ok if I go ahead and submit a PR for that?

[ywluogg]

I realized my change for #2559 needs to heavily based on Add timeout setting to Steps. I saw this commit was held by #3239 Are both issues' commits going to be merged soon?