Fix logs and describe for Pipelines-in-Pipelines - #2889
Conversation
Resolve child PipelineRun TaskRuns recursively in tkn pipelinerun logs and describe. TaskRuns from child PipelineRuns are now displayed with a ">" prefix (e.g., "call-child > greet") to indicate nesting depth. This works for any level of nesting and supports both available and follow/live modes. Implementation Details: - Logs: recursive getOrderedTasks/getChildOrderedTasks and recursive GetTaskRunsWithStatus tracking. - Describe: replaced direct ChildReferences iteration with GetTaskRunsWithStatus for automatic PinP resolution.
tekton-robot
added
the
release-note
|
|
tekton-robot
added
the
size/XXL
|
Hi @twoGiants , this PR is ready for review. Kindly review it when you have time. |
danielfbm
left a comment
danielfbm
left a comment
There was a problem hiding this comment.
Did some basic review here and left comments inline. These are mostly suggestions from my part, and does not replace a review from maintainers, just flagging what I found here.
Good work overal @kamal2730 👍
I will play with your branch to see how it works paired with tektoncd/pipeline#9432
| } | ||
| childPRs[cr.PipelineTaskName] = childPR | ||
| } | ||
| } |
There was a problem hiding this comment.
This loop re-fetches every child PipelineRun from the API, but GetTaskRunsWithStatus on line 242 already does that recursively (see pkg/pipelinerun/tracker.go getTaskRunsWithStatusRecursive, case "PipelineRun"). For each child PR we now issue 2 GETs per call to getOrderedTasks, and the cost compounds with nesting depth.
Consider either: (a) dropping this loop and looking up child PRs via the prefixed entries already in trsMap, or (b) having GetTaskRunsWithStatus return the fetched child PR objects alongside the TR status map so we fetch once. Functionally fine; perf/clarity smell.
There was a problem hiding this comment.
this nit is still pending. getOrderedTasks calls GetTaskRunsWithStatus at :243, which already recurses through every child PipelineRun (getTaskRunsWithStatusRecursive, case "PipelineRun"). The loop at :249 then re-fetches each child PR again via GetPipelineRun → getChildOrderedTasks → getOrderedTasks → GetTaskRunsWithStatus, so each child PR is fetched at least twice and it compounds with nesting depth. Functionally correct (and the IsNotFound → continue you added here is a nice touch 👍), so basically an extra-round-trips smell. If it's easy, either look the child PRs up from the entries already in trsMap/childPRNames, or have GetTaskRunsWithStatus hand back the fetched child-PR objects so we fetch once. It would be great if we could fix this, wdys?
| if pr.Spec.PipelineRef.Resolver != "" { | ||
| if pr.Status.PipelineSpec != nil { | ||
| tasks = append(tasks, pr.Status.PipelineSpec.Tasks...) | ||
| tasks = append(tasks, pr.Status.PipelineSpec.Finally...) |
There was a problem hiding this comment.
This is the actual functional fix in this file — adding Finally to the resolver branch — but no test exercises it. All current PinP log tests use embedded PipelineSpec or non-resolver PipelineRef. Recommend adding TestPipelineRunLogs_ResolverPipeline_WithFinally: a parent PR with Spec.PipelineRef.Resolver != "" and Status.PipelineSpec.Finally populated, asserting finally logs appear after task logs.
| return nil, err | ||
| } | ||
| ordered = append(ordered, childTasks...) | ||
| } |
There was a problem hiding this comment.
Implicit else branch: if a tasks entry matches neither trNames nor childPRs, it is silently dropped. This was effectively the prior behavior of SortTasksBySpecOrder, but the new control flow makes it easier to introduce a regression where a misnamed child key drops the task without any signal. Worth either (a) logging at debug level, or (b) adding a test that confirms a pending/not-yet-started child task is dropped cleanly without panic, so the behavior is locked in.
| } | ||
| trStatuses[cr.Name] = &v1.PipelineRunTaskRunStatus{ | ||
| PipelineTaskName: cr.PipelineTaskName, | ||
| PipelineTaskName: taskName, |
There was a problem hiding this comment.
Behavior regression for nested retries caused by this prefixing. findNewTaskruns (tracker.go:183, outside this diff) does if trs.PipelineTaskName == pipelineTask.Name { retries = pipelineTask.Retries }. With the new prefix, trs.PipelineTaskName for any nested TR becomes "call-child > greet", while pipelineTask.Name there is "call-child" (the parent's pipeline task). They will never match, so retries is always 0 for any TaskRun inside a child PipelineRun.
Result: in live/follow mode, retries on nested TaskRuns are not tracked and retry log streams may be missed. The PR description claims follow mode is supported for PinP, so this should be exercised. Suggested fix: when trs.PipelineTaskName contains " > ", look up the retries from the corresponding child PR's Status.PipelineSpec.Tasks (you already fetch those PRs in this function).
Also: no test asserts retry counts for PinP children — add TestFindNewTaskruns_PinP_RetryCount.
| case "PipelineRun": | ||
| childPR, err := GetPipelineRun(pipelineRunGroupResource, c, cr.Name, ns) | ||
| if err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
If GetPipelineRun fails here (child PR not yet visible to informer cache, RBAC issue, network glitch), the entire GetTaskRunsWithStatus call returns an error and both tkn pipelinerun logs and tkn pipelinerun describe fail with no partial output. No test simulates this. In live mode especially, this is plausible — the parent PR may reference a child that hasn't propagated yet. Consider either (a) tolerating NotFound (skip the child for this pass), or (b) at minimum adding a test that pins the fail-fast behavior so it's an explicit choice.
| TaskRunName: trName, | ||
| PipelineTaskName: trs.PipelineTaskName, | ||
| Status: trs.Status, | ||
| }) |
There was a problem hiding this comment.
Downstream sort.Sort(taskRunList) (description.go:192) relies on TaskRunWithStatusList.Less, which dereferences trs[i].Status.StartTime. Child TaskRuns fetched via the new recursive path may not yet have a StartTime if the child PR is still starting. The comparator handles nil StartTime via early returns, but no test exercises a PinP describe where a nested TR has nil Status.StartTime. Recommend a TestPipelineRunDescribe_PinP_NilStartTime to lock ordering behavior.
| for _, pt := range tasks { | ||
| if _, ok := trNames[pt.Name]; ok { | ||
| // Direct TaskRun child — use existing sort logic | ||
| ordered = append(ordered, taskrunpkg.SortTasksBySpecOrder([]v1.PipelineTask{pt}, trsMap)...) |
There was a problem hiding this comment.
--task filter breaks for PinP tasks. readAvailablePipelineLogs calls taskrunpkg.Filter(ordered, r.tasks), which does exact-string matching against tr.Task (taskrun.go:74 — if filter[tr.Task]). With this PR, Run.Task is now a prefixed string like "call-child > greet", so:
| # | User runs | Result |
|---|---|---|
| 1 | tkn pr logs --task "call-child > greet" |
works |
| 2 | tkn pr logs --task greet |
silently drops all child logs |
| 3 | tkn pr logs --task call-child |
silently drops all child logs |
| 4 | no --task flag |
works (all PinP tests cover this) |
The error message for case 2/3 will be passed filtered tasks: [greet] is not available, available tasks are: [call-child > greet], which is confusing UX. The same issue affects live-follow via taskrunpkg.IsFiltered (taskrun.go:50).
At minimum, add a test covering case 1 and document the required parent > child format. Better: change Filter to a substring/suffix match for PinP entries, so --task greet matches "call-child > greet". Wdys?
There was a problem hiding this comment.
Follow-up on the filter fix (works well now for parent-prefix and leaf 👍): for deeper nesting a > b > c, a middle segment --task b still drops, and --task "a > b" only matches exactly (not as a prefix of a > b > c), since Filter checks prefix + leaf only. Probably fine to leave as-is, just worth a one-line doc note on the --task format for PinP. Wdys? Not blocking in my opinion.
| return nil, err | ||
| } | ||
| for i := range childOrdered { | ||
| childOrdered[i].Task = parentTaskName + " > " + childOrdered[i].Task |
There was a problem hiding this comment.
DisplayName is not prefixed alongside Task. setUpTask (pipeline_reader.go:211) passes tr.DisplayName straight to r.setDisplayName, and that ends up in Log.TaskDisplayName (line 195). If a child task defines displayName, users will see an unprefixed display name next to a prefixed task name — the opposite of the consistency this PR is trying to establish.
| childOrdered[i].Task = parentTaskName + " > " + childOrdered[i].Task | |
| for i := range childOrdered { | |
| childOrdered[i].Task = parentTaskName + " > " + childOrdered[i].Task | |
| if childOrdered[i].DisplayName != "" { | |
| childOrdered[i].DisplayName = parentTaskName + " > " + childOrdered[i].DisplayName | |
| } | |
| } |
Also worth adding a test where a child PipelineTask carries DisplayName to lock the format.
|
|
||
| taskName := cr.PipelineTaskName | ||
| if prefix != "" { | ||
| taskName = prefix + " > " + taskName |
There was a problem hiding this comment.
SUGGESTION: The separator " > " is now a magic string in three production sites: tracker.go:236, tracker.go:249, and pipeline_reader.go:284. If the display format ever changes (/, |, indentation), the three sites plus the golden file TestPipelineRunDescribe_PinP.golden all need to be touched in lockstep. Extracting a constant keeps them aligned:
// In pkg/pipelinerun/tracker.go top-level:
// ChildTaskSeparator separates parent and child pipeline task names in
// Pipelines-in-Pipelines describe/logs output (e.g. "call-child > greet").
const ChildTaskSeparator = " > "Then replace the three literals with pipelinerunpkg.ChildTaskSeparator / ChildTaskSeparator. Optional but cheap.
| } | ||
| return ordered, nil | ||
| } | ||
| func (r *Reader) getChildOrderedTasks(childPR *v1.PipelineRun, parentTaskName string) ([]taskrunpkg.Run, error) { |
There was a problem hiding this comment.
nit: missing blank line between getOrderedTasks and getChildOrderedTasks. Every other top-level function pair in this file is separated by a blank line.
| func (r *Reader) getChildOrderedTasks(childPR *v1.PipelineRun, parentTaskName string) ([]taskrunpkg.Run, error) { | |
| return ordered, nil | |
| } | |
| func (r *Reader) getChildOrderedTasks(childPR *v1.PipelineRun, parentTaskName string) ([]taskrunpkg.Run, error) { |
|
[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 |
|
@danielfbm Thanks for a very detailed review, I addressed most of them, review once again if you have time. |
There was a problem hiding this comment.
Pull request overview
This PR adds Pipelines-in-Pipelines (PinP) support to tkn pipelinerun logs and tkn pipelinerun describe by recursively resolving TaskRuns from child PipelineRuns and displaying nested tasks using a > prefix.
Changes:
- Recursively collect TaskRuns across child PipelineRuns (
GetTaskRunsWithStatus) and use it indescribe. - Update pipeline log task ordering to inline child PipelineRun tasks with prefixed names, including
finallytasks for resolver-based PipelineRuns. - Extend task filtering logic and add substantial test coverage + golden outputs for PinP scenarios.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/taskrun/taskrun.go | Adds ChildTaskSeparator and extends Filter to handle nested task names. |
| pkg/taskrun/taskrun_test.go | Adds unit tests for PinP-aware filtering behavior. |
| pkg/pipelinerun/tracker.go | Implements recursive TaskRun status resolution and PinP-aware retry lookup (but see comment about deep nesting retries). |
| pkg/pipelinerun/tracker_test.go | Adds tests for recursive TaskRun discovery, deep nesting, missing child PRs, and retry behavior. |
| pkg/pipelinerun/description.go | Switches describe output to use recursive GetTaskRunsWithStatus so PinP TaskRuns appear. |
| pkg/log/pipeline_reader.go | Orders tasks to inline child PipelineRun tasks and includes Finally for resolver-based PipelineRuns. |
| pkg/cmd/pipelinerun/testdata/TestPipelineRunDescribe_PinP.golden | New golden output for PinP describe. |
| pkg/cmd/pipelinerun/testdata/TestPipelineRunDescribe_PinP_NilStartTime.golden | New golden output for PinP describe with nil TaskRun start time. |
| pkg/cmd/pipelinerun/logs_test.go | Adds log tests for PinP (available mode) and resolver+finally coverage. |
| pkg/cmd/pipelinerun/describe_v1_test.go | Adds describe tests covering PinP and nil start time formatting. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if strings.Contains(trs.PipelineTaskName, ChildTaskSeparator) { | ||
| segments := strings.SplitN(trs.PipelineTaskName, ChildTaskSeparator, 2) | ||
| parentTaskName := segments[0] | ||
| childTaskName := segments[1] | ||
| for _, cr := range pr.Status.ChildReferences { |
| // ChildTaskSeparator separates parent and child pipeline task names in Pipelines-in-Pipelines describe/logs output. | ||
| const ChildTaskSeparator = " > " | ||
|
|
There was a problem hiding this comment.
Thanks for the follow-up here, @kamal2730. Pulled your latest edits and it now resolves Copilot's points 🐱 👍
This pass is purely about test quality:
- Pattern conformance: seeding (
test.SeedTestData+pipelinetest.Data+cb.Unstructured*), golden-file usage in describe,test.AssertOutputin logs, naming, and version/parallel structure all match the existing siblings. No inconsistencies. - Duplication:
TestPipelineRunLogs_PinP_DisplayNameis still a ~100-line clone of..._PinP_Available, and the parent / child-PR / child-TR / parent-Pipeline "quartet" is now hand-built inline in 8 test functions (the new deep-nesting retry test added another). A singlenewPinPFixturebuilder + small decorators collapses both. Details inline. - Coverage: the symmetric non-PinP retry arm (
findNewTaskruns'else if pr.Status.PipelineSpec != nil) still isn't asserted anywhere withRetries > 0. Inline.
Thanks again for the work on this one 👍
| test.AssertOutput(t, strings.Join(expectedLogs, "\n")+"\n", output) | ||
| } | ||
|
|
||
| func TestPipelineRunLogs_PinP_DisplayName(t *testing.T) { |
There was a problem hiding this comment.
This function is a ~100-line near-clone of TestPipelineRunLogs_PinP_Available (logs_test.go:4890). The fixtures are byte-for-byte identical; the only deltas are DisplayName: "my-display" on the child task (:5697), prlo.Long = true (:5765), and the expected string (:5770).
Classic decorate, don't clone. If the success-path fixture is extracted (see the shared-builder comment on tracker_test.go), this whole test collapses to a few lines:
func TestPipelineRunLogs_PinP_DisplayName(t *testing.T) {
f := newPinPFixture(ns)
f.childTask().DisplayName = "my-display"
// seed, run with Long=true, assert "[call-child > my-display : echo] ..."
}Wdys?
There was a problem hiding this comment.
Much better with the pinPFixture + succeededTR. There's still a little object-level duplication (the parent/child PR + parent Pipeline quartet is built inline across the PinP tests, differing by one field), which is acceptable in my opinion. Will need the maintainer's opinion on this one.
| } | ||
| } | ||
|
|
||
| func TestFindNewTaskruns_PinP_DeepNestingWithRetry(t *testing.T) { |
There was a problem hiding this comment.
IMPORTANT (maintainability): the parent → child-PipelineRun → child-TaskRun (+ parent Pipeline) "quartet" is hand-built inline across 8 test functions now (this new deep-nesting case added another), with the same ChildReferences: [{Kind: "PipelineRun", PipelineTaskName: ...}] block retyped each time:
logs_test.go:PinP_Available(4890),PinP_DisplayName(5671),PinP_PendingChild(5229),PinP_ChildPR_NotFound(5773)describe_v1_test.go:PinP_NilStartTime(2875)tracker_test.go:RetryCount(623),DeepNestingWithRetry(742),ChildPR_NotFound(867)
There's no shared PinP fixture in the repo today. Recommend one comprehensive builder for the success-path quartet, then each test mutates only the field under test:
// returns the canonical parent PR + child PR + child TR + parent Pipeline
func newPinPFixture(ns, parentTask, childTask string) (parentPR, childPR *v1.PipelineRun, childTR *v1.TaskRun, parentPipe *v1.Pipeline)That removes the DisplayName clone and the 8-way ChildReferences repetition in one move. Happy to sketch it if useful.
There was a problem hiding this comment.
Same here — the new pinPFixture builder + succeededTR cut the bulk of the setup duplication. There is a remaining inline quartet across the tests that is mostly cosmetic in my opinion; not sure if worth another round (need maintainers input here @vdemeester @twoGiants).
| }, | ||
| } | ||
|
|
||
| trs := []*v1.TaskRun{ |
There was a problem hiding this comment.
nit: this "succeeded TaskRun, one terminated step, ExitCode: 0" status block is repeated nearly verbatim across the PinP tests (e.g. PinP_Available :4902/:4908, here in PinP_PendingChild, and the resolver trs entries). A small helper keeps the ConditionSucceeded / ExitCode:0 shape from drifting between copies:
func succeededTR(name, ns, pod, step string) *v1.TaskRun| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| test.AssertOutput(t, "[call-child > my-display : echo] Hello from child\n\n", output) |
There was a problem hiding this comment.
nit: the " > " separator is re-typed in this expected string, but the production const taskrun.ChildTaskSeparator already exists (now the single source of truth after 54386dc). Low priority since this is the literal asserted CLI output — but formatting the expectation through the const (e.g. fmt.Sprintf("[%s%smy-display : echo] ...", "call-child", taskrun.ChildTaskSeparator)) keeps test and production in lockstep if the separator ever changes.
| "testing" | ||
| ) | ||
|
|
||
| func TestFilter_Empty_ReturnsAll(t *testing.T) { |
There was a problem hiding this comment.
nit: TestFilter_* is 11 near-identical functions (:21–:102) that differ only by input slice / filter args / expected count — a classic table-driven case. The repo uses both styles, but for purely mechanical cases like this the table-driven form is the more idiomatic fit and drops the boilerplate:
tests := []struct {
name string
trs []Run
filter []string
want int
}{ /* ... */ }
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) { /* assert len(Filter(tt.trs, tt.filter)) == tt.want */ })
}Would love to hear if the maintainers have a preference here — wdys @vdemeester @twoGiants ?
| @@ -0,0 +1,121 @@ | |||
| // Copyright © 2019 The Tekton Authors. | |||
There was a problem hiding this comment.
nit: this file is new (2026), but the header says 2019.
| // Copyright © 2019 The Tekton Authors. | |
| // Copyright © 2026 The Tekton Authors. |
| } | ||
| } | ||
|
|
||
| func TestFindNewTaskruns_PinP_RetryCount(t *testing.T) { |
There was a problem hiding this comment.
Nicely done — Retries: 2 is non-default, and the assertion fails if the separator branch is reverted (control would fall to the non-PinP else if, leaving retries 0). 👍
IMPORTANT (coverage): the symmetric non-PinP path still isn't asserted anywhere with a non-zero value. findNewTaskruns' else if pr.Status.PipelineSpec != nil arm (tracker.go:214 — retries read from the parent spec) is only reached by TestTracker_pipelinerun_complete, which leaves Status.PipelineSpec nil — so that sub-branch never runs with Retries > 0. It's pre-existing logic this PR re-indented into the else if, so not a regression — but since you're adding child-retry tests, a sibling case with a non-PinP task whose parent Status.PipelineSpec.Tasks[x].Retries = N asserting retries == N would lock it in. Wdys?
danielfbm
left a comment
danielfbm
left a comment
There was a problem hiding this comment.
Left a few comments and, from what I see the work is mostly done 👍. I don't have the permissions to resolve the threads so I just added replies with some notes asking for project's maintainers opinion. How can we move this forward @vdemeester @twoGiants?
| }, | ||
| } | ||
|
|
||
| trs := []*v1.TaskRun{ |
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| test.AssertOutput(t, "[call-child > my-display : echo] Hello from child\n\n", output) |
| "testing" | ||
| ) | ||
|
|
||
| func TestFilter_Empty_ReturnsAll(t *testing.T) { |
| @@ -0,0 +1,121 @@ | |||
| // Copyright © 2019 The Tekton Authors. | |||
| } | ||
| } | ||
|
|
||
| func TestFindNewTaskruns_PinP_RetryCount(t *testing.T) { |
| } | ||
| childPRs[cr.PipelineTaskName] = childPR | ||
| } | ||
| } |
There was a problem hiding this comment.
this nit is still pending. getOrderedTasks calls GetTaskRunsWithStatus at :243, which already recurses through every child PipelineRun (getTaskRunsWithStatusRecursive, case "PipelineRun"). The loop at :249 then re-fetches each child PR again via GetPipelineRun → getChildOrderedTasks → getOrderedTasks → GetTaskRunsWithStatus, so each child PR is fetched at least twice and it compounds with nesting depth. Functionally correct (and the IsNotFound → continue you added here is a nice touch 👍), so basically an extra-round-trips smell. If it's easy, either look the child PRs up from the entries already in trsMap/childPRNames, or have GetTaskRunsWithStatus hand back the fetched child-PR objects so we fetch once. It would be great if we could fix this, wdys?
| for _, pt := range tasks { | ||
| if _, ok := trNames[pt.Name]; ok { | ||
| // Direct TaskRun child — use existing sort logic | ||
| ordered = append(ordered, taskrunpkg.SortTasksBySpecOrder([]v1.PipelineTask{pt}, trsMap)...) |
There was a problem hiding this comment.
Follow-up on the filter fix (works well now for parent-prefix and leaf 👍): for deeper nesting a > b > c, a middle segment --task b still drops, and --task "a > b" only matches exactly (not as a prefix of a > b > c), since Filter checks prefix + leaf only. Probably fine to leave as-is, just worth a one-line doc note on the --task format for PinP. Wdys? Not blocking in my opinion.
| test.AssertOutput(t, strings.Join(expectedLogs, "\n")+"\n", output) | ||
| } | ||
|
|
||
| func TestPipelineRunLogs_PinP_DisplayName(t *testing.T) { |
There was a problem hiding this comment.
Much better with the pinPFixture + succeededTR. There's still a little object-level duplication (the parent/child PR + parent Pipeline quartet is built inline across the PinP tests, differing by one field), which is acceptable in my opinion. Will need the maintainer's opinion on this one.
| } | ||
| } | ||
|
|
||
| func TestFindNewTaskruns_PinP_DeepNestingWithRetry(t *testing.T) { |
There was a problem hiding this comment.
Same here — the new pinPFixture builder + succeededTR cut the bulk of the setup duplication. There is a remaining inline quartet across the tests that is mostly cosmetic in my opinion; not sure if worth another round (need maintainers input here @vdemeester @twoGiants).
| ordered = append(ordered, childTasks...) | ||
| } | ||
| } | ||
| return ordered, nil |
There was a problem hiding this comment.
BLOCKER (regression): this silently reverts 187a8358: "Fix TaskRun order in tkn pr logs", Fixes #1372 for all pipelines, not just PinP ones.
The chain: main called SortTasksBySpecOrder(tasks, trsMap) once with the whole tasks slice, and that function ends with sort.Sort(trs) (taskrun.go:123) which orders by CompletionTime. This version calls it at :284 with []v1.PipelineTask{pt}, a one-element slice, so sort.Sort sorts a single element and is a no-op. Nothing re-sorts afterwards (readAvailablePipelineLogs :117 just iterates in order), so log output is now in pipeline-spec declaration order.
I verified it with a plain, non-PinP 2-task pipeline: spec order [alpha, beta], beta completes at t+10s, alpha at t+60s.
| branch | output |
|---|---|
main |
[beta : sb] BBB → [alpha : sa] AAA: completion order |
| this branch | [alpha : sa] AAA → [beta : sb] BBB: spec order |
No existing test catches it because the test added with #1372 only has a single TaskRun with times set, so there's nothing to order.
Sorting the fully-assembled list restores the old semantics and extends them to PinP (child Runs already carry StartTime/CompletionTime, since getChildOrderedTasks only rewrites Task/DisplayName):
| return ordered, nil | |
| sort.Sort(taskrunpkg.Runs(ordered)) | |
| return ordered, nil |
(plus "sort" in the import block).
I applied exactly this locally and ran every PinP test included, worked fine.
Worth a regression test too, since the suite currently has nothing covering this here:
| # | Case | Test today |
|---|---|---|
| 1 | 2 parallel tasks, completion order ≠ spec order | none |
| 2 | PinP child tasks interleaved with parent tasks by completion time | none |
| 3 | single task (degenerate) | covered |
Note this only affects the available (non-follow) path. readLivePipelineLogs fans out goroutines and never ordered anything. Wdys?
| &tr.Status, | ||
| }) | ||
| } | ||
| for trName, trs := range trStatuses { |
There was a problem hiding this comment.
BLOCKER (regression): swapping the ChildReferences slice for the trStatuses map makes tkn pr describe output nondeterministic.
Go randomises map iteration order, and sort.Sort at :192 is not stable. TaskRunWithStatusList.Less (:160, just above the hunk so I can't anchor there) compares StartTime only, so any two TaskRuns with an equal StartTime tie, and the printed order is whatever the map handed over. Equal StartTime is not exotic: metav1.Time serialises at second granularity, so sibling TaskRuns started in the same second collide routinely.
Ran desc 50× against two sibling TaskRuns sharing a StartTime:
| branch | distinct orderings over 50 runs |
|---|---|
main |
1 ✅ (ChildReferences slice order is stable) |
| this branch | 2 ❌ — run-aaa,run-bbb × 41, run-bbb,run-aaa × 9 |
Neither TestPipelineRunDescribe_PinP nor ..._NilStartTime catches it, both have exactly one TaskRun.
sort.SliceStable alone won't help (the input order is already random); the tie has to be broken inside Less. This preserves the current ordering and nil-handling and just adds a deterministic tiebreaker:
func (trs TaskRunWithStatusList) Less(i, j int) bool {
iNil := trs[i].Status == nil || trs[i].Status.StartTime == nil
jNil := trs[j].Status == nil || trs[j].Status.StartTime == nil
switch {
case iNil && jNil:
return trs[i].TaskRunName < trs[j].TaskRunName
case jNil:
return false
case iNil:
return true
}
if trs[i].Status.StartTime.Equal(trs[j].Status.StartTime) {
return trs[i].TaskRunName < trs[j].TaskRunName
}
return trs[j].Status.StartTime.Before(trs[i].Status.StartTime)
}Verified locally, works on 50x as well.
A two-TaskRun-same-StartTime describe test would lock it in. Wdys?
| trNames[t.PipelineTaskName] = name | ||
| } | ||
| // Pre-fetch child PipelineRuns if not already cached | ||
| if childPRCache == nil { |
There was a problem hiding this comment.
Thanks for the new changes appreciate the pass at this 🙏. Adding to myy earlier thread here since the shape of the problem changed a bit.
The cache is inert. childPRCache is never passed a non-nil value, so it's always rebuilt from scratch:
getOrderedTasks(pr)→getOrderedTasksRec(pr, nil)- recursion:
getChildOrderedTasks(:298) →getOrderedTasks(childPR)→getOrderedTasksRec(childPR, nil)
Both entry points pass nil, so the parameter is effectively dead code today.
if childPRCache == nil guard at :267 would skip the child's own pre-fetch entirely, and then:
- grandchildren would never be found → nested output silently truncated at depth 2, and
- if a parent and child pipeline happen to share a task name,
childPRCache[pt.Name]at :286 would recurse into the wrong PipelineRun.
The original duplication is still there, and it compounds. GetTaskRunsWithStatus at :250 already walks and fetches every descendant PipelineRun (tracker.go getTaskRunsWithStatusRecursive, case "PipelineRun"), and then this loop fetches the direct children again, and then each recursion level repeats the whole thing. For a 3-level chain a > b > c that's 5 GetPipelineRun calls for 2 distinct child PRs:
F(pr) = descendants(pr) [inside GetTaskRunsWithStatus]
+ direct-children(pr) [this pre-fetch loop]
+ Σ F(child)
F(P) = 2 + 1 + F(C1); F(C1) = 1 + 1 + F(C2); F(C2) = 0 → F(P) = 5
(and the TaskRun GETs duplicate the same way). If anything the pre-fetch is now slightly hungrier than before, since it walks all of childPRNames unconditionally rather than only the ones present in tasks.
Two ways to actually collapse it to a single fetch:
- (a) look the child PRs up from the entries already resolved in
trsMap, or - (b) have
GetTaskRunsWithStatusreturn the child-PR objects it fetched alongside the status map, and reuse those here.
(b) is more invasive but kills the duplication at the root for both logs and describe. Still not blocking from my side, but if it isn't going to be fixed in this PR, I would rather see childPRCache dropped entirely and getOrderedTasksRec folded back into getOrderedTasks, so nobody inherits a parameter that looks like an optimisation but isn't. Wdys?
| case "PipelineRun": | ||
| childPR, err := GetPipelineRun(pipelineRunGroupResource, c, cr.Name, ns) | ||
| if err != nil { | ||
| if apierrors.IsNotFound(err) { |
There was a problem hiding this comment.
question: the IsNotFound → continue here is a nice touch for the pending-child case 👍, but it's asymmetric with the TaskRun arm 12 lines up (:264-266), where any error, including NotFound, aborts the entire call.
So in a nested tree: a child PipelineRun that hasn't propagated yet degrades gracefully, but a single TaskRun that was pruned or GC'd makes the whole tkn pr logs / tkn pr describe fail with no partial output. That asymmetry gets more likely the deeper the tree, since there are more TaskRuns than child PRs.
Was that deliberate? If yes, no objection, just worth a one-line comment saying so. If not, the same IsNotFound → continue on the TaskRun arm would make partial output the consistent behaviour on both paths.
| for _, cr := range currentPR.Status.ChildReferences { | ||
| if cr.Kind == "PipelineRun" && cr.PipelineTaskName == segments[i] { | ||
| childPR, err := GetPipelineRun(pipelineRunGroupResource, t.Client, cr.Name, t.Ns) | ||
| if err == nil { |
There was a problem hiding this comment.
nit: the if err == nil swallows the failure and leaves currentPR pointing at the previous level, then the loop carries on to segments[i+1] as if the hop had succeeded.
Usually this is ok, you end up back at pr, currentPR != pr is false at :199, and retries stays 0. But on a partial failure in a deeper chain (a > b > c, hop to b fails, hop to c then matched against a's children) you could read Retries off the wrong PipelineSpec rather than falling back to 0.
Since a failed lookup here can only ever make the retry count less correct, breaking out of the segment walk on error would make the fallback explicit:
childPR, err := GetPipelineRun(pipelineRunGroupResource, t.Client, cr.Name, t.Ns)
if err != nil {
// can't resolve the chain; leave retries at 0 rather than guessing
currentPR = pr
break
}
currentPR = childPRLow priority, mostly about making the intent legible, since a bare if err == nil reads like an oversight even when it isn't.
| for _, t := range ts { | ||
| if strings.Contains(t, ChildTaskSeparator) { | ||
| continue | ||
| } | ||
| segments := strings.Split(tr.Task, ChildTaskSeparator) | ||
| if len(segments) <= 1 { | ||
| continue | ||
| } |
There was a problem hiding this comment.
nit: segments is loop-invariant, it's recomputed from tr.Task on every iteration of the ts loop, and the len(segments) <= 1 guard is really a property of tr, not of t. Hoisting both out lets a non-PinP tr.Task skip the inner loop entirely instead of splitting once per filter term:
| for _, t := range ts { | |
| if strings.Contains(t, ChildTaskSeparator) { | |
| continue | |
| } | |
| segments := strings.Split(tr.Task, ChildTaskSeparator) | |
| if len(segments) <= 1 { | |
| continue | |
| } | |
| segments := strings.Split(tr.Task, ChildTaskSeparator) | |
| if len(segments) <= 1 { | |
| continue | |
| } | |
| for _, t := range ts { | |
| if strings.Contains(t, ChildTaskSeparator) { | |
| continue | |
| } |
Behaviour is identical; just fewer allocations on the common (non-nested) path.
Changes
Resolve child PipelineRun TaskRuns recursively in tkn pipelinerun logs and describe. TaskRuns from child PipelineRuns are now displayed with a ">" prefix (e.g., "call-child > greet") to indicate nesting depth. This works for any level of nesting and supports both available and follow/live modes.
Implementation Details:
Fixes #2886
Submitter Checklist
These are the criteria that every PR should meet, please check them off as you
review them:
make checkmake generatedSee the contribution guide
for more details.
Release Notes