feat: Add support for PipelineRef in Pipeline in pipeline#9432
feat: Add support for PipelineRef in Pipeline in pipeline#9432danielfbm wants to merge 1 commit intotektoncd:mainfrom
Conversation
- This commit adds full support for referencing a pipelien inside another pipeline definition including using pipeline resolvers. - Single level and multi level cycle cases are all treated during pipeline execution, failing with explicit error message
|
/kind feature |
|
cc @twoGiants |
twoGiants
left a comment
There was a problem hiding this comment.
Thank you for your contribution @danielfbm and welcome to Tekton 😸 👍
I am very happy that you took on this important task of continuing the implementation of the pipeline in pipeline functionality. Great job 🏅
I enjoyed reviewing your PR.
Please add e2e tests once you finished the implementation.
See all my comments below and an additional question out of curiosity => how much was done by Claude Code giving the branch name danielfbm:feat/pinp-ref-claude ?
Keep up the good work and let me know if there are any questions. Write on Slack if I don't answer here for to long.
| func GetChildPipelineFunc(ctx context.Context, k8s kubernetes.Interface, tekton clientset.Interface, requester remoteresource.Requester, pipelineRun *v1.PipelineRun, pipelineRef *v1.PipelineRef, verificationPolicies []*v1alpha1.VerificationPolicy) rprp.GetPipeline { | ||
| namespace := pipelineRun.Namespace | ||
|
|
||
| switch { |
There was a problem hiding this comment.
You can improve the existing code as you go. The switch/case is not needed. An if-condition is enough.
| } | ||
| resolver := resolution.NewResolver(requester, pipelineRun, string(pipelineRef.Resolver), resolverPayload) | ||
| return resolvePipeline(ctx, resolver, name, namespace, k8s, tekton, verificationPolicies) | ||
| } |
There was a problem hiding this comment.
This is mostly duplicated from GetPipelineFunc and can be refactored removing duplication.
| // GetChildPipelineFunc is a factory function that will use the given PipelineRef from a PipelineTask | ||
| // to return a valid GetPipeline function that looks up the pipeline. It handles both local cluster | ||
| // and remote resolution. | ||
| func GetChildPipelineFunc(ctx context.Context, k8s kubernetes.Interface, tekton clientset.Interface, requester remoteresource.Requester, pipelineRun *v1.PipelineRun, pipelineRef *v1.PipelineRef, verificationPolicies []*v1alpha1.VerificationPolicy) rprp.GetPipeline { |
There was a problem hiding this comment.
New public API without tests, please add unit tests.
| vp, | ||
| ) | ||
|
|
||
| getPipelineFunc := resources.GetChildPipelineFunc( |
There was a problem hiding this comment.
| getPipelineFunc := resources.GetChildPipelineFunc( | |
| getChildPipelineFunc := resources.GetChildPipelineFunc( |
| pipelineMeta.Namespace, pipelineMeta.Name, nfErr) | ||
| } else if errors.As(err, &pnfErr) { | ||
| pr.Status.MarkFailed(v1.PipelineRunReasonCouldntGetPipeline.String(), | ||
| "Pipeline %s/%s can't be Run; it contains Pipelines that don't exist: %s", |
There was a problem hiding this comment.
| "Pipeline %s/%s can't be Run; it contains Pipelines that don't exist: %s", | |
| "Pipeline %s/%s can't be Run; it contains child Pipelines that don't exist: %s", |
| logger := logging.FromContext(ctx) | ||
| rpt.PipelineTask = resources.ApplyPipelineTaskContexts(rpt.PipelineTask, pr.Status, facts) | ||
|
|
||
| workspaces, _, err := c.getTaskrunWorkspaces(ctx, pr, rpt) |
There was a problem hiding this comment.
Why adding workspaces now? It's also a separate feature which should be implemented in a separate PR for PinP.
| } | ||
|
|
||
| // For PipelineRef tasks, detect and prevent pipeline-in-pipeline cycles. | ||
| childAnnotations := createChildResourceAnnotations(pr) |
There was a problem hiding this comment.
Great attempt! Appreciate the effort and that you thought about capturing cycles.
I see one core issue with this approach: it's fragile because it relies on annotations and which are mutable and comma separated pipeline names.
I see two options of doing it:
- Build a PinP dependency graph by resolving all
pipelineRefs recursively in the first reconcile cycle and reuse existing cycle detection logic. - Use the existing
ownerReferencesand thetekton.dev/pipelinelabel to walk up the tree up to the root in each child PipelineRun and look for reoccurringtekton.dev/pipelinelabels values or for. If you find one => you have a cycle so you can return an error.
Option 1
We have a findCyclesInDependencies in dag.go#L135. Which you could make public and reuse for cycle detection. For that you'd need to implement a recursive pipelineInPipelineDependencyGraphBuilder which walks all pipelineRef tasks recursively, resolves them, and constructs map[string][]string of Pipeline→Pipeline dependencies. Then you pass the graph to findCyclesInDependencies (needs to be made public) and do that before resolvePipelineState().
Would find cycles right away and error out.
Option 2
This one can find cycles much later, depending on how deep they are nested but is easier to implement.
BUT you'd need to fix a small bug in the current implementation for it. Currently createChildResourceLabels copies all parent labels to the child, including tekton.dev/pipeline. So if the parent is running "Pipeline A" and creates a child for "Pipeline B", the child PipelineRun gets tekton.dev/pipeline: Pipeline-A -- which is wrong. The child is running Pipeline B, not A. This needs a fix. I've added a comment below.
I lean towards Option 1. Wdys @danielfbm ? And what do you say @vdemeester and @afrittoli ?
There was a problem hiding this comment.
Agree with option 1, it seems a cleaner approach, even with the recursive resolving of pipelineRefs the solution seems more robust.
| Name: childPipelineRunName, | ||
| Namespace: pr.Namespace, | ||
| OwnerReferences: []metav1.OwnerReference{*kmeta.NewControllerRef(pr)}, | ||
| Labels: createChildResourceLabels(pr, rpt.PipelineTask.Name, true), |
There was a problem hiding this comment.
🐛 Bug 🐛
Currently createChildResourceLabels copies all parent labels to the child, including tekton.dev/pipeline. So if the parent is running "Pipeline A" and creates a child for "Pipeline B", the child PipelineRun gets tekton.dev/pipeline: Pipeline-A, which is wrong. The child is running Pipeline B, not A. This needs a fix.
|
|
||
| // For PipelineRef tasks, detect and prevent pipeline-in-pipeline cycles. | ||
| childAnnotations := createChildResourceAnnotations(pr) | ||
| if rpt.PipelineTask.PipelineRef != nil && rpt.ResolvedPipeline.PipelineName != "" { |
There was a problem hiding this comment.
This will be re-worked anyway but for the future => each condition needs tests covering it.
There was a problem hiding this comment.
A few more tests are missing. I am expecting:
- a happy path test with one
pipelineRef - a complex 2 and more levels deep nested
pipelineReftests - cycle detection tests
- error tests
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
| }, | ||
| Spec: v1.PipelineRunSpec{ | ||
| PipelineSpec: rpt.PipelineTask.PipelineSpec, | ||
| PipelineSpec: rpt.ResolvedPipeline.PipelineSpec, |
There was a problem hiding this comment.
Why also embedding in PipelineSpec for PipelineRefs? For PipelineRef you can use Spec.PipelineRef and the when the child pipelinerun will be reconciled this pipeline will be resolved. Then you don't need tekton.dev/pipeline label when searching for cycles but can use the pipeline name in Spec.PipelineRef. This would simplify Option 2 for cycle detection from above.
| // If the alpha feature is enabled, and the user has configured pipelineSpec or pipelineRef, it will enter here. | ||
| // Currently, the controller is not yet adapted, and to avoid a panic, an error message is provided here. | ||
| // TODO: Adjust the logic here once the feature is supported in the future. | ||
| return nil, fmt.Errorf("currently, Task %q does not support PipelineRef, please use PipelineSpec, TaskRef or TaskSpec instead", pipelineTask.Name) |
Thank you so much for the thoughtful review. I will do the changes accordingly.
Oops, caught red-handed 😄 it did most of the coding, but I did generate some testing data and script for it to validate the code against, which helped with the edge cases (although, the code is far from optimal). I wanted to do a full review before opening this PR but I got caught in some urgent work so I though would be best to get your opinion on this, sorry if the code is not per project standards. I will follow with your change suggestions and will update here as soon as it is done. |
Sure thing. Looking forward to the improvements 😸 👍
Appreciate the honesty!
This was the right intention. Follow your intention for your next contribution and the re-work on this PR (and I strongly encourage you to continue to contribute). This saves maintainers valuable time.
If you need my input on a problem you want to solve in Tekton in future contact me on Slack in the right channel or pick a way forward you think is good and open a well written and thoroughly, human reviewed PR. Review the code yourself and let Claude do a review too. You can actually ask Claude to scrutinize it's own code review and it probably will drop half of the issues it found.
No, it's alright and please review any code you contribute yourself thoroughly next time before submitting (as mentioned above). I'll appreciate it. Keep it up! 👍 |
another pipeline definition including using pipeline resolvers.
pipeline execution, failing with explicit error message
Changes
Submitter Checklist
As the author of this PR, please check off the items in this checklist:
/kind <type>. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tepRelease Notes