PipelineRun Cancellation is not working

[ghost]

Expected Behavior

Cancelling a PipelineRun should result in its TaskRuns being put into a cancelled state.

Our integration test, test/cancel_test.go should pass.

Actual Behavior

Cancelling a PipelineRun no longer seems to cancel the underlying TaskRun. It appears that each attempt to cancel the TaskRuns hit errors like this:

error(s) from cancelling TaskRun(s) from PipelineRun pear: Operation cannot be fulfilled on taskruns.tekton.dev \"pear-foo-cb9rg\": the object has been modified; please apply your changes to the latest version and try again

The e2e test/cancel_test.go fails.

Steps to Reproduce the Problem

1. go test -v -count=1 -tags=e2e -timeout=20m -run ".*PipelineRunCancel.*" ./test
2. Watch the PipelineRuns - they are cancelled almost immediately.
3. Look at the TaskRuns - these never get cancelled and the test times out.

Additional Info

• Kubernetes version:

  Output of kubectl version:

I've tested against a 1.18 server and a 1.15 server. Both exhibit the same issue.

Client Version: version.Info{Major:"1", Minor:"15", GitVersion:"v1.15.5", GitCommit:"20c265fef0741dd71a66480e35bd69f18351daea", GitTreeState:"clean", BuildDate:"2019-10-15T19:16:51Z", GoVersion:"go1.12.10", Compiler:"gc", Platform:"darwin/amd64"}
Server Version: version.Info{Major:"1", Minor:"18", GitVersion:"v1.18.0", GitCommit:"9e991415386e4cf155a24b1da15becaa390438d8", GitTreeState:"clean", BuildDate:"2020-03-25T14:50:46Z", GoVersion:"go1.13.8", Compiler:"gc", Platform:"linux/amd64"}

• Tekton Pipeline version:

Latest master

[ghost]

/kind bug

[vincent-pli]

@sbwsg I checked the code:

pipeline/pkg/reconciler/pipelinerun/cancel.go

Lines 48 to 54 in 8a059fe

	rprt.TaskRun.Spec.Status = v1alpha1.TaskRunSpecStatusCancelled
	if _, err := clientSet.TektonV1alpha1().TaskRuns(pr.Namespace).UpdateStatus(rprt.TaskRun); err != nil {
	errs = append(errs, err.Error())
	}
	if _, err := clientSet.TektonV1alpha1().TaskRuns(pr.Namespace).Update(rprt.TaskRun); err != nil {
	errs = append(errs, err.Error())
	}

It update the status then the spec, it will raise the error you mentioned.
Why we need update the status of taskrun which will be cancelled? since I'm not found other such update in reconcile of pipelinerun.

[ghost]

Ahhh I see. In order to make this work the TaskRun would have to be re-fetched after UpdateStatus to get the latest version?

Why we need update the status of taskrun which will be cancelled? since I'm not found other such update in reconcile of pipelinerun.

This is a good question. If a TaskRun is put into a cancelled state I would expect that the TaskRun reconciler would be responsible for this.

[ghost]

Actually now I'm confused - I would expect the PipelineRun reconciler to be responsible for putting each TaskRun into a Cancelled state. But I would expect the TaskRun reconciler to perform any non-Status Update that is necessary in response to this.

[ghost]

Sidenote: I find it weird that any PRs are passing in Pipelines right now given this...

[vdemeester]

If you cancel a PipelineRun, the pipelinerun reconcilier needs to update the "wanted" status of all TaskRuns to cancel.. So for we the Update make sense, the UpdateStatus doesn't 🤔

[vincent-pli]

I'm not sure, but I think the status update of taskrun should be take charge by reconcile of taskrun, pipelinerun just make the change...

[vdemeester]

@vincent-pli yep, the PipelineRun controller should update the spec.status of TaskRun and let the TaskRun reconcilier handle the rest (hence we shouldn't update status.xxx of TaskRun from the PipelineRun controller).

[ghost]

I've been trying to fix this today but haven't made very much progress. I think I've managed to reduce the number of failures but I cannot get this test to pass consistently 100% of the time. I've opened a PR with my work-in-progress changes. If anyone else has an opportunity to look at it I'd be very appreciative, otherwise I will pick it up again on Monday.

[ghost]

Now that #2369 is closed I'm going to call this done. Let's reopen if it starts appearing again.