task results reassertion

pritidesai commented 4 years ago

TaskResultsNotProduced: TaskRun for a task should result in failure if task results are not populated. Tasks are declared successful today even before initializing task results from container status. updateTaskRunResourceResult is executed when a taskRun is declared successful. TaskRun is declared complete and successful here, we should add a check on task results even before updating taskRun.status.
InvalidTaskResultReference: Task execution order is set such that the task producing a result is executed before the task referencing that result. In case of a failure of a referenced task, pipeline exits without executing the rest of the pipeline tasks. But when a referenced task succeeds and does not instantiate the result, the task referencing that result is not executed and pipeline results in failure. Pipeline exits with PipelineValidationFailed since the result did not exist. Replace PipelineValidationFailed with something more specific InvalidTaskResultReference.

An example of such task and pipeline execution here.

I think, we might not even get to this kind of error if TaskResultsNotProduced is implemented.

MissingTaskResultReference: A task might be set to produce task results but these results are not referenced in any of the pipeline tasks. This scenario is ignored today, does not impact task execution order and does not produce any failure. Pipeline should result in failure if task results are not referenced at all.

Please feel free to suggest better naming if these does not make sense.

bobcatfish commented 4 years ago

Hey @pritidesai ! I think I'm missing a bit of context for this one: is this about changing the error "reason" fields in Status conditions?

pritidesai commented 4 years ago

yes changing the reason field from generic PipelineValidationFailed to specific InvalidTaskResultReference and also changing the existing behavior where taskRun is marked successful without checking if task results were initialized/populated or not ... change TaskRun reconciler such that, check on task results are done before changing taskRun status to sucess.

nikhil-thomas commented 4 years ago

/area api

tekton-robot commented 4 years ago

Stale issues rot after 30d of inactivity. Mark the issue as fresh with /remove-lifecycle rotten. Rotten issues close after an additional 30d of inactivity. If this issue is safe to close now please do so with /close.

/lifecycle rotten

Send feedback to tektoncd/plumbing.

tekton-robot commented 4 years ago

Rotten issues close after 30d of inactivity. Reopen the issue with /reopen. Mark the issue as fresh with /remove-lifecycle rotten.

/close

Send feedback to tektoncd/plumbing.

tekton-robot commented 4 years ago

@tekton-robot: Closing this issue.

In response to [this](https://github.com/tektoncd/pipeline/issues/2701#issuecomment-674364177): >Rotten issues close after 30d of inactivity. >Reopen the issue with `/reopen`. >Mark the issue as fresh with `/remove-lifecycle rotten`. > >/close > >Send feedback to [tektoncd/plumbing](https://github.com/tektoncd/plumbing). Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.

vdemeester commented 4 years ago

/remove-lifecycle rotten /remove-lifecycle stale /reopen

tekton-robot commented 4 years ago

@vdemeester: Reopened this issue.

In response to [this](https://github.com/tektoncd/pipeline/issues/2701#issuecomment-674757813): >/remove-lifecycle rotten >/remove-lifecycle stale >/reopen Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.

bobcatfish commented 4 years ago

@pritidesai looking at this more closely would you say #3497 is a duplicate of this issue? It sounds like this issue is about trying to add more validation around expectations for Task results?

pritidesai commented 4 years ago

This issue is about documenting and implementing the expected behavior in cases such as TaskResultsNotProduced i.e. a task must fail if the result is not populated (same as issue #3497), and InvalidTaskResultReference which was implemented with PR #2538.

MissingTaskResultReference is something not discussed much but does not warrant any changes in pipeline. A task is allowed to produce a number of results but pipeline does not enforce all of them being utilized by other tasks.

Yup, we can close this as a duplicate of #3497

tektoncd / pipeline

task results reassertion #2701