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

Audit and document "paths" in Tekton Pipelines API #1030

Closed
bobcatfish opened this issue Jun 27, 2019 · 5 comments · Fixed by #2383
Closed

Audit and document "paths" in Tekton Pipelines API #1030

bobcatfish opened this issue Jun 27, 2019 · 5 comments · Fixed by #2383
Assignees
Labels
help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines.

Comments

@bobcatfish
Copy link
Collaborator

bobcatfish commented Jun 27, 2019

Expected Behavior

  1. It should be clear to users of Tekton Pipelines what special directories will be available to executing containers, what the contents of those directories are, and what the consequences are of editing them.
  2. It should be clear to contributors as well, and all directories should be defined in one location in the code

Actual Behavior

There are a number of special directories, but I'm not sure how many or how they relate to each other. There is no one place in the docs to find this info, nor one place in the code (paths like /builder and /workspace are often hardcoded directly).

Additional Info

  • These directories should be considered part of the Tekton Pipelines API (https://github.com/tektoncd/pipeline/blob/master/api_compatibility_policy.md)
  • We may want to consider randomizing some of these dir names (e.g. in case someone tries to use a container that has a /workspace already)
  • While doing this investigation we may discover that we want to actually change some of these paths
@bobcatfish bobcatfish added help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. maybe-next-milestone For consideration when planning the next milestone labels Jun 27, 2019
@houshengbo
Copy link

/assign @houshengbo

@houshengbo
Copy link

houshengbo commented Jun 28, 2019

@bobcatfish
Thanks for raising this issue. I have been thinking of the special folders and still got baffled all the time. I used to submit a PR to shield the folder names /workspace/source from users for kaniko: tektoncd/catalog#32.

I actually propose the idea that we keep these special folders as internal as possible, so that user do not need to care about them UNLESS they have to.

With that being implemented, instead of using paths like, /workspace/user_path/something, or /builder/user_path/something, etc, users can directly apply user_path/something. For example, if the github repo is taken as the source, we can directly use the path under the home dir of the github repo, like github_path, instead of /workspace/hardcode_source_name/github_path. Whether /workspace is randomized or not does not really matter any more.

IMO, there are really many benefits, if we can shield these special folders(as many as possible): 1. We surely simplifies how we use Tekton. Folks only care the path, they need to care. 2. Since all the hardcoded path or folders are shielded away from users, we do not have to document these special folders as well. It is a simplification of reading our docs.

Does it make sense?

@bobcatfish
Copy link
Collaborator Author

That's an interesting idea @houshengbo !

I think there are kind of 4 main use cases (maybe some others I'm not thinking of):

  1. Paths to PipelineResources, i.e. inputs and outputs, which have always been at /workspace
  2. Paths to implementation details that users (hopefully) shouldn't have to know about but which are important to ppl working on Tekton Pipelines, e.g. /builder/tools/entrypoint which is the path to the entrypoint binary
  3. The home directory of the node is made available in /builder and is common between steps
  4. Users provide data back to the controller, e.g. the path to a built image digest defaults to /builder/home/image-outputs/{resource-name}

I think what you are proposing would address (1) by making it so that path to resources is always /<resource name>/? (I think (2), (3) and (4) are unrelated?) I think the biggest downside here is that it makes it way more likely that the paths to resources will conflict with paths on the node or in the executing container. E.g. if you called a resource etc you would be conflicting with /etc on disk. Knowing that all resources will predictably be in /workspace reduces the potential conflicts significantly (and if we added randomization onto that, e.g. /workspace-<some-guid>/ then the chances would be almost 0).

@bobcatfish bobcatfish added this to the Pipelines 0.7 🐱 milestone Aug 12, 2019
@chhsia0
Copy link
Contributor

chhsia0 commented Aug 23, 2019

A small issue related to name conflicts: right now weird things might happen if we have an input resource named output and another output-only resource ;)

@bobcatfish bobcatfish removed the maybe-next-milestone For consideration when planning the next milestone label Sep 6, 2019
@skaegi
Copy link
Contributor

skaegi commented Oct 8, 2019

I think we might consider simplifying things for the stuff we get for free, and should either go all-in on "workspace" as a concept or not at all. I get the idea of targetPath customization for input and output resources but it really dilutes the value of the "workspace". Setting HOME to /builder is confusing but perhaps I could be educated here.

Anyway...
If "all-in" for /workspace we would for the task containerspec:

  1. Create a /workspace volumeMount pointing at a shared randomly named emptyDir volume
  2. Set HOME to /workspace
  3. Set workingDir to /workspace
  4. Always map inputs to /workspace/input/{input-name} and output to /workspace/output/{output-name}

[This is basically what we have today with a change to HOME and no targetPath resource support]

OR

  1. Do not create a workspace volume (A user can do this manually)
  2. Do not set HOME (Again can be done manually)
  3. Do not set workingDir (Again can be done manually)
  4. A user MUST provide a mountPath for all input and output

[So efficient... but takes away some of the value of Tekton convention]

@bobcatfish bobcatfish assigned bobcatfish and unassigned houshengbo Apr 13, 2020
bobcatfish added a commit to bobcatfish/pipeline that referenced this issue Apr 13, 2020
Update our API compatibility policy as discussed and outlined in the
beta policy and plan doc (https://docs.google.com/document/d/1H8I2Rk4kLdQaR4mV0A71Qbk-1FxXFrmvisEAjLKT6H0/edit#
which is visible to members of
[the tekton-dev mailing
list](https://github.com/tektoncd/community/blob/master/contact.md#mailing-list)).

As a bonus, since we consider the directories used by Tekton as part of
our API, this also fixes tektoncd#1030.
bobcatfish added a commit to bobcatfish/pipeline that referenced this issue Apr 13, 2020
Update our API compatibility policy as discussed and outlined in the
beta policy and plan doc (https://docs.google.com/document/d/1H8I2Rk4kLdQaR4mV0A71Qbk-1FxXFrmvisEAjLKT6H0/edit#
which is visible to members of
[the tekton-dev mailing
list](https://github.com/tektoncd/community/blob/master/contact.md#mailing-list)).

As a bonus, since we consider the directories used by Tekton as part of
our API, this also fixes tektoncd#1030.
bobcatfish added a commit to bobcatfish/pipeline that referenced this issue Apr 17, 2020
Update our API compatibility policy as discussed and outlined in the
beta policy and plan doc (https://docs.google.com/document/d/1H8I2Rk4kLdQaR4mV0A71Qbk-1FxXFrmvisEAjLKT6H0/edit#
which is visible to members of
[the tekton-dev mailing
list](https://github.com/tektoncd/community/blob/master/contact.md#mailing-list)).

As a bonus, since we consider the directories used by Tekton as part of
our API, this also fixes tektoncd#1030.
tekton-robot pushed a commit that referenced this issue Apr 20, 2020
Update our API compatibility policy as discussed and outlined in the
beta policy and plan doc (https://docs.google.com/document/d/1H8I2Rk4kLdQaR4mV0A71Qbk-1FxXFrmvisEAjLKT6H0/edit#
which is visible to members of
[the tekton-dev mailing
list](https://github.com/tektoncd/community/blob/master/contact.md#mailing-list)).

As a bonus, since we consider the directories used by Tekton as part of
our API, this also fixes #1030.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants