Fix deletion when TaskRun is part of PipelineRun

[sayan-biswas]

Fixes #689

Changes

Prevent deletion of any object OwnerRef check is disabled and yet the object has a PipelineRun as owner ref.

This is needed because the Pipeline Controller doesn't handle the edge case where the child TaskRun can be deleted without deleting the parent PipelineRun, and as a result the deletion causes the PipelineRun object to get updated with a "Pending" status for that task.

If an aggressive pruning is required, this can be removed in future once the above scenario is handled by the Pipeline Controller.

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you review them:

• Has Docs included if any changes are user facing
• Has Tests included if any functionality added or changed
• Tested your changes locally (if this is a code change)
• Follows the commit message standard
• Meets the Tekton contributor standards (including functionality, content, code)
• Has a kind label. You can add a comment on this PR that contains /kind <type>. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tep
• Release notes block below has been updated with any user-facing changes (API changes, bug fixes, changes requiring upgrade notices or deprecation warnings)
• Release notes contain the string "action required" if the change requires additional action from users switching to the new release

Release Notes

This change will prevent the controller from directly pruning TaskRuns which are initiated by a PipelineRun. Instead these TaskRuns will be deleted only when the owner PipelineRun is deleted.

[tekton-robot]

The following is the coverage report on the affected files.
Say /test pull-tekton-results-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/watcher/reconciler/dynamic/dynamic.go	66.2%	69.3%	3.1

[Roming22]

/lgtm

[tekton-robot]

@Roming22: changing LGTM is restricted to collaborators

In response to this:

/lgtm

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.

[gabemontero]

This is needed because the Pipeline Controller doesn't handle the edge case where the child TaskRun can be deleted without deleting the parent PipelineRun, and as a result the deletion causes the PipelineRun object to get updated with a "Pending" status for that task.

to be clear @sayan-biswas , and apologies if you feel it is too obvious, but rather then updating with a "pending" status for the task, can you explicitly state what updates if any the pipeline controller should make on the PipelineRun object for the task? some status of "deleted" ?

Among other things I am curious because of the need to handle the deletion of taskruns owned by pipelineruns that are still alive in some other scenarios.

Also, coming in new to this, I am a bit curious why we might not ever want to always honor the owner ref ... is it to simply speed up pruning ?

[sayan-biswas]

@gabemontero As per my understanding, the pipeline controller propagates the status of the child TaskRuns to the parent PipelineRun object when it changes. But in this case, by design, deletion of TaskRun will trigger a reconciliation but I don't get what is the point of updating the parent PipelineRun object to "Pending", is it not possible to keep it as it is.
Its been a long time I've visited the tekton pipeline project code, let me find out what is causing this, then I'll have more understanding.

[Roming22]

Also, coming in new to this, I am a bit curious why we might not ever want to always honor the owner ref ... is it to simply speed up pruning ?

We have PipelineRuns that have an ownerRef. Those PipelineRuns are not being pruned, and end up polluting the namespace.

[adambkaplan]

Code changes look good - I found that some of the comments in the test code need to be fixed as the comment does not match the logic in the test.

Otherwise my concerns fall into the general "code hygiene" category:

1. Include a body in the commit message that explains the what and why of the change. See the Tekton commit message standards
2. Fill in the release note of the pull request description - this is a behavior change that warrants a release note.

[adambkaplan]

Fix comment - resource should exist.
There are other identical comments that should be corrected - appears to be a copy/paste issue.

Suggested change

	// Make sure that the resource no longer exists
	// Make sure the TaskRun is not deleted

[sayan-biswas]

Done

[adambkaplan]

I'm setting aside user-facing docs concerns, as we really don't have great documentation today.

[adambkaplan]

Does this fix #689?

[tekton-robot]

The following is the coverage report on the affected files.
Say /test pull-tekton-results-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/watcher/reconciler/dynamic/dynamic.go	68.3%	69.3%	1.1

[sayan-biswas]

Does this fix #689?

Yes.

[tekton-robot]

The following is the coverage report on the affected files.
Say /test pull-tekton-results-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/watcher/reconciler/dynamic/dynamic.go	68.3%	69.3%	1.1

[adambkaplan]

/lgtm

[tekton-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adambkaplan, Roming22, vdemeester

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 [adambkaplan,vdemeester]

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