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

Enforce task ordering in pipeline when they share PVC workspace #1878

Closed
ghost opened this issue Jan 15, 2020 · 13 comments
Closed

Enforce task ordering in pipeline when they share PVC workspace #1878

ghost opened this issue Jan 15, 2020 · 13 comments
Assignees
Labels
kind/feature Categorizes issue or PR as related to a new feature.

Comments

@ghost
Copy link

ghost commented Jan 15, 2020

Expected Behavior

In #1802 we introduced workspaces to pipelines. One problem that arises when using a PVC workspace across multiple tasks is that multiple task pods may attempt to mount the PVC for writing at once. This is problematic - pipelines can appear to hang for no reason.

So the proposal is to introduce a "from" syntax for workspaces. A PipelineTask claiming a workspace "from" another PipelineTask would cause Tekton to order the tasks one after the other. This would allow a PVC workspace to be injected into the first task and the second to be prevented from running until the first is finished working with the PVC.

One other alternative here would be to enforce a rule that is specific to PVC workspaces - if multiple tasks rely on that workspace then they will be executed in the order that they appear in the PipelineTask list and will not be allowed to run simultaneously. However this doesn't allow for situations where multiple tasks may simply want to read the contents of the PVC which can be allowed.

Actual Behavior

Pipeline tasks that share a PVC workspace currently have to explicitly declare ordering with a "runAfter" section of their yaml or they may hang.

@ghost ghost added the kind/feature Categorizes issue or PR as related to a new feature. label Jan 15, 2020
@vdemeester vdemeester added this to the Pipelines 1.0/beta 🐱 milestone Jan 16, 2020
@ghost
Copy link
Author

ghost commented Jan 24, 2020

I've attempted a different design than either of those mentioned in the issue description.

In the WIP PR I've opened a pipeline author uses variable substitution to declare that one task's workspace is dependent on another's:

  tasks:
    - name: task1
      taskRef:
        name: write-to-workspace
      workspaces:
        - name: output
          workspace: pipeline-ws1
    - name: task2
      taskRef:
        name: read-from-workspace
      workspaces:
        - name: src
          workspace: $(task1.output)

@pritidesai
Copy link
Member

$(task1.workspaces.output) ?

@ghost
Copy link
Author

ghost commented Feb 7, 2020

Update here: we closed #1936 with a view to picking it up again at some point in the future.

For whenever we pick this up again: This issue doesn't mandate that we should continue to use the "from" syntax design. We can introduce any other syntax we want if the feature becomes necessary. We could potentially use variable substitution here but it's unclear precisely which value would be substituted into which other field. However the nice thing about using a variable would be that it has parity with the task results linking that is being worked on.

@ghost
Copy link
Author

ghost commented Feb 10, 2020

So after some more conversation around the design of "from" we've decided that we still do want to be able to express a link between one task's workspace and another task consuming that same workspace. We think there might be a good way to use variable interpolation to express that as well.

@bobcatfish
Copy link
Collaborator

WIP #1936

@bobcatfish bobcatfish modified the milestones: Pipelines 1.0/beta 🐱, Pipelines 1.1 / Post-beta 🐱 Mar 16, 2020
@afrittoli afrittoli removed this from the Pipelines Post-beta 🐱 milestone May 4, 2020
@jlpettersson
Copy link
Member

#2630 introduces a way so that Tasks can use Workspaces in parallel, or in sequence (using runAfter:)

@tekton-robot
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Collaborator

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

@tekton-robot tekton-robot added the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Aug 14, 2020
@bobcatfish
Copy link
Collaborator

/remove-lifecycle rotten
/assign

i still want this functionality - the work that @jerop has been doing around conditions has highlighted how important it is to be able to express the difference between ordering (via runafter) and resource dependencies like this

@bobcatfish bobcatfish reopened this Aug 14, 2020
@tekton-robot tekton-robot removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Aug 14, 2020
@jlpettersson
Copy link
Member

i still want this functionality

@bobcatfish why? I find this case invalid now.

One problem that arises when using a PVC workspace across multiple tasks is that multiple task pods may attempt to mount the PVC for writing at once. This is problematic - pipelines can appear to hang for no reason.

This is no longer true. The problem was also described in #2586
And solved in #2630 - when adding the affinity assistant - so that tasks can access the workspace concurrently.

alternative ... if multiple tasks rely on that workspace then they will be executed in the order that they appear in the PipelineTask list and will not be allowed to run simultaneously.

This is contradictory to #2586 but if the behaviour is wanted, it can be done by explicit runAfter:

@bobcatfish
Copy link
Collaborator

@bobcatfish why? I find this case invalid now.

In features like conditions we distinguish between 1) ordering (runAfter) and 2) resource dependencies (when a Task relies on a result or a PipelineResource from another Task). Depending on a workspace that has been acted on by a previous Task is a case of (2) but since workspaces do not have a "from" or similar syntax we have no way of expressing (2) and it will always look like (1).

One problem that arises when using a PVC workspace across multiple tasks is that multiple task pods may attempt to mount the PVC for writing at once. This is problematic - pipelines can appear to hang for no reason.

This problem is different from the one that I want "from" syntax to solve, so I don't mind opening another issue instead of this one.

@bobcatfish
Copy link
Collaborator

created #3109

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature.
Projects
None yet
Development

No branches or pull requests

6 participants