PipelineRun reconciler shouldn't care about how the Timeout field is implemented in Custom Task Run.

[XinruZhang]

PipelineRun reconciler shouldn't care about how the Timeout field is implemented in Custom Task Run.

Currently in PipelineRun reconciler, it cancells the Run when it detects that Run is TimeOut:

pipeline/pkg/reconciler/pipelinerun/pipelinerun.go

Lines 657 to 659 in 2b54123

	if err := c.processRunTimeouts(ctx, pr, pipelineRunState); err != nil {
	return err
	}

by calling

pipeline/pkg/apis/pipeline/v1alpha1/run_types.go

Lines 230 to 241 in 2b54123

	func (r *Run) HasTimedOut(c clock.PassiveClock) bool {
	if r.Status.StartTime == nil || r.Status.StartTime.IsZero() {
	return false
	}
	timeout := r.GetTimeout()
	// If timeout is set to 0 or defaulted to 0, there is no timeout.
	if timeout == apisconfig.NoTimeoutDuration {
	return false
	}
	runtime := c.Since(r.Status.StartTime.Time)
	return runtime > timeout
	}

This behavior leads to flaky unit test results now and then. See the test result of TestWaitCustomTask_PipelineRun/Wait_Task_Retries_on_Timeout in

• https://storage.googleapis.com/tekton-prow/pr-logs/pull/tektoncd_pipeline/5547/pull-tekton-pipeline-alpha-integration-tests/1574802170165858304/build-log.txt
• https://storage.googleapis.com/tekton-prow/pr-logs/pull/tektoncd_pipeline/5648/pull-tekton-pipeline-alpha-integration-tests/1582078450888347648/build-log.txt

Say we have defined the following PipelineRun

apiVersion: tekton.dev/v1beta1
kind: Pipeline
metadata:
  name: custom-task-pipeline
spec:
  tasks:
  - name: run-wait
    timeout: "1s"
    retries: 1
    taskRef:
      apiVersion: wait.testing.tekton.dev/v1alpha1
      kind: Wait
    params:
    - name: duration
      value: "2s"
---
apiVersion: tekton.dev/v1beta1
kind: PipelineRun
metadata:
  name: custom-task-pipelinerun
spec:
  pipelineRef:
    apiVersion: tekton.dev/v1beta1
    name: custom-task-pipeline

Expected Behavior

1. PR reconciler creates a Run: custom-task-pipelinerun-run-wait

   pipeline/pkg/reconciler/pipelinerun/pipelinerun.go

   Lines 790 to 801 in 8399472

   	case rpt.IsCustomTask() && rpt.IsMatrixed():
   	rpt.Runs, err = c.createRuns(ctx, rpt, pr)
   	if err != nil {
   	recorder.Eventf(pr, corev1.EventTypeWarning, "RunsCreationFailed", "Failed to create Runs %q: %v", rpt.RunNames, err)
   	return fmt.Errorf("error creating Runs called %s for PipelineTask %s from PipelineRun %s: %w", rpt.RunNames, rpt.PipelineTask.Name, pr.Name, err)
   	}
   	case rpt.IsCustomTask():
   	rpt.Run, err = c.createRun(ctx, rpt.RunName, nil, rpt, pr)
   	if err != nil {
   	recorder.Eventf(pr, corev1.EventTypeWarning, "RunCreationFailed", "Failed to create Run %q: %v", rpt.RunName, err)
   	return fmt.Errorf("error creating Run called %s for PipelineTask %s from PipelineRun %s: %w", rpt.RunName, rpt.PipelineTask.Name, pr.Name, err)
   	}

2. Run reconciler reconcils the Run, continues reconciling because the Run hasn't TimedOut.
3. Run reconciler detects the Run has timed out, marks it as failed on TimedOut after 1s, example:

   pipeline/test/custom-task-ctrls/wait-task/pkg/reconciler/reconciler.go

   Lines 126 to 139 in 8399472

   	if r.Status.StartTime != nil && elapsed > timeout {
   	logger.Infof("The Custom Task Run %v timed out", r.GetName())
   	r.Status.CompletionTime = &metav1.Time{Time: c.Clock.Now()}
   	r.Status.MarkRunFailed("TimedOut", WaitTaskCancelledByRunTimeoutMsg)
   	// Retry if the current RetriesStatus hasn't reached the retries limit
   	if r.Spec.Retries > len(r.Status.RetriesStatus) {
   	logger.Infof("Run timed out, retrying... %#v", r.Status)
   	retryRun(r)
   	return controller.NewRequeueImmediately()
   	}
   	return nil
   	}

4. PR reconciler detects the Run is marked as TimedOut, update its ChildReferences or RunsStatus:

   pipeline/pkg/reconciler/pipelinerun/pipelinerun.go

   Lines 684 to 691 in 8399472

   	if cfg.FeatureFlags.EmbeddedStatus == config.FullEmbeddedStatus || cfg.FeatureFlags.EmbeddedStatus == config.BothEmbeddedStatus {
   	pr.Status.TaskRuns = pipelineRunFacts.State.GetTaskRunsStatus(pr)
   	pr.Status.Runs = pipelineRunFacts.State.GetRunsStatus(pr)
   	}
   	if cfg.FeatureFlags.EmbeddedStatus == config.MinimalEmbeddedStatus || cfg.FeatureFlags.EmbeddedStatus == config.BothEmbeddedStatus {
   	pr.Status.ChildReferences = pipelineRunFacts.State.GetChildReferences()
   	}

Actual Behavior

1. PR reconciler creates a Run: custom-task-pipelinerun-run-wait

   pipeline/pkg/reconciler/pipelinerun/pipelinerun.go

   Lines 790 to 801 in 8399472

   	case rpt.IsCustomTask() && rpt.IsMatrixed():
   	rpt.Runs, err = c.createRuns(ctx, rpt, pr)
   	if err != nil {
   	recorder.Eventf(pr, corev1.EventTypeWarning, "RunsCreationFailed", "Failed to create Runs %q: %v", rpt.RunNames, err)
   	return fmt.Errorf("error creating Runs called %s for PipelineTask %s from PipelineRun %s: %w", rpt.RunNames, rpt.PipelineTask.Name, pr.Name, err)
   	}
   	case rpt.IsCustomTask():
   	rpt.Run, err = c.createRun(ctx, rpt.RunName, nil, rpt, pr)
   	if err != nil {
   	recorder.Eventf(pr, corev1.EventTypeWarning, "RunCreationFailed", "Failed to create Run %q: %v", rpt.RunName, err)
   	return fmt.Errorf("error creating Run called %s for PipelineTask %s from PipelineRun %s: %w", rpt.RunName, rpt.PipelineTask.Name, pr.Name, err)
   	}

2. Run reconciler reconcils the Run, continues reconciling because the Run hasn't TimedOut.
3. [The flaky part] PR reconciler processRunTimeouts

   pipeline/pkg/reconciler/pipelinerun/pipelinerun.go

   Lines 657 to 659 in 2b54123

   	if err := c.processRunTimeouts(ctx, pr, pipelineRunState); err != nil {
   	return err
   	}

   at this time, The Run has TimedOut, but the Run reconciler hasn't update the Run's status yet, then PR reconciler tries to cancel the Run

   pipeline/pkg/reconciler/pipelinerun/pipelinerun.go

   Lines 718 to 725 in 2b54123

   	if rpt.Run != nil && !rpt.Run.IsCancelled() && rpt.Run.HasTimedOut(c.Clock) && !rpt.Run.IsDone() {
   	logger.Infof("Cancelling run task: %s due to timeout.", rpt.RunName)
   	err := cancelRun(ctx, rpt.RunName, pr.Namespace, c.PipelineClientSet)
   	if err != nil {
   	errs = append(errs,
   	fmt.Errorf("failed to patch Run `%s` with cancellation: %s", rpt.RunName, err).Error())
   	}
   	}

[XinruZhang]

Took another look at the timeout section of TEP-0002:

For a PipelineRun with either a pipeline level timeout configured and/or the custom task level timout configuration, timeout is updated to the run with same policy as it is for task runs. On timeout, the running run's status is updated with "RunCancelled"

Therefore this is actually aligned with what's designed, but this exact implementation causes the flaky test results.

The statement "timeout is updated to the run with same policy as it is for task runs" in the design is NOT true in the current implementation. For TaskRun, the PipelineRun reconciler doesn't care about how the TaskRunSpec.TimeOut -- TaskRun reconciler handles that.

cc @lbernick @jerop @pritidesai

[tekton-robot]

@XinruZhang: The label(s) kind/flaky cannot be applied, because the repository doesn't have them.

In response to this:

/kind flaky

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[XinruZhang]

/kind flake

[XinruZhang]

Do we want to remove

pipeline/pkg/reconciler/pipelinerun/pipelinerun.go

Lines 657 to 659 in 2b54123

	if err := c.processRunTimeouts(ctx, pr, pipelineRunState); err != nil {
	return err
	}

and update TEP-0002 to reflect the change?

At least we need to update TEP-0002 to reflect that PipelineRun reconciler doesn't cancel TaskRun when its run time bigger than TaskRunSpec.Timeout.

It is worth mentiong here: prior to #5134, pipelinerun reconciler handles the timeout for taskrun.

[XinruZhang]

/assign

[XinruZhang]

Do we want to remove

pipeline/pkg/reconciler/pipelinerun/pipelinerun.go

Lines 657 to 659 in 2b54123

	if err := c.processRunTimeouts(ctx, pr, pipelineRunState); err != nil {
	return err
	}

and update TEP-0002 to reflect the change?
At least we need to update TEP-0002 to reflect that PipelineRun reconciler doesn't cancel TaskRun when its run time bigger than TaskRunSpec.Timeout.

It is worth mentiong here: prior to #5134, pipelinerun reconciler handles the timeout for taskrun.

@ScrapCodes, wondering if you have any thoughts 😀

[ScrapCodes]

Hi @XinruZhang , I want to respond to this and the other PR as well, currently I am working in a limited capacity and enjoying vacations upto Tuesday. Thanks!