From 56ea14c02601b5373c2631726ca8637676521146 Mon Sep 17 00:00:00 2001 From: Maximilien Raulic Date: Fri, 19 Sep 2025 11:27:50 +0200 Subject: [PATCH 1/3] fix(pod): order TaskRun status steps to match pod spec containers The `TaskRun.Status.Steps` list was not guaranteed to reflect the true execution order when StepActions were involved. Steps populated earlier (from StepAction resolution) were left at the front, with inline steps appended later, causing a mismatch with the pod container order and confusing dashboards. This change fixes the issue by creating a temporary slice aligned with the (already sorted) pod step container sequence and then replace `trs.Steps` in one shot. We still preserve existing `Provenance` for matching steps by name. --- pkg/pod/status.go | 26 ++++---- pkg/pod/status_test.go | 142 +++++++++++++++++++++++++++++++++++++++-- 2 files changed, 151 insertions(+), 17 deletions(-) diff --git a/pkg/pod/status.go b/pkg/pod/status.go index 357665bc715..272b351c924 100644 --- a/pkg/pod/status.go +++ b/pkg/pod/status.go @@ -268,8 +268,15 @@ func setTaskRunStatusBasedOnStepStatus(ctx context.Context, logger *zap.SugaredL } } + // Build a lookup map for step state provenances. + stepStateProvenances := make(map[string]*v1.Provenance) + for _, ss := range trs.Steps { + stepStateProvenances[ss.Name] = ss.Provenance + } + // Continue with extraction of termination messages - for _, s := range stepStatuses { + orderedStepStates := make([]v1.StepState, len(stepStatuses)) + for i, s := range stepStatuses { // Avoid changing the original value by modifying the pointer value. state := s.State.DeepCopy() taskRunStepResults := []v1.TaskRunStepResult{} @@ -378,18 +385,13 @@ func setTaskRunStatusBasedOnStepStatus(ctx context.Context, logger *zap.SugaredL Inputs: sas.Inputs, Outputs: sas.Outputs, } - foundStep := false - for i, ss := range trs.Steps { - if ss.Name == stepState.Name { - stepState.Provenance = ss.Provenance - trs.Steps[i] = stepState - foundStep = true - break - } - } - if !foundStep { - trs.Steps = append(trs.Steps, stepState) + if stepStateProvenance, exist := stepStateProvenances[stepState.Name]; exist { + stepState.Provenance = stepStateProvenance } + orderedStepStates[i] = stepState + } + if len(orderedStepStates) > 0 { + trs.Steps = orderedStepStates } return errors.Join(errs...) diff --git a/pkg/pod/status_test.go b/pkg/pod/status_test.go index 72aefedbd4d..31ccdada00b 100644 --- a/pkg/pod/status_test.go +++ b/pkg/pod/status_test.go @@ -814,14 +814,14 @@ func TestMakeTaskRunStatus_StepArtifacts(t *testing.T) { func TestMakeTaskRunStatus(t *testing.T) { for _, c := range []struct { - desc string - podStatus corev1.PodStatus - pod corev1.Pod - want v1.TaskRunStatus + desc string + podStatus corev1.PodStatus + pod corev1.Pod + stepStates []v1.StepState + want v1.TaskRunStatus }{{ desc: "empty", podStatus: corev1.PodStatus{}, - want: v1.TaskRunStatus{ Status: statusRunning(), TaskRunStatusFields: v1.TaskRunStatusFields{ @@ -1741,6 +1741,137 @@ func TestMakeTaskRunStatus(t *testing.T) { CompletionTime: &metav1.Time{Time: time.Now()}, }, }, + }, { + desc: "TaskRun status steps ordering based on pod spec containers", + pod: corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pod", + }, + Spec: corev1.PodSpec{ + Containers: []corev1.Container{{ + Name: "step-first-inline", + }, { + Name: "step-second-remote", + }, { + Name: "step-third-inline", + }, { + Name: "step--inline", + }, { + Name: "step-fourth-remote", + }, { + Name: "step-fifth-remote", + }}, + }, + Status: corev1.PodStatus{ + Phase: corev1.PodSucceeded, + ContainerStatuses: []corev1.ContainerStatus{{ + Name: "step-second-remote", + State: corev1.ContainerState{ + Terminated: &corev1.ContainerStateTerminated{}, + }, + }, { + Name: "step-fourth-remote", + State: corev1.ContainerState{ + Terminated: &corev1.ContainerStateTerminated{}, + }, + }, { + Name: "step--inline", + State: corev1.ContainerState{ + Terminated: &corev1.ContainerStateTerminated{}, + }, + }, { + Name: "step-first-inline", + State: corev1.ContainerState{ + Terminated: &corev1.ContainerStateTerminated{}, + }, + }, { + Name: "step-fifth-remote", + State: corev1.ContainerState{ + Terminated: &corev1.ContainerStateTerminated{}, + }, + }, { + Name: "step-third-inline", + State: corev1.ContainerState{ + Terminated: &corev1.ContainerStateTerminated{}, + }, + }}, + }, + }, + stepStates: []v1.StepState{ + { + Name: "second-remote", + Provenance: &v1.Provenance{ + RefSource: &v1.RefSource{ + URI: "test-uri", + Digest: map[string]string{"sha256": "digest"}, + }, + }, + }, + { + Name: "fourth-remote", + }, + { + Name: "fifth-remote", + Provenance: &v1.Provenance{ + RefSource: nil, + }, + }, + }, + want: v1.TaskRunStatus{ + Status: statusSuccess(), + TaskRunStatusFields: v1.TaskRunStatusFields{ + Steps: []v1.StepState{{ + ContainerState: corev1.ContainerState{ + Terminated: &corev1.ContainerStateTerminated{}, + }, + Name: "first-inline", + Container: "step-first-inline", + }, { + ContainerState: corev1.ContainerState{ + Terminated: &corev1.ContainerStateTerminated{}, + }, + Name: "second-remote", + Container: "step-second-remote", + Provenance: &v1.Provenance{ + RefSource: &v1.RefSource{ + URI: "test-uri", + Digest: map[string]string{"sha256": "digest"}, + }, + }, + }, { + ContainerState: corev1.ContainerState{ + Terminated: &corev1.ContainerStateTerminated{}, + }, + Name: "third-inline", + Container: "step-third-inline", + }, { + ContainerState: corev1.ContainerState{ + Terminated: &corev1.ContainerStateTerminated{}, + }, + Name: "-inline", + Container: "step--inline", + }, { + ContainerState: corev1.ContainerState{ + Terminated: &corev1.ContainerStateTerminated{}, + }, + Name: "fourth-remote", + Container: "step-fourth-remote", + }, { + ContainerState: corev1.ContainerState{ + Terminated: &corev1.ContainerStateTerminated{}, + }, + Name: "fifth-remote", + Container: "step-fifth-remote", + Provenance: &v1.Provenance{ + RefSource: nil, + }, + }}, + Sidecars: []v1.SidecarState{}, + Artifacts: &v1.Artifacts{}, + // We don't actually care about the time, just that it's not nil + CompletionTime: &metav1.Time{Time: time.Now()}, + }, + }, }, { desc: "include non zero exit code in a container termination message if entrypoint is set to ignore the error", pod: corev1.Pod{ @@ -1964,6 +2095,7 @@ func TestMakeTaskRunStatus(t *testing.T) { Status: v1.TaskRunStatus{ TaskRunStatusFields: v1.TaskRunStatusFields{ StartTime: &metav1.Time{Time: startTime}, + Steps: c.stepStates, }, }, } From 6290b162cc0d3b4e3e0d1589f4d933a5b4ddba42 Mon Sep 17 00:00:00 2001 From: Maximilien Raulic Date: Fri, 19 Sep 2025 12:53:42 +0200 Subject: [PATCH 2/3] feat(taskspec): include inline steps in `Status.Steps` when StepActions are used When at least one StepAction is referenced, `TaskRun.Status.Steps` is pre-populated to capture provenance for remote steps during resolution. However, inline steps were not included at that stage and only appeared later during pod-based status reconciliation. This contributed to confusing ordering and visibility for dashboards relying on `Status.Steps`. In the Tekton Dashboard specifically, StepAction-backed steps showed up first (because they were present in `Status.Steps` right after resolution), while inline steps appeared only after the pod was created and `MakeTaskRunStatus` had run to append them. This resulted in steps "popping" into view and reshuffling, which was confusing. When a `TaskRun` references at least one `StepAction`, in the resolution flow, the inline `Step` should also append an entry in `Status.Steps` even if it has no `Provenance`. add comment for readibility --- pkg/reconciler/taskrun/resources/taskspec.go | 12 +- .../taskrun/resources/taskspec_test.go | 210 ++++++++++++++++++ 2 files changed, 217 insertions(+), 5 deletions(-) diff --git a/pkg/reconciler/taskrun/resources/taskspec.go b/pkg/reconciler/taskrun/resources/taskspec.go index 323e0f34da4..9c685bf5c8f 100644 --- a/pkg/reconciler/taskrun/resources/taskspec.go +++ b/pkg/reconciler/taskrun/resources/taskspec.go @@ -174,6 +174,8 @@ func resolveStepRef(ctx context.Context, taskSpec v1.TaskSpec, taskRun *v1.TaskR // updateTaskRunProvenance update the TaskRun's status with source provenance information for a given step func updateTaskRunProvenance(taskRun *v1.TaskRun, stepName string, stepIndex int, source *v1.RefSource, stepStatusIndex map[string]int) { + var provenance *v1.Provenance + // The StepState already exists. Update it in place if index, found := stepStatusIndex[stepName]; found { if taskRun.Status.Steps[index].Provenance == nil { @@ -183,10 +185,12 @@ func updateTaskRunProvenance(taskRun *v1.TaskRun, stepName string, stepIndex int return } + provenance = &v1.Provenance{RefSource: source} + // No existing StepState found. Create and append a new one newState := v1.StepState{ Name: pod.TrimStepPrefix(pod.StepName(stepName, stepIndex)), - Provenance: &v1.Provenance{RefSource: source}, + Provenance: provenance, } taskRun.Status.Steps = append(taskRun.Status.Steps, newState) } @@ -237,15 +241,13 @@ func GetStepActionsData(ctx context.Context, taskSpec v1.TaskSpec, taskRun *v1.T for i, step := range taskSpec.Steps { if step.Ref == nil { steps[i] = step + updateTaskRunProvenance(taskRun, step.Name, i, nil, stepStatusIndex) // create StepState for inline step with nil provenance continue } stepRefResolution := stepRefResolutions[i] steps[i] = *stepRefResolution.resolvedStep - - if stepRefResolution.source != nil { - updateTaskRunProvenance(taskRun, stepRefResolution.resolvedStep.Name, i, stepRefResolution.source, stepStatusIndex) - } + updateTaskRunProvenance(taskRun, stepRefResolution.resolvedStep.Name, i, stepRefResolution.source, stepStatusIndex) } return steps, nil diff --git a/pkg/reconciler/taskrun/resources/taskspec_test.go b/pkg/reconciler/taskrun/resources/taskspec_test.go index c9b91a45f91..147961c32bf 100644 --- a/pkg/reconciler/taskrun/resources/taskspec_test.go +++ b/pkg/reconciler/taskrun/resources/taskspec_test.go @@ -756,6 +756,216 @@ spec: } } +func TestGetStepActionsData_Status(t *testing.T) { + firstStepAction := parse.MustParseV1beta1StepAction(t, ` +metadata: + name: first-stepaction + namespace: default +spec: + image: myImage +`) + firstStepActionSource := v1.RefSource{ + URI: "ref-source", + Digest: map[string]string{"sha256": "abcd123456"}, + } + + firstStepActionBytes, err := yaml.Marshal(firstStepAction) + if err != nil { + t.Fatal("failed to marshal StepAction", err) + } + rr := test.NewResolvedResource(firstStepActionBytes, map[string]string{}, &firstStepActionSource, nil) + requester := test.NewRequester(rr, nil, resource.ResolverPayload{}) + + tests := []struct { + name string + tr *v1.TaskRun + stepActions []*v1beta1.StepAction + want v1.TaskRunStatus + }{ + { + name: "inline only", + tr: &v1.TaskRun{ + ObjectMeta: metav1.ObjectMeta{ + Name: "mytaskrun", + Namespace: "default", + }, + Spec: v1.TaskRunSpec{ + TaskSpec: &v1.TaskSpec{ + Steps: []v1.Step{ + { + Name: "first-inline", + Image: "ubuntu", + }, + { + Name: "second-inline", + Image: "ubuntu", + }, + }, + }, + }, + }, + want: v1.TaskRunStatus{}, + }, { + name: "StepAction only", + tr: &v1.TaskRun{ + ObjectMeta: metav1.ObjectMeta{ + Name: "mytaskrun", + Namespace: "default", + }, + Spec: v1.TaskRunSpec{ + TaskSpec: &v1.TaskSpec{ + Steps: []v1.Step{ + { + Name: "first-remote", + Ref: &v1.Ref{ + Name: "first-stepaction", + ResolverRef: v1.ResolverRef{ + Resolver: "foobar", + }, + }, + }, + { + Name: "second-remote", + Ref: &v1.Ref{ + Name: "second-stepaction", + }, + }, + }, + }, + }, + }, + stepActions: []*v1beta1.StepAction{ + firstStepAction, + { + ObjectMeta: metav1.ObjectMeta{ + Name: "second-stepaction", + Namespace: "default", + }, + Spec: v1beta1.StepActionSpec{ + Image: "myimage", + }, + }, + }, + want: v1.TaskRunStatus{ + TaskRunStatusFields: v1.TaskRunStatusFields{ + Steps: []v1.StepState{ + { + Name: "first-remote", + Provenance: &v1.Provenance{ + RefSource: &v1.RefSource{ + URI: "ref-source", + Digest: map[string]string{"sha256": "abcd123456"}, + }, + }, + }, + { + Name: "second-remote", + Provenance: &v1.Provenance{}, + }, + }, + }, + }, + }, + { + name: "mixed inline and StepAction", + tr: &v1.TaskRun{ + ObjectMeta: metav1.ObjectMeta{ + Name: "mytaskrun", + Namespace: "default", + }, + Spec: v1.TaskRunSpec{ + TaskSpec: &v1.TaskSpec{ + Steps: []v1.Step{ + { + Name: "first-inline", + Image: "ubuntu", + }, + { + Name: "second-remote", + Ref: &v1.Ref{ + Name: "first-stepaction", + ResolverRef: v1.ResolverRef{ + Resolver: "foobar", + }, + }, + }, + { + Name: "third-inline", + Image: "ubuntu", + }, + { + Name: "fourth-remote", + Ref: &v1.Ref{ + Name: "second-stepaction", + }, + }, + }, + }, + }, + }, + stepActions: []*v1beta1.StepAction{ + firstStepAction, + { + ObjectMeta: metav1.ObjectMeta{ + Name: "second-stepaction", + Namespace: "default", + }, + Spec: v1beta1.StepActionSpec{ + Image: "myimage", + }, + }, + }, + want: v1.TaskRunStatus{ + TaskRunStatusFields: v1.TaskRunStatusFields{ + Steps: []v1.StepState{ + { + Name: "first-inline", + Provenance: &v1.Provenance{}, + }, + { + Name: "second-remote", + Provenance: &v1.Provenance{ + RefSource: &v1.RefSource{ + URI: "ref-source", + Digest: map[string]string{"sha256": "abcd123456"}, + }, + }, + }, + { + Name: "third-inline", + Provenance: &v1.Provenance{}, + }, + { + Name: "fourth-remote", + Provenance: &v1.Provenance{}, + }, + }, + }, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ctx := t.Context() + tektonclient := fake.NewSimpleClientset() + for _, sa := range tt.stepActions { + if err := tektonclient.Tracker().Add(sa); err != nil { + t.Fatal(err) + } + } + + _, err := GetStepActionsData(ctx, *tt.tr.Spec.TaskSpec, tt.tr, tektonclient, nil, requester) + if err != nil { + t.Fatalf("Did not expect an error but got : %s", err) + } + if d := cmp.Diff(tt.want, tt.tr.Status); d != "" { + t.Errorf("the taskrun status did not match what was expected diff: %s", diff.PrintWantGot(d)) + } + }) + } +} + func TestGetStepActionsData(t *testing.T) { taskRunUser := int64(1001) stepActionUser := int64(1000) From 52a30a3fd4d9f063e0459aac80ee16d9a83b1833 Mon Sep 17 00:00:00 2001 From: Maximilien Raulic Date: Tue, 30 Sep 2025 12:55:14 +0200 Subject: [PATCH 3/3] feat(taskspec): create Status.Steps even without StepActions With this change, TaskRun.Status.Steps are now populated even when there is only inline steps defined. --- pkg/reconciler/taskrun/resources/taskspec.go | 28 +++++++++++-------- .../taskrun/resources/taskspec_test.go | 18 ++++++++++-- 2 files changed, 33 insertions(+), 13 deletions(-) diff --git a/pkg/reconciler/taskrun/resources/taskspec.go b/pkg/reconciler/taskrun/resources/taskspec.go index 9c685bf5c8f..a1cf1e2dbb7 100644 --- a/pkg/reconciler/taskrun/resources/taskspec.go +++ b/pkg/reconciler/taskrun/resources/taskspec.go @@ -197,9 +197,24 @@ func updateTaskRunProvenance(taskRun *v1.TaskRun, stepName string, stepIndex int // GetStepActionsData extracts the StepActions and merges them with the inlined Step specification. func GetStepActionsData(ctx context.Context, taskSpec v1.TaskSpec, taskRun *v1.TaskRun, tekton clientset.Interface, k8s kubernetes.Interface, requester remoteresource.Requester) ([]v1.Step, error) { - // If there are no step-ref to resolve, return immediately + steps := make([]v1.Step, len(taskSpec.Steps)) + + // Init step states and known step states indexes lookup map + if taskRun.Status.Steps == nil { + taskRun.Status.Steps = []v1.StepState{} + } + stepStatusIndex := make(map[string]int, len(taskRun.Status.Steps)) + for i, stepState := range taskRun.Status.Steps { + stepStatusIndex[stepState.Name] = i + } + + // If there are no step-ref to resolve, return immediately with nil provenance if !hasStepRefs(&taskSpec) { - return taskSpec.Steps, nil + for i, step := range taskSpec.Steps { + steps[i] = step + updateTaskRunProvenance(taskRun, step.Name, i, nil, stepStatusIndex) // create StepState with nil provenance + } + return steps, nil } // Phase 1: Concurrently resolve all StepActions @@ -229,15 +244,6 @@ func GetStepActionsData(ctx context.Context, taskSpec v1.TaskSpec, taskRun *v1.T } // Phase 2: Sequentially merge results into the final step list and update status - if taskRun.Status.Steps == nil { - taskRun.Status.Steps = []v1.StepState{} - } - stepStatusIndex := make(map[string]int, len(taskRun.Status.Steps)) - for i, stepState := range taskRun.Status.Steps { - stepStatusIndex[stepState.Name] = i - } - - steps := make([]v1.Step, len(taskSpec.Steps)) for i, step := range taskSpec.Steps { if step.Ref == nil { steps[i] = step diff --git a/pkg/reconciler/taskrun/resources/taskspec_test.go b/pkg/reconciler/taskrun/resources/taskspec_test.go index 147961c32bf..ff88dd5f8cd 100644 --- a/pkg/reconciler/taskrun/resources/taskspec_test.go +++ b/pkg/reconciler/taskrun/resources/taskspec_test.go @@ -737,7 +737,8 @@ spec: Status: v1.TaskRunStatus{ TaskRunStatusFields: v1.TaskRunStatusFields{ Steps: []v1.StepState{{ - Name: "step1", + Name: "step1", + Provenance: &v1.Provenance{}, }}, }, }, @@ -804,7 +805,20 @@ spec: }, }, }, - want: v1.TaskRunStatus{}, + want: v1.TaskRunStatus{ + TaskRunStatusFields: v1.TaskRunStatusFields{ + Steps: []v1.StepState{ + { + Name: "first-inline", + Provenance: &v1.Provenance{}, + }, + { + Name: "second-inline", + Provenance: &v1.Provenance{}, + }, + }, + }, + }, }, { name: "StepAction only", tr: &v1.TaskRun{