Skip to content

Commit 73ff66e

Browse files
committed
fix(taskspec): ensure TaskRun.Status.Steps ordering matches pod execution order
The TaskRun.Status.Steps list was not guaranteed to reflect the true execution order when StepActions were involved, causing confusing behavior in dashboards like Tekton Dashboard where steps would appear to "pop" into view and reshuffle. This change addresses three related issues: 1. Steps populated from StepAction resolution appeared first in Status.Steps, while inline steps were only added later during pod-based status reconciliation, creating a mismatch with actual execution order. 2. When TaskRuns used StepActions, inline steps were missing from Status.Steps during the resolution phase, contributing to the ordering confusion. 3. The final Status.Steps ordering didn't match the pod container sequence, making it difficult for dashboards to display accurate step progression. The fix ensures that: - Status.Steps are populated for both StepAction-backed and inline steps during resolution, even when there is no StepActions involved - The final Status.Steps ordering is aligned with the pod step container sequence by creating a temporary slice and replacing trs.Steps in one shot - Existing Provenance information is preserved for matching steps by name
1 parent 724cb82 commit 73ff66e

File tree

10 files changed

+411
-45
lines changed

10 files changed

+411
-45
lines changed

pkg/pod/status.go

Lines changed: 14 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -268,8 +268,15 @@ func setTaskRunStatusBasedOnStepStatus(ctx context.Context, logger *zap.SugaredL
268268
}
269269
}
270270

271+
// Build a lookup map for step state provenances.
272+
stepStateProvenances := make(map[string]*v1.Provenance)
273+
for _, ss := range trs.Steps {
274+
stepStateProvenances[ss.Name] = ss.Provenance
275+
}
276+
271277
// Continue with extraction of termination messages
272-
for _, s := range stepStatuses {
278+
orderedStepStates := make([]v1.StepState, len(stepStatuses))
279+
for i, s := range stepStatuses {
273280
// Avoid changing the original value by modifying the pointer value.
274281
state := s.State.DeepCopy()
275282
taskRunStepResults := []v1.TaskRunStepResult{}
@@ -378,18 +385,13 @@ func setTaskRunStatusBasedOnStepStatus(ctx context.Context, logger *zap.SugaredL
378385
Inputs: sas.Inputs,
379386
Outputs: sas.Outputs,
380387
}
381-
foundStep := false
382-
for i, ss := range trs.Steps {
383-
if ss.Name == stepState.Name {
384-
stepState.Provenance = ss.Provenance
385-
trs.Steps[i] = stepState
386-
foundStep = true
387-
break
388-
}
389-
}
390-
if !foundStep {
391-
trs.Steps = append(trs.Steps, stepState)
388+
if stepStateProvenance, exist := stepStateProvenances[stepState.Name]; exist {
389+
stepState.Provenance = stepStateProvenance
392390
}
391+
orderedStepStates[i] = stepState
392+
}
393+
if len(orderedStepStates) > 0 {
394+
trs.Steps = orderedStepStates
393395
}
394396

395397
return errors.Join(errs...)

pkg/pod/status_test.go

Lines changed: 137 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -814,14 +814,14 @@ func TestMakeTaskRunStatus_StepArtifacts(t *testing.T) {
814814

815815
func TestMakeTaskRunStatus(t *testing.T) {
816816
for _, c := range []struct {
817-
desc string
818-
podStatus corev1.PodStatus
819-
pod corev1.Pod
820-
want v1.TaskRunStatus
817+
desc string
818+
podStatus corev1.PodStatus
819+
pod corev1.Pod
820+
stepStates []v1.StepState
821+
want v1.TaskRunStatus
821822
}{{
822823
desc: "empty",
823824
podStatus: corev1.PodStatus{},
824-
825825
want: v1.TaskRunStatus{
826826
Status: statusRunning(),
827827
TaskRunStatusFields: v1.TaskRunStatusFields{
@@ -1741,6 +1741,137 @@ func TestMakeTaskRunStatus(t *testing.T) {
17411741
CompletionTime: &metav1.Time{Time: time.Now()},
17421742
},
17431743
},
1744+
}, {
1745+
desc: "TaskRun status steps ordering based on pod spec containers",
1746+
pod: corev1.Pod{
1747+
ObjectMeta: metav1.ObjectMeta{
1748+
Name: "pod",
1749+
},
1750+
Spec: corev1.PodSpec{
1751+
Containers: []corev1.Container{{
1752+
Name: "step-first-inline",
1753+
}, {
1754+
Name: "step-second-remote",
1755+
}, {
1756+
Name: "step-third-inline",
1757+
}, {
1758+
Name: "step--inline",
1759+
}, {
1760+
Name: "step-fourth-remote",
1761+
}, {
1762+
Name: "step-fifth-remote",
1763+
}},
1764+
},
1765+
Status: corev1.PodStatus{
1766+
Phase: corev1.PodSucceeded,
1767+
ContainerStatuses: []corev1.ContainerStatus{{
1768+
Name: "step-second-remote",
1769+
State: corev1.ContainerState{
1770+
Terminated: &corev1.ContainerStateTerminated{},
1771+
},
1772+
}, {
1773+
Name: "step-fourth-remote",
1774+
State: corev1.ContainerState{
1775+
Terminated: &corev1.ContainerStateTerminated{},
1776+
},
1777+
}, {
1778+
Name: "step--inline",
1779+
State: corev1.ContainerState{
1780+
Terminated: &corev1.ContainerStateTerminated{},
1781+
},
1782+
}, {
1783+
Name: "step-first-inline",
1784+
State: corev1.ContainerState{
1785+
Terminated: &corev1.ContainerStateTerminated{},
1786+
},
1787+
}, {
1788+
Name: "step-fifth-remote",
1789+
State: corev1.ContainerState{
1790+
Terminated: &corev1.ContainerStateTerminated{},
1791+
},
1792+
}, {
1793+
Name: "step-third-inline",
1794+
State: corev1.ContainerState{
1795+
Terminated: &corev1.ContainerStateTerminated{},
1796+
},
1797+
}},
1798+
},
1799+
},
1800+
stepStates: []v1.StepState{
1801+
{
1802+
Name: "second-remote",
1803+
Provenance: &v1.Provenance{
1804+
RefSource: &v1.RefSource{
1805+
URI: "test-uri",
1806+
Digest: map[string]string{"sha256": "digest"},
1807+
},
1808+
},
1809+
},
1810+
{
1811+
Name: "fourth-remote",
1812+
},
1813+
{
1814+
Name: "fifth-remote",
1815+
Provenance: &v1.Provenance{
1816+
RefSource: nil,
1817+
},
1818+
},
1819+
},
1820+
want: v1.TaskRunStatus{
1821+
Status: statusSuccess(),
1822+
TaskRunStatusFields: v1.TaskRunStatusFields{
1823+
Steps: []v1.StepState{{
1824+
ContainerState: corev1.ContainerState{
1825+
Terminated: &corev1.ContainerStateTerminated{},
1826+
},
1827+
Name: "first-inline",
1828+
Container: "step-first-inline",
1829+
}, {
1830+
ContainerState: corev1.ContainerState{
1831+
Terminated: &corev1.ContainerStateTerminated{},
1832+
},
1833+
Name: "second-remote",
1834+
Container: "step-second-remote",
1835+
Provenance: &v1.Provenance{
1836+
RefSource: &v1.RefSource{
1837+
URI: "test-uri",
1838+
Digest: map[string]string{"sha256": "digest"},
1839+
},
1840+
},
1841+
}, {
1842+
ContainerState: corev1.ContainerState{
1843+
Terminated: &corev1.ContainerStateTerminated{},
1844+
},
1845+
Name: "third-inline",
1846+
Container: "step-third-inline",
1847+
}, {
1848+
ContainerState: corev1.ContainerState{
1849+
Terminated: &corev1.ContainerStateTerminated{},
1850+
},
1851+
Name: "-inline",
1852+
Container: "step--inline",
1853+
}, {
1854+
ContainerState: corev1.ContainerState{
1855+
Terminated: &corev1.ContainerStateTerminated{},
1856+
},
1857+
Name: "fourth-remote",
1858+
Container: "step-fourth-remote",
1859+
}, {
1860+
ContainerState: corev1.ContainerState{
1861+
Terminated: &corev1.ContainerStateTerminated{},
1862+
},
1863+
Name: "fifth-remote",
1864+
Container: "step-fifth-remote",
1865+
Provenance: &v1.Provenance{
1866+
RefSource: nil,
1867+
},
1868+
}},
1869+
Sidecars: []v1.SidecarState{},
1870+
Artifacts: &v1.Artifacts{},
1871+
// We don't actually care about the time, just that it's not nil
1872+
CompletionTime: &metav1.Time{Time: time.Now()},
1873+
},
1874+
},
17441875
}, {
17451876
desc: "include non zero exit code in a container termination message if entrypoint is set to ignore the error",
17461877
pod: corev1.Pod{
@@ -1964,6 +2095,7 @@ func TestMakeTaskRunStatus(t *testing.T) {
19642095
Status: v1.TaskRunStatus{
19652096
TaskRunStatusFields: v1.TaskRunStatusFields{
19662097
StartTime: &metav1.Time{Time: startTime},
2098+
Steps: c.stepStates,
19672099
},
19682100
},
19692101
}

pkg/reconciler/taskrun/resources/taskspec.go

Lines changed: 24 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -174,6 +174,8 @@ func resolveStepRef(ctx context.Context, taskSpec v1.TaskSpec, taskRun *v1.TaskR
174174

175175
// updateTaskRunProvenance update the TaskRun's status with source provenance information for a given step
176176
func updateTaskRunProvenance(taskRun *v1.TaskRun, stepName string, stepIndex int, source *v1.RefSource, stepStatusIndex map[string]int) {
177+
var provenance *v1.Provenance
178+
177179
// The StepState already exists. Update it in place
178180
if index, found := stepStatusIndex[stepName]; found {
179181
if taskRun.Status.Steps[index].Provenance == nil {
@@ -183,19 +185,36 @@ func updateTaskRunProvenance(taskRun *v1.TaskRun, stepName string, stepIndex int
183185
return
184186
}
185187

188+
provenance = &v1.Provenance{RefSource: source}
189+
186190
// No existing StepState found. Create and append a new one
187191
newState := v1.StepState{
188192
Name: pod.TrimStepPrefix(pod.StepName(stepName, stepIndex)),
189-
Provenance: &v1.Provenance{RefSource: source},
193+
Provenance: provenance,
190194
}
191195
taskRun.Status.Steps = append(taskRun.Status.Steps, newState)
192196
}
193197

194198
// GetStepActionsData extracts the StepActions and merges them with the inlined Step specification.
195199
func GetStepActionsData(ctx context.Context, taskSpec v1.TaskSpec, taskRun *v1.TaskRun, tekton clientset.Interface, k8s kubernetes.Interface, requester remoteresource.Requester) ([]v1.Step, error) {
196-
// If there are no step-ref to resolve, return immediately
200+
steps := make([]v1.Step, len(taskSpec.Steps))
201+
202+
// Init step states and known step states indexes lookup map
203+
if taskRun.Status.Steps == nil {
204+
taskRun.Status.Steps = []v1.StepState{}
205+
}
206+
stepStatusIndex := make(map[string]int, len(taskRun.Status.Steps))
207+
for i, stepState := range taskRun.Status.Steps {
208+
stepStatusIndex[stepState.Name] = i
209+
}
210+
211+
// If there are no step-ref to resolve, return immediately with nil provenance
197212
if !hasStepRefs(&taskSpec) {
198-
return taskSpec.Steps, nil
213+
for i, step := range taskSpec.Steps {
214+
steps[i] = step
215+
updateTaskRunProvenance(taskRun, step.Name, i, nil, stepStatusIndex) // create StepState with nil provenance
216+
}
217+
return steps, nil
199218
}
200219

201220
// Phase 1: Concurrently resolve all StepActions
@@ -225,27 +244,16 @@ func GetStepActionsData(ctx context.Context, taskSpec v1.TaskSpec, taskRun *v1.T
225244
}
226245

227246
// Phase 2: Sequentially merge results into the final step list and update status
228-
if taskRun.Status.Steps == nil {
229-
taskRun.Status.Steps = []v1.StepState{}
230-
}
231-
stepStatusIndex := make(map[string]int, len(taskRun.Status.Steps))
232-
for i, stepState := range taskRun.Status.Steps {
233-
stepStatusIndex[stepState.Name] = i
234-
}
235-
236-
steps := make([]v1.Step, len(taskSpec.Steps))
237247
for i, step := range taskSpec.Steps {
238248
if step.Ref == nil {
239249
steps[i] = step
250+
updateTaskRunProvenance(taskRun, step.Name, i, nil, stepStatusIndex) // create StepState for inline step with nil provenance
240251
continue
241252
}
242253

243254
stepRefResolution := stepRefResolutions[i]
244255
steps[i] = *stepRefResolution.resolvedStep
245-
246-
if stepRefResolution.source != nil {
247-
updateTaskRunProvenance(taskRun, stepRefResolution.resolvedStep.Name, i, stepRefResolution.source, stepStatusIndex)
248-
}
256+
updateTaskRunProvenance(taskRun, stepRefResolution.resolvedStep.Name, i, stepRefResolution.source, stepStatusIndex)
249257
}
250258

251259
return steps, nil

0 commit comments

Comments
 (0)