Add Hooks into Pipeline Lifecycle

[vtereso]

Expected Behavior

Tekton Pipelines have hooks into the execution of tasks. This way, an additional system/resource is not needed to monitor the internal status. Put another way, Tasks do not need to concern themselves with updating an external system about their results.

Actual Behavior

Currently, there is no way for Tasks to report their results unless done so explicitly within a step. This makes what should be "composable" units more specified. PipelineResources address this in some capacity by allowing steps to be prepended/appended to Task (a hook of sorts), but fall short because their execution is a contract that constricts the Task and are highly specified, which will not address all use cases. Instead, expose a before/after or beforeAll/afterAll field in the PipelineSpec (the internals of this tag would be analogous to a step). With this shift of responsibility, the Pipeline becomes the more specified unit that can internally resolve the general Task lifecycle.

Regarding implementation, the PipelineRun could create unique TaskRun to resolve this lifecycle or the steps could be injected (as current with PipelineResources), where this could be to the benefit of data locality. To summarize and clarify, the Pipeline would explicitly define the "injection" behavior and it would be implicit to the Task.

As an example, a Pipeline can have a beforeAll/afterAll hook on multiple Task where each of these is a CI/CD step that will have a corresponding status in a pull request. This ties nicely with the recent addition of 9873614 so that users can simply specify a Task that is a script and easily add it to a Pipeline definition to improve their CI/CD experience.

[imjasonh]

Can you motivate this a bit more with a concrete example? What workflow is unnecessarily hard to describe today, that this makes better? Maybe include an outline of a Pipeline YAML before + after this change.

[bobcatfish]

Couple of related ideas:

• Allowing for actions to be taken in failure scenarios #1376 <-- taking actions in failure scenarios (finally clause)
• Design: Notifications #49 <-- notifications
• We've also talked about emitting cloudevents throughout execution (cant find an issue about it tho!!) which might be one way to implement this
• PipelineResources can execute before and after steps in a Task (tho not currently in a failure scenario), so in some ways they are a way of hooking into execution

One fear: i dont want us to end up in a scenario like apache we end up with many many different kinds of hooks such as HOOK_REALLY_FIRST and HOOK_REALLY_LAST 😅

[vtereso]

I have something like the following in mind for a possible example:

apiVersion: tekton.dev/v1alpha1
kind: Pipeline
metadata:
  name: pull-request-pipeline
spec:
  params:
  - name: repositoryUrl
    description: The url of the GitHub repository
  - name: revision
    description: The revision of the repository
  - name: pullRequestUrl
    description: The url of the pull request
  - name: secretName
    description: The secret to be used to authenticate against GitHub
  - name: secretKey
    description: The key with the secret (specified by secretName) that stores the authentication information for GitHub
  beforeAll:
    steps:
    - name: pull-request-set-status
      image: github.com/tektoncd/pipeline/cmd/pullrequest-init
      command: 
      - /ko-app/pullrequest-init
      env:
      - name: GITHUBTOKEN
        valueFrom:
          secretKeyRef:
            name: $(params.secretName)
            key: $(params.secretKey)
      args:
      - -url
      - $(params.pullRequestUrl)
      - -path
      - /workspace2
      - -mode
      - download
  afterAll:
    steps:
    - name: pull-request-set-status
      image: github.com/tektoncd/pipeline/cmd/pullrequest-init
      command: 
      - /ko-app/pullrequest-init
      env:
      - name: GITHUBTOKEN
        valueFrom:
          secretKeyRef:
            name: $(params.secretName)
            key: $(params.secretKey)
      args:
      - -url
      - $(params.pullRequestUrl)
      - -path
      - /workspace2
      - -mode
      - upload
  tasks:
  # This task will clone the repository to some volume that will be reused by the next steps
  - name: git-clone-repo
    taskRef:
      name: git-clone
    params:
    - name: repositoryUrl
      value: $(params.repositoryUrl)
    - name: revision
      value: $(params.revision)
  - name: build-repo
    runAfter:
    - git-clone-repo
    taskRef:
      name: go-build-repo
  - name: lint-repo
    runAfter:
    - git-clone-repo
    taskRef:
      name: go-lint-repo
  - name: test-repo
    runAfter:
    - build-repo
    taskRef:
      name: go-test-repo

To add an additional "notification" step, you would just have to add another afterAll step, where this could be something like a slack notification. While making the above, I recognize that the status and some other meta data will likely have to be fed into these injected steps. Also as mentioned in the community call, we would want to ensure the start of execution for these hooks whether the underlying TaskRun failed or not (in the case that the steps are injected rather than being in their own TaskRun).

What this does is allows the Task to be extremely simplistic, which would make generating Pipelines easier. For example, the user might only write a script and that would get automatically converted into a Task; this Task could be injected into the definition of a Pipeline (maybe internal to a/the DSL?). The go-lint-repo Task could follow this paradigm and would look something like the following:

apiVersion: tekton.dev/v1alpha1
kind: Task
metadata:
  name: go-lint-repo
spec:
  volumes:
  - name: clone-dir
    persistentVolumeClaim:
      claimName: host-path-pvc
  steps:
  - image: golang # contains bash
    script: |
      #!/usr/bin/env bash
      go build /repo
    volumeMounts:
    - mountPath: /repo
      name: clone-dir

The above example takes the implementation/approach of making unique TaskRun rather than injecting step (the reason for the volume sharing) for hooks. This has a nice feature of not having to provide special "finally" rules internally since the TaskRun will be created indefinitely (likely accomplished with an automatic run after on each step when handled by the reconciler). If done through step injections to the defined Task, behavior similar to the GitResource could be obtained where the above volume sharing wouldn't be necessary, but this would clone the repository for each Task and for larger repositories this may be more overhead. However, volume sharing probably has similar issues.

[vtereso]

@imjasonh Since #1438 is still ongoing, I didn't want to get too into the weeds with the volume stuff in any particular way. The pull request hooks aren't really providing much value/information in this example, but conceptually, each TaskRun in this example would be responsible for its own status on a pull request. I don't know exactly how the GitHub API works, but I imagine this could be modified however would be necessary for this intended result.

[vtereso]

With consideration for individual TaskRun being injected in place this proposed before/after field, this could also be satisfied with a field like beforeTask/afterTask where they would also be recyclable. The latter implementation for injecting steps into the specified Task is similar to the idea for a Step resource (as others have mentioned before) where this could also be accomplished with a merge operation on Task themselves (e.g. Task with a single step).

Yet another alternative could be for Task to expose an injection point within there definition so that code can be placed inside rather than surrounding (again similar Step or Task merge behavior). For instance, a Task could look as follows:

apiVersion: tekton.dev/v1alpha1
kind: Task
metadata:
  name: slack-notify
spec:
  steps:
  - $(injection)
  - image: slack-image # contains bash
    script: |
      #!/usr/bin/env bash
      # slack stuff

One issue here is that this Task isn't well defined standalone. Also, I don't believe this is as flexible as it forces Task authors to consider injection points and could get messy, but thought it worth mentioning.

[vtereso]

@afrittoli What are your thoughts on this? I saw the notifications design document, but not sure the direction this work has gone on in.

[vdemeester]

/kind feature

[bobcatfish]

We've also talked about emitting cloudevents throughout execution (cant find an issue about it tho!!) which might be one way to implement this

I've opened #2082 to track emitting events during execution, that feels to me like it would be enough to resolve this issue, but before I close it, what do other folks think?

[ghost]

Yeah agreed - I think we've decomposed this larger idea into a number of smaller issues including the new event emitting one. I think it's safe to close.

[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.