TaskRun.Status.TaskSpec is sometimes overwritten without replacements applied

[abayer]

Discovered this due to TestPipelineRunStatusSpec/pipeline_status_spec_updated e2e test being flaky:

init_test.go:138: Create namespace arendelle-t9lb4 to deploy to
    init_test.go:166: Verify SA "default" is created in namespace "arendelle-t9lb4"
    pipelinerun_test.go:110: Setting up test resources for "pipeline status spec updated" test in namespace arendelle-t9lb4
    pipelinerun_test.go:120: Waiting for PipelineRun pipeline-task-update in namespace arendelle-t9lb4 to complete
    pipelinerun_test.go:124: Making sure the expected TaskRuns [task1] were created
    pipelinerun_test.go:164: Checking if parameter replacements have been updated in the spec.
    pipelinerun_test.go:171: Expected replaced parameter value : Hello World! in Script: #!/usr/bin/env bash
        echo "$(params.HELLO)"
         But not found

Seen on https://prow.tekton.dev/view/gs/tekton-prow/pr-logs/pull/tektoncd_pipeline/5573/pull-tekton-pipeline-alpha-integration-tests/1575135529287028736 among others.

/kind bug

[abayer]

So the resulting pod has the right script - the parameter replacement is performed there. But for some reason I have yet to figure out, the updated TaskSpec used to create the pod is not also being saved to tr.Status.TaskSpec after the replacement has happened.

[abayer]

Ok, some hacky debug logging later, it turns out that tr.Status.TaskSpec.Steps[0].Script is getting updated properly, but we only do the applyParamsContextsResultsAndWorkspaces when there isn't already a pod, and are (somehow, still not exactly sure how) overwriting tr.Status.TaskSpec with every reconcile, so that the original TaskSpec keeps getting put back.

[abayer]

Yup, more debug logging discoveries: sometimes, the first call to taskrun.go's reconcile after the tr.Status.TaskSpec has been updated sees a nil tr.Status.TaskSpec. When that's the case, we end up with the parameter value unreplaced in tr.Status.TaskSpec.Steps[0].Script. So what's making that be nil?

The only thing I've found so far that differs is that when we end up with a nil tr.Status.TaskSpec, the pipelinerun has been reconciled between reconcile calls to the TaskRun, but I'm not sure if that's just coincidence on a small sample size.

[abayer]

Ok, that pattern seems to be consistent - if the PipelineRun is reconciled after the TaskRun reconciliation that creates the pod and updates tr.Status.TaskSpec, but before the next reconciliation of that TaskRun, tr.Status.TaskSpec is nil at the start of that second TaskRun reconciliation and the "original" TaskSpec is written to tr.Status.TaskSpec. That strongly suggests something in the PipelineRun reconciliation, at that specific time, is resulting in wiping tr.Status.TaskSpec...

[abayer]

EDIT: ok, major correction this time - I misread the logs. Nothing actually differs in terms of order of operation after all.

But here's the order anyway, for posterity's sake.

• reconcile is called for the TaskRun is reconciled
• A Pending event gets broadcast for the TaskRun
• The generated reconciler.go code for the TaskRun calls updater.UpdateStatus with the correct tr.Status.TaskSpec
• Found a TaskRun pipeline-task-update-task1 that was missing from the PipelineRun status shows up from the PipelineRun reconciler
• The PipelineRun is reconciled
• The TaskRun reconciler.go calls updater.UpdateStatus again, still with the correct tr.Status.TaskSpec
• A Started event gets broadcast for the TaskRun
• reconcile is called for the TaskRun again - when the test's failing, tr.Status.TaskSpec is now nil

[abayer]

Ok, pretty sure this is due to 9cf7590, thanks to @wlynch noticing a new chains failure (tektoncd/chains#577) between Pipeline v0.40.0 and v0.40.1 - that's the exact code I've been looking at for this flake, but I somehow didn't notice that it had just been moved. 🤦