PipelineRun's completed status can be overwritten by failed results lookup

[ghost]

Expected Behavior

When a PipelineRun is completed the final status it records should be considered immutable and not change after that point.

Actual Behavior

When a Pipeline includes Pipeline Results there is a slim chance that its PipelineRuns' final status can be overwritten. This is because the PipelineRun reconciler does not process Pipeline Results until after the Run is in a completed state. The code for this is here:

pipeline/pkg/reconciler/pipelinerun/pipelinerun.go

Lines 326 to 328 in f121db9

	pr.Status.MarkFailed(ReasonCouldntGetPipeline,
	"Error retrieving pipeline for pipelinerun %s/%s: %s",
	pr.Namespace, pr.Name, err)

That linked function, updatePipelineResults() is called from

pipeline/pkg/reconciler/pipelinerun/pipelinerun.go

Line 179 in f121db9

c.updatePipelineResults(ctx, pr, getPipelineFunc)

which is inside a block guarded by if pr.IsDone(). Despite this the updatePipelineResults func can change the state of the PipelineRun to indicate a failure. This therefore has the opportunity to modify an existing completed status and overwrite information pertinent to the user.

This issue was created in response to feedback on PR 3472: https://github.com/tektoncd/pipeline/pull/3472/files#r552632569

[ghost]

The code has moved around a bit since #3472 has been merged but the issue still affects the reconciler here:

pipeline/pkg/reconciler/pipelinerun/pipelinerun.go

Lines 179 to 182 in 6e6b6c8

	msg := fmt.Sprintf("Failed to get Pipeline Spec to process Pipeline Results for PipelineRun %s/%s: %v", pr.Namespace, pr.Name, err)
	logger.Error(msg)
	logger.Warnf("An error processing Pipeline Results overwrites existing Succeeded Condition for PipelineRun %s/%s: %v", pr.Namespace, pr.Name, pr.Status.GetCondition(apis.ConditionSucceeded))
	pr.Status.MarkFailed(ReasonCouldntGetPipeline, msg)