Tasks should be sorted in Pipeline spec order not alphabetically

[a-roberts]

Expected behavior

Tasks should be sorted by the order they are defined in the Pipeline, not alphabetically

Actual behavior

They're sorted alphabetically

Steps to reproduce the problem

1. Create a PipelineRun with at least three tasks
2. Refresh the page once all tasks complete and see they're ordered alphabetically

Environment (e.g. Docker Desktop, OpenShift)

All

Tekton Pipelines version (e.g. 0.9.2, 0.8.0, master)

Any

Tekton Triggers version (e.g. 0.2, 0.1, master)

Any

Tekton Dashboard version (e.g. 0.4.0, 0.3.0, master)

All

[a-roberts]

@AlanGreene do you think this'd be particularly hard to do? I've noticed we've got a sorting by time already on the TaskRuns page, for full disclosure it's being asked for as a useful feature to have for the upcoming release of the Kabanero project as there's a validate task that actually happens first, but it's displayed at the bottom

[AlanGreene]

hrmm displaying them alphabetically (the current mystery behaviour we have no explicit code for...) is definitely not right, but I'm not sure if sorting by timestamp is right either as it won't be consistent across multiple PipelineRuns for the same Pipeline where some Tasks are run in parallel.

Ideally we'd display them in topologically sorted order (walk the DAG essentially), but this would take a bit of work considering the current approach to processing the data.

Maybe we could display them in the order they're defined in the Pipeline definition? That would be relatively simple and we already do it for steps so could take a similar approach.

If we do want to sort by timestamp it would be simple enough to do in either the PipelineRun or TaskTree component assuming the metadata including timestamps is available at those points.

[a-roberts]

hrmm displaying them alphabetically (the current mystery behaviour we have no explicit code for...) is definitely not right, but I'm not sure if sorting by timestamp is right either as it won't be consistent across multiple PipelineRuns for the same Pipeline where some Tasks are run in parallel.

Ideally we'd display them in topologically sorted order (walk the DAG essentially), but this would take a bit of work considering the current approach to processing the data.

Maybe we could display them in the order they're defined in the Pipeline definition? That would be relatively simple and we already do it for steps so could take a similar approach.

If we do want to sort by timestamp it would be simple enough to do in either the PipelineRun or TaskTree component assuming the metadata including timestamps is available at those points.

Good points, yeah I like the idea of doing it based on the position in the Pipeline for now, I'm wary there's run after and folks needn't place things in order really but at least it'll be done in a well defined way and to some specification

[anneqm]

consensus here: prefer displaying in the order defined in the Pipeline definition rather than by timestamp. Can we update the issue description? :)

[a-roberts]

consensus here: prefer displaying in the order defined in the Pipeline definition rather than by timestamp. Can we update the issue description? :)

Done

[a-roberts]

Right, got a test Pipeline, PipelineRun and some tasks, actually fixing the code now, the getTektonAPI get does return the Task names in name order 🤔

---
  apiVersion: tekton.dev/v1alpha1
  kind: Pipeline
  metadata:
    name: test-pipeline
    namespace: tekton-pipelines
  spec:
    tasks:
      - name: b-task
        taskRef:
          kind: Task
          name: step-b-task
      - name: c-task
        taskRef:
          kind: Task
          name: step-c-task
      - name: a-task
        taskRef:
          kind: Task
          name: step-a-task

apiVersion: tekton.dev/v1alpha1
kind: PipelineRun
metadata:
  name: test-pipelinerun
  namespace: tekton-pipelines
spec:
  pipelineRef:
    name: test-pipeline

---
apiVersion: tekton.dev/v1alpha1
kind: Task
metadata:
  name: step-b-task
  namespace: tekton-pipelines
spec:
  steps:
    - name: step-b-task-step
      image: ubuntu
      script: ls
---
apiVersion: tekton.dev/v1alpha1
kind: Task
metadata:
  name: step-a-task
  namespace: tekton-pipelines
spec:
  steps:
    - name: step-a-task-step
      image: ubuntu
      script: ls
---
apiVersion: tekton.dev/v1alpha1
kind: Task
metadata:
  name: step-c-task
  namespace: tekton-pipelines
spec:
  steps:
    - name: step-c-task-step
      image: ubuntu
      script: ls

Indeed this gives
currently

I'm looking at introducing moment to sort by timestamp as at least that'd solve the simple case and we'd expand from there. Had a look at the existing step reordering code and I can't currently see how to apply that to this case.

The structure of the PipelineRun is

apiVersion: tekton.dev/v1alpha1
kind: PipelineRun
metadata:
  annotations:
    kubectl.kubernetes.io/last-applied-configuration: |
      {"apiVersion":"tekton.dev/v1alpha1","kind":"Pipeline","metadata":{"annotations":{},"name":"test-pipeline","namespace":"tekton-pipelines"},"spec":{"tasks":[{"name":"b-task","taskRef":{"kind":"Task","name":"step-b-task"}},{"name":"c-task","taskRef":{"kind":"Task","name":"step-c-task"}},{"name":"a-task","taskRef":{"kind":"Task","name":"step-a-task"}}]}}
  creationTimestamp: "2020-02-20T10:41:16Z"
  generation: 1
  labels:
    tekton.dev/pipeline: test-pipeline
  name: test-pipelinerun
  namespace: tekton-pipelines
  resourceVersion: "2048384"
  selfLink: /apis/tekton.dev/v1alpha1/namespaces/tekton-pipelines/pipelineruns/test-pipelinerun
  uid: 858ce52d-53cd-11ea-b0ef-025000000001
spec:
  pipelineRef:
    name: test-pipeline
  timeout: 1h0m0s
status:
  completionTime: "2020-02-20T10:41:40Z"
  conditions:
  - lastTransitionTime: "2020-02-20T10:41:40Z"
    message: All Tasks have completed executing
    reason: Succeeded
    status: "True"
    type: Succeeded
  startTime: "2020-02-20T10:41:16Z"
  taskRuns:
    test-pipelinerun-a-task-8wtpl:
      pipelineTaskName: a-task
      status:
        completionTime: "2020-02-20T10:41:40Z"
        conditions:
        - lastTransitionTime: "2020-02-20T10:41:40Z"
          message: All Steps have completed executing
          reason: Succeeded
          status: "True"
          type: Succeeded
        podName: test-pipelinerun-a-task-8wtpl-pod-fg2kb
        startTime: "2020-02-20T10:41:16Z"
        steps:
        - container: step-step-a-task-step
          imageID: docker-pullable://ubuntu@sha256:8d31dad0c58f552e890d68bbfb735588b6b820a46e459672d96e585871acc110
          name: step-a-task-step
          terminated:
            containerID: docker://d84368770f93c28909a5580c9e8f1bd2dae4ded77f5746dccadc3245217b4bde
            exitCode: 0
            finishedAt: "2020-02-20T10:41:39Z"
            reason: Completed
            startedAt: "2020-02-20T10:41:39Z"
    test-pipelinerun-b-task-c6xbd:
      pipelineTaskName: b-task
      status:
        completionTime: "2020-02-20T10:41:38Z"
        conditions:
        - lastTransitionTime: "2020-02-20T10:41:38Z"
          message: All Steps have completed executing
          reason: Succeeded
          status: "True"
          type: Succeeded
        podName: test-pipelinerun-b-task-c6xbd-pod-qsqln
        startTime: "2020-02-20T10:41:16Z"
        steps:
        - container: step-step-b-task-step
          imageID: docker-pullable://ubuntu@sha256:8d31dad0c58f552e890d68bbfb735588b6b820a46e459672d96e585871acc110
          name: step-b-task-step
          terminated:
            containerID: docker://4e8dd521e6889340e421b5cd2f8f3223d50af6ca1ff5f613bc86e6323fe4b1b8
            exitCode: 0
            finishedAt: "2020-02-20T10:41:37Z"
            reason: Completed
            startedAt: "2020-02-20T10:41:37Z"
    test-pipelinerun-c-task-2gb4l:
      pipelineTaskName: c-task
      status:
        completionTime: "2020-02-20T10:41:35Z"
        conditions:
        - lastTransitionTime: "2020-02-20T10:41:35Z"
          message: All Steps have completed executing
          reason: Succeeded
          status: "True"
          type: Succeeded
        podName: test-pipelinerun-c-task-2gb4l-pod-md5g5
        startTime: "2020-02-20T10:41:16Z"
        steps:
        - container: step-step-c-task-step
          imageID: docker-pullable://ubuntu@sha256:8d31dad0c58f552e890d68bbfb735588b6b820a46e459672d96e585871acc110
          name: step-c-task-step
          terminated:
            containerID: docker://92d745a43552fd010579af9e170d46e96419f9f09f7735fa79a8d64a178412bf
            exitCode: 0
            finishedAt: "2020-02-20T10:41:34Z"
            reason: Completed
            startedAt: "2020-02-20T10:41:34Z"

note that startedAt is present on the steps themselves and not on the Tasks that get run as part of the TaskRun.

[a-roberts]

I have something working, needs more testing though.

While a PipelineRun is still happening:

And when it's done, it's the same.

I'm sorting on terminated.startedAt here, I've had to introduce moment for the useful before/after methods. Oddly enough I've only changed PipelineRuns here, not TaskRuns, and yet TaskRuns appears to be sorted by startedAt already:

[a-roberts]

This is open now for anyone else to build upon to actually implement the displaying of steps by Spec order, I implemented by earliest startedAt time

Here's some code in use by the Dashboard as part of the IBM public cloud team offering for Tekton that @AlanGreene has kindly shared that we can build upon, perhaps adding a switch to change the sorting method used:

sortRunsByPipelinePosition(runs, pipeline) {
    if (pipeline && runs.length) {
      const holder = [];
      pipeline.spec.tasks.forEach(pipelineTask => {
        const taskRun = runs.find(
          run =>
            run.metadata.labels["tekton.dev/pipelineTask"] === pipelineTask.name
        );
        if (taskRun) {
          holder.push(taskRun);
        }
      });
      return runs.length === holder.length ? holder : runs;
    }
    return runs;
  }

[tekton-robot]

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.
If this issue is safe to close now please do so with /close.

/lifecycle rotten

Send feedback to tektoncd/plumbing.

[tekton-robot]

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

/close

Send feedback to tektoncd/plumbing.

[tekton-robot]

@tekton-robot: Closing this issue.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

/close

Send feedback to tektoncd/plumbing.

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.

[AlanGreene]

/remove-lifecycle rotten
/reopen

[tekton-robot]

@AlanGreene: Reopened this issue.

In response to this:

/remove-lifecycle rotten
/reopen

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.

[AlanGreene]

Revisiting the comment above, relevant piece for sorting in Pipeline spec order:

const orderedTaskRuns = [];
pipeline.spec.tasks.forEach(pipelineTask => {
  const taskRun = taskRuns.find(
    run =>
      run.metadata.labels["tekton.dev/pipelineTask"] === pipelineTask.name
  );
  if (taskRun) {
    orderedTaskRuns.push(taskRun);
  }
});

The line:
return runs.length === holder.length ? holder : runs;
is problematic, as when the additional TaskRuns due to Conditions are present this can result in a lot of reordering while the PipelineRun is in progress and is quite annoying for users.

We should review the order in PipelineRun.status.taskRuns or other locations we might rely on to see if they're stable in recent Pipelines releases. I know there was a recent change to ensure the step order is consistent (including the init steps), but not sure if similar has been done for TaskRuns.

[AlanGreene]

Also relevant: #1445
If we move display of retries under a single entry for the TaskRun this would also help simplify the UI for users as well as reducing issues due to sorting.