Move timeout, affinity, nodeSelector to taskrun and pipelinerun

[nader-ziada]

Proposed changes

• move timeout, affinity, nodeselector from task to taskrun
• move timeout, affinity, nodeselector from pipeline to pipelinerun

These are runtime parameters that are more concerned with the actual
running of a task more than the definition

/cc @bobcatfish @shashwathi

[bobcatfish]

o no my docs

[nader-ziada]

o no my docs

This will rebase on your docs once its merged in and I'll update the docs accordingly

[abayer]

I'm not convinced that Timeout should be on PipelineRun and TaskRun rather than Pipeline and Task - for example, this would mean that there's no way to have a single TaskRun created by a PipelineRun to have its own Timeout. In Jenkins Pipeline, there's heavy usage of timeouts for specific stages (i.e., Task equivalents) and whole pipelines as part of authoring the pipeline, and I think we really need the same behavior here.

[nader-ziada]

@abayer this is a valid point, I was just proposing this change because it doesn't seem like there is agreement on where timeout should live.
I thought about having timeout in the run because the task and pipeline are just like templates for what steps are needed and everything else about the actual running of these steps comes from the run, for example the service account and values of params and so on

[abayer]

@pivotal-nader-ziada - I guess I think of timeout as part of the "template" in that case - though being able to override the timeout in a Task or Pipeline via TaskRun/PipelineRun sounds like a good idea to me.

[nader-ziada]

@abayer can you give an example of how the timeout feature is used in jenkins that would not work in this model?

[shashwathi]

/lgtm

I have left some minor comment about simplifying timeout logic in pipelinerun controller. Rest looks great @pivotal-nader-ziada .

[shashwathi]

Can we simplify the timeout logic? I agree that this change was not in this PR.

taskRunTimeout = &metav1.Duration{Duration: 0 * time.Second}
if pr.Spec.Timeout != nil {
    pTimeoutTime := pr.Status.StartTime.Add(pr.Spec.Timeout.Duration)
              // set taskrun timeout only if its valid timeout
          if !time.Now().After(pTimeoutTime) {
            taskRunTimeout = pr.Spec.Timeout
        }
    } else {
     // set timeout as nil if PR timeout is not set
        taskRunTimeout = nil
    }

[knative-prow-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pivotal-nader-ziada, shashwathi

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 [pivotal-nader-ziada,shashwathi]

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

[shashwathi]

/hold

[shashwathi]

I am adding hold to merge @bobcatfish docs PR first and address @abayer comments. This PR is good from my end

[shashwathi]

/hold cancel

[knative-metrics-robot]

The following is the coverage report on pkg/.
Say /test pull-knative-build-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1alpha1/pipeline_validation.go	92.2%	91.7%	-0.5
pkg/apis/pipeline/v1alpha1/pipelinerun_validation.go	92.3%	93.8%	1.4
pkg/reconciler/v1alpha1/pipelinerun/pipelinerun.go	84.8%	84.1%	-0.8
pkg/reconciler/v1alpha1/taskrun/taskrun.go	74.0%	74.2%	0.3
test/builder/pipeline.go	95.0%	90.5%	-4.5
test/builder/task.go	98.2%	95.9%	-2.3

[shashwathi]

/lgtm

[abayer]

@pivotal-nader-ziada My biggest concern is the inability to control timeouts for individual tasks from a Pipeline or PipelineRun, so I’m going to get a follow up to this up shortly to add timeouts to PipelineTask.

[abayer]

Actually, I'm going to hold off on that for the moment - lemme see how what you've implemented works out in practice. I still feel that there's a need to be able to say "if the execution of this Task in this Pipeline takes longer than X, time out", but I may be wrong.

[bobcatfish]

Actually, I'm going to hold off on that for the moment - lemme see how what you've implemented works out in practice. I still feel that there's a need to be able to say "if the execution of this Task in this Pipeline takes longer than X, time out", but I may be wrong.

Sounds good! We can definitely iterate on this @abayer :D