adding label `memberOf` to identify a taskRun in the pipeline #4121

pritidesai · 2021-07-28T05:47:45Z

Changes

A label named memberOf is added to all the taskRuns/Runs created while executing the pipeline based on their respective membership. A taskRun will have a label tekton.dev/memberOf=tasks for the task defined under "tasks" section and a taskRun will have a label tekton.dev/memberOf=finally for the task defined under "finally" section.

Implementation Iterations

Iteration1:

A label is added to a taskRun for a task which is part of the finally section. The label added with a pattern tekton.dev/finallyTask and set to true for a finally task.

Closes #3721
Closes #3715

/kind feature

Submitter Checklist

As the author of this PR, please check off the items in this checklist:

Docs included if any changes are user facing
Tests included if any functionality added or changed
Follows the commit message standard
Meets the Tekton contributor standards (including
functionality, content, code)
Release notes block below has been filled in or deleted (only if no user facing changes)

Release Notes

A taskRun/Run in a pipeline will have a new label `tekton.dev/memberOf=tasks` for the task defined under "tasks" section and `tekton.dev/memberOf=finally` for the task defined under "finally" section.

tekton-robot · 2021-07-28T05:49:43Z

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

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/pipelinerun.go	83.4%	83.5%	0.1

tekton-robot · 2021-07-28T06:11:19Z

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

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/pipelinerun.go	83.4%	83.6%	0.2

tekton-robot · 2021-07-28T09:59:31Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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~~ [vdemeester]

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

afrittoli

Thanks for this, it sounds like a good idea!!
I would alter the implementation slightly though - see comments inline.

afrittoli · 2021-07-28T15:21:58Z

pkg/reconciler/pipelinerun/pipelinerun.go

+
+	// check if a task is part of the finally section, add a label to identify it during the runtime
+	if pr.Status.PipelineSpec != nil {
+		for _, f := range pr.Status.PipelineSpec.Finally {
+			if pipelineTask.Name == f.Name {
+				labels[pipeline.GroupName+pipeline.FinallyLabelKey] = "true"
+				break
+			}
+		}
+	}


This doesn't feel like 100% right place for this, this function combines Task spec and TaskRun labels, which is why the function does not have access to the pipeline spec, and you have to go and get it from the status. Whether a PipelineTask belongs to the finally section is a Pipeline level concern, so perhaps we could pass that info down to the createTaskRun function, adding an extra boolean param:

if rprt.IsFinalTask(pipelineRunFacts) { rprt.TaskRun, err = c.createTaskRun(ctx, rprt, pr, as.StorageBasePath(pr), getFinallyTaskRunTimeout, True) } else { rprt.TaskRun, err = c.createTaskRun(ctx, rprt, pr, as.StorageBasePath(pr), getTaskRunTimeout, False) }

Alternatively you could use the GetPipelineFunc function, which returns the spec from the status if available, or it will fetch it otherwise.

thanks @afrittoli I think this was a reasonable place since its combining labels from the task and taskRun. Thanks for suggestion, I have moved it to getTaskrunLabels function where we are adding other tekton.dev labels such as tekton.dev/pipelineRun and tekton.dev/pipelineTask.

Also, its safe and reasonable to get the specification from the status after we added it as a source of truth in #3941

createTaskRun is growing complex with every new functionality we add. We really need a factory style interface for the taskRun and Run instead of operating on the parameters.

afrittoli · 2021-07-28T15:22:17Z

pkg/reconciler/pipelinerun/pipelinerun.go

@@ -929,6 +929,16 @@ func combineTaskRunAndTaskSpecLabels(pr *v1beta1.PipelineRun, pipelineTask *v1be
 			labels[key] = value
 		}
 	}
+
+	// check if a task is part of the finally section, add a label to identify it during the runtime
+	if pr.Status.PipelineSpec != nil {


If nil we just ignore it and go on?

This if statement is just avoiding nil pointer reference and this logic is mainly for the finally task. By the time, a finally taskRun is getting created, the status is guaranteed to have the specification (which should be initialized before the first taskRun is created).

tekton-robot · 2021-07-28T16:55:59Z

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

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/pipelinerun.go	83.4%	83.6%	0.2

ghost · 2021-08-03T14:37:33Z

I am curious why "tekton.dev/finallyTask":"true" was chosen over the "memberOf" approach? I had a quick scan through the linked issues and couldn't find any pros/cons one way or the other.

I have a hunch that "memberOf" is better for the following reason:

Imagine you have a cluster full of completed taskruns from your current Tekton install. You update to the new Tekton version which adds the "finallyTask" label. You query for "tekton.dev/finallyTask" of "true" and this returns all the finally tasks created after the label was introduced. Then you want the set of dag tasks so you query for "tekton.dev/finallyTask" of <blank> or <nil> or whatever your tooling supports. This returns both all of the dag taskruns and all of the old taskruns created before the new label was introduced (regardless of finally/dag).

With "memberOf" you can query directly for all the finally taskruns created after the label was introduced, all of the dag taskruns created after the label was introduced, and all of the "other" taskruns created before the label was introduced.

No problem if we do decide it's preferable to stick with a label of "true", just wanted to make sure this choice is discussed first. I'd much prefer we don't come back and add another label like "memberOf" later if we can simply introduce it now.

WDYT?

vdemeester · 2021-08-04T07:57:43Z

No problem if we do decide it's preferable to stick with a label of "true", just wanted to make sure this choice is discussed first. I'd much prefer we don't come back and add another label like "memberOf" later if we can simply introduce it now.

WDYT?

I share the same "thoughts" as @sbwsg, I think the "memberOf" approach could work really well here.

pritidesai · 2021-08-05T00:18:52Z

I am curious why "tekton.dev/finallyTask":"true" was chosen over the "memberOf" approach? I had a quick scan through the linked issues and couldn't find any pros/cons one way or the other.

The only reason to choose one over the other: bool might be easier to consume compared to a pipeline specific field finally/tasks 😺 At the same time, you have captured a very compelling significance of using memberOf, changing it to have memberOf label 👩‍💻

tekton-robot · 2021-08-05T03:15:42Z

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

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/pipelinerun.go	83.4%	83.7%	0.3

tekton-robot · 2021-08-05T03:25:11Z

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

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/pipelinerun.go	83.4%	83.7%	0.3

A label is added to a taskRun for a task which is part of the finally section. The label added is tekton.dev/finallyTask and set to true for a finally task.

tekton-robot · 2021-08-05T04:22:40Z

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

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/pipelinerun.go	83.4%	83.7%	0.3

ghost · 2021-08-05T12:08:50Z

docs/labels.md

+			<td><code>tekton.dev/memberOf</code></td>
+			<td><code>TaskRuns</code> that are created automatically during the execution of a <code>PipelineRun</code>.</td>
+			<td><code>TaskRuns, Pods</code></td>
+			<td><code>tasks</code> or <code>finally</code> depending on the <code>PipelineTask</code>'s membership in the <code>Pipeline</code>.</td>


nice 👍 maps to the field names in the pipeline.

ghost · 2021-08-05T12:19:04Z

Thanks @pritidesai !

/lgtm

tekton-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. labels Jul 28, 2021

tekton-robot requested review from imjasonh and vdemeester July 28, 2021 05:47

tekton-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jul 28, 2021

pritidesai added this to the Pipelines v0.27 milestone Jul 28, 2021

pritidesai force-pushed the feature-3721 branch from af40a15 to 5f72b0b Compare July 28, 2021 06:09

vdemeester approved these changes Jul 28, 2021

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 28, 2021

afrittoli reviewed Jul 28, 2021

View reviewed changes

pritidesai force-pushed the feature-3721 branch from 5f72b0b to f5287b0 Compare July 28, 2021 16:53

pritidesai force-pushed the feature-3721 branch from f5287b0 to 169c27a Compare August 5, 2021 03:13

tekton-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Aug 5, 2021

pritidesai force-pushed the feature-3721 branch from 169c27a to 07fb2e2 Compare August 5, 2021 03:22

adding label to identify a finally taskRun in a pipeline

bee59de

A label is added to a taskRun for a task which is part of the finally section. The label added is tekton.dev/finallyTask and set to true for a finally task.

pritidesai force-pushed the feature-3721 branch from 07fb2e2 to bee59de Compare August 5, 2021 04:20

pritidesai changed the title ~~adding label to identify a finally taskRun in the pipeline~~ adding label memeberOf to identify a taskRun in the pipeline Aug 5, 2021

pritidesai changed the title ~~adding label memeberOf to identify a taskRun in the pipeline~~ adding label memberOf to identify a taskRun in the pipeline Aug 5, 2021

ghost reviewed Aug 5, 2021

View reviewed changes

tekton-robot assigned ghost Aug 5, 2021

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 5, 2021

tekton-robot merged commit 2b4f374 into tektoncd:main Aug 5, 2021

pritidesai deleted the feature-3721 branch August 5, 2021 16:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adding label `memberOf` to identify a taskRun in the pipeline #4121

adding label `memberOf` to identify a taskRun in the pipeline #4121

pritidesai commented Jul 28, 2021 •

edited

Loading

tekton-robot commented Jul 28, 2021

tekton-robot commented Jul 28, 2021

tekton-robot commented Jul 28, 2021

afrittoli left a comment

afrittoli Jul 28, 2021

pritidesai Jul 28, 2021

afrittoli Jul 28, 2021

pritidesai Jul 28, 2021

tekton-robot commented Jul 28, 2021

ghost commented Aug 3, 2021 •

edited by ghost

Loading

vdemeester commented Aug 4, 2021

pritidesai commented Aug 5, 2021 •

edited

Loading

tekton-robot commented Aug 5, 2021

tekton-robot commented Aug 5, 2021

tekton-robot commented Aug 5, 2021

ghost Aug 5, 2021

ghost commented Aug 5, 2021

adding label memberOf to identify a taskRun in the pipeline #4121

adding label memberOf to identify a taskRun in the pipeline #4121

Conversation

pritidesai commented Jul 28, 2021 • edited Loading

Changes

Submitter Checklist

Release Notes

tekton-robot commented Jul 28, 2021

tekton-robot commented Jul 28, 2021

tekton-robot commented Jul 28, 2021

afrittoli left a comment

Choose a reason for hiding this comment

afrittoli Jul 28, 2021

Choose a reason for hiding this comment

pritidesai Jul 28, 2021

Choose a reason for hiding this comment

afrittoli Jul 28, 2021

Choose a reason for hiding this comment

pritidesai Jul 28, 2021

Choose a reason for hiding this comment

tekton-robot commented Jul 28, 2021

ghost commented Aug 3, 2021 • edited by ghost Loading

vdemeester commented Aug 4, 2021

pritidesai commented Aug 5, 2021 • edited Loading

tekton-robot commented Aug 5, 2021

tekton-robot commented Aug 5, 2021

tekton-robot commented Aug 5, 2021

ghost Aug 5, 2021

Choose a reason for hiding this comment

ghost commented Aug 5, 2021

adding label `memberOf` to identify a taskRun in the pipeline #4121

adding label `memberOf` to identify a taskRun in the pipeline #4121

pritidesai commented Jul 28, 2021 •

edited

Loading

ghost commented Aug 3, 2021 •

edited by ghost

Loading

pritidesai commented Aug 5, 2021 •

edited

Loading