Add Pipeline Label to pipelineRun and tasksRun

[nader-ziada]

Expected Behavior

The PipelineRun and TasksRun object should have a label with the pipeline name. Can be called knative.dev/pipeline with a value of the actual pipeline name.

The TaskRun controller can use the Pipeline name in Pod metadata and log messages.

Actual Behavior

Currently there are no labels added

Additional Info

• The PR to add this functionality should add an integration test to validate labels are propagating correctly
• Similar example in serving and the Spec for it
• Should we have any other labels? such as Task name label on the TaskRun?

[bobcatfish]

One thing I'm wondering from this description is what the purpose of the labels are? e.g. what use cases do you need labels in (part of this is probably me not being too familiar with labels!)

[nader-ziada]

One example is the taskRun pod can include the pipeline name and task name in the logs to make it easier to filter these logs

[tejal29]

Are you also thinking of adding PipelineRun label to TaskRun in this task?

[nader-ziada]

It would make sense to add the PipelineRun name as a label on the TaskRun

[bobcatfish]

Okay I looked into our options a bit about how to reference child resources (e.g. a PipelineRun's TaskRuns, a TaskRun owns a Build) (slightly expanding the scope of this task 😅 ), looking at 4 options:

1. Labels (e.g. knative.dev/pipelinerun: kritis-pipeline-run-12321312984)
2. Using OwnerReferences
3. Using ObjectReferences
4. Using the names of the child resources

Labels

Inside a controller we can use the informer's Lister() to list by label:

	// Try getting the related TaskRuns by label, using the name of this PipelineRun
	selector := labels.SelectorFromSet(map[string]string{"knative.dev/pipelinerun": key})
	taskRuns, err := c.taskRunLister.TaskRuns(namespace).List(selector)
	if err != nil {
		c.Logger.Errorf("Error retrieving TaskRuns by label for %s", key)
	} else {
		for _, tr := range taskRuns {
			c.Logger.Infof("THIS IS MY TASK RUN %v", tr.GetName())
		}
	}

OwnerReferences

As far as I can tell it's not possible to query for all objects with a particular owner reference. I found these two thread requesting this feature:

• Provide capability to query all children resources kubernetes/kubernetes#54498
• kubectl get should have a way to filter for advanced pods status kubernetes/kubernetes#49387 (comment) (requests via FieldSelectors)

Which is unfortunate since this is pretty much exactly what we're looking for!

ObjectReferences

I completely forgot that in our PipelineRun schema, we actually already have references to the created TaskRuns:

https://github.com/knative/build-pipeline/blob/f9d5590b53aede86384259a8a4d15adec2002a81/pkg/apis/pipeline/v1alpha1/pipelinerun_types.go#L70

The go type is a bit incomplete, but in the yaml you can see what we were getting at:

Names

• Since TaskRuns and Builds have a 1:1 mapping, it probably makes sense to use the same name for both
• In PipelineRuns, we can construct a deterministic name by combining the name of the PipelineRun and the name of the Task inside the referenced Pipeline

So we could query for the child resources by name, though it seems that fuzzy or regex based searching isn't clearly possible (apparently kubernetes/kubernetes#9248 (comment) that's what labels are for 🤣) - even though it is actually supported by the describe command.

@tejal29 pointed out there is some complication here tho - what do we want to do if the same PipelineRun or TaskRun (i.e. with the same name) is invoked? To work around that we could add a random element to the names, or alternatively we could require the Runs have unique names.

Conclusion

I suggest that when retrieving child resources in the PipelineRun controllers, we use references to TaskRuns in the Status, since we'll need those anyway (the controller will need to know which it has started).

I think sticking to using the same name for the TaskRun and its child Build (which @aaron-prindle is already doing).

But as @pivotal-nader-ziada pointed out, it can be handy for the controllers when logging to know about what created them, and @tejal29 also pointed out it's handy on the command line to retrieve all related TaskRuns for example, so I think we should add:

• PipelineRun:
  • knative.dev/pipeline - can added by PipelineRun controller
• TaskRun:
  • knative.dev/pipeline - if this was triggered by a PipelineRun, the related pipeline
  • knative.dev/pipelineRun - if this was triggered by a PipelineRun
• Build:
  • knative.dev/pipeline - if this was triggered by a PipelineRun, the related pipeline
  • knative.dev/pipelineRun - if this was triggered by a PipelineRun
  • knative.dev/taskRun - if this was triggered by a TaskRun (even tho the name will be the same)

I'll add these to the existing controllers as they get to the point where they can be added, unless they are added by folks working on those controllers before I get a chance.

(let me know if you want any more added!)

[bobcatfish]

I'm gonna unassign myself from this since I haven't gotten a chance to work on it: the work to do here is to add the labels as described in the previous comment.

[vdemeester]

/assign @tanner-bruce

[knative-prow-robot]

@vdemeester: GitHub didn't allow me to assign the following users: tanner-bruce.

Note that only knative members and repo collaborators can be assigned.
For more information please see the contributor guide

In response to this:

/assign @tanner-bruce

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.