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 0030 Workspace Paths Technical Proposal #285

Closed
wants to merge 1 commit into from
Closed

TEP 0030 Workspace Paths Technical Proposal #285

wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Dec 1, 2020

This commit adds a technical proposal to solve the problem statement laid out in
TEP 0030.

Workspace Paths allow a Task author to name specific important paths that they
require be provided on a Workspace or that they promise to produce onto a
Workspace. These paths are validated by Tekton to ensure that they exist before
or after Steps execute.

@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 sbwsg
You can assign the PR to them by writing /assign @sbwsg 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 Dec 1, 2020
@vdemeester vdemeester added the kind/tep Categorizes issue or PR as related to a TEP (or needs a TEP). label Dec 3, 2020
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.

Thanks for this feature, I think it will be a great addition to Tekton.

I share the concern about task authors specifying too many paths, but as you mentioned I think that's something we can address with good examples and blueprint tasks for people to use. Also it may be worth referring to the work that will be needed (not part of this TEP) to update several tasks in the catalog to use paths.

Another concern that I have is that declaring paths may work in future against usability of tasks outside of a pipeline. This is something that can be addressed when working on features like step injection, by allowing pre-steps to fulfil the required paths before they're validated (are they validated?) @jerop fyi.

In my understanding, using a variable that refers to a path produced by a task, must introduce a DAG dependency between the tasks. Is this correct? Either case, I think this should be called out.

I missed details about when validation will happen and how it will be treated. Will the pipeline run controller verify that a path exists on a workspace if a pipeline task declared it? What happens in case of failure?

path: $(tasks.clone.workspaces.output.paths.produced.Dockerfile.path)
```

The benefit here is that Tekton will validate that the `clone` Task did in fact produce the
Copy link
Member

Choose a reason for hiding this comment

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

+1
I think an extra benefit is that this allows adding some task path support when using tasks that do not yet use paths

Copy link
Author

Choose a reason for hiding this comment

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

Excellent point, I've mentioned this as an additional benefit. Thank you!

**Con**:
- No longer follow the nesting of the JSON/YAML

### Alternative Design: Path Params & Results
Copy link
Member

Choose a reason for hiding this comment

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

I don't dislike this option, because it open the doors for paths not being satisfied by a workspace necessarily... but I still like the main proposal better.
In combination with some form of pre-step injection it would work great, i.e. in case of a git repo, we could have multiple versions of steps that satisfy the declared paths in different ways, e.g. by clone a repo but also by extracting a tarball of sources or else. In case of a TaskRun the workspace could be a local emptyDir making it possible to benefit from paths already at TaskRun level.

Copy link
Author

Choose a reason for hiding this comment

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

I've added this as an additional Pro in the list.

@ghost
Copy link
Author

ghost commented Dec 4, 2020

it may be worth referring to the work that will be needed (not part of this TEP) to update several tasks in the catalog to use paths.

Good point, I have added a "Follow-On Work" section to mention updates to the catalog tasks.

Another concern that I have is that declaring paths may work in future against usability of tasks outside of a pipeline. This is something that can be addressed when working on features like step injection, by allowing pre-steps to fulfil the required paths before they're validated (are they validated?) @jerop fyi.

This is a very interesting point and shares something in common with the Workspace isolation described in TEP 29. When Workspaces are isolated to certain steps - either through explicit declaration or because an injected step doesn't know about them - we need to ensure that Path validation only occurs for those parts of the Task that utilize them.

Psuedo YAML combining Step Workspaces with Workspace Paths:

kind: Task
workspaces:
- name: git-credentials
  paths:
    expected:
    - name: private-key
- name: repo-checkout
steps:
- name: validate-something
- name: checkout-repo
  workspaces:
  - name: git-credentials
  - name: repo-checkout
- name: send-message

Here ^ only the checkout-repo step should perform any workspace validation.

In my understanding, using a variable that refers to a path produced by a task, must introduce a DAG dependency between the tasks. Is this correct? Either case, I think this should be called out.

Yes, you're right. Added to the TEP.

I missed details about when validation will happen and how it will be treated. Will the pipeline run controller verify that a path exists on a workspace if a pipeline task declared it? What happens in case of failure?

Aha, I had mentioned the specific implementation details in the Design Details section but they were quite slim. I've filled them out some more to answer these questions. In summary:

  • All of the validation will happen in the Entrypoint binary. When paths are expected, the entrypoint binary must check their existence before running a Step's command. When paths are produced, the entrypoint binary checks their existence after running the Step's command.
  • Workspace validation failure results in failure of the TaskRun and therefore also failure of the PipelineRun.

@ghost
Copy link
Author

ghost commented Dec 4, 2020

I've written up another alternative design which I called "Step Paths". It's interesting but I think too brittle? There's yet another alternative where the paths could be declared at the top level of a Task Spec, independent of any Workspace. At this point I'm just shuffling YAML around though - the nuts and bolts of the design remain largely the same.

This commit adds a technical proposal to solve the problem statement laid out in
TEP 0030.

Workspace Paths allow a Task author to name specific important paths that they
require be provided on a Workspace or that they promise to produce onto a
Workspace. These paths are validated by Tekton to ensure that they exist before
or after Steps execute.
@ghost
Copy link
Author

ghost commented Jan 6, 2021

I'm going to close this PR for the time being. My focus is on getting the Credentials UX issue into a completed state. Between this and Step / Sidecar Workspaces my hunch is that this one is less important for that issue.

I'll leave the branch intact so that we can reopen this as and when a more pressing need presents itself.

@ghost ghost closed this Jan 6, 2021
This pull request was closed.
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
None yet
Development

Successfully merging this pull request may close these issues.

3 participants