Closed sayan-biswas closed 8 months ago
tekton-robot
commented
8 months ago 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
commented
8 months ago /lgtm
tekton-robot
commented
8 months ago @Roming22: changing LGTM is restricted to collaborators
gabemontero
commented
8 months ago 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
commented
8 months ago @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
commented
8 months ago 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
commented
8 months ago I'm setting aside user-facing docs concerns, as we really don't have great documentation today.
adambkaplan
commented
8 months ago Does this fix #689?
tekton-robot
commented
8 months ago 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
commented
8 months ago Does this fix #689?
Yes.
tekton-robot
commented
8 months ago 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 |
tekton-robot
commented
8 months ago [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
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:
/kind <type>. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tepRelease Notes