Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TEP-0063: Workspace Dependencies #446

Closed
wants to merge 1 commit into from

Conversation

jerop
Copy link
Member

@jerop jerop commented Jun 3, 2021

In #413, we added the problem statement for TEP-0063: Workspace Dependencies that discusses the need to give users a way to specify resource dependencies based on Workspaces. That will ensure that failure and skipping strategies for common CI/CD use cases work and users don't get unexpected failures.

In this change, we add the proposal and discuss the alternatives for solving that problem. To enable users to specify resource dependencies based on Workspaces, we will provide a field - useAfter - that can be used to specify that a given Task should use a specific Workspace after another Task has already used it. It will be used to construct the Directed Acyclic Graph that represents the Pipeline to enforce the execution order.

/kind tep

@tekton-robot tekton-robot added the kind/tep Categorizes issue or PR as related to a TEP (or needs a TEP). label Jun 3, 2021
@tekton-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign jerop
You can assign the PR to them by writing /assign @jerop in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@tekton-robot tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 3, 2021
@ghost
Copy link

ghost commented Jun 7, 2021

👍 this looks great from my pov. thanks @jerop !

/lgtm

@tekton-robot tekton-robot assigned ghost Jun 7, 2021
@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 7, 2021
@afrittoli
Copy link
Member

/cc @pritidesai

@afrittoli
Copy link
Member

/assign @pritidesai

@nikhil-thomas
Copy link
Member

/assign @nikhil-thomas

@tekton-robot tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 14, 2021
@pritidesai
Copy link
Member

@pritidesai needs review

In tektoncd#413, we added the problem
statement for [TEP-0063: Workspace Dependencies](https://github.com/tektoncd/community/blob/main/teps/0063-workspace-dependencies.md)
that discusses the need to give users a way to specify resource
dependencies based on `Workspaces`. That will ensure that failure and
skipping strategies for common CI/CD use cases work and users don't get
unexpected failures.

In this change, we add the proposal and discuss the alternatives for
solving that problem. To enable users to specify resource dependencies
based on `Workspaces`, we will provide a field - `useAfter` - that can
be used to specify that a given `Task` should use a specific `Workspace`
after another `Task` has already used it. It will be used to construct
the `Directed Acyclic Graph` that represents the `Pipeline` to enforce
the execution order.
@tekton-robot
Copy link
Contributor

New changes are detected. LGTM label has been removed.

@tekton-robot tekton-robot removed lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jun 22, 2021
@pritidesai
Copy link
Member

pritidesai commented Jul 1, 2021

Today, if the WhenExpressions in manual-approval evaluate to False, then manual-approval, build-image,
deploy-image and slack-msg are all skipped. In TEP-0059 Skip Guarded Task Only,
we'll provide the flexibility to unblock the execution of build-image, deploy-image and slack-msg when
manual-approval is skipped. This would allow the user to reuse the Pipeline in both scenarios (merging and not merging).

If the approver resource is a Result, we can identify it as a resource dependency and skip slack-msg. However, if the
approver resource is passed through a Workspace, we can't identify that as a resource dependency. So we'll execute
slack-msg which would fail because it can't resolve missing resources. When slack-msg fails, the whole Pipeline
would fail.

Sorry @jerop for going back to the use cases added in the initial PR.

I think these two statements contradict each other. The first para says TEP-0059 unblocks the execution of the slack-msg when manual-approval is skipped. slack-msg is a resource dependent through a task result on manual-approval. My understanding is at least the slack-msg is scheduled and attempted, the result references are resolved and then marked as skipped if the result resolution is not successful without creating a taskRun.

In the second para, we are saying the resource dependency is discovered and will skip slack-msg.

Copy link
Member

@afrittoli afrittoli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this @jerop !

I think I understand the problem, however I don't see what a pipeline author could express through the proposed syntax that cannot already be expressed with the existing runAfter?

The reason for this is that the dependency still has to be expressed as a list of tasks as opposed to a list of resources produced by these tasks.

We lack in Tekton today the ability to express such resources though.
An approximation could be folders or files in a workspace?

@bobcatfish
Copy link
Contributor

The reason for this is that the dependency still has to be expressed as a list of tasks as opposed to a list of resources produced by these tasks.

I'm not totally following @afrittoli - I think the proposed syntax would contain both? e.g.

      workspaces:
        - name: input
          workspace: shared-data
          useAfter: 
          - task1

This would be saying that the resource shared-data should be "used after" task1 which implies that this task expects something task1 to do something to shared-data (which could be mutating it, or even could be something like verifying the content).

With today's runAfter syntax, users can express ordering but nothing more - i.e. they can't express when a task depends on another task doing something with a workspace (i.e. a "resource dependency" vs an "ordering dependency")

Copy link
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

      workspaces:
        - name: input
          workspace: shared-data
          useAfter: 
          - task1

This would be saying that the resource shared-data should be "used after" task1 which implies that this task expects something task1 to do something to shared-data (which could be mutating it, or even could be something like verifying the content).

With today's runAfter syntax, users can express ordering but nothing more - i.e. they can't express when a task depends on another task doing something with a workspace (i.e. a "resource dependency" vs an "ordering dependency")

How "important" is that to express when a task depends on another task doing something with a workspace ? What I mean is, as proposed, nothing ensure/validates that the previous task has done something with the workspace, very similar to runAfter. What could be done is validating that the workspace used in task B is also bound to task A (but I don't see it described in the TEP). The Workspace TEP was a bit more involved n that.

Comment on lines +372 to +375
By enabling users to specify resource dependencies based on `Workspaces`, we prevent unexpected failures when we
roll out skipping and failure strategies for common CI/CD use cases. Without this, users would have needed to build
workarounds in their `Pipelines` to address those failures thus making them less reusable. Hence, this proposal
maintains the reusability of Tekton `Pipelines`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we show/describe what those workarounds are and how they reduce the re-usability ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/tep Categorizes issue or PR as related to a TEP (or needs a TEP). size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
Status: Proposed
Development

Successfully merging this pull request may close these issues.

7 participants