Adding TEP for accessing task execution status at runtime #234

pritidesai · 2020-10-16T06:14:40Z

This TEP has a proposal on how to access task execution status at runtime within the pipeline.

Issue: tektoncd/pipeline#1020
PoC: tektoncd/pipeline#3390

pritidesai · 2020-10-16T06:15:24Z

/kind tep

bobcatfish

I really like your approach to narrow the scope of this feature down to just the execution status! I'd been assuming we'd want to provide the entire status but the more I think about it, it probably makes more sense to be intentional about the individual fields we want to provide (and understand why they are needed)

My main feedback:

I'd like to map these values more directly to the possible values that the succeeded condition can take on and potentially handle states that aren't represented there with additional variable replacement values (but maybe leaving them out for now; i.e. start with the bare minimum now and build on it)
In the spirit of supporting additional values, structuring this such that we can add more values (e.g. context.pipelineRun.Tasks.deployment.status instead of just context.pipelineRun.Tasks.deployment)
I also wonder if it would make sense to constrain this just to finally tasks for now, since we don't have any use cases for other tasks and also since this can lead to some racy behavior

teps/0027-task-execution-status-at-runtime.md

ghost · 2020-10-28T12:46:11Z

teps/0027-task-execution-status-at-runtime.md

+
+## Proposal
+
+Introduce a new context variable `$(context.pipelineRun.Tasks.<pipelineTask>.status)` which resolves to one of the execution states:


Small thing related to variable naming - we already have a tasks variable with the following form:

$(tasks.<task-name>.results.<result-name>)

I am wondering if it makes sense to continue following this form for status variables as well? Was there a particular reason that this var was nested under context.pipelineRun?

Alternatively, should we consider moving the results variables under context.pipelineRun?

I am wondering if it makes sense to continue following this form for status variables as well?

Following results form, would lead us to $(tasks.<task-name>.status) 🤔

Was there a particular reason that this var was nested under context.pipelineRun?

Status is something not generated by the task itself, its something controller generating for each task which I think falls under context.pipelineRun similar to pipelineRun name, id, etc. And in future, I am anticipating, this is something auto generated without having task specify explicit mapping i.e. script can directly access context variable with $(context.pipelineRun.[*]).

I am happy to change it though.

Status is something not generated by the task itself, its something controller generating for each task which I think falls under context.pipelineRun similar to pipelineRun name, id, etc.

This makes sense, I hadn't thought of it this way, thanks @pritidesai !

teps/0027-task-execution-status-at-runtime.md

pritidesai · 2020-10-28T17:50:56Z

I have renamed the file, TEP number 27 was picked by other TEP and merged 😢 Will up the number when its close to getting merge.

bobcatfish

This is looking great!

I have one remaining quibble, carrying on @sbwsg 's thoughts about the structure of the variable used for substitution but am happy to approve and merge before we sort that out if needed!

teps/XXXX-task-execution-status-at-runtime.md

bobcatfish · 2020-10-28T20:23:14Z

teps/XXXX-task-execution-status-at-runtime.md

+| ----- | ----------- |
+| `Succeeded` | The `pipelineTask` was successful i.e. a respective pod was created and completed successfully. The `pipelineTask` had a `taskRun` with `Succeeded` `ConditionType` and  `True` `ConditionStatus`. |
+| `Failed` | The `pipelineTask` failed i.e. a respective pod was created but exited with error. The `pipelineTask` has a `taskRun` with `Succeeded` `ConditionType`,  `False` `ConditionStatus` and have exhausted all the retries. |
+| `None` | no execution state available either (1) the `pipeline` stopped executing `dag` tasks before it could get to this task i.e. this task was not started/executed  or (2) the `pipelineTask` is `skipped` because of `when expression` or one of the parent tasks was `skipped`. It is part of `pipelineRun.Status.SkippedTasks`. |


👍 👍 👍 I think starting with this set makes a lot of sense and we can always add more states later!

bobcatfish · 2020-10-30T19:47:58Z

I'm happy to merge this!

As far as I'm concerned:

/approve

Maybe @sbwsg @afrittoli @vdemeester or others want to weigh in?

ghost

Approve from my side too but I think you need someone from another org to given the final +1!

ghost · 2020-11-02T15:36:55Z

teps/XXXX-task-execution-status-at-runtime.md

+    - name: rollback
+      params:
+        - name: deployment-state
+          value: "$(context.pipelineRun.Tasks.deployment.status)"


One more nit about the variable - if we stick with this format, I am wondering why we use Tasks instead of tasks? Apologies if I missed this elsewhere in the conversation!

tekton-robot · 2020-11-02T15:38:48Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bobcatfish, sbwsg

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:

~~teps/OWNERS~~ [bobcatfish,sbwsg]

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

bobcatfish · 2020-11-02T16:11:10Z

total nit @pritidesai I think the filename should change from teps/XXXX-task-execution-status-at-runtime.md to teps/0027-task-execution-status-at-runtime.md

bobcatfish · 2020-11-02T16:16:55Z

also looks like https://github.com/tektoncd/community/pull/244/files is also TEP-0027 so maybe we change this one XD

pritidesai · 2020-11-03T00:12:38Z

picked up 0028 for this TEP for now, will move 0028 to next available number.

pritidesai · 2020-11-03T00:16:16Z

@afrittoli its ready for review, PTAL 🙏

This TEP has a proposal on how to access task execution status at runtime within the pipeline.

afrittoli

Thanks for this!
It looks good and I look forward to this feature.
I think it will combine nicely with when expressions on finally tasks, allowing pipeline editors to skip unneeded finally tasks without altering the definition of the task itself (great for re-use).
/lgtm

afrittoli · 2020-11-03T12:26:47Z

teps/0028-task-execution-status-at-runtime.md

+## Alternatives
+
+* If the states defined in this [Proposal](#proposal) are not clear, one alternative is to change these states and align them
+to `pipelineRun` [execution status](https://github.com/tektoncd/pipeline/blob/master/docs/pipelineruns.md#monitoring-execution-status) (`Unknown`, `True`, `False`). Also change the proposed variable pattern to match these states.


NIT: I kind of liked this alternative, since it maps nicely to the condition, and we could later extend this to give access to other conditions or other fields in the condition, e.g. by doing:

tasks.<pipelineTask>.conditions.succeeded.status

However this syntax would be much more verbose, and if we need in future more details about conditions we can still introduce the conditions field.

tekton-robot requested review from bobcatfish and iancoffey October 16, 2020 06:14

tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 16, 2020

pritidesai changed the title ~~Adding TEP for accessing task execution status at runtime~~ Adding TEP for accessing task execution status at runtime - 0027 Oct 16, 2020

tekton-robot added the kind/tep Categorizes issue or PR as related to a TEP (or needs a TEP). label Oct 16, 2020

pritidesai mentioned this pull request Oct 16, 2020

access execution status of any task in finally tektoncd/pipeline#3390

Merged

4 tasks

bobcatfish reviewed Oct 16, 2020

View reviewed changes

vdemeester reviewed Oct 19, 2020

View reviewed changes

pritidesai force-pushed the pt-exec-status branch 3 times, most recently from af85b2a to 108662a Compare October 20, 2020 00:15

pritidesai mentioned this pull request Oct 21, 2020

Design and implement task results in final tasks tektoncd/pipeline#2557

Closed

bobcatfish mentioned this pull request Oct 22, 2020

Be able to use when expressions in finally Tasks tektoncd/pipeline#3438

Closed

pritidesai force-pushed the pt-exec-status branch from 108662a to 12277cb Compare October 27, 2020 06:16

ghost reviewed Oct 28, 2020

View reviewed changes

pritidesai force-pushed the pt-exec-status branch from 12277cb to 1d8d120 Compare October 28, 2020 17:48

pritidesai changed the title ~~Adding TEP for accessing task execution status at runtime - 0027~~ Adding TEP for accessing task execution status at runtime Oct 28, 2020

bobcatfish reviewed Oct 28, 2020

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 30, 2020

ghost approved these changes Nov 2, 2020

View reviewed changes

pritidesai force-pushed the pt-exec-status branch from 1d8d120 to b5f8d12 Compare November 3, 2020 00:07

pritidesai force-pushed the pt-exec-status branch from b5f8d12 to e320b5b Compare November 3, 2020 00:16

Adding TEP for accessing task execution status at runtime

372cfa5

This TEP has a proposal on how to access task execution status at runtime within the pipeline.

pritidesai force-pushed the pt-exec-status branch from e320b5b to 372cfa5 Compare November 3, 2020 00:18

afrittoli reviewed Nov 3, 2020

View reviewed changes

tekton-robot assigned afrittoli Nov 3, 2020

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 3, 2020

tekton-robot merged commit b16dc94 into tektoncd:master Nov 3, 2020

afrittoli mentioned this pull request Nov 3, 2020

TEP-0039 Add Variable retries and retry-count #239

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding TEP for accessing task execution status at runtime #234

Adding TEP for accessing task execution status at runtime #234

pritidesai commented Oct 16, 2020

pritidesai commented Oct 16, 2020

bobcatfish left a comment

ghost Oct 28, 2020

pritidesai Oct 28, 2020 •

edited

Loading

ghost Oct 28, 2020

pritidesai commented Oct 28, 2020

bobcatfish left a comment

bobcatfish Oct 28, 2020

pritidesai Oct 28, 2020

bobcatfish commented Oct 30, 2020

ghost left a comment

ghost Nov 2, 2020

tekton-robot commented Nov 2, 2020

bobcatfish commented Nov 2, 2020

bobcatfish commented Nov 2, 2020

pritidesai commented Nov 3, 2020

pritidesai commented Nov 3, 2020

afrittoli left a comment

afrittoli Nov 3, 2020


		## Proposal

		Introduce a new context variable `$(context.pipelineRun.Tasks.<pipelineTask>.status)` which resolves to one of the execution states:

Adding TEP for accessing task execution status at runtime #234

Adding TEP for accessing task execution status at runtime #234

Conversation

pritidesai commented Oct 16, 2020

pritidesai commented Oct 16, 2020

bobcatfish left a comment

Choose a reason for hiding this comment

ghost Oct 28, 2020

Choose a reason for hiding this comment

pritidesai Oct 28, 2020 • edited Loading

Choose a reason for hiding this comment

ghost Oct 28, 2020

Choose a reason for hiding this comment

pritidesai commented Oct 28, 2020

bobcatfish left a comment

Choose a reason for hiding this comment

bobcatfish Oct 28, 2020

Choose a reason for hiding this comment

pritidesai Oct 28, 2020

Choose a reason for hiding this comment

bobcatfish commented Oct 30, 2020

ghost left a comment

Choose a reason for hiding this comment

ghost Nov 2, 2020

Choose a reason for hiding this comment

tekton-robot commented Nov 2, 2020

bobcatfish commented Nov 2, 2020

bobcatfish commented Nov 2, 2020

pritidesai commented Nov 3, 2020

pritidesai commented Nov 3, 2020

afrittoli left a comment

Choose a reason for hiding this comment

afrittoli Nov 3, 2020

Choose a reason for hiding this comment

pritidesai Oct 28, 2020 •

edited

Loading