Propagate Pipeline name label to PipelineRun

[jerop]

Changes

Currently, a PipelineRun started in a Cancelled or Pending state doesn't receive the Pipeline name label.

As such, a user cannot find all the PipelineRuns related to a given Pipeline. This is a challenge for a user searching for associated PipelineRuns in Pending state in order to start them.

In this change, we ensure that the Pipeline name label is propagated from the Pipeline to the PipelineRun.

Fixes #3903.

/kind bug

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

pipeline name label is propagated to pipelinerun, even when the pipelinerun is pending or cancelled

[tekton-robot]

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.2%	83.3%	0.2

[jerop]

/test check-pr-has-kind-label

[tekton-robot]

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.2%	83.2%	0.1

[vdemeester]

/cc @sbwsg

[tekton-robot]

[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

[ghost]

Thinking about this some more I'm a bit torn about this change. The pending state of PipelineRuns was originally introduced to limit burden on busy clusters and allow (per the TEP) "a custom controller to implement any PipelineRun scheduling policy that is transparent to the PipelineRun controller and PipelineRun creator".

So I'm wondering two things:

1. Are we maybe going to end up re-introducing burden on the cluster here because even in Pending state we are now hitting the cluster to resolve a pipelineRef?
2. Do we consider this a breaking change to a non-alpha feature? Previously Pending meant that resolution wasn't going to happen, but now it will. Should we be concerned about that?

I might be overthinking this though and it's fine. WDYT @jerop?

Pinging @tektoncd/core-maintainers here to get others' input too.

[jerop]

@sbwsg thanks for the additional context from the TEP!

1. Are we maybe going to end up re-introducing burden on the cluster here because even in Pending state we are now hitting the cluster to resolve a pipelineRef?

if users are having trouble starting the pending pipelineruns for a given pipeline because of the missing pipeline label, maybe the burden to resolve that may be necessary? it would not be as much load as actually starting up the pipelinerun, so I wonder if that hit is acceptable?

it makes me also wonder how common this operation is, i.e. do many users need to start pending pipelineruns for a specific pipeline? or do they start pending pipelineruns based on other criteria?

cc @jbarrick-mesosphere @andrewballantyne

2. Do we consider this a breaking change to a non-alpha feature? Previously Pending meant that resolution wasn't going to happen, but now it will. Should we be concerned about that?

good point, i'm not sure about this 🤔

if we consider it a breaking change and still want to make this change, does this mean this will be behind its own feature flag (not the general feature flag), right?

[tekton-robot]

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.2%	83.2%	0.1

[ghost]

Yeah I can definitely get on board with the idea that this isn't so much additional overhead that it's problematic. 👍

I think you're right about the flagging - if we want to make a backwards-incompatible change to behaviour it needs its own feature flag.

Given that we don't have usage data it's tricky to determine whether anyone's actually relying on this specific order of operations. That fact alone makes me lean towards treating it as a backwards-incompatible change - in the absence of data I tend towards assuming we'd break someone / anyone, just to be cautious - but I don't feel super strongly about it, esp since we're in beta.

This might be a good topic to raise in the API WG to get a general consensus on whether folks agree or not that it's backwards-incomatible?

[jerop]

/test pull-tekton-pipeline-alpha-integration-tests

[andrewballantyne]

Just to add my two cents in here as I logged #3903.

Inside OpenShift Console using the OpenShift Pipelines Operator (which packages Tekton for us), we have a details/list page that presents a list to the user of all Pipeline Runs that are associated with the current selected Pipeline. The problem here is we use the pipeline label to do that lookup. So the Pending Pipelines currently do not display in that list despite having a reference to the Pipeline within' them. It would be nice to have that label in order to allow for the list to present for Pending PipelineRuns; alongside the existing currently running and past ran ones.

The only coding alternative I'm aware of is to fetch all Pipeline Runs in the namespace and then to filter them down based on the pipelineRef. This has an obvious undesirable weight.

The reason this was logged was because we added a feature to start the paused Pipelines, but finding them is not as easy due to the aforementioned issues. Cancelled Pipelines was lumped into this because it is also missing that label but there isn't any real action for the user.

The how, cost and implementation details I'll obviously leave up to you guys. There are a few work arounds the user can do to find their Pipeline that is Paused or Cancelled, it'll just not be straight forward.

[jerop]

/test pull-tekton-pipeline-alpha-integration-tests

[ghost]

Ahhh I think we can set that one specific label without performing any resolution / changing order of operations here. We know the pipeline's name as soon as we have the PipelineRun. It's either:

1. The spec.pipelineRef.name
2. The name of the PipelineRun (if spec.pipelineSpec is used instead of pipelineRef)

[tekton-robot]

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.2%	82.9%	-0.2

[tekton-robot]

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.2%	82.8%	-0.4

[tekton-robot]

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.2%	82.8%	-0.4

[tekton-robot]

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.2%	82.7%	-0.4

[jerop]

Ahhh I think we can set that one specific label without performing any resolution / changing order of operations here. We know the pipeline's name as soon as we have the PipelineRun. It's either:

1. The spec.pipelineRef.name
2. The name of the PipelineRun (if spec.pipelineSpec is used instead of pipelineRef)

great idea @sbwsg - updated the PR to only add the pipeline name label - thanks!

[ghost]

Perfect! Possibly a good candidate to bring in to v0.27.2 as well? I will do that tomorrow.

/lgtm
/add-label needs-cherry-pick

[jerop]

Perfect! Possibly a good candidate to bring in to v0.27.2 as well? I will do that tomorrow.

yes yes, thank you @sbwsg 😁

[andrewballantyne]

I appreciate all the help and thanks for getting this done @jerop